[rtl,clkmgr] Fix missing jitter regwen #26259
Conversation
Fixes lowRISC#26140 Signed-off-by: Guillermo Maturana <[email protected]>
matutem
requested review from
rswarbrick,
moidx and
vogelpi
and removed request for
a team
| mubi4_t prev_value; | ||
| mubi4_t new_value; |
There was a problem hiding this comment.
Nit: I'd change the names to something like value0, value to avoid it looking like prev_value was the value that the register had before this task.
There was a problem hiding this comment.
It was the value before this task is called to change it to new_value.
sw/device/lib/dif/dif_clkmgr.c
Outdated
| /** | ||
| * Checks if the jitter enable register is locked. | ||
| * | ||
| * Jitter enable register is locked by CLKMGR_JITTER_REGWEN. |
There was a problem hiding this comment.
I think this should probably be "The jitter enable ..." ? (Similarly for "The external clock ..." below)
|
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/clkmgr/data/clkmgr.hjson |
| @@ -187,8 +187,10 @@ | |||
| jitter_o clkmgr output to toggle. Verify this output is connected to AST's | |||
| clk_src_sys_jen_i input using formal. | |||
|
|
|||
| If jitter_regwen is enabled check that jitter_enable can be toggled. | |||
| If jitter_regwen is disabled check that jitter_enable cannot be changed. | |||
There was a problem hiding this comment.
Nit: the test always disables the jitter_regwen and then checks that jitter_enable cannot be changed. I think this is great. The test can run independent whether the jitter_enable is locked already.
There was a problem hiding this comment.
The test depends on the current state of jitter_regwen: if it is unlocked it checks that jitter_enable can be flipped, and if locked it checks it cannot. This is per line 50 in clkmgr_jitter_test.c.
There was a problem hiding this comment.
This is correct. What I meant is: after the if on line 50, the code enters the function which first disables the regwen and then tries changing the value. This part is executed independent whether the jitter_regwen is enabled or disabled when the test starts.
So one could change the test description to something like:
If jitter_regwen is enabled check that jitter_enable can be toggled, and disable jitter_regwen afterwards.
If jitter_regwen is disabled check that jitter_enable cannot be changed.
But it's not that important. The great thing is that this is now properly tested :-)
| // This checks there are no errors. | ||
| CHECK_STATUS_OK(clkmgr_testutils_check_measurement_counts(&clkmgr)); | ||
| CHECK_STATUS_OK(clkmgr_testutils_disable_clock_counts(&clkmgr)); | ||
| if (kDeviceType == kDeviceSimDV || kDeviceType == kDeviceSilicon) { |
There was a problem hiding this comment.
IIRC, you added a print statement in earlgrey_1.0.0 to notify the user that we don't check this on FPGA. This would probably be useful here as well.
|
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/clkmgr/data/clkmgr.hjson |
Enhance testing at block and top level. Enhance clkmgr dif for this feature. Fixes lowRISC#26258 Signed-off-by: Guillermo Maturana <[email protected]>
matutem
force-pushed
the
fix_missing_jitter_regwen
branch
from
36e6a95 to
9ebaa40
Compare
This has two commits:
Fixes #26258