-
Notifications
You must be signed in to change notification settings - Fork 327
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: Mismatch napot when mcontrol.maskmax is not expected #1127
Conversation
Checkpatch fails:
Please, add an empty line between commit title and description. |
2a520d3
to
df64a13
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.
Only minor comments, nothing significant.
@JanMatCodasip, @MarekVCodasip, could you please take a look?
On a side note:
@sunnyzhu-learning, have you tested the change? What was the simulator/HW used?
Q:have you tested the change? What was the simulator/HW used?
|
df64a13
to
f0bbdb3
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
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 your contribution! Overall, I like the improvement.
I do have several review comments, though. If you can please look at them.
Thank you.
f0bbdb3
to
7811d03
Compare
7811d03
to
90162b3
Compare
@sunnyzhu-learning, what is the reason for closing the PR? |
reopen this PR,solved code merge conflict,I'm very sorry,poor use of PR,@en-sc @JanMatCodasip please code review,thanks.... |
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.
@sunnyzhu-learning Thank you for addressing our feedback so far.
I have several more comments. If you can please look at them.
bf0451f
to
f05c6c1
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.
@sunnyzhu-learning Thank you for addressing the review feedback so far.
I have posted two last comments (and that should be all from me).
f05c6c1
to
4a405a0
Compare
1.Remove trigger_request_info::tdata1_ignore_mask 2.Adding ignore napot matching condition Signed-off-by: Songhe Zhu <[email protected]> Reviewed-by: Evgeniy Naydanov <[email protected]> Reviewed-by: Jan Matyas <[email protected]>
4a405a0
to
85c836b
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.
@sunnyzhu-learning, thank you for your contribution.
assert(sizeof(riscv_reg_t) * 8 == 64); | ||
for (unsigned int i = 0; i < 64; i++) { |
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.
assert(sizeof(riscv_reg_t) * 8 == 64); | |
for (unsigned int i = 0; i < 64; i++) { | |
const unsigned int riscv_reg_bits = sizeof(riscv_reg_t) * CHAR_BIT; | |
for (unsigned int i = 0; i < riscv_reg_bits; i++) { |
if ((1 & (reg >> i)) == 0) | ||
return i; | ||
} | ||
return 64; |
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.
return 64; | |
return riscv_reg_bits; |
@en-sc Sorry,No timely feedback,I took a seven-day vacation,next PR will modify above changes~ |
From #1124,
1.Rename
trigger_request_info::tdata1_ignore_mask
->trigger_request_info::maskmax
.2.Add the something like the following to
set_trigger()
:addressed
@en-sc Thanks for your suggestion, internal discussion a lot, and first push code to github,delay....please review~