Merge pull request #2492 from tlyu/fix-rp2040-ctrl-xfer

work around possible RP2040 erratum

4 files changed, 47 insertions, 34 deletions

diff --git a/.idea/.gitignore b/.idea/.gitignore
index 73f69e095..b0811f163 100644
--- a/.idea/.gitignore
+++ b/.idea/.gitignore
@@ -6,3 +6,5 @@
 /dataSources.local.xml
 # Editor-based HTTP Client requests
 /httpRequests/
+# GitHub Copilot persisted chat sessions
+/copilot/chatSessions
diff --git a/src/device/usbd.c b/src/device/usbd.c
index 87542e9aa..ab572e095 100644
--- a/src/device/usbd.c
+++ b/src/device/usbd.c
@@ -68,9 +68,9 @@ typedef struct {
     uint8_t remote_wakeup_support : 1; // configuration descriptor's attribute
     uint8_t self_powered          : 1; // configuration descriptor's attribute
   };
-
   volatile uint8_t cfg_num; // current active configuration (0x00 is not configured)
   uint8_t speed;
+  volatile uint8_t setup_count;
 
   uint8_t itf2drv[CFG_TUD_INTERFACE_MAX];   // map interface number to driver (0xff is invalid)
   uint8_t ep2drv[CFG_TUD_ENDPPOINT_MAX][2]; // map endpoint to driver ( 0xff is invalid ), can use only 4-bit each
