-
Notifications
You must be signed in to change notification settings - Fork 660
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
Split IOBinders into IOBinders and HarnessBinders | punch out clocks to harness for simwidgets and bridges #670
Conversation
881031b
to
0cf02ae
Compare
… to harness for simwidgets and bridges
0cf02ae
to
0f50e4d
Compare
Cool, this seems a lot less repetitive |
876a8f9
to
ab21c53
Compare
generators/chipyard/src/main/scala/config/TutorialConfigs.scala
Outdated
Show resolved
Hide resolved
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 barring the comments on the docs.
While this is certainly late, I think it might worth having the |
I think out-of-scope for this PR, but I agree this is an important use case to consider. What is diplomatically elaborated in the |
I need to do more experimenting but it seems like to "place" an In the old version of the |
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 had a lot of comments but nothing show stopping.
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 can't say i like the current implementation as we're losing a bunch of static type safety as we ascend through the layers. Furthermore, dynamic type problems will likely only manifest after the bulk of the dut has been elaborated...
I think a cleaner solution from a type-system perspective would have the Chip-side I/O binder return a container class of a specific type with references to all of the important pieces that a harness-side binder could draw from (as opposed to a Seq[Data]). This is essentially analagous to the digital top trait, but for chip top -- except it does not expect to compose into a larger cake. This class would have members for only the I/O that should exist, and they'll have the right types. If the harness binder wants to introspect on stuff done that can't be inferred from I/O, references to that can be passed out too. Additionally, it can also provide a reference to the system if that harness-binder needs access to stuff that lives there, collapsing the 3-tuple into a 2-tuple. It also has the potential benefit of decoupling the harness-side binder from the digital-top-side trait if that's desired.
The cost as i see it is more boiler plate but that might be a wash because it cleans up the harness side binder.
@davidbiancolin The latest commit removes most of the type matching in the |
d6bca76
to
8f9574f
Compare
Wow the typed version looks so much better thanks for changing that @jerryz123 and thanks @davidbiancolin for holding this to a high standard. |
gpio.pins.zipWithIndex.map({ case (pin, j) => | ||
val g = IO(Analog(1.W)).suggestName(s"gpio_${i}_${j}") | ||
val iocell = genFn().suggestName(s"iocell_gpio_${i}_${j}") | ||
val iocell = system.p(IOCellKey).gpio().suggestName(s"iocell_gpio_${i}_${j}") | ||
iocell.io.o := pin.o.oval |
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.
We dropped some of the gpio pin signals here. Perhaps we should augment the genericIOCells with pue
and ds
?
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.
When we discussed this previously those were deemed "Extra" signals that we'd add along with other non-logical signals (because they don't affect the 1/0 logical state of the IO)
55b2082
to
a5385c0
Compare
Firesim bumped and AGFIs regenerated. |
IOBinders now generate only chip-side hardware. Specifically, IOBinders instantiate IOCells for chip-like IOs, and directly punch through the signals for simulation-only IOs.
A new set of HarnessBinders generate hardware which sits in the TestHarness (or the FireSim "harness"). A portMap created during evaluation of the IOBinders holds references to ChipTop IOs, and is used during HarnessBinder evaluation to pass references to the corresponding IOs for each HarnessBinder.
FireSim's BridgeBinders are also moved into the FireSim "harness" (outside of ChipTop, where they previously lived), and now override existing HarnessBinders. FireSim uses the same IOBinders as SW-sim, so IOCell models are also instantiated for FireSim
Due to the BridgeBinders refactoring, it was also necessary to punch out clocks to BridgeBinders/HarnessBinders for simulation-only blocks. This fixes multiclock simulations
Related issue:
Type of change: new feature
Impact: rtl change
Release Notes