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

[englishbreakfast] Move to ordinary topgen flow #25747

Merged
merged 2 commits into from
Jan 10, 2025
Merged

Conversation

a-will
Copy link
Contributor

@a-will a-will commented Dec 24, 2024

Now that VLNVs have been uniquified, move English Breakfast to be checked in and using the ordinary topgen flow.

Note that this doesn't get rid of prepare_sw.py. That is a different layer. :)

@Razer6
Copy link
Member

Razer6 commented Dec 24, 2024

That's great! /cc @pamaury also for SW

@pamaury
Copy link
Contributor

pamaury commented Dec 25, 2024

Just FYI, I had the ok from @vogelpi to break English Breakfast in master if we need to, in particular if we want to first turn EB into a proper top and later fix the SW for it. In #24453 I was able to make it work but it required many small changes to the SW and required multitop. It might be a good idea to discuss EB in the relevant WG and try to sync the multitop SW and EB work so that we don't break it for too long (also we would need to make an announcement).

@a-will a-will force-pushed the cw305 branch 3 times, most recently from 8fbaa54 to 44811cc Compare December 27, 2024 16:09
@a-will
Copy link
Contributor Author

a-will commented Dec 27, 2024

One interesting unsolved problem (which again would make adding VLNVs on the command line nice...): The ipgen'd tops need definitions from packages from other ipgen'd IPs to be complete for lint checks. The block-level for clkmgr needs a pwrmgr to also be generated.

@a-will a-will force-pushed the cw305 branch 4 times, most recently from 43371c2 to e66a5b7 Compare December 27, 2024 23:58
@a-will
Copy link
Contributor Author

a-will commented Dec 28, 2024

Just FYI, I had the ok from @vogelpi to break English Breakfast in master if we need to, in particular if we want to first turn EB into a proper top and later fix the SW for it. In #24453 I was able to make it work but it required many small changes to the SW and required multitop. It might be a good idea to discuss EB in the relevant WG and try to sync the multitop SW and EB work so that we don't break it for too long (also we would need to make an announcement).

@pamaury The goal for this PR would be to merely align the englishbreakfast hardware with the topgen flows we use regularly (and adjust topgen and ipgen'd cores to make that possible). So, the prepare_sw.py script would still be used until multi-top work lands, and there would be no changes for the software flow until then. Also, the hardware logic shouldn't actually change here, either! It's merely an alignment of the pre-bazel tooling.

Some of the changes here are also relevant to darjeeling, since the VLNV bits for ipgen'd cores are likely needed to have multiple tops at all.

Random aside (not directed at anyone in particular): Perhaps someday in the future, topgen-fusesoc.py can return with more planning. We probably could partially generate fusesoc core files, but that script was only intended as a stopgap until the relevant cores were converted to ipgen.

@a-will a-will force-pushed the cw305 branch 4 times, most recently from b02c5dd to a9b4833 Compare December 28, 2024 01:37
@a-will
Copy link
Contributor Author

a-will commented Dec 28, 2024

Now I find myself fiddling with make_new_dif and friends, and... it strikes me that our software really needs to go in an obvious opentitan-specific prefix. Otherwise, out-of-tree usage gets annoying. Even so, I'm not sure some of the test generation for make_new_dif is going to scale properly--At least to me, right now, it feels really awkward with all the assumptions.

This investigation started with darjeeling's soc_proxy not having DIFs yet, since the autogen'd PLIC tests started referring to non-existent bazel targets.

@a-will a-will force-pushed the cw305 branch 5 times, most recently from e431979 to ae6066e Compare December 28, 2024 19:26
@a-will
Copy link
Contributor Author

a-will commented Dec 28, 2024

I think I may need to actually propose and implement that fusesoc change to add VLNVs via the command line. It's become very tedious to gather all the required cores that implement virtual VLNVs for every "top" core that is part of ipgen outputs.

This is all before we get rid of primgen, which merely adds yet another core to the list. The dependency hell comes from a weaving of different pieces from different IPs, several of which are the output of ipgen, and there are dependencies that aren't even part of the ip_interfaces library. (like the use of top_pkg which seems to be missing in the depend list for some IPs)

What that command-line option can do much better than core files is avoid dependency cycles. Core files alone can get the job done, as shown in this PR, but to avoid the possibility of cycles, you have to be very disciplined and only include the virtual provider cores in non-default targets. The core files are also less flexible for swapping prims, and this PR reveals we have many different top modules across the various targets, so need all of those core files to pull in the expected virtual provider cores.

@a-will a-will force-pushed the cw305 branch 3 times, most recently from 88ebb8b to f3a07da Compare December 29, 2024 06:42
@a-will
Copy link
Contributor Author

a-will commented Dec 29, 2024

Finally all green! Here's the strategy that led to it:

  • All constants (e.g. in packages) should be leaf cores with no dependencies (except for other packages, taking care to avoid dependency cycles). No Verilog modules should be part of those fusesoc cores or their dependencies.
  • For a given IP template, all core files in the tree must be templated and given VLNVs from instance_vlnv()
  • For any IPs that reference core files in a separate IP template tree, the IP template and user must use a virtual VLNV reference.
  • To ensure the correct virtual provider VLNV is chosen for a given templated core, each "top" core of a templated IP depends on a separate, manually-written core that fills in those dependencies, conditioned on the fileset_top flag. A "top" core is a fusesoc core that has targets that would require it to be used as the "system" for "fusesoc run."

That last bullet was very tedious to get right.

English Breakfast hardware was pretty straightforward to bring up (for the existing Verilator and CW305 applications). Software test target generation was cut for any top that isn't Earl Grey, since this was broken for other tops, and fixing that is part of the multi-top SW effort (i.e. not relevant to the hardware work here).

@a-will
Copy link
Contributor Author

a-will commented Dec 29, 2024

Some TODOs:

  • Formally define a topgen-supported MCU subsystem as containing exactly one clkmgr, pwrmgr, and rstmgr, plus cores defined in the top hjson
  • Make alert_handler truly optional, as it brings in too many dependencies for a small MCU like English Breakfast
    • Remove pwrmgr's dependency on alert_handler_pkg.
      • Use ordinary parameters for N_ESC_SEV and PING_CNT_DW
    • Remove rstmgr's dependency on alert_handler_pkg.
      • This is currently used for alert_crashdump_t, which depends on various alert_handler parameters. However, rstmgr only treats it as a packed array of bits, so there's no need for this complex type. (This has been punted! englishbreakfast simply directly depends on earlgrey's package for the type right now)
  • Consolidate the virtual provider cores into just the one (which only pulls in leaf cores with packages / interfaces)

