Update riscv/debug_defines (to sync with riscv-debug-spec commit 40b9a05) #973

kr-sc · 2023-12-04T09:06:10Z

Change-Id: Ie969866d1de83360a5f45e96e22108b58b8aa02f

timsifive

Looks OK. Just to check, can you show some debug output that uses the new code?

en-sc · 2023-12-06T09:04:29Z

I would suggest updating to a more recent version to include these changes: riscv/riscv-debug-spec#908, riscv/riscv-debug-spec#909. Please, note that debug_reg_printer.[ch] should also be updated if the changes are included.

kr-sc · 2023-12-06T14:10:31Z

Looks OK. Just to check, can you show some debug output that uses the new code?

Before:

log_debug_reg(): [riscv.cpu] dtmcs=0x61 { version=1.0, dmistat=0, idle=0, dmireset=0, dtmhardreset=0, errinfo=not implemented, abits=6, }

After:

log_debug_reg(): [riscv.cpu] dtmcs=0x61 {version=1_0 errinfo=not_implemented abits=6}

timsifive · 2023-12-07T00:13:18Z

I made riscv/riscv-debug-spec#922 to address the checkpatch issue. When that merges, please update this PR.

Change-Id: Ie969866d1de83360a5f45e96e22108b58b8aa02f Signed-off-by: Kirill Radkin <[email protected]>

JanMatCodasip

I have few suggestions related to code structure or naming. Otherwise this looks good.

JanMatCodasip · 2023-12-11T06:58:19Z

src/target/riscv/debug_reg_printer.h

@@ -2,6 +2,12 @@

 #include "debug_defines.h"

