Aon timer v3 dv changes

[antmarzam]

Refactor of the TB mainly consisting on making the TB aware of when a given register write makes it to the aon CDC side.

In addition:

• the TB now integrates "timed" registers re-used from USBDEV. In essence, this "timed" registers track the backdoor value and based on the predictions keep a valid prediction until the values kick-in.
• The TB checks the value returned in intr_state reads

[rswarbrick]

This is only a partial review of the last commit, but it seems I've left quite a lot of notes! :-)

[rswarbrick]

I wonder whether it would make sense to move this into the task, possibly by making wkup_intr_predicted_values be essentially this line, calling a worker task.

[antmarzam]

I think it's more clearly understood we don't want the task to block by wrapping it around fork...join_none.
We could create another wrapper task appending *_non_blocking to the end, but I'm not sure we gain much

[rswarbrick]

This isn't very easy to reason about though: the line says something like "spawn off a process that will do something eventually, but not tell you when it's finished". That's a pretty hard structure to understand! I wonder whether there's a different structure where this task can set some flag the modifies some other task that's running permanently or something?

[antmarzam]

I don't think in this case it's important to know when this task finishes. It just pushes a prediction and keeps it in check with the actual value of the RTL.

I don't see the value in having a task running permanently, this task only runs whenever there is an interrupt state prediction, until the point the prediction becomes true in the RTL.

I can add a uvm_info to the fork after either the reset or the predicted value is committed in the RTL if you think that's needed?

[rswarbrick]

I don't think we need the extra info print.

I've re-read your previous message and I hadn't properly understood it before (sorry). Maybe the best thing is to make the task non-blocking (which, looking again, was my initial suggestion). Something like this, maybe?

task aon_timer_scoreboard::wkup_intr_predicted_values(bit exp_wkup_intr);
  static int unsigned last_cycle_count = 0;

  fork begin
    if (last_cycle_count != timed_regs.time_now) begin
      `uvm_info(`gfn, $sformatf("%m - Predicted wkup_intr = 0x%0x", exp_wkup_intr), UVM_DEBUG)
      predicted_wkup_intr_q.push_back(exp_wkup_intr);
      last_cycle_count = timed_regs.time_now;
      fork
        begin: iso_fork
          fork
            begin : wait_values_to_propagate
              // do backdoor read and delete values no longer valid.
              check_intr_value_propagated(WKUP);
            end : wait_values_to_propagate
            begin : reset_kill
              wait (under_reset);
            end : reset_kill
          join_any
          disable fork;
        end : iso_fork
      join
    end
  end
  join_none
endtask : wkup_intr_predicted_values

(and similarly for the other analogous tasks)

[rswarbrick]

Hmm, I don't completely understand this. It seems to be putting details about the scoreboard structure into the base sequence, that doesn't look completely right. What goes wrong if this wait is skipped?

[antmarzam]

Can you elaborate with regards to the scoreboard details?

As far as I can see this doesn't use the scoreboard. It just references the RAL like the rest of the sequence.
In this particular task (run_intr_test_vseq), the whole model and checker is mixed up instead of having a predictor and scoreboard to do the modelling and checking (respectively).

If you don't block for an ongoing prediction to finish you are taking the risk of comparing against an old predicted mirrored value and causing mismatches.

Ideally, we'd want the VSEQ just generating stimulus, and the TB checking the behaviour is correct, but cip_base_vseq already tries to do everything, so I just wanted to ensure by the time we reach the comparison the up to date predicted value is available

[rswarbrick]

Ok, I think I still don't quite understand what's going on here.

I think that the call to wait_for_prediction will block the activity of the virtual sequence. Where does the predicting_value flag get written / cleared?

[antmarzam]

The problem here relates to the aon timer not being "quick enough" in predicting intr_state values.
The "quick enough" refers to when you are trying to predict the value for intr_state but you can't because register.busy()=1 , and predicting a value will fail. However, we still need the prediction to move forward, otherwise the comparison in cip_base_vseq will fail (hence the blocking).

A register is busy when is being accessed.

Predicting value is a responsability of each TB prediction. So typically you do:

predicting_value = 1;
wait(reg_name.busy()==0);
if (!reg_name.predict())
  $fatal("prediction failed")
predicting_value = 0;

