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

Refactor TLBusWrapper and CrossingHelper #1548

Merged
merged 12 commits into from
Jul 23, 2018
Merged

Refactor TLBusWrapper and CrossingHelper #1548

merged 12 commits into from
Jul 23, 2018

Conversation

hcook
Copy link
Member

@hcook hcook commented Jul 19, 2018

I refactored both the TLBusWrapper and CrossingHelper APIs. The former is slightly more flexible and has better code reuse. The latter is a complete re-write No intended functional RTL changes other than signal names.

TLBusWrapper:

  • Unprotect or removed the base class's adapter methods
  • Moved the common master and slave attachment functions into traits CanAttachTLSlaves and CanAttachTLMasters
  • Changed how the existing buses clock cross to each other based on the following changes:

CrossingHelper:

  • HasCrossing is deprecated, CrossingWrapper uses the replacement trait CrossesToOnlyOneClockDomain
  • Renames subsystem.SubsystemClockCrossing to diplomacy.ClockCrossingType
  • Puts HasClockDomainCrossing base trait in diplomacy
  • Defines several [Protocol][Out|In]wardCrossingHelper case classes for each protocol type and [Protocol]ClockDomainCrossing implicit class conversions in each package to add crossing methods to instances of HasClockDomainCrossing that need to be crossed
  • Adds ClockCrossingType argument to individual calls to CrossingHelper methods

Move subsystem.SubsystemClockCrossing => diplomacy.ClockCrossingType
Put abstract CrossingHelper base class in diplomacy
Make concrete AXI4CrossingHelper, TLCrossingHelper, IntCrossingHelper
Replace CrossingWrapper with AXI4CrossingWrapper, TLCrossingWrapper
@hcook hcook requested a review from terpstra July 19, 2018 21:29
Copy link
Contributor

@terpstra terpstra left a comment

Choose a reason for hiding this comment

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

I think we need to discuss this a bit more...

}
}

class AXI4CrossingWrapper(val crossing: ClockCrossingType)(implicit p: Parameters) extends SimpleLazyModule with LazyScope {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you split the crossing helpers by type like you've done, then I think this API (the wrapper) has to die. You typically need to cross more than one bus type and this wrapper can only cross AXI. So it's mostly useless.

Copy link
Contributor

Choose a reason for hiding this comment

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

The way these wrappers work is to drop a half of the crossing in the island and place the other half in the caller. We need to IMO reconsider a few things: is the island a new module layer just doing the crossing? How do we relate multiple clocks each with multiple busses.

Copy link
Contributor

Choose a reason for hiding this comment

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

On the one hand, you really want only one helper per clock domain in the island. On the other hand, you want to change the crossing type based on the relationship between that domain and the clock where the other half is dropped.

@hcook
Copy link
Member Author

hcook commented Jul 19, 2018

We decided to:

  • Put the deprecation only on the methods in HasCrossing
  • De-deprecate CrossingWrapper and delete the protocol-specific Wrappers
  • Move the crossingType arg from the constructor to the function call
  • Remove the prefixes on the protocol-specific function name

@hcook
Copy link
Member Author

hcook commented Jul 22, 2018

@terpstra I implemented the changes we discussed. Seems strictly better than before!

I need to re-check the names before merging, but in the meantime can you unblock this PR?

@hcook hcook merged commit 06bafa9 into master Jul 23, 2018
@hcook hcook deleted the crossing-cleanups branch July 23, 2018 19:15
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.

None yet

2 participants