target/riscv: cleanup riscv_enumerate_triggers() #909
Conversation
src/target/riscv/riscv.c
Outdated
|
|
||
| r->triggers_enumerated = true; | ||
| r->trigger_count = t + 1; |
There was a problem hiding this comment.
This is incorrect. Why do you do this increment?
en-sc
force-pushed
the
en-sc/cleanup-enumerate-triggers
branch
from
4069033 to
d4cdb99
Compare
en-sc
force-pushed
the
en-sc/cleanup-enumerate-triggers
branch
from
d4cdb99 to
ea43a46
Compare
src/target/riscv/riscv.c
Outdated
| if (get_field(tinfo, CSR_TINFO_INFO) == 1) | ||
| /* trigger doesn’t exist */ | ||
| return ERROR_TARGET_RESOURCE_NOT_AVAILABLE; |
There was a problem hiding this comment.
This check has been made less defensive instead of more defensive.
The original code checked for tinfo==1 and tinfo==0.
Please, if you do not wish to extend the condition to the suggested tinfo & 0x1, leave at least the current state, unless there is very good reason to make the check weaker.
en-sc
force-pushed
the
en-sc/cleanup-enumerate-triggers
branch
3 times, most recently
from
ff1f07d to
9306ab4
Compare
| const bool trigger_count_available = | ||
| riscv_enumerate_triggers(target) == ERROR_OK; | ||
| riscv_print_info_line_if_available(CMD, "hart", "trigger_count", | ||
| r->trigger_count, trigger_count_available); |
There was a problem hiding this comment.
You shouldn't print "unavailable" if the problem is that the number of triggers could not be detected, because it could also be interpreted to mean that there are no triggers present. Something like "detection failed" is much more clear.
Instead of creating riscv_print_info_line_if_available, I prefer adding riscv_print_info_line_str and renaming riscv_print_info_line to riscv_print_info_line_int. Then the caller, which knows much more about the details, can decide what to show the user.
There was a problem hiding this comment.
According to this comment, the format of the riscv info command is intended to be used directly with TCL's array set.
So I have assumed that it is intended for the output to be machine-readable. Providing a programmer with a way to output an arbitrary string will greatly complicate the parsing of the output.
I can change the string that is currently used.
Currently:
hart.trigger_count unavailable
Reports that trigger_count is not available.
I can document this in doc/openocd.texi.
There was a problem hiding this comment.
Good point about the TCL array command. Please update the documentation and then leave the code as-is.
en-sc
force-pushed
the
en-sc/cleanup-enumerate-triggers
branch
2 times, most recently
from
8608f17 to
8092e4d
Compare
src/target/riscv/riscv.c
Outdated
| /* Mask off the top bit, which is used as tdrmode in old | ||
| * implementations. */ |
There was a problem hiding this comment.
It'd be good to adjust this comment per Tim's reply:
| /* Mask off the top bit, which is used as tdrmode in old | |
| * implementations. */ | |
| /* Mask off the top bit, which is used as tdrmode in legacy RISC-V Debug Spec | |
| * (old revisions of v0.11 spec). */ |
en-sc
force-pushed
the
en-sc/cleanup-enumerate-triggers
branch
from
8092e4d to
a90ec63
Compare
1. Propagate error codes. 2. Disable leftover triggers in case `tinfo` is supported. Change-Id: Ie20edb4d8b9245e13ac8757bf6afe549ac99c4f1 Signed-off-by: Evgeniy Naydanov <[email protected]>
en-sc
force-pushed
the
en-sc/cleanup-enumerate-triggers
branch
from
a90ec63 to
c286f93
Compare
Addressed. |
tinfois supported.Change-Id: Ie20edb4d8b9245e13ac8757bf6afe549ac99c4f1