-
Notifications
You must be signed in to change notification settings - Fork 809
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
[racl] Follow-up fixes to TLUL adapter and reg_top #26056
Conversation
parameter bit RaclErrorRsp = 1'b1, | ||
parameter bit RaclErrorRsp = EnableRacl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm convinced that this is cleaner code, so it looks like the right thing to do. But does it actually change behaviour? I think the parameter is only used to qualify racl_error_o
. This, in turn, is only true if we hit a register that doesn't have a matching racl permission and every register gets a permission if EnableRacl
is false.
Is that correct? Probably worth explaining in the commit message why the change is needed (either cleaning stuff up or fixing some bug that Rupert didn't see!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No there is no change in behaviour. Is proposed by Adrian, this is jus a safeguard to not send out a spurious racl error if RACL is disabled. Nothing else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I included that in the commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rswarbrick If the racl_error_o generation logic is correct, such that RACL errors cannot be raised spuriously, then it has no functional impact on non-RACL designs. It's belt'n'braces.
cc3ec4a
to
7f69d97
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing up the rd_req/wr_req logic in the two adapters. Just the templated (+autogen) 'racl_error_o' logic that needs fixing now, I think.
parameter bit RaclErrorRsp = 1'b1, | ||
parameter bit RaclErrorRsp = EnableRacl, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rswarbrick If the racl_error_o generation logic is correct, such that RACL errors cannot be raised spuriously, then it has no functional impact on non-RACL designs. It's belt'n'braces.
56fb6b5
to
9f5d722
Compare
Signed-off-by: Robert Schilling <[email protected]>
…uest Signed-off-by: Robert Schilling <[email protected]>
The RACL address hit needs to be qualified with the associated read or write access signal. Otherwise, a denied RACL access might not get logged as en error. Signed-off-by: Robert Schilling <[email protected]>
This acts as a safe guard and ensures that there are no spuriouos error logs if RACL is disabled. This does not have any functional impact. Signed-off-by: Robert Schilling <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM.
CHANGE AUTHORIZED: hw/ip/i2c/rtl/i2c.sv |
CI failures unrelated. |
This PR is a follow up to the review in #26004 (review):
It adds: