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

[Proposal] Stronger and more complete reset types #161

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

darthscsi
Copy link
Collaborator

Make all resets use a uniform type. Expand that type to cover asynchronous set, synchronous release resets. Expand that type to handle both active-high and active-low resets.

Provide a couple of ops that allow writing generic code on uninferred reset signals.

Tighten type conversion to make UInt<1> not equivalent to synchronous reset.

spec.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@mwachs5 mwachs5 left a comment

Choose a reason for hiding this comment

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

This looks great!

One thing to discuss is whether the abi can allow us to put a _n on an active low reset signal once it's lowered to verilog

spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
spec.md Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
spec.md Outdated
The reset signal must be a `Reset`{.firrtl}, `UInt<1>`{.firrtl}, or `AsyncReset`{.firrtl}, and the type of initialization value must be equivalent to the declared type of the register (see [@sec:type-equivalence] for details).
If the reset signal is an `AsyncReset`{.firrtl}, then the reset value must be a constant type.
The reset signal must be a `Reset`{.firrtl} or `Reset<_,_>`{.firrtl}, and the type of initialization value must be equivalent to the declared type of the register (see [@sec:type-equivalence] for details).
If the reset signal is an `Reset<async,_>`{.firrtl} or an `Reset<asyncsync,_>`{.firrtl}, then the reset value must be a constant type.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to include a one-sentence justification for why this is required.

spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
@mmaloney-sf
Copy link
Collaborator

I love this.

I added a few comments, mostly in terms of the presentation.

Copy link
Collaborator

@jackkoenig jackkoenig left a comment

Choose a reason for hiding this comment

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

This looks great!

It would be really useful to have an example or two of generic reset handling code. In particular we need ways to invert and do some simple boolean operations (eg. OR) resets generically where inverting a reset should change the Active while keeping the kind--I'd just like to understand how to build these operations using the existing primitives. In particular, if I have a Reset<_, _> I need to invert it and get back effectively Reset<_, !_> in a way that reset inference can still fill in the holes.

spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
spec.md Outdated Show resolved Hide resolved
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.

5 participants