@@ -378,6 +378,7 @@ bool tud_init(uint8_t rhport) {
 
   TU_LOG_USBD("USBD init on controller %u\r\n", rhport);
   TU_LOG_INT(CFG_TUD_LOG_LEVEL, sizeof(usbd_device_t));
+  TU_LOG_INT(CFG_TUD_LOG_LEVEL, sizeof(dcd_event_t));
   TU_LOG_INT(CFG_TUD_LOG_LEVEL, sizeof(tu_fifo_t));
   TU_LOG_INT(CFG_TUD_LOG_LEVEL, sizeof(tu_edpt_stream_t));
 
@@ -482,7 +483,12 @@ void tud_task_ext(uint32_t timeout_ms, bool in_isr) {
         break;
 
       case DCD_EVENT_SETUP_RECEIVED:
+        _usbd_dev.setup_count--;
         TU_LOG_BUF(CFG_TUD_LOG_LEVEL, &event.setup_received, 8);
+        if (_usbd_dev.setup_count) {
+          TU_LOG_USBD("  Skipped since there is other SETUP in queue\r\n");
+          break;
+        }
 
         // Mark as connected after receiving 1st setup packet.
         // But it is easier to set it every time instead of wasting time to check then set
@@ -1063,6 +1069,11 @@ TU_ATTR_FAST_FUNC void dcd_event_handler(dcd_event_t const* event, bool in_isr)
       // skip osal queue for SOF in usbd task
       break;
 
+    case DCD_EVENT_SETUP_RECEIVED:
+      _usbd_dev.setup_count++;
+      send = true;
+      break;
+
     default:
       send = true;
       break;
diff --git a/src/portable/raspberrypi/rp2040/dcd_rp2040.c b/src/portable/raspberrypi/rp2040/dcd_rp2040.c
index ba7e9f912..5c564cb1c 100644
--- a/src/portable/raspberrypi/rp2040/dcd_rp2040.c
+++ b/src/portable/raspberrypi/rp2040/dcd_rp2040.c
@@ -60,7 +60,7 @@ TU_ATTR_ALWAYS_INLINE static inline struct hw_endpoint* hw_endpoint_get_by_num(u
   return &hw_endpoints[num][dir];
 }
 
-static struct hw_endpoint* hw_endpoint_get_by_addr(uint8_t ep_addr) {
+TU_ATTR_ALWAYS_INLINE static inline struct hw_endpoint* hw_endpoint_get_by_addr(uint8_t ep_addr) {
   uint8_t num = tu_edpt_number(ep_addr);
   tusb_dir_t dir = tu_edpt_dir(ep_addr);
   return hw_endpoint_get_by_num(num, dir);
@@ -192,12 +192,29 @@ static void __tusb_irq_path_func(hw_handle_buff_status)(void) {
   }
 }
 
-TU_ATTR_ALWAYS_INLINE static inline void reset_ep0_pid(void) {
+TU_ATTR_ALWAYS_INLINE static inline void reset_ep0(void) {
   // If we have finished this transfer on EP0 set pid back to 1 for next
   // setup transfer. Also clear a stall in case
-  uint8_t addrs[] = {0x0, 0x80};
-  for (uint i = 0; i < TU_ARRAY_SIZE(addrs); i++) {
-    struct hw_endpoint* ep = hw_endpoint_get_by_addr(addrs[i]);
+  for (uint8_t dir = 0; dir < 2; dir++) {
+    struct hw_endpoint* ep = hw_endpoint_get_by_num(0, dir);
+    if (ep->active) {
+      // Abort any pending transfer from a prior control transfer per USB specs
+      // Due to Errata RP2040-E2: ABORT flag is only applicable for B2 and later (unusable for B0, B1).
+      // Which means we are not guaranteed to safely abort pending transfer on B0 and B1.
+      uint32_t const abort_mask = (dir ? USB_EP_ABORT_EP0_IN_BITS : USB_EP_ABORT_EP0_OUT_BITS);
+      if (rp2040_chip_version() >= 2) {
+        usb_hw_set->abort = abort_mask;
+        while ((usb_hw->abort_done & abort_mask) != abort_mask) {}
+      }
+
+      _hw_endpoint_buffer_control_set_value32(ep, USB_BUF_CTRL_DATA1_PID | USB_BUF_CTRL_SEL);
+      hw_endpoint_reset_transfer(ep);
+
+      if (rp2040_chip_version() >= 2) {
+        usb_hw_clear->abort_done = abort_mask;
+        usb_hw_clear->abort = abort_mask;
+      }
+    }
     ep->next_pid = 1u;
   }
 }
@@ -267,7 +284,7 @@ static void __tusb_irq_path_func(dcd_rp2040_irq)(void) {
     uint8_t const* setup = remove_volatile_cast(uint8_t const*, &usb_dpram->setup_packet);
 
     // reset pid to both 1 (data and ack)
-    reset_ep0_pid();
+    reset_ep0();
 
     // Pass setup packet to tiny usb
     dcd_event_setup_received(0, setup, true);
@@ -355,6 +372,8 @@ static void __tusb_irq_path_func(dcd_rp2040_irq)(void) {
 void dcd_init(uint8_t rhport) {
   assert(rhport == 0);
 
+  TU_LOG(2, "Chip Version B%u\r\n", rp2040_chip_version());
+
   // Reset hardware to default state
   rp2040_usb_init();
 
diff --git a/src/portable/raspberrypi/rp2040/rp2040_usb.c b/src/portable/raspberrypi/rp2040/rp2040_usb.c
index b5bace972..1ca711c77 100644
--- a/src/portable/raspberrypi/rp2040/rp2040_usb.c
+++ b/src/portable/raspberrypi/rp2040/rp2040_usb.c
@@ -35,13 +35,6 @@
 //--------------------------------------------------------------------+
 // MACRO CONSTANT TYPEDEF PROTOTYPE
 //--------------------------------------------------------------------+
-
-// Direction strings for debug
-const char* ep_dir_string[] = {
-    "out",
-    "in",
-};
-
 static void _hw_endpoint_xfer_sync(struct hw_endpoint* ep);
 
 #if TUD_OPT_RP2040_USB_DEVICE_UFRAME_FIX
@@ -105,22 +98,14 @@ void __tusb_irq_path_func(_hw_endpoint_buffer_control_update32)(struct hw_endpoi
     value |= or_mask;
     if (or_mask & USB_BUF_CTRL_AVAIL) {
       if (*ep->buffer_control & USB_BUF_CTRL_AVAIL) {
-        panic("ep %d %s was already available", tu_edpt_number(ep->ep_addr), ep_dir_string[tu_edpt_dir(ep->ep_addr)]);
+        panic("ep %02X was already available", ep->ep_addr);
       }
       *ep->buffer_control = value & ~USB_BUF_CTRL_AVAIL;
-      // 12 cycle delay.. (should be good for 48*12Mhz = 576Mhz)
+      // 4.1.2.5.1 Con-current access: 12 cycles (should be good for 48*12Mhz = 576Mhz) after write to buffer control
       // Don't need delay in host mode as host is in charge
-#if !CFG_TUH_ENABLED
-      __asm volatile (
-          "b 1f\n"
-          "1: b 1f\n"
-          "1: b 1f\n"
-          "1: b 1f\n"
-          "1: b 1f\n"
-          "1: b 1f\n"
-          "1:\n"
-          : : : "memory");
-#endif
+      if ( !is_host_mode()) {
+        busy_wait_at_least_cycles(12);
+      }
     }
   }
 
@@ -204,9 +189,7 @@ void hw_endpoint_xfer_start(struct hw_endpoint* ep, uint8_t* buffer, uint16_t to
 
   if (ep->active) {
     // TODO: Is this acceptable for interrupt packets?
-    TU_LOG(1, "WARN: starting new transfer on already active ep %d %s\r\n", tu_edpt_number(ep->ep_addr),
-           ep_dir_string[tu_edpt_dir(ep->ep_addr)]);
-
+    TU_LOG(1, "WARN: starting new transfer on already active ep %02X\r\n", ep->ep_addr);
     hw_endpoint_reset_transfer(ep);
   }
 
@@ -314,8 +297,7 @@ bool __tusb_irq_path_func(hw_endpoint_xfer_continue)(struct hw_endpoint* ep) {
 
   // Part way through a transfer
   if (!ep->active) {
-    panic("Can't continue xfer on inactive ep %d %s", tu_edpt_number(ep->ep_addr),
-          ep_dir_string[tu_edpt_dir(ep->ep_addr)]);
+    panic("Can't continue xfer on inactive ep %02X", ep->ep_addr);
   }
 
   // Update EP struct from hardware state
@@ -324,8 +306,7 @@ bool __tusb_irq_path_func(hw_endpoint_xfer_continue)(struct hw_endpoint* ep) {
   // Now we have synced our state with the hardware. Is there more data to transfer?
   // If we are done then notify tinyusb
   if (ep->remaining_len == 0) {
-    pico_trace("Completed transfer of %d bytes on ep %d %s\r\n",
-               ep->xferred_len, tu_edpt_number(ep->ep_addr), ep_dir_string[tu_edpt_dir(ep->ep_addr)]);
+    pico_trace("Completed transfer of %d bytes on ep %02X\r\n", ep->xferred_len, ep->ep_addr);
     // Notify caller we are done so it can notify the tinyusb stack
     hw_endpoint_lock_update(ep, -1);
     return true;