target/riscv: Add support for external triggers #1179
Conversation
doc/openocd.texi
Outdated
| @@ -11476,6 +11476,11 @@ The second argument configures how OpenOCD should use the selected trigger featu | |||
| With no parameters, prints current trigger features configuration. | |||
| @end deffn | |||
|
|
|||
| @deffn {Command} {riscv set_external_trigger} value | |||
| Associate the supplied external trigger with the SMP halt group for the harts. When the external trigger | |||
There was a problem hiding this comment.
I believe that external trigger does not necessarily implies and SMP group.
There was a problem hiding this comment.
Thank you - i've dropped the word "SMP" from the documentation.
There was a problem hiding this comment.
If we decide to keep the status quo -- keep using halt groups for SMP groups -- then a simple Tcl command like this looks all right to me.
Few suggestions:
- One halt group can have multiple external triggers (1:N relationship). So I'd call the command
addrather thanset. - Since the command is only used in the configuration phase (COMMAND_CONFIG), it looks unnecessary to have a
removecommand to remove the trigger from the group. - If the current relationship between halt groups and SMP stays in place, then the command name can contain
smp-- as an additional hint for the user that it will apply to the whole SMP group. - The command should contain
haltorhalt_groupsomewhere it its name to indicate it is for halt groups, not for resume groups.
For example:
[target_name] riscv smp_add_ext_trigger halt_group <ext_trigger_number>
# halt_group would remain the only supported trigger type for the time beingAdd support for associating a halt group with an external trigger via a newly exposed configuration option "riscv set_external_trigger". Change-Id: If10c67d2e14d8bc7cd6d59011b3215fda4ff4b02 Signed-off-by: Rob Bradford <[email protected]>
rbradford
force-pushed
the
external-triggers
branch
from
e9fef03 to
cfd4dc2
Compare
|
@rbradford I've left some additional comments. Though, I believe we are far from done :(. The more I look at this patch and the the current codebase - the more concerns I have. Please, keep in mind that the current implementation of halt groups in OpenOCD is very questionable leading to issues like #902 . I would go as far as to say that the concept of halt groups is not implemented properly. The more I look at the relevant codebase - the more it seems to me that proper support for halt/resume groups should be implemented first. Once that is done we could adopt something like this patch. Others could have a different opinion, though - @JanMatCodasip , @en-sc do you have any comments regarding the matter? |
| @@ -2051,6 +2077,9 @@ static int examine(struct target *target) | |||
| else | |||
| LOG_TARGET_INFO(target, "Core %d could not be made part of halt group %d.", | |||
| info->index, target->smp); | |||
| if (r->external_trigger) | |||
There was a problem hiding this comment.
- it's not quite clear why this code is under
target->smpcondition. We can control an external triggger without creating an SMP group. - @en-sc I remember you wanted to implement an interface to debug module. Could you please assess if we can implement the functionality in question without this interface?
if (r->external_trigger)condition is dubious, since even the debug spec implies that external trigger
have numbers starting from "0":
By convention external triggers 0-7 are bidirectional,
triggers 8-11 are input-only, and triggers 12-15 are output-only but this is not required.
- what about trigger directions?
- do you have plans to add tests targeting this functionality?
There was a problem hiding this comment.
@aap-sc Thanks for taking the time to reply - some specific answers to your points
- I was using
target->smpas that was a condition to have access to a halt group - Well spotted - indeed trigger 0 is valid - this is wrong (I used external trigger 1 in my Spike test version for this change which is why I didn't spot this.) I will address this
- I'm not sure trigger direction is something OpenOCD can handle since it's just a convention in the numbering
- I hacked Spike to group some harts together separately and then "joined" them with an external trigger.
| @@ -11476,6 +11476,11 @@ The second argument configures how OpenOCD should use the selected trigger featu | |||
| With no parameters, prints current trigger features configuration. | |||
| @end deffn | |||
|
|
|||
| @deffn {Command} {riscv set_external_trigger} value | |||
There was a problem hiding this comment.
RISC-V debug spec allows external triggers to be associated with halt and resume groups. I assume you are trying to add support for halt groups only. Looks like this command needs to be renamed, since resume groups are out of scope, or you should extend the interface to support resume groups too.
There was a problem hiding this comment.
Indeed - my initial focus was only halt groups - this should be renamed if we go ahead with this very limited solution.
|
I will be able to take a closer look at the end of this week. |
| @@ -11476,6 +11476,11 @@ The second argument configures how OpenOCD should use the selected trigger featu | |||
| With no parameters, prints current trigger features configuration. | |||
| @end deffn | |||
|
|
|||
| @deffn {Command} {riscv set_external_trigger} value | |||
There was a problem hiding this comment.
My initial idea was to create external triggers separately in the cfg file and list them in the target smp command, something like this:
# when no dmexttrigger
target create hart0 riscv -dbgbase 0
target create hart1 riscv -dbgbase 0x400
target smp hart0 hart1
# when support dmexttrigger, type0 halt, type1 resume
target create dm0extrigger0 riscv-dmexttrigger -index 0 -type 0 -dbgbase 0
target create dm0extrigger1 riscv-dmexttrigger -index 1 -type 0 -dbgbase 0
target create hart0 riscv -dbgbase 0
target create dm1extrigger0 riscv-dmexttrigger -index 0 -type 0 -dbgbase 0x400
target create dm1extrigger1 riscv-dmexttrigger -index 1 -type 0 -dbgbase 0x400
target create hart1 riscv -dbgbase 0x400
target smp hart0 hart1 dm0extrigger0 dm0extrigger1 dm1extrigger0 dm1extrigger1
But there are a lot of changes required, so currently associating external trigger with a target seems like an easy enough way to start supporting it.
Suppose we support it this way, then my suggestion is let this cmd support multiple external triggers instead of just one.
For example, if the external trigger is not bidirectional, we need at least two triggers, one input-only and one output-only, to support smp with cores on other DM.
There are more complex situations that may require more triggers.
There was a problem hiding this comment.
The design of this syntax looks really nice - i'm very happy to wait on this or if you prefer take a go at implementing that style myself?
There was a problem hiding this comment.
I haven't started looking at the code yet, just a basic idea that needs further thought.
I think we can move forward with your current implementation and make the external trigger available to those who need it first.
There was a problem hiding this comment.
@zqb-all Thank you for your idea above.
Still, I'd recommend to not abuse target create in order to assign external triggers to SMP groups. "Target" has a well-defined meaning in OpenOCD: it is a debuggable entity with its state, registers, target API operations (target_type.h), ... External trigger is not a target. So I'd prefer to find a different syntax to assign external triggers to halt goups.
There was a problem hiding this comment.
Still, I'd recommend to not abuse
target createin order to assign external triggers to SMP groups.
Agreed. Some concepts in debug spec are not explicitly represented in OpenOCD cfg file now. For example, DM is only specified with dbgbase when target create, and halt group is only implicitly used for target smp, so it is not easy to create a suitable syntax to configure a dmextrigger to a specific group now.
When I have a more mature idea, I will create another PR to discuss it.
Thank you for your feedback on this idea.
| return ERROR_FAIL; | ||
| } | ||
|
|
||
| r->external_trigger = value; |
There was a problem hiding this comment.
It would be safer to add some checks to prevent user from associating one trigger to multiple harts on same DM.
There was a problem hiding this comment.
Indeed this is a good point - the external trigger is logically associated with a halt group which will span multiple targets (assuming the one target per hart configuration method.)
I will take a close look at the SMP / halt-resume groups implementation to form an opinion on this matter (which I wanted to do anyway). It just may take me a day or two to get to this. |
|
Hello all, I have checked the overall state of SMP and halt/resume groups in riscv-openocd. To summarize the current state:
Comments, questions:
|
| /* Halt group may be associated with an external trigger */ | ||
| unsigned int external_trigger; |
There was a problem hiding this comment.
As pointed out in another comment, there can be multiple external triggers in a halt (or resume) group.
The code, on the other hand, seems to force 1:1 relationship.
Add support for associating a halt group with an external trigger via a newly exposed configuration option "riscv set_external_trigger".
Change-Id: If10c67d2e14d8bc7cd6d59011b3215fda4ff4b02