target/riscv: Add support for external triggers #1243

lz-bro · 2025-03-31T07:45:49Z

Add support for associating a halt/resume group with an external trigger via a newly exposed configuration option "riscv smp_add_ext_triggers".

Original merge request:
#1179

Original author: Rob Bradford
[email protected]
https://github.com/rbradford

lz-bro · 2025-03-31T08:12:02Z

I want to continue the implementation of the external triggers for halt/resume groups base on @rbradford‘s work，But current implementation has not yet completed the resume group for external triggers。If I want to implement it, what do I need to pay attention to? What I can confirm now is that a diable haltgroup is required before the step, and the haltgroup will be restored after the step. @JanMatCodasip, @aap-sc could you please take a look and give some suggestions?

rbradford · 2025-03-31T17:05:56Z

src/target/riscv/riscv.c

+		}
+		int value = atoi(CMD_ARGV[i + 1]);
+		if (value <= 0 || value > 16) {
+			LOG_ERROR("%s is not a valid integer argument for command.", CMD_ARGV[i + 1]);


This came up in the previous review - 0 is a valid external trigger. Also 16 is not a valid - so should this be (value < 0 || value >= 16)

It would be better to say:

LOG_ERROR("%s is not a valid external trigger number - expecting a number in range 0..15.", CMD_ARGV[i + 1]);

Please, if this error message could be improved.

rbradford · 2025-03-31T17:06:32Z

Thank you for restarting this work.

src/target/riscv/riscv.c

src/target/riscv/riscv-013.c

JanMatCodasip

I have managed a partial review.

src/target/riscv/riscv-013.c

JanMatCodasip · 2025-05-27T16:00:32Z

src/target/riscv/riscv.c

+		return ERROR_COMMAND_SYNTAX_ERROR;
+	}
+
+	unsigned int index = 0;


The command is called "add_ext_triggers" so the user may think that it can be called multiple times and the parameters would merge.

This is not the case - the function overwrites the values of the previous call.

Please pick one of these two options:

Rename the function to "set"

Or make sure that the previously set triggers are not overwritten. In this case the function can be simplified to take just two arguments, and the user can call it as many times as they need.

Rename the function to "set", addressed.

I am afraid this is not fully addressed yet. If the user calls these two commands:

riscv smp_set_ext_triggers halt_group 2 halt_group 3 halt_group 4 riscv smp_set_ext_triggers halt_group 5

... the current result would be that only triggers 5, 3 and 4, would be assigned. It would not be very intuitive.

I would prefer if all the triggers (2, 3, 4 and 5) were assigned.

When the trigger structure (struct riscv_ext_trigger) is re-arranged as suggested in another comment, this should not be a problem anymore.

JanMatCodasip · 2025-05-27T16:05:42Z

src/target/riscv/riscv.h

