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

Switch PRCI to HarnessBinder/IOBinders #900

Merged
merged 2 commits into from
Oct 1, 2021
Merged

Switch PRCI to HarnessBinder/IOBinders #900

merged 2 commits into from
Oct 1, 2021

Conversation

jerryz123
Copy link
Contributor

Type of change: new feature

Impact: rtl change

Release Notes
This change makes PRCI more "Chipyard-like". A system trait contains PRCI control registers to be able to independently control clock/reset to the tiles.
The PRCI graph is driven in the IOBinder, which contains different types of clock-driving patterns (FireSim vs RTL-sim vs FPGA-proto vs tapeout).
Harness binders can pass in any signals that the IOBinder clock driver might need, as usual.

The default ChipyardPRCI implementation contains reset-ctrl registers to allow MMIO to bring harts in and out of reset.
The default implementation also allows MMIO to selectively clock-gate harts (this feature is disabled in FireSim).

@jerryz123
Copy link
Contributor Author

Wow I forgot I made this PR. Can we get this looked at before I forget more about what happened here. @davidbiancolin @abejgonzalez

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.

I'll look at the FireSim code later but the CY code seems reasonable. Have you tried running this on the FPGA configs?

@@ -324,6 +325,21 @@ class WithSimDromajoBridge extends ComposeHarnessBinder({

class WithTieOffCustomBootPin extends OverrideHarnessBinder({
(system: CanHavePeripheryCustomBootPin, th: HasHarnessSignalReferences, ports: Seq[Bool]) => {
ports.foreach(_ := false.B)
val pin = PlusArg("custom_boot_pin", width=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Copy link
Contributor

Choose a reason for hiding this comment

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

We should figure out how to get PlusArg to work with FireSim. Seems like a chill ugrad project.

Copy link
Contributor

@abejgonzalez abejgonzalez Sep 10, 2021

Choose a reason for hiding this comment

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

I'll add it to the CY meeting notes. Additionally, I think this harness binder name should probably change since it isn't strictly a tie-off anymore. Maybe something like WithCustomBootPinPlusArg?

generators/chipyard/src/main/scala/TestHarness.scala Outdated Show resolved Hide resolved
generators/chipyard/src/main/scala/IOBinders.scala Outdated Show resolved Hide resolved
@jerryz123 jerryz123 force-pushed the clocking-overhaul branch 2 times, most recently from fa24e1b to eb16f22 Compare September 7, 2021 18:07
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 need to have a closer look, but i really like that we aren't treating PRCI specially anymore now that IOBinders are diplomatic.

Would like to see more in-source docs for new diplomatic nodes, since reading lazy modules can be inscrutable for new users.

@@ -324,6 +325,21 @@ class WithSimDromajoBridge extends ComposeHarnessBinder({

class WithTieOffCustomBootPin extends OverrideHarnessBinder({
(system: CanHavePeripheryCustomBootPin, th: HasHarnessSignalReferences, ports: Seq[Bool]) => {
ports.foreach(_ := false.B)
val pin = PlusArg("custom_boot_pin", width=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

We should figure out how to get PlusArg to work with FireSim. Seems like a chill ugrad project.

generators/firechip/src/main/scala/FireSim.scala Outdated Show resolved Hide resolved
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. I would just like to check that the FPGA prototyping side isn't broken w/ this.

@davidbiancolin
Copy link
Contributor

Two things you need to do:

  • Since there's no test that actually checks the correct frequencies are picked up by the bridge, can you eyeball-confirm those aren't borked.
  • Essentially Ditto abe's comment --- you should regenerate AGFIs. Bonus points (beer?) if you write the CI machinery in FireSIm to do linux boot on the default targets.

@jerryz123
Copy link
Contributor Author

I believe these should be the correct frequencies for the FireSimMulticlockAXIOverSerialConfig:
image

@jerryz123
Copy link
Contributor Author

This should be ready for merge. (Reminder to self to merge-commit to avoid breaking recursive fsim-as-top)

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.

@jerryz123 jerryz123 merged commit d8d7602 into dev Oct 1, 2021
@jerryz123 jerryz123 deleted the clocking-overhaul branch November 3, 2021 06:49
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.

3 participants