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_hander,rtl] Add support for uniquification #25800

Merged
merged 6 commits into from
Jan 10, 2025

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Jan 7, 2025

This PR adds support for uniquification of the alert handler in case there are multiple in the design. The PR is split into multiple to make the review easier.

  1. Rename alert_pkg to alert_handler_pkg. The rename is done to ease the uniquification of the alert handler. From alert_handler_pkg, alert_handler will be the module instance name.
  2. Rename core and SV files to templates but without any change
  3. Add the module_instance_name to the tpldesc file
  4. Fix template control characters that would change the rendered results
  5. Use module_instance_name within the templates and uniquify the alert_handler
  6. Do not render module_instance_name into Bazel BUILD file templates.

This only modifies the templates. Rendering the alert_handler (without uniquification) does not yield any RTL or core file diff, which is a good sign.

@Razer6 Razer6 requested a review from a team as a code owner January 7, 2025 19:16
@Razer6 Razer6 requested review from matutem and removed request for a team January 7, 2025 19:16
@Razer6 Razer6 marked this pull request as draft January 7, 2025 19:17
@Razer6 Razer6 force-pushed the uniquify-alert-handler branch from b93e55c to 5fa77ab Compare January 7, 2025 19:48
@Razer6 Razer6 requested review from rswarbrick and vogelpi and removed request for matutem January 8, 2025 06:39
@Razer6 Razer6 marked this pull request as ready for review January 8, 2025 06:40
@Razer6 Razer6 requested a review from msfschaffner as a code owner January 8, 2025 06:40
@Razer6 Razer6 force-pushed the uniquify-alert-handler branch from d03a852 to 02c1acc Compare January 8, 2025 07:36
@@ -195,6 +195,10 @@ def _filename_without_tpl_suffix(self, filepath: Path) -> str:
""" Get the name of the file without a '.tpl' suffix. """
assert filepath.suffix == '.tpl'
filename = filepath.stem
# Do not render the module_instance_name into bazel BUILD file templates
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm interested to know what went wrong with this :-) But don't we want to move this commit to come before the change to using module_instance_name?

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea of the logic in this function is to unquify the filenames if it is a *.tpl file. So alert_handler_foo.sv.tpl becomes <module_instance_name>_foo.sv. Note, this only works if the file is correctly prefixed with default module name. In this case it is alert_handler. Otherwise, it would overwrite parts of the original file name or everything (due to to the indexing here filename[len(self.ip_template.name):]).

We have templated BUILD files, i.e., BUILD.tpl. Due to the uniquification (and the wrong prefix), BUILD.tpl is rendered to <module_instance_name>. To fix this, I exclude BUILD template files from the file uniquification at all.

But don't we want to move this commit to come before the change to using module_instance_name?

It does not matter. The OpenTitan code bases does not have a uniquified alert_handler that sets the module_instance_name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahah, I see. Thanks for the explanation.

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.

This looks very sensible to me. And thank you for breaking it into small(ish) commits: it made the review much easier!

@Razer6 Razer6 requested review from a-will and removed request for msfschaffner January 8, 2025 14:38
@rswarbrick
Copy link
Contributor

CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/data/alert_handler.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_accu.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_class.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_esc_timer.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_lpg_ctrl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_ping_timer.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_pkg.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_reg_wrap.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/rstmgr/data/rstmgr.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/rstmgr/rtl/rstmgr.sv
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

Although this looks like a lot of changes, it's essentially just making some names overridable. No risk to the design: the default values for the names are the ones we have been using.

@Razer6 Razer6 force-pushed the uniquify-alert-handler branch from 02c1acc to b941cf3 Compare January 8, 2025 16:03
@vogelpi
Copy link
Contributor

vogelpi commented Jan 9, 2025

I think we should probably first merge #25773. I fear merging one or the other will require some rebasing and this one here seems to be smaller to rebase. What is your view @a-will @Razer6 ?

@a-will
Copy link
Contributor

a-will commented Jan 9, 2025

I think we should probably first merge #25773. I fear merging one or the other will require some rebasing and this one here seems to be smaller to rebase. What is your view @a-will @Razer6 ?

Your analysis is probably correct (i.e. fewer commits to carry conflict resolution steps through), but no preference here. There will be some minor conflicts in the fusesoc core files, and one of us would have to deal with it either way.

@Razer6
Copy link
Member Author

Razer6 commented Jan 9, 2025

@a-will Is your PR ready? Happy to merge yours first and I rebase

@a-will
Copy link
Contributor

a-will commented Jan 9, 2025

@a-will Is your PR ready? Happy to merge yours first and I rebase

I believe so. I'm just letting it finish off the CI FPGA tests, and then we could click the merge button.

@Razer6
Copy link
Member Author

Razer6 commented Jan 9, 2025

I believe so. I'm just letting it finish off the CI FPGA tests, and then we could click the merge button.

That sounds good to me!

Razer6 added 6 commits January 9, 2025 21:04
The rename is done to ease the uniquification of the alert handler.
From alert_handler_pkg, alert_handler will be the module instance name.

Signed-off-by: Robert Schilling <[email protected]>
Files are only renamed. Templates don't use the module_instance_name yet.

Signed-off-by: Robert Schilling <[email protected]>
@Razer6 Razer6 force-pushed the uniquify-alert-handler branch from b941cf3 to e325fe8 Compare January 9, 2025 20:46
@Razer6
Copy link
Member Author

Razer6 commented Jan 9, 2025

@a-will @vogelpi I just rebased to the new changes. Can you take another look?

@rswarbrick
Copy link
Contributor

@a-will, @vogelpi: Thanks for the reviews. This looks good now, but someone is going to need to explicitly authorize the changes.

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.

Thanks for nicely splitting the changes into digestible commits @Razer6 . LGTM!

@vogelpi
Copy link
Contributor

vogelpi commented Jan 10, 2025

CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/data/alert_handler.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_accu.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_class.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_esc_timer.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_lpg_ctrl.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_ping_timer.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_pkg.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/rtl/alert_handler_reg_wrap.sv
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/rstmgr/data/rstmgr.hjson
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/rstmgr/rtl/rstmgr.sv
CHANGE AUTHORIZED: hw/top_earlgrey/rtl/autogen/top_earlgrey.sv

The only Earlgrey RTL changes resulting from this PR are related to renaming the alert_pkg to alert_handler_pkg. This is fine.

@vogelpi vogelpi merged commit dc134b4 into lowRISC:master Jan 10, 2025
38 checks passed
@Razer6 Razer6 deleted the uniquify-alert-handler branch January 13, 2025 15:18
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