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

Add local reset via VECTRESET support for cortex-m device #114

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+19 −4
Open

Add local reset via VECTRESET support for cortex-m device #114

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eec81fb
Add local reset via VECTRESET support for cortex-m device
windowsair Jun 6, 2024
94ad23c
Merge branch 'master' into reset_target
thegecko Sep 28, 2024
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
23 changes: 19 additions & 4 deletions src/processor/cortex-m.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,18 +259,33 @@ export class CortexM extends ADI implements Processor {
/**
* set the target to reset state
* @param hardwareReset use hardware reset pin or software reset
* @param localReset true: Requests a local CPU reset using VECTRESET, false: Requests a reset by an external system resource using SYSRESETREQ
* @param timeout Milliseconds to wait before aborting wait
* @returns Promise
*/
public async setTargetResetState(hardwareReset: boolean = true): Promise<void> {
public async setTargetResetState(hardwareReset: boolean = true, localReset: boolean = false, timeout: number = 1000): Promise<void> {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Adding new parameters localReset and timeout has IMO potential for confusion. They get skipped if hardwareReset is true. Adding a new method with the enhanced functionality (and reset types as enums) might be a better approach. Alternatively, the dependencies between such parameters needs to be better documented in the API description.
  • Also, local/vector resets are only available for v7-M based CPUs, e.g. Cortex-M3/M4/M7. v6-M and v8-M based CPUs don't have that capability. Writes to the AIRCR.VECTRESET bit should be ignored by HW. But one could argue that it is the consumer's responsibility to check if a certain operation will succeed.
  • Adding timeout and checking for reset recovery is in general a good idea for all reset types (also the HW reset). But as done here, the changes significantly impact timings and functionality of this function. I am a little worried about the impact on other consumers of dapjs not expecting the newly introduced delays for SYSRESETREQ.
  • timeout isn't really a timeout here but a delay. For a timeout, it should regularly poll DHCSR and return early once the reset completed. Otherwise, this method is now delayed by 1 second by default. When polling, you need to make sure to handle target access errors. Depending on the reset implementation in HW, the DAP/CPU subsystem/CPU debug registers may be temporarily unavailable.
  • If going the route of checking for reset recovery you may want to indicate this via a return value of the method. This allows consumers handling of a the case where the reset does not recover (in time).

// Enable Reset Vector Catch
await this.halt();
Copy link
Contributor

@jreineckearm jreineckearm Jun 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would be careful with halting the CPU here unconditionally. This changes behavior of this method while strictly speaking writing AIRCR.SYSRESETREQ doesn't come with being unpredictable if the CPU is running before reset. On the other hand, the reset vector catch is always enabled in this method as well. So this probably only impacts corner cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, maybe only halt when using VECTRESET?

await this.writeMem32(DebugRegister.DEMCR, DemcrMask.CORERESET);

// Clear S_RESET_ST
await this.readMem32(DebugRegister.DHCSR);

if (hardwareReset === true) {
await this.reset();
} else {
const value = await this.readMem32(NvicRegister.AIRCR);
await this.writeMem32(NvicRegister.AIRCR, AircrMask.VECTKEY | value | AircrMask.SYSRESETREQ);
await this.writeMem32(DebugRegister.DEMCR, 0);
return;
}

const value = localReset ? AircrMask.VECTRESET : AircrMask.SYSRESETREQ;
await this.writeMem32(NvicRegister.AIRCR, AircrMask.VECTKEY | value);

const waitReset = async (): Promise<boolean> => {
const ret = await this.readMem32(DebugRegister.DHCSR);
return !!(ret & DhcsrMask.S_RESET_ST); // Wait for S_RESET_ST bit set
};

await this.waitDelay(waitReset, timeout);
await this.writeMem32(DebugRegister.DEMCR, 0);
}
}
Loading