-
Notifications
You must be signed in to change notification settings - Fork 795
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
[ipgen,otp_ctrl] Change otp_ctrl to ipgen #25503
base: master
Are you sure you want to change the base?
Conversation
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.
LGTM.
fab6d27
to
cfce607
Compare
CHANGE_AUTHORIZED: BLOCKFILE |
CHANGE_AUTHORIZED: BLOCKFILE |
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.
LGTM, thanks.
At this point, can we hold-off merging that until the full ipgen flow for otp_ctrl is available? We, downstream, have a custom otp_ctrl based on hw/ip/otp_ctrl. Since we are in a quite critical phase now, I want to avoid adding another thing to deal with, e.g., porting those changes to Darjeeling, which eventually look different when the ipgen version is available.
Please build the future PRs on top of this one. Then we can merge the ipgen version as a direct sequence of PRs (or a single one).
Thanks @matutem |
CHANGE AUTHORIZED: BLOCKFILE |
f447b49
to
024893d
Compare
@andreaskurth I am not quite sure how to deal with the change checks: I created the list of authorized changes, but that doesn't prevent the check to fail due to too many files. From what I see, check-pr-changes-allowed.py script just helps generating the list of |
266d111
to
657cdcb
Compare
@matutem: We can run CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/otp_ctrl/rtl/otp_ctrl_top_specific_pkg.sv This is a new file to which top-specific (in this case for Earlgrey) |
43ff428
to
14812a9
Compare
Yes @andreaskurth, the otp_ctrl_top_specific_pkg package has the items that were in otp_ctrl_pkg but that are top-specific. I take it your approval was with the understanding that these miscellaneous items would be handled adequately. |
2f8fc3e
to
0c950eb
Compare
88de25f
to
b3d89cd
Compare
- lowrisc:ip:tlul | ||
- lowrisc:prim:all | ||
- lowrisc:prim:ram_1p | ||
- lowrisc:prim:otp |
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 hierarchy is wonky (not the fault of this PR; it's inherited and is similar to issues with flash_ctrl). An ipgen IP depends on the effectively virtual prim:otp
, and that depends on the very same ipgen IP that instantiated it. In a world of multiple parameterizations, that doesn't work at all.
I feel like the OTP macro ought to live outside the otp_ctrl IP and instantiated in the "chip" level. Or...
Alternatively, perhaps we could have a template parameter for the OTP macro's (unique) module name and generate a generic macro with ipgen (instead of having a non-ipgen prim:otp
). Stuff the generic macro we created in its own core file with a virtual VLNV. Then, otp_ctrl.core (and otp_ctrl.sv) could instantiate that module, instead of the non-ipgen prim_otp
, and depend on the virtual VLNV. We'd essentially be creating a virtual macro library on the fly with ipgen, and top level core files would choose either the generated generic macro or an integrator's own.
Would that make sense to you?
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.
On second thought, the "prim" CSR layout makes no sense to anyone but one particular integrator. Maybe that should become a top-specific, vendor-specific IP that gets instantiated at "chip" level. 🤔
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 guess both are a separate discussion. They do make sense but we firs should get otp_ctrl to ipgen and then iterate on that. Right now, this PR is gating to bringup Darjeeling on master.
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 guess both are a separate discussion. They do make sense but we firs should get otp_ctrl to ipgen and then iterate on that. Right now, this PR is gating to bringup Darjeeling on master.
Oh, agreed on taking the incremental steps and not gating this PR on that particular issue. This was merely brought up for discussion.
80495aa
to
ae0a055
Compare
Convert englishbreakfast to use the ordinary topgen flow. Commit the generated code like other tops. Remove the fileset_top and fileset_topgen flags, in addition to the topgen-fusesoc.py script. The fileset_top and fileset_topgen flags are now completely unused, since all the IPs that once depended on them have been reimplemented as ipgen cores. Remove the cruft. Signed-off-by: Alexander Williams <[email protected]>
This commit is similar to other ipgen conversions, but changes a few
extra things because it needs to change the generation of otp memory
images. It merges the following 32 individual commits:
generated.
for example, ### is replaced by ${"###"}
file used to process the contents of a given file.
a virtual target.
fusesoc core.
no sense to use them to build targets.
with uniformly in the PR that fixes [ipgen] Insert per-ip DO NOT EDIT comment #25444.
needs to have topgen in sys.path. This was not needed for some reason,
perhaps the introduction of packages ends up requiring it.
per top generated files in the right place.
are more requests for random numbers in topgen.
the ipgen flow.
but keep otp_ctrl_pkg since it shows in types of unconnected ports.
Fixes #25019