-
Notifications
You must be signed in to change notification settings - Fork 331
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: new ebreak
controls
#1168
base: riscv
Are you sure you want to change the base?
Conversation
6b1faa6
to
ccb13a1
Compare
@JanMatCodasip, @MarekVCodasip ping |
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.
Overall the change looks all right - thank you.
I have finished my review and do not expect to have any further comments.
src/target/riscv/riscv.c
Outdated
{ | ||
struct riscv_private_config *pc = target->private_config; | ||
if (!pc) { | ||
pc = default_riscv_private_config(); |
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.
There is an issue with private_config
deallocation in case of an error.
Please, see https://review.openocd.org/c/openocd/+/8637/comments/512687b3_e6274fb6
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.
Should be fixed by https://review.openocd.org/c/openocd/+/8642
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.
The issue is somewhat minor and is not specific to this patch.
IMHO, the patch can be merged without the fix.
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 agree - no problem with merging this now.
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.
Thank you for addressing the suggestions so far.
I have noticed three more items, and shouldn't have more comments than that.
* Deprecate `riscv set_ebreak*` commands. * Introduce RISC-V-sepecific `configure` parameter `-ebreak` instead. * Separate controls for VS and VU modes. Change-Id: I0ff88318dcb52af6923eb9f20f9d0c056ad09cf0 Signed-off-by: Evgeniy Naydanov <[email protected]>
Please, note: In the last change I didn't only address review comments. |
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.
if (e != JIM_OK) | ||
return e; | ||
|
||
if (!goi->isconfigure && goi->argc > 0) { |
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.
Needs to be adjusted after !1186 is merged due to 8c739a4
riscv set_ebreak*
commands.configure
parameter-ebreak
instead.Change-Id: I0ff88318dcb52af6923eb9f20f9d0c056ad09cf0