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

Split IOBinders into IOBinders and HarnessBinders | punch out clocks to harness for simwidgets and bridges #670

Merged
merged 12 commits into from
Sep 14, 2020

Conversation

jerryz123
Copy link
Contributor

@jerryz123 jerryz123 commented Sep 4, 2020

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

@jwright6323
Copy link
Contributor

Cool, this seems a lot less repetitive

Copy link
Contributor

@abejgonzalez abejgonzalez left a 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.

@abejgonzalez
Copy link
Contributor

While this is certainly late, I think it might worth having the IOBinders work with a LazyModule TestHarness. For example, in the fpga-shells case, the TestHarness cannot extend the VCU118Shell since it is a LazyModule (technically things compile... however the harness functions are determined in the 2nd phase of Diplomacy so only non-diplomatic items can be put in the harness function). Probably out of scope of this PR though...

@jerryz123
Copy link
Contributor Author

While this is certainly late, I think it might worth having the IOBinders work with a LazyModule TestHarness. For example, in the fpga-shells case, the TestHarness cannot extend the VCU118Shell since it is a LazyModule (technically things compile... however the harness functions are determined in the 2nd phase of Diplomacy so only non-diplomatic items can be put in the harness function). Probably out of scope of this PR though...

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 VCU118Shell, and how would HarnessBinders interact with them, supposing that HarnessBinders supported being evaluated in a Lazy context?

@abejgonzalez
Copy link
Contributor

While this is certainly late, I think it might worth having the IOBinders work with a LazyModule TestHarness. For example, in the fpga-shells case, the TestHarness cannot extend the VCU118Shell since it is a LazyModule (technically things compile... however the harness functions are determined in the 2nd phase of Diplomacy so only non-diplomatic items can be put in the harness function). Probably out of scope of this PR though...

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 VCU118Shell, and how would HarnessBinders interact with them, supposing that HarnessBinders supported being evaluated in a Lazy context?

I need to do more experimenting but it seems like to "place" an Overlay which elaborates the top-level IOs of the TestHarness (and adds the tcl/*dc collateral), you need to create BundleBridgeSource of the ChipTop IOs and pass that into the Overlay "place" function. This requires you to have the harness functions be called in the 1st Diplomacy phase.

In the old version of the IOBinders, there was nothing stopping you calling the functions in the foreach in the 1st Diplomacy area. The problem was that the harnessFunctions array was appended to in the ChipTop in the module implementation. I don't know exactly how it works in this PR since that problematic code was deleted... but I assume the functionality is the same as before.

Copy link
Contributor

@colinschmidt colinschmidt left a 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.

.circleci/defaults.sh Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
docs/Customization/IOBinders.rst Outdated Show resolved Hide resolved
docs/Customization/IOBinders.rst Outdated Show resolved Hide resolved
docs/Customization/IOBinders.rst Outdated Show resolved Hide resolved
generators/chipyard/src/main/scala/HarnessBinders.scala Outdated Show resolved Hide resolved
generators/chipyard/src/main/scala/IOBinders.scala Outdated Show resolved Hide resolved
generators/chipyard/src/main/scala/IOBinders.scala Outdated Show resolved Hide resolved
generators/chipyard/src/main/scala/IOBinders.scala Outdated Show resolved Hide resolved
generators/chipyard/src/main/scala/IOBinders.scala Outdated Show resolved Hide resolved
Copy link
Contributor

@davidbiancolin davidbiancolin left a 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.

@jerryz123
Copy link
Contributor Author

jerryz123 commented Sep 9, 2020

@davidbiancolin The latest commit removes most of the type matching in the HarnessBinders. The case where a HarnessBinder / IOBinder pair match the same DigitalTop trait, but expect different ChipTop IO bundle types, still manifests as a runtime ClassCastException, rather than a compile-tile type error. However, I believe the current implementation minimizes the number of classes/bundles which must be defined to support the binders system for arbitrary digital IOs.

@colinschmidt
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor

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)

@jerryz123
Copy link
Contributor Author

Firesim bumped and AGFIs regenerated.
Still waiting on approvals for firesim/icenet#27, firesim/firesim#625

@jerryz123 jerryz123 merged commit 16c8011 into dev Sep 14, 2020
@jerryz123 jerryz123 deleted the harness-refactor branch September 17, 2020 22:18
@alonamid alonamid mentioned this pull request Nov 24, 2020
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.

6 participants