Add support for symmetric serial-tilelink #1716
Conversation
jerryz123
requested review from
vikramjain236,
schwarz-em,
kevindna and
T-K-233
generators/chipyard/src/main/scala/harness/HarnessBinders.scala
Outdated
Show resolved
Hide resolved
jerryz123
force-pushed
the
symmetric_sertl
branch
from
2ab68dc
to
86f028a
Compare
| new testchipip.serdes.WithSerialTL(Seq(testchipip.serdes.SerialTLParams( // 1 serial tilelink port | ||
| manager = Some(testchipip.serdes.SerialTLManagerParams( // port acts as a manager of offchip memory | ||
| memParams = Seq(testchipip.serdes.ManagerRAMParams( // 4 GB of off-chip memory | ||
| address = BigInt("80000000", 16), |
There was a problem hiding this comment.
Do we want these addresses to be configurable? What if my chip has address space larger than 80000000?
There was a problem hiding this comment.
This is describing the base address of DRAM in the example ChipLikeRocketConfig. The user is free to select any base+size here, as long as it does not conflict with something else, and as long as the FPGA setup is configured to match.
| isMemoryDevice = true | ||
| )), | ||
| client = Some(testchipip.serdes.SerialTLClientParams()), // Allow an external manager to probe this chip | ||
| phyParams = testchipip.serdes.ExternalSyncSerialParams(width=4) // 4-bit bidir interface, sync'd to an external clock |
There was a problem hiding this comment.
I am assuming that width=4 is only for testing? Have you tested with wider interface?
There was a problem hiding this comment.
Yes, wider interfaces have been built and tested in the past. I haven't changed the behavior of the serializer for the ExternalSync/InternalSync cases, so this is just changing to the user parameterizes the width.
| // Simulates 2X of the SymmetricChipletRocketConfig in a multi-sim config | ||
| class MultiSimSymmetricChipletRocketConfig extends Config( | ||
| new chipyard.harness.WithAbsoluteFreqHarnessClockInstantiator ++ | ||
| new chipyard.harness.WithMultiChipSerialTL(chip0=0, chip1=1, chip0portId=1, chip1portId=1) ++ |
There was a problem hiding this comment.
Why is the chip0portId the same =1 ?
Can chipId and chip portId be different? Why do we need both?
There was a problem hiding this comment.
Each chip exposes 2 serial TL ports
- Chip0
- serTL0 => goes to FPGA bringup platform
- serTL1 => goes to Chip1
- Chip1
- serTL0 => goes to FPGA bringup platform
- serTL1 => goes to Chip0
| val bits = port.io.bits | ||
| if (DataMirror.directionOf(port.io.clock) == Direction.Input) { | ||
| port.io.clock := th.harnessBinderClock | ||
| case (th: HasHarnessInstantiators, port: SerialTLPort, chipId: Int) if (port.portId == 0) => { |
There was a problem hiding this comment.
Does the SerialTL port to external memory / bringup FPGA have to be on port 0? If so, is this documented somewhere?
There was a problem hiding this comment.
No, its not required to be port0, but for convention, I think it should be. This HarnessBinder assumes its port0, as you've noticed. We should stick to this standard, but I'm not sure this is easy to enforce in the generator (since the generator doesn't know what a serial-TL port will connect to.).
There was a problem hiding this comment.
That seems reasonable. It does seem difficult to add an assert for that. How about adding a config to the TL-serdesser which allows for adding a port instead of requiring the user to configure all the ports at once? Like WithSerialTL but instead of passing a seq, pass SerialTLParams and (optionally?) an index. Since the abstract config already adds the external serial-TL port, this will help enforce that port0 is the external port and overwriting all the ports can be use-at-your-own-risk.
This could probably be added later though. I do think the chipyard documentation should be updated at some point with all the chiplet/multi-chiptop changes, and the external port0 thing would be good to note.
There was a problem hiding this comment.
How about adding a config to the TL-serdesser which allows for adding a port instead of requiring the user to configure all the ports at once?
Yeah, that can be added. I think it will inevitably be confusing, however, to keep track of what is being "appended to", vs "overwritten".
jerryz123
force-pushed
the
symmetric_sertl
branch
2 times, most recently
from
fe13a6b
to
0a6e895
Compare
jerryz123
force-pushed
the
symmetric_sertl
branch
from
eee3617
to
8600640
Compare
jerryz123
force-pushed
the
symmetric_sertl
branch
from
fc761fc
to
5f2b218
Compare
jerryz123
force-pushed
the
symmetric_sertl
branch
from
71ae781
to
41651ed
Compare
Merge remote-tracking branch 'origin/main' into symmetric_sertl
|
Merging this to fix a bug on main and to unblock other work. |
Updated APIs:
ClockedIO[SerialIO]and variants are nowInternalSyncSerialIO/ExternalSyncSerialIO, for claritySimDRAM/SimTSInow can take achipIdparameter, allowing simulating multiple SimTSI interfaces, as well as multiple DRAM interfaces across multiple chipsNew:
SourceSyncSerialIO, configuration option for testchipip's serial-TL generator that generates a symmetric credited source-synchronous interfaceSymmetricRocketChipletConfig, demonstrates a RocketConfig that exposes a second symmetric serial-TL port, that can be connected to an instance of itselfRelated PRs / Issues:
Type of change:
Impact:
Contributor Checklist:
mainas the base branch?changelog:<topic>label?changelog:label?.conda-lock.ymlfile if you updated the conda requirements file?Please Backport?