[clkmgr] Align behavior of jitter_enable CSR #26040

vogelpi · 2025-01-28T02:03:47Z

This PR aligns RTL, block-level DV, DIF and doc to the modified behavior of the jitter_enable CSR. The corresponding ROM change is #26027.

rswarbrick · 2025-01-28T16:43:10Z

hw/ip_templates/clkmgr/data/clkmgr.hjson.tpl

+          // Exclude this register from any automated CSR tests as the behavior is non-standard.
+          tags: ["excl:CsrAllTests:CsrExclAll"]


Not a big deal, but is that exclusion change necessary? Which CSR test was failing?

I am checking this again right now. It might actually not be required anymore. I'll feed back.

It turns out this is really required. Without the change, all CSR tests except for the csr_hw_reset test fail.

This CSR is declared RW for software, so automated tests will expect after a write you will read back what you wrote. I expect the cip_lib csr_rw family of tests to fail.

rswarbrick · 2025-01-28T16:47:14Z

sw/device/lib/dif/dif_clkmgr_unittest.cc

-  EXPECT_WRITE32(CLKMGR_JITTER_ENABLE_REG_OFFSET, kMultiBitBool4False);
+  EXPECT_READ32(CLKMGR_JITTER_ENABLE_REG_OFFSET, kMultiBitBool4False);
  EXPECT_DIF_OK(dif_clkmgr_jitter_set_enabled(&clkmgr_, kDifToggleDisabled));


This looks a bit strange to me: as far as I know, it's no longer possible to disable jitter after it has been enabled. Do we just need to update the comment on line 35?

Thanks for your feedback @rswarbrick ! This here is just the DIF unittest. It doesn't take into account the behavior of the hardware (the jitter not being disablable once enabled). What this checks is whether the DIF functions when called with different arguments generate the correct number of reads and writes. And the test does two calls to the corresponding DIF function: once to enable the jitter (a read and a write) and once to disable it (one read only).

You are right, the comment is misleading, perhaps it should say attempt to disable

Ah, now I understand. Sorry @rswarbrick , I've misread your comment earlier. I'll update this with the next push.

andreaskurth

LGTM as an ECO. Thanks @vogelpi!

andreaskurth · 2025-01-28T17:23:43Z

util/reggen/fpv_csr.sv.tpl

@@ -86,6 +86,14 @@ module ${mod_base}_csr_assert_fpv import tlul_pkg::*;
  // accesses).
  bit [${hro_idx_width-1}:0] hro_idx;
  bit [${addr_msb}:0] normalized_addr;
+  % if lblock == "clkmgr":
+    % for hro_reg in hro_regs_list:
+      % if hro_reg.name.lower() == "jitter_enable":


Just a note here that while having code specific to a block and register hard-coded into reggen isn't a scalable or future-proof solution, I think it's totally appropriate for implementing this ECO on the earlgrey_1.0.0 branch.

andreaskurth · 2025-01-28T17:28:09Z

CHANGE AUTHORIZED: hw/ip_templates/clkmgr/data/clkmgr.hjson.tpl
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/clkmgr/data/clkmgr.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/clkmgr/rtl/clkmgr_reg_top.sv

Approved ECO

moidx · 2025-01-28T17:41:10Z

CHANGE AUTHORIZED: hw/ip_templates/clkmgr/data/clkmgr.hjson.tpl
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/clkmgr/data/clkmgr.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/clkmgr/rtl/clkmgr_reg_top.sv

Approved ECO

matutem · 2025-01-28T18:48:09Z

sw/device/lib/dif/dif_clkmgr_unittest.cc