In short, unless a TB manually implements a better way of predicting the registers (considering the prediction may block if busy and checking the prediction didn't fail) this change is transparent to any other TBs. Because wait_for_prediction only blocks if predicting_value=1

[rswarbrick]

I'm not sure about this comment: the function isn't checking anything, but returns the predicted value, I think?

[antmarzam]

All of the timed_regs code reuses the code from Adrian adapting it to the aon_timer.
You can see the usb timed regs here with the same comment:

opentitan/hw/ip/usbdev/dv/env/usbdev_timed_regs.sv

Line 74 in 56f2e95

// Check a DUT read from the specified register against any timed expectations.

The read function does check and does not return anything (void), so I don't think the comment is misleading. See how this function calls the specific timed register read function which does the checking:

opentitan/hw/ip/usbdev/dv/env/timed_reg.sv

Lines 80 to 106 in 56f2e95

	function uvm_reg_data_t read(int unsigned time_now, uvm_reg_data_t act_data);
	// Properties of this register field.
	uvm_reg_data_t f_mask = (1 << field.get_n_bits()) - 1;
	int unsigned lsb = field.get_lsb_pos();
	// Predicted value of this register field.
	uvm_reg_data_t pred_val;
	`uvm_info(`gfn, $sformatf(" - field %s : pred valid %d prev 0x%0x new 0x%0x",
	field.get_name(), pred_valid, pred_latest.val_prev,
	pred_latest.val_new), UVM_HIGH)
	if (pred_valid) begin
	uvm_reg_data_t act_val = (act_data >> lsb) & f_mask;
	`uvm_info(`gfn, $sformatf(" (time_now 0x%0x latest_time 0x%0x)", pred_latest.latest_time,
	time_now), UVM_HIGH)
	if (act_val == pred_latest.val_new) begin
	`uvm_info(`gfn, " Prediction met", UVM_HIGH)
	pred_val = pred_latest.val_new;
	// Prediction met; no longer required.
	pred_valid = 1'b0;
	end else begin
	`uvm_info(`gfn, $sformatf(" Prediction not met; act 0x%0x", act_val), UVM_MEDIUM)
	`DV_CHECK_EQ(act_val, pred_latest.val_prev)
	`DV_CHECK_GT(pred_latest.latest_time - time_now, 0)
	pred_val = pred_latest.val_prev;
	end
	// Retain the latest prediction.
	read_latest = pred_val;
	end else begin

I think you authored Adrian's PR which is the same :/

[rswarbrick]

