Skip to content

Commit

Permalink
[rtl, prim_reg_cdc_arb] Conditional coverage simplification
Browse files Browse the repository at this point in the history
`end else if (dst_req_i && !dst_req_q && busy) begin` is equivalent to
`end else if (dst_req_i && busy) begin`.

In order to guarantee the above happens an assertion has been added checking:
`dst_req_i |-> !dst_req_q`

The reasoning for the simplification is:

In prim_reg_cdc, we expect to see a request that comes from the SW
as a signal on the src domain (either src_we_i or src_re_i).
This gets synchronised to the destination clock by
u_src_to_dst_req.
This then appears to the arbiter as dst_req_i.
If there is another request in flight (probably an update from HW),
the SW request get stored in the dst_req_q register.
The stored request gets cleared on the first posedge of the dst
clock after the previous request is complete, because it causes
dst_lat_d to be high.
To see dst_req_i && dst_req_q, we would need prim_reg_cdc to
send another request before that happened.
When the first SW request appears in prim_reg_cdc, we set busy
until we see src_ack.
This goes high when the SW request has been synchronised to the
dst clock by u_dst_update_sync.
As such, we can't see another request from prim_reg_cdc before
src_req_q has been cleared. Clearing src_req_q takes exactly one
clock crossing time and allowing prim_reg_cdc to send the second
request takes the same time.

This is part of the efforts to close code coverage for AON timer and taken as
feedback from Rupert in PR:
lowRISC#25705 (comment)

Signed-off-by: Antonio Martinez Zambrana <[email protected]>
  • Loading branch information
antmarzam committed Jan 28, 2025
1 parent 6f5b5dd commit 43754a0
Showing 1 changed file with 8 additions and 1 deletion.
9 changes: 8 additions & 1 deletion hw/ip/prim/rtl/prim_reg_cdc_arb.sv
Original file line number Diff line number Diff line change
Expand Up @@ -126,12 +126,19 @@ module prim_reg_cdc_arb #(
// dst_lat_d is safe to used here because dst_req_q, if set,
// always has priority over other hardware based events.
dst_req_q <= '0;
end else if (dst_req_i && !dst_req_q && busy) begin
end else if (dst_req_i && busy) begin
// if destination request arrives when a handshake event
// is already ongoing, hold on to request and send later
dst_req_q <= 1'b1;
end
end

// dst_req_q will be 0 when dst_req_i is set, this assertion checks the conditional branch
// (dst_req_i && !dst_req_q && busy) can be simplified to avoid conditional coverage
// holes
`ASSERT(Not_Dst_req_q_while_dst_req_i_A, dst_req_i |-> !dst_req_q,
clk_dst_i, !rst_dst_ni)

assign dst_req = dst_req_q | dst_req_i;

// Hold data at the beginning of a transaction
Expand Down

0 comments on commit 43754a0

Please sign in to comment.