@@ -33,10 +33,11 @@ class JitterEnableTest : public ClkMgrTest {};
 // dif_clkmgr_jitter_set_enabled doesn't perform a read, just a write.
 TEST_F(JitterEnableTest, SetEnabled) {
  // Disable jitter.
-  EXPECT_WRITE32(CLKMGR_JITTER_ENABLE_REG_OFFSET, kMultiBitBool4False);
+  EXPECT_READ32(CLKMGR_JITTER_ENABLE_REG_OFFSET, kMultiBitBool4False);
  EXPECT_DIF_OK(dif_clkmgr_jitter_set_enabled(&clkmgr_, kDifToggleDisabled));

  // Enable jitter.


I wonder if we should also check that non-mubi writes result in a value of mubi True.

With the present implementation of dif_clkmgr_jitter_set_enabled() we cannot test this. Because the function checks the argument: if it doesn't match kMultiBitBool4False or kMultiBitBool4True, i.e., if it's an invalid MuBi value, then the function returns kDifBadArg. This part of the function did not change with this PR.

I am open to change this, too. But TBH, I don't think it really matters for the ECO. We should try to land this asap and create the tag. There will be time to make further updates to the software / DIF and the documentation afterwards.

That is an artifact of the dif code, and we could write bypassing dif, but I agree this is low priority.

matutem · 2025-01-28T18:53:56Z

util/reggen/fpv_csr.sv.tpl

@@ -191,7 +199,14 @@ module ${mod_base}_csr_assert_fpv import tlul_pkg::*;
      if (d2h.d_valid) begin
        if (pend_trans[d_source_idx].wr_pending == 1) begin
          if (!d2h.d_error && regwen[hro_idx]) begin
+            % if lblock == "clkmgr":


Thanks so much for updating this: I would not have caught it, mostly because I can't run these tests.

By the way, do we check regwens in FPV?

The funny thing about this template is that it's used for generating clkmgr_csr_assert_fpv.sv which is also used during regular block-level DV (that's how I found out about the need for changing this file). Whether we check the regwens in FPV I don't know. TBH, I don't think we have an FPV setup for clkmgr, do we?

In any case the test would need to be created, since there is no way to infer which CSR a regwen is meant to affect. No magic bullets for these besides careful review.

matutem · 2025-01-28T18:57:28Z

util/reggen/reg_top.sv.tpl

-        else:
-          bit_sel = f'{field.bits.msb}:{field.bits.lsb}'
-        wd_expr = f'{clk_base_name}{reg_name}_wdata[{bit_sel}]'
+      if lblock == "clkmgr" and reg_name == "jitter_enable":


These changes in reg_top.sv.tpl are ugly, but luckily they'll just stay in this branch.

matutem · 2025-01-28T19:02:18Z

sw/device/lib/dif/dif_clkmgr.c

