Revamp the config system for Top/Harness

[jerryz123]

It is strange that we use the config/mixin system to parameterize everything except for the Top/Harness, for which we still have to use a system of maintaining copies of Tops and Harnesses with precisely chosen traits to do what we want. There is also a lack of concrete documentation regarding the interactions between keys, traits, and mixins, and the standard patterns for using them. I restuctured the config system around what I believe should be the correct patterns for using these, and I define them below.

• Keys specify some parameter which controls some custom widget.
  • Keys should be Option types, and the default value of None should mean no change to the system. (The custom widget is not instantiated and connected)
  • Keys should be defined and documented in sub-projects, since they generally deal with some specific block, and not system-level integration.
  • Users developing a custom block should define a custom key for their block as an Option type, with the default value set to None, preferably
• Traits specify that the System has been parameterized to read some custom Key and optionally instantiate and connect that widget.
  • Traits should not mandate the instantiation of that widget. In other words, we should try to encourage traits to be written with CanHave semantics. Many Rocketchip traits already have CanHave semantics, since they are no-ops if their corresponding keys are set to None.
  • Traits should be defined and documented in subprojects, alongside their corresponding keys. This is because they primarily deal with instantiating and connecting some custom block, and not system-level integration.
  • Users developing a custom block should define their own CanHave trait and add it to whatever Top they are using (either example/Top or a copy of it).
• Mixins set the keys to a non-default value.
  • Mixins which affect the Harness/Top, should set the BuildTop key to instantiate whatever custom block is needed in the Harness and wire the Top ports to whatever is desired. See the WithTSI or WithGPIO mixins for examples. Mixins should also set the parameters key for the custom widget (for example, BlockDeviceKey)
  • These should be defined and documented centrally in Chipyard, since these are what users will primarily interact with to customize their system.
  • Users developing a custom block should define a mixin setting the key for their block in ConfigMixins.

Some other notes from these changes:

• There is only one BuildTop key. Adding ports to a Top is done by finding the top via p(BuildTop, site), and calling the method of that top which connects the ports.
  • This enables intuitive composability of Tops. E.g: WithGPIO ++ WithPWMTL results in a Top with both GPIO and PWM pins, with no other changes necessary.
• By adding the success signal to the BuildTop function, we avoid the need to instantiate a separate TestHarness for DTM-bringup.
• The ConfigMixins now contain mixins that set both the BuildTop key (which controls the Top ports), as well as the widget keys (which control the instantiation and parameterization of the widgets). Thus one mixin is sufficient to both set custom ports, and connect those custom ports to a custom module.
• The WithNoGPIO mixin is an unfortunate byproduct of sifive-blocks not setting a default value for PeripheryGPIOKey. I currently have a PR open for this which should allow us to remove all those mixins from the default. Set PeripheryKey defaults to Nil sifive/sifive-blocks#123
• Encouraging traits to be written with CanHave semantics simplifies convergence between FireSim/Chipyard targets
  • The FireSimDUT and Chipyard's Top can be merged into one Top with a superset of all CanHave traits. Then, the divergence between the two designs can be configured at the mixin level, rather than the trait level

[davidbiancolin]

[I know this WIP, these are mostly just thoughts on the current implementation.]

