add sndfifo owner info to skip rewriting data for retrying NAKed

2 files changed, 120 insertions, 73 deletions

diff --git a/src/host/usbh.h b/src/host/usbh.h
index 359684169..d0a5732e5 100644
--- a/src/host/usbh.h
+++ b/src/host/usbh.h
@@ -81,7 +81,7 @@ enum {
 };
 
 typedef struct {
-  uint8_t max_nak; // max NAK per endpoint per frame
+  uint8_t max_nak; // max NAK per endpoint per frame to save CPU/SPI bus usage
   uint8_t cpuctl; // R16: CPU Control Register
   uint8_t pinctl; // R17: Pin Control Register. FDUPSPI bit is ignored
 } tuh_configure_max3421_t;
diff --git a/src/portable/analog/max3421/hcd_max3421.c b/src/portable/analog/max3421/hcd_max3421.c
index 3d054b66e..215feb481 100644
--- a/src/portable/analog/max3421/hcd_max3421.c
+++ b/src/portable/analog/max3421/hcd_max3421.c
@@ -168,14 +168,14 @@ enum {
 };
 
 enum {
-  MAX_NAK_DEFAULT = 1 // Number of NAK per endpoint per usb frame
+  MAX_NAK_DEFAULT = 1 // Number of NAK per endpoint per usb frame to save CPU/SPI bus usage
 };
 
 enum {
   EP_STATE_IDLE        = 0,
   EP_STATE_COMPLETE    = 1,
   EP_STATE_ABORTING    = 2,
-  EP_STATE_ATTEMPT_1   = 3, // pending 1st attempt
+  EP_STATE_ATTEMPT_1   = 3, // Number of attempts to transfer in a frame. Incremented after each NAK
   EP_STATE_ATTEMPT_MAX = 15
 };
 
@@ -183,17 +183,20 @@ enum {
 //
 //--------------------------------------------------------------------+
 
+typedef struct TU_ATTR_PACKED {
+  uint8_t ep_num   : 4;
+  uint8_t is_setup : 1;
+  uint8_t is_out   : 1;
+  uint8_t is_iso   : 1;
+} hxfr_bm_t;
+
+TU_VERIFY_STATIC(sizeof(hxfr_bm_t) == 1, "size is not correct");
+
 typedef struct {
   uint8_t daddr;
 
-  union { ;
-    struct TU_ATTR_PACKED {
-      uint8_t ep_num   : 4;
-      uint8_t is_setup : 1;
-      uint8_t is_out   : 1;
-      uint8_t is_iso   : 1;
-    }hxfr_bm;
-
+  union {
+    hxfr_bm_t hxfr_bm;
     uint8_t hxfr;
   };
 
@@ -203,13 +206,12 @@ typedef struct {
     uint16_t packet_size : 11;
   };
 
-  bool received_nak;
   uint16_t total_len;
   uint16_t xferred_len;
   uint8_t* buf;
 } max3421_ep_t;
 
-TU_VERIFY_STATIC(sizeof(max3421_ep_t) == 16, "size is not correct");
+TU_VERIFY_STATIC(sizeof(max3421_ep_t) == 12, "size is not correct");
 
 typedef struct {
   volatile uint16_t frame_count;
@@ -220,7 +222,16 @@ typedef struct {
   uint8_t hien;
   uint8_t mode;
   uint8_t peraddr;
-  uint8_t hxfr;
+  union {
+    hxfr_bm_t hxfr_bm;
+    uint8_t hxfr;
+  };
+
+  // owner of data in SNDFIFO, for retrying NAKed without re-writing to FIFO
+  struct {
+    uint8_t daddr;
+    uint8_t hxfr;
+  }sndfifo_owner;
 
   atomic_flag busy; // busy transferring
 
@@ -318,33 +329,9 @@ bool tuh_max3421_reg_write(uint8_t rhport, uint8_t reg, uint8_t data, bool in_is
   return ret;
 }
 
-static void fifo_write(uint8_t rhport, uint8_t reg, uint8_t const * buffer, uint16_t len, bool in_isr) {
-  uint8_t hirq;
-  reg |= CMDBYTE_WRITE;
-
-  max3421_spi_lock(rhport, in_isr);
-
-  tuh_max3421_spi_xfer_api(rhport, &reg, &hirq, 1);
-  _hcd_data.hirq = hirq;
-  tuh_max3421_spi_xfer_api(rhport, buffer, NULL, len);
-
-  max3421_spi_unlock(rhport, in_isr);
-}
-
-static void fifo_read(uint8_t rhport, uint8_t * buffer, uint16_t len, bool in_isr) {
-  uint8_t hirq;
-  uint8_t const reg = RCVVFIFO_ADDR;
-
-  max3421_spi_lock(rhport, in_isr);
-
-  tuh_max3421_spi_xfer_api(rhport, &reg, &hirq, 1);
-  _hcd_data.hirq = hirq;
-  tuh_max3421_spi_xfer_api(rhport, NULL, buffer, len);
-
-  max3421_spi_unlock(rhport, in_isr);
-}
-
-//------------- register write helper -------------//
+//--------------------------------------------------------------------
+// Register helper
+//--------------------------------------------------------------------
 TU_ATTR_ALWAYS_INLINE static inline void hirq_write(uint8_t rhport, uint8_t data, bool in_isr) {
   reg_write(rhport, HIRQ_ADDR, data, in_isr);
   // HIRQ write 1 is clear
@@ -378,6 +365,47 @@ TU_ATTR_ALWAYS_INLINE static inline void sndbc_write(uint8_t rhport, uint8_t dat
   reg_write(rhport, SNDBC_ADDR, data, in_isr);
 }
 
+//--------------------------------------------------------------------
+// FIFO access (receive, send, setup)
+//--------------------------------------------------------------------
+static void hwfifo_write(uint8_t rhport, uint8_t reg, const uint8_t* buffer, uint8_t len, bool in_isr) {
+  uint8_t hirq;
+  reg |= CMDBYTE_WRITE;
+
+  max3421_spi_lock(rhport, in_isr);
+
+  tuh_max3421_spi_xfer_api(rhport, &reg, &hirq, 1);
+  _hcd_data.hirq = hirq;
+  tuh_max3421_spi_xfer_api(rhport, buffer, NULL, len);
+
+  max3421_spi_unlock(rhport, in_isr);
+}
+
+// Write to SNDFIFO if len > 0 and update SNDBC
+TU_ATTR_ALWAYS_INLINE static inline void hwfifo_send(uint8_t rhport, const uint8_t* buffer, uint8_t len, bool in_isr) {
+  if (len) {
+    hwfifo_write(rhport, SNDFIFO_ADDR, buffer, len, in_isr);
+  }
+  sndbc_write(rhport, len, in_isr);
+}
+
+TU_ATTR_ALWAYS_INLINE static inline void hwfifo_setup(uint8_t rhport, const uint8_t* buffer, bool in_isr) {
+  hwfifo_write(rhport, SUDFIFO_ADDR, buffer, 8, in_isr);
+}
+
+static void hwfifo_receive(uint8_t rhport, uint8_t * buffer, uint16_t len, bool in_isr) {
+  uint8_t hirq;
+  uint8_t const reg = RCVVFIFO_ADDR;
+
+  max3421_spi_lock(rhport, in_isr);
+
+  tuh_max3421_spi_xfer_api(rhport, &reg, &hirq, 1);
+  _hcd_data.hirq = hirq;
+  tuh_max3421_spi_xfer_api(rhport, NULL, buffer, len);
+
+  max3421_spi_unlock(rhport, in_isr);
+}
+
 //--------------------------------------------------------------------+
 // Endpoint helper
 //--------------------------------------------------------------------+
@@ -418,7 +446,7 @@ static void free_ep(uint8_t daddr) {
   }
 }
 
-// Check if endpoint has an queued transfer and not reach max NAK
+// Check if endpoint has a queued transfer and not reach max NAK in this frame
 TU_ATTR_ALWAYS_INLINE static inline bool is_ep_pending(max3421_ep_t const * ep) {
   uint8_t const state = ep->state;
   return ep->packet_size && (state >= EP_STATE_ATTEMPT_1) &&
@@ -488,6 +516,7 @@ bool hcd_init(uint8_t rhport) {
   reg_write(rhport, PINCTL_ADDR, _tuh_cfg.pinctl | PINCTL_FDUPSPI, false);
 
   // v1 is 0x01, v2 is 0x12, v3 is 0x13
+  // Note: v1 and v2 has host OUT errata whose workaround is not implemented in this driver
   uint8_t const revision = reg_read(rhport, REVISION_ADDR, false);
   TU_LOG2_HEX(revision);
   TU_ASSERT(revision == 0x01 || revision == 0x12 || revision == 0x13, false);
@@ -616,27 +645,44 @@ bool hcd_edpt_open(uint8_t rhport, uint8_t daddr, tusb_desc_endpoint_t const * e
   return true;
 }
 
+/* The microcontroller repeatedly writes the SNDFIFO register R2 to load the FIFO with up to 64 data bytes.
+ * Then the microcontroller writes the SNDBC register, which this does three things:
+ * 1. Tells the MAX3421E SIE (Serial Interface Engine) how many bytes in the FIFO to send.
+ * 2. Connects the SNDFIFO and SNDBC register to the USB logic for USB transmission.
+ * 3. Clears the SNDBAVIRQ interrupt flag. If the second FIFO is available for µC loading, the SNDBAVIRQ immediately re-asserts.
+
+                                               +-----------+
+                                           --->| SNDBC-A   |
+                                          /    | SNDFIFO-A |
+                                         /     +-----------+
+      +------+       +-------------+    /                              +----------+
+      | MCU  |------>| R2: SNDFIFO |----     << Write R7 Flip >>    ---| MAX3241E |
+      |(hcd) |       | R7: SNDBC   |                               /   |   SIE    |
+      +------+       +-------------+                              /    +----------+
+                                              +-----------+      /
+                                               | SNDBC-B   |    /
+                                               | SNDFIFO-B |<---
+                                               +-----------+
+  Note: xact_out() is called when starting a new transfer, continue a transfer (isr) or retry a transfer (NAK)
+        For NAK retry, we do not need to write to FIFO or SNDBC register again.
+*/
 static void xact_out(uint8_t rhport, max3421_ep_t *ep, bool switch_ep, bool in_isr) {
   // Page 12: Programming BULK-OUT Transfers
-  // TODO double buffered
+  // TODO: double buffering for ISO transfer
   if (switch_ep) {
     peraddr_write(rhport, ep->daddr, in_isr);
-
-    uint8_t const hctl = (ep->data_toggle ? HCTL_SNDTOG1 : HCTL_SNDTOG0);
+    const uint8_t hctl = (ep->data_toggle ? HCTL_SNDTOG1 : HCTL_SNDTOG0);
     reg_write(rhport, HCTL_ADDR, hctl, in_isr);
   }
 
-  if (!ep->received_nak){
-    // do not write to fifo or sdnbc register again if previous attempt got NAK
-    uint8_t const xact_len = (uint8_t) tu_min16(ep->total_len - ep->xferred_len, ep->packet_size);
-    if (xact_len) {
-      // only check SNDBAV IRQ if there is data to send
-      TU_ASSERT(_hcd_data.hirq & HIRQ_SNDBAV_IRQ,);
-      fifo_write(rhport, SNDFIFO_ADDR, ep->buf, xact_len, in_isr);
-    }
-
-    sndbc_write(rhport, xact_len, in_isr);
+  // Only write to sndfifo and sdnbc register if it is not a NAKed retry
+  if (!(ep->daddr == _hcd_data.sndfifo_owner.daddr && ep->hxfr == _hcd_data.sndfifo_owner.hxfr)) {
+    // skip SNDBAV IRQ check, overwrite sndfifo if needed
+    const uint8_t xact_len = (uint8_t) tu_min16(ep->total_len - ep->xferred_len, ep->packet_size);
+    hwfifo_send(rhport, ep->buf, xact_len, in_isr);
   }
+  _hcd_data.sndfifo_owner.daddr = ep->daddr;
+  _hcd_data.sndfifo_owner.hxfr = ep->hxfr;
 
   hxfr_write(rhport, ep->hxfr, in_isr);
 }
@@ -655,7 +701,7 @@ static void xact_in(uint8_t rhport, max3421_ep_t *ep, bool switch_ep, bool in_is
 
 static void xact_setup(uint8_t rhport, max3421_ep_t *ep, bool in_isr) {
   peraddr_write(rhport, ep->daddr, in_isr);
-  fifo_write(rhport, SUDFIFO_ADDR, ep->buf, 8, in_isr);
+  hwfifo_setup(rhport, ep->buf, in_isr);
   hxfr_write(rhport, HXFR_SETUP, in_isr);
 }
 
@@ -669,7 +715,7 @@ static void xact_generic(uint8_t rhport, max3421_ep_t *ep, bool switch_ep, bool
 
     // status
     if (ep->buf == NULL || ep->total_len == 0) {
-      uint8_t const hxfr = (uint8_t) (HXFR_HS | (ep->hxfr & HXFR_OUT_NIN));
+      const uint8_t hxfr = (uint8_t) (HXFR_HS | (ep->hxfr & HXFR_OUT_NIN));
       peraddr_write(rhport, ep->daddr, in_isr);
       hxfr_write(rhport, hxfr, in_isr);
       return;
@@ -830,11 +876,11 @@ static void xfer_complete_isr(uint8_t rhport, max3421_ep_t *ep, xfer_result_t re
 }
 
 static void handle_xfer_done(uint8_t rhport, bool in_isr) {
-  uint8_t const hrsl = reg_read(rhport, HRSL_ADDR, in_isr);
-  uint8_t const hresult = hrsl & HRSL_RESULT_MASK;
-  uint8_t const ep_num = _hcd_data.hxfr & HXFR_EPNUM_MASK;
-  uint8_t const hxfr_type = _hcd_data.hxfr & 0xf0;
-  uint8_t const ep_dir = ((hxfr_type & HXFR_SETUP) || (hxfr_type & HXFR_OUT_NIN)) ? 0 : 1;
+  const uint8_t hrsl = reg_read(rhport, HRSL_ADDR, in_isr);
+  const uint8_t hresult = hrsl & HRSL_RESULT_MASK;
+  const uint8_t ep_num = _hcd_data.hxfr_bm.ep_num;
+  const uint8_t hxfr_type = _hcd_data.hxfr & 0xf0;
+  const uint8_t ep_dir = ((hxfr_type & HXFR_SETUP) || (hxfr_type & HXFR_OUT_NIN)) ? 0 : 1;
 
   max3421_ep_t *ep = find_opened_ep(_hcd_data.peraddr, ep_num, ep_dir);
   TU_VERIFY(ep, );
@@ -849,9 +895,9 @@ static void handle_xfer_done(uint8_t rhport, bool in_isr) {
           // control endpoint -> retry immediately and return
           hxfr_write(rhport, _hcd_data.hxfr, in_isr);
           return;
-        } else if (EP_STATE_ATTEMPT_1 <= ep->state && ep->state < EP_STATE_ATTEMPT_MAX) {
+        }
+        if (EP_STATE_ATTEMPT_1 <= ep->state && ep->state < EP_STATE_ATTEMPT_MAX) {
           ep->state++;
-          ep->received_nak = true;
         }
       }
 
@@ -861,7 +907,6 @@ static void handle_xfer_done(uint8_t rhport, bool in_isr) {
         hxfr_write(rhport, _hcd_data.hxfr, in_isr);
       } else if (next_ep) {
         // switch to next pending endpoint
-        // TODO could have issue with double buffered if not clear previously out data
         xact_generic(rhport, next_ep, true, in_isr);
       } else {
         // no more pending in this frame -> clear busy
@@ -875,7 +920,6 @@ static void handle_xfer_done(uint8_t rhport, bool in_isr) {
 
     case HRSL_SUCCESS:
       xfer_result = XFER_RESULT_SUCCESS;
-      ep->received_nak = false;
       break;
 
     case HRSL_STALL:
@@ -905,11 +949,15 @@ static void handle_xfer_done(uint8_t rhport, bool in_isr) {
     if (ep->state == EP_STATE_COMPLETE) {
       xfer_complete_isr(rhport, ep, xfer_result, hrsl, in_isr);
     }else {
-      // more to transfer
-      hxfr_write(rhport, _hcd_data.hxfr, in_isr);
+      hxfr_write(rhport, _hcd_data.hxfr, in_isr); // more to transfer
     }
   } else {
     // SETUP or OUT transfer
+
+    // clear sndfifo owner since data is sent
+    _hcd_data.sndfifo_owner.daddr = 0xff;
+    _hcd_data.sndfifo_owner.hxfr = 0xff;
+
     uint8_t xact_len;
 
     if (hxfr_type & HXFR_SETUP) {
@@ -926,8 +974,7 @@ static void handle_xfer_done(uint8_t rhport, bool in_isr) {
     if (xact_len < ep->packet_size || ep->xferred_len >= ep->total_len) {
       xfer_complete_isr(rhport, ep, xfer_result, hrsl, in_isr);
     } else {
-      // more to transfer
-      xact_out(rhport, ep, false, in_isr);
+      xact_out(rhport, ep, false, in_isr); // more to transfer
     }
   }
 }
@@ -987,16 +1034,16 @@ void hcd_int_handler(uint8_t rhport, bool in_isr) {
   // not call this handler again. So we need to loop until all IRQ are cleared
   while (hirq & (HIRQ_RCVDAV_IRQ | HIRQ_HXFRDN_IRQ)) {
     if (hirq & HIRQ_RCVDAV_IRQ) {
-      uint8_t const ep_num = _hcd_data.hxfr & HXFR_EPNUM_MASK;
+      const uint8_t ep_num = _hcd_data.hxfr_bm.ep_num;
       max3421_ep_t* ep = find_opened_ep(_hcd_data.peraddr, ep_num, 1);
       uint8_t xact_len = 0;
 
       // RCVDAV_IRQ can trigger 2 times (dual buffered)
       while (hirq & HIRQ_RCVDAV_IRQ) {
-        uint8_t rcvbc = reg_read(rhport, RCVBC_ADDR, in_isr);
+        const uint8_t rcvbc = reg_read(rhport, RCVBC_ADDR, in_isr);
         xact_len = (uint8_t) tu_min16(rcvbc, ep->total_len - ep->xferred_len);
         if (xact_len) {
-          fifo_read(rhport, ep->buf, xact_len, in_isr);
+          hwfifo_receive(rhport, ep->buf, xact_len, in_isr);
           ep->buf += xact_len;
           ep->xferred_len += xact_len;
         }