+enum riscv_debug_reg_show {


Please, could you give a descriptive name to this enum, e.g. display_mode (or similar), rather than show.

Suggested change

enum riscv_debug_reg_show {

enum riscv_debug_reg_display_mode {

Correction per kr-sc's comment: This change should go to the debug spec repository instead (riscv-debug-spec).

JanMatCodasip · 2023-12-11T06:58:40Z

src/target/riscv/debug_reg_printer.c

@@ -62,22 +62,35 @@ static uint64_t riscv_debug_reg_field_value(riscv_debug_reg_field_info_t field,

 static unsigned int riscv_debug_reg_fields_to_s(char *buf, unsigned int offset,
 	struct riscv_debug_reg_field_list_t (*get_next)(riscv_debug_reg_ctx_t contex),
-	riscv_debug_reg_ctx_t context, uint64_t value)
+	riscv_debug_reg_ctx_t context, uint64_t value,
+	enum riscv_debug_reg_show show)


Suggested change

enum riscv_debug_reg_show show)

enum riscv_debug_reg_display_mode display_mode)

Same as above: This change should go to the debug spec repository instead (riscv-debug-spec).

JanMatCodasip · 2023-12-11T07:08:50Z

src/target/riscv/debug_reg_printer.c

+
+		uint64_t field_value = riscv_debug_reg_field_value(list.field, value);
+
+		if (show == RISCV_DEBUG_REG_SHOW_ALL ||
+				(show == RISCV_DEBUG_REG_HIDE_UNNAMED_0 &&
+					(field_value != 0 ||
+						(list.field.values && list.field.values[0]))) ||
+				(show == RISCV_DEBUG_REG_HIDE_ALL_0 && field_value != 0)) {
+			curr += get_len_or_sprintf(buf, curr, separator);
+			curr += riscv_debug_reg_field_to_s(buf, curr, list.field, context,
+							field_value);
+			separator = " ";
+		}


The condition of the "if" is complex, which makes it hard to understand and modify. Please, could you simplify this whole part of the code - for example as shown below.

Suggested change

uint64_t field_value = riscv_debug_reg_field_value(list.field, value);

if (show == RISCV_DEBUG_REG_SHOW_ALL ||

(show == RISCV_DEBUG_REG_HIDE_UNNAMED_0 &&

(field_value != 0 ||

(list.field.values && list.field.values[0]))) ||

(show == RISCV_DEBUG_REG_HIDE_ALL_0 && field_value != 0)) {

curr += get_len_or_sprintf(buf, curr, separator);

curr += riscv_debug_reg_field_to_s(buf, curr, list.field, context,

field_value);

separator = " ";

}

uint64_t field_value = riscv_debug_reg_field_value(list.field, value);

const bool isZero = (field_value == 0);

const bool isUnnamed = (!list.field.values) || (!list.field.values[0]);

if (display_mode == RISCV_DEBUG_REG_HIDE_UNNAMED_0 && isUnnamed && isZero)

// Don't display this field

continue;

if (display_mode == RISCV_DEBUG_REG_HIDE_ALL_0 && isZero)

// Don't display this field

continue;

// Show this field

curr += get_len_or_sprintf(buf, curr, separator);

curr += riscv_debug_reg_field_to_s(buf, curr, list.field, context,

field_value);

separator = " ";

Same as above: This change should go to the debug spec repository instead (riscv-debug-spec).

kr-sc · 2023-12-11T10:54:48Z

@JanMatCodasip I think that we need patch to debus spec with your suggestions. What if we merge this patch and make a subsequent update with your suggestions to debug spec and openocd later?

JanMatCodasip · 2023-12-11T13:13:49Z

kr-sc: I think that we need patch to debug spec with your suggestions.

Ah, when looking at the changes, I missed the fact that the files debug_reg_printer.* are auto-generated from the debug spec repository. Thank you for the reminder.

kr-sc: What if we merge this patch and make a subsequent update with your suggestions to debug spec and openocd later?

Sure, I agree, let's merge this now as is.

If you are happy with the suggested changes, I believe it would be good to apply them to the riscv-debug-spec repo.

timsifive previously approved these changes Dec 5, 2023

View reviewed changes

en-sc mentioned this pull request Dec 6, 2023

target/riscv: report helpfull location during register decode #972

Merged

kr-sc dismissed timsifive’s stale review via 16f9630 December 6, 2023 12:12

kr-sc force-pushed the kr-sc/update-debug-defines-os branch 2 times, most recently from 16f9630 to 02f7348 Compare December 6, 2023 12:22

kr-sc changed the title ~~Update riscv/debug_defines (to sync with https://github.com/riscv/riscv-debug-spec/pull/902)~~ Update riscv/debug_defines (to sync with riscv-debug-spec commit a992f2ea8) Dec 6, 2023

kr-sc force-pushed the kr-sc/update-debug-defines-os branch 2 times, most recently from f33f004 to 30a5323 Compare December 6, 2023 14:03

kr-sc force-pushed the kr-sc/update-debug-defines-os branch from 30a5323 to 420e5d2 Compare December 7, 2023 17:53

kr-sc changed the title ~~Update riscv/debug_defines (to sync with riscv-debug-spec commit a992f2ea8)~~ Update riscv/debug_defines (to sync with riscv-debug-spec commit 40b9a05) Dec 7, 2023

Update riscv/debug_defines (to sync with riscv-debug-spec:40b9a05)

84e6a4e

Change-Id: Ie969866d1de83360a5f45e96e22108b58b8aa02f Signed-off-by: Kirill Radkin <[email protected]>

kr-sc force-pushed the kr-sc/update-debug-defines-os branch from 420e5d2 to 84e6a4e Compare December 7, 2023 17:59

timsifive approved these changes Dec 8, 2023

View reviewed changes

JanMatCodasip requested changes Dec 11, 2023

View reviewed changes

JanMatCodasip approved these changes Dec 11, 2023

View reviewed changes

timsifive merged commit b5e0ec3 into riscv-collab:riscv Dec 11, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update riscv/debug_defines (to sync with riscv-debug-spec commit 40b9a05) #973

Update riscv/debug_defines (to sync with riscv-debug-spec commit 40b9a05) #973

kr-sc commented Dec 4, 2023

timsifive left a comment

en-sc commented Dec 6, 2023

kr-sc commented Dec 6, 2023

timsifive commented Dec 7, 2023

JanMatCodasip left a comment

JanMatCodasip Dec 11, 2023

JanMatCodasip Dec 11, 2023

JanMatCodasip Dec 11, 2023

JanMatCodasip Dec 11, 2023

JanMatCodasip Dec 11, 2023

JanMatCodasip Dec 11, 2023

kr-sc commented Dec 11, 2023

JanMatCodasip commented Dec 11, 2023 •

edited

Loading

		@@ -2,6 +2,12 @@

		#include "debug_defines.h"

		enum riscv_debug_reg_show {

	enum riscv_debug_reg_show {
	enum riscv_debug_reg_display_mode {

	enum riscv_debug_reg_show show)
	enum riscv_debug_reg_display_mode display_mode)

Update riscv/debug_defines (to sync with riscv-debug-spec commit 40b9a05) #973

Update riscv/debug_defines (to sync with riscv-debug-spec commit 40b9a05) #973

Conversation

kr-sc commented Dec 4, 2023

timsifive left a comment

Choose a reason for hiding this comment

en-sc commented Dec 6, 2023

kr-sc commented Dec 6, 2023

timsifive commented Dec 7, 2023

JanMatCodasip left a comment

Choose a reason for hiding this comment

JanMatCodasip Dec 11, 2023

Choose a reason for hiding this comment

JanMatCodasip Dec 11, 2023

Choose a reason for hiding this comment

JanMatCodasip Dec 11, 2023

Choose a reason for hiding this comment

JanMatCodasip Dec 11, 2023

Choose a reason for hiding this comment

JanMatCodasip Dec 11, 2023

Choose a reason for hiding this comment

JanMatCodasip Dec 11, 2023

Choose a reason for hiding this comment

kr-sc commented Dec 11, 2023

JanMatCodasip commented Dec 11, 2023 • edited Loading

JanMatCodasip commented Dec 11, 2023 •

edited

Loading