RP2040 USB: clear stale SOF flag before remote wakeup

[swift502]

Hi!

My Pi Pico based QMK keyboard can be woken up after being suspended (Win PC is turned off) and then never goes back to sleep. The RGB stays on the whole night and it's pretty but then I can't sleep either. 😭

I personally tested and confirmed that this patch fixes the issue for my particular use case.

However I don't understand any of this, so I ask for a review from someone more experienced, and I'll be happy to refine the PR further if needed. Below is an analysis by Claude Opus 5.5 which guided me on the fix. A QMK workaround exists but I'd obviously prefer to fix the root cause and not apply workarounds in QMK.

Thank you! Let me know what you think!
- Jan

[swift502]

Claude Opus 5.5 Analysis

What goes wrong

When the device asks the host to wake up (remote wakeup), the driver can decide the host has answered even when it hasn't. The device then switches to active and stays that way, although nothing is talking to it. This is easiest to see with a PC that is shut down but still powers its USB ports: pressing a key "wakes" the device, and it never returns to suspend.

Why it happens

As far as I can tell from the source, the driver relies on two things.

First, the comment in usb_lld_wakeup_host() notes that the RP2040 doesn't raise its resume interrupt when the device itself starts the wakeup. So the driver waits for the next Start of Frame instead, which is a sensible signal that the host is running the bus again. It does this by enabling the SOF interrupt:

ChibiOS-Contrib/os/hal/ports/RP/LLD/USBDv1/hal_usb_lld.h

Lines 416 to 422 in 7561ba7

	#define usb_lld_wakeup_host(usbp) \
	do { \
	/* remote wakeup doesn't trigger the wakeup interrupt, therefore \
	* we use the SOF interrupt to detect resume of the bus. */ \
	USB->INTE |= USB_INTE_DEV_SOF; \
	USB->SET.SIECTRL = USB_SIE_CTRL_RESUME; \
	} while (false)

And the SOF branch of the ISR completes the wakeup:

ChibiOS-Contrib/os/hal/ports/RP/LLD/USBDv1/hal_usb_lld.c

Lines 390 to 405 in 7561ba7

	/* SOF handling.*/
	if (ints & USB_INTS_DEV_SOF) {
	/* SOF interrupt was used to detect resume of the USB bus after issuing a
	* remote wake up of the host, therefore we disable it again. */
	if (usbp->config->sof_cb == NULL) {
	USB->INTE &= ~USB_INTE_DEV_SOF;
	}
	if (usbp->state == USB_SUSPENDED) {
	_usb_wakeup(usbp);
	}
	_usb_isr_invoke_sof_cb(usbp);
	/* Clear SOF flag by reading SOF_RD */
	(void)USB->SOFRD;
	}

Second, the RP2040 keeps its "SOF received" status set until the frame number register (SOF_RD) is read. The ISR handles this by reading the register, but the ISR only runs for SOFs while that interrupt is enabled. During normal operation it is only enabled if a sof_cb is configured:

ChibiOS-Contrib/os/hal/ports/RP/LLD/USBDv1/hal_usb_lld.c

Lines 499 to 508 in 7561ba7

	/* Enable interrupts */
	USB->INTE = USB_INTE_SETUP_REQ |
	USB_INTE_DEV_RESUME_FROM_HOST |
	USB_INTE_DEV_SUSPEND |
	USB_INTE_BUS_RESET |
	USB_INTE_BUFF_STATUS;
	if (usbp->config->sof_cb != NULL) {
	USB->INTE |= USB_INTE_DEV_SOF;
	}

Without a sof_cb, the status from the last frame before suspend is still set when the wakeup starts. Enabling the interrupt then triggers it straight away, and the ISR treats that old frame as proof that the host resumed.

The fix

Read SOF_RD in usb_lld_wakeup_host() just before enabling the interrupt. This clears the old status, so only a frame that arrives after the wakeup request can complete it, which I believe is what the existing code intends.

Possible side effects

I've tried to think through what else this could affect:

• A host that really wakes up: this should work as before. The host sends new frames after resuming, each one sets the status again, and the interrupt fires as expected.
• A host that starts the resume itself: that goes through DEV_RESUME_FROM_HOST, which this change doesn't touch.
• The read itself: as far as I know, reading SOF_RD only returns the frame number and clears this status, nothing else.
• Configurations with a sof_cb: the interrupt is always on there, so the status is normally cleared by the ISR anyway. The extra read could in theory swallow one pending SOF. Since the wakeup only happens while the bus is suspended and no frames are being sent, I don't think a callback can actually be lost.
• Visible behavior change: a device whose wakeup request isn't answered now stays suspended instead of becoming active on its own. That seems to match what the USB specification expects, but firmware that happened to rely on the old behavior would notice the difference.

This was tested on one RP2040 keyboard running QMK, using the steps described above. I may well be missing context about why the code is written this way, so corrections are welcome.

[swift502]

This also seems to be compatible with the intention behind e284aab by @KarlK90. It's been a long time, but just in case you had spare time to confirm or deny this is valid, I'm tagging.