@@ -365,6 +377,8 @@ struct riscv_info {
 	bool wp_allow_ge_lt_trigger;

 	bool autofence;
+
+	riscv_ext_trigger_t external_triggers[RISCV_MAX_EXTTRIGGERS];


External triggers are only part of 0.13/1.0 version of the spec. Furthermore, external triggers are a property of the debug module. For that reason, the information about the external triggers should be moved to dm013_info_t.

riscv_command_handlers is part of riscv.c. If the information about the external triggers is moved to dm013_info_t, it seems a bit weird if using dm013_info_t requires including riscv-013.c in riscv.c? Besides, put the dm013_info_t definition into riscv-013.h and include it, or create a new riscv013_command_handlers to include this smp_set_ext_triggers command? Do you have any suggestions?

You can do it this way:

Implement a new function in riscv013.c into struct riscv_info, called e.g. riscv013_set_external_trigger.

Add it as a new callback into riscv_info, called e.g. set_external_trigger.

Keep the command handler in riscv.c but check whether r->set_external_trigger is implemented.

As an example, you can take a look at COMMAND_HANDLER(riscv_authdata_write) which follows this approach.

Thank you for your answer, but I encountered a new problem. The mode of this command is COMMAND_CONFIG, riscv smp_set_ext_triggers command used before 'init' and r->set_external_trigger is not implemented before init target.

@JanMatCodasip I want to introduce the set_group command instead of smp_set_ext_triggers, Create a set_group command handler in riscv.c but check whether r->set_group is implemented, then called set_group in riscv013.c

{ .name = "set_group", .handler = riscv_set_group, .mode = COMMAND_ANY, .usage = "grouptype group ['triger' triggernum]", .help = "Set a hart or a given external trigger to the halt group." },

{ .name = "smp_set_ext_triggers", .handler = riscv_smp_set_ext_triggers, .mode = COMMAND_CONFIG, .usage = "num1 grouptype1 [num2 grouptype2]......", .help = "Set the given external triggers to the halt group." },

This way, users can also set hat group for harts and triggers.

src/target/riscv/riscv.c

src/target/riscv/riscv-013.c

src/target/riscv/riscv.c

src/target/riscv/riscv.h

JanMatCodasip · 2025-05-27T16:26:43Z

src/target/riscv/riscv.c

+		}
+		int value = atoi(CMD_ARGV[i + 1]);
+		if (value <= 0 || value > 16) {
+			LOG_ERROR("%s is not a valid integer argument for command.", CMD_ARGV[i + 1]);


It would be better to say:

LOG_ERROR("%s is not a valid external trigger number - expecting a number in range 0..15.", CMD_ARGV[i + 1]);

JanMatCodasip · 2025-06-13T06:28:03Z

doc/openocd.texi

+@deffn {Command} {riscv smp_add_ext_triggers} grouptype1 num1 [grouptype2 num2] ...
+Associate the supplied external trigger with the halt group for the harts. When
+the external trigger fires the harts in the halt group will be halted.
+@end deffn


The command was renamed to riscv smp_set_ext_triggers, so the documentation should be updated, too.

It would be good to stress that the only supported value for grouptype is halt_group.

It would be beneficial to explain why only halt groups are supported.

The command can associate multiple triggers with the halt group, so plural should be used (trigger -> triggers).

The text spoke about trigger firing -> harts halting. The other direction should be mentioned, too: hart halted -> trigger notified.

For example:

Suggested change

@deffn {Command} {riscv smp_add_ext_triggers} grouptype1 num1 [grouptype2 num2] ...

Associate the supplied external trigger with the halt group for the harts. When

the external trigger fires the harts in the halt group will be halted.

@end deffn

@deffn {Command} {riscv smp_add_ext_triggers} grouptype1 num1 [grouptype2 num2] ...

OpenOCD assigns targets of a SMP group into a RISC-V halt group. This command associates the supplied external trigger(s) with this halt group.

When an external input trigger fires, the harts in the halt group will get halted. Similarly, all external output triggers will be notified when a hart in the halt group halts.

The only supported value of @var{grouptype} is @option{halt_group}. RISC-V resume groups

are not allowed because the concept of a target resuming due to a different reason than user's resume

request is not supported by OpenOCD and GDB.

@end deffn

To others: Please feel free to suggest even different wording, if you like.

Nice description, thanks.

JanMatCodasip · 2025-06-13T06:38:47Z

src/target/riscv/riscv-013.c

+		get_field(read_val, DM_DMCS2_DMEXTTRIGGER) == external_trigger.dmexttrigger &&
+		get_field(read_val, DM_DMCS2_HGSELECT) == 1) {
+		LOG_TARGET_INFO(target, "External trigger %d added to %s group %d", external_trigger.dmexttrigger,
+				external_trigger.grouptype ? "resume" : "halt", group);


Please, don't rely on the fact that 1 stands for resume group and 0 for halt group. Instead, use the named enum items.

Suggested change

external_trigger.grouptype ? "resume" : "halt", group);

external_trigger.grouptype == RESUME_GROUP ? "resume" : "halt", group);

JanMatCodasip · 2025-06-13T06:39:07Z

src/target/riscv/riscv-013.c

+	assert(group <= 31);
+	assert(external_trigger.dmexttrigger < 16);
+	write_val = set_field(write_val, DM_DMCS2_GROUP, group);
+	write_val = set_field(write_val, DM_DMCS2_GROUPTYPE, external_trigger.grouptype);


(same)

Suggested change

write_val = set_field(write_val, DM_DMCS2_GROUPTYPE, external_trigger.grouptype);

