target/riscv: make sure target is halted when reset_halt is set #1214

jhqian · 2025-01-27T03:46:56Z

some MCU will need certain period of time to be halted after ndmreset is issued, so in deassert_reset, it needs to make sure MCU is halted before clearing DM_DMCONTROL_HALTREQ.

Change-Id: I6d7ef7b9b33aff65cb996968fb28cd62e3e1fb16

aap-sc · 2025-01-27T08:59:53Z

src/target/riscv/riscv-013.c

+	control = 0;
+	control = set_field(control, DM_DMCONTROL_DMACTIVE, 1);
+	control = set_dmcontrol_hartsel(control, info->index);
+	result = dm_write(target, DM_DMCONTROL, control);


Should not this write be conditional?

According to original code, HALTREQ needs to be cleared no matter reset_haltreq is set or not. According to the spec, HALTREQ set to 1 or 0 has no effect for a halted hart, so no need to be conditional.

My point is that the original code had only 1 write to DM_DMCONTROL . Now it has two. What is the point of the second write if there was no haltreq?

it seems there's an issue related to GD32V which needs HALTREQ to be cleared after reset, otherwise "reset" command does not behave as expected. So I cleared the HALTREQ after reset is acknowledged. For more details about code changes related to the issue I mentioned, please refer to 7d2ea18

t seems there's an issue related to GD32V which needs HALTREQ to be cleared after reset, otherwise "reset" command does not behave as expected.

I agree with @aap-sc:

In case target->reset_halt == false, there is no HATLREQ set, and therefore no reason to clear it. The clearing of HATLREQ (second write to dmcontrol) should, IMO, only occur when target->reset_halt == true.

@jhqian Is that correct understanding, or am I missing something?

aap-sc · 2025-01-27T09:08:29Z

src/target/riscv/riscv-013.c

@@ -2924,22 +2924,50 @@ static int deassert_reset(struct target *target)
 	riscv_scan_set_delay(&info->learned_delays, RISCV_DELAY_BASE,
 			orig_base_delay);

-	/* Ack reset and clear DM_DMCONTROL_HALTREQ if previously set */
+	/* Ack reset with DM_DMCONTROL_HALTREQ for those MCUs which need a


I believe that the original comment should stay as is, and the phrase about

period of time to be halted after reset is released

should be moved to the wait loop.

I'm okay to move "wait for a period of time" into while, but to keep "clear DM_DMCONTROL_HALTREQ" as is perhaps not aligned with current code, as that bit will be cleared later. how about change the line of 2927 as following:
/* Act reset with DM_DMCONTROL_HALTREQ based on target->reset_haltreq */

@jhqian I am sorry, the wording of the comment is a little unclear. Did you mean to say this?

Please, make sure that all comments and descriptions are very clear and understandable. This helps all code readers, incl. reviewers.

/* Acknowledge the reset by writing dmcontrol.ackhavereset <- 1, and at the same time, * keep the dmcontrol.haltreq unchanged. This is required for certain processors that * take longer time to halt after leaving the reset. */

(Anyway, the code will probably change due to other review suggestions, and this specific comment will probably become irrelevant.)

jhqian · 2025-01-27T11:28:07Z

as the file has been diverged from upstream, I made similar changes accordingly in upstream: https://review.openocd.org/c/openocd/+/8725

- some MCU will need certain period of time to be halted after ndmreset is issued, so in deassert_reset, it needs to make sure MCU is halted before clearing DM_DMCONTROL_HALTREQ. Change-Id: I6d7ef7b9b33aff65cb996968fb28cd62e3e1fb16 Signed-off-by: Ryan QIAN <[email protected]>

en-sc

@jhqian, thank you for the patch.
I have a couple of concerns:

AFAIU, this is a workaround. Please see [3.2. Reset Control] (RISC-V Debug Spec. Version 1.0.0-rc4, Revised 2024-12-05):

When a hart comes out of reset and haltreq or hasresethaltreq are set, the hart will immediately enter Debug Mode (halted state).

