From c3e47c31ccd42b144a8a770ed56b8cdc4c2932cc Mon Sep 17 00:00:00 2001 From: Jonathan Bell Date: Mon, 5 Dec 2022 11:56:42 +0000 Subject: [PATCH 1/7] rp2040: export hw_endpoint_start_next_buffer() and hw_endpoint_lock_update() The next change to the driver requires the export of these functions. Leave the lock unimplemented for now. Also move hw_set and hw_clear aliases into the top-level header file. --- src/portable/raspberrypi/rp2040/dcd_rp2040.c | 3 --- src/portable/raspberrypi/rp2040/hcd_rp2040.c | 3 --- src/portable/raspberrypi/rp2040/rp2040_usb.c | 25 +++++++++----------- src/portable/raspberrypi/rp2040/rp2040_usb.h | 10 ++++++++ 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/src/portable/raspberrypi/rp2040/dcd_rp2040.c b/src/portable/raspberrypi/rp2040/dcd_rp2040.c index 4952d29b1..33edf0f1e 100644 --- a/src/portable/raspberrypi/rp2040/dcd_rp2040.c +++ b/src/portable/raspberrypi/rp2040/dcd_rp2040.c @@ -46,9 +46,6 @@ /* Low level controller *------------------------------------------------------------------*/ -#define usb_hw_set hw_set_alias(usb_hw) -#define usb_hw_clear hw_clear_alias(usb_hw) - // Init these in dcd_init static uint8_t *next_buffer_ptr; diff --git a/src/portable/raspberrypi/rp2040/hcd_rp2040.c b/src/portable/raspberrypi/rp2040/hcd_rp2040.c index 9e6bdad44..28abd7939 100644 --- a/src/portable/raspberrypi/rp2040/hcd_rp2040.c +++ b/src/portable/raspberrypi/rp2040/hcd_rp2040.c @@ -56,9 +56,6 @@ static_assert(PICO_USB_HOST_INTERRUPT_ENDPOINTS <= USB_MAX_ENDPOINTS, ""); static struct hw_endpoint ep_pool[1 + PICO_USB_HOST_INTERRUPT_ENDPOINTS]; #define epx (ep_pool[0]) -#define usb_hw_set hw_set_alias(usb_hw) -#define usb_hw_clear hw_clear_alias(usb_hw) - // Flags we set by default in sie_ctrl (we add other bits on top) enum { SIE_CTRL_BASE = USB_SIE_CTRL_SOF_EN_BITS | USB_SIE_CTRL_KEEP_ALIVE_EN_BITS | diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c index 5c7645902..77d5f2f8f 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.c +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c @@ -38,14 +38,11 @@ const char *ep_dir_string[] = { "in", }; -TU_ATTR_ALWAYS_INLINE static inline void _hw_endpoint_lock_update(__unused struct hw_endpoint * ep, __unused int delta) { - // todo add critsec as necessary to prevent issues between worker and IRQ... - // note that this is perhaps as simple as disabling IRQs because it would make - // sense to have worker and IRQ on same core, however I think using critsec is about equivalent. -} +#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); -static void _hw_endpoint_start_next_buffer(struct hw_endpoint *ep); // if usb hardware is in host mode TU_ATTR_ALWAYS_INLINE static inline bool is_host_mode(void) @@ -157,7 +154,7 @@ static uint32_t __tusb_irq_path_func(prepare_ep_buffer)(struct hw_endpoint *ep, } // Prepare buffer control register value -static void __tusb_irq_path_func(_hw_endpoint_start_next_buffer)(struct hw_endpoint *ep) +void __tusb_irq_path_func(hw_endpoint_start_next_buffer)(struct hw_endpoint *ep) { uint32_t ep_ctrl = *ep->endpoint_control; @@ -201,7 +198,7 @@ static void __tusb_irq_path_func(_hw_endpoint_start_next_buffer)(struct hw_endpo void hw_endpoint_xfer_start(struct hw_endpoint *ep, uint8_t *buffer, uint16_t total_len) { - _hw_endpoint_lock_update(ep, 1); + hw_endpoint_lock_update(ep, 1); if ( ep->active ) { @@ -218,8 +215,8 @@ 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); - _hw_endpoint_lock_update(ep, -1); + hw_endpoint_start_next_buffer(ep); + hw_endpoint_lock_update(ep, -1); } // sync endpoint buffer and return transferred bytes @@ -312,7 +309,7 @@ static void __tusb_irq_path_func(_hw_endpoint_xfer_sync) (struct hw_endpoint *ep // Returns true if transfer is complete bool __tusb_irq_path_func(hw_endpoint_xfer_continue)(struct hw_endpoint *ep) { - _hw_endpoint_lock_update(ep, 1); + hw_endpoint_lock_update(ep, 1); // Part way through a transfer if (!ep->active) { @@ -329,15 +326,15 @@ bool __tusb_irq_path_func(hw_endpoint_xfer_continue)(struct hw_endpoint *ep) pico_trace("Completed transfer of %d bytes on ep %d %s\n", ep->xferred_len, tu_edpt_number(ep->ep_addr), ep_dir_string[tu_edpt_dir(ep->ep_addr)]); // Notify caller we are done so it can notify the tinyusb stack - _hw_endpoint_lock_update(ep, -1); + hw_endpoint_lock_update(ep, -1); return true; } else { - _hw_endpoint_start_next_buffer(ep); + hw_endpoint_start_next_buffer(ep); } - _hw_endpoint_lock_update(ep, -1); + hw_endpoint_lock_update(ep, -1); // More work to do return false; } diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.h b/src/portable/raspberrypi/rp2040/rp2040_usb.h index b65d32fd4..be1f1e5ec 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.h +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.h @@ -26,6 +26,9 @@ #define __tusb_irq_path_func(x) x #endif +#define usb_hw_set hw_set_alias(usb_hw) +#define usb_hw_clear hw_clear_alias(usb_hw) + #define pico_info(...) TU_LOG(2, __VA_ARGS__) #define pico_trace(...) TU_LOG(3, __VA_ARGS__) @@ -79,6 +82,13 @@ void rp2040_usb_init(void); void hw_endpoint_xfer_start(struct hw_endpoint *ep, uint8_t *buffer, uint16_t total_len); bool hw_endpoint_xfer_continue(struct hw_endpoint *ep); void hw_endpoint_reset_transfer(struct hw_endpoint *ep); +void hw_endpoint_start_next_buffer(struct hw_endpoint *ep); + +TU_ATTR_ALWAYS_INLINE static inline void hw_endpoint_lock_update(__unused struct hw_endpoint * ep, __unused int delta) { + // todo add critsec as necessary to prevent issues between worker and IRQ... + // note that this is perhaps as simple as disabling IRQs because it would make + // sense to have worker and IRQ on same core, however I think using critsec is about equivalent. +} void _hw_endpoint_buffer_control_update32(struct hw_endpoint *ep, uint32_t and_mask, uint32_t or_mask); From 73b0047efc4cc657a92b356dd9b5046a44277b33 Mon Sep 17 00:00:00 2001 From: Jonathan Bell Date: Thu, 5 Jan 2023 13:36:51 +0000 Subject: [PATCH 2/7] 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); From 0d2078d295667ef985675c398db7fd6a893be895 Mon Sep 17 00:00:00 2001 From: Jonathan Bell Date: Tue, 24 Jan 2023 12:02:32 +0000 Subject: [PATCH 3/7] rp2040: shuffle hw_endpoint members Ordering by element size prevents alignment holes, and as a consequence the host mode version of the struct is the same size as device, as pad bytes at the end are used instead. --- src/portable/raspberrypi/rp2040/rp2040_usb.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.h b/src/portable/raspberrypi/rp2040/rp2040_usb.h index 717ec3514..e7fee3e3a 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.h +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.h @@ -51,7 +51,7 @@ typedef struct hw_endpoint // Transfer direction (i.e. IN is rx for host but tx for device) // allows us to common up transfer functions bool rx; - + uint8_t ep_addr; uint8_t next_pid; @@ -64,17 +64,19 @@ typedef struct hw_endpoint // Buffer pointer in usb dpram uint8_t *hw_data_buf; - // Current transfer information - bool active; - uint16_t remaining_len; - uint16_t xferred_len; - // User buffer in main memory uint8_t *user_buf; + // Current transfer information + uint16_t remaining_len; + uint16_t xferred_len; + // Data needed from EP descriptor uint16_t wMaxPacketSize; + // Endpoint is in use + bool active; + // Interrupt, bulk, etc uint8_t transfer_type; From 0cce42fcc672cb3e0d2fe16c20b46aab9d8f94d5 Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 31 Jan 2023 11:38:15 +0700 Subject: [PATCH 4/7] minor clean up --- src/portable/raspberrypi/rp2040/dcd_rp2040.c | 19 ++++++++------ src/portable/raspberrypi/rp2040/rp2040_usb.c | 26 +++++++++++++------- src/portable/raspberrypi/rp2040/rp2040_usb.h | 2 ++ 3 files changed, 31 insertions(+), 16 deletions(-) diff --git a/src/portable/raspberrypi/rp2040/dcd_rp2040.c b/src/portable/raspberrypi/rp2040/dcd_rp2040.c index 35faba628..d5a105a29 100644 --- a/src/portable/raspberrypi/rp2040/dcd_rp2040.c +++ b/src/portable/raspberrypi/rp2040/dcd_rp2040.c @@ -246,32 +246,37 @@ 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) { + bool keep_sof_alive = false; + 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++) { + 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; + if (rp2040_ep_needs_sof(ep) && ep->active) keep_sof_alive = true; + // Deferred enable? - if (ep->pending) { + 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 (!keep_sof_alive && !_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); } diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c index 6a9f162a5..b7790e0c1 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.c +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c @@ -57,13 +57,14 @@ 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 (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 @@ -78,10 +79,8 @@ bool rp2040_critical_frame_period(struct hw_endpoint *ep) } 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; + return (tu_edpt_dir(ep->ep_addr) == TUSB_DIR_IN && + ep->transfer_type == TUSB_XFER_BULK); } #endif @@ -115,12 +114,15 @@ void __tusb_irq_path_func(hw_endpoint_reset_transfer)(struct hw_endpoint *ep) ep->user_buf = 0; } -void __tusb_irq_path_func(_hw_endpoint_buffer_control_update32)(struct hw_endpoint *ep, uint32_t and_mask, uint32_t or_mask) { +void __tusb_irq_path_func(_hw_endpoint_buffer_control_update32)(struct hw_endpoint *ep, uint32_t and_mask, uint32_t or_mask) +{ uint32_t value = 0; + if ( and_mask ) { value = *ep->buffer_control & and_mask; } + if ( or_mask ) { value |= or_mask; @@ -146,6 +148,7 @@ void __tusb_irq_path_func(_hw_endpoint_buffer_control_update32)(struct hw_endpoi #endif } } + *ep->buffer_control = value; } @@ -247,13 +250,18 @@ void hw_endpoint_xfer_start(struct hw_endpoint *ep, uint8_t *buffer, uint16_t to ep->user_buf = buffer; if (rp2040_ep_needs_sof(ep)) + { usb_hw_set->inte = USB_INTS_DEV_SOF_BITS; + } - if(!rp2040_critical_frame_period(ep)) { + if(!rp2040_critical_frame_period(ep)) + { hw_endpoint_start_next_buffer(ep); - } else { + } 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 e7fee3e3a..d41abfee0 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.h +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.h @@ -82,6 +82,7 @@ typedef struct hw_endpoint // Transfer scheduled but not active uint8_t pending; + #if CFG_TUH_ENABLED // Only needed for host uint8_t dev_addr; @@ -89,6 +90,7 @@ typedef struct hw_endpoint // If interrupt endpoint uint8_t interrupt_num; #endif + } hw_endpoint_t; #if !TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX From 19b6cbc616e65dcb2ecdcac66f873ce416a4bf25 Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 31 Jan 2023 14:48:11 +0700 Subject: [PATCH 5/7] add e15 prefix or walkaround related functions, also minor refactor --- src/portable/raspberrypi/rp2040/dcd_rp2040.c | 177 ++++++++++--------- src/portable/raspberrypi/rp2040/rp2040_usb.c | 57 +++--- src/portable/raspberrypi/rp2040/rp2040_usb.h | 10 +- 3 files changed, 125 insertions(+), 119 deletions(-) diff --git a/src/portable/raspberrypi/rp2040/dcd_rp2040.c b/src/portable/raspberrypi/rp2040/dcd_rp2040.c index d5a105a29..ab1185102 100644 --- a/src/portable/raspberrypi/rp2040/dcd_rp2040.c +++ b/src/portable/raspberrypi/rp2040/dcd_rp2040.c @@ -244,128 +244,133 @@ static void __tusb_irq_path_func(reset_non_control_endpoints)(void) static void __tusb_irq_path_func(dcd_rp2040_irq)(void) { - uint32_t const status = usb_hw->ints; - uint32_t handled = 0; + uint32_t const status = usb_hw->ints; + uint32_t handled = 0; - if (status & USB_INTF_DEV_SOF_BITS) - { - bool keep_sof_alive = false; + if ( status & USB_INTF_DEV_SOF_BITS ) + { + bool keep_sof_alive = false; - handled |= USB_INTF_DEV_SOF_BITS; + handled |= USB_INTF_DEV_SOF_BITS; #if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX - last_sof = time_us_32(); + // Errata 15 Walkaround for Device Bulk-In endpoint + e15_last_sof = time_us_32(); - for (uint8_t i = 0; i < USB_MAX_ENDPOINTS; i++) + for ( uint8_t i = 0; i < USB_MAX_ENDPOINTS; i++ ) + { + struct hw_endpoint * ep = hw_endpoint_get_by_num(i, TUSB_DIR_IN); + + // Active Bulk IN endpoint requires SOF + if ( (ep->transfer_type == TUSB_XFER_BULK) && ep->active ) { - struct hw_endpoint *ep = hw_endpoint_get_by_num(i, TUSB_DIR_IN); + keep_sof_alive = true; + 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) + if ( ep->pending ) { - hw_endpoint_start_next_buffer(ep); ep->pending = 0; + hw_endpoint_start_next_buffer(ep); } hw_endpoint_lock_update(ep, -1); } + } #endif - // disable SOF interrupt if it is used for RESUME in remote wakeup - if (!keep_sof_alive && !_sof_enable) usb_hw_clear->inte = USB_INTS_DEV_SOF_BITS; + // disable SOF interrupt if it is used for RESUME in remote wakeup + 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); - } + dcd_event_sof(0, usb_hw->sof_rd & USB_SOF_RD_BITS, true); + } - // xfer events are handled before setup req. So if a transfer completes immediately - // before closing the EP, the events will be delivered in same order. - if (status & USB_INTS_BUFF_STATUS_BITS) - { - handled |= USB_INTS_BUFF_STATUS_BITS; - hw_handle_buff_status(); - } + // xfer events are handled before setup req. So if a transfer completes immediately + // before closing the EP, the events will be delivered in same order. + if ( status & USB_INTS_BUFF_STATUS_BITS ) + { + handled |= USB_INTS_BUFF_STATUS_BITS; + hw_handle_buff_status(); + } - if (status & USB_INTS_SETUP_REQ_BITS) - { - handled |= USB_INTS_SETUP_REQ_BITS; - uint8_t const *setup = (uint8_t const *)&usb_dpram->setup_packet; + if ( status & USB_INTS_SETUP_REQ_BITS ) + { + handled |= USB_INTS_SETUP_REQ_BITS; + uint8_t const * setup = (uint8_t const*) &usb_dpram->setup_packet; - // reset pid to both 1 (data and ack) - reset_ep0_pid(); + // reset pid to both 1 (data and ack) + reset_ep0_pid(); - // Pass setup packet to tiny usb - dcd_event_setup_received(0, setup, true); - usb_hw_clear->sie_status = USB_SIE_STATUS_SETUP_REC_BITS; - } + // Pass setup packet to tiny usb + dcd_event_setup_received(0, setup, true); + usb_hw_clear->sie_status = USB_SIE_STATUS_SETUP_REC_BITS; + } #if FORCE_VBUS_DETECT == 0 - // Since we force VBUS detect On, device will always think it is connected and - // couldn't distinguish between disconnect and suspend - if (status & USB_INTS_DEV_CONN_DIS_BITS) + // Since we force VBUS detect On, device will always think it is connected and + // couldn't distinguish between disconnect and suspend + if (status & USB_INTS_DEV_CONN_DIS_BITS) + { + handled |= USB_INTS_DEV_CONN_DIS_BITS; + + if ( usb_hw->sie_status & USB_SIE_STATUS_CONNECTED_BITS ) { - handled |= USB_INTS_DEV_CONN_DIS_BITS; - - if ( usb_hw->sie_status & USB_SIE_STATUS_CONNECTED_BITS ) - { - // Connected: nothing to do - }else - { - // Disconnected - dcd_event_bus_signal(0, DCD_EVENT_UNPLUGGED, true); - } - - usb_hw_clear->sie_status = USB_SIE_STATUS_CONNECTED_BITS; + // Connected: nothing to do + }else + { + // Disconnected + dcd_event_bus_signal(0, DCD_EVENT_UNPLUGGED, true); } + + usb_hw_clear->sie_status = USB_SIE_STATUS_CONNECTED_BITS; + } #endif - // SE0 for 2.5 us or more (will last at least 10ms) - if (status & USB_INTS_BUS_RESET_BITS) - { - pico_trace("BUS RESET\n"); + // SE0 for 2.5 us or more (will last at least 10ms) + if ( status & USB_INTS_BUS_RESET_BITS ) + { + pico_trace("BUS RESET\n"); - handled |= USB_INTS_BUS_RESET_BITS; + handled |= USB_INTS_BUS_RESET_BITS; - usb_hw->dev_addr_ctrl = 0; - reset_non_control_endpoints(); - dcd_event_bus_reset(0, TUSB_SPEED_FULL, true); - usb_hw_clear->sie_status = USB_SIE_STATUS_BUS_RESET_BITS; + usb_hw->dev_addr_ctrl = 0; + reset_non_control_endpoints(); + dcd_event_bus_reset(0, TUSB_SPEED_FULL, true); + usb_hw_clear->sie_status = USB_SIE_STATUS_BUS_RESET_BITS; #if TUD_OPT_RP2040_USB_DEVICE_ENUMERATION_FIX - // Only run enumeration walk-around if pull up is enabled - if ( usb_hw->sie_ctrl & USB_SIE_CTRL_PULLUP_EN_BITS ) rp2040_usb_device_enumeration_fix(); + // Only run enumeration walk-around if pull up is enabled + if ( usb_hw->sie_ctrl & USB_SIE_CTRL_PULLUP_EN_BITS ) rp2040_usb_device_enumeration_fix(); #endif - } + } - /* Note from pico datasheet 4.1.2.6.4 (v1.2) - * If you enable the suspend interrupt, it is likely you will see a suspend interrupt when - * the device is first connected but the bus is idle. The bus can be idle for a few ms before - * the host begins sending start of frame packets. You will also see a suspend interrupt - * when the device is disconnected if you do not have a VBUS detect circuit connected. This is - * because without VBUS detection, it is impossible to tell the difference between - * being disconnected and suspended. - */ - if (status & USB_INTS_DEV_SUSPEND_BITS) - { - handled |= USB_INTS_DEV_SUSPEND_BITS; - dcd_event_bus_signal(0, DCD_EVENT_SUSPEND, true); - usb_hw_clear->sie_status = USB_SIE_STATUS_SUSPENDED_BITS; - } + /* Note from pico datasheet 4.1.2.6.4 (v1.2) + * If you enable the suspend interrupt, it is likely you will see a suspend interrupt when + * the device is first connected but the bus is idle. The bus can be idle for a few ms before + * the host begins sending start of frame packets. You will also see a suspend interrupt + * when the device is disconnected if you do not have a VBUS detect circuit connected. This is + * because without VBUS detection, it is impossible to tell the difference between + * being disconnected and suspended. + */ + if ( status & USB_INTS_DEV_SUSPEND_BITS ) + { + handled |= USB_INTS_DEV_SUSPEND_BITS; + dcd_event_bus_signal(0, DCD_EVENT_SUSPEND, true); + usb_hw_clear->sie_status = USB_SIE_STATUS_SUSPENDED_BITS; + } - if (status & USB_INTS_DEV_RESUME_FROM_HOST_BITS) - { - handled |= USB_INTS_DEV_RESUME_FROM_HOST_BITS; - dcd_event_bus_signal(0, DCD_EVENT_RESUME, true); - usb_hw_clear->sie_status = USB_SIE_STATUS_RESUME_BITS; - } + if ( status & USB_INTS_DEV_RESUME_FROM_HOST_BITS ) + { + handled |= USB_INTS_DEV_RESUME_FROM_HOST_BITS; + dcd_event_bus_signal(0, DCD_EVENT_RESUME, true); + usb_hw_clear->sie_status = USB_SIE_STATUS_RESUME_BITS; + } - if (status ^ handled) - { - panic("Unhandled IRQ 0x%x\n", (uint) (status ^ handled)); - } + if ( status ^ handled ) + { + panic("Unhandled IRQ 0x%x\n", (uint) (status ^ handled)); + } } #define USB_INTS_ERROR_BITS ( \ diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c index b7790e0c1..d25e6c8fb 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.c +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c @@ -51,37 +51,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; +// Errata 15 Walkaround for Device Bulk-In endpoint to avoid schedule an transfer +// within last 20% of an USB frame. -bool rp2040_critical_frame_period(struct hw_endpoint *ep) +volatile uint32_t e15_last_sof = 0; + +// check if Errata 15 walkround is needed for this endpoint +static bool __tusb_irq_path_func(e15_is_bulkin_ep) (struct hw_endpoint *ep) { - uint32_t delta; + return (!is_host_mode() && tu_edpt_dir(ep->ep_addr) == TUSB_DIR_IN && + ep->transfer_type == TUSB_XFER_BULK); +} - 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; - } +// check if we need to apply Errata 15 workaround: ie.g +// Enpoint is BULK IN and is currently in critical frame period i.e 20% of last usb frame +static bool __tusb_irq_path_func(e15_is_critical_frame_period) (struct hw_endpoint *ep) +{ + TU_VERIFY(e15_is_bulkin_ep(ep)); /* 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; + uint32_t delta = time_us_32() - e15_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); + TU_LOG(3, "Avoiding sof %u now %lu last %lu\n", (usb_hw->sof_rd + 1) & USB_SOF_RD_BITS, time_us_32(), e15_last_sof); return true; } -bool rp2040_ep_needs_sof(struct hw_endpoint *ep) { - return (tu_edpt_dir(ep->ep_addr) == TUSB_DIR_IN && - ep->transfer_type == TUSB_XFER_BULK); -} +#else + +#define e15_is_bulkin_ep(x) false +#define e15_is_critical_frame_period(x) false + #endif void rp2040_usb_init(void) @@ -249,17 +253,17 @@ void hw_endpoint_xfer_start(struct hw_endpoint *ep, uint8_t *buffer, uint16_t to ep->active = true; ep->user_buf = buffer; - if (rp2040_ep_needs_sof(ep)) + if ( e15_is_bulkin_ep(ep) ) { usb_hw_set->inte = USB_INTS_DEV_SOF_BITS; } - if(!rp2040_critical_frame_period(ep)) - { - hw_endpoint_start_next_buffer(ep); - } else + if ( e15_is_critical_frame_period(ep) ) { ep->pending = 1; + } else + { + hw_endpoint_start_next_buffer(ep); } hw_endpoint_lock_update(ep, -1); @@ -356,6 +360,7 @@ static void __tusb_irq_path_func(_hw_endpoint_xfer_sync) (struct hw_endpoint *ep bool __tusb_irq_path_func(hw_endpoint_xfer_continue)(struct hw_endpoint *ep) { hw_endpoint_lock_update(ep, 1); + // Part way through a transfer if (!ep->active) { @@ -377,10 +382,12 @@ bool __tusb_irq_path_func(hw_endpoint_xfer_continue)(struct hw_endpoint *ep) } else { - if(!rp2040_critical_frame_period(ep)) { - hw_endpoint_start_next_buffer(ep); - } else { + if ( e15_is_critical_frame_period(ep) ) + { ep->pending = 1; + } else + { + hw_endpoint_start_next_buffer(ep); } } diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.h b/src/portable/raspberrypi/rp2040/rp2040_usb.h index d41abfee0..a06407f23 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.h +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.h @@ -93,14 +93,8 @@ typedef struct hw_endpoint } 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); +#if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX +extern volatile uint32_t e15_last_sof; #endif void rp2040_usb_init(void); From 6759721e9aceb52f176f3068438c103739c29768 Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 31 Jan 2023 17:37:14 +0700 Subject: [PATCH 6/7] move errata to end of c file --- src/portable/raspberrypi/rp2040/rp2040_usb.c | 104 ++++++++++++------- 1 file changed, 65 insertions(+), 39 deletions(-) diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c index d25e6c8fb..6f9cf34be 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.c +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c @@ -32,6 +32,10 @@ #include #include "rp2040_usb.h" +//--------------------------------------------------------------------+ +// MACRO CONSTANT TYPEDEF PROTOTYPE +//--------------------------------------------------------------------+ + // Direction strings for debug const char *ep_dir_string[] = { "out", @@ -40,6 +44,14 @@ const char *ep_dir_string[] = { static void _hw_endpoint_xfer_sync(struct hw_endpoint *ep); +#if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX + static bool e15_is_bulkin_ep(struct hw_endpoint *ep); + static bool e15_is_critical_frame_period(struct hw_endpoint *ep); +#else + #define e15_is_bulkin_ep(x) (false) + #define e15_is_critical_frame_period(x) (false) +#endif + // if usb hardware is in host mode TU_ATTR_ALWAYS_INLINE static inline bool is_host_mode(void) { @@ -47,47 +59,9 @@ TU_ATTR_ALWAYS_INLINE static inline bool is_host_mode(void) } //--------------------------------------------------------------------+ -// +// Implementation //--------------------------------------------------------------------+ -#if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX -// Errata 15 Walkaround for Device Bulk-In endpoint to avoid schedule an transfer -// within last 20% of an USB frame. - -volatile uint32_t e15_last_sof = 0; - -// check if Errata 15 walkround is needed for this endpoint -static bool __tusb_irq_path_func(e15_is_bulkin_ep) (struct hw_endpoint *ep) -{ - return (!is_host_mode() && tu_edpt_dir(ep->ep_addr) == TUSB_DIR_IN && - ep->transfer_type == TUSB_XFER_BULK); -} - -// check if we need to apply Errata 15 workaround: ie.g -// Enpoint is BULK IN and is currently in critical frame period i.e 20% of last usb frame -static bool __tusb_irq_path_func(e15_is_critical_frame_period) (struct hw_endpoint *ep) -{ - TU_VERIFY(e15_is_bulkin_ep(ep)); - - /* 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. - */ - uint32_t delta = time_us_32() - e15_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, time_us_32(), e15_last_sof); - return true; -} - -#else - -#define e15_is_bulkin_ep(x) false -#define e15_is_critical_frame_period(x) false - -#endif - void rp2040_usb_init(void) { // Reset usb controller @@ -396,4 +370,56 @@ bool __tusb_irq_path_func(hw_endpoint_xfer_continue)(struct hw_endpoint *ep) return false; } +//--------------------------------------------------------------------+ +// Errata 15 +//--------------------------------------------------------------------+ + +#if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX + +/* 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. +*/ + +volatile uint32_t e15_last_sof = 0; + +// check if Errata 15 is needed for this endpoint i.e device bulk-in +static bool __tusb_irq_path_func(e15_is_bulkin_ep) (struct hw_endpoint *ep) +{ + return (!is_host_mode() && tu_edpt_dir(ep->ep_addr) == TUSB_DIR_IN && + ep->transfer_type == TUSB_XFER_BULK); +} + +// check if we need to apply Errata 15 walk-around : i.e +// Endpoint is BULK IN and is currently in critical frame period i.e 20% of last usb frame +static bool __tusb_irq_path_func(e15_is_critical_frame_period) (struct hw_endpoint *ep) +{ + TU_VERIFY(e15_is_bulkin_ep(ep)); + + /* 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. + */ + uint32_t delta = time_us_32() - e15_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, time_us_32(), e15_last_sof); + return true; +} + +#endif + + #endif From ddb061f639956781ac5644e5c9ec2371c534ea57 Mon Sep 17 00:00:00 2001 From: hathach Date: Tue, 31 Jan 2023 19:03:31 +0700 Subject: [PATCH 7/7] fix typos --- src/portable/raspberrypi/rp2040/dcd_rp2040.c | 4 ++-- src/portable/raspberrypi/rp2040/rp2040_usb.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/portable/raspberrypi/rp2040/dcd_rp2040.c b/src/portable/raspberrypi/rp2040/dcd_rp2040.c index ab1185102..b5fa90c92 100644 --- a/src/portable/raspberrypi/rp2040/dcd_rp2040.c +++ b/src/portable/raspberrypi/rp2040/dcd_rp2040.c @@ -254,7 +254,7 @@ static void __tusb_irq_path_func(dcd_rp2040_irq)(void) handled |= USB_INTF_DEV_SOF_BITS; #if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX - // Errata 15 Walkaround for Device Bulk-In endpoint + // Errata 15 workaround for Device Bulk-In endpoint e15_last_sof = time_us_32(); for ( uint8_t i = 0; i < USB_MAX_ENDPOINTS; i++ ) @@ -340,7 +340,7 @@ static void __tusb_irq_path_func(dcd_rp2040_irq)(void) usb_hw_clear->sie_status = USB_SIE_STATUS_BUS_RESET_BITS; #if TUD_OPT_RP2040_USB_DEVICE_ENUMERATION_FIX - // Only run enumeration walk-around if pull up is enabled + // Only run enumeration workaround if pull up is enabled if ( usb_hw->sie_ctrl & USB_SIE_CTRL_PULLUP_EN_BITS ) rp2040_usb_device_enumeration_fix(); #endif } diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c index 6f9cf34be..df05697fe 100644 --- a/src/portable/raspberrypi/rp2040/rp2040_usb.c +++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c @@ -401,7 +401,7 @@ static bool __tusb_irq_path_func(e15_is_bulkin_ep) (struct hw_endpoint *ep) ep->transfer_type == TUSB_XFER_BULK); } -// check if we need to apply Errata 15 walk-around : i.e +// check if we need to apply Errata 15 workaround : i.e // Endpoint is BULK IN and is currently in critical frame period i.e 20% of last usb frame static bool __tusb_irq_path_func(e15_is_critical_frame_period) (struct hw_endpoint *ep) {