Serial-tilelink backing memory #673

jerryz123 · 2020-09-07T21:31:32Z

This adds support for "LBWIF" backing memory through serialized tilelink. Default configs still use the AXI4 port, but more realistic "chip-like" configs can set the lbwif memory to be used as main backing memory.
Additionally, the SerialAdapter is moved offchip, as it should be considered a harness-level component that can access the chip through the serial TileLink port.

Adding support for other memory channels ("hbwif"), or using a different protocol for request serialization remains future work.

generators/chipyard/src/main/scala/HarnessBinders.scala

colinschmidt

Right now this seems a little strange to me. In my opinion there are really 2 choices to make:
Do you have a serial port going off chip?
What protocols does it support (in, out, in+out)?
This implementation often seems to end up with a strange duplication because its making the choice:
Do you have inward serial support?
Do you have outward serial support?
This ends up with duplicated code and sometimes RTL.

N.B. Usually we only build configurations that are in-only (simulation-only mostly), or in+out (almost all silicon).
I could imagine building an out-only system where jtag is used to do the in half and the serial port does the out half, but its not much more hardware to support in+out over the serial interface.

generators/chipyard/src/main/scala/DigitalTop.scala

generators/chipyard/src/main/scala/HarnessBinders.scala

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

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

colinschmidt · 2020-09-14T15:44:25Z

Also is there a testchipip PR with the changes here?

jerryz123 · 2020-09-14T16:58:38Z

I'll open a testchipip PR after the harnessbinders stuff goes in.

The reason I split the two serial types is because serialized-TSI and serialized-TL are really two mostly orthogonal serial interfaces.
There are 4 possible configurations.

No serialized-TL with no serialized-TSI
No serialized-TL with serialized-TSI. This is the normal chipyard design, with an chip-side SerialAdapter talking to a harness-side SimSerial
Serialized-TL with no serialized-TSI. This configuration supports an external Tilelink manager for cacheable memory, and an external Tilelink client for the fbus. If the withHarnessSerialAdapter argument is set, the harness-side Tilelink client connection is used by a harness-side SerialAdapter. This is the common "chip" like config.
Serialized-TL with serialized-TSI. This configuration supports an external Tilelink manager for cacheable memory, and an internal SerialAdapter talking to a harness-side SimSerial. This config may be bugged if the user explicitly specifies an external SerialAdapter as well. This use case is pretty unrealistic for chips, but I found it useful to support this use-case when I was debugging the external-serial-adapter.

I don't think there is much duplication to support use-case 4. The CanHavePeripherySerial{TL/TSI} traits, and their HarnessBinders are very different from each other. The two serialized interfaces are naturally orthogonal, and I think it would be more confusing to have a single CanHavePeripherySerial trait/iobinders/harnessbinders which have to read some key to figure out what the serial interface is being used for.

At a general principle, I think each Trait-IOBinder-HarnessBinder "vertical" should correspond to an interface and its use case, rather than just the interface itself. For the same reason why we have separate traits/IOBinders/HarnessBinders/ for each external AXI4 type, we should have separate traits/IOBinders/HarnessBinders for serialized tilelink and serialized TSI.

jwright6323 · 2020-09-14T17:14:26Z

Personally, I've struggled enough with the LBWIF code that I think we should scrap it and implement a simpler protocol that doesn't make the host-side RTL dependent on the target configuration (no source/sink ID bits, no address bitwidth dependency, etc.)

jerryz123 · 2020-09-14T18:03:24Z

The solution is to deprecate support for the on-chip serial-adapter, and only support serialized-tilelink.

jwright6323 · 2020-09-14T18:10:27Z

The solution is to deprecate support for the on-chip serial-adapter, and only support serialized-tilelink.

I don't think so- if you are serializing tilelink your host RTL still needs to be aware of the chip's TL parameters. A more generic, higher-level transfer protocol (even one that has TL ports on either end), would get rid of that dependency. Even if you solved the problem of generating the host RTL automatically (as I/we have proposed in the past), you still run into trouble if you want to test multiple chips with the same FPGA bitstream.

jerryz123 · 2020-09-14T18:51:48Z

I don't think so

Sorry, I meant the solution to the duplication between Serial-TL and Serial-TSI.

I agree improvements to the serialization protocol could reduce the host-dependency on the chip. Perhaps chiplink might be a place to look, but I think that might be over-engineered for our use case.

colinschmidt · 2020-09-14T18:52:04Z

I think you won't be able to get around a dependency on the chip RTL (without wasting on-chip hardware). The solution we talked about today is going to do the correct inspection of chip parameters from the harness side to ensure that everything matches up.

colinschmidt · 2020-09-14T18:54:33Z

