-
Notifications
You must be signed in to change notification settings - Fork 766
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
[top] top_earlgrey_* organization #295
Comments
This seems very reasonable. We have certainly made functional core designs, and then wrapped them with process/platform dependent padring files which hold all the pads, and process dependent guts. In fact this is often extended such that any process dependent cells get pulled to the top level (SRAMs, or mixed signal IPs). For example, in ASIC's SRAMs are not just process dependent, but often depend on the integrator, whereas in some FPGA's there may be different IP wrappers for a particular SRAM configuration that are dependent on the revision of the design tool. |
The approach SGTM. My only comment is to ensure top-level DV TB DUT is top_earlgrey in the new structure, so connections between top_earlgrey_core to padring, etc. are covered. |
I am also OK with having |
The current structure allows you to have different toplevel ports for different targets, and have one place to instantiate target/device specific modules. If you remove that, you end up with
Please see https://github.com/lowRISC/stwg-base/issues/89 for the full discussion that happened when we choose the current structure. |
So regarding ports, part of why we are doing this is because we are putting
in infrastructure to define the ports / pinmux / pads as part of
top_earlgrey.hjson.
And yes, regarding other modules such as top level analog, clock
generation, my opinion is that everything would need to be wrapped in a
prim_* like architecture. The goal of this really is to focus the number
of DUTs we verify across different targets to unify as much as we can.
If in the end we still need a different top for a specific purpose, I think
that can still be supported.
…On Thu, Oct 3, 2019 at 10:23 AM Philipp Wagner ***@***.***> wrote:
The current structure allows you to have different toplevel ports for
different targets, and have one place to instantiate target/device specific
modules. If you remove that, you end up with
- Ifdefs around toplevel ports, e.g. to support single-ended vs
differential clocks, etc.
- even more wrapping code prim-like modules.
Please see lowRISC/stwg-base#89
<https://github.com/lowRISC/stwg-base/issues/89> for the full discussion
that happened when we choose the current structure.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#295?email_source=notifications&email_token=AAH2RSQNJ424COCV73U7L3DQMYTCDA5CNFSM4I4B5C4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAI6DAQ#issuecomment-538042754>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAH2RST7TUXLR6LNDFD26MDQMYTCDANCNFSM4I4B5C4A>
.
|
On Thu, Oct 03, 2019 at 10:23:47AM -0700, Philipp Wagner wrote:
The current structure allows you to have different toplevel ports for different targets, and have one place to instantiate target/device specific modules. If you remove that, you end up with
- Ifdefs around toplevel ports, e.g. to support single-ended vs differential clocks, etc.
- even more wrapping code prim-like modules.
Please see for the full discussion that happened when we choose the current structure.
A part of this issue is somewhat related to the PINMUX/ PADS stuff. Eventually
OpenTitan needs analog pads/ or behavioral model pads inside top_earlgrey.
Combining ASIC and Nexys Video into the top module will help a lot to make them
integrated. Verilator is somewhat special I think as it has DPI interface. But
we could move the DPI to the top wrapper like tb in ASIC top test environment.
If not, we could end up having separate pad instances for asic and fpga. In my
opinion, at least we can minimize the changes by including target specific
verilog files outside of the top_earlgrey. ifdef could be minimal in that case.
|
Concerning the technology dependent modules I second @tjaychen in that I also think they should be abstracted using a However, @imphil is right that we have to be careful with IPs that have a different interface across different technologies. While the version of padmux / padctrl that we are currently working on can handle a parametric number of generic IOs, we may run into issues with clocking infrastructure IP (like the FPGA clock managers, differential clock signals), or blocks that rely on other pad functionality than just CMOS-style IO (like DDR memory controllers). Let's revisit https://github.com/lowRISC/stwg-base/issues/89 and discuss this in more detail in our next meeting (or if it takes to long, schedule a separate meeting just for that). |
A couple comments from my side:
@msfschaffner We do have a silicon meeting on Tuesday. Can you talk with Scott to see if we can discuss this topic there. Otherwise, please find a date/time for a dedicated short meeting on this. |
Thanks for the thoughts Philiipp / Michael.
Let me clarify some of my thoughts
- I prefer strongly to re-use as much code as possible across the
various platforms. So I tend to like to start very restrictive and see
where those assumptions breakdown, I also tend to like to include chip
connectivity as much as possible in that equation
- I agree that it will be difficult to find a common set of ports
across all targets. However the differences feel very small to me right
now (FPGA may have extra clock ports and different port names (USB),
verilator has dpi modules that belong in a more DV like testbench
construct), and I'd rather start with marking out those specific features
than maintaining an entirely separate module.
- The separate module in my opinion just have a way of getting out of
hand over time. Same could be said of ifdefs or other feature separation
mechanisms, but if they are all in one place, it will at least
be easier to
notice that a 'slow death' is happening
- One wishlist item for me is re-using the DV environment on FPGA
builds, and I just think there's a better chance of that
happening if most
of the code, inclusive of the top, is the same. (Sri can
correct me if I'm
wrong, but I think DV is always best run at the absolute top and not just
the target independent portion)
- The separation of top / core in my mind is not so much about what is
target dependent or independent
- To me the line is more what makes a chip and what does not. By
that I mean I would like for the 'top_*_core' to be a module that can be
instantiated in a larger chip as a subsystem. While the top_* non-core
should be the final wrapper that inserts all the pieces that
make it a full
chip.
- So to me this means the top* non-core should include things like
pads, *MAY* include top level analog (oscillators, internal
regulators, charge pumps etc) but *NOT* things like memories and
analog blocks that can be functionally isolated (such as TRNG)
- There is a separate discussion here on what we think is the
right approach to absorb technology / vendor specific blocks
such as TRNG,
I have some of my own thoughts I've shared with Martin, but I
would like to
hear others as well.
- To some extent, I think the results of this discussion are not SUPER
critical, because I expect when partners come on board they will give
recommendations that actually fit their tapeout needs (for example, they
may want the primary DFT controller at a certain level, they may want to
actually lump all analog components in one place, they may want to create
their own top_* every single time), so it is in my opinion good to ensure
we do not pick something that makes it difficult to pivot later. Re-naming
top_earlgrey to top_earlgrey_core, and merging the tops where possible now
(with perhaps a few parameters and defines to mark out the different top
needs) feels like a decent middle-ground to me. If we discover that there
are actually way more features to separate, it will be easy to move back
also (although in that scenario, I think as you suggested, I'd vote for a
templated generate flow for the different tops)
…On Fri, Oct 4, 2019 at 8:35 AM msfschaffner ***@***.***> wrote:
Hey Philipp, thanks for your comments. I tend to agree with most of them,
but as you say more discussion is needed such that everybody is on the same
page here. I'll check with @sjgitty <https://github.com/sjgitty> and
@eunchan <https://github.com/eunchan> whether we have enough time in our
meeting next week.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#295?email_source=notifications&email_token=AAH2RSW6W7MUXWN4VE4YWY3QM5PFXA5CNFSM4I4B5C4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAMBLYI#issuecomment-538449377>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAH2RSRPF24ZZT65AQZCZJ3QM5PFXANCNFSM4I4B5C4A>
.
|
Thanks @imphil for linking lowRISC/stwg-base#89. That is helpful to understand the current state. I agree with @msfschaffner . We should avoid However, I don't understand how having fewer top-levels eases testing (especially since we are not talking about reducing the number of targets but rather having a single top-level supporting all targets). I would rather say what we want is to reduce the differences between the existing top-levels. Looking for example at the Nexys and ASIC top-levels, the main differences are the clocking and the pad control. As outlined above, I don't think we can unify the clocking. But why don't we just move the pad control into |
@vogelpi and yes, the padmux / pinmux IP will go a long way to solving this issue. @msfschaffner / @eunchan will be discussing this tomorrow, so it will be good to get more information then. As you say, the main differences are pad and clocks, and I felt that clocks could be unified with a prim_wrap + a single ifdef for whether there is an external clock (we may even ignore that for now since I'm making guesses about the final silicon). Lastly, I still prefer a single top because I strongly prefer DV to be able to cover a large majority of code across all platforms. Fairly recently (maybe a month ago), I noticed a connection error in the FGPA pin top that caused GDB to not fully work. If we have a top level test in DV that exercised that path for example, it would have been easily caught. I've also personally run into a lot of cases where things worked in DV and just did not in FPGA, and they were a pain to track down - so i've more or less developed a blind desire to keep things consistent between the two when possible. There will still be deviations to be sure, but like I mentioned before, I prefer to start very restrictive, and open up later if necessary. It may be good for others to chime in also. |
On prim_clock: I don't see a common interface we could standardize on. We have
I also don't see much value in spending effort of having a primitive with a shared interface. The clock is always configured target/board specific, and needed in the top-level file. So instantiating the right module there, instead of somehow passing config information to a primitive and instantiating that, doesn't strike me as a significant win. Regarding the FPGA pin case you mentioned: this was an example where we had different pinmux configurations between FPGA and other targets. If pinmux is able to handle this, there's no need to have this code in the FPGA-specific toplevel any more (and that's what we're aiming for). You'd still need to have DV to test this configuration, of course. |
regarding output, it's possible to have more than 1 correct? At least OpenTitan as spec'd by scott is meant to have both a primary clock and a fixed clock. But you are right, this pat will always be the same. Maybe I'm missing something, but when I look at the FPGA clocking, doesn't it just take an external clock and reset pin? I'm sure there are more configurations wrapped up underneath, but I don't really think it would be necessary to expose them at a wrapper level. Yes I do agree there could be different clock inputs, but to me that delta is small enough that I don't think it warrants a separate top. Now if the story becomes our design is big enough it needs to be split into multiple FPGAs, or that the FPGA needs a very specific set of pins that are different from the normal top, then I agree, a separate top makes sense. In our current scenario though, I really don't think there is much of a difference, and that's one of the reasons I would like to merge them. If they do become sufficiently different it is easy to split off again, and nothing we do now is meant to prevent that choice in the future. |
Boy, this one is getting long, but I didn't want to open a new issue. Cut/Pasting my
|
Before this issue moves forward, Can we get an agreement of |
yes that seems like the right thing to do to me.
…On Tue, Nov 12, 2019 at 5:20 PM Eunchan Kim ***@***.***> wrote:
Before this issue moves forward, Can we get an agreement of
top_earlgrey_verilator.sv to be moved to dv/ directory other than rtl/? I
am pretty sure the top_earlgrey_verilator module is for verification/
validation not the design.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#295?email_source=notifications&email_token=AAH2RSUV32XX5GLLUVYAORDQTNI5RA5CNFSM4I4B5C4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOED4Q5VY#issuecomment-553193175>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAH2RSWNDHOVE5WLDY2LRQDQTNI5RANCNFSM4I4B5C4A>
.
|
yes, I am also in favor of moving it into the |
Just adding a note that we should remember to implement the top level reorganization as discussed in December at some point. This issue is related: #892 |
Hey Michael,
I think either you or I drew up a diagram of what this should look like, we
should probably attack to the bug as a reference.
…On Fri, Jan 17, 2020, 18:11 Michael Schaffner ***@***.***> wrote:
Just adding a note that we should remember to implement the top level
reorganization as discussed in December at some point.
This issue is related: #892
<#892>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#295?email_source=notifications&email_token=AAH2RSUJTB6UBDMQWTZEPIDQ6JQLPA5CNFSM4I4B5C4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJJN6RA#issuecomment-575856452>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAH2RSXYOFWIEJGSN6CIOGDQ6JQLPANCNFSM4I4B5C4A>
.
|
@msfschaffner when you have a moment can you refresh this issue? Perhaps it is closable now, but we should at least refresh with the latest thinking. |
Question regarding top_earlgrey_verilator has popped up on the SW meeting. As a part of DIF we have at least one "sanity" test per DIF, which is ran on verilator via CI. At the moment Pinmux and Pad Control on verilator doesn't not have the same functionality as on FPGA (please see the related issue: #2707 ), namely padring is not instantiated, which prevents us from writing a comprehensive test at this time. How important it is for the MS4 I don't know, maybe @gkelly could clarify this for. |
Sorry @sjgitty, I completely missed this one. The top-level organization has in the meantime evolved quite a bit, but in a different way as we've sketched out above. Below is an update on the current plan and status. Current PlanBelow is a diagram from @tjaychen which reflects the current thinking. First of all, the chip-level top will eventually only contain:
Other IPs that need bus access (like the Since the As part of the For the Verilator testbench, the idea is to align it with the FPGA and ASIC tops as well, although there are a couple of hurdles that need to be overcome in order to enable simulation of the AST model, which currently contains constructs that are not supported by Verilator. Also, AST generates multiple clocks with different frequencies and we need to make sure this can be handled in the Verilator case (for instance, by setting them all to the same base clock fed in from the testbench using As indicated in the diagram above, we may rename @tjaychen, @eunchan let me know if I missed something above. Current StatusMost of the changes listed below are tasked for MS4.
other:
Related IssuesAnother issue that is related to the top-level organization is the actual configuration of the OpenTitan SoC. That may however not be entirely possible, especially if we intend to keep support for the lower cost development boards with Vivado WebPack support alive (the SoC configuration planned for the ASIC implementation does not fit on the nexysvideo, nor other such low-cost boards). Hence, we should start thinking about ways of supporting different system configurations - but that discussion merits its own thread (just wanted to bring this up here). |
@silvestrst - for notifications on new comments |
FYI, this status tracker has been updated. Several important updates RE toplevel organization, pinmux and templating have landed. |
latest status: ast has been merged into fpga and verilator. Verilator still has not merged into |
most of the work here is done. The remaining work is tracked in #6103 |
Currently, there are 3 overall top_earlgrey_* organizations
top_earlgrey_asic
top_earlgrey_verilator
top_earlgrey_nexysvideo
Each basically follows the structure of ...
top_earlgrey_*
-> top specific pin/pad logic
-> top specific reset / clock logic
-> top_earlgrey
Instead of maintaining 3 separate top wrappers, we'd like to suggest having only one functional top in the following organization
top_earlgrey
-> top_earlgrey_core (today's top_earlgrey)
-> pinmux / padring
-> other specific top level logic
This will allow us to stop maintaining 3 different versions, and also concentrate testing.
To accomplish this, there are a few things that need to be done
Please let us know what you think.
One alternative approach is to no separate top_earlgrey and top_earlgrey_core, but instead make the top level template flexible enough to know when to generate what. That's something we should think about as well
@asb
@imphil
@vogelpi
@eunchan
@msfschaffner
@sriyerg
@aytong
@martin-lueker
@sjgitty
The text was updated successfully, but these errors were encountered: