From cbc92bd389856de3de8224fbdf6413e6ce61b001 Mon Sep 17 00:00:00 2001 From: hathach Date: Mon, 26 Aug 2024 12:24:29 +0700 Subject: [PATCH] add sndfifo owner info to skip rewriting data for retrying NAKed --- hw/bsp/family_support.cmake | 8 +- src/host/usbh.h | 2 +- src/portable/analog/max3421/hcd_max3421.c | 191 ++++++++++++++-------- 3 files changed, 127 insertions(+), 74 deletions(-) diff --git a/hw/bsp/family_support.cmake b/hw/bsp/family_support.cmake index b457fcfac6..02ebe1e735 100644 --- a/hw/bsp/family_support.cmake +++ b/hw/bsp/family_support.cmake @@ -236,7 +236,7 @@ function(family_configure_common TARGET RTOS) if (NOT TARGET segger_rtt) add_library(segger_rtt STATIC ${TOP}/lib/SEGGER_RTT/RTT/SEGGER_RTT.c) target_include_directories(segger_rtt PUBLIC ${TOP}/lib/SEGGER_RTT/RTT) -# target_compile_definitions(segger_rtt PUBLIC SEGGER_RTT_MODE_DEFAULT=SEGGER_RTT_MODE_BLOCK_IF_FIFO_FULL) + target_compile_definitions(segger_rtt PUBLIC SEGGER_RTT_MODE_DEFAULT=SEGGER_RTT_MODE_BLOCK_IF_FIFO_FULL) endif() target_link_libraries(${TARGET} PUBLIC segger_rtt) endif () @@ -418,6 +418,12 @@ exit" COMMAND ${JLINKEXE} -device ${JLINK_DEVICE} ${OPTION_LIST} -if ${JLINK_IF} -JTAGConf -1,-1 -speed auto -CommandFile $/${TARGET}.jlink VERBATIM ) + + # optional flash post build +# add_custom_command(TARGET ${TARGET} POST_BUILD +# COMMAND ${JLINKEXE} -device ${JLINK_DEVICE} ${OPTION_LIST} -if ${JLINK_IF} -JTAGConf -1,-1 -speed auto -CommandFile $/${TARGET}.jlink +# VERBATIM +# ) endfunction() diff --git a/src/host/usbh.h b/src/host/usbh.h index 359684169e..d0a5732e55 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 3d054b66e2..214b7c586f 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, ®, &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, ®, &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, uint16_t len, bool in_isr) { + uint8_t hirq; + reg |= CMDBYTE_WRITE; + + max3421_spi_lock(rhport, in_isr); + + tuh_max3421_spi_xfer_api(rhport, ®, &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, uint16_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, ®, &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; }