However, this exact change is classifyed as a bugfix in the spec (listed under [1.2.1.1. Bugfixes from 0.13 to 1.0]).

Though looking at the comments this is more of a clarification:
Clarify that harts halt out of reset if haltreq=1 riscv/riscv-debug-spec#419 (comment)
The second loop seems excessive. Please see the comment I left.
If introducing the second loop is the only option, have you considered addressing the issue using reset-deassert-post/pre event handlers?
@tom-van, I'd suggest first reviewing the code here since IMHO the reset procedure is much simpler in the fork.

en-sc · 2025-01-27T12:41:54Z

src/target/riscv/riscv-013.c

+						get_field(dmstatus, DM_DMSTATUS_ALLHALTED) ? "true" : "false");
+				return ERROR_TIMEOUT_REACHED;
+			}
+		} while (!get_field(dmstatus, DM_DMSTATUS_ALLHALTED));


I don't get the need for the second write of dmcontrol.
Halt request should have been already generated.

Why can't this condition be merged with the condition of the first loop?

Moreover, I'm a bit concerned. What is being reported in dmstatus so that the first loop exits? Is the selected hart "running"? If so, I believe at least a warning should be issued, so that user understands that reset halt executes an unspecified amount of instructions before actually halting (which is, AFAIK, not the case on other RISC-V HW).

although halt request was set, but there're HWs which need a period of time to be aware of that request after ndmreset is deasserted by ACKHAVERESET. So the second loop is to make sure selected harts are actually halted.

I seem to have the same opinion as @en-sc:

Instead of creating another do-while loop, could the existing loop above be updated this way?

const riscv_reg_t expected_state = target->reset_halt ? DM_DMSTATUS_ALLHALTED : DM_DMSTATUS_ALLRUNNING; /* Keep waiting until both these conditions become true: * - there is no hart hart that is marked as unavailable * - all harts get to the expected state (halted/running) */ do { /* ... */ } while (get_field(dmstatus, DM_DMSTATUS_ANYUNAVAIL) || !get_field(dmstatus, expected_state));

jhqian · 2025-01-27T13:08:50Z

@jhqian, thank you for the patch. I have a couple of concerns:

AFAIU, this is a workaround. Please see [3.2. Reset Control] (RISC-V Debug Spec. Version 1.0.0-rc4, Revised 2024-12-05):

When a hart comes out of reset and haltreq or hasresethaltreq are set, the hart will immediately enter Debug Mode (halted state).

However, this exact change is classifyed as a bugfix in the spec (listed under [1.2.1.1. Bugfixes from 0.13 to 1.0]).
Though looking at the comments this is more of a clarification:
Clarify that harts halt out of reset if haltreq=1 riscv/riscv-debug-spec#419 (comment)

The second loop seems excessive. Please see the comment I left.

If introducing the second loop is the only option, have you considered addressing the issue using reset-deassert-post/pre event handlers?

@tom-van, I'd suggest first reviewing the code here since IMHO the reset procedure is much simpler in the fork.

Regarding 3, I did try that, since I'm not sure how I can tell if "current" reset is a "reset run" or "reset halt", I can only "halt" on each reset-deassert. I believe it degrades the functionality.

tom-van · 2025-01-27T13:55:21Z

4. @tom-van, I'd suggest first reviewing the code here since IMHO the reset procedure is much simpler in the fork.

Acked.
I copy my reply for completeness:

@jhqian:

But according to the funciton's intension, it needs target to be halted on exit
when reset_haltreq is set, right?

The function reset_deassert() does NOT guarantee the target is halted on its exit. As the name suggests, it handles reset deassert, nothing more.
Actual waiting for halted state after reset halt is handled in Tcl, see:
https://review.openocd.org/c/openocd/+/8725/4/src/target/startup.tcl#140

Note that a pluggable event reset-deassert-post is called between reset_deassert() and arp_waitstate halted 1000

