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

Unify supernode/harness-clocking across chipyard/firesim/fpga #1474

Merged
merged 15 commits into from
May 24, 2023
Merged

Conversation

jerryz123
Copy link
Contributor

@jerryz123 jerryz123 commented May 12, 2023

This PR unifies TestHarness/SuperNode/HarnessClocking across chipyard/firesim/and firechip. This saves 200 LOC, while maintaining existing capabilities. It also makes it easier to bring up a new FPGA target, you can see the reduction in LOC in the ArtyHarness.

The general principle here is that there are two categories of clockSinks. ChipTops can request clocks for any clock ports they expose, at any frequency. The user will also specify a single frequency HarnessBinderFrequency that be provided to all the HarnessBinder collateral as the implicit clock.

The Harness generates the set of requested frequencies.

  • RTL sims use ClockSourceAtFreq to generate the exact requested frequency
  • FireSim uses RationalClockBridge to approximate frequency ratios
  • FPGA prototypes currently tie all requested clocks to the referenceClock, but can be modified with a custom HarnessClockInstantiator to tie clocks to specific FPGA clock domais
  • Chips for tapeout should instantiate a PLL/clock-receivers, and request slow reference clock frequencies as IO from the harness

Changed

  • buildtopClock/Reset -> harnessBinderClock/Reset : these are used to clock anything in a HarnessBinder
  • withClock(buildtopClock, buildtopReset) no longer needed
  • WithClockGroupsCombinedByName changed to allow overriding previously set clock groupings. This allows the AbstractConfig to specify a "default" set of clock groups across buses
  • HasHarnessSignalReferences -> HasHarnessInstantiators, this trait now does almost all harness-level HarnessBinder/clocking construction, deduplicating stuff across chipyard/firechip/fpga
  • Supernode stuff in firechip is now MultiChip in chipyard
  • ClockBridgeInstantiator in firechip is refactored to fit chipyard's HarnessClockInstantiator API

Removed

  • DefaultClockFrequencyKey. Now clock frequencies must be either requested explicitly, or a clock group with no specified frequency will take the clock of a group it is combined with. This gets rid of one more user clocking API that is difficult to reason about
  • TestChipMultiClockConfig/TestChipBusFreqs , now ChipLikeRocketConfig demonstrates a tapeout-like design
  • WithFireSimSimpleClocks is removed, it is the same as WithPassthroughClockGenerator
  • WithFireSimHarnessClockBinder is removed, it is the same as WithClockAndResetFromHarness
  • Ability to dynamically configure diplomatic lazy sifive fpga-shells based on the design. We want to move away from FPGA-shells for future boards anyways.
  • This technically drops support for a synchronous-reset-pin on ChipTop, but that isn't a realistic design anyways.

Related PRs / Issues:

Type of change:

  • Bug fix
  • New feature
  • Other enhancement

Impact:

  • RTL change
  • Software change (RISC-V software)
  • Build system change
  • Other

Contributor Checklist:

  • Did you set main as the base branch?
  • Is this PR's title suitable for inclusion in the changelog and have you added a changelog:<topic> label?
  • Did you state the type-of-change/impact?
  • Did you delete any extraneous prints/debugging code?
  • Did you mark the PR with a changelog: label?
  • (If applicable) Did you update the conda .conda-lock.yml file if you updated the conda requirements file?
  • (If applicable) Did you add documentation for the feature?
  • (If applicable) Did you add a test demonstrating the PR?
  • (If applicable) Did you mark the PR as Please Backport?

Copy link
Member

@T-K-233 T-K-233 left a comment

Choose a reason for hiding this comment

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

This is niiiiiiice!

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.

Overall looks super nice. Couple of questions for clarification.

