[dd, prim_reg_cdc] simplifying condition to help with CCOV

[antmarzam]

prim_reg_cdc has a conditional coverage hole in the condition: src_busy_q && src_ack since src_ack is only 1 if src_busy_q. this is also ensured via the assertion SrcAckBusyChk_A

[rswarbrick]

Hi @antmarzam! I think this has stalled a bit? My review was just nitty stuff: I think this should be good to go otherwise! Would you mind tidying the little things up so we can merge it?

[rswarbrick]

Thanks, this looks good to me.

[rswarbrick]

CHANGE AUTHORIZED: hw/ip/prim/rtl/prim_reg_cdc.sv

This shouldn't have any functional impact, which is actually checked by an assertion that has been in place for a long time (and hasn't fired, to my knowledge!). So the risk to earlgrey is essentially zero.

[vogelpi]

Thanks for pinging me on this one @rswarbrick . I would like @andreaskurth to take a look at this when he returns (next week).

At a first glance, I think the proposed change here doesn't work. Because src_ack is driven by prim_reg_cdc_arb.sv based on src_req and id_q. Both these signals are in the destination clock domain while src_busy_q sits in the source domain. Related to this, there is a comment in the header of the file saying:

// Note on domain resets
//
// When a single domain is reset, assertions of the internal signals 'dst_req'
// and/or 'src_ack' may occur as the two ends of a pulse synchronizer
// (prim_pulse_sync or prim_sync_reqack, NRZ option) are briefly inconsistent,
// generating a spurious pulse at the destination.
//
// These pulses are prevented from propagating outside of this module, provided
// that the reset does not occur whilst a transaction is in progress; firmware
// is responsible for preventing that.

Please let's hold off on this one until our CDC expert Andreas can assess this.

[rswarbrick]

Looking into this carefully, I'm rather confused! I've just done a bit
of tracing and src_ack comes from the src_ack_o port of u_arb.
This is computed as:

    assign src_ack_o = src_req & (id_q == SelSwReq);

where src_req is driven by u_dst_update_sync on clk_src_i and
id_q is updated with an always_ff block on clk_dst_i.

I think those are two different clocks! (It's not particularly obvious
because u_dst_update_sync has its src and dst arguments passed
the other way around).

Is that correct?

[vogelpi]

Yes, I might have missed that src_req is from the source domain but there are definitively two clocks getting mixed here and this makes things very hard. Let's wait for Andreas.

[andreaskurth]

While I agree that the original code this PR suggests to change isn't ideal and may be problematic for coverage closure, I strongly recommend we don't change it at this point. The reasoning is as follows:

• Logic involved in CDCs needs to be checked and signed off with tools outside RTL simulation. This has happened with the current code but not for the change suggested in this PR.
• The mentioned assertion, even if it holds for all our RTL simulations, doesn't guarantee that there are no boundary cases that we don't hit in RTL simulation (e.g., due to clock jitter, clock drift, staggered resets). As such, the assertion is necessary but not sufficient for correctness; CDC sign-off tools should cover those boundary cases.
• For future tape-outs and to strengthen our open-source sign-off methodology, we may want to overhaul this code more generally. As part of this, we have to work with CDC sign-off tools as well as RTL simulations. I suggest we continue the coverage closure efforts on this code when we do that.

For now, I recommend waiving this coverage hole because it falls outside the responsibility of RTL simulation.

[antmarzam]

recommend waiving this coverage hole because it falls ou

While I agree that the original code this PR suggests to change isn't ideal and may be problematic for coverage closure, I strongly recommend we don't change it at this point. The reasoning is as follows:

• Logic involved in CDCs needs to be checked and signed off with tools outside RTL simulation. This has happened with the current code but not for the change suggested in this PR.
• The mentioned assertion, even if it holds for all our RTL simulations, doesn't guarantee that there are no boundary cases that we don't hit in RTL simulation (e.g., due to clock jitter, clock drift, staggered resets). As such, the assertion is necessary but not sufficient for correctness; CDC sign-off tools should cover those boundary cases.
• For future tape-outs and to strengthen our open-source sign-off methodology, we may want to overhaul this code more generally. As part of this, we have to work with CDC sign-off tools as well as RTL simulations. I suggest we continue the coverage closure efforts on this code when we do that.

For now, I recommend waiving this coverage hole because it falls outside the responsibility of RTL simulation.

Thank you @andreaskurth , I've created an issue to better track this. Issue is here.

I'll file a new PR adding exclusions for this condition!

[andreaskurth]

Sounds good, thanks @antmarzam. I'll close this PR for the time being.