I think this is a step in the right direction but i think the problem with the current implementation is that leans heavily on there existing only a single top class with the complete set of all possible canHave traits (or you're forced to extend it). I'm not sure if this restriction is a prudent one.

It might be better to relax the type BuildTop elaborates (say, all the way to a Module or LazyModule), and then engineer a more flexible solution to generate the harness around it. The partial-function-based solution we use in FireSim (see BridgeBinders.scala) is one attempt at this, but it too has its limitations.

A second independent point is that relying on up to gradually build up the test harness might prove to be a little bit dangerous. What if multiple mixins try to connect to the same interfaces? It's perfectly legal and even a powerful feature, but only if you order your configs right -- this is another most common gotcha when using Config.

[jerryz123]

I think this is a step in the right direction but i think the problem with the current implementation is that leans heavily on there existing only a single top class with the complete set of all possible canHave traits (or you're forced to extend it). I'm not sure if this restriction is a prudent one.

I'm assuming a world where all the traits can be parameterized to be no-ops. In that case, I'm not sure what issues would arise from making the standard pattern "add your custom trait to the base Top". Ideally we would be able to define new Tops with custom Traits at runtime, but I'm not sure how to do that, and this seems like a better middle-ground for now.

It might be better to relax the type BuildTop elaborates (say, all the way to a Module or LazyModule), and then engineer a more flexible solution to generate the harness around it. The partial-function-based solution we use in FireSim (see BridgeBinders.scala) is one attempt at this, but it too has its limitations.

The partial function stuff is interesting. My concern with that approach is that it would require defining another layer of abstraction wrapping all the methods defined in the various traits, if I understand correctly.

A second independent point is that relying on up to gradually build up the test harness might prove to be a little bit dangerous. What if multiple mixins try to connect to the same interfaces? It's perfectly legal and even a powerful feature, but only if you order your configs right -- this is another most common gotcha when using Config.

Yeah, the examples for this case I can think of are the block device and success ports. I hope that in the documentation, we can explain clearly enough why things like WithDTM ++ WithTSI and WithSimBlockDevice ++ WithBlockDeviceModel are nonsensical. We could even potentially add some way to check in the BuildTop definition that the port has not already been connected, but I can't think of the best way to do that. Otherwise, it seems rare to have multiple conflicting mixins to attach to the same custom interface.

[jerryz123]

I added more documentation on how custom traits work. This should be good to go now, pending sub-project PRs. I'd like to get this in before the upcoming Chipyard release.

[colinschmidt]

Looks pretty good. Just a few questions.

[jwright6323]

Is this tying all of the GPIO inputs to 0 inside the DUT?

[jerryz123]

This code was copied from the older WithGPIOTop mixin. I'm not sure what the right thing to do here is, but I would be happy to implement it

[jerryz123]

This is called from outside the DUT (in the TestHarness), so it should be tying them to 0 outside the DUT.

[jwright6323]

I think we want a separate PR that adds IO cell blackboxes to have "Correct" functionality. That's been a work in progress for a while, but we can reprioritize it.

[abejgonzalez]

While I agree with John on this, I don't think we need to block on this? Also, it might be good to add a comment that this is connecting from outside the DUT.

[jwright6323]

separate PR

means it's not blocking

[jwright6323]

Is this circumventing reset synchronizers?

[jerryz123]

Could you clarify? This was copied from the old DTMTop module, and I'm not familiar with some of its mechanics.

[jwright6323]

Unless something's changed (which is possible), this is connecting the test-harness-level clock and reset to the DUT. That should be fine for simulation, but we might want to note that in a real chip you'd probably have an async top-level reset and a ResetCatchAndSync somewhere inside the chip. I was asking the question because I don't actually know if that ResetCatchAndSync is in the RocketSubsystem already or not (maybe @colinschmidt can chime in).

[jerryz123]

It looks like Rocket Chip's TestHarness does

  dut.reset := reset | dut.debug.map { debug => AsyncResetReg(debug.ndreset) }.getOrElse(false.B)

I guess we should be using the AsyncResetReg here?

[jwright6323]

Yeah, we should fix that, but it wasn't the only problem I was referring to. The other thing was that you likely won't have a clock pin; you probably have a PLL, etc. that would generate a clock you need to synchronize reset to.

[jerryz123]

So in Chipyard currently the hierarchy is TestHarness -> Top, while for real chips the hierarchy would likely look like TestHarness -> ChipTop -> Top. In BEAGLE the chip-specific clocking and reset needs are handled within ChipTop.

With the current BuildTop rules, it looks like the way to do this would be to call p(BuildTop) within ChipTop, creating something like:

class ChipTop(implicit val p: Parameters) extends RawModule {
  val sys_clk = Wire(Clock())
  val sys_rst = Wire(Bool())
  val sys_success = Wire(Bool())
  val sys = p(BuildTop)(sys_clock, sys_reset, p, sys_success)
  ...
}

The issue is currently we don't have this extra ChipTop layer within Chipyard. It seems like that would be a good thing to add in the future.

[colinschmidt]

I don't think rocket-chip currently does catch and sync for the top-level reset. They may be moving in that direction based on recent PRs, but its certainly not in our tagged version yet.
I like it would be cool to have something that is a "reasonable" ChipTop in chipyard so we can make the knowledge of how to do this "safely" more widely known.
I think thats outside of the scope of this PR, in that this doesn't change the current state of chipyard w.r.t. clock&reset. But we should file an issue to track a "realistic clock and reset mixin".

[jerryz123]

I'll file an issue for ChipTop, while I'll leave it to someone more familiar with the clock&reset issues to write that.

#385

[abejgonzalez]

Overall seems fine with me. John covered most of my cleanliness comments on the GCD stuff.

[davidbiancolin]

I'm still skeptical this will scale out how we'd like (see previous comment) and i'm a little worried the BuildTop trick might be a little too cute but otherwise this looks great to me.

[davidbiancolin]

This may be an appropriate time to explain, or at least allude to, what site is.

[alonamid]

+1

[jerryz123]

I found it difficult to mention site without discussing the rest of the CDE system. I added a sub-section on CDEs to the Advanced-Concepts section, and linked to it from here.

@davidbiancolin @alonamid do you mind reviewing that page I just added? Its docs/Advanced-Concepts/CDEs.rst. I think that's the final set of changes that need reviewing.

[abejgonzalez]

Can you confirm that all the configs in Chipyard at least build with this approach?

[jerryz123]

Can you confirm that all the configs in Chipyard at least build with this approach?

Good call @abejgonzalez . Turns out the SHA3 Configs were all broken after the recent rocket chip bump. I fixed that and bumped the SHA3 submodule in this PR as well. All RocketConfigs elaborated verilog successfully. The BlockDevice and GCD configs (which utilize scala resources) also compiled in VCS correctly.

This is still waiting on several submodule PRs to get merged

• Refactor traits as CanHave traits testchipip#58
• Adjust BlockDevBridge to match new Option-type parameters firesim/firesim#434
• Refactor Has traits into CanHave traits firesim/icenet#17
• Fix SHA3-enabled builds sha3#22

[abejgonzalez]

Can you confirm that all the configs in Chipyard at least build with this approach?

Did you also check the FireChip configs (make sure things build with FireSim)?

[jerryz123]

Can you confirm that all the configs in Chipyard at least build with this approach?

Did you also check the FireChip configs (make sure things build with FireSim)?

Both FireSim and FireSimNoNIC targets work. Perhaps we should add these to the regressions.

[colinschmidt]

LGTM

[abejgonzalez]

LGTM pending passing tests.

[jerryz123]

Merging. CI fails because I did not merge the FireSim submodule PR, so @davidbiancolin can deal with the circular dependency madness.