Comment on lines +29 to +32
new chipyard.harness.WithHarnessBinderClockFreqMHz(50) ++
new chipyard.config.WithMemoryBusFrequency(50.0) ++
new chipyard.config.WithSystemBusFrequency(50.0) ++
new chipyard.config.WithPeripheryBusFrequency(50.0) ++
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple of questions on this:

  • For FPGA prototypes, can you do multi-clock designs? Or are you enforced to have a single frequency across all clock domains? If that is the case, is there some require/assert to enforce this?
  • Why should the harness clock have a specific value? Can't you just choose the default fastest clock of the system and use that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For FPGA prototypes, can you do multi-clock designs? Or are you enforced to have a single frequency across all clock domains? If that is the case, is there some require/assert to enforce this?

The existing FPGA designs are all single clock. To support multi-clock FPGA, a HarnessClockInstantiator variant would have to be designed that would configure the clock generator IP for that particular board. That is future work for the next use case which wants multi-clock-target FPGA prototypes.

The broadcast-single-clock approach is the simpler thing to do, and hopefully it can make it more clear how one might bring up a new board. The AllClocksFromHarnessClockInstantiator will check/enforce that all requested clocks match the reference clock frequency.

Why should the harness clock have a specific value? Can't you just choose the default fastest clock of the system and use that?

Its not clear to me that using the fastest clock in the system is the most obvious behavior. On a realistic system, where the tile runs faster than the uncore, this doesn't really make sense.

Its also not clear to me that using the slowest clock is the most intuitive behvior either.

I feel like the most intuitive, and realistic, behavior, is for harness clocks to be completely async/unrelated with chip clocks.
A more aggressive refactor of the HarnessBinders could remove the harnessBinderClock entirely, and force all HarnessBinders to request their own clock at whatever frequency they need, instead of relying on some implicit clock.

Comment on lines +271 to +273
new chipyard.harness.WithHomogeneousMultiChip(n=4, new Config(
new freechips.rocketchip.subsystem.WithExtMemSize((1 << 30) * 8L) ++ // 8GB DRAM per node
new FireSimRocketConfig)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow!

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.

Overall looks super nice. Couple of questions for clarification.

@harrisonliew
Copy link
Contributor

This is nice! One question before I approve:

RTL sims use ClockSourceAtFreq to generate the exact requested frequency
FireSim uses RationalClockBridge to approximate frequency ratios
FPGA prototypes tie all requested clocks to the referenceClock

Let's say I am making a chip with multiple clocks, some of which are intended to be high-speed analog clocks. For simulation, this makes sense. But is a FPGA harness required to also generate those otherwise analog clocks and tie them to the reference clock? Or is there a mechanism for denoting that some clocks only apply to simulation but not for FPGA harnesses?

@jerryz123
Copy link
Contributor Author

If you are generating a design with high-speed analog clocks, what would you even want when putting the design on a FPGA? The analog blocks wouldn't be synthesizable, so you'd have to cut those out anyways.

The FPGA prototype flows can be extended in the future to support multi-clock configurations under whatever constraints the FPGA PLLs and clock resources can support, this would involve only specifying a different HarnessClockInstantiator that ties chiptop clock domains to specific FPGA clock domains.

@harrisonliew
Copy link
Contributor

The analog blocks wouldn't be synthesizable, so you'd have to cut those out anyways.

Not true, we should be able to use behavioral models (or eventually, mixed-signal simulation). Anyways, if we are envisioning being able to have those additional FPGA clock domains, this should be trivial to support.

@jerryz123
Copy link
Contributor Author

Right, we should support multiclock prototypes with synthesizable models of analog blocks.

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 (lets hold off on merging this until mid-next week)

Comment on lines 113 to 114
# Ibex cannot run the riscv-tests binaries for some reason
# make run-binary-fast -C $LOCAL_SIM_DIR $DISABLE_SIM_PREREQ ${mapping[$1]} BINARY=$RISCV/riscv64-unknown-elf/share/riscv-tests/isa/rv32ui-p-simple
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any easy way to compile a 32b test? If not, maybe we should just check that Ibex compiles (and not runs)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not at the moment, the default libgloss install is only rv64.

Ibex seems to fail this due to not implementing some necessary CSR 🤷‍♂️

@jerryz123 jerryz123 merged commit 473e4c4 into main May 24, 2023
5 checks passed
@jerryz123 jerryz123 deleted the unify branch May 24, 2023 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants