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

[kmac, rtl/dv/sw] Remove unused error code #25035

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

nasahlpa
Copy link
Member

@nasahlpa nasahlpa commented Nov 7, 2024

The ErrShadowRegUpdate code is never used in the RTL. Instead, according to the documentation, on a shadow register update error, a recoverable alert shall be triggered - this recoverable alert then is signaled in the STATUS register and not in the error code register.

Hence, this commit removes this error code as well as the corresponding SW code (kDifErrorShadowRegisterUpdate).

Comment on lines 459 to 466
// Error Shadow register update
ErrShadowRegUpdate = 8'h C0,

// Error due to lc_escalation_en_i or fatal fault
ErrFatalError = 8'h C1,
ErrFatalError = 8'h C0,

// Error due to the counter integrity check failure inside MsgFifo.Packer
ErrPackerIntegrity = 8'h C2,
ErrPackerIntegrity = 8'h C1,

// Error due to the counter integrity check failure inside MsgFifo.Fifo
ErrMsgFifoIntegrity = 8'h C3
ErrMsgFifoIntegrity = 8'h C2
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the "right" way to handle this would be just remove ErrShadowRegUpdate without modifying the other encodings. Because touching the other encodings would mean an change to the software interface: software compliant with A0/A1 would only be partially compatible. This would mean a version increment. We should avoid this at this point.

Maybe it we should just remove it now but add a note somewhere (in an issue?) to edit also the other encodings next time we do an interface change anyway?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

My assumption was that SW from the master branch anyways is not compatible with A0/A1 (for different reasons). Hence, I decided to make the change more clean by also moving the error codes.

I created an issue (#25052) for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Pascal! You're right about this but we still want to be careful about introducing breaking changes into the IPs if possible.

The `ErrShadowRegUpdate` code is never used in the RTL. Instead, according
to the documentation, on a shadow register update error, a recoverable alert
shall be triggered - this recoverable alert then is signaled in the STATUS
register and not in the error code register.

Hence, this commit removes this error code as well as the corresponding
SW code (`kDifErrorShadowRegisterUpdate`).

Signed-off-by: Pascal Nasahl <[email protected]>
@vogelpi
Copy link
Contributor

vogelpi commented Nov 8, 2024

CHANGE AUTHORIZED: hw/ip/kmac/rtl/kmac_pkg.sv

This PR touches an RTL file to remove an unused error code. The used error codes are not touches. This is okay.

@nasahlpa nasahlpa marked this pull request as ready for review November 8, 2024 10:46
@nasahlpa nasahlpa requested review from a team as code owners November 8, 2024 10:46
@nasahlpa nasahlpa requested review from matutem and jadephilipoom and removed request for a team, andreaskurth, matutem and jadephilipoom November 8, 2024 10:46
@nasahlpa
Copy link
Member Author

nasahlpa commented Nov 8, 2024

CHANGE AUTHORIZED: hw/ip/kmac/rtl/kmac_pkg.sv

This PR touches an RTL file to remove an unused error code. The used error codes are not touches. This is okay.

@nasahlpa nasahlpa merged commit 2c5ba75 into lowRISC:master Nov 11, 2024
41 checks passed
@nasahlpa nasahlpa deleted the kmac_del_err_shadow_reg_update branch November 11, 2024 06:18
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