[prim,clkmgr,rtl] Simplify prim_clock_meas by removing RefCnt #26029
Conversation
rswarbrick
force-pushed
the
prim-cnt-meas-refcnt
branch
from
e989ed3 to
3eabca5
Compare
There was a problem hiding this comment.
I am okay with the change assuming I understand this all correctly (see my comment), sorry for the delay @rswarbrick .
Before merging this, would you mind rebasing this please and wait for CI to run through? Because the PR is 3 weeks old now.
| end else begin : gen_normal_case | ||
| logic [RefCntWidth-1:0] cnt_ref; | ||
| assign valid = valid_ref & (int'(cnt_ref) == RefCnt - 1); | ||
| always_ff @(posedge clk_i or negedge rst_ni) begin | ||
| if (!rst_ni) begin | ||
| cnt_ref <= '0; | ||
| end else if (!cnt_en && |cnt_ref) begin | ||
| cnt_ref <= '0; | ||
| end else if (cnt_en && valid) begin | ||
| cnt_ref <= '0; | ||
| end else if (cnt_en && valid_ref) begin | ||
| cnt_ref <= cnt_ref + 1'b1; | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
The following points make me think that it is fine to remove this option but I would like to get your input on this as well Rupert:
- There is no single instance in any design with
RefCnt != 0. So this feature is not used anywhere. - We don't have any DV for this feature (from the Git history, I can see that the
RefCnt == 1special case was specifically added to ease coverage closure). So even if we would start using the feature, there would be additional DV work. - With a
RefCntvalue >= 1, the module can count clock cycles across multiple reference clock cycles thereby allowing to average count values across multiple reference clocks. WithRefCnt == 1, every reference clock edge clears the counting and triggers a comparison with the threshold registers. I would argue that since we have configurable low and high thresholds in clkmgr, we can just tune these to increase tolerance if needed. But I don't see a strong argument for averaging across multiple reference clocks. Or do I miss something?
There was a problem hiding this comment.
I think I completely agree. Annoyingly, my "git grep" promptly found some stray uses of the parameter name! So I'll force-push in a sec to expunge them as well.
And I'll rebase and wait for CI.
rswarbrick
force-pushed
the
prim-cnt-meas-refcnt
branch
from
3eabca5 to
d1bd329
Compare
|
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_clock_meas.sv This PR removes an unused and untested feature. This has no functional impact. |
This parameter is only ever set to 1: we don't do things like testing a clock that should be 50% faster by setting Cnt=30, RefCnt=20. Simplify the code slightly by getting rid of the support. This commit also expands a comment explaining Cnt: my initial read suggested that this was the maximum value of a counter, which gives the wrong width if Cnt is a power of two. Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
prim-cnt-meas-refcnt
branch
from
d1bd329 to
9d6e395
Compare
|
Oops: I left a stray usage of a computed parameter (the reason for the CI failure). The force-push fixes that silliness. |
|
Hi @rswarbrick , IIUC @matutem suggested that we should actually keep the feature. Let's put this on hold and discuss in the issue here: #26385 |
|
Cool, that makes sense. I'll mark it as a draft (in case we decide to rip out the current machinery). |
This parameter is only ever set to 1: we don't do things like testing a clock that should be 50% faster by setting Cnt=30, RefCnt=20. Simplify the code slightly by getting rid of the support.
This commit also expands a comment explaining Cnt: my initial read suggested that this was the maximum value of a counter, which gives the wrong width if Cnt is a power of two.