So to keep other RISC-V targets intact and not to diverge from other targets we should leave HALTREQ set on reset_deassert() exit and clear HALTREQ later: perhaps when poll detects halt. Would it be possible?

@en-sc:

The second loop seems excessive. Please see the comment I left.

Exactly. I have the same feeling.

If introducing the second loop is the only option, have you considered addressing the issue using reset-deassert-post/pre event handlers?

Doesn't seem possible as the problem probably begins when reset_deassert() clears HALTREQ and it's perhaps too late to set it back in reset-deassert-post.

en-sc · 2025-01-27T14:14:56Z

The issue

I believe the issue is actually with the current condition of the first loop.

	} while (get_field(dmstatus, DM_DMSTATUS_ALLUNAVAIL) &&
			!get_field(dmstatus, DM_DMSTATUS_ALLHAVERESET));

If unavailable is not implemented, the loop will just skip waiting for havereset.
Next haltreq will get canceled and the reset acknoleged (though it didn't finish yet).

How to address

I'd suggest dropping the check on get_field(dmstatus, DM_DMSTATUS_ALLUNAVAIL) from the first loop's condition. It seems like the check was intended to replace the check on halted/running, and the check on havereset was introduced later.
e2cfd4e#diff-701864e2be3210275085c84b8f6fa7f9eb6171a2a18ca71638be42a155c08c63R2190

en-sc · 2025-01-27T14:27:38Z

The function reset_deassert() does NOT guarantee the target is halted on its exit. As the name suggests, it handles reset deassert, nothing more. Actual waiting for halted state after reset halt is handled in Tcl, see: https://review.openocd.org/c/openocd/+/8725/4/src/target/startup.tcl#140

@tom-van, so we should not actually wait to come out of reset here but instead report target->state == TARGET_UNKNOWN in the poll handler for a target under reset?

en-sc · 2025-01-27T14:42:05Z

On a side note, does anyone have access to a GD32VF103 board so that the change can be tested on it?

tom-van · 2025-01-27T14:50:50Z

I'd suggest dropping the check on get_field(dmstatus, DM_DMSTATUS_ALLUNAVAIL) from the first loop's condition. It seems like the check was intended to replace the check on halted/running, and the check on havereset was introduced later.

Sounds promising.

The function reset_deassert() does NOT guarantee the target is halted on its exit. As the name suggests, it handles reset deassert, nothing more. Actual waiting for halted state after reset halt is handled in Tcl, see: https://review.openocd.org/c/openocd/+/8725/4/src/target/startup.tcl#140

@tom-van, so we should not actually wait to come out of reset here but instead report target->state == TARGET_UNKNOWN in the poll handler for a target under reset?

I would be very careful with using TARGET_UNKNOWN in this case.
Lot of OpenOCD code regards this state as a big problem.
I mean we don't need to wait for halt in reset_deassert().
I think that waiting for coming out of reset fits in reset_deassert().
And if RISC-V spec requires coming out of rest in the requested state (run or halt), this is a bonus.

tom-van · 2025-01-27T14:51:47Z

On a side note, does anyone have access to a GD32VF103 board so that the change can be tested on it?

I have one.

JanMatCodasip · 2025-01-28T07:40:06Z

src/target/riscv/riscv-013.c

@@ -2924,22 +2924,50 @@ static int deassert_reset(struct target *target)
 	riscv_scan_set_delay(&info->learned_delays, RISCV_DELAY_BASE,
 			orig_base_delay);

-	/* Ack reset and clear DM_DMCONTROL_HALTREQ if previously set */
+	/* Ack reset with DM_DMCONTROL_HALTREQ for those MCUs which need a


@jhqian I am sorry, the wording of the comment is a little unclear. Did you mean to say this?

Please, make sure that all comments and descriptions are very clear and understandable. This helps all code readers, incl. reviewers.

/* Acknowledge the reset by writing dmcontrol.ackhavereset <- 1, and at the same time, * keep the dmcontrol.haltreq unchanged. This is required for certain processors that * take longer time to halt after leaving the reset. */

(Anyway, the code will probably change due to other review suggestions, and this specific comment will probably become irrelevant.)

JanMatCodasip · 2025-01-28T08:34:29Z

src/target/riscv/riscv-013.c

+						get_field(dmstatus, DM_DMSTATUS_ALLHALTED) ? "true" : "false");
+				return ERROR_TIMEOUT_REACHED;
+			}
+		} while (!get_field(dmstatus, DM_DMSTATUS_ALLHALTED));


