From 73b0047efc4cc657a92b356dd9b5046a44277b33 Mon Sep 17 00:00:00 2001 From: Jonathan Bell Date: Thu, 5 Jan 2023 13:36:51 +0000 Subject: [PATCH] rp2040: avoid device-mode state machine hang Don't mark IN buffers as available during the last 200us of a full-speed frame. This avoids a situation seen with the USB2.0 hub on a Raspberry Pi 4 where a late IN token before the next full-speed SOF can cause port babble and a corrupt ACK packet. The nature of the data corruption has a chance to cause device lockup. Use the next SOF to mark delayed buffers as available. This reduces available Bulk IN bandwidth by approximately 20%, and requires that the SOF interrupt is enabled while these transfers are ongoing. Inherit the top-level enable from the corresponding Pico-SDK flag. Applications that will not use the device in a situation where it could be plugged into a Pi 4 or Pi 400 (for example, when directly connected to a commodity hub or other host) can turn off the flag in the SDK. v2: use a field in hw_endpoint to mark pending. v3: Partial rewrite following review comments - Stub functions out if the workaround is not required - Only force-enable SOF while any vulnerable endpoints are active - Respect dcd_sof_enable() functionality - Get rid of all but necessary ifdef hackery - Fix a bug where the "endpoint lock" was used with an uninitialised pointer. --- hw/bsp/rp2040/family.cmake | 1 + src/portable/raspberrypi/rp2040/dcd_rp2040.c | 25 ++++++++- src/portable/raspberrypi/rp2040/rp2040_usb.c | 54 +++++++++++++++++--- src/portable/raspberrypi/rp2040/rp2040_usb.h | 24 ++++++++- 4 files changed, 96 insertions(+), 8 deletions(-) diff --git a/hw/bsp/rp2040/family.cmake b/hw/bsp/rp2040/family.cmake index 5dba9dc39..f83120567 100644 --- a/hw/bsp/rp2040/family.cmake +++ b/hw/bsp/rp2040/family.cmake @@ -116,6 +116,7 @@ if (NOT TARGET _rp2040_family_inclusion_marker) target_compile_definitions(tinyusb_additions INTERFACE PICO_RP2040_USB_DEVICE_ENUMERATION_FIX=1 + PICO_RP2040_USB_DEVICE_UFRAME_FIX=1 ) if(DEFINED LOG) diff --git a/src/portable/raspberrypi/rp2040/dcd_rp2040.c b/src/portable/raspberrypi/rp2040/dcd_rp2040.c index 33edf0f1e..35faba628 100644 --- a/src/portable/raspberrypi/rp2040/dcd_rp2040.c +++ b/src/portable/raspberrypi/rp2040/dcd_rp2040.c @@ -246,13 +246,32 @@ static void __tusb_irq_path_func(dcd_rp2040_irq)(void) { uint32_t const status = usb_hw->ints; uint32_t handled = 0; + bool keep_sof_alive = false; if (status & USB_INTF_DEV_SOF_BITS) { handled |= USB_INTF_DEV_SOF_BITS; +#if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX + last_sof = time_us_32(); + + for (uint8_t i = 0; i < USB_MAX_ENDPOINTS; i++) { + struct hw_endpoint *ep = hw_endpoint_get_by_num(i, TUSB_DIR_IN); + hw_endpoint_lock_update(ep, 1); + // Bulk IN endpoint in a transfer? + if (rp2040_ep_needs_sof(ep) && ep->active) + keep_sof_alive = true; + // Deferred enable? + if (ep->pending) { + hw_endpoint_start_next_buffer(ep); + ep->pending = 0; + } + hw_endpoint_lock_update(ep, -1); + } +#endif // disable SOF interrupt if it is used for RESUME in remote wakeup - if (!_sof_enable) usb_hw_clear->inte = USB_INTS_DEV_SOF_BITS; + if (!keep_sof_alive && !_sof_enable) + usb_hw_clear->inte = USB_INTS_DEV_SOF_BITS; dcd_event_sof(0, usb_hw->sof_rd & USB_SOF_RD_BITS, true); } @@ -449,7 +468,11 @@ void dcd_sof_enable(uint8_t rhport, bool en) usb_hw_set->inte = USB_INTS_DEV_SOF_BITS; }else { + // Don't clear immediately if the SOF workaround is in use. + // The SOF handler will conditionally disable the interrupt. +#if !TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX usb_hw_clear->inte = USB_INTS_DEV_SOF_BITS; +#endif } } diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c index 77d5f2f8f..6a9f162a5 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.c +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c @@ -38,10 +38,6 @@ const char *ep_dir_string[] = { "in", }; -#if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX -volatile uint32_t last_sof = 0; -#endif - static void _hw_endpoint_xfer_sync(struct hw_endpoint *ep); // if usb hardware is in host mode @@ -54,6 +50,41 @@ TU_ATTR_ALWAYS_INLINE static inline bool is_host_mode(void) // //--------------------------------------------------------------------+ +#if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX +volatile uint32_t last_sof = 0; + +bool rp2040_critical_frame_period(struct hw_endpoint *ep) +{ + uint32_t delta; + + if (usb_hw->main_ctrl & USB_MAIN_CTRL_HOST_NDEVICE_BITS) + return false; + + if (tu_edpt_dir(ep->ep_addr) == TUSB_DIR_OUT || + ep->transfer_type == TUSB_XFER_INTERRUPT || + ep->transfer_type == TUSB_XFER_ISOCHRONOUS) + return false; + + /* Avoid the last 200us (uframe 6.5-7) of a frame, up to the EOF2 point. + * The device state machine cannot recover from receiving an incorrect PID + * when it is expecting an ACK. + */ + delta = time_us_32() - last_sof; + if (delta < 800 || delta > 998) { + return false; + } + TU_LOG(3, "Avoiding sof %u now %lu last %lu\n", (usb_hw->sof_rd + 1) & USB_SOF_RD_BITS, now, last_sof); + return true; +} + +bool rp2040_ep_needs_sof(struct hw_endpoint *ep) { + if (tu_edpt_dir(ep->ep_addr) == TUSB_DIR_IN && + ep->transfer_type == TUSB_XFER_BULK) + return true; + return false; +} +#endif + void rp2040_usb_init(void) { // Reset usb controller @@ -215,7 +246,14 @@ void hw_endpoint_xfer_start(struct hw_endpoint *ep, uint8_t *buffer, uint16_t to ep->active = true; ep->user_buf = buffer; - hw_endpoint_start_next_buffer(ep); + if (rp2040_ep_needs_sof(ep)) + usb_hw_set->inte = USB_INTS_DEV_SOF_BITS; + + if(!rp2040_critical_frame_period(ep)) { + hw_endpoint_start_next_buffer(ep); + } else { + ep->pending = 1; + } hw_endpoint_lock_update(ep, -1); } @@ -331,7 +369,11 @@ bool __tusb_irq_path_func(hw_endpoint_xfer_continue)(struct hw_endpoint *ep) } else { - hw_endpoint_start_next_buffer(ep); + if(!rp2040_critical_frame_period(ep)) { + hw_endpoint_start_next_buffer(ep); + } else { + ep->pending = 1; + } } hw_endpoint_lock_update(ep, -1); diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.h b/src/portable/raspberrypi/rp2040/rp2040_usb.h index be1f1e5ec..717ec3514 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.h +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.h @@ -11,11 +11,21 @@ #include "hardware/structs/usb.h" #include "hardware/irq.h" #include "hardware/resets.h" +#include "hardware/timer.h" #if defined(PICO_RP2040_USB_DEVICE_ENUMERATION_FIX) && !defined(TUD_OPT_RP2040_USB_DEVICE_ENUMERATION_FIX) #define TUD_OPT_RP2040_USB_DEVICE_ENUMERATION_FIX PICO_RP2040_USB_DEVICE_ENUMERATION_FIX #endif +#if defined(PICO_RP2040_USB_DEVICE_UFRAME_FIX) && !defined(TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX) +#define TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX PICO_RP2040_USB_DEVICE_UFRAME_FIX +#endif + +#if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX +#undef PICO_RP2040_USB_FAST_IRQ +#define PICO_RP2040_USB_FAST_IRQ 1 +#endif + #ifndef PICO_RP2040_USB_FAST_IRQ #define PICO_RP2040_USB_FAST_IRQ 0 #endif @@ -67,7 +77,9 @@ typedef struct hw_endpoint // Interrupt, bulk, etc uint8_t transfer_type; - + + // Transfer scheduled but not active + uint8_t pending; #if CFG_TUH_ENABLED // Only needed for host uint8_t dev_addr; @@ -77,6 +89,16 @@ typedef struct hw_endpoint #endif } hw_endpoint_t; +#if !TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX +#define rp2040_critical_frame_period(x) false +#define rp2040_ep_needs_sof(x) false +#else +extern volatile uint32_t last_sof; + +bool rp2040_critical_frame_period(struct hw_endpoint *ep); +bool rp2040_ep_needs_sof(struct hw_endpoint *ep); +#endif + void rp2040_usb_init(void); void hw_endpoint_xfer_start(struct hw_endpoint *ep, uint8_t *buffer, uint16_t total_len);