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

[alert_handler,rtl] Fix esc_ping_en widening cast #26380

Merged
merged 2 commits into from
Feb 21, 2025

Conversation

qmn
Copy link
Contributor

@qmn qmn commented Feb 20, 2025

Caught this with Verilator linting -- esc_ping_en is a N_ESC_SEV-wide (currently 4) one-hot signal that is shifted by esc_cnt (which goes up to 3). As a-will observes in #22568 (comment), the too-narrow cast didn't cause a problem because the LHS causes us to widen again to N_ESC_SEV anyway.

esc_ping_en is a N_ESC_SEV-wide one-hot signal that is shifted by
esc_cnt, which can go up to 3. As a-will observes in
lowRISC#22568 (comment),
the too-narrow cast didn't cause a problem because the LHS causes
us to widen again to N_ESC_SEV anyway.

Signed-off-by: Quan Nguyen <[email protected]>
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.

Thank you very much for catching the stupid bug in my code! I think this will currently fail CI because it is used as a template for some auto-generated (and checked in) files.

To regenerate them, "make -C hw" should work.

@qmn
Copy link
Contributor Author

qmn commented Feb 20, 2025

Thank you for the prompt review! And, d'oh, thanks for the nudge to regenerate.

Copy link
Contributor

@a-will a-will left a comment

Choose a reason for hiding this comment

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

LGTM 😄

@a-will
Copy link
Contributor

a-will commented Feb 20, 2025

CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_ping_timer.sv

No functional change occurs to earlgrey. This is a fixup to an inert cast to make the RHS have the correct width without relying on being the final expression before an assignment.

@rswarbrick
Copy link
Contributor

CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_ping_timer.sv

A bugfix in a mistake in my code which (very conveniently) didn't actually break anything: thanks for the analysis. But it looked silly and this is going to do the same thing but less mysteriously!

@rswarbrick rswarbrick merged commit 7858cde into lowRISC:master Feb 21, 2025
42 checks passed
@qmn qmn deleted the qmn/alert-handler-ping-req-width-fix branch February 21, 2025 13:44
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.

3 participants