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

Add An Optional L2:MBus crossing; Misc Xtype + Bus Frequency Config Mixins #2724

Merged
merged 3 commits into from
Nov 11, 2020

Conversation

davidbiancolin
Copy link
Contributor

This upstreams some of stuff we're using in Chipyard, that was discussed in ucb-bar/chipyard#690. It splats out the existing SubsystemCrossingParams into multiple Keys per Henry's suggestion. We could consider deprecating that class now.

I've also added the ability to disable the default DriveClockFromMaster behavior in the provided topologies. We're driving them ourselves in Chipyard, mostly because we want to make clock naming independent of xtype selection.

One idea I had for making it easier to define custom topologies downstream, might be to push this into the Parameters instance with two Maps:

  1. Map[Location, TLBusWrapperInstantiationLike] to represent instantiations
  2. Map[(Location, Location), TLBusWrapperConnection] to represent connections.

This might help avoid needing to duplicate a bunch of code to handle parameterizing multiple similar intermediate topologies (you wouldn't need to have any named subclasses of TLBusWrapperTopology) I am, however, always a little concerned about pushing more complexity into p.

Related issue: ucb-bar/chipyard#690

Type of change: other enhancement

Impact: API modification

Development Phase: implementation

Release Notes

  • Added optional MBus crossing to CoherentBusTopology

@hcook
Copy link
Member

hcook commented Nov 10, 2020

The named subclasses of TLBusWrapperTopology are just there for backwards-compatibility convenience and to serve as examples. TLBusWrapperTopology itself has two members which are essentially the Maps you're proposing, unless I am missing something. It's not an abstract class, so one could just make an instance of one and populate its Maps however one wished...

Also TLNetworkTopologyLocated is quite soft, it doesn't even require a Seq[TLBusWrapperTopology] but rather only a
Seq[CanInstantiateWithinContextThatHasTileLinkLocations with CanConnectWithinContextThatHasTileLinkLocations]. Though perhaps that is making it difficult for you to up(TLNetworkTopologyLocated).map(_.copy(...)) to make some adjustment? (Really need to learn how to use lenses or something to deal with this i-only-want-to-change-this-one-field-of-a-base-class pattern... https://github.com/alexarchambault/data-class ?) TBH we are currently more into making fresh topologies from scratch than gently mutating the pre-canned ones, but I am pretty open to suggestion on improvements in this area...

@davidbiancolin
Copy link
Contributor Author

The named subclasses of TLBusWrapperTopology are just there for backwards-compatibility convenience and to serve as examples. TLBusWrapperTopology itself has two members which are essentially the Maps you're proposing, unless I am missing something. It's not an abstract class, so one could just make an instance of one and populate its Maps however one wished...

That's right. Yeah we'd just build an anonymous subclass from the maps.

From a Chipyard developer's perspective, my preference is to try to mirror how you guys (SiFive) wield RC as we'll be less likely to find ourselves fighting with the library. It's not always clear to us just how you do so -- since RC is just the tip of an enormous code base behind it. Comments like:

TBH we are currently more into making fresh topologies from scratch than gently mutating the pre-canned ones

are super helpful. We'll look to do the same.

@davidbiancolin davidbiancolin merged commit 75c01f1 into master Nov 11, 2020
@davidbiancolin davidbiancolin deleted the bus-crossing-misc branch November 11, 2020 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants