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,rtl] FIFO Empty intr after FIFO seen full #23739

Merged
merged 1 commit into from
Jul 1, 2024

Conversation

martin-velay
Copy link
Contributor

  - fix RTL issue lowRISC#23738
  - prevent to raise a FIFO empty interrupt when not required in case
    the FIFO is seen full while a stop is triggered.

Signed-off-by: Martin Velay <[email protected]>
@martin-velay
Copy link
Contributor Author

FYI: in case somebody want to see the issue, you can get this commit 3551b1c and run the following command:
util/dvsim/dvsim.py hw/ip/hmac/dv/hmac_sim_cfg.hjson -i hmac_burst_wr -t xcelium --fixed-seed 54280119642660756066577499865407739571210164218606631048403677680909362592195 --gui

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.

Thanks @martin-velay , the fix looks good to me but I suggest NOT to merge this right now to NOT cause any schedule impact for the TO (for details see my reasoning in the linked issue #23739 ).

I am not against fixing this bug but I think the bug is not critical enough to risk any delays. If others feel completely differently, I don't want block this though.

@martin-velay martin-velay marked this pull request as draft June 20, 2024 14:45
@martin-velay
Copy link
Contributor Author

I have converted this PR into "draft" to avoid to get it merged by mistake. I'll change the status back into "ready for review" once the TO branch will have been created.

@vogelpi vogelpi added the NoECO_IntegratePostM5 Accepted for integration post M5 label Jun 25, 2024
@vogelpi
Copy link
Contributor

vogelpi commented Jun 25, 2024

As documented here, it has been decided in the triage meeting 2024-06-21 to not do an ECO for this but to merge it post M5.

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.

LGTM!

@vogelpi
Copy link
Contributor

vogelpi commented Jun 28, 2024

CHANGE AUTHORIZED: hw/ip/hmac/rtl/hmac.sv

@vogelpi vogelpi marked this pull request as ready for review June 28, 2024 08:57
@vogelpi
Copy link
Contributor

vogelpi commented Jun 28, 2024

I've marked the PR as ready and approved the changes. I don't suggest to merge this / ECO this but I would like this to be ready just in case another window for merging the change opens up again.

martin-velay added a commit to martin-velay/opentitan that referenced this pull request Jun 28, 2024
  - Remove this workaround when the PR lowRISC#23739 will have been merged

Signed-off-by: Martin Velay <[email protected]>
martin-velay added a commit to martin-velay/opentitan that referenced this pull request Jun 28, 2024
  - Remove this workaround when the PR lowRISC#23739 will have been merged

Signed-off-by: Martin Velay <[email protected]>
martin-velay added a commit to martin-velay/opentitan that referenced this pull request Jun 28, 2024
  - Remove this workaround when the PR lowRISC#23739 will have been merged

Signed-off-by: Martin Velay <[email protected]>
martin-velay added a commit to martin-velay/opentitan that referenced this pull request Jun 28, 2024
  - Remove this workaround when the PR lowRISC#23739 will have been merged

Signed-off-by: Martin Velay <[email protected]>
martin-velay added a commit to martin-velay/opentitan that referenced this pull request Jun 28, 2024
  - Remove this workaround when the PR lowRISC#23739 will have been merged

Signed-off-by: Martin Velay <[email protected]>
martin-velay added a commit to martin-velay/opentitan that referenced this pull request Jun 28, 2024
  - Remove this workaround when the PR lowRISC#23739 will have been merged

Signed-off-by: Martin Velay <[email protected]>
martin-velay added a commit to martin-velay/opentitan that referenced this pull request Jun 28, 2024
  - Remove this workaround when the PR lowRISC#23739 will have been merged

Signed-off-by: Martin Velay <[email protected]>
@andreaskurth
Copy link
Contributor

CHANGE AUTHORIZED: hw/ip/hmac/rtl/hmac.sv

In case there's another synthesis opportunity; otherwise we'll have to revert this.

@martin-velay
Copy link
Contributor Author

CHANGE AUTHORIZED: hw/ip/hmac/rtl/hmac.sv

In case there's another synthesis opportunity; otherwise we'll have to revert this.

@andreaskurth, if there is no other opportunity to integrate this fix in this TO, we can still merge this fix into the master afterward. No?

@vogelpi
Copy link
Contributor

vogelpi commented Jul 1, 2024

I am merging this now to create the next RC. We might need to revert this if we decide not to re-start synthesis. In this case, we would merge the change later to have it in potential follow-up spins / a future TO.

@vogelpi vogelpi merged commit 6409aa6 into lowRISC:master Jul 1, 2024
33 of 34 checks passed
@martin-velay
Copy link
Contributor Author

@vogelpi: as this RTL fix has been merged, should I keep this temporary workaround commit 55e2027?

@vogelpi
Copy link
Contributor

vogelpi commented Jul 1, 2024

@vogelpi: as this RTL fix has been merged, should I keep this temporary workaround commit 55e2027?

Please keep it in a local branch of yours in case we need to revert the RTL fix, thanks.

@martin-velay
Copy link
Contributor Author

@vogelpi: as this RTL fix has been merged, should I keep this temporary workaround commit 55e2027?

Please keep it in a local branch of yours in case we need to revert the RTL fix, thanks.

Done! #23749 (comment)

@martin-velay martin-velay deleted the fifo_empty branch July 17, 2024 07:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NoECO_IntegratePostM5 Accepted for integration post M5
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants