[dv,pwm] DV improvements for PWM #25718
Conversation
|
For reviewers (possibly me!): The first commit comes from #25717, which should be merged first. |
hw/ip/pwm/dv/env/pwm_env_pkg.sv
Outdated
| } dc_blink_t; | ||
| } duty_cycle_t; | ||
|
|
||
| // Blink mode parameters (BLINK_PARAM_x). |
There was a problem hiding this comment.
I think I'd be inclined to use something like BLINK_PARAM_* to avoid "x" clashing with the X name!
There was a problem hiding this comment.
Switched both to _i for clarity.
hw/ip/pwm/dv/env/pwm_scoreboard.sv
Outdated
| channel_cfg.ClkDiv = next_cfg.ClkDiv; | ||
| channel_cfg.DcResn = next_cfg.DcResn; | ||
| end | ||
| // Enable bit applies immediately. |
There was a problem hiding this comment.
I'm not sure about "immediately" here. Maybe "is always accepted" (to match the words a few lines earlier)
hw/dv/sv/pwm_monitor/pwm_monitor.sv
Outdated
| // int unsigned clk_div, int unsigned dc_resn, | ||
| // int unsigned clk_elapsed); |
There was a problem hiding this comment.
I'm not sure I understand these commented arguments. Maybe add an explanatory comment?
There was a problem hiding this comment.
Oops, code restructuring; they are due to be deleted. Thanks.
hw/dv/sv/pwm_monitor/pwm_monitor.sv
Outdated
| // phase. | ||
| first_activation = 1'b1; | ||
| // Calculate how many core clocks a single pulse cycle takes; we require this information so | ||
| // that we may detect a pulse cycle in which the PWM assert is not asserted at all |
There was a problem hiding this comment.
Nit: Maybe "PWM output"?
hw/ip/pwm/dv/env/pwm_scoreboard.sv
Outdated
| @@ -144,7 +146,7 @@ task pwm_scoreboard::process_tl_access(tl_seq_item item, | |||
| // enabled with an arbitrary phase relationship to the shared phase counter. | |||
| ignore_state_change[ii] = SettleTime; | |||
| end | |||
| txt = $sformatf("\n Channel[%d] : %0b",ii, channel_en[ii]); | |||
| txt = $sformatf("\n Channel[%d] : %0b",ii, channel_en[ii]) ; | |||
There was a problem hiding this comment.
Nit: Missing space before second argument?
There was a problem hiding this comment.
Fair enough; lingering formatting issue from earlier DV...there are more than a few and I shy away from changing them generally. At some point I should address them all in a single commit with no functional impact.
hw/ip/pwm/dv/env/pwm_scoreboard.sv
Outdated
|
|
||
| endtask | ||
| `uvm_info(`gfn, $sformatf("Ch %0d active %0d first %0d", channel, active_cycles, first_activation[channel]), UVM_LOW) |
There was a problem hiding this comment.
I think this might be over length?
Introduce separate structures for the DUTY_CYCLE_i and BLINK_PARAM_i registers to reduce the potential for confusion. Remove the crosstalk between duty cycles an pulse cycle counts in `rand_pwm_blink.` This confusion appears to have resulted from the fact that in heartbeat mode BLINK_PARAM_i.Y represents an increment to the duty cycle rather than a count of pulses cycles. Other tidy ups/clarifications; no functional change in this commit beyond adjusting the ranges of A,B,X and Y values exercised. Signed-off-by: Adrian Lees <a.lees@lowrisc.org>
The majority of the sequence items were being rejected on account of a mismatch between the expected pulse period and that observed by the monitor. This discrepancy was the result of the monitor running on the wrong clock signal. Signed-off-by: Adrian Lees <a.lees@lowrisc.org>
Duty cycle prediction was very overcomplicated, confusing the task of forming the current prediction with that of then updating the state for the subsequent prediction. Tidy and clarify the detection of active and inactive output states within the monitor. The currently-required 'settle time' at start up and transition between blinking phases may be achieved using the same logic. Signed-off-by: Adrian Lees <a.lees@lowrisc.org>
Introduce phase monitoring and checking. Run the monitors on disabled channels too. Modify and simplify duty cycle prediction to meet the specification. Implement low power mode testing (TL-UL clock stopping and resuming). Introduce some handling of reset with an eye towards V3. Report the duty cycle as a 16-bit fixed-point number as per the hardware specification and implementation; this aids comprehension and diagnostics. Signed-off-by: Adrian Lees <a.lees@lowrisc.org>
To avoid incurring ping timeouts when the TL-UL clock is stopped during low power mode, ensure that the alert agent is operating on the same clock and not asynchronously. Signed-off-by: Adrian Lees <a.lees@lowrisc.org>
This PR offers extensive improvements to the amount of verification performed by the existing DV environment.
Unfortunately the majority of the sequence items were being silently ignored previously as a result of a couple of clear faults in the DV environment (incorrect clock, ignoring items that had a 'period' mismatch, and a 'settle time' whenever the blink phase changed).
The DV also made no attempt to monitor disabled channels, pulse cycles with no change in the output (duty cycle of zero), or the phase delay of outputs. The low power mode testing was also deficient.
This PR improves the DV and achieves a near 100% pass rate; there is an issue with 'pwm_alert_test' at present, and some sequences will fail on account of the initial pulse cycle being hard to predict because of the CDC and channel reprogramming/start up behavior. This should be improved by a later PR.
The existing test sequences are largely unmodified, but we shall definitely want some additional sequences or constraints to hit awkward edge cases, since most of the parameters have a large range and can interact badly (eg. large 'clk_div' and large 'dc_resn' on the phase counter, or large 'blink_param.x' and small 'blink_param.y' in heartbeat mode. In each case the simulation time quickly becomes infeasible).