[ac_range_check,dv] Included logging support in testbench #27645

LouisTheLuis · 2025-07-16T20:20:24Z

This commit includes covergroups for logging registers as well as introducing logging in the smoke sequence.

martin-velay

Some comments are minors but some others will require some changes in order to make this DV robust in case of reuse in other context (refers to the hard coded values) and also to ensure proper checking (refers to illegal_bins).

But, this PR looks great in general!

martin-velay · 2025-07-18T12:55:51Z

hw/ip_templates/ac_range_check/dv/env/ac_range_check_env_cov.sv

+    log_enable_X_log_written_X_deny_th_X_cnt_reached_X_intr_state : 
+      cross log_enable_cp, log_written_cp, deny_th_cp, cnt_reached_cp, intr_state_cp
+    {
+      // If logging is globally disabled, then all the fields should be empty


We had recent talks at lowRISC about the usage of illegal_bins. The conclusion was that we shouldn't use them as a checker.
The issues are:

Your check will be exercised only when the coverage will be enabled and only when your covergroup will be sampled.

Secondly, all the simulator don't react the same way, your test could be marked as passed even if it actually encountered some illegal bins.

In this comment, you'll find alternatives.

Mm, the illegal bins are not really used as a checker in this case. The purpose has been to "get rid" of the coverage crosses that do not count to full coverage (cases that are not to be seen); that way they are not seen in the coverage reports and we can get cleaner results. For a sampled illegal bin, it will trigger a simulation failure (similar to an assertion)!

I think this has also been discussed previously on #27332

For a sampled illegal bin, it will trigger a simulation failure (similar to an assertion)!

This is true for VCS but not for Xcelium! See #27226 (comment).

If you just want to ignore these values from your coverage, you should probably use ignore_bins.

If you want to be sure that these values never happen, in that case please ensure to have an associated checker as described in this example DV style guide: Do not use illegal_bins as a checker style-guides#88 (comment)

martin-velay · 2025-07-18T13:00:47Z

hw/ip_templates/ac_range_check/dv/env/ac_range_check_scoreboard.sv

@@ -268,14 +272,33 @@ task ac_range_check_scoreboard::process_tl_access(tl_seq_item item,
  //  - for read, update prediction at AChanRead phase and compare at DChanRead phase
  case (csr_name)
    // Add individual case item for each csr
+    // TODO: Coverage sampling for interrupts should be moved to the CIP
+    // instead of it being performed in the scoreboard / predictor. Why it is
+    // implemented in such a manner must be examined.


That's a good question. I don't have the answer, but maybe it's because some IPs have to deal with it in special ways?
@rswarbrick, do you know?

martin-velay · 2025-07-18T13:07:46Z

hw/ip_templates/ac_range_check/dv/env/ac_range_check_scoreboard.sv

+          cov.intr_pins_cg.sample(.intr_pin(0), 
+                                  .intr_pin_value(cfg.intr_vif.sample()));
+        end
+      end
    end
    "intr_enable": begin
      // FIXME TODO MVy
    end
    "intr_test": begin


Details

FYI, when you'll test the interrupts at port level, that's the way I've done it for the HMAC. It might not the best approach but I share it in case it may help:

opentitan/hw/ip/hmac/dv/env/hmac_scoreboard.sv

Line 730 in 823d45f

// Check interrupt pins in test mode

martin-velay · 2025-07-18T13:11:05Z

hw/ip_templates/ac_range_check/dv/env/ac_range_check_predictor.sv

@@ -23,6 +23,8 @@ class ac_range_check_predictor extends uvm_component;
  int all_unfilt_a_chan_cnt;          // Total number of received transactions on unfilt A channel
  int exp_unfilt_d_chan_cnt;
  int exp_filt_a_chan_cnt;
+  bit [7:0] deny_cnt;


I think, It'd be better to use directly the parameter DenyCountWidth from the ac_range_check_reg_pkg

I am getting some issues when I try to import ac_range_check_reg_pkg to ac_range_check_predictor using dvsim and VCS:

Error-[SV-LCM-PND] Package not defined src/lowrisc_darjeeling_dv_ac_range_check_env_0.1/seq_lib/ac_range_check_smoke_vseq.sv, 6 ac_range_check_env_pkg, "ac_range_check_reg_pkg::" Package scope resolution failed. Token 'ac_range_check_reg_pkg' is not a package. Originating module 'ac_range_check_env_pkg'. Move package definition before the use of the package.

Apparently, VCS and dvsim parse the testbench files before they parse the ones in the rtl directory. Is there a better solution for this?

You can point to the ac_range_check_ral_pkg.

From the predictor you can do something like:
env_cfg.ral.log_status.deny_cnt.get_n_bits()

martin-velay · 2025-07-18T13:22:17Z

hw/ip_templates/ac_range_check/dv/env/ac_range_check_predictor.sv

+      void'(env_cfg.ral.log_address.predict
+          (.value(log_address), .kind(UVM_PREDICT_DIRECT)));
+    end else if (`gmv(range_attr_csr.log_denied_access) == MuBi4True) begin 
+      overflow_flag = (deny_cnt == 255) ? 1'b1 : 1'b0;


I feel it would be better to avoid as much as we can the hard coded values. This deny_cnt may differ from a project to another one.

Suggested change

overflow_flag = (deny_cnt == 255) ? 1'b1 : 1'b0;

overflow_flag = (deny_cnt == (2^DenyCountWidth)-1) ? 1'b1 : 1'b0;

(Same for the line below)

martin-velay · 2025-07-18T13:48:27Z