lowRISC / opentitan

OpenTitan: Open source silicon root of trust
https://www.opentitan.org
Apache License 2.0
2.51k stars 745 forks source link

[prim_ram] Add reset signal to prim_ram_1p/prim_ram_2p? #2494

Open imphil opened 4 years ago

imphil commented 4 years ago

prim_ram_1p/prim_ram_2p currently don't have a reset signal input, as they don't need one. (Note that the _adv wrappers have a rst signal; this discussion is about the underlying primitives.)

For writing assertions, a reset signal, however, is helpful. Other use cases might come to mind.

I don't have a strong feeling either way, WDYT @tjaychen @msfschaffner @sjgitty @sriyerg?

rswarbrick commented 4 years ago

Another option might be to stick the assertions in a "protocol checker" module which you instantiate whenever you actually use the primitive (maybe with a macro or wrapper module; maybe not). That might make it a bit more obvious which bits are part of the actual design and which bits are just for sanity checks.

tjaychen commented 4 years ago

i'm not against adding a reset for assertion purposes. If the reset really ends up being unloaded, in the final round of synthesis it will be pruned anyways.

is this related to Michael's PR where he just uses a hardcoded 0 for one of the memory assertions?

On Mon, Jun 15, 2020 at 2:55 AM Rupert Swarbrick notifications@github.com wrote:

Another option might be to stick the assertions in a "protocol checker" module which you instantiate whenever you actually use the primitive (maybe with a macro or wrapper module; maybe not). That might make it a bit more obvious which bits are part of the actual design and which bits are just for sanity checks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/lowRISC/opentitan/issues/2494#issuecomment-644028930, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAH2RSQR3D47CUEGDFPIIX3RWXVYZANCNFSM4N57ZWWQ .

imphil commented 4 years ago

is this related to Michael's PR where he just uses a hardcoded 0 for one of the memory assertions?

Yes, I filed this issue to discuss if we want resets, or not, and to unblock #2487 in the meantime.

msfschaffner commented 4 years ago

I am also ok with adding a reset signal to these wrappers for assertion purposes, since as Tim mentioned, it will just be pruned away if unused. The protocol checker would also be an option, but for this one assertion I think we need not necessarily introduce another module...

tjaychen commented 2 years ago

This is now related to #12602 In addition to bringing in a reset, in general it would be nice to have an init function so that software would not need to do it manually in the future.

msfschaffner commented 11 months ago

Related issue #2263