Use ipgen to generate rv_plic #8431

imphil · 2021-09-29T17:41:08Z

This PR converts rv_plic to an IP template, and instantiates it through ipgen. Since rv_plic is the first IP block to be converted, the PR also contains some preparatory commits. Only the last commit ([rv_plic] Produce top_earlgrey instance of rv_plic with ipgen) is the actual conversion. Reviewers are encouraged to look at this commit especially.

Since most of the conversion job is renaming files, and all files in hw/top_earlgrey/ip_autogen are auto-generated, the diff isn't as beautiful as it could be -- but the manual work is still rather limited.

This conversion is effectively a clean-up, no functional change on OpenTitan is expected.

[rv_plic] Produce top_earlgrey instance of rv_plic with ipgen

This rather large commit makes rv_plic an IP template, and then uses
ipgen to instantiate the block with the right parametrization for
top_earlgrey.

In contrast to the previous approach, `hw/top_earlgrey/ip_autogen`
now contains a full copy of rv_plic under a unique FuseSoC core
name, `lowrisc:opentitan:top_earlgrey_rv_plic`.

Unfortunately, doing so requires a fair amount of reshuffling, which
cannot be easily split into individual commits while keeping the whole
tree building. Here's what was done:

* Move `ip/rv_plic` to `ip_templates/rv_plic`.
* Remove the `reg_rv_plic.py` tooling, which is now replaced by ipgen.
* Change `topgen.py` to generate the toplevel-specific instance of
  `rv_plic` through ipgen.
* Adjust references in the documentation as necessary.
* Adjust the software build as necessary.
* The FPV testbench is now only run for the Earl Grey-instantiated IP
  block, there is no more "generic" testbench. Update all references
  pointing to the testbench.

imphil · 2021-10-01T16:24:46Z

@cindychip This PR reshuffles the FPV testbench a bit, and I can't really test the result. Would you mind checking out this PR and run FPV on it?

The relevant part from the commit message:

The FPV testbench is now only run for the Earl Grey-instantiated IP
block, there is no more "generic" testbench. Update all references
pointing to the testbench.

The FPV FuseSoC core is now named lowrisc:opentitan:top_earlgrey_rv_plic_fpv.

tjaychen · 2021-10-04T21:18:11Z

util/topgen.py

-                    "Falling back to template %s for initial validation." %
-                    str(hjson_file))
+            # validation, use the Hjson file with default values.
+            # TODO: All of this is a rather ugly hack that we need to get rid


@imphil sorry i'm a bit late to the party.
Does this comment mean hjson's won't be templated in the future? or that they will be templated in a specific way?

First of all: in this PR and the first stage of ipgen work, nothing changes here from how we did it before: we use the default template in the first run, and then the real template in the second run of topgen.

In the medium-term future, I'd hope that we can avoid string-templating Hjson files. Instead, it would be great if we can use what are template variables now as proper variables in Hjson files. But all of this is for another day.

ah yeah i think the main thing we run into there is when we using a templating variable to create different versions of a register.. we don't really support the idea of nested multi-reg right now.

tjaychen · 2021-10-04T21:24:10Z

hw/top_earlgrey/formal/top_earlgrey_fpv_cfgs.hjson

-               name: rv_plic_fpv
-               fusesoc_core: lowrisc:fpv:rv_plic_fpv
+               name: top_earlgrey_rv_plic_fpv
+               fusesoc_core: lowrisc:opentitan:top_earlgrey_rv_plic_fpv


the middle portion here "opentitan" seems a bit different from before.

Yes, it's different, but intentional: these IP blocks are specific to OpenTitan, they are not generic lowRISC IP. I therefore put them into the "opentitan" library (that's what the second part of the Vendor-Library-Name-Version (VLNV) string stands for).

i guess the part where this is confusing to me is because all the stuff under hw/ip and hw/ip_autogen are all under the opentitan repo. So in a way it feels like they are all specific to opentitan.

I actually thought you would want something like lowrisc:top_earlgrey:rv_plic_fpv to show their specificity to the top level while maintaining a more generic function. What do you think?

I know we also talked about the oddball dependencies we have now between certain modules..that probably plays a factor as well.

The "lowrisc" namespace is actually for all things lowRISC produces, which is more than OpenTitan. We for example share IP blocks with Ibex and other things we're (now or in the future are/might be) working on.

We had for a long time plans to split out the "common" lowRISC IP into a separate repository (e.g. in discussions with Scott and Mark ages ago). We never followed through with that due to the churn it would create for the OpenTitan project. Instead, we went for using the opentitan repository as "source", and vendor the IP blocks into other repositories, e.g. Ibex (here).

