[prim,rtl] Rephrase a term in prim_reg_cdc.sv #25481
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @vogelpi, Would you mind taking a look at this and checking the change is reasonable? If so, I'd really appreciate an explicit "change authorised" comment (and you are in a rather closer timezone than Matute!) |
vogelpi
approved these changes
Dec 5, 2024
hw/ip/prim/rtl/prim_reg_cdc.sv
Outdated
| // be copied back. | ||
| logic dst_to_src; | ||
| if (DstWrReq) begin : gen_wr_req | ||
| assign dst_to_src = src_busy_q && src_ack | src_update && !busy; |
There was a problem hiding this comment.
The functionality looks good to me (thanks for the clarifying comment above!) as src_update can only ever be non-zero if DstWrReq is 1.
But:
- We should convert the bitwise OR
|here to a boolean OR||for consistency. src_updateandbusyare unused in caseDstWrReqis 0. The latter is used in an assertion so lint may not realize this, the former should generate a lint error/warning that we should fix.
There was a problem hiding this comment.
- Oops about the
|: thanks! - Good point about the unused signals. I've added an
unused_...signal in the obvious way and can see a lint error go away. Thanks!
|
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_reg_cdc.sv This doesn't implement a functional change but helps to improve coverage. |
If DstWrReq is false then the src_update signal cannot be true. This is because it is driven by the src_update_o port on a prim_reg_cdc_arb instance. If DstWrReq is false then the instance wires src_update_o to zero (in the gen_passthru generate block). This rewrite will behave equivalently in the same way as before but will no longer generate any conditional coverage terms that depend on src_update if DstWrReq is false. Note that if DstWrReq is zero then the src_update and busy signals are constant zero and are no longer actually used. To avoid a lint error from the unused signals in this situation, they get "sampled" in the gen_passthru block. Signed-off-by: Rupert Swarbrick <[email protected]>
rswarbrick
force-pushed
the
prim-reg-cdc-coverage
branch
from
57bccd7 to
7119020
Compare
|
CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_reg_cdc.sv |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is trying to remove a coverage item that was a hole for some blocks that I was (wrongly) trying to waive with #25088. I hope this solution will be cleaner.
If DstWrReq is false then the src_update signal cannot be true. This is because it is driven by the src_update_o port on a prim_reg_cdc_arb instance. If DstWrReq is false then the instance wires src_update_o to zero (in the gen_passthru generate block).
This rewrite will behave equivalently in the same way as before but will no longer generate any conditional coverage terms that depend on src_update if DstWrReq is false.