openocd does not allow to query status of dcsr.ebreak{u,s,m} #918

kr-sc · 2023-09-14T15:21:34Z

Extend riscv set_ebreak* commands.
Now it can be called without args to print current value.

riscv_ebreak* flags are moved to riscv_info struct.

src/target/riscv/riscv.c

JanMatCodasip

(1) As for the riscv_ebreak_* OpenOCD's internal configuration flags:

I agree that there should be a way how the user can view the current settings (a "getter"). This applies to not only to commands riscv set_ebreak*, but in general to all configuration commands.

I am not in favor of putting it into riscv info. The output of riscv info would become a mixture of the detected hardware capabilities and current OpenOCD's settings, which are two different sets of information.

The getters had already been discussed a little here:

@timsifive Since this topic has re-occurred, I recommend to discuss and agree on how the config getters should look like. I don't have strong preferences, but would like it to be:

consistent (at least in the RISC-V target)
intuitive and easy for the user, if possible
done in such a way that it passes upstream review

(2) As for adding `dcsr.ebreak*` values to `riscv info`:

I recommend to not add any information that can be read equally easy by the existing commands (reg, get_reg) and possibly some bit-masking.

Furthermore, items of riscv info would ideally be documented so that the user knows what they mean. That's another reason why to not add more items (unless the benefits of having them there are large enough).

kr-sc · 2023-09-19T13:15:30Z

Remove flags values from riscv info output
Change fucntionality of riscv set_ebreak* commands: now it prints actual riscv_ebreak* flags value when it is called without args
Move flags values to riscv_info struct

JanMatCodasip

This change looks all right to me overall - thanks. I have only few smaller suggestions.

Note that I am out of office until Oct 9 so I won't be able make another review of this change. But since it is almost complete, that should not be an issue. I'll leave the last quick check + merge to @timsifive.

JanMatCodasip · 2023-09-21T05:42:43Z

src/target/riscv/riscv.c