Maybe we should support different serialization via a parameter? If it is constrained to TL on both ends and Clocked[SerialIO] in the middle people should be able to explore different variants.

jerryz123 · 2020-09-14T18:55:45Z

The testchipip PR I will open contains a require which verifies that the host-side serdesser and the chip-side serdesser agreed on the same parameters.

jwright6323 · 2020-09-14T19:01:58Z

I think you won't be able to get around a dependency on the chip RTL (without wasting on-chip hardware). The solution we talked about today is going to do the correct inspection of chip parameters from the harness side to ensure that everything matches up.

I disagree, but maybe we should discuss what some reasonable tradeoffs would be in a separate forum

colinschmidt · 2020-09-14T19:21:19Z

Sure. We can take the discussion offline.
I think there is minimal work to integrate the current working code that lives outside of chipyard into chipyard.
I also think most of it won't be thrown away if you were to switch up the serialization format.
Therefore I think this PR should go forward as discussed and we can figure out if we want to change the serialization format and what we would change it to at a later date after in person or more online discussions.

jwright6323 · 2020-09-14T19:29:23Z

I think it's fine to merge this in, but my main goal would be to prevent anyone from taping out with this, because that would then require supporting this for another year or two. We have an opportunity to break that cycle now, I think.

jerryz123 · 2020-09-14T19:34:26Z

As anyone looked at chiplink for serializing tilelink?

colinschmidt · 2020-09-14T19:35:37Z

It wasn't public last time I needed something like it.

jerryz123 · 2020-09-14T19:37:22Z

There seems to be stuff in sifive-blocks: https://github.com/sifive/sifive-blocks/blob/master/src/main/scala/devices/chiplink/ChipLink.scala

And freedom references chiplink in a couple places.

I think there is value in preserving this simpler protocol, as long as it works and the limitations are clear.

jwright6323 · 2020-09-14T19:42:31Z

I don't think this is a "Simple" protocol; I would argue it is actually overly complex. It may be simple in concept ("Just take tilelink and serialize it"), but it's extremely fragile and difficult to debug because there's so much context dependence to each 32-bit packet.

jerryz123 · 2020-09-14T20:56:39Z

Lets move this discussion to ucb-bar/testchipip#98

jerryz123 · 2020-09-15T02:46:59Z

I've removed the on-chip serial-adapter support. Now all designs use an external serial-adapter.
The external harness RAM is by default small (4KB), and uses an unused physical memory region.
The LBWIFMemoryRocketConfig (open to name suggestions) sets the external harness RAM to 256 MB, and sets its base address to DRAM_BASE, effectively replacing the functionality of the AXI4 backing memory.

colinschmidt

A few nitty comments, but LGTM.

generators/chipyard/src/main/scala/DigitalTop.scala

colinschmidt · 2020-09-17T16:43:26Z

generators/chipyard/src/main/scala/HarnessBinders.scala

