-
Notifications
You must be signed in to change notification settings - Fork 353
target/riscv: extend trigger
controls
#1261
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
base: riscv
Are you sure you want to change the base?
Conversation
a430bb2
to
867dc63
Compare
4d19092
to
019f89a
Compare
019f89a
to
bfe7e7a
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use LOT_TARGET_* wherever possible. This applies elsewhere as well.
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 is a a syntax error, Is it better to use LOG_ERROR?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
852a9f9
to
0744077
Compare
riscv mtrigger
command.riscv *trigger list
commandAction identifiers 2-5 are reserved for trace actions in the RISC-V Debug Specification, I would like to use them in the future .