Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: riscv
Choose a base branch
from
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion src/target/riscv/riscv-013.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 */

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @JanMatCodasip. Agree, comments should be clear enough.

* period of time to be halted after reset is released */
control = 0;
control = set_field(control, DM_DMCONTROL_DMACTIVE, 1);
control = set_field(control, DM_DMCONTROL_ACKHAVERESET, 1);
control = set_field(control, DM_DMCONTROL_HALTREQ, target->reset_halt ? 1 : 0);
control = set_dmcontrol_hartsel(control, info->index);
result = dm_write(target, DM_DMCONTROL, control);
if (result != ERROR_OK)
return result;

if (target->reset_halt) {
/* Wait for all harts to halt for those MCUs mentioned above */
time_t halt_start = time(NULL);
do {
result = dmstatus_read(target, &dmstatus, true);
if (result != ERROR_OK)
return result;

if (time(NULL) - halt_start > riscv_get_command_timeout_sec()) {
LOG_TARGET_ERROR(target, "Hart didn't halt after reset in %ds; "
"dmstatus=0x%x (anyhalted=%s, allhalted=%s); "
"Increase the timeout with riscv set_command_timeout_sec.",
riscv_get_command_timeout_sec(), dmstatus,
get_field(dmstatus, DM_DMSTATUS_ANYHALTED) ? "true" : "false",
get_field(dmstatus, DM_DMSTATUS_ALLHALTED) ? "true" : "false");
return ERROR_TIMEOUT_REACHED;
}
} while (!get_field(dmstatus, DM_DMSTATUS_ALLHALTED));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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));

target->state = TARGET_HALTED;
target->debug_reason = DBG_REASON_DBGRQ;
} else {
target->state = TARGET_RUNNING;
target->debug_reason = DBG_REASON_NOTHALTED;
}

/* clear DM_DMCONTROL_HALTREQ */
control = 0;
control = set_field(control, DM_DMCONTROL_DMACTIVE, 1);
control = set_dmcontrol_hartsel(control, info->index);
result = dm_write(target, DM_DMCONTROL, control);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not this write be conditional?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

if (result != ERROR_OK)
return result;

info->dcsr_ebreak_is_set = dcsr_ebreak_config_equals_reset_value(target);
return ERROR_OK;
}
Expand Down
Loading