@Razer6
Copy link
Member

Razer6 commented Dec 29, 2024

  • Formally define a topgen-supported MCU subsystem as containing exactly one clkmgr, pwrmgr, and rstmgr, plus cores defined in the top hjson

I don't think this restriction is needed. With the support of unmanaged external clocks and resets, you can run MCUs without that IPs. In fact, I see some some running ;-)

Make alert_handler truly optional, as it brings in too many dependencies for a small MCU like English Breakfast

An easy fix might to make the alerts outgoing and tie them off at the uncore.

@a-will
Copy link
Contributor Author

a-will commented Dec 29, 2024

  • Formally define a topgen-supported MCU subsystem as containing exactly one clkmgr, pwrmgr, and rstmgr, plus cores defined in the top hjson

I don't think this restriction is needed. With the support of unmanaged external clocks and resets, you can run MCUs without that IPs. In fact, I see some some running ;-)

Ah, fair enough--That probably wasn't a complete enough sentence. By "a" topgen-supported subsystem, I meant that it is one clear admissible architecture. Currently, clkmgr and rstmgr depend on pwrmgr, though, so to have either of the former two, you would need the latter. Otherwise, we need to do more development / fixing.

In general, though, it would be good to be able to readily explain what topgen expects and what it will generate from the input. We can expand capabilities from there. :)

Make alert_handler truly optional, as it brings in too many dependencies for a small MCU like English Breakfast

An easy fix might to make the alerts outgoing and tie them off at the uncore.

You mean in addition to the items I have listed? English Breakfast already doesn't have an alert_handler, but pwrmgr and rstmgr call it out as a dependency unnecessarily, so that line item was just about dealing with the current IP organization.

@a-will
Copy link
Contributor Author

a-will commented Dec 30, 2024

I'll note also that this restriction is one that forecloses having multiple outputs of ipgen with different parameterizations in the same type:

  • For any IPs that reference core files in a separate IP template tree, the IP template and user must use a virtual VLNV reference.

To move away from this, we'd have to make topgen / ipgen aware of inter-IP dependencies and not use virtual VLNV references, since there can be only one provider for a virtual VLNV. Then the direct dependencies would have to be generated in fusesoc core files (as well as ensuring there are no clashes with names in the global namespace).

I feel like virtual cores are a kludge for inter-IP dependencies, but removing the limit of having only one use of a template is a bit of a project of its own.

Darjeeling's software tests only avoided overwriting the earlgrey tests
by nature of sorting earlier alphabetically. Move autogenerated tests to
the top-specific output directories. Fix up paths to match.

Signed-off-by: Alexander Williams <[email protected]>
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]>
@a-will a-will changed the title WIP: Uniquify core VLNVs + promote englishbreakfast to full topgen [englishbreakfast] Move to ordinary topgen flow Jan 9, 2025
@a-will a-will marked this pull request as ready for review January 10, 2025 01:28
@a-will a-will requested review from HU90m and vogelpi and removed request for a team and msfschaffner January 10, 2025 01:28
Copy link
Contributor

@vogelpi vogelpi left a comment

Choose a reason for hiding this comment

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

This looks good to me, there are two CI steps failing but this is due to the number of files being touched. The PR doesn't change any RTL used in Earlgrey.

Thanks @a-will for doing this work and for keeping English Breakfast alive!

@a-will a-will merged commit 7de34fe into lowRISC:master Jan 10, 2025
36 of 38 checks passed
@a-will a-will deleted the cw305 branch January 10, 2025 21:06
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.

4 participants