Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[dv, aon_timer] Waiving RTL unreachable statements pending rtl change #26165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

antmarzam
Copy link
Contributor

These exclusions cover the missing branch/line coverage for aon_timer. Once are PRs #25812/#26072 make it to maste these exclusions can be ammendeed/removed.

There's also a github issue #26164 to re-evaluate the RTL changes which may affect CDC sign-off.

@antmarzam antmarzam requested a review from a team as a code owner February 7, 2025 13:43
@antmarzam antmarzam requested review from marnovandermaas and rswarbrick and removed request for a team February 7, 2025 13:43
@antmarzam antmarzam force-pushed the aon_timer_temporary_rtl_exclusions branch from 7cf94f9 to efeaf2e Compare February 7, 2025 15:39
These exclusions cover the missing branch/line coverage for aon_timer.
Once are PRs  lowRISC#25812/lowRISC#26072 make it to maste these exclusions can be
ammendeed/removed.

There's also a github issue lowRISC#26164 to re-evaluate the RTL changes which may
affect CDC sign-off.

Signed-off-by: Antonio Martinez Zambrana <[email protected]>
@antmarzam antmarzam force-pushed the aon_timer_temporary_rtl_exclusions branch from efeaf2e to 857de0c Compare February 7, 2025 16:34
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nitty comments, but this looks sensible to me.

Branch 4 "3547459906" "gen_wr_req.state_q" (6) "gen_wr_req.state_q default,-,-,-,-"
CHECKSUM: "1125143680 279071062"
INSTANCE: tb.dut.u_reg.u_wkup_count_hi_cdc
ANNOTATION: "Src_ack will only be set after src_busy_q is set. This can be removed via RTL modification, and it will be revisit after the next CDC sign-off. Issue is #26164"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Grammar nit: "Revisited"

//==================================================
CHECKSUM: "2180545894 2543311903"
INSTANCE: tb.dut.u_reg.u_wkup_count_hi_cdc.u_arb
ANNOTATION: "Default statement can't be hit. PR#26072 takes care of ammending RTL. In the meantime excluded to close coverage."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to drop the PR number here. Otherwise, we end up with a "dead link" if we close that PR and open a different one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants