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

[topgen] All RACL configs are relative to the project root #26122

Merged
merged 1 commit into from
Feb 12, 2025

Conversation

Razer6
Copy link
Member

@Razer6 Razer6 commented Feb 4, 2025

To allow topgen to be called from different paths.

@Razer6 Razer6 requested a review from msfschaffner as a code owner February 4, 2025 15:59
@Razer6 Razer6 requested review from rswarbrick, a-will, vogelpi, davidschrammel and matutem and removed request for msfschaffner February 4, 2025 16:00
Copy link
Contributor

@matutem matutem left a comment

Choose a reason for hiding this comment

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

Nice try, but it ends up causing generate_racl to have an extra argument, which becomes a special case in the new topgen. Ideally the racl_config path would start at hw/top_darjeeling, since that requires no additional args: topgen.py has a file scope variable holding the path to repo_top.

@Razer6
Copy link
Member Author

Razer6 commented Feb 4, 2025

Previosly, I had SRCTREE_TOP / topcfg['racl_config']. It was requested to change that to be relative to the HJSON file.

@a-will
Copy link
Contributor

a-will commented Feb 4, 2025

Nice try, but it ends up causing generate_racl to have an extra argument, which becomes a special case in the new topgen. Ideally the racl_config path would start at hw/top_darjeeling, since that requires no additional args: topgen.py has a file scope variable holding the path to repo_top.

From repo_top doesn't make sense. This is a file provided in the topgen user's tree, not the topgen tool's tree. Note that those are not necessarily the same root!

REPO_TOP is a fine root for tool-provided files (and one of the roots for referenced IPs and IP templates). However, if you have hard-coded in-tree paths for user files in the "new topgen," expect changes requested on that PR. 😄

@matutem
Copy link
Contributor

matutem commented Feb 5, 2025

I noticed you already place some racl hjson files under hw / top_darjeeling / data / racl. Could we assume that path and just have a file naming convention? Internally we deduce that from path to the top config hjson file.

@a-will
Copy link
Contributor

a-will commented Feb 5, 2025

I noticed you already place some racl hjson files under hw / top_darjeeling / data / racl. Could we assume that path and just have a file naming convention? Internally we deduce that from path to the top config hjson file.

So, to elaborate on the suggestion from @matutem , it might look like this:

-  racl_config: 'racl/racl.hjson'
+  racl_config: 'racl'  # Points to a file located at <toplevel.hjson dir>/racl/<racl_config>.hjson

...

-      racl_mappings: {
-        soc: 'racl/all_rd_wr_mapping.hjson'
-      }
+      racl_mappings: {
+        soc: 'all_rd_wr_mapping' # Points to a file located at <toplevel.hjson dir>/racl/<racl_mappings>.hjson
+      }

I don't have a particular opinion for whether to use names (which would be like the current approach with xbar) or direct file paths in toplevel.hjson.

@Razer6
Copy link
Member Author

Razer6 commented Feb 5, 2025

So, to elaborate on the suggestion from @matutem , it might look like this:

-  racl_config: 'racl/racl.hjson'
+  racl_config: 'racl'  # Points to a file located at <toplevel.hjson dir>/racl/<racl_config>.hjson

...

-      racl_mappings: {
-        soc: 'racl/all_rd_wr_mapping.hjson'
-      }
+      racl_mappings: {
+        soc: 'all_rd_wr_mapping' # Points to a file located at <toplevel.hjson dir>/racl/<racl_mappings>.hjson
+      }

I don't have a particular opinion for whether to use names (which would be like the current approach with xbar) or direct file paths in toplevel.hjson.

Frankly, I disagree with that approach. All this adds is another assumption rather than being explicit, e.g., place the files relative to the toplevel.hjson. Furthermore, downstream users may want to point Darjeelings files to a different configuration and leave the default configuration untouched.

Copy link
Contributor

@matutem matutem left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@alees24 alees24 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 this; I've tried it on the Darjeeling DV sim and it's fine, so I've dropped my hack.
It just needs linting fixes from CI.

alees24 pushed a commit to alees24/opentitan that referenced this pull request Feb 11, 2025
Modified version of changes from lowRISC#26122; do NOT merge this commit

Signed-off-by: Adrian Lees <[email protected]>
@Razer6 Razer6 force-pushed the racl-path branch 2 times, most recently from f013690 to 6a4c642 Compare February 12, 2025 08:55
@rswarbrick rswarbrick merged commit 0ca3b9f into lowRISC:master Feb 12, 2025
42 checks passed
@Razer6 Razer6 deleted the racl-path branch February 12, 2025 12:10
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.

5 participants