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

Support diplomatic IOBinders #699

Merged
merged 7 commits into from
Nov 3, 2020
Merged

Support diplomatic IOBinders #699

merged 7 commits into from
Nov 3, 2020

Conversation

jerryz123
Copy link
Contributor

Related issue:

  • New OverrideLazyIOBinder and ComposeLazyIOBinder APIs to write IOBinders which modify diplomatic parameters
  • Clock/reset boring replaced with ClockSinkNode auto wiring
  • Fixes make TOP=DigitalTop and adds regression test

Type of change: new feature

Impact: software change

Release Notes

Copy link
Contributor

@colinschmidt colinschmidt left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. I like that we can still have non-lazy iobinders.

generators/chipyard/src/main/scala/HarnessBinders.scala Outdated Show resolved Hide resolved

abstract trait HasIOBinders { this: LazyModule =>
val lazySystem: LazyModule
private val iobinders = p(IOBinders)
Copy link
Contributor

@abejgonzalez abejgonzalez Oct 21, 2020

Choose a reason for hiding this comment

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

There is no "ApplyIOBinders" function right. So where do the IOBinder functions (both lazy and non-lazy) get called? I'm not exactly sure here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The evaluation of lzy and imp below evaluates the IOBinders, or wraps them in InModuleBody if they are not lazyiobinders. The implementation is a bit hairy because I wanted to avoid having to modify all of the existing concrete iobinders,

Copy link
Contributor

@abejgonzalez abejgonzalez left a comment

Choose a reason for hiding this comment

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

Other than the questions I have this LGTM. This (from an initial port) works with my FPGA work (can generate Verilog and a proper DTS).

@lsteveol
Copy link

I hope this doesn't sound too pushy, but do you guys have an ETA on this being merged? I would like to try out some of these new features. Thanks!

Copy link
Contributor

@davidbiancolin davidbiancolin left a comment

Choose a reason for hiding this comment

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

Cool! It'd be nice to see the boring utils stuff ripped out given we can use diplomacy to plumb out the clock now.

generators/chipyard/src/main/scala/HarnessBinders.scala Outdated Show resolved Hide resolved
})
)
})
class OverrideHarnessBinder[T, S <: HasHarnessSignalReferences, U <: Data](fn: => (T, S, Seq[U]) => Seq[Any])
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we're approaching diplomacy-level type genericization.

generators/chipyard/src/main/scala/HarnessBinders.scala Outdated Show resolved Hide resolved
@jerryz123 jerryz123 merged commit 3741515 into dev Nov 3, 2020
@alonamid alonamid mentioned this pull request Nov 24, 2020
@jerryz123 jerryz123 deleted the lazy-iobinders branch December 24, 2020 05:20
case system: T => composer(upfn)(system, th, pts)
case _ =>
}
case _ =>
Copy link
Contributor

Choose a reason for hiding this comment

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

@jerryz123 I'm bumping to start using this code. Can you help me understand why you need the empty default matching cases on lines 55 and 57 of the new file here (i.e. the case _ => lines)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this was done to avoid MatchErrors. In the case where the type of the actual elaborated TestHarness does not match the expected Harness type, or in the case where type of the actual elaborated System does not match the expected System type, this expression just becomes a no-op.
This is useful because it lets us build up a collection of default HarnessBinders through Configs without thinking about exactly what hardware design is being requested.

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.

6 participants