target/riscv: extend trigger controls#1261
Conversation
lz-bro
force-pushed
the
extend-trigger-control
branch
2 times, most recently
from
a430bb2 to
867dc63
Compare
lz-bro
force-pushed
the
extend-trigger-control
branch
3 times, most recently
from
4d19092 to
019f89a
Compare
lz-bro
force-pushed
the
extend-trigger-control
branch
from
019f89a to
bfe7e7a
Compare
MarekVCodasip
left a comment
MarekVCodasip
left a comment
There was a problem hiding this comment.
Hello, thanks for taking the effort for this patch. Here some things that I have found.
|
|
||
| } else { | ||
| LOG_ERROR("First argument must be either 'set' or 'clear'."); | ||
| LOG_ERROR("First argument must be either 'set', 'clear' or 'list'."); |
There was a problem hiding this comment.
Please use LOT_TARGET_* wherever possible. This applies elsewhere as well.
There was a problem hiding this comment.
This is a a syntax error, Is it better to use LOG_ERROR?
There was a problem hiding this comment.
Yes, LOG_TARGET_, not LOT_TARGET_. Typo on my part. Sorry.
src/target/riscv/riscv.c
Outdated
| else if (!strcmp(CMD_ARGV[i], "exception")) | ||
| action = CSR_ITRIGGER_ACTION_BREAKPOINT; | ||
| else if (!strcmp(CMD_ARGV[i], "halt")) | ||
| action = CSR_ITRIGGER_ACTION_DEBUG_MODE; | ||
| else if (!strcmp(CMD_ARGV[i], "trace_on")) | ||
| action = CSR_ITRIGGER_ACTION_TRACE_ON; | ||
| else if (!strcmp(CMD_ARGV[i], "trace_off")) | ||
| action = CSR_ITRIGGER_ACTION_TRACE_OFF; | ||
| else if (!strcmp(CMD_ARGV[i], "trace_notify")) | ||
| action = CSR_ITRIGGER_ACTION_TRACE_NOTIFY; |
There was a problem hiding this comment.
It seems that this part of the code is repeated in many places, could try to refactor it?
It could be a function which takes a look on the last argument of the command and decides if its one of the valid trigger actions.
src/target/riscv/riscv.c
Outdated
| } | ||
|
|
||
| /* monotonic counter/id-number for match triggers */ | ||
| static int mtrigger_unique_id = -CSR_TDATA1_TYPE_MCONTROL6; |
There was a problem hiding this comment.
I am not sure that CSR_TDATA1_TYPE_MCONTROL6 is a good way to base the IDs on.
I think it would be better to define two constants, MCONTROL_MANUAL_TRIGGER_ID_START and MCONTROL_MANUAL_TRIGGER_ID_END and use those.
Breakpoints and watchpoints placed by OpenOCD start at 0 and count up, so ensure the range for manual triggers is in negative numbers.
There was a problem hiding this comment.
For each physical trigger contains:
-1: the trigger is available
-3: The trigger is used by the icount command
-4: The trigger is used by the itrigger command
-5: The trigger is used by the etrigger command
>= 0: unique_id of the breakpoint/watchpoint that is using it.
So, unique_id <= -6 could be used by the mcontrol command.
/* monotonic counter/id-number for match triggers */
static int mcontrol_unique_id = -CSR_TDATA1_TYPE_MCONTROL6;
unique_id = mcontrol_unique_id--;
5f64f45 to
852a9f9
Compare
* Add control for action mode. * Introduce `riscv mtrigger` command. * Introduce `riscv *trigger list` command
lz-bro
force-pushed
the
extend-trigger-control
branch
from
852a9f9 to
0744077
Compare
|
@MarekVCodasip can we continue merging or any comments? |
|
@lz-bro, I'd suggest you post the patch to mainline OpenOCD (https://review.openocd.org) -- the RISC-V part got updated there and now it is more recent then in this repo. |
|
@en-sc Thank you! I'll push the patch to mainline OpenOCD. |
3 participants
riscv mtriggercommand.riscv *trigger listcommandAction identifiers 2-5 are reserved for trace actions in the RISC-V Debug Specification, I would like to use them in the future .