write_val = set_field(write_val, DM_DMCS2_GROUPTYPE, external_trigger.grouptype == RESUME_GROUP ? 1 : 0);

JanMatCodasip · 2025-06-13T06:40:04Z

src/target/riscv/riscv-013.c

+{
+	uint32_t write_val = DM_DMCS2_HGSELECT;
+	assert(group <= 31);
+	assert(external_trigger.dmexttrigger < 16);


(Optionally)

Detail for additional safety:

Suggested change

assert(external_trigger.dmexttrigger < 16);

assert(external_trigger.dmexttrigger < 16);

assert(external_trigger.dmexttrigger.grouptype == HALT_GROUP || external_trigger.dmexttrigger.grouptype == RESUME_GROUP);

I think the following code is safe enough.
write_val = set_field(write_val, DM_DMCS2_GROUPTYPE, (grouptype == HALT_GROUP) ? 0 : 1);

JanMatCodasip · 2025-06-13T06:40:25Z

src/target/riscv/riscv-013.c

+				external_trigger.grouptype ? "resume" : "halt", group);
+	} else {
+		LOG_TARGET_ERROR(target, "Could not add external trigger %d to %s group %d", external_trigger.dmexttrigger,
+				external_trigger.grouptype ? "resume" : "halt", group);


(same)

Suggested change

external_trigger.grouptype ? "resume" : "halt", group);

external_trigger.grouptype == RESUME_GROUP ? "resume" : "halt", group);

JanMatCodasip · 2025-06-13T07:12:24Z

src/target/riscv/riscv.c

+		int value = atoi(CMD_ARGV[i + 1]);
+		if (value < 0 || value > 15) {
+			LOG_ERROR("%s is not a valid integer argument for command.", CMD_ARGV[i + 1]);
+			return ERROR_FAIL;


(same as above)

Suggested change

return ERROR_FAIL;

return ERROR_COMMAND_ARGUMENT_INVALID;

JanMatCodasip · 2025-06-13T07:21:05Z

src/target/riscv/riscv.h

+typedef struct {
+	grouptype_t grouptype;
+	uint32_t dmexttrigger;
+	bool is_set;
+} riscv_ext_trigger_t;


Please, don't introduce new typedefs for structures. The upstream code style (at openocd.org) no longer allows them. Use just struct riscv_ext_trigger.

The RISC-V debug spec says: "At any given time, each hart and each trigger is member of exactly one halt group and one resume group." Please consider re-arranging the structure to reflect it:

The array index (0..15) would mean the trigger number

Each array item (struct riscv_ext_trigger) should say if halt group should be set, and what the halt group number is

struct riscv_ext_trigger { bool is_haltgroup_set; unsigned int haltgroup_num; /* In future, this can be added: bool is_resumegroup_set; unsigned int resumegroup_num; */ };

Thank you for your suggestion, it is very helpful, and is is_haltgroup_set seems unnecessary?

JanMatCodasip · 2025-06-13T07:27:59Z

src/target/riscv/riscv.c

+		return ERROR_COMMAND_SYNTAX_ERROR;
+	}
+
+	unsigned int index = 0;


I am afraid this is not fully addressed yet. If the user calls these two commands:

riscv smp_set_ext_triggers halt_group 2 halt_group 3 halt_group 4 riscv smp_set_ext_triggers halt_group 5

... the current result would be that only triggers 5, 3 and 4, would be assigned. It would not be very intuitive.

I would prefer if all the triggers (2, 3, 4 and 5) were assigned.

When the trigger structure (struct riscv_ext_trigger) is re-arranged as suggested in another comment, this should not be a problem anymore.

JanMatCodasip · 2025-06-13T07:44:49Z

src/target/riscv/riscv-013.c

