Diplomatic multiclock #614

jerryz123 · 2020-06-23T22:16:58Z

Related issue:
chipsalliance/rocket-chip#1795

Type of change: new feature

Impact: rtl change

colinschmidt

Seems to be a reasonable first pass. I can see how this would be used for more complicated clock driving strategies at the top-level.
I agree with the TODOs in your initial comment, we should have a rational clock tile crossing example with the divider built in and an async tile crossing with two clocks driving from off chip.

generators/chipyard/src/main/scala/ChipTop.scala

jerryz123 · 2020-07-08T03:58:54Z

I cleaned this up substantially. Changes include

ChipTop is now LazyModule, as it contains ClockNodes
ChipyardClockKey lets you define a function to drive the diplomatic clocks of the system
Two SW-simulation demonstrations of ChipyardClockKey, harnessClock just passes through the clock from the TestHarness, and passes it everywhere. harnessMultiClock divides the harnessClock, and uses that for the uncore
MultiClockRocketConfig uses the harnessMultiClock setting, but it actually doesn't simulate correctly, because SimSerial and SimDRAM receive the fast TestHarness clock, instead of the divided clock. The fix for this will come later, as it is more involved.
ConfigValName is removed, I think this also resolves the naming issues with TestHarness appearing everywhere
FireSimMultiClockPOC is removed, as the base FireSim design now supports multiclock through the ChipyardClockKey.
FireSimMulticlockRocketConfig demonstrates the old FireSimMulticlockPOC config
FireSim now builds ChipTop, instead of DigitalTop, removing a bit of code duplication.

.circleci/do-rtl-build.sh

colinschmidt · 2020-08-17T23:11:10Z

generators/chipyard/src/main/scala/ChipTop.scala

