Skip to content

Commit fa614b7

Browse files
rswarbrickrswarbrick
committed
[adc_ctrl,dv] Put adc_ctrl_filter_cfg inside a proper class
This is inspired by trying to solve randomisation errors. It turns out that randomising values that aren't in a *static* structure causes some exciting problems with "solve before" constraints. In hindsight, I think this change isn't quite needed: I could have bodged something into the existing code. But I think it's probably an improvement anyway (and I've got it working!). One change is to make some of the class variable names a bit clearer. For example, the "cond" field in adc_chn*_filter_ctl_* controls whether the filter matches inside the range or outside of it. I actually guessed wrong at first, but I think this makes a strong argument for making the code more explicit on the DV side. That is now reflected with a variable called "match_outside" (instead of my original guess of "match_inside", which is exactly backwards!) Indeed, I've just checked again and looked at `theory_op_operation.md`. That document *does* mention a field called "cond" and that it controls whether the filter matches inside or outside of the range. It *doesn't* say which... This clearly needs tidying up, but I think that sort of work can be folded into the next proper release. Signed-off-by: Rupert Swarbrick <[email protected]>
1 parent 9db9e3f commit fa614b7

File tree

9 files changed

+207
-116
lines changed

9 files changed

+207
-116
lines changed

‎hw/ip/adc_ctrl/dv/env/adc_ctrl_env.core‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ filesets:
1111
- lowrisc:dv:cip_lib
1212
files:
1313
- adc_ctrl_env_pkg.sv
14+
- adc_ctrl_filter_cfg.sv: {is_include_file: true}
1415
- adc_ctrl_env_cfg.sv: {is_include_file: true}
1516
- adc_ctrl_env_var_filter_cfg.sv : {is_include_file: true}
1617
- adc_ctrl_env_cov.sv: {is_include_file: true}

‎hw/ip/adc_ctrl/dv/env/adc_ctrl_env_cfg.sv‎

Lines changed: 18 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,8 @@ class adc_ctrl_env_cfg extends cip_base_env_cfg #(
3636
rand int pwrup_time;
3737
rand int wakeup_time;
3838

39-
// Filter values filter_cfg[channel][filter]
40-
rand adc_ctrl_filter_cfg_t filter_cfg[][];
39+
// Filter values
40+
rand adc_ctrl_filter_cfg filter_cfg[ADC_CTRL_CHANNELS][ADC_CTRL_NUM_FILTERS];
4141

4242
// Debouncing sample counts for normal and low power modes
4343
rand int np_sample_cnt;
@@ -52,7 +52,12 @@ class adc_ctrl_env_cfg extends cip_base_env_cfg #(
5252
`uvm_field_int(lp_sample_cnt, UVM_DEFAULT)
5353
`uvm_object_utils_end
5454