+			if (r->external_triggers[i].is_set) {
+				if (add_ext_trigger_to_group(target, target->smp, r->external_triggers[i]) != ERROR_OK)
+					return ERROR_FAIL;


It appears that haltgroup_supported from riscv013_info_t should be checked here. If it is false, then a warning should be displayed, and add_ext_trigger_to_group() not called.

Thanks, addressed.

JanMatCodasip · 2025-06-13T07:52:25Z

src/target/riscv/riscv.h

@@ -365,6 +377,8 @@ struct riscv_info {
 	bool wp_allow_ge_lt_trigger;

 	bool autofence;
+
+	riscv_ext_trigger_t external_triggers[RISCV_MAX_EXTTRIGGERS];


You can do it this way:

Implement a new function in riscv013.c into struct riscv_info, called e.g. riscv013_set_external_trigger.

Add it as a new callback into riscv_info, called e.g. set_external_trigger.

Keep the command handler in riscv.c but check whether r->set_external_trigger is implemented.

As an example, you can take a look at COMMAND_HANDLER(riscv_authdata_write) which follows this approach.

JanMatCodasip · 2025-06-13T08:10:36Z

src/target/riscv/riscv.c

+				"The only supported group type is 'halt_group'.", CMD_ARGV[0]);
+			return ERROR_COMMAND_SYNTAX_ERROR;
+		}
+		int value = atoi(CMD_ARGV[i + 1]);


It looks like you can use COMMAND_PARSE_NUMBER(uint, CMD_ARGV[i + 1], trigger_num); instead of atoi().

Add support for associating a halt group with current target or an external trigger via a newly exposed configuration option "riscv set_group". Original merge request: riscv-collab#1179 Original author: Rob Bradford [email protected] https://github.com/rbradford

lz-bro force-pushed the add-external-triggers branch 2 times, most recently from a066280 to 3f63412 Compare March 31, 2025 08:05

rbradford reviewed Mar 31, 2025

View reviewed changes

lz-bro force-pushed the add-external-triggers branch 2 times, most recently from bf7c3ac to 8216d33 Compare April 3, 2025 03:43

JanMatCodasip reviewed May 23, 2025

View reviewed changes

src/target/riscv/riscv.c Outdated Show resolved Hide resolved

src/target/riscv/riscv-013.c Outdated Show resolved Hide resolved

lz-bro force-pushed the add-external-triggers branch 2 times, most recently from 3475a75 to b17e870 Compare May 26, 2025 07:49

JanMatCodasip requested changes May 27, 2025

View reviewed changes

lz-bro force-pushed the add-external-triggers branch from b17e870 to fa7fe0f Compare May 28, 2025 03:26

JanMatCodasip requested changes Jun 13, 2025

View reviewed changes

JanMatCodasip reviewed Jun 13, 2025

View reviewed changes

lz-bro force-pushed the add-external-triggers branch 4 times, most recently from 881db17 to 1a982f0 Compare June 23, 2025 02:42

lz-bro force-pushed the add-external-triggers branch from 1a982f0 to 63f9a1f Compare June 23, 2025 02:46

	external_trigger.grouptype ? "resume" : "halt", group);
	external_trigger.grouptype == RESUME_GROUP ? "resume" : "halt", group);

	write_val = set_field(write_val, DM_DMCS2_GROUPTYPE, external_trigger.grouptype);
	write_val = set_field(write_val, DM_DMCS2_GROUPTYPE, external_trigger.grouptype == RESUME_GROUP ? 1 : 0);

	assert(external_trigger.dmexttrigger < 16);
	assert(external_trigger.dmexttrigger < 16);
	assert(external_trigger.dmexttrigger.grouptype == HALT_GROUP \|\| external_trigger.dmexttrigger.grouptype == RESUME_GROUP);

target/riscv: Add support for external triggers #1243

Are you sure you want to change the base?

target/riscv: Add support for external triggers #1243

Uh oh!

Conversation

lz-bro commented Mar 31, 2025

Uh oh!

lz-bro commented Mar 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rbradford commented Mar 31, 2025

Uh oh!

Uh oh!

Uh oh!

JanMatCodasip left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lz-bro May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JanMatCodasip Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lz-bro Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lz-bro Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lz-bro May 28, 2025 •

edited

Loading

JanMatCodasip Jun 13, 2025 •

edited

Loading

lz-bro Jun 19, 2025 •

edited

Loading

lz-bro Jun 17, 2025 •

edited

Loading

JanMatCodasip Jun 13, 2025 •

edited

Loading

JanMatCodasip Jun 13, 2025 •

edited

Loading