+	RISCV_INFO(r);
+
+	if (CMD_ARGC == 0) {
+		command_print(CMD, "riscv_ebreakm enabled = %d", r->riscv_ebreakm);


Optionally - if you like:

Suggested change

command_print(CMD, "riscv_ebreakm enabled = %d", r->riscv_ebreakm);

command_print(CMD, "riscv_ebreakm enabled = %s", r->riscv_ebreakm ? "yes" : "no");

I think, that now it much easier to parse output of this command than with your suggestion, so I would like to leave it as it is.

Let's follow existing OpenOCD commands. E.g. handle_poll_command() prints out a boolean value, and it uses:
command_print(CMD, "background polling: %s", jtag_poll_get_enabled() ? "on" : "off");

JanMatCodasip · 2023-09-21T05:43:53Z

src/target/riscv/riscv.c

@@ -4196,6 +4214,7 @@ COMMAND_HANDLER(handle_info)
 		riscv_enumerate_triggers(target) == ERROR_OK;
 	riscv_print_info_line_if_available(CMD, "hart", "trigger_count",
 				r->trigger_count, trigger_count_available);
+


This whitespace change (blank line) can now be reverted.

Removed. Addressed.

JanMatCodasip · 2023-09-21T05:49:26Z

src/target/riscv/riscv.c

+	if (CMD_ARGC == 0) {
+		command_print(CMD, "riscv_ebreakm enabled = %d", r->riscv_ebreakm);
+		return ERROR_OK;
+	} else if (CMD_ARGC != 1) {
+		LOG_ERROR("Command takes 0 or 1 parameters");
 		return ERROR_COMMAND_SYNTAX_ERROR;
 	}
-	COMMAND_PARSE_ON_OFF(CMD_ARGV[0], riscv_ebreakm);
+
+	COMMAND_PARSE_ON_OFF(CMD_ARGV[0], r->riscv_ebreakm);
 	return ERROR_OK;


For readability, I recommend this structure. Applies to all the three commands.

if (CMD_ARGC == 0) { command_print(CMD, "riscv_ebreakm enabled = %d", r->riscv_ebreakm); return ERROR_OK; } else if (CMD_ARGC == 1) { COMMAND_PARSE_ON_OFF(CMD_ARGV[0], r->riscv_ebreakm); return ERROR_OK; } else { LOG_ERROR("Command takes 0 or 1 parameters"); return ERROR_COMMAND_SYNTAX_ERROR; }

src/target/riscv/riscv.c

Findings addressed.

timsifive · 2023-09-25T19:43:06Z

src/target/riscv/riscv.c

+	RISCV_INFO(r);
+
+	if (CMD_ARGC == 0) {
+		command_print(CMD, "riscv_ebreakm enabled = %d", r->riscv_ebreakm);


Let's follow existing OpenOCD commands. E.g. handle_poll_command() prints out a boolean value, and it uses:
command_print(CMD, "background polling: %s", jtag_poll_get_enabled() ? "on" : "off");

src/target/riscv/riscv.h

Extend riscv set_ebreak* commands. Now it can be called without args to print current value. riscv_ebreak* flags are moved to riscv_info struct. Change-Id: Ib46e6b6dfc0117599c7f6715c7aaf113e63bd7dc Signed-off-by: Kirill Radkin <[email protected]>

timsifive

Looks good. Thanks for this improvement.

JanMatCodasip reviewed Sep 15, 2023

View reviewed changes

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

JanMatCodasip previously requested changes Sep 19, 2023

View reviewed changes

kr-sc force-pushed the kr-sc/allow-to-query-status-dcsr-ebreak branch 2 times, most recently from 244d45e to caa9250 Compare September 19, 2023 12:50

JanMatCodasip reviewed Sep 21, 2023

View reviewed changes

JanMatCodasip mentioned this pull request Sep 21, 2023

provide riscv-specific controls to disable triggers from being used for watchpoints #917

Merged

kr-sc force-pushed the kr-sc/allow-to-query-status-dcsr-ebreak branch from caa9250 to 7c60e0a Compare September 25, 2023 10:42

timsifive requested changes Sep 25, 2023

View reviewed changes

kr-sc force-pushed the kr-sc/allow-to-query-status-dcsr-ebreak branch from 7c60e0a to ee2bc80 Compare September 26, 2023 08:55

timsifive approved these changes Sep 27, 2023

View reviewed changes

timsifive merged commit 75b5de6 into riscv-collab:riscv Sep 29, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

openocd does not allow to query status of dcsr.ebreak{u,s,m} #918

openocd does not allow to query status of dcsr.ebreak{u,s,m} #918

kr-sc commented Sep 14, 2023 •

edited

Loading

JanMatCodasip left a comment •

edited

Loading

kr-sc commented Sep 19, 2023

JanMatCodasip left a comment •

edited

Loading

JanMatCodasip Sep 21, 2023

kr-sc Sep 25, 2023

timsifive Sep 25, 2023

kr-sc Sep 26, 2023

JanMatCodasip Sep 21, 2023

kr-sc Sep 25, 2023

JanMatCodasip Sep 21, 2023

kr-sc Sep 25, 2023

timsifive Sep 25, 2023

timsifive left a comment

	command_print(CMD, "riscv_ebreakm enabled = %d", r->riscv_ebreakm);
	command_print(CMD, "riscv_ebreakm enabled = %s", r->riscv_ebreakm ? "yes" : "no");

openocd does not allow to query status of dcsr.ebreak{u,s,m} #918

openocd does not allow to query status of dcsr.ebreak{u,s,m} #918

Conversation

kr-sc commented Sep 14, 2023 • edited Loading

JanMatCodasip left a comment • edited Loading

Choose a reason for hiding this comment

(1) As for the riscv_ebreak_* OpenOCD's internal configuration flags:

(2) As for adding dcsr.ebreak* values to riscv info:

kr-sc commented Sep 19, 2023

JanMatCodasip left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsifive left a comment

Choose a reason for hiding this comment

kr-sc commented Sep 14, 2023 •

edited

Loading

JanMatCodasip left a comment •

edited

Loading

(2) As for adding `dcsr.ebreak*` values to `riscv info`:

JanMatCodasip left a comment •

edited

Loading