+  multi_bit_bool_t clk_jitter_enable_val =
+      mmio_region_read32(clkmgr->base_addr, CLKMGR_JITTER_ENABLE_REG_OFFSET);
+  if (clk_jitter_enable_val == kMultiBitBool4True &&
+      new_jitter_enable_val == kMultiBitBool4False) {


Suggested change

new_jitter_enable_val == kMultiBitBool4False) {

new_jitter_enable_val != kMultiBitBool4True) {

This will change with the next force push.

matutem

LGTM

matutem · 2025-01-28T19:10:03Z

Please consider the suggested changes.

vogelpi · 2025-01-28T19:47:10Z

Thanks for your reviews everybody! I am happy to include the suggested changes later tonight. (I am currently traveling.)

Signed-off-by: Pirmin Vogel <[email protected]>

vogelpi · 2025-01-28T23:11:32Z

Update: I've now pushed those changes proposed by @matutem and @rswarbrick which I was able to implement with reasonable effort. We should try to land this asap and I intend to merge this as soon as CI passes.

matutem · 2025-01-29T00:21:29Z

sw/device/lib/dif/dif_clkmgr.c

+    return kDifLocked;
+  }
+
+  if (new_jitter_enable_val == kMultiBitBool4True) {


This "if" is incorrect: even if we try to write False it will write True. I propose you remove the If, and replace new_jitter_enable_val below by kMultiBitBool4True and you will be true to the hardware. Even the test for kDifLocked seems somewhat redundant (all writes will happen, but they keep overwriting True), but returning a non-OK status should alert the software writer that something is going amiss. However, nothing can flag that trying to write a non-True mubi value won't work as intended.

Sorry @matutem , I merged the PR before seeing this. Can we fix this in the follow-up containing also the new software test, please?

I made the change in my test PR: #26058

Thanks @matutem !

matutem · 2025-01-29T00:29:24Z

sw/device/lib/dif/dif_clkmgr.c

+  multi_bit_bool_t clk_jitter_enable_val =
+      mmio_region_read32(clkmgr->base_addr, CLKMGR_JITTER_ENABLE_REG_OFFSET);
+  if (clk_jitter_enable_val == kMultiBitBool4True &&
+      new_jitter_enable_val != kMultiBitBool4True) {


This could simply be new_state != kDifToggleEnabked, and changing the write below to always be kMultiBitBool4True you realize new_jitter_enable_val is unused.

vogelpi requested review from a team and msfschaffner as code owners January 28, 2025 02:03

vogelpi requested review from rswarbrick, alees24, moidx, timothytrippel and matutem and removed request for a team, msfschaffner and alees24 January 28, 2025 02:03

vogelpi added the ECO Accepted as an ECO label Jan 28, 2025

vogelpi requested review from andreaskurth and cfrantz and removed request for rswarbrick and timothytrippel January 28, 2025 02:05

vogelpi force-pushed the clkmgr-jitter-enable-align branch 2 times, most recently from 61d2aa9 to f17ed7f Compare January 28, 2025 09:40

rswarbrick reviewed Jan 28, 2025

View reviewed changes

vogelpi force-pushed the clkmgr-jitter-enable-align branch from f17ed7f to ad20325 Compare January 28, 2025 17:03

andreaskurth approved these changes Jan 28, 2025

View reviewed changes

moidx approved these changes Jan 28, 2025

View reviewed changes

matutem reviewed Jan 28, 2025

View reviewed changes

matutem approved these changes Jan 28, 2025

View reviewed changes

[clkmgr] Align behavior of jitter_enable CSR

40c716e

Signed-off-by: Pirmin Vogel <[email protected]>

vogelpi force-pushed the clkmgr-jitter-enable-align branch from ad20325 to 40c716e Compare January 28, 2025 23:05

matutem reviewed Jan 29, 2025

View reviewed changes

vogelpi merged commit 676fe22 into lowRISC:earlgrey_1.0.0 Jan 29, 2025
32 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clkmgr] Align behavior of jitter_enable CSR #26040

[clkmgr] Align behavior of jitter_enable CSR #26040

vogelpi commented Jan 28, 2025

rswarbrick Jan 28, 2025

vogelpi Jan 28, 2025

vogelpi Jan 28, 2025

matutem Jan 28, 2025

rswarbrick Jan 28, 2025

vogelpi Jan 28, 2025

matutem Jan 28, 2025 •

edited

Loading

vogelpi Jan 28, 2025

andreaskurth left a comment

andreaskurth Jan 28, 2025

andreaskurth commented Jan 28, 2025

moidx commented Jan 28, 2025

matutem Jan 28, 2025

vogelpi Jan 28, 2025

matutem Jan 29, 2025

matutem Jan 28, 2025

matutem Jan 28, 2025

vogelpi Jan 28, 2025

matutem Jan 28, 2025

matutem Jan 28, 2025

matutem Jan 28, 2025

vogelpi Jan 28, 2025

matutem left a comment

matutem commented Jan 28, 2025

vogelpi commented Jan 28, 2025

vogelpi commented Jan 28, 2025

matutem Jan 29, 2025 •

edited

Loading

vogelpi Jan 29, 2025

matutem Jan 29, 2025

vogelpi Jan 30, 2025

matutem Jan 29, 2025

		// Exclude this register from any automated CSR tests as the behavior is non-standard.
		tags: ["excl:CsrAllTests:CsrExclAll"]

	new_jitter_enable_val == kMultiBitBool4False) {
	new_jitter_enable_val != kMultiBitBool4True) {

[clkmgr] Align behavior of jitter_enable CSR #26040

[clkmgr] Align behavior of jitter_enable CSR #26040

Conversation

vogelpi commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matutem Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskurth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreaskurth commented Jan 28, 2025

moidx commented Jan 28, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matutem left a comment

Choose a reason for hiding this comment

matutem commented Jan 28, 2025

vogelpi commented Jan 28, 2025

vogelpi commented Jan 28, 2025

matutem Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matutem Jan 28, 2025 •

edited

Loading

matutem Jan 29, 2025 •

edited

Loading