55-
`uvm_object_new
55+
function new(string name="");
56+
super.new(name);
57+
foreach (filter_cfg[channel, filter]) begin
58+
filter_cfg[channel][filter] = new;
59+
end
60+
endfunction
5661

5762
virtual function void initialize(bit [31:0] csr_base_addr = '1);
5863
list_of_alerts = adc_ctrl_env_pkg::LIST_OF_ALERTS;
@@ -84,17 +89,6 @@ class adc_ctrl_env_cfg extends cip_base_env_cfg #(
8489

8590
endfunction
8691

87-
// Constraints
88-
// Set the size of the filter_cfg array
89-
constraint filter_cfg_size_c {
90-
// Size of first dimension
91-
filter_cfg.size == ADC_CTRL_CHANNELS;
92-
93-
// Size of second dimension
94-
foreach (filter_cfg[channel]) {filter_cfg[channel].size == ADC_CTRL_NUM_FILTERS;}
95-
96-
}
97-
9892
// Valid values
9993
constraint valid_c {
10094
pwrup_time inside {[0 : 2 ** 4 - 1]};
@@ -117,7 +111,10 @@ class adc_ctrl_env_cfg extends cip_base_env_cfg #(
117111
// Default filter configuration
118112
// This is the one assumed for normal use
119113
foreach (filter_cfg[channel, filter]) {
120-
soft filter_cfg[channel][filter] == FILTER_CFG_DEFAULTS[filter];
114+
soft filter_cfg[channel][filter].min_v == FILTER_CFG_DEFAULTS[filter].min_v;
115+
soft filter_cfg[channel][filter].max_v == FILTER_CFG_DEFAULTS[filter].max_v;
116+
soft filter_cfg[channel][filter].match_outside == FILTER_CFG_DEFAULTS[filter].match_outside;
117+
soft filter_cfg[channel][filter].enabled == FILTER_CFG_DEFAULTS[filter].enabled;
121118
}
122119
}
123120

@@ -138,15 +135,12 @@ class adc_ctrl_env_cfg extends cip_base_env_cfg #(
138135
super.do_print(printer);
139136

140137
// Implement filter_cfg - 2d array of structs
141-
printer.print_array_header("filter_cfg", filter_cfg.size);
142-
for (int channel = $low(filter_cfg); channel <= $high(filter_cfg); channel++) begin
143-
printer.print_array_header($sformatf("filter_cfg[%0d]", channel), filter_cfg[channel].size());
144-
for (
145-
int filter = $low(filter_cfg[channel]); filter <= $high(filter_cfg[channel]); filter++
146-
) begin
147-
printer.print_generic($sformatf("filter_cfg[%0d][%0d]", channel, filter),
148-
"adc_ctrl_filter_cfg_t", $bits(filter_cfg[channel][filter]),
149-
$sformatf("%p", filter_cfg[channel][filter]));
138+
printer.print_array_header("filter_cfg", ADC_CTRL_CHANNELS);
139+
for (int channel = 0; channel < ADC_CTRL_CHANNELS; channel++) begin
140+
printer.print_array_header($sformatf("filter_cfg[%0d]", channel), ADC_CTRL_NUM_FILTERS);
141+
for (int filter = 0; filter < ADC_CTRL_NUM_FILTERS; filter++) begin
142+
printer.print_object($sformatf("filter_cfg[%0d][%0d]", channel, filter),
143+
filter_cfg[channel][filter]);
150144
end
151145
printer.print_array_footer();
152146
end

‎hw/ip/adc_ctrl/dv/env/adc_ctrl_env_cov.sv‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,12 +20,12 @@ class adc_ctrl_filter_cg_wrapper #(
2020
covergroup adc_ctrl_filter_cg(
2121
int channel, int filter
2222
) with function sample (
23-
adc_ctrl_filter_cfg_t cfg, bit is_interrupt = 0, bit is_wakeup = 0, bit clk_gate = 0
23+
adc_ctrl_filter_cfg cfg, bit is_interrupt = 0, bit is_wakeup = 0, bit clk_gate = 0
2424
);
2525
option.name = $sformatf("adc_ctrl_filter_cg_%0d_%0d", channel, filter);
2626
option.per_instance = 1;
2727

28-
cond_cp: coverpoint cfg.cond;
28+
match_outside_cp: coverpoint cfg.match_outside;
2929
min_v_cp: coverpoint cfg.min_v {
3030
bins minimum = {0};
3131
bins values[NUMBER_VALUES] = {[1 : (2 ** FILTER_WIDTH - 2)]};
@@ -36,14 +36,14 @@ class adc_ctrl_filter_cg_wrapper #(
3636
bins values[NUMBER_VALUES] = {[1 : (2 ** FILTER_WIDTH - 2)]};
3737
bins maximum = {2 ** FILTER_WIDTH - 1};
3838
}
39-
en_cp: coverpoint cfg.en;
39+
en_cp: coverpoint cfg.enabled;
4040
interrupt_cp: coverpoint is_interrupt;
4141
wakeup_cp: coverpoint is_wakeup;
4242
clk_gate_cp: coverpoint clk_gate;
43-
intr_min_v_cond_xp: cross interrupt_cp, min_v_cp, cond_cp;
44-
intr_max_v_cond_xp: cross interrupt_cp, max_v_cp, cond_cp;
45-
wakeup_min_v_cond_xp: cross wakeup_cp, min_v_cp, cond_cp;
46-
wakeup_max_v_cond_xp: cross wakeup_cp, max_v_cp, cond_cp;
43+
intr_min_v_cond_xp: cross interrupt_cp, min_v_cp, match_outside_cp;
44+
intr_max_v_cond_xp: cross interrupt_cp, max_v_cp, match_outside_cp;
45+
wakeup_min_v_cond_xp: cross wakeup_cp, min_v_cp, match_outside_cp;
46+
wakeup_max_v_cond_xp: cross wakeup_cp, max_v_cp, match_outside_cp;
4747
wakeup_gated_xp : cross wakeup_cp, clk_gate_cp;
4848
endgroup
4949

@@ -96,7 +96,7 @@ class adc_ctrl_env_cov extends cip_base_env_cov #(
9696
endfunction : build_phase
9797

9898
// Sample filter coverage
99-
virtual function void sample_filter_cov(int channel, int filter, adc_ctrl_filter_cfg_t cfg,
99+
virtual function void sample_filter_cov(int channel, int filter, adc_ctrl_filter_cfg cfg,
100100
bit is_interrupt = 0, bit is_wakeup = 0,
101101
bit clk_gate = 0);
102102
adc_ctrl_filter_cg_wrapper_insts[channel][filter].adc_ctrl_filter_cg.sample(cfg, is_interrupt,

‎hw/ip/adc_ctrl/dv/env/adc_ctrl_env_pkg.sv‎

Lines changed: 11 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -74,39 +74,25 @@ package adc_ctrl_env_pkg;
7474
AdcCtrlResetModeHw
7575
} adc_ctrl_reset_mode_e;
7676

77-
// Filter condition coding
78-
typedef enum bit {
79-
ADC_CTRL_FILTER_COND_IN = 0,
80-
ADC_CTRL_FILTER_COND_OUT = 1
81-
} adc_ctrl_filter_cond_e;
82-
83-
// Filter configuration
84-
typedef struct {
85-
adc_ctrl_filter_cond_e cond; // Condition
86-
int min_v; // Minimum value
87-
int max_v; // Maximum value
88-
bit en; // Enable
89-
} adc_ctrl_filter_cfg_t;
77+
typedef class adc_ctrl_filter_cfg;
9078

9179
// Constants
9280
// Filter defaults - applies to all channels
93-
const
94-
adc_ctrl_filter_cfg_t
95-
FILTER_CFG_DEFAULTS[] = '{
96-
'{min_v: 149, max_v: 279, cond: ADC_CTRL_FILTER_COND_IN, en: 1},
97-
'{min_v: 391, max_v: 524, cond: ADC_CTRL_FILTER_COND_IN, en: 1},
98-
'{min_v: 712, max_v: 931, cond: ADC_CTRL_FILTER_COND_IN, en: 1},
99-
'{min_v: 712, max_v: 847, cond: ADC_CTRL_FILTER_COND_IN, en: 1},
100-
'{min_v: 349, max_v: 512, cond: ADC_CTRL_FILTER_COND_IN, en: 1},
101-
'{min_v: 405, max_v: 503, cond: ADC_CTRL_FILTER_COND_IN, en: 1},
102-
'{min_v: 186, max_v: 279, cond: ADC_CTRL_FILTER_COND_IN, en: 1},
103-
'{min_v: 116, max_v: 954, cond: ADC_CTRL_FILTER_COND_OUT, en: 1}
81+
const adc_ctrl_filter_cfg FILTER_CFG_DEFAULTS[] = '{
82+
adc_ctrl_filter_cfg::make("default0", 149, 279, 1),
83+
adc_ctrl_filter_cfg::make("default1", 391, 524, 1),
84+
adc_ctrl_filter_cfg::make("default2", 712, 931, 1),
85+
adc_ctrl_filter_cfg::make("default3", 712, 847, 1),
86+
adc_ctrl_filter_cfg::make("default4", 349, 512, 1),
87+
adc_ctrl_filter_cfg::make("default5", 405, 503, 1),
88+
adc_ctrl_filter_cfg::make("default6", 186, 279, 1),
89+
adc_ctrl_filter_cfg::make("default7", 116, 954, 0)
10490
};
105-
// functions and tasks
10691

10792
// package sources
10893
`include "adc_ctrl_env_cfg.sv"
10994
`include "adc_ctrl_env_var_filter_cfg.sv"
95+
`include "adc_ctrl_filter_cfg.sv"
11096
`include "adc_ctrl_env_cov.sv"
11197
`include "adc_ctrl_virtual_sequencer.sv"
11298
`include "adc_ctrl_scoreboard.sv"

‎hw/ip/adc_ctrl/dv/env/adc_ctrl_env_var_filter_cfg.sv‎

Lines changed: 56 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -58,44 +58,61 @@ class adc_ctrl_env_var_filters_cfg extends adc_ctrl_env_cfg;
5858
}
5959

6060
constraint filters_values_c {
61-
solve max_val, min_val, apply_max_v before filter_cfg;
62-
foreach (filter_cfg[channel]) {
63-
foreach (filter_cfg[channel, filter]) {
64-
// Set valid values
65-
filter_cfg[channel][filter].min_v inside {[0 : MAX_VALUE]};
66-
filter_cfg[channel][filter].max_v inside {[0 : MAX_VALUE]};
67-
filter_cfg[channel][filter].max_v >= filter_cfg[channel][filter].min_v;
68-
69-
// Set first channel to about 1/8 full range so 3/32 to 5/32
70-
// then make others within 1/64 range of it so there is some overlap
71-
if (channel == 0) {
72-
// Channel 0
73-
// Make this a soft constraint as it can be broken by minimum and maximum values
74-
// If min_v == full range then so must max_v, if max_v == 0 then so must min_v
75-
soft (filter_cfg[channel][filter].max_v - filter_cfg[channel][filter].min_v) inside {
76-
[THREE_THIRTYSECONDTHS:FIVE_THIRTYSECONDTHS]};
77-
// Set maximum/minimum values
78-
if (max_val[filter]) {
79-
if (apply_max_v[filter]) {
80-
filter_cfg[channel][filter].max_v == MAX_VALUE;
81-
} else {
82-
filter_cfg[channel][filter].min_v == MAX_VALUE;
83-
}
61+
foreach (filter_cfg[channel, filter]) {
62+
// Ordering constraints to make sure that the distribution of max_val, min_val, apply_max_v is
63+
// independent of the values for each of the channels / filters.
64+
solve max_val, min_val, apply_max_v before filter_cfg[channel][filter].min_v;
65+
solve max_val, min_val, apply_max_v before filter_cfg[channel][filter].max_v;
66+
67+
// Set first channel to about 1/8 full range so 3/32 to 5/32
68+
// then make others within 1/64 range of it so there is some overlap
69+
if (channel == 0) {
70+
// Channel 0
71+
// Make this a soft constraint as it can be broken by minimum and maximum values
72+
// If min_v == full range then so must max_v, if max_v == 0 then so must min_v
73+
soft (filter_cfg[channel][filter].max_v - filter_cfg[channel][filter].min_v) inside {
74+
[THREE_THIRTYSECONDTHS:FIVE_THIRTYSECONDTHS]};
75+
// Set maximum/minimum values
76+
if (max_val[filter]) {
77+
if (apply_max_v[filter]) {
78+
filter_cfg[channel][filter].max_v == MAX_VALUE;
79+
} else {
80+
filter_cfg[channel][filter].min_v == MAX_VALUE;
8481
}
85-
if (min_val[filter]) {
86-
if (apply_max_v[filter]) {
87-
filter_cfg[channel][filter].max_v == 0;
88-
} else {
89-
filter_cfg[channel][filter].min_v == 0;
90-
}
82+
}
83+
if (min_val[filter]) {
84+
if (apply_max_v[filter]) {
85+
filter_cfg[channel][filter].max_v == 0;
86+
} else {
87+
filter_cfg[channel][filter].min_v == 0;
9188
}
92-
} else {
93-
// Other channels within 1/64
94-
(filter_cfg[channel][filter].min_v - filter_cfg[0][filter].min_v) inside
95-
{[-ONE_SIXTYFOURTH : ONE_SIXTYFOURTH]};
96-
(filter_cfg[channel][filter].max_v - filter_cfg[0][filter].max_v) inside
97-
{[-ONE_SIXTYFOURTH : ONE_SIXTYFOURTH]};
9889
}
90+
} else {
91+
// Other channels within ONE_SIXTYFOURTH of the first channel.
92+
//
93+
// Than taking the difference and claiming it is in a range [-n, n] doesn't work very
94+
// cleanly, because the variables are unsigned. To avoid a problem, we can cheat and convert
95+
// each claim to two one-sided versions.
96+
//
97+
// Instead of saying (a - b) inside [-n:n], recast it (in signed integers) as the pair
98+
//
99+
// a - b <= n
100+
// b - a <= n
101+
//
102+
// Then we can get rid of the possible negative numbers by casting it as this pair of
103+
// equations over the natural numbers:
104+
//
105+
// a <= b + n
106+
// b <= a + n
107+
//
108+
// That will basically work! But it might overflow (and wrap) if a or b is large.
109+
// Practically speaking, this won't matter (because n is an int unsigned), but we can make
110+
// it really clear by adding an extra bit to the LHS each time.
111+
{1'b0, filter_cfg[channel][filter].min_v} <= filter_cfg[0][filter].min_v + ONE_SIXTYFOURTH;
112+
{1'b0, filter_cfg[0][filter].min_v} <= filter_cfg[channel][filter].min_v + ONE_SIXTYFOURTH;
113+
114+
{1'b0, filter_cfg[channel][filter].max_v} <= filter_cfg[0][filter].max_v + ONE_SIXTYFOURTH;
115+
{1'b0, filter_cfg[0][filter].max_v} <= filter_cfg[channel][filter].max_v + ONE_SIXTYFOURTH;
99116
}
100117
}
101118
}
@@ -105,12 +122,10 @@ class adc_ctrl_env_var_filters_cfg extends adc_ctrl_env_cfg;
105122
$onehot(~mirror_controls);
106123
}
107124
constraint filters_control_c {
108-
foreach (filter_cfg[channel]) {
109-
foreach (filter_cfg[channel, filter]) {
110-
if (mirror_controls[filter]) {
111-
filter_cfg[channel][filter].cond == filter_cfg[0][filter].cond;
112-
filter_cfg[channel][filter].en == filter_cfg[0][filter].en;
113-
}
125+
foreach (filter_cfg[channel, filter]) {
126+
if (mirror_controls[filter]) {
127+
filter_cfg[channel][filter].match_outside == filter_cfg[0][filter].match_outside;
128+
filter_cfg[channel][filter].enabled == filter_cfg[0][filter].enabled;
114129
}
115130
}
116131
}

‎hw/ip/adc_ctrl/dv/env/adc_ctrl_filter_cfg.sv‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
// Copyright lowRISC contributors (OpenTitan project).
2+
// Licensed under the Apache License, Version 2.0, see LICENSE for details.
3+
// SPDX-License-Identifier: Apache-2.0
4+
5+
// The configuration for a single filter
6+
7+
class adc_ctrl_filter_cfg extends uvm_object;
8+
// An interval of values (inclusive).
9+
rand bit [ADC_CTRL_DATA_WIDTH-1:0] min_v;
10+
rand bit [ADC_CTRL_DATA_WIDTH-1:0] max_v;
11+
12+
// If match_outside is true, the filter matches a value iff it is not in [min_v, max_v]. If not,
13+
// the filter matches a value iff it is *inside* [min_v, max_v].
14+
rand bit match_outside;
15+
16+
// Control whether the filter is enabled
17+
rand bit enabled;
18+
19+
`uvm_object_utils_begin(adc_ctrl_filter_cfg)
20+
`uvm_field_int(min_v, UVM_DEFAULT)
21+
`uvm_field_int(max_v, UVM_DEFAULT)
22+
`uvm_field_int(match_outside, UVM_DEFAULT)
23+
`uvm_field_int(enabled, UVM_DEFAULT)
24+
`uvm_object_utils_end
25+
26+
// Ensure that min_v <= max_v
27+
extern constraint min_le_max_c;
28+
29+
extern function new (string name="");
30+
31+
// A static factory method that returns a filter with the range [min_v, max_v] and the
32+
// given match_outside / enabled values.
33+
extern static function adc_ctrl_filter_cfg make(string name,
34+
bit [ADC_CTRL_DATA_WIDTH-1:0] min_v,
35+
bit [ADC_CTRL_DATA_WIDTH-1:0] max_v,
36+
bit match_outside,
37+
bit enabled = 1'b1);
38+
endclass
39+
40+
constraint adc_ctrl_filter_cfg::min_le_max_c {
41+
min_v <= max_v;
42+
}
43+
44+
function adc_ctrl_filter_cfg::new (string name="");
45+
super.new(name);
46+
endfunction
47+
48+
function adc_ctrl_filter_cfg
49+
adc_ctrl_filter_cfg::make(string name,
50+
bit [ADC_CTRL_DATA_WIDTH-1:0] min_v,
51+
bit [ADC_CTRL_DATA_WIDTH-1:0] max_v,
52+
bit match_outside,
53+
bit enabled = 1'b1);
54+
adc_ctrl_filter_cfg ret;
55+
56+
if (max_v < min_v) begin
57+
`uvm_fatal("adc_ctrl_filter_cfg::make",
58+
$sformatf("Backwards min_v/max_v range of [%0x, %0x]", min_v, max_v))
59+
end
60+
61+
ret = adc_ctrl_filter_cfg::type_id::create(name);
62+
ret.min_v = min_v;
63+
ret.max_v = max_v;
64+
ret.match_outside = match_outside;
65+
ret.enabled = enabled;
66+
return ret;
67+
endfunction

0 commit comments

Comments
 (0)