Conversation
There was a problem hiding this comment.
Thanks a lot for your contribution, overall, LGTM!
I left some minor comments, mostly related to naming conventions that differ from the official SystemRDL style.
We could agree on a common structure for naming internal sub-fields. In particular, as highlighted in the SystemRDL style guide, I would recommend avoiding excessive prefixes, which can be overly verbose. Since the generated top-level register file follows a structured approach, the regfile name is already visible when accessing internal sub-fields, making such prefixes unnecessary.
| ) { | ||
| reg msip { | ||
| name = "MSIP"; | ||
| desc = "Machine Software Interrupt Pending"; |
There was a problem hiding this comment.
We could add a note like: active high/low to improve the documentation
| desc = "Machine Software Interrupt Pending"; | ||
| field { | ||
| name = "P"; | ||
| desc = "Machine Software Interrupt Pending"; |
|
I took the liberty to push some commits to this PR that also address @Lore0599 suggestions🤓 The hierarchical names should now be as minimal as possible without redundant prefixes. I also renamed |
local `peakrdl-regblock` version was older
paulsc96
left a comment
paulsc96
left a comment
There was a problem hiding this comment.
Looks solid overall. But I have no prior experience with SystemRDL, so I can't really vouch for the new generation approach.
It's a bit unfortunate that we are still stuck with generation to begin with, but SystemRDL/PeakRDL#54 / SystemRDL/PeakRDL-regblock#33 / SystemRDL/systemrdl-compiler#58 don't seem like they will make any progress this millenium, so we might aswell move now.
I won't be around much longer, so if anyone wants to take over maintainership of this repo, go right ahead.
|
Yes, parametrizable register block output would be amazing. But it's already an improvement to what we had before |
4 participants
No description provided.