-  (system: CanHavePeripherySerial, th: HasHarnessSignalReferences, ports: Seq[ClockedIO[SerialIO]]) => {
-    ports.map { p => SerialAdapter.tieoff(Some(p.bits)) }
-    Nil
+class WithSerialAdapterTiedOff extends OverrideHarnessBinder({


These two classes are intended to be mutually exclusive, right?
I think if they aren't there is some potentially weird behavior where the SimSerial is still connected to success and FESVR but not connected to the chip.

I also wonder if we would ever get a request for WithSerialMemTiedOff.

Thinking about this a little bit more I think the SimSerial widget will just hang when there is no chip response so success will never go spuriously high.

They are mutually exclusive. I

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

colinschmidt · 2020-09-17T16:46:24Z

generators/chipyard/src/main/scala/Subsystem.scala


 trait CanHaveHTIF { this: BaseSubsystem =>
  // Advertise HTIF if system can communicate with fesvr
  if (this match {
-    case _: CanHavePeripherySerial if p(SerialKey) => true
+    case _: CanHavePeripheryTLSerial if p(SerialTLKey).nonEmpty => true


If you mix in WithSerialAdapterTiedOff this should be false. Not sure if the keys value is able to change that, but I don't think so.

WithSerialAdapterTiedOff is a harness-configuration fragment, so it should probably not affect this chip-generator code.

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

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

colinschmidt · 2020-09-17T17:01:43Z

generators/firechip/src/main/scala/TargetConfigs.scala

@@ -131,7 +131,7 @@ class FireSimSmallSystemConfig extends Config(
  new WithoutClockGating ++
  new WithoutTLMonitors ++
  new freechips.rocketchip.subsystem.WithExtMemSize(1 << 28) ++
-  new testchipip.WithTSI ++
+  new testchipip.WithDefaultSerialTL ++


This small config seems like a good place to put a FireSim that test LBWIF. (reduces number of off-chip IOs of the modeled system by a lot)

I'm currently uncertain if FireSim configs without the AXI4 backing memory work correctly, since they use a separate channel to write the program binary into DRAM.

They don't work correctly, issue opened here: firesim/firesim#627

jerryz123 force-pushed the serial-tl branch from 57d1e75 to 4686b56 Compare September 8, 2020 07:34

jerryz123 mentioned this pull request Sep 8, 2020

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

Merged

4 tasks

colinschmidt reviewed Sep 10, 2020

View reviewed changes

generators/chipyard/src/main/scala/HarnessBinders.scala Outdated Show resolved Hide resolved

colinschmidt reviewed Sep 10, 2020

View reviewed changes

generators/chipyard/src/main/scala/HarnessBinders.scala Outdated Show resolved Hide resolved

Support Tilelink over serial

6c5bce5

jerryz123 force-pushed the serial-tl branch from 70369a2 to 6c5bce5 Compare September 13, 2020 18:59

colinschmidt requested changes Sep 14, 2020

View reviewed changes

jerryz123 added 2 commits September 14, 2020 19:35

Merge remote-tracking branch 'origin/dev' into serial-tl

f9cc1dc

Deprecate support for on-chip SerialAdapter

0d8e871

jerryz123 force-pushed the serial-tl branch from a6f4e37 to 0d8e871 Compare September 15, 2020 02:43

jerryz123 changed the base branch from harness-refactor to dev September 15, 2020 02:44

jerryz123 requested a review from jwright6323 September 15, 2020 02:44

jerryz123 changed the title ~~[WIP] Serial-tilelink backing memory~~ Serial-tilelink backing memory Sep 15, 2020

jerryz123 added 2 commits September 16, 2020 10:29

Bump testchipip

36ccb12

Bump testchipip

269af01

jerryz123 force-pushed the serial-tl branch from 50e31cc to 269af01 Compare September 16, 2020 20:52

colinschmidt approved these changes Sep 17, 2020

View reviewed changes

jerryz123 force-pushed the serial-tl branch from 80a22ca to 6372af2 Compare September 18, 2020 03:50

Merge remote-tracking branch 'origin/dev' into serial-tl

b9622c5

jerryz123 force-pushed the serial-tl branch from 6372af2 to b9622c5 Compare September 18, 2020 08:02

jerryz123 added 2 commits September 18, 2020 10:41

Bump firesim

aa355c7

Bump Firesim

bbf941c

jerryz123 merged commit ba05b32 into dev Sep 18, 2020

alonamid mentioned this pull request Nov 24, 2020

Chipyard 1.4.0 Release #599

Merged

jerryz123 deleted the serial-tl branch December 24, 2020 05:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serial-tilelink backing memory #673

Serial-tilelink backing memory #673

jerryz123 commented Sep 7, 2020 •

edited

Loading

colinschmidt left a comment

colinschmidt commented Sep 14, 2020

jerryz123 commented Sep 14, 2020 •

edited

Loading

jwright6323 commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

jwright6323 commented Sep 14, 2020

jerryz123 commented Sep 14, 2020 •

edited

Loading

colinschmidt commented Sep 14, 2020

colinschmidt commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

jwright6323 commented Sep 14, 2020

colinschmidt commented Sep 14, 2020

jwright6323 commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

colinschmidt commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

jwright6323 commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

jerryz123 commented Sep 15, 2020

colinschmidt left a comment

colinschmidt Sep 17, 2020

colinschmidt Sep 17, 2020

jerryz123 Sep 17, 2020

colinschmidt Sep 17, 2020

jerryz123 Sep 17, 2020

colinschmidt Sep 17, 2020

jerryz123 Sep 17, 2020

jerryz123 Sep 17, 2020

Serial-tilelink backing memory #673

Serial-tilelink backing memory #673

Conversation

jerryz123 commented Sep 7, 2020 • edited Loading

colinschmidt left a comment

Choose a reason for hiding this comment

colinschmidt commented Sep 14, 2020

jerryz123 commented Sep 14, 2020 • edited Loading

jwright6323 commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

jwright6323 commented Sep 14, 2020

jerryz123 commented Sep 14, 2020 • edited Loading

colinschmidt commented Sep 14, 2020

colinschmidt commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

jwright6323 commented Sep 14, 2020

colinschmidt commented Sep 14, 2020

jwright6323 commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

colinschmidt commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

jwright6323 commented Sep 14, 2020

jerryz123 commented Sep 14, 2020

jerryz123 commented Sep 15, 2020

colinschmidt 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

jerryz123 commented Sep 7, 2020 •

edited

Loading

jerryz123 commented Sep 14, 2020 •

edited

Loading

jerryz123 commented Sep 14, 2020 •

edited

Loading