+    // The implicit clock and reset for the system is also, by convention, used for all the IOBinders
+    // TODO: This may not be the right thing to do in all cases
+    withClockAndReset(implicit_clock, implicit_reset) {
+      val (_ports, _iocells, _harnessFunctions) = p(IOBinders).values.flatMap(f => f(lSystem) ++ f(lSystem.module)).unzip3


I think we will want to understand how to generate the clock for the test harness side of things.
I don't think we want to have the parts inside the system punch their clock into the test harness.
Either the testharness module should be able to recover the clock from an explicit pin that would exist in a real ASIC (e.g. DDR or serial links with clock recovery), or it should explicitly require some configuration of the test harness component to work correctly (e.g. uart baud rate).

But we can discuss this more later.

I generally agree with the thought here, but i think it gets tricky when we want to model IFs that might not actually be used in a tape out. If we look at memory system AXI4 as a example -- it really has no business going off chip in the first place. It seems to me that it would be OK in this case to add the IO since the IF is not really fit-for-tapeout in the first place.

generators/chipyard/src/main/scala/ChipTop.scala

colinschmidt · 2020-08-17T23:19:18Z

generators/chipyard/src/main/scala/Clocks.scala

+
+      simpleClockGroupSourceNode.map { n => n.out.unzip._1.map { out: ClockGroupBundle =>
+        out.member.elements.map { case (name, data) =>
+          // This is mega hacks, how are you actually supposed to do this?


This can't be the way, it seems hackier than before there was diplomatic clocking.

💀

At least everything is contained in one place now.

generators/chipyard/src/main/scala/ConfigFragments.scala

generators/chipyard/src/main/scala/TestHarness.scala

colinschmidt · 2020-08-17T23:29:10Z

generators/chipyard/src/main/scala/TestHarness.scala

-
-  def success = io.success
-  def harnessReset = this.reset.asBool
+  ldut.harnessFunctions.foreach(_(this))


None of the IOBinders depended on being called before the success val is set right?

I don't think so? IOBinders can either set or read the success signal. Are you envisioning a case where the order of evaluation matters?

colinschmidt · 2020-08-17T23:30:23Z

generators/chipyard/src/main/scala/config/AbstractConfig.scala

@@ -17,6 +17,7 @@ class AbstractConfig extends Config(
  new chipyard.config.WithBootROM ++                             // use default bootrom
  new chipyard.config.WithUART ++                                // add a UART
  new chipyard.config.WithL2TLBs(1024) ++                        // use L2 TLBs
+  new chipyard.config.WithNoSubsystemDrivenClocks ++             // drive the subsystem diplomatic clocks from ChipTop instead of using implicit clocks


Glad we an abstract config 👍
I would be tempted to make the default config the divided version, since that is more "realistic".

That would be cool, if only it worked 😢

I think we should do that once it does work.

generators/chipyard/src/main/scala/config/RocketConfigs.scala

davidbiancolin

Mostly nits. Cleaning up and unifying the firesim/chipyard handling is going to depend on what SiFive suggests.

generators/chipyard/src/main/scala/ChipTop.scala

davidbiancolin · 2020-08-18T01:45:26Z

generators/chipyard/src/main/scala/ChipTop.scala

+    // The implicit clock and reset for the system is also, by convention, used for all the IOBinders
+    // TODO: This may not be the right thing to do in all cases
+    withClockAndReset(implicit_clock, implicit_reset) {
+      val (_ports, _iocells, _harnessFunctions) = p(IOBinders).values.flatMap(f => f(lSystem) ++ f(lSystem.module)).unzip3


I generally agree with the thought here, but i think it gets tricky when we want to model IFs that might not actually be used in a tape out. If we look at memory system AXI4 as a example -- it really has no business going off chip in the first place. It seems to me that it would be OK in this case to add the IO since the IF is not really fit-for-tapeout in the first place.

generators/chipyard/src/main/scala/Clocks.scala

davidbiancolin · 2020-08-18T01:53:45Z

generators/chipyard/src/main/scala/IOBinders.scala

      Debug.tieoffDebug(debugPortOpt, resetctrlOpt, Some(psdPort))(system.p)
      // tieoffDebug doesn't actually tie everything off :/
      debugPortOpt.foreach { d =>
-        d.clockeddmi.foreach({ cdmi => cdmi.dmi.req.bits := DontCare; cdmi.dmiClock := th.clock })
+        // TODO: Using harness clock/reset will be incorrect when systemClock =/= harnessClock


But the harness is expected to provide the clock here right, so this becomes a debug module configuration problem?

You are right. I blanket applied this comment everywhere harnessClock was referenced in the IOBinders, but this case should actually not be using the harnessClock at all.

So it turns out the way we drive the debug module clocks is pretty wrong, debug.clock should not be punched off-chip to be driven.
We should fix the WithSimDebug and WithTiedOffDebug IOBinders in another PR.

Sounds good.

I think there is probably some confusion between the DMI_DTM signals going off chip (which is only intended as a simulation config) and the JTAG_DTM version which has a pretty clear set of signals that should go off-chip (and is intended to be actually implemented in the real world). Perhaps this confusion is carried through to the IOBinders?

Yeah that is accurate. I think the problem is that we are plumbing through the entire DebugIO bundle, instead of just DebugIO.systemjtag or DebugIO.clockeddmi.

DebugIO.clock appears to require a clock which is synchronous to the Tilelink buses (https://github.com/chipsalliance/rocket-chip/blob/3176ed81c2bfa1a24622c511a1e4b0777bc9377c/src/main/scala/devices/debug/Debug.scala#L1775-L1780), so it should not be generated offchip. I feel like we should be calling Debug.connectClockAndReset somewhere within the chip...

generators/chipyard/src/main/scala/TestHarness.scala

davidbiancolin · 2020-08-18T02:01:48Z

generators/firechip/src/test/scala/ScalaTestSuite.scala

@@ -106,8 +106,8 @@ class BoomF1Tests extends FireSimTestSuite("FireSim", "DDR3FRFCFSLLC4MB_FireSimL
 class RocketNICF1Tests extends FireSimTestSuite("FireSim", "WithNIC_DDR3FRFCFSLLC4MB_FireSimRocketConfig", "BaseF1Config")
 // Multiclock tests
 class RocketMulticlockF1Tests extends FireSimTestSuite(
-  "FireSimMulticlockPOC",
-  "FireSimQuadRocketMulticlockConfig",
+  "FireSim",


This is going to require changing the inis.

I will do this when I regenerate AGFIs

jerryz123 · 2020-08-26T07:52:02Z

@davidbiancolin @colinschmidt
I favor merging this PR as-is (after agfi regeneration), assuming no major changes are still necessary. There are still a number of problems to solve before multi-clock designs can be cleanly enabled. Those should be addressed in follow-up PRs, as they may require extensive changes through sub-projects, and additional rocketchip bumps.

davidbiancolin · 2020-08-26T16:21:05Z

I'm fine with that -- but you should put a failing require or assert in the divided config.

colinschmidt · 2020-08-26T16:23:44Z

Yeah. Merging is fine as we have plenty of time before the next release.
Is there a list of things that need to be fixed or is that an unknown unknown? If you know of a list or at least a partial list its probably worth filing a tracking issue.

colinschmidt

LGTM

jerryz123 requested review from davidbiancolin and colinschmidt June 23, 2020 22:18

jerryz123 marked this pull request as draft June 23, 2020 22:24

colinschmidt reviewed Jun 24, 2020

View reviewed changes

jerryz123 force-pushed the diplomatic-clocks branch from 3ef0e68 to 48e700e Compare July 7, 2020 03:14

Rough initial implementation of diplomatic multiclock

c023cf0

jerryz123 force-pushed the diplomatic-clocks branch from 48e700e to c023cf0 Compare July 7, 2020 05:02

jerryz123 changed the title ~~[RFC] Diplomatic Multiclock~~ Initial simple diplomatic clocks Jul 7, 2020

jerryz123 changed the base branch from rc-retile to simple_configs July 7, 2020 05:03

jerryz123 force-pushed the diplomatic-clocks branch 2 times, most recently from 05e3385 to ffd3b2d Compare July 8, 2020 00:37

Support FireSim diplomatic multiclock

56e1aeb

jerryz123 force-pushed the diplomatic-clocks branch from ffd3b2d to 56e1aeb Compare July 8, 2020 03:58

jerryz123 marked this pull request as ready for review July 8, 2020 04:01

jerryz123 changed the title ~~Initial simple diplomatic clocks~~ Diplomatic multiclock Jul 8, 2020

jerryz123 changed the base branch from simple_configs to dev July 10, 2020 00:12

jerryz123 added the DO NOT MERGE label Jul 20, 2020

Merge branch 'dev' into diplomatic-clocks

fdfef87

jerryz123 force-pushed the diplomatic-clocks branch 3 times, most recently from 3b7edfd to f13e864 Compare August 4, 2020 19:14

Bump to July 2020 rocketchip

578ae6f

jerryz123 force-pushed the diplomatic-clocks branch from f13e864 to 578ae6f Compare August 4, 2020 21:00

jerryz123 added 2 commits August 5, 2020 14:21

Merge remote-tracking branch 'origin/dev' into diplomatic-clocks

9e44313

Fix Reset bug

abc75e9

jerryz123 force-pushed the diplomatic-clocks branch from f69b293 to abc75e9 Compare August 8, 2020 00:50

colinschmidt reviewed Aug 17, 2020

View reviewed changes

davidbiancolin reviewed Aug 18, 2020

View reviewed changes

jerryz123 mentioned this pull request Aug 24, 2020

Expose control over take field for all ClockSinkParameters in system chipsalliance/rocket-chip#2618

Closed

jerryz123 force-pushed the diplomatic-clocks branch 4 times, most recently from 87bbc63 to ce81f02 Compare August 26, 2020 04:39

jerryz123 mentioned this pull request Aug 26, 2020

Clean multiclock support #662

Closed

7 tasks

Address PR comments

e275a45

jerryz123 force-pushed the diplomatic-clocks branch from ce81f02 to e275a45 Compare August 26, 2020 19:35

Bump Firesim for new AGFIs

5a25ee5

jerryz123 removed the DO NOT MERGE label Aug 27, 2020

jerryz123 requested review from colinschmidt and davidbiancolin August 27, 2020 17:50

colinschmidt approved these changes Aug 27, 2020

View reviewed changes

Bump testchipip

239b6b6

jerryz123 merged commit ee1ce11 into dev Aug 28, 2020

alonamid mentioned this pull request Nov 24, 2020

Chipyard 1.4.0 Release #599

Merged

jerryz123 deleted the diplomatic-clocks branch December 24, 2020 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diplomatic multiclock #614

Diplomatic multiclock #614

jerryz123 commented Jun 23, 2020 •

edited

Loading

colinschmidt left a comment

jerryz123 commented Jul 8, 2020

colinschmidt Aug 17, 2020

davidbiancolin Aug 18, 2020

colinschmidt Aug 17, 2020

jerryz123 Aug 18, 2020

colinschmidt Aug 17, 2020

jerryz123 Aug 17, 2020

colinschmidt Aug 17, 2020

jerryz123 Aug 17, 2020

davidbiancolin Aug 18, 2020

davidbiancolin left a comment

davidbiancolin Aug 18, 2020

davidbiancolin Aug 18, 2020

jerryz123 Aug 25, 2020

jerryz123 Aug 26, 2020

davidbiancolin Aug 26, 2020

colinschmidt Aug 26, 2020

jerryz123 Aug 26, 2020

davidbiancolin Aug 18, 2020

jerryz123 Aug 26, 2020

jerryz123 commented Aug 26, 2020

davidbiancolin commented Aug 26, 2020

colinschmidt commented Aug 26, 2020

colinschmidt left a comment

Diplomatic multiclock #614

Diplomatic multiclock #614

Conversation

jerryz123 commented Jun 23, 2020 • edited Loading

colinschmidt left a comment

Choose a reason for hiding this comment

jerryz123 commented Jul 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidbiancolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerryz123 commented Aug 26, 2020

davidbiancolin commented Aug 26, 2020

colinschmidt commented Aug 26, 2020

colinschmidt left a comment

Choose a reason for hiding this comment

jerryz123 commented Jun 23, 2020 •

edited

Loading