[dd, aon_timer cov_closure] CCOV exclusion file for aon_timer #25705

antmarzam · 2024-12-19T11:29:59Z

All the exclusions in this file are unreachable

hw/ip/aon_timer/dv/cov/aon_timer_unr_manually_excluded_excl.el

rswarbrick · 2024-12-19T15:05:15Z

hw/ip/aon_timer/dv/cov/aon_timer_unr_manually_excluded_excl.el

+Condition 2 "1682497780" "(dst_req_i && ((!gen_wr_req.dst_req_q)) && gen_wr_req.busy) 1 -1" (2 "101")
+CHECKSUM: "2815520955 4109606122"
+INSTANCE: tb.dut.u_reg.u_wkup_count_lo_cdc.u_arb
+ANNOTATION: "It can't be hit because dst_update_ack won't be set prior to dst_update_req"


I think this is the same comment as the one for line 117?

I've created a PR for this: #25814

rswarbrick

I know that I haven't got to the bottom of the list, but I think there are a few items to work through so far :-)

rswarbrick · 2025-01-23T13:07:25Z

I think this is easy, but should these two commits be squashed?

` end else if (dst_req_i && 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]>

`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]>

`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: #25705 (comment) Signed-off-by: Antonio Martinez Zambrana <[email protected]>

antmarzam · 2025-01-29T17:46:47Z

@rswarbrick I've removed the exclusions that have been addressed via RTL modifications and have squashed both commits together!

Also @vogelpi : Do you mind taking a look at comment it would be helpful to get your two cents on it :)

All the exclusions in this file are unreachable In addition, rtl exclusions added to config file Signed-off-by: Antonio Martinez Zambrana <[email protected]>

antmarzam · 2025-01-30T18:28:35Z

@rswarbrick : I've just pushed the changes to removed the exclusion that's no longer needed after your PR #26072 makes it to master

rswarbrick · 2025-01-30T18:28:39Z

I've just worked through the discussion on this PR and I'm pretty convinced! :-)

@antmarzam: Do you think this exclusion list is correct for the current head of master? If so, I think this should be good to merge.

antmarzam · 2025-01-30T18:33:56Z

Yeah I think it's ready to be merged :)

rswarbrick · 2025-01-31T12:46:54Z

This is great, thanks!

antmarzam requested a review from a team as a code owner December 19, 2024 11:30

antmarzam requested review from hcallahan-lowrisc, rswarbrick and a team and removed request for a team December 19, 2024 11:30