So while it is the most ergonomic choice right now to keep all IP in the opentitan repository, I'd like to use the opportunity to get the naming and namespacing more aligned with this vision and avoid a potential massive renaming down the line.

ah okay that's a fair point. The common modules being inside the opentitan repo is a bit odd and i imagine will cause issues elsewhere as well.

btw @imphil , let's say in the long term world we split hw/ip into its own common repository, would the hw/ip_template stuff also go there? Technically it feels like it should right? Those are not necessarily specific to opentitan.

Yes, that would be my current take on it.

yeah that sounds good and very consistent.
thanks!

tjaychen · 2021-10-04T21:42:09Z

this looks pretty good to me overall, although i think things are big enough that I wouldn't be able to tell if something would get majorly broken until we merged.

I've left a comment above since i'm a bit confused about the opentitan naming.

rswarbrick

This looks like a great improvement: nice!

msfschaffner

Thanks @imphil and sorry for being late to the party here.

LGTM in general - however I found a couple of issues when piping this through the FPV flow, see comments (@cindychip is out on vacation at the moment).

hw/top_earlgrey/formal/top_earlgrey_fpv_cfgs.hjson

msfschaffner · 2021-10-06T01:30:32Z

hw/top_earlgrey/ip/rv_plic/data/autogen/BUILD

-    srcs = [
-        "rv_plic.hjson",
-    ],
-)


it looks like this removes bazel build rules... we probably have to leave these in place (or generate them) since this may otherwise break the new bazel-based SW build flow (CC @drewmacrae to confirm that this is the case).

All Bazel-related things are unsupported and experimental for as long as we don't even have agreement on using Bazel going forward; I cannot even test it. This will need to be sorted out up by the people experimenting with Bazel currently after the fact, unfortunately.

Thanks for flagging this, I have a PR to fix it, if you have bazel installed and you're interested you can test the bazel rules with bazel test //...

I still need to write a getting started doc for it. If you don't have it installed I can point to instructions here: https://docs.bazel.build/versions/main/install-ubuntu.html

hw/ip/rv_plic/fpv/rv_plic_component_fpv.core

hw/ip_templates/rv_plic/fpv/rv_plic_fpv.core.tpl

msfschaffner · 2021-10-06T01:47:13Z

hw/top_earlgrey/ip_autogen/rv_plic/data/rv_plic.hjson

-// Licensed under the Apache License, Version 2.0, see LICENSE for details.
-// SPDX-License-Identifier: Apache-2.0
-//
-// ------------------- W A R N I N G: A U T O - G E N E R A T E D   C O D E !! -------------------//


could this potentially use the autogen header library that @sriyerg has added recently?

None of the files in the ip_autogen folder have autogen headers:

All files in ip_autogen are auto-generated, I don't want to give the impression that only some of them are.

Adding headers to all files is tricky: ipgen doesn't know and care about file types; adding generated headers would require per-extension logic to e.g. use the right types of comments.

We were talking about symlinking some files in ip_autogen to ip_templates, which would prevent added headers.