I just chatted to Adrian about this! (because I realised I didn't quite understand). And now I understand a bit more. And I'm convinced that the comment is correct (although the code is maybe a bit more confusing than it needs to be: not sure)

@alees24: Thanks for your help!

[rswarbrick]

If I understand correctly, this is copying modelled values for the various timed registers (stored in intr_status_exp) into the supplied state argument. Maybe the comment could be expanded to say something like this? (I didn't know what "capture" meant here)

[antmarzam]

I've added this:

// The argument passed by reference `state` stores the value predicted for intr_state for each
// type of timer

[rswarbrick]

Thanks, that makes sense. Could you expand things slightly further to explain what the three functions do (and how they are different)?

[antmarzam]

I'm a bit confused by how github displays the diff. The capture_* tasks have been replaced by capture_timed_regs_independently

P.S. I did accidentally leave a reference to capture_timed_regs_wkup at the top in the declaration. Will fix with my next push!

[rswarbrick]

Just a partial review (sorry: I've got to run off and do something else), but here's something, at least.

[rswarbrick]

Tiny nit: Double space before "this"

[antmarzam]

Addressed

[rswarbrick]

Not a big deal, but would something like this be simpler?

  bit actual_value = hdl_read_bit(path);
  while (actual_value != value) begin
    cfg.aon_clk_rst_vif.wait_clks(1);
    hdl_read_bit(path);
  end

Note that we don't need case equality: the type is bit rather than logic.

[antmarzam]

I don't mind either way. Do you think your suggestion is more readable? I thought of using do-while just because we needed to at least read the value once.

[rswarbrick]

Nit: I think you want "the value being updated" ? Also there's a stray space before "RTL".

[antmarzam]

Addressed

[rswarbrick]

I don't think we need the extra info print.

I've re-read your previous message and I hadn't properly understood it before (sorry). Maybe the best thing is to make the task non-blocking (which, looking again, was my initial suggestion). Something like this, maybe?

task aon_timer_scoreboard::wkup_intr_predicted_values(bit exp_wkup_intr);
  static int unsigned last_cycle_count = 0;

  fork begin
    if (last_cycle_count != timed_regs.time_now) begin
      `uvm_info(`gfn, $sformatf("%m - Predicted wkup_intr = 0x%0x", exp_wkup_intr), UVM_DEBUG)
      predicted_wkup_intr_q.push_back(exp_wkup_intr);
      last_cycle_count = timed_regs.time_now;
      fork
        begin: iso_fork
          fork
            begin : wait_values_to_propagate
              // do backdoor read and delete values no longer valid.
              check_intr_value_propagated(WKUP);
            end : wait_values_to_propagate
            begin : reset_kill
              wait (under_reset);
            end : reset_kill
          join_any
          disable fork;
        end : iso_fork
      join
    end
  end
  join_none
endtask : wkup_intr_predicted_values

(and similarly for the other analogous tasks)

[rswarbrick]

I'd probably be inclined to inline the specialisation into the call sites, giving something like this:

          if (intr_state_val[WDOG])
            update_timed_regs_independently(.r(TimedIntrStateWdogBark), .tmr(WDOG));
          if (intr_state_val[WKUP])
            update_timed_regs_independently(.r(TimedIntrStateWkupExpired), .tmr(WKUP));

If we could make a map from timers_e to register pointer, this could be completely parametrised (and similarly at the other call sites)

[antmarzam]

I was thinking of only passing WKUP/WDOG to the function and figure the timed reg type inside. I now have a function called timers_e2time_reg_e which does this. I'll use it here too.

I can't remember the exact reason why I was casting for to timers_e, but I think I may have gotten a warning/error.

[rswarbrick]

I think this now has a name (wait_for_we_pulse) in the scoreboard. I'd try to make that task visible in the sequence as well (possibly by copy-pasting it), so that this code can just be:

  fork
    begin : iso_fork
      fork
        wait(cfg.under_reset);
        begin
          csr_utils_pkg::csr_wr(ptr, value);
          // After write we wait for WE to go high and then low
          wait_for_we_pulse(path_to_we);
        end
      join_any
      disable fork;
    end : iso_fork
  join

I'd even compress it further, to this, but I know you're not convinced by the macro:

  `DV_SPINWAIT_EXIT(begin
                      csr_utils_pkg::csr_wr(ptr, value);
                      // After write we wait for WE to go high and then low
                      wait_for_we_pulse(path_to_we);
                    end,
                    wait(cfg.under_reset))

[antmarzam]

I think that makes sense, maybe moving wait_for_we_pulse and hdl_read_bit to the aon package

[rswarbrick]

Ok, I think I still don't quite understand what's going on here.

I think that the call to wait_for_prediction will block the activity of the virtual sequence. Where does the predicting_value flag get written / cleared?

[rswarbrick]

I just chatted to Adrian about this! (because I realised I didn't quite understand). And now I understand a bit more. And I'm convinced that the comment is correct (although the code is maybe a bit more confusing than it needs to be: not sure)

@alees24: Thanks for your help!

[rswarbrick]

Some comments about the list of function/task prototypes at the top of the scoreboard.

[rswarbrick]

I spent a bit of time trying to properly understand how this works, and I ended up with something like this:

  // A write to wkup_cause may take some delay to arrive because of CDC crossing (from the TL bus on
  // the main clock to the register on the aon clock). This timing might overlap with an expected
  // watchdog interrupt. To avoid this causing a problem, we avoid checking the predicted value (in
  // run_wdog_bark_timer) if the timing collides.
  //
  // aon_clk_cycle is a counter of AON cycles which is kept in sync by track_aon_clk_cycle. It gets
  // snapshotted to last_wkup_cause_write_aon_clk_cycle in predict_wkup_cause when the write enable
  // for the aon_wkup_cause register drops. We can see that there has been a collision if the two
  // values are equal.

BUT I'm a little worried that there's still a race condition. The read of this counter is synchronised to the main clock. The write might be synchronised to either clock (depending on whether the wkup_cause register was busy). Can't we end up with things being sampled before being checked?

[rswarbrick]

I'm not sure either of these lines are needed? The functions below are not fifos or queues :-)

[antmarzam]

Removed - I didn't add them I don't think. May have accidentally moved them. I think they may came from the TB template generator

[rswarbrick]

Could you add a documentation comment for this task?

[antmarzam]

Addressed

[rswarbrick]

Could you add a documentation comment for this task?

[antmarzam]

Addressed

[rswarbrick]

I'd probably suggest listing the timers and interrupts that are getting modelled and checked (to make it easier for a reader to grep for them).

I'd also suggest explicitly noting that the task doesn't return.

[antmarzam]

Addressed

[rswarbrick]

I'd suggest writing a comment for each task to make it really explicit (you can always copy/paste or refer across!). When I first looked, I didn't understand that the three tasks are unrelated.

This will also need to explicitly say that each task runs forever and can be safely killed.

[antmarzam]

Tried to add some detail. Hopefully clear by now :)

[rswarbrick]

I think this comment needs expanding a bit to explain what's going on. We also need to duplicate it (with tweaks) for the other two copies. Something like this might work:

  // Whenever the sample_coverage event fires, sample the 64-bit wkup_count counter for the
  // wake_up_timer_thold_hit_cg covergroup.
  //
  // Runs forever, and can be safely killed at any time.

[antmarzam]

Addressed

[rswarbrick]

Let's put a different comment for each function/task. For this one, I'd suggest something like the following. I'd also drop the output argument: just return something: it's much easier!

  // Return both predicted timed register values, captured from intr_state_exp.
  extern function bfm_timed_regs_t capture_timed_regs();

[antmarzam]

Added a very similar comment but changed "Return" for "Set" since I think "return" may get a bit confusing in tasks/functions

[rswarbrick]

I'd suggest getting rid of this function: the implementation is pretty trivial and can just be inlined into capture_timed_regs.

[antmarzam]

I agree the implementation is trivial, but I was thinking to keep the re-use of Adrian's timed register looking alike the most I could. Just in case if we end up using them in multiple cases the way they are hooked up is kinda similar.
Thoughts?

[rswarbrick]

This function does not exist.

[antmarzam]

I realised that today (haven't pushed changes yet). Apparently VCS does not care, but Cadence does