-
Notifications
You must be signed in to change notification settings - Fork 779
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
[pwrmgr,ipgen] Convert pwrmgr to ipgen #19801
Conversation
652b485
to
586e4d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a reasonably big PR at the moment :-) Would it make sense to split out the first few commits into their own PR? We could get them in (and forget about them!) quickly while we think about the bigger changes that come later.
@rswarbrick I broke this PR into about 9 commits to address this issue. I think it is reasonable to commit this, since it addresses a 2 year old issue, and it has helped a lot in finding other areas of concern for multi-top, like the document generation, and make_new_dif. In other words, this doesn't address multi-top, but it will make it easier to deal with it. |
@rswarbrick Regarding breaking this down into multiple PRs, the only sensible partition would be to have the first 5 commits into a PR, but they are almost trivial. The commits after that should go together. The interesting commits are
I added some of this info to each commit to help with per commit reviews. |
7d7d39f
to
3979235
Compare
The kokoro issue should now be resolved #19853. |
This has no functional changes. Format the code with lintpy. Simplify some path handling. Add more typing in function declarations. Signed-off-by: Guillermo Maturana <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
Remove redundant commas and quotes. Signed-off-by: Guillermo Maturana <[email protected]>
Populate the default for object parameters with representative data. Ideally these should have an associated validator function. Signed-off-by: Guillermo Maturana <[email protected]>
This is a straight copy with no changes to the files. Signed-off-by: Guillermo Maturana <[email protected]>
The files are almost identical to the previous ones, except they use ${top_name} instead of a specific name and consecutive '#' are escaped so they are not considered md comments. This escape replaces things like `###` by `${"###"}`. Signed-off-by: Guillermo Maturana <[email protected]>
This changes topgen.py to generate pwrmgr using ipgen instead of the former way. The old hw/<top>/ip/pwrmgr files are no longer created by topgen. Other than the change in topgen.py, all new files are just a side-effect of running `make -C hw`. Signed-off-by: Guillermo Maturana <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
Signed-off-by: Guillermo Maturana <[email protected]>
Ok, thanks for the explanation. I'm afraid that I'm not enthusiastic about reviewing a 15-commit PR. I'm hoping there's some way to partition it into smaller pieces. If it's honestly impossible to split it, I guess someone will have to review the whole lot at once. But I guess we'll also have to squash all the commits together to make the change atomic. I'm not sure I'm convinced that's the case: the current diff ~10,000 lines so you're not going to get any meaningful review (from anyone!) without some form of splitting. |
I think it would help a lot if you reorganized the commits to do a move, instead of copy...delete. Most of the changes are from the copying (and another big chunk was the automatic lint fixes). By the looks of it, though, after the change to a move, most of it would probably need to be squashed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't look too closely at the file copying/deleting commits or the autogeneration one (other than the topgen changes), but the rest of it look reasonable to me. Thanks for getting us one step closer to multi-top!
Regarding the commits, I think it's probably impossible to do all this in a simple, clean, atomic way. Maybe the copy+delete could be merged to make a single move commit that would be slightly cleaner. But I don't think it's critical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would nice to document the template parameters a bit further in the future. These descriptions are a little terse on their own, but maybe there could be links to a subsection in theory of operations about template parameterization that goes into a bit more depth. E.g. the difference between 'peripheral', 'int' and 'debug' indexes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tend to agree we need to describe parameterization. It should be a non-top-specific doc and location in the book, and the existing docs are generated, so end up top-specific. Perhaps pwrmgr/README.md.tpl is the most generic? I would punt this to another PR...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes let’s punt for now, it equally will apply to all ipgen blocks
@rswarbrick @a-will The reason I added specific comments and broke this into many commits, and highlighted in each commit what was the substantial change, is so you don't waste time looking at the creation or deletion steps. The effective number of lines changed in this commit is a whole lot less than 10,000. This is a tricky PR. If I make it all in one commit (for atomicity), I may be asked again to break it into more commits. And if I change the commits to change the copy and delete step into a move there will be commits where nothing works. I think the simplest way to really check this commit is to diff the files in ip_autogen against the union of the previous files in both ip/pwrmgr and the autogens in top_earlgrey/ip/pwrmgr. There should be very few minor differences. You will notice the absence of the "// Do NOT EDIT" comments. I think that can be handled in a separate commit. The reason is that now every single file in ip_autogen should have such notice, not just the ones created from templates. That change will also need to be retrofitted to alert_handler and rv_plic. I created #19943 for this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the cleanup @matutem! This was long overdue.
- lowrisc:prim:subreg | ||
- "!fileset_topgen ? (lowrisc:systems:pwrmgr_reg)" | ||
- "fileset_topgen ? (lowrisc:systems:topgen-reg-only)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is really nice to see these things disappear ;).
CHANGE AUTHORIZED: hw/ip_templates/pwrmgr/rtl/pwrmgr.sv |
This needs another authorization to be merged. |
CHANGE AUTHORIZED: hw/ip_templates/pwrmgr/rtl/pwrmgr.sv |
This has 16 commits, the first few are minor cleanups, and then
Remember the ip_autogen files are generated, so they don't need much attention when reviewing.
Addresses #8440