Aon timer fcov fixes #26167
Aon timer fcov fixes #26167
Conversation
antmarzam
force-pushed
the
aon_timer_fcov_fixes
branch
from
78048c8 to
6b8ec0d
Compare
Signed-off-by: Antonio Martinez Zambrana <[email protected]>
Signed-off-by: Antonio Martinez Zambrana <[email protected]>
WDOG/WKUP coverage tasks have been moved from the "predictions thread" and now can be sampled whenever these registers are affected and not just during WKUP/WDOG count predictions. In addition, ref keyword has been removed from coverage tasks since an event is already a reference and the keyword is not needed. wait_for_sleep fix for when there aren't any count cycles to compute (e.g. when count register is already greater than threshold by the time WDOG is enabled) Signed-off-by: Antonio Martinez Zambrana <[email protected]>
Plus also making the Vseqs reset aware Signed-off-by: Antonio Martinez Zambrana <[email protected]>
antmarzam
force-pushed
the
aon_timer_fcov_fixes
branch
from
6b8ec0d to
75e299b
Compare
| @@ -23,6 +23,10 @@ package aon_timer_env_pkg; | |||
| parameter uint NUM_ALERTS = 1; | |||
| parameter string LIST_OF_ALERTS[] = {"fatal_fault"}; | |||
|
|
|||
| // WKUP/WDOG counters/threshold registers widths respectively | |||
There was a problem hiding this comment.
Grammar nit: A bit of me really wants an apostrophe around here somewhere! But that would end up rather ugly. Maybe "Widths of WKUP/WDOG counters/threshold registers, respectively." ?
| extern task body(); | ||
| endclass : aon_timer_custom_intr_vseq | ||
|
|
||
| function aon_timer_custom_intr_vseq::new (string name=""); |
There was a problem hiding this comment.
I think I'd be tempted to shuffle this to ensure the declarations are in the same order as the definitions.
| bit [1:0] read_intr_state; | ||
| for (int i = 1; i <= num_trans; i++) begin | ||
| // Write random value to the COUNT registers | ||
| // cfg.aon_clk_rst_vif.wait_clks(1); |
| if (!this.randomize()) | ||
| `uvm_fatal(`gfn, "Randomization Failure") |
There was a problem hiding this comment.
I always feel a little uncomfortable when a class method randomises the class instance: it feels a bit like rebuilding the floor we're standing on :-)
Note that we don't want to re-randomise num_trans.
| fork | ||
| begin : iso_fork_1 | ||
| fork | ||
| begin : execute_code | ||
| cfg.aon_clk_rst_vif.wait_clks(1); | ||
| end : execute_code | ||
| begin : detect_reset | ||
| wait (cfg.under_reset); | ||
| end : detect_reset | ||
| join_any | ||
| disable fork; | ||
| end : iso_fork_1 | ||
| join |
There was a problem hiding this comment.
Can't we use wait_clks_or_rst here? I think it would be a lot simpler. (Unless we're worried about precise timing between different resets)
| `uvm_info(`gfn, $sformatf("Setting CTRL registers with enables for WKUP=0x%0x/WDOG=0x%0x", | ||
| wkup_enable, wdog_enable), UVM_DEBUG) | ||
| csr_utils_pkg::csr_wr(ral.wdog_ctrl.enable, wdog_enable); | ||
| if (cfg.under_reset) return; |
There was a problem hiding this comment.
Again, it doesn't really matter but I'd be tempted to drop some of these early returns. The csr_wr / csr_rd calls will exit instantly if we're under reset anyway. (Obviously, we need to keep the last one!)
| if (cfg.under_reset) return; | ||
|
|
||
| `uvm_info(`gfn, "\n\t Waiting for AON Timer to finish (interrupt)", UVM_HIGH) | ||
| // cfg.aon_clk_rst_vif.wait_clks(5); |
There was a problem hiding this comment.
Again, commented code. Probably best to replace the stuff below with wait_clks_or_rst.
Also, sensible to add an early exit just after that. I doubt it will matter, but it becomes a bit more obvious if the loop doesn't go around again.
| parameter WKUP_WIDTH = 64; | ||
| parameter WDOG_WIDTH = 32; | ||
| parameter PRESCALER_WIDTH = 12; |
There was a problem hiding this comment.
To avoid the magic constants, can't we derive these from the register package? I'm thinking like using $bits on a field structure.
| @@ -27,22 +30,22 @@ class aon_timer_env_cov extends cip_base_env_cov #(.CFG_T(aon_timer_env_cfg)); | |||
| bit wkup_cause); | |||
| prescale_cp: coverpoint prescale { | |||
| bins prescale_0 = {0}; | |||
| bins prescale[32] = {[1:$]}; | |||
| bins prescale[32] = {[1: {PRESCALER_WIDTH{1'b1}}-1]}; | |||
There was a problem hiding this comment.
I think this could go into a const like max_wkup?
| bins prescale_max = {'1}; | ||
| } | ||
| bark_thold_cp: coverpoint bark_thold { | ||
| bins bark_0 = {0}; | ||
| bins bark[32] = {[1:$]}; | ||
| bins bark[32] = {[1:{WDOG_WIDTH{1'b1}}-1]}; |
There was a problem hiding this comment.
Can't this just be max_wkup - 1?
This PR tackles the Functional coverage gap to reach to 100%.
A few more constrained tests have been added for easier coverage closure (min/max threshold and intr_test).
Some of the covergroups have been fixed since not all bins were populated due to incorrect range values.
Mid-TL_UL transaction reset have been added as well.
Running a regression with these improvements plus the remaining RTL exclusions from PR #26165 brings CCOV over ~99%.
FCOV should already be at 100% for a single regression run (
-rx 1).There's a couple of missing items for
tb.dut.u_reg.u_wkup_count_hi_cdcand its arbiter below which I'm still working on generating stimulus for.The goal is also to diminish the regression length to allow for faster sign-off and less simulation cycles
This is still WIP!