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

Avoid Subsystem Implicit Clock #2719

Merged
merged 8 commits into from
Nov 18, 2020
Merged

Avoid Subsystem Implicit Clock #2719

merged 8 commits into from
Nov 18, 2020

Conversation

davidbiancolin
Copy link
Contributor

@davidbiancolin davidbiancolin commented Nov 10, 2020

An alternate implementation to #2622.

Two problems:

  • Not sure if the extra layers of module hierarchy are desired, but we can always inline the CD wrappers.
  • Something needs to supply the IBus ClockSource when there's no PLIC.

I think it would be really cool if we had a mixin for LazyModule, DiplomaticImplicitClockAndReset or something, that would add a ClockSinkNode and automatically drive the ModuleImp's implicit clock and reset using that node's resulting clock bundle. This is effectively covered by extending ClockSinkDomain.

Related issue:

Type of change other enhancement

Impact: API modification

Development Phase: proposal

Release Notes

src/main/scala/devices/debug/Periphery.scala Show resolved Hide resolved
src/main/scala/subsystem/RTC.scala Show resolved Hide resolved
src/main/scala/subsystem/InterruptBus.scala Outdated Show resolved Hide resolved
src/main/scala/devices/tilelink/CLINT.scala Show resolved Hide resolved
plic.node := tlbus.coupleTo("plic") { TLFragmenter(tlbus) := _ }
plic.intnode :=* ibus.toPLIC

// TODO: What should be responsible for defining the ibus domain when there's no PLIC?
ibus.interruptBusDomainWrapper.clockNode := tlbus.fixedClockNode
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this gets at the heart of why I didn't want to put the PLIC inside the TLBusWrapper its tilelink port is attached to. A situation where IBus is on its own clock domain, the PLIC is on that clock domain, and the crossing is put between the TileLink control port of the PLIC and the TLBus a very plausible scenario that we are interested in enabling. Not to say you have to add the crossing capability now in this trait. I think this particular line of code can just go live here alongside some other associated technical debt (and just change tlbus to sbus)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to check i understand you correctly: this will put the IBus in the same domain as the SBus for now. Would this not be problematic if the CBus and SBus are running at different frequencies, given where the PLIC attaches by default currently?

If so, I'm alright with this. I think we're probably going to just mandate CBUS = SBUS = FBUS for the time being in Chipyard anyways.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CBUS = SBUS = FBUS

Yes we have a similar mandate for similar simplifying reasons, and only tweak the amount of synchronous buffering that is inserted between those three. But I want to be able to move to a situation where the domains are {SBUS}x{CBUS}x{IBUS,PLIC}x{TILES}. This does mean the PLIC will need more XType arguments to describe its relationship to CBUS and TILES than it currently has, but I think this approach is moving it in that direction, whereas the previous one wedded PLIC to CBUS by hierarchically including it there which I think we would have to walk back later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make the change of handling this in BaseSubsystem for now? Then I think we are good to go and I will do some testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, i'll do that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{SBUS}x{CBUS}x{IBUS,PLIC}x{TILES}

Yeah, this would be great.

@hcook hcook self-requested a review November 18, 2020 00:41
@hcook hcook merged commit eb186d0 into master Nov 18, 2020
@davidbiancolin davidbiancolin deleted the avoid-subsystem-implicit-clock branch September 28, 2021 04:36
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