[prim,rtl] Rewrite a case statement to avoid unreachable case

[rswarbrick]

Instead of the default case being unreachable (unless state_q is X), rewrite things so that the last item is caught by the default case.

This is clearly safe at the moment (there are just two possible states!) but this adds the GoodState_A assertion to make sure no future RTL change can make this unsafe.

[rswarbrick]

Note that this interacts with the coverage exclusions in #25705. If that PR lands before this one, we will need to amend this PR to drop some exclusions from the aon_timer manual exclusion list.

@antmarzam FYI (but I'm happy to do the leg work if it needs doing)

[antmarzam]

Note that this interacts with the coverage exclusions in #25705. If that PR lands before this one, we will need to amend this PR to drop some exclusions from the aon_timer manual exclusion list.

@antmarzam FYI (but I'm happy to do the leg work if it needs doing)

No worries, since you pushed this PR I'll clean up my other PR so we can get it merged on master!

[a-will]

Don't we keep these in case synthesis takes liberties with the state encoding? (e.g. converting to one-hot for some reason)

Maybe it's fine as long as there is a default, but the examples in the style guide all show "recovery" circuits.

@vogelpi @andreaskurth to help me in case my memory / understanding has gone rotten. ;)

[rswarbrick]

I think that we just need some default. The previous code was something like "If the state is silly then do X" and I propose change it to "If the state is silly, behave as if it is the following sensible value".

Clearly a bad idea if this is some sparsely encoded FSM in a security module! Here, we have exactly two states, represented by a single bit...

[vogelpi]

Can't we just put an empty default here, i.e., default:;. IMHO this would be more readable. Does this also create a coverage issue?

[vogelpi]

The empty default is also what we have in our style guide: https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md#case-statements

[rswarbrick]

Hmm. Good question: I'm not sure. I'll try it out.

[andreaskurth]

On a second thought, shouldn't UNR take care of this exclusion?

[antmarzam]

I think UNR would automatically exclude this case. Although we don't have VCS UNR licenses.

We do have a UNR waiver file from august: https://github.com/lowRISC/opentitan/blob/master/hw/ip/aon_timer/dv/cov/aon_timer_unr_excl.el

I think the reason why UNR didn't consider this case was unreachable may be that at the time UNR was run, the state signal was a 2-bit signal which perhaps made the tool believe the default state could be reached?
state signal was simplified to 1-bit only here

[rswarbrick]

Thanks for looking into this carefully. Part of my motivation for avoiding needing UNR here is that this is a primitive. It would be nicer not to depend on UNR at each instantiation site.

[andreaskurth]

Ah, I see. And a waiver would also be cumbersome because it would have to be written for every instantiation, right?

[andreaskurth]

Since this FSM has just two states, how about we rewrite to make it simpler and remove the default case, along the lines of:

logic idle_d, idle_q;

always_comb begin
  idle_d = idle_q;
  // ... other default assignments ...
  if (idle_q) begin
    // ...
  end else begin // not idle = waiting
    // ...
  end
end