I'd therefore much prefer developers getting used to the fact that all in ip_autogen (as in other autogen folders) is autogenerated and shouldn't be manually edited. (And ultimately, maybe we can get rid of the autogenerated checked-in files completely, but that's a separate discussion.)

fair enough, let's leave this as is then.

Add the ability to produce IP blocks through ipgen instead of having custom logic within topgen. In this commit this logic isn't use yet. A difference between topgen-generated IP blocks and ipgen-generated ones is the slightly different directory structure: ipgen does produce a full IP block directory, matching a "regular" IP block, and does not include "autogen" subdirectories. Signed-off-by: Philipp Wagner <[email protected]>

The hw/ip_templates directory is meant to host IP templates, which can be used after they have been rendered through ipgen. Since those templates may contain partial FuseSoC core files, but are not meant to be used as template, we add a FUSESOC_IGNORE file, excluding the directory from the core search path. Signed-off-by: Philipp Wagner <[email protected]>

Signed-off-by: Philipp Wagner <[email protected]>

This rather large commit makes rv_plic an IP template, and then uses ipgen to instantiate the block with the right parametrization for top_earlgrey. In contrast to the previous approach, `hw/top_earlgrey/ip_autogen` now contains a full copy of rv_plic under a unique FuseSoC core name, `lowrisc:opentitan:top_earlgrey_rv_plic`. Unfortunately, doing so requires a fair amount of reshuffling, which cannot be easily split into individual commits while keeping the whole tree building. Here's what was done: * Move `ip/rv_plic` to `ip_templates/rv_plic`. * Remove the `reg_rv_plic.py` tooling, which is now replaced by ipgen. * Change `topgen.py` to generate the toplevel-specific instance of `rv_plic` through ipgen. * Adjust references in the documentation as necessary. * Adjust the software build as necessary. * The FPV testbench is now only run for the Earl Grey-instantiated IP block, there is no more "generic" testbench. Update all references pointing to the testbench. Signed-off-by: Philipp Wagner <[email protected]>

imphil · 2021-10-06T11:58:43Z

Thanks for trying the FPV stuff, @msfschaffner. Would you mind giving it another try to see if the latest push fixed things?

msfschaffner

Ok it looks like the FPV setup works again now.

There are some failing assertions - but they seem to have been failing before as well so this is unrelated.

imphil requested review from asb, cindychip, eunchan, mcy, moidx, msfschaffner, sriyerg, tjaychen and weicaiyang as code owners September 29, 2021 17:41

imphil removed request for asb, tjaychen, eunchan, mcy, moidx, cindychip, msfschaffner, sriyerg and weicaiyang September 29, 2021 17:41

imphil force-pushed the ipgen-plic branch 2 times, most recently from 043a046 to 32b35b3 Compare September 29, 2021 21:52

imphil mentioned this pull request Sep 29, 2021

[tracking] Use ipgen to produce templated IPs #8440

Closed

17 tasks

imphil force-pushed the ipgen-plic branch from 32b35b3 to 3b225f0 Compare September 30, 2021 20:59

imphil requested review from tjaychen and rswarbrick September 30, 2021 21:05

imphil changed the title ~~[DRAFT] Use ipgen to generate rv_plic~~ Use ipgen to generate rv_plic Sep 30, 2021

imphil requested review from cindychip, eunchan, msfschaffner and sriyerg September 30, 2021 21:06

imphil force-pushed the ipgen-plic branch from 3b225f0 to 6ae33cd Compare October 1, 2021 09:57

imphil force-pushed the ipgen-plic branch 3 times, most recently from a79b699 to bf6b8e1 Compare October 1, 2021 16:17

imphil force-pushed the ipgen-plic branch 3 times, most recently from e2d1d28 to 5e0bdac Compare October 4, 2021 12:00

tjaychen reviewed Oct 4, 2021

View reviewed changes

tjaychen approved these changes Oct 4, 2021

View reviewed changes

rswarbrick approved these changes Oct 5, 2021

View reviewed changes

imphil force-pushed the ipgen-plic branch from 5e0bdac to 0d3d635 Compare October 5, 2021 13:53

msfschaffner reviewed Oct 6, 2021

View reviewed changes

imphil added 4 commits October 6, 2021 12:56

[doc] Add auto-generated top_earlgrey files in doc

c9f9f4d

Signed-off-by: Philipp Wagner <[email protected]>

imphil force-pushed the ipgen-plic branch from 0d3d635 to d700905 Compare October 6, 2021 11:56

msfschaffner self-requested a review October 7, 2021 00:21

msfschaffner approved these changes Oct 7, 2021

View reviewed changes

imphil merged commit c720ac8 into lowRISC:master Oct 7, 2021

hcallahan-lowrisc mentioned this pull request Feb 4, 2024

[doc] CIP interrupt doc updates #21126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ipgen to generate rv_plic #8431

Use ipgen to generate rv_plic #8431

imphil commented Sep 29, 2021 •

edited

Loading

imphil commented Oct 1, 2021

tjaychen Oct 4, 2021

imphil Oct 4, 2021

tjaychen Oct 4, 2021

tjaychen Oct 4, 2021

imphil Oct 4, 2021

tjaychen Oct 4, 2021 •

edited

Loading

imphil Oct 4, 2021

tjaychen Oct 4, 2021

tjaychen Oct 5, 2021

imphil Oct 5, 2021

tjaychen Oct 5, 2021

tjaychen commented Oct 4, 2021

rswarbrick left a comment

msfschaffner left a comment

msfschaffner Oct 6, 2021

imphil Oct 6, 2021

drewmacrae Oct 7, 2021

msfschaffner Oct 6, 2021

imphil Oct 6, 2021

msfschaffner Oct 7, 2021

imphil commented Oct 6, 2021

msfschaffner left a comment

Use ipgen to generate rv_plic #8431

Use ipgen to generate rv_plic #8431

Conversation

imphil commented Sep 29, 2021 • edited Loading

imphil commented Oct 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjaychen Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjaychen commented Oct 4, 2021

rswarbrick left a comment

Choose a reason for hiding this comment

msfschaffner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imphil commented Oct 6, 2021

msfschaffner left a comment

Choose a reason for hiding this comment

imphil commented Sep 29, 2021 •

edited

Loading

tjaychen Oct 4, 2021 •

edited

Loading