[ipgen,topgen] Uniquify VLNVs and reorganize core hierarchies #25773
Conversation
a-will
requested review from
rswarbrick,
mundaym,
a team,
msfschaffner and
cfrantz
as code owners
|
Note that virtual VLNVs can be a trap, and we should be careful about using them in scenarios where we might want a second instance of something with different parameters. This PR reduces the number of these virtual VLNVs that are in our code base. |
|
Ah, probably need to remove bbca444, since it's more for englishbreakfast moving to the ordinary topgen flow, but I've removed those commits for this PR's sake. |
| @@ -1 +0,0 @@ | |||
|
|
|||
There was a problem hiding this comment.
This looks good to me, many thanks @a-will (and also @adk) for your work on this!
I just have some minor questions. For the DV interface changes, the review feedback of a DV expert like @rswarbrick or @matutem would certainly be useful.
|
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/data/alert_handler.hjson The only Earl Grey RTL changes resulting from this PR are in |
|
@matutem @rswarbrick In case it helps cut down webpage loading time for a more DV-focused review, this is the relevant commit for changes to DV files (i.e. that aren't fusesoc core files): If we end up doing a rebase and the commit hash changes, it's the one with this description: |
| @@ -51,5 +51,11 @@ | |||
| type: "object" | |||
| default: [] | |||
| } | |||
| { | |||
There was a problem hiding this comment.
This should be unnecessary if we fix the VLNV convention :) I guess the problem you face is that currently this name is not going through instance_vlnv?
There was a problem hiding this comment.
This was a conscious choice to not totally forbid running ipgen outside of topgen, actually. But... probably adjusting the VLNV convention would make this have a nicer structure.
At some point, this global package requirement should go away, though. It is not reasonable to require a globally defined top_pkg::TL_<property> to be consumed, since interconnection networks are not always uniform.
| default: "1" | ||
| } | ||
| { | ||
| name: "pwrmgr_vlnv_prefix" |
There was a problem hiding this comment.
Another side-effect of the hopefully-to-be-replaced instance_vlnv implementation.
There was a problem hiding this comment.
They might change some, but I don't think we can remove these template variables altogether. At some point, we need to allow multiple specializations of clkmgr / pwrmgr / rstmgr in one topgen run.
| type: "string" | ||
| default: "" | ||
| } | ||
| { |
There was a problem hiding this comment.
Ditto regarding instance_vlnv.
Place the prim_mubi_pkg package into a separate prim_mubi_pkg fusesoc core, so it can be used freely for interfaces without pulling in the RTL. This helps avoid dependency cycles in the build graph. Adjust tlul:headers to depend only on the prim_mubi_pkg core. Signed-off-by: Alexander Williams <[email protected]>
The underlying generic prim library tries to depend on prim:flop, but the OT fusesoc fork cannot run generators on generated cores, leading to missing files. Signed-off-by: Alexander Williams <[email protected]>
…TL files Signed-off-by: Andreas Kurth <[email protected]>
Move the shared interfaces for SVAs out of the templated IP directories to avoid duplicate cores. Signed-off-by: Alexander Williams <[email protected]>
Make the IP <-> top-level VLNV interfaces virtual, and assign unique VLNVs to the specific ast, sensor_ctrl, ibex_pmp_rest_pkg, jtag_id_pkg, scan_role_pkg, and top_pkg cores that once were owned by earlgrey. Signed-off-by: Andreas Kurth <[email protected]>
Make all core files have instance-unique VLNVs. Move packages referenced by other IPs to leaf cores in the dependency tree. Add a template parameter to specify the real top_pkg. Co-authored-by: Andreas Kurth <[email protected]> Signed-off-by: Alexander Williams <[email protected]>
Add ability to supply ipgen's instance_vlnv() with an explicit reference to another VLNV, so topgen can stitch together related ipgen outputs (together with an IP template that does the work). In addition, add the specific top-level VLNVs for top_pkg references in alert_handler. Signed-off-by: Alexander Williams <[email protected]>
Make all core files have instance-unique VLNVs. Move packages referenced by other IPs to leaf cores in the dependency tree. Add a template parameter to specify the real top_pkg and scan_role_pkg. Co-authored-by: Andreas Kurth <[email protected]> Signed-off-by: Alexander Williams <[email protected]>
Remove references to alert_pkg. The escalation severity levels and ping counter width value can come from parameters instead. Removing the dependency on alert_handler helps make the alert_handler an optional part of a topgen-supported design. English Breakfast already does not have an alert_handler, for example. Also remove the pwrmgr_components core, which did not appear to have a reason to be split from the overarching pwrmgr core. (Or, put another way, remove the pwrmgr core and promote pwrmgr_components to pwrmgr.) Depend on the specific VLNVs, since topgen is aware of what they are upon generating outputs. Remove ip_interfaces:pwrmgr and pwrmgr_reg virtual VLNVs. There are no users for these virtual VLNVs, and dropping them helps move us closer to allowing multiple instances, if needed. There is still a virtual VLNV exported for pwrmgr_pkg, since non-ipgen cores currently still use it. Co-authored-by: Andreas Kurth <[email protected]> Signed-off-by: Alexander Williams <[email protected]>
Use topgen and template parameters to create a mostly self-contained clkmgr output without virtual VLNVs. The only virtual VLNV that remains is the one assigned to clkmgr_pkg, and clkmgr no longer depends on any virtual VLNVs. The template parameters supply the real VLNVs required for this top-specific generated core. Co-authored-by: Andreas Kurth <[email protected]> Signed-off-by: Alexander Williams <[email protected]>
Uniquify VLNVs, and ensure intra-ipgen-output VLNV references are real (i.e. not virtual). In addition, add direct dependencies to the top's expected packages. Some IPs depend on flash_ctrl's virtual VLNVs, so drop only the unused ones. Make the generated flash_ctrl depend on no virtual VLNVs. Co-authored-by: Andreas Kurth <[email protected]> Signed-off-by: Alexander Williams <[email protected]>
Uniquify VLNVs, and ensure intra-ipgen-output VLNV references are real (i.e. not virtual). In addition, add direct dependencies to the top's expected packages. Co-authored-by: Andreas Kurth <[email protected]> Signed-off-by: Alexander Williams <[email protected]>
Make rv_plic like the other ipgen cores and use instance_vlnv. Co-authored-by: Andreas Kurth <[email protected]> Signed-off-by: Alexander Williams <[email protected]>
Uniquify the VLNVs for top-level lint and sim targets. Use the direct VLNVs for dependencies to top-specific cores. Add a special core for earlgrey's AST to bring in the direct VLNVs required to guarantee simulation against earlgrey parameters. Adjust lint and simulation configs to map to these adjusted VLNVs. Co-authored-by: Andreas Kurth <[email protected]> Signed-off-by: Alexander Williams <[email protected]>
Signed-off-by: Andreas Kurth <[email protected]>
|
CHANGE AUTHORIZED: hw/top_earlgrey/ip_autogen/alert_handler/data/alert_handler.hjson |
It'd be best to look at each commit individually, but the summary is that the changes move towards the following:
fusesoc runis should now (hopefully) provide a deterministic set of dependencies with no cycles in the graph.Some minor changes have been made to begin making alert_handler optional, since English Breakfast doesn't use it. pwrmgr's use of
alert_pkghas been changed to simple parameters, which currently only use the defaults (since alert_handler's original values are not actually parameterizable right now anyway). There is much more work that could be done across other IPs, but I'm not equipped to handle feature enablement / disablement for them. 😄The use of globally-accessible constants is pervasive in our code base, and it probably ought to have a more critical eye checking for abuse. Also, the naming conventions here are likely to grate, considering we throw all the weight into the Name portion of the VLNV and barely use the Library. Suggestions for changes are expected, haha.
This takes some bits from #25703 and #25747, and hopefully it provides a more reviewable history and breakdown.