-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
RP2040: avoid XIP hangs during flash operations with scheduler=cores #5411
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.
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
base: dev
Are you sure you want to change the base?
Changes from 9 commits
ae60b18
caa56da
920df83
c907842
51901b2
d1af9b4
d9f4924
7e24ce0
27373a0
c1f3c64
ddc5485
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| //go:build tinygo && rp2040 | ||
|
|
||
| // "Flash safe" follows the RP2040/Pico SDK terminology: flash operations | ||
| // must run while the other core is not executing from XIP flash. | ||
| // | ||
| // Use linkname to call runtime hooks from package machine without creating | ||
| // an import cycle. | ||
|
|
||
| package machine | ||
|
|
||
| import ( | ||
| "runtime/interrupt" | ||
| _ "unsafe" | ||
| ) | ||
|
|
||
| //go:linkname rp2040EnterFlashSafeSection runtime.rp2040EnterFlashSafeSection | ||
| func rp2040EnterFlashSafeSection() interrupt.State | ||
|
|
||
| //go:linkname rp2040ExitFlashSafeSection runtime.rp2040ExitFlashSafeSection | ||
| func rp2040ExitFlashSafeSection(state interrupt.State) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,7 +3,6 @@ | |
| package machine | ||
|
|
||
| import ( | ||
| "runtime/interrupt" | ||
| "unsafe" | ||
| ) | ||
|
|
||
|
|
@@ -206,11 +205,20 @@ func doFlashCommand(tx []byte, rx []byte) error { | |
| if len(tx) != len(rx) { | ||
| return errFlashInvalidWriteLength | ||
| } | ||
| if len(tx) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| txbuf := make([]byte, len(tx)) | ||
| copy(txbuf, tx) | ||
|
|
||
| state := rp2040EnterFlashSafeSection() | ||
| defer rp2040ExitFlashSafeSection(state) | ||
|
|
||
| C.flash_do_cmd( | ||
| (*C.uint8_t)(unsafe.Pointer(&tx[0])), | ||
| (*C.uint8_t)(unsafe.Pointer(&txbuf[0])), | ||
| (*C.uint8_t)(unsafe.Pointer(&rx[0])), | ||
| C.ulong(len(tx))) | ||
| C.ulong(len(txbuf))) | ||
|
|
||
| return nil | ||
| } | ||
|
|
@@ -223,20 +231,25 @@ func (f flashBlockDevice) writeAt(p []byte, off int64) (n int, err error) { | |
| return 0, errFlashCannotWritePastEOF | ||
| } | ||
|
|
||
| state := interrupt.Disable() | ||
| defer interrupt.Restore(state) | ||
|
|
||
| // rp2040 writes to offset, not actual address | ||
| // e.g. real address 0x10003000 is written to at | ||
| // 0x00003000 | ||
| address := writeAddress(off) | ||
| padded := flashPad(p, int(f.WriteBlockSize())) | ||
| buf := make([]byte, len(padded)) | ||
| copy(buf, padded) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, this is potentially very expensive. In fact,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here. I removed the unconditional RAM copy and now use the padded slice directly, so flashPad's buffer is not copied again. |
||
| if len(buf) == 0 { | ||
| return 0, nil | ||
| } | ||
|
|
||
| state := rp2040EnterFlashSafeSection() | ||
| defer rp2040ExitFlashSafeSection(state) | ||
|
|
||
| C.flash_range_write(C.uint32_t(address), | ||
| (*C.uint8_t)(unsafe.Pointer(&padded[0])), | ||
| C.ulong(len(padded))) | ||
| (*C.uint8_t)(unsafe.Pointer(&buf[0])), | ||
| C.ulong(len(buf))) | ||
|
|
||
| return len(padded), nil | ||
| return len(buf), nil | ||
| } | ||
|
|
||
| func (f flashBlockDevice) eraseBlocks(start, length int64) error { | ||
|
|
@@ -245,8 +258,8 @@ func (f flashBlockDevice) eraseBlocks(start, length int64) error { | |
| return errFlashCannotErasePastEOF | ||
| } | ||
|
|
||
| state := interrupt.Disable() | ||
| defer interrupt.Restore(state) | ||
| state := rp2040EnterFlashSafeSection() | ||
| defer rp2040ExitFlashSafeSection(state) | ||
|
|
||
| C.flash_erase_blocks(C.uint32_t(address), C.ulong(length*f.EraseBlockSize())) | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,109 @@ | ||||
| //go:build rp2040 && scheduler.cores | ||||
|
|
||||
| package runtime | ||||
|
|
||||
| import ( | ||||
| "device/arm" | ||||
| "device/rp" | ||||
| "runtime/interrupt" | ||||
| "runtime/volatile" | ||||
| _ "unsafe" // required for //go:section | ||||
| ) | ||||
|
|
||||
| const ( | ||||
| rp2040FlashSafeIdle uint8 = iota | ||||
| rp2040FlashSafeLocked | ||||
| rp2040FlashSafeRelease | ||||
| ) | ||||
|
|
||||
| // rp2040FlashSafeState is used to synchronize the core that performs a flash | ||||
| // operation with the other core that must stop executing from XIP flash. | ||||
| var rp2040FlashSafeState volatile.Register8 | ||||
|
|
||||
| // flashSafeLock serializes Enter/Exit so that only one core at a time | ||||
| // owns the flash-safe state machine. The other core can still participate | ||||
| // as a victim through the FIFO interrupt while spinning on this lock. | ||||
| // | ||||
| // id: 24 is reserved here. ids 20-23 are already used by printLock, | ||||
| // schedulerLock, atomicsLock, futexLock (see runtime_rp2.go). | ||||
| var flashSafeLock = spinLock{id: 24} | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please either put this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I moved flashSafeLock next to the other RP2 runtime spinLock definitions in runtime_rp2.go, so the spinLock IDs remain visible in one place. |
||||
|
|
||||
| // rp2040EnterFlashSafeSection enters a section in which RP2040 flash operations | ||||
| // may temporarily disable XIP. | ||||
| // | ||||
| // The multicore path asks the other core to enter the flash-safe interrupt | ||||
| // handler and waits until it acknowledges that it is parked. Local interrupts | ||||
| // are disabled after the other core is parked. | ||||
| func rp2040EnterFlashSafeSection() interrupt.State { | ||||
| if !secondaryCoresStarted { | ||||
| return interrupt.Disable() | ||||
| } | ||||
|
|
||||
| flashSafeLock.Lock() | ||||
|
|
||||
| core := currentCPU() | ||||
| rp2040FlashSafeState.Set(rp2040FlashSafeIdle) | ||||
|
|
||||
| for i := uint32(0); i < numCPU; i++ { | ||||
| if i == core { | ||||
| continue | ||||
| } | ||||
| rp2040FlashSafePauseCore(i) | ||||
| } | ||||
|
|
||||
| for rp2040FlashSafeState.Get() != rp2040FlashSafeLocked { | ||||
| spinLoopWait() | ||||
| } | ||||
|
|
||||
| return interrupt.Disable() | ||||
| } | ||||
|
|
||||
| // rp2040ExitFlashSafeSection exits a section entered by | ||||
| // rp2040EnterFlashSafeSection. | ||||
| func rp2040ExitFlashSafeSection(state interrupt.State) { | ||||
| if secondaryCoresStarted { | ||||
| rp2040FlashSafeState.Set(rp2040FlashSafeRelease) | ||||
| arm.Asm("sev") | ||||
|
|
||||
| for rp2040FlashSafeState.Get() != rp2040FlashSafeIdle { | ||||
| spinLoopWait() | ||||
| } | ||||
|
|
||||
| flashSafeLock.Unlock() | ||||
| } | ||||
|
|
||||
| interrupt.Restore(state) | ||||
| } | ||||
|
|
||||
| func rp2040FlashSafePauseCore(core uint32) { | ||||
| _ = core // RP2040 SIO FIFO writes to the other core. | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: this isn't doing anything (though the comment is helpful and would be useful to keep)
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. I removed the unused assignment and kept the RP2040 SIO FIFO comment. |
||||
| rp.SIO.FIFO_WR.Set(rp2SIOFIFOCommandFlashSafe) | ||||
| arm.Asm("sev") | ||||
| } | ||||
|
|
||||
| // rp2FlashSafeInterruptHandler runs on the other core while this core is | ||||
| // performing a flash operation that temporarily disables XIP. | ||||
| // | ||||
| // This function MUST be placed in RAM (.ramfuncs section). During the | ||||
| // flash operation the QSPI flash is in non-XIP mode and instruction | ||||
| // fetches from the 0x10000000 region will fail. The wait loop below | ||||
| // runs entirely from RAM so that the parked core can keep executing. | ||||
| // | ||||
| //go:section .ramfuncs | ||||
| func rp2FlashSafeInterruptHandler(core uint32) { | ||||
| _ = core | ||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here, is this for linter reasons? Right not it just adds noise.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. It wasn't for linter reasons; removing it doesn't cause any warning, so I removed the unused assignment here as well. |
||||
|
|
||||
| state := interrupt.Disable() | ||||
|
|
||||
| rp2040FlashSafeState.Set(rp2040FlashSafeLocked) | ||||
| arm.Asm("sev") | ||||
|
|
||||
| for rp2040FlashSafeState.Get() == rp2040FlashSafeLocked { | ||||
| arm.Asm("wfe") | ||||
| } | ||||
|
|
||||
| interrupt.Restore(state) | ||||
|
|
||||
| rp2040FlashSafeState.Set(rp2040FlashSafeIdle) | ||||
| arm.Asm("sev") | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| //go:build rp2040 && !scheduler.cores | ||
|
|
||
| package runtime | ||
|
|
||
| import "runtime/interrupt" | ||
|
|
||
| func rp2040EnterFlashSafeSection() interrupt.State { | ||
| return interrupt.Disable() | ||
| } | ||
|
|
||
| func rp2040ExitFlashSafeSection(state interrupt.State) { | ||
| interrupt.Restore(state) | ||
| } | ||
|
|
||
| func rp2FlashSafeInterruptHandler(uint32) { | ||
| // No-op on single-core schedulers. | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| //go:build rp2350 | ||
|
|
||
| package runtime | ||
|
|
||
| func rp2FlashSafeInterruptHandler(uint32) { | ||
| // No-op on RP2350. RP2350 flash-safe handling is intentionally unchanged. | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this need to be copied to a newly heap-allocated array, instead of just using
txdirectly?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this is an important concern. I removed the unconditional RAM copy here.
I originally added it after seeing failures when the source buffer itself was placed in flash, but that is a separate pre-existing limitation: using an XIP flash address as the source buffer is not safe even with the tasks scheduler.
For this PR, I think it is better to rely on the caller passing a RAM-resident buffer, as before, and avoid adding an unconditional heap allocation. XIP-flash source buffers should be handled explicitly in a separate change, either by copying only when the source overlaps XIP flash or by returning an error for that case.