-
Notifications
You must be signed in to change notification settings - Fork 611
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 support for asynchronous reset #1011
Conversation
Add new AsyncReset type that extends trait Reset. The "reset-type" of a register is set by the type of the in scope Reset: val asyncReset: AsyncReset = IO(Input(AsyncReset())) val syncReset: Bool = IO(Input(Bool())) val asyncReg = withReset(asyncReset) { RegInit(0.U) } val syncReg = withReset(syncReset) { RegInit(0.U) } AsyncReset can be cast to and from Bool. Whereas synchronous reset is equivalent to a mux in front of a flip-flop and thus can be driven by logic, asynchronous reset requires that the reset value is a constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some concerns and questions in the code comments
Summary from Chisel Dev meeting 15/02/2019:
Per that last point, I think the Chisel API on a Module-level is great: class MyModule extends RawModule {
val reset = IO(Input(AsyncReset()))
...
}
// elsewhere
val mod = Module(new MyModule)
mod.reset := nonAsyncReset If The case I'm a little less clear on is how you do something like this in a function. The best I can come up with: def myFunc(x: UInt): UInt = {
// I need to specify that the reset *must* be async, but Module.reset may have type Reset
val myAsyncReg = withReset(WireInit(AsyncReset(), Module.reset)) {
RegInit(123.U)
}
myAsyncReg := x
myAsyncReg
} Note that it might feel obvious that something like this would work, but it will not: val myAsyncReset = withReset(Module.reset.asAsyncReset) { RegInit(123.U) } .as* casts are reinterpretations, as such EDIT: to clarify a bit, using On the bright side, the future "check async resets are properly used" (similar to a check for legal clock crossings), will catch misuse here, so maybe this just isn't a problem |
@jackkoenig any updates on this PR? |
@johnsbrew I apologize for the delay (and then my delay in responding). I am working on it, you can see my WIP (https://github.com/freechipsproject/firrtl/tree/infer-reset). Reset inference has definitely been trickier than I thought, but I think I'm relatively close, I'll let you know when it seems to work 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly looks good, but potential leftover / unused code.
Also, you still have a documentation TODO.
def apply(): AsyncReset = new AsyncReset | ||
} | ||
|
||
// TODO: Document this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO that needs doing
@@ -43,6 +43,8 @@ object Module extends SourceInfoDoc { | |||
// Save then clear clock and reset to prevent leaking scope, must be set again in the Module | |||
val clockAndReset: Option[ClockAndReset] = Builder.currentClockAndReset | |||
Builder.currentClockAndReset = None | |||
// Record the outer reset for determining type of implicit reset in MultiIOModules | |||
Builder.outerReset = clockAndReset.map(_.reset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this used anywhere?
Resets in compatibility Modules will remain Bools
So there's a remaining issue with interoperability with compatibility mode. Here are the two options I see:
(1) is the current issue, it seems like it'd be the best at first blush but I think every single instantiation of a Chisel Module in chisel3 code breaks |
This involves casting the Reset type of chisel3 Modules to Bool for Chisel Modules
Is |
@jackkoenig I just noticed that a piece of code that used to work with import chisel3._
import chisel3.experimental.RawModule
class Hello extends RawModule {
val clock = IO(Input(Clock()))
val reset = IO(Input(Reset()))
val in = IO(Input(UInt(16.W)))
val out = IO(Output(UInt(16.W)))
withClockAndReset(clock, reset) {
out := RegNext(in, 12345.U)
}
}
class HelloWorld extends RawModule {
val clock = IO(Input(Clock()))
val mux = IO(Input(Bool()))
val in = IO(Input(UInt(16.W)))
val out = IO(Output(UInt(16.W)))
// reset2 fails with AsyncReset, works with Bool()
//val reset = IO(Input(Bool()))
val reset = IO(Input(AsyncReset()))
// Neither of the two variants below work if reset is AsyncReset
// Both variants work if reset is Bool
val reset2 = IO(chiselTypeOf(reset))
//val reset2 = IO(Input(chiselTypeOf(reset)))
val m = Module(new Hello)
m.clock := clock
m.reset := Mux(mux, reset, reset2)
m.in := in
out := m.out
}
chisel3.Driver.execute(Array[String](), () => new HelloWorld) Error message:
|
@edwardcwang thanks for the heads up! I'll investigate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few documentation nits (including a TODO that maybe needs to be done) and a question about a test.
|
||
behavior of "Reset" | ||
|
||
it should "allow writing modules that are reset agnostic" in { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it also worth testing that this work with the implicit clock and reset in Module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tests using a Module
are testing this, in particular, AsyncResetQueueTester
ensures that Queue
works with async reset.
Done, obviously still fails until chipsalliance/firrtl#1068 is merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm.
Consider adding summary technical documentation as release notes: adds AsyncReset type and generic Reset type that can be (inferred in FIRRTL to) Bool or AsyncReset. Largely exposes FIRRTL types in the Chisel API, most of the heavy lifting is in FIRRTL.
Hi there! override val reset = IO(Input(AsyncReset())) to my LazyModuleImp definition but run into a FIRRTL error. Is there documentation somewhere on how to add an async reset? Thanks! |
There's is a PR of documentation for the website that needs a little cleanup but has useful information: freechipsproject/www.chisel-lang.org#74 |
Great, thanks for the help! |
The reset documentation was really helpful! I looked around and saw that class MyAlwaysAsyncResetModule extends Module with RequireAsyncReset {
val myAsyncResetReg = RegInit(false.B) // reset is of type AsyncReset
} I get a type not found error for requireAsyncReset. I think I'm on the most recent version of Chisel - is there some other package that needs to be imported? Thanks! |
The In the meantime, if you really want this feature, this may be in a snapshot release on Sonatype (oss.sonatype.org appears to be down right now...). You can also publish FIRRTL and Chisel3 locally and use that. However, I wouldn't advise either of these approaches unless you're intending to do development of a Chisel3 library. Sorry about that. |
Hey seldridge, Thanks for the follow up! Is there an alternative way to implement an async reset that doesn't require Chisel 3.3? I'm not wedded to using the Thanks for all the hard work, and hope you're staying safe! Tim |
yes, if you have a higher level wrapping module you can do something like:
|
Thanks for the quick reply! We're actually looking for a top level way of making our entire design asynchronously reset instead of just a single module. Is there a top level parameter that can make every reset asynchronous rather than changing individual modules? Thanks! Tim |
In @chisel3.experimental.chiselName
class MyTopModule extends RawModule {
val clock = IO(Input(Clock()))
val reset = IO(Input(AsyncReset()))
// rest of ports
withClockAndReset(clock, reset) {
// module instantiations and logic here
// vals in here will get named by chiselName
}
} |
@timsliu i had the exact same question ;p |
Hi everyone, Thanks for all the quick support! I'm using Chipyard 1.2. I tried using a RawModule but the problem I have is the Top already extends another class, and so I can't make it also extend RawModule. I've tried wrapping where my Top is built with a RawModule: @chisel3.experimental.chiselName
class MyTopModule extends RawModule {
val clock = IO(Input(Clock()))
val reset = IO(Input(AsyncReset()))
// rest of ports
withClockAndReset(clock, reset) {
// module instantiations and logic here
Module(LazyModule(new Top()(p)).suggestName("top").module)
}
} and then using this wrapper when I instantiate Top: case object BuildTop extends Field[Parameters => Any]((p: Parameters) => Module(new MyTopModule(p))) But it leads to a FIRRTL error
I'm not sure how to change the type of @mwachs5 I've also tried your suggestion and built my Top like so: case object BuildTop extends Field[Parameters => Any] ((p: Parameters) => {
withReset(reset.asAsyncReset){
Module(LazyModule(new Top()(p)).suggestName("top").module)
}
} But here Thanks! Tim |
Hey again! Just wanted to check back in and see where Chisel 3.3 is and if Thanks! Tim |
Chisel 3.3.0-RC1 was published on Friday, so this is now available (and you can verify by querying the latest API documentation): |
Great, I'll check it out. Thanks for all the hard work on this!! |
Fixes #219 * Adds AsyncResetType (similar to ClockType) * Registers with reset signal of type AsyncResetType are async reset registers * Registers with async reset can only be reset to literal values * Add initialization logic for async reset registers
Add new AsyncReset type that extends trait Reset. The "reset-type" of a
register is set by the type of the in scope Reset:
AsyncReset can be cast to and from Bool. Whereas synchronous reset is
equivalent to a mux in front of a flip-flop and thus can be driven by
logic, asynchronous reset requires that the reset value is a constant.
Fixes #343
This relies on
chipsalliance/firrtl#1011and chipsalliance/firrtl#1068Related issue:
Type of change: other enhancement
Impact: API addition (no impact on existing code)
Development Phase: implementation
Release Notes
Add support for asynchronous reset