I seem to have the same opinion as @en-sc:

Instead of creating another do-while loop, could the existing loop above be updated this way?

const riscv_reg_t expected_state = target->reset_halt ? DM_DMSTATUS_ALLHALTED : DM_DMSTATUS_ALLRUNNING; /* Keep waiting until both these conditions become true: * - there is no hart hart that is marked as unavailable * - all harts get to the expected state (halted/running) */ do { /* ... */ } while (get_field(dmstatus, DM_DMSTATUS_ANYUNAVAIL) || !get_field(dmstatus, expected_state));

JanMatCodasip · 2025-01-28T08:42:39Z

src/target/riscv/riscv-013.c

+	control = 0;
+	control = set_field(control, DM_DMCONTROL_DMACTIVE, 1);
+	control = set_dmcontrol_hartsel(control, info->index);
+	result = dm_write(target, DM_DMCONTROL, control);


t seems there's an issue related to GD32V which needs HALTREQ to be cleared after reset, otherwise "reset" command does not behave as expected.

I agree with @aap-sc:

In case target->reset_halt == false, there is no HATLREQ set, and therefore no reason to clear it. The clearing of HATLREQ (second write to dmcontrol) should, IMO, only occur when target->reset_halt == true.

@jhqian Is that correct understanding, or am I missing something?

jhqian force-pushed the riscv branch from 73d5899 to 62c0eb7 Compare January 27, 2025 07:22

aap-sc requested a review from en-sc January 27, 2025 08:56

aap-sc reviewed Jan 27, 2025

View reviewed changes

jhqian force-pushed the riscv branch from 62c0eb7 to fb750ab Compare January 27, 2025 11:35

en-sc requested changes Jan 27, 2025

View reviewed changes

en-sc mentioned this pull request Jan 27, 2025

deassert_reset should not wait for a target to come out of reset and should not update target->state #1216

Open

JanMatCodasip requested changes Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

target/riscv: make sure target is halted when reset_halt is set #1214

target/riscv: make sure target is halted when reset_halt is set #1214

jhqian commented Jan 27, 2025

aap-sc Jan 27, 2025

jhqian Jan 27, 2025

aap-sc Jan 27, 2025

jhqian Jan 27, 2025

JanMatCodasip Jan 28, 2025

aap-sc Jan 27, 2025

jhqian Jan 27, 2025

JanMatCodasip Jan 28, 2025

jhqian commented Jan 27, 2025

en-sc left a comment

en-sc Jan 27, 2025

jhqian Jan 27, 2025

JanMatCodasip Jan 28, 2025

jhqian commented Jan 27, 2025

tom-van commented Jan 27, 2025

en-sc commented Jan 27, 2025

en-sc commented Jan 27, 2025

en-sc commented Jan 27, 2025

tom-van commented Jan 27, 2025

tom-van commented Jan 27, 2025

JanMatCodasip Jan 28, 2025

JanMatCodasip Jan 28, 2025

JanMatCodasip Jan 28, 2025

target/riscv: make sure target is halted when reset_halt is set #1214

Are you sure you want to change the base?

target/riscv: make sure target is halted when reset_halt is set #1214

Conversation

jhqian commented Jan 27, 2025

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

jhqian commented Jan 27, 2025

en-sc left a 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

jhqian commented Jan 27, 2025

tom-van commented Jan 27, 2025

en-sc commented Jan 27, 2025

The issue

How to address

en-sc commented Jan 27, 2025

en-sc commented Jan 27, 2025

tom-van commented Jan 27, 2025

tom-van commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment