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

[hmac,dv] Move IDLE check from vseq to scb #25735

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

Conversation

martin-velay
Copy link
Contributor

@martin-velay martin-velay commented Dec 20, 2024

- Move the IDLE pin check from the base sequence to the SCB as it makes
more sense to perform check there.

Signed-off-by: Martin Velay <mvelay@lowrisc.org>
@martin-velay martin-velay marked this pull request as ready for review December 20, 2024 16:26
@martin-velay martin-velay requested a review from a team as a code owner December 20, 2024 16:26
@martin-velay martin-velay self-assigned this Dec 20, 2024
Copy link
Contributor

@vogelpi vogelpi 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 good to me!

Can this really fix the issues with the stress_all_with_rand_reset test?

Comment on lines +85 to +86
cfg.clk_rst_vif.wait_clks(1); // Wait one clock cycle to be sure IDLE state will be set
check_idle(1'b1); // Check IDLE after a reset has occured
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks sensible, but would it make more sense put put it in the fork below? That way, tasks like hmac_process_fifo_status don't have to wait before they start.

@@ -139,7 +139,6 @@ task hmac_base_vseq::dut_init(string reset_kind = "HARD");
if (do_hmac_init) begin
hmac_init();
end
`DV_CHECK_EQ(cfg.hmac_vif.is_idle(), 1'b1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm extremely enthusiastic about taking this sort of thing out of the sequences. Thanks!

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

Successfully merging this pull request may close these issues.

3 participants