Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[rom_ctrl, dv] Removal of a signal to cover cond coverage hole #25682

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KinzaQamar
Copy link
Contributor

@KinzaQamar KinzaQamar commented Dec 17, 2024

rom_ctrl coverage report shows a coverage hole for the conditional statement as 110 not covered. The description below explains why we can't cover the case counter_lnt and !kmac_rom_vld_o

Starting with counter_lnt = 1 means that we get the last address of a non top word. counter_lnt is delayed by a cycle through last_nontop_d in rom_ctrl_counter.sv.

Next is kmac_rom_vld_o which also delayed by a cycle through kmac_rom_vld_d. The kmac_rom_vld_d signal depends on counter_read_req which goes immediately high after reset. Then checker fsm state should be in state ReadingLow and
!counter_lnt meaning we haven't reached to the top.

Now assuming that we are out of reset, first request to the ROM has been sent, checker fsm is sending nontop data to KMAC and we still haven't reached to the top; makes kmac_rom_vld_d go high. But when we reach to the last address of the nontop data, we get counter_lnt go high and kmac_rom_vld_d go low. Since kmac_rom_vld_o delayed through kmac_rom_vld_d, we get counter_lnt with kmac_rom_vld_o.

rom_ctrl coverage report shows a coverage hole for the conditional
statement as 110 not covered. The description below explains why
we can't cover the case counter_lnt and !kmac_rom_vld_o

Starting with counter_lnt = 1 means that we get the last address of
a non top word. counter_lnt is delayed by a cycle through
last_nontop_d in rom_ctrl_counter.sv.

Next is kmac_rom_vld_o which also delayed by a cycle through
kmac_rom_vld_d. The kmac_rom_vld_d signal depends on counter_read_req
which goes immediately high after reset. Then checker fsm state should
be in state ReadingLow and !counter_lnt meaning we haven't reached to
the top.

Now assuming that we are out of reset, first request to the ROM has been
sent, checker fsm is sending nontop data to KMAC and we still haven't
reached to the top; makes kmac_rom_vld_d go high. But when we reach to
the last address of the nontop data, we get counter_lnt go high and
kmac_rom_vld_d go low. Since kmac_rom_vld_o delayed through
kmac_rom_vld_d, we get counter_lnt with kmac_rom_vld_o.

Signed-off-by: Kinza Qamar <[email protected]>
Copy link
Contributor

@rswarbrick rswarbrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks right to me, and thanks for adding the assertion at the bottom.

@andreaskurth: would you mind taking a look? I think this has zero risk (especially because the first symptom of a mistake will be a test failure)

@rswarbrick
Copy link
Contributor

CHANGE AUTHORIZED: hw/ip/rom_ctrl/rtl/rom_ctrl_fsm.sv

Carefully reasoned (and I agree with the logic!). And there's an assertion to make sure we find out if we made a mistake.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants