-
Notifications
You must be signed in to change notification settings - Fork 332
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
target/riscv: allow hexadecimal values to expose_csr-like commands #1162
Conversation
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.
Small comments
6f88874
to
92532db
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.
LGTM (reviewed internally).
I will be able to review this change by the end of the week. |
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.
LGTM, thanks.
I am sending one optional suggestion and one general comment.
LGTM, I wanted this feature too but haven't really put in the time to implement it yet, thank you for implementing it |
92532db
to
13bc1fa
Compare
Note to reviewers: the following checks were added in most recent update:
Would you kindly take a look? |
This comment seems a bit misleading because in the code there is a prior explicit check for On a more general point, why not just allow decimal numbers that start with |
But there is a precondition: "If we are here". Meaning we've already checked for hexadecimal numbers a few lines above.
Because it suspiciously looks like someone copy-pasted a hexadecimal number from some documentation and forgot to add a leading "0x".
Same point as above.
People may copy-paste a hexadecimal number. |
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 have submitted few last minor comments and I do not expect to have any more. Other than that the change looks fine.
af1fb32
to
b72d80d
Compare
hexadecimal values are often used in the documentation. Forcing user to convert CSRs addresses to decimal is unnecessary.
b72d80d
to
ba8c1ee
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.
LGTM, thank you.
@TommyMurphyTM1234 could you please clarify is my reasoning is fine with you? Or do you still have objections or concerns? |
@TommyMurphyTM1234 ping) |
Well, if no additional objections follow, I'm going to merge this by the end of week (around Friday). |
Apologies for the delay - I have no strong objections - definitely none that should hold this up further. |
/* Register prefix: "csr_" or "custom_" */ | ||
strcpy(name, reg_type); | ||
name[strlen(reg_type)] = '_'; |
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.
This MR introduced a regression since these lines were deleted. It seems that I've made a mistake while my changes were rebased (our internal change reviewed by @en-sc had this code). The regression was detected by our internal testsuite.
I'll post a patch shortly and will try to extend riscv-tests.
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.
MR with the fix: #1176
No description provided.