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

[hw,alert_handler,rtl] Use full-scope package definition in toplevel #25935

Merged
merged 1 commit into from
Jan 20, 2025

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Jan 18, 2025

We use a stiching tool to automatically connect ports outside of the OpenTitan bounding box. This tool relies on the fact having the full-scope struct type available in the port list rather than an already imported type. This PR adds the full-scope port type to the alert and escalation interface of the alert handler, which we also use outside of OpenTitan.

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.

This seems fine, but at some point, it sounds like we'll break the stitching tool when we introduce type parameters for some ports.

...or not, if the stitching tool only cared here because the relevant port went up to a "top-level" port. I notice that alert_crashdump_t was left alone.

@Razer6
Copy link
Member Author

Razer6 commented Jan 18, 2025

This seems fine, but at some point, it sounds like we'll break the stitching tool when we introduce type parameters for some ports.

Hehe, I'm just preparing one PR. Let me check on the alert_crashdump_t.

@vogelpi
Copy link
Contributor

vogelpi commented Jan 20, 2025

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

This PR doesn't change the functionality of Earlgrey.

Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

LGTM

@vogelpi vogelpi added the CI:Rerun Rerun failed CI jobs label Jan 20, 2025
@github-actions github-actions bot removed the CI:Rerun Rerun failed CI jobs label Jan 20, 2025
@a-will
Copy link
Contributor

a-will commented Jan 20, 2025

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

@rswarbrick
Copy link
Contributor

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

This won't change behaviour: it's just making some name resolution manual.

@rswarbrick rswarbrick merged commit a69332d into lowRISC:master Jan 20, 2025
40 checks passed
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.

4 participants