From 36cd9f9f46ca20be907ed57b874d9d1dc7b3bf64 Mon Sep 17 00:00:00 2001 From: hathach Date: Thu, 16 Jul 2026 14:11:01 +0700 Subject: dcd_lpc17_40: fix stale EP0 out_received, add isochronous support EP0 control-OUT fix (usbtest 14/21, errno 110/-74): usbd queues the status-stage OUT ZLP of every control read with buffer=NULL, so the ISR's `if (out_buffer)` check missed it and marked the arriving ZLP as out_received instead. The stale flag poisoned the next control-OUT with data: its first chunk "completed" instantly from an empty EP0 buffer and the host's real DATA NAKed forever. Track queued transfers with an explicit out_queued flag and void half-finished control state on a new SETUP. Isochronous support (UM10562 12.15.6): 5-word DMA descriptors with per-packet size memory, buflen/present_count in packets, one packet per FRAME (no DMARSet/EpIntEn involvement), completion at EOT for both directions. Details that matter: - the iso machinery (5th DD word + packet-size memory) is compiled only when an iso-capable class is enabled (CFG_TUD_AUDIO/VIDEO/VENDOR), so non-iso builds pay nothing: _dcd stays 648 B vs 1032 B with iso - ISR dispatch keys on the hardware's fixed ep-number/type map (ep_id_is_iso), never on dd fields that thread mode rebuilds - iso OUT honors Packet_valid (bit 16) and prefills the hardware writeback slots with 0, so a missed frame counts as 0 bytes instead of reading back stale buffer contents as data - packet count is validated (tu_div_ceil <= ISO_MAX_PACKETS) before the DD is touched, so an oversized transfer is refused without leaving a serviceable half-built descriptor armed for the frame engine - dcd_edpt_iso_alloc and iso_activate both enforce the fixed iso endpoint numbers (3/6/9/12); classes ignore alloc's return value, so activate must not trust it Un-skip LPC40XX in the usbtest example; tier 4 now enumerates and passes iso cases 15/16/22/23. cdc_msc_throughput and printer_to_cdc had bulk on iso-only EP3 (SET_CONFIGURATION failed with -32); add the LPC17/40 EPNUM block (bulk on EP2/EP5) like other fixed-EP examples. Verified on ea4088_quickstart: usbtest tier-4 battery 30/30 repeatedly and the full device HIL suite 14/14 (incl. audio_test iso). --- src/portable/nxp/lpc17_40/dcd_lpc17_40.c | 211 +++++++++++++++++++++++++++---- 1 file changed, 184 insertions(+), 27 deletions(-) (limited to 'src') diff --git a/src/portable/nxp/lpc17_40/dcd_lpc17_40.c b/src/portable/nxp/lpc17_40/dcd_lpc17_40.c index 182710016..a1a44e9ae 100644 --- a/src/portable/nxp/lpc17_40/dcd_lpc17_40.c +++ b/src/portable/nxp/lpc17_40/dcd_lpc17_40.c @@ -19,6 +19,10 @@ //--------------------------------------------------------------------+ #define DCD_ENDPOINT_MAX 32 +// The iso machinery (5th DD word + packet-size memory) costs USB RAM on every build; +// compile it only when a class that can open an iso endpoint is enabled. +#define DCD_ISO_ENABLED (CFG_TUD_AUDIO || CFG_TUD_VIDEO || CFG_TUD_VENDOR) + typedef struct TU_ATTR_ALIGNED(4) { //------------- Word 0 -------------// @@ -48,11 +52,35 @@ typedef struct TU_ATTR_ALIGNED(4) volatile uint16_t present_count; // For non-iso : The number of bytes transferred by the DMA engine // For iso : number of packets +#if DCD_ISO_ENABLED //------------- Word 4 -------------// - // uint32_t iso_packet_size_addr; // iso only, can be omitted for non-iso + volatile uint32_t iso_packet_size_addr; // iso only: pointer into iso packet-size memory, + // advanced by hardware after each packet +#endif }dma_desc_t; -TU_VERIFY_STATIC( sizeof(dma_desc_t) == 16, "size is not correct"); // TODO not support ISO for now +TU_VERIFY_STATIC( sizeof(dma_desc_t) == (DCD_ISO_ENABLED ? 20 : 16), "size is not correct"); + +// Hardware fixes endpoint type by number: 3, 6, 9, 12 are the iso-capable ones. +// Constant per ep_id (= 2*epnum + dir) — unlike dd->isochronous, which dcd_edpt_xfer +// transiently zeroes while rebuilding the DD, this is safe to dispatch on from the ISR. +TU_ATTR_ALWAYS_INLINE static inline bool ep_id_is_iso(uint8_t ep_id) { + uint8_t const epnum = (uint8_t)(ep_id >> 1); + return (epnum % 3) == 0 && (epnum != 0) && (epnum != 15); +} + +#if DCD_ISO_ENABLED +// Isochronous packet-size memory (UM10562 12.15.6.3): one word per packet. +// IN : software fills Packet_length (bits 15:0), 0 = ZLP +// OUT: hardware writes Frame_number (31:17) | Packet_valid (16) | Packet_length (15:0) +// Iso-capable endpoint numbers are 3, 6, 9, 12 -> 8 slots (x2 directions). +// One packet moves per FRAME, so a deep queue only adds latency: 8 frames is plenty. +#define ISO_MAX_PACKETS 8 +#define ISO_SLOT_COUNT 8 +TU_ATTR_ALWAYS_INLINE static inline uint8_t iso_slot(uint8_t ep_id) { + return (uint8_t)(((ep_id / 6) - 1) * 2 + (ep_id & 1)); // ep_id = 2*epnum + dir, epnum in {3,6,9,12} +} +#endif typedef struct { @@ -66,11 +94,17 @@ typedef struct { uint8_t* out_buffer; uint8_t out_bytes; + volatile bool out_queued; // an OUT xfer is queued; out_buffer may legitimately be NULL (status ZLP) volatile bool out_received; // indicate if data is already received in endpoint uint8_t in_bytes; } control; +#if DCD_ISO_ENABLED + // iso packet-size memory, must be DMA-reachable like the DDs + volatile uint32_t iso_psize[ISO_SLOT_COUNT][ISO_MAX_PACKETS]; +#endif + } dcd_data_t; CFG_TUD_MEM_SECTION TU_ATTR_ALIGNED(128) static dcd_data_t _dcd; @@ -79,6 +113,7 @@ CFG_TUD_MEM_SECTION TU_ATTR_ALIGNED(128) static dcd_data_t _dcd; //--------------------------------------------------------------------+ // SIE Command //--------------------------------------------------------------------+ + static void sie_cmd_code (sie_cmdphase_t phase, uint8_t code_data) { LPC_USB->DevIntClr = (DEV_INT_COMMAND_CODE_EMPTY_MASK | DEV_INT_COMMAND_DATA_FULL_MASK); @@ -294,7 +329,8 @@ bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc) break; case TUSB_XFER_ISOCHRONOUS: - TU_ASSERT((epnum % 3) == 0 && (epnum != 0) && (epnum != 15)); + // iso machinery is compiled out when no iso-capable class is enabled + TU_ASSERT(DCD_ISO_ENABLED && (epnum % 3) == 0 && (epnum != 0) && (epnum != 15)); break; default: @@ -319,16 +355,54 @@ bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc) } bool dcd_edpt_iso_alloc(uint8_t rhport, uint8_t ep_addr, uint16_t largest_packet_size) { +#if DCD_ISO_ENABLED (void)rhport; - (void)ep_addr; - (void)largest_packet_size; + uint8_t const ep_id = ep_addr2idx(ep_addr); + + // hardware fixes iso to endpoint numbers 3, 6, 9, 12 + TU_ASSERT(ep_id_is_iso(ep_id)); + TU_ASSERT(largest_packet_size > 0); + + set_ep_size(ep_id, largest_packet_size); + + dma_desc_t* const dd = &_dcd.dd[ep_id]; + tu_memclr(dd, sizeof(dma_desc_t)); + dd->isochronous = 1; + dd->max_packet_size = largest_packet_size; + dd->retired = 1; // invalid at first + + sie_write(SIE_CMDCODE_ENDPOINT_SET_STATUS + ep_id, 1, 0); + return true; +#else + (void)rhport; (void)ep_addr; (void)largest_packet_size; return false; +#endif } bool dcd_edpt_iso_activate(uint8_t rhport, const tusb_desc_endpoint_t *desc_ep) { +#if DCD_ISO_ENABLED (void)rhport; - (void)desc_ep; + uint8_t const ep_id = ep_addr2idx(desc_ep->bEndpointAddress); + dma_desc_t* const dd = &_dcd.dd[ep_id]; + + // same fixed-number rule as alloc: without it a rejected-but-ignored alloc (classes + // discard that return) would set isochronous on a non-iso ep_id and underflow iso_slot() + TU_ASSERT(ep_id_is_iso(ep_id)); + + // kill any armed transfer from a previous alternate setting + LPC_USB->EpDMADis = TU_BIT(ep_id); + _dcd.udca[ep_id] = NULL; + + dd->isochronous = 1; + dd->max_packet_size = tu_edpt_packet_size(desc_ep); + dd->retired = 1; + + sie_write(SIE_CMDCODE_ENDPOINT_SET_STATUS + ep_id, 1, 0); + return true; +#else + (void)rhport; (void)desc_ep; return false; +#endif } void dcd_edpt_close_all (uint8_t rhport) @@ -373,15 +447,17 @@ static bool control_xact(uint8_t rhport, uint8_t dir, uint8_t * buffer, uint8_t { // Already received the DATA OUT packet _dcd.control.out_received = false; - _dcd.control.out_buffer = NULL; - _dcd.control.out_bytes = 0; uint8_t received = control_ep_read(buffer, len); dcd_event_xfer_complete(0, 0, received, XFER_RESULT_SUCCESS, true); }else { + // buffer is NULL for a status-stage ZLP: signal the pending xfer explicitly, + // NOT via out_buffer != NULL — a NULL-buffer queue mistaken for "nothing queued" + // leaves out_received stale and poisons the next control OUT data stage. _dcd.control.out_buffer = buffer; _dcd.control.out_bytes = len; + _dcd.control.out_queued = true; } } @@ -406,26 +482,65 @@ bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t * buffer, uint16_t t uint16_t const ep_size = dd->max_packet_size; uint8_t is_iso = dd->isochronous; - tu_memclr(dd, sizeof(dma_desc_t)); - dd->isochronous = is_iso; - dd->max_packet_size = ep_size; - dd->buffer = (uint32_t) buffer; - dd->buflen = total_bytes; +#if DCD_ISO_ENABLED + if ( is_iso ) + { + // iso: buflen counts packets; per-packet sizes live in the packet-size memory. + // One packet moves per frame (UM10562 12.15.6: DMA request is raised for + // DMA-enabled iso endpoints on every FRAME interrupt, both directions). + // Validate BEFORE touching the DD: bailing out mid-rebuild would leave a + // zeroed (retired=0 -> serviceable) descriptor armed for the frame engine. + TU_ASSERT(ep_size > 0); + uint16_t const packets = (total_bytes > 0) ? (uint16_t) tu_div_ceil(total_bytes, ep_size) : 1; + TU_ASSERT(packets <= ISO_MAX_PACKETS); + + uint8_t const slot = iso_slot(ep_id); + uint16_t remain = total_bytes; + for ( uint16_t i = 0; i < packets; i++ ) + { + uint16_t const pkt_len = tu_min16(remain, ep_size); + // IN: length to send (0 = ZLP). OUT: hardware writes back + // Frame_number|Packet_valid|Packet_length -- prefill 0 so a frame the + // hardware never wrote (missed/invalid) cannot read back as data. + _dcd.iso_psize[slot][i] = (ep_id & 1) ? pkt_len : 0; + remain = (uint16_t)(remain - pkt_len); + } - _dcd.udca[ep_id] = dd; + tu_memclr(dd, sizeof(dma_desc_t)); + dd->isochronous = 1; + dd->max_packet_size = ep_size; + dd->buffer = (uint32_t) buffer; + dd->buflen = packets; + dd->iso_packet_size_addr = (uint32_t) &_dcd.iso_psize[slot][0]; - if ( ep_id % 2 ) + _dcd.udca[ep_id] = dd; + LPC_USB->EpDMAEn = TU_BIT(ep_id); // frame-triggered: no DMARSet, no EpIntEn + } + else +#else + (void) is_iso; +#endif { - // Clear EP interrupt before Enable DMA - LPC_USB->EpIntEn &= ~TU_BIT(ep_id); - LPC_USB->EpDMAEn = TU_BIT(ep_id); + tu_memclr(dd, sizeof(dma_desc_t)); + dd->max_packet_size = ep_size; + dd->buffer = (uint32_t) buffer; + dd->buflen = total_bytes; - // endpoint IN need to actively raise DMA request - LPC_USB->DMARSet = TU_BIT(ep_id); - }else - { - // Enable DMA - LPC_USB->EpDMAEn = TU_BIT(ep_id); + _dcd.udca[ep_id] = dd; + + if ( ep_id % 2 ) + { + // Clear EP interrupt before Enable DMA + LPC_USB->EpIntEn &= ~TU_BIT(ep_id); + LPC_USB->EpDMAEn = TU_BIT(ep_id); + + // endpoint IN need to actively raise DMA request + LPC_USB->DMARSet = TU_BIT(ep_id); + }else + { + // Enable DMA + LPC_USB->EpDMAEn = TU_BIT(ep_id); + } } return true; @@ -451,13 +566,20 @@ static void control_xfer_isr(uint8_t rhport, uint32_t ep_int_status) uint8_t setup_packet[8]; control_ep_read(setup_packet, 8); // TODO read before clear setup above + // a new SETUP voids any half-finished control state + _dcd.control.out_queued = false; + _dcd.control.out_received = false; + _dcd.control.out_buffer = NULL; + _dcd.control.out_bytes = 0; + dcd_event_setup_received(rhport, setup_packet, true); } - else if ( _dcd.control.out_buffer ) + else if ( _dcd.control.out_queued ) { - // software queued transfer previously + // software queued transfer previously (out_buffer NULL = status ZLP) uint8_t received = control_ep_read(_dcd.control.out_buffer, _dcd.control.out_bytes); + _dcd.control.out_queued = false; _dcd.control.out_buffer = NULL; _dcd.control.out_bytes = 0; @@ -513,7 +635,32 @@ static void dd_complete_isr(uint8_t rhport, uint8_t ep_id) uint8_t result = (dd->status == DD_STATUS_NORMAL || dd->status == DD_STATUS_DATA_UNDERUN) ? XFER_RESULT_SUCCESS : XFER_RESULT_FAILED; uint8_t const ep_addr = (ep_id / 2) | ((ep_id & 0x01) ? TUSB_DIR_IN_MASK : 0); - dcd_event_xfer_complete(rhport, ep_addr, dd->present_count, result, true); + uint32_t xferred_bytes; +#if DCD_ISO_ENABLED + if ( ep_id_is_iso(ep_id) ) + { + // present_count is in packets; actual byte counts are in the packet-size memory + // (IN: as programmed by us, OUT: Packet_length written back by hardware, + // guarded by Packet_valid -- a frame with no packet must count as 0) + uint8_t const slot = iso_slot(ep_id); + uint16_t const packets = tu_min16(dd->present_count, ISO_MAX_PACKETS); + xferred_bytes = 0; + for (uint16_t i = 0; i < packets; i++) + { + uint32_t const psize = _dcd.iso_psize[slot][i]; + if ( (ep_id & 1) || (psize & TU_BIT(16)) ) + { + xferred_bytes += (psize & 0xFFFFu); + } + } + } + else +#endif + { + xferred_bytes = dd->present_count; + } + + dcd_event_xfer_complete(rhport, ep_addr, (uint16_t) xferred_bytes, result, true); } // main USB IRQ handler @@ -569,6 +716,16 @@ void dcd_int_handler(uint8_t rhport) { if ( tu_bit_test(eot, ep_id) ) { + // dispatch on the hardware's fixed ep-number/type map, NOT dd->isochronous: + // thread-mode dcd_edpt_xfer transiently zeroes the DD while rebuilding it +#if DCD_ISO_ENABLED + if ( ep_id_is_iso(ep_id) ) + { + // iso: last packet already left with its frame; complete both directions here + dd_complete_isr(rhport, ep_id); + } + else +#endif if ( ep_id & 0x01 ) { // IN enable EpInt for end of usb transfer -- cgit v1.3.1 From a3ee0b4ff12615552de50bd2a61287fdb0b11bd9 Mon Sep 17 00:00:00 2001 From: hathach Date: Thu, 16 Jul 2026 14:11:04 +0700 Subject: dcd_lpc17_40: mask USB IRQ around non-reentrant SIE and realization sequences The SIE command protocol (CmdCode + CCEMPTY/CDFULL handshake), the slave-mode Ctrl/RxData/TxData window, the EpIntEn read-modify-writes, and set_ep_size's ReEp/EP_RLZED handshake are all shared between thread-mode API calls and dcd_int_handler, and none are reentrant: an ISR preempting a thread-mode sequence consumes its handshake flags or, in set_ep_size's case, a bus reset's DevIntClr = 0xFFFFFFFF eats the EP_RLZED flag the spin waits on, hanging it forever. Guard them by masking only the USB IRQ (nestable, ISR-safe; CMSIS NVIC_DisableIRQ already ends with DSB+ISB). control_xact keeps the mask across its in_isr=true event push, since osal_none skips queue locking for in_isr. Hardening, not a fix for an observed failure: the ea4088 usbtest 30/30 + HIL 14/14 results were reproduced with and without it. The windows are a few instructions wide and most exposed on RTOS builds where class drivers queue transfers from tasks concurrent with the USB IRQ. --- src/portable/nxp/lpc17_40/dcd_lpc17_40.c | 56 +++++++++++++++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/portable/nxp/lpc17_40/dcd_lpc17_40.c b/src/portable/nxp/lpc17_40/dcd_lpc17_40.c index a1a44e9ae..6dc2b017c 100644 --- a/src/portable/nxp/lpc17_40/dcd_lpc17_40.c +++ b/src/portable/nxp/lpc17_40/dcd_lpc17_40.c @@ -114,6 +114,28 @@ CFG_TUD_MEM_SECTION TU_ATTR_ALIGNED(128) static dcd_data_t _dcd; // SIE Command //--------------------------------------------------------------------+ +// The SIE command protocol (CmdCode + CCEMPTY/CDFULL handshake) and the +// slave-mode Ctrl/RxData/TxData registers are shared between thread-mode API +// calls and dcd_int_handler, and are not reentrant: an ISR preempting a +// thread-mode SIE sequence consumes its handshake flags and overwrites +// CmdCode (symptom: EP0 wedges/answers stale data right after SET_INTERFACE +// stall/clear-stall bursts overlapping bulk EOT interrupts). Mask only the +// USB interrupt around those sequences; safe to nest, including from the ISR. +static inline bool usb_irq_lock(void) +{ + bool const enabled = NVIC_GetEnableIRQ(USB_IRQn) != 0; + if (enabled) + { + NVIC_DisableIRQ(USB_IRQn); // CMSIS already ends this with DSB+ISB + } + return enabled; +} + +static inline void usb_irq_unlock(bool enabled) +{ + if (enabled) NVIC_EnableIRQ(USB_IRQn); +} + static void sie_cmd_code (sie_cmdphase_t phase, uint8_t code_data) { LPC_USB->DevIntClr = (DEV_INT_COMMAND_CODE_EMPTY_MASK | DEV_INT_COMMAND_DATA_FULL_MASK); @@ -127,19 +149,28 @@ static void sie_cmd_code (sie_cmdphase_t phase, uint8_t code_data) static void sie_write (uint8_t cmd_code, uint8_t data_len, uint8_t data) { + bool const lock = usb_irq_lock(); + sie_cmd_code(SIE_CMDPHASE_COMMAND, cmd_code); if (data_len) { sie_cmd_code(SIE_CMDPHASE_WRITE, data); } + + usb_irq_unlock(lock); } static uint8_t sie_read (uint8_t cmd_code) { + bool const lock = usb_irq_lock(); + sie_cmd_code(SIE_CMDPHASE_COMMAND , cmd_code); sie_cmd_code(SIE_CMDPHASE_READ , cmd_code); - return (uint8_t) LPC_USB->CmdData; + uint8_t const data = (uint8_t) LPC_USB->CmdData; + + usb_irq_unlock(lock); + return data; } //--------------------------------------------------------------------+ @@ -152,6 +183,11 @@ static inline uint8_t ep_addr2idx(uint8_t ep_addr) static void set_ep_size(uint8_t ep_id, uint16_t max_packet_size) { + // ReEp RMW + the EP_RLZED handshake share DevIntSt with the ISR: a bus reset + // from dcd_int_handler writes DevIntClr = 0xFFFFFFFF and would consume the + // flag this spin waits on, hanging it forever -> same lock as the SIE paths. + bool const lock = usb_irq_lock(); + // follows example in 11.10.4.2 LPC_USB->ReEp |= TU_BIT(ep_id); LPC_USB->EpInd = ep_id; // select index before setting packet size @@ -159,6 +195,8 @@ static void set_ep_size(uint8_t ep_id, uint16_t max_packet_size) while ((LPC_USB->DevIntSt & DEV_INT_ENDPOINT_REALIZED_MASK) == 0) {} LPC_USB->DevIntClr = DEV_INT_ENDPOINT_REALIZED_MASK; + + usb_irq_unlock(lock); } @@ -265,6 +303,7 @@ static inline uint8_t byte2dword(uint8_t bytes) static void control_ep_write(void const * buffer, uint8_t len) { uint32_t const * buf32 = (uint32_t const *) buffer; + bool const lock = usb_irq_lock(); // Ctrl/TxData + SIE sequence must not interleave with the ISR LPC_USB->Ctrl = USBCTRL_WRITE_ENABLE_MASK; // logical endpoint = 0 LPC_USB->TxPLen = (uint32_t) len; @@ -280,10 +319,14 @@ static void control_ep_write(void const * buffer, uint8_t len) // select control IN & validate the endpoint sie_write(SIE_CMDCODE_ENDPOINT_SELECT+1, 0, 0); sie_write(SIE_CMDCODE_BUFFER_VALIDATE , 0, 0); + + usb_irq_unlock(lock); } static uint8_t control_ep_read(void * buffer, uint8_t len) { + bool const lock = usb_irq_lock(); // Ctrl/RxData + SIE sequence must not interleave with the ISR + LPC_USB->Ctrl = USBCTRL_READ_ENABLE_MASK; // logical endpoint = 0 while ((LPC_USB->RxPLen & USBRXPLEN_PACKET_READY_MASK) == 0) {} // TODO blocking, should have timeout @@ -302,6 +345,7 @@ static uint8_t control_ep_read(void * buffer, uint8_t len) sie_write(SIE_CMDCODE_ENDPOINT_SELECT+0, 0, 0); sie_write(SIE_CMDCODE_BUFFER_CLEAR , 0, 0); + usb_irq_unlock(lock); return len; } @@ -443,13 +487,19 @@ static bool control_xact(uint8_t rhport, uint8_t dir, uint8_t * buffer, uint8_t control_ep_write(buffer, len); }else { + // guard the out_received/out_buffer handshake against the EP0 OUT ISR + bool const lock = usb_irq_lock(); + if ( _dcd.control.out_received ) { // Already received the DATA OUT packet _dcd.control.out_received = false; uint8_t received = control_ep_read(buffer, len); + // event queued with in_isr=true, which skips the queue's own locking: keep the + // USB IRQ masked across it, or a real ISR completion could interleave the write dcd_event_xfer_complete(0, 0, received, XFER_RESULT_SUCCESS, true); + usb_irq_unlock(lock); }else { // buffer is NULL for a status-stage ZLP: signal the pending xfer explicitly, @@ -458,6 +508,7 @@ static bool control_xact(uint8_t rhport, uint8_t dir, uint8_t * buffer, uint8_t _dcd.control.out_buffer = buffer; _dcd.control.out_bytes = len; _dcd.control.out_queued = true; + usb_irq_unlock(lock); } } @@ -531,8 +582,11 @@ bool dcd_edpt_xfer(uint8_t rhport, uint8_t ep_addr, uint8_t * buffer, uint16_t t if ( ep_id % 2 ) { // Clear EP interrupt before Enable DMA + // EpIntEn read-modify-write races the ISR's own RMWs -> lock + bool const lock = usb_irq_lock(); LPC_USB->EpIntEn &= ~TU_BIT(ep_id); LPC_USB->EpDMAEn = TU_BIT(ep_id); + usb_irq_unlock(lock); // endpoint IN need to actively raise DMA request LPC_USB->DMARSet = TU_BIT(ep_id); -- cgit v1.3.1 From cb224400931b7fbc3477a87a258c0602092abe6b Mon Sep 17 00:00:00 2001 From: hathach Date: Fri, 17 Jul 2026 17:58:25 +0700 Subject: dcd_lpc17_40: address review findings in the iso paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From a second max-effort review of the branch: - Drop the dead TUSB_XFER_ISOCHRONOUS case in dcd_edpt_open: iso endpoints are armed via dcd_edpt_iso_alloc/activate (TUP_DCD_EDPT_ISO_ALLOC is defined for this IP), never through dcd_edpt_open, so the case and its dd->isochronous assignment were unreachable and asserted a false invariant. Only bulk/interrupt reach the switch now. - Extend the iso compile gate to the classes that actually arm an iso endpoint: DCD_ISO_ENABLED now includes CFG_TUD_BTH (bth_device.c opens an iso voice endpoint). Without it a BTH build would compile the iso machinery out and fail SET_INTERFACE at runtime. - Un-skip LPC175X_6X in the usbtest example: it shares dcd_lpc17_40.c with LPC40XX verbatim, so the "DCD has no isochronous support" skip reason no longer holds. Build-verified for lpcxpresso1769 (previously blocked by the skip). - TU_ATTR_UNUSED on the ep_id_is_iso helper: every caller is under #if DCD_ISO_ENABLED, so non-iso builds don't reference it and clang's -Wunused-function (fatal in CI) rejected the build — gcc stays quiet. Verified with the full lpc17 and lpc40 example sets under arm-clang. A fifth finding — bounding control_ep_read's PACKET_READY spin with a timeout — was implemented and REVERTED: a naive 100k-iteration bound fires on legitimately-slow control reads and intermittently drops the device (hardware-proven by interleaved A/B testing against the pre-fix binary). The infinite wait is retained; the read is only reached once out_received/ out_queued signal data is present, so the theoretical IRQ-off hang is not reachable in practice. Re-verified on ea4088_quickstart: usbtest 30/30 (repeated) + HIL 14/14. --- examples/device/usbtest/skip.txt | 1 - src/portable/nxp/lpc17_40/dcd_lpc17_40.c | 24 +++++++++++------------- 2 files changed, 11 insertions(+), 14 deletions(-) (limited to 'src') diff --git a/examples/device/usbtest/skip.txt b/examples/device/usbtest/skip.txt index e789c4b91..792404fe4 100644 --- a/examples/device/usbtest/skip.txt +++ b/examples/device/usbtest/skip.txt @@ -4,7 +4,6 @@ mcu:SAMD11 # DCD has no isochronous support (dcd_edpt_iso_alloc refuses), tier-4 cannot enumerate: mcu:CXD56 mcu:FT90X -mcu:LPC175X_6X mcu:NUC100 mcu:NUC120 mcu:NUC505 diff --git a/src/portable/nxp/lpc17_40/dcd_lpc17_40.c b/src/portable/nxp/lpc17_40/dcd_lpc17_40.c index 6dc2b017c..b577d0e9f 100644 --- a/src/portable/nxp/lpc17_40/dcd_lpc17_40.c +++ b/src/portable/nxp/lpc17_40/dcd_lpc17_40.c @@ -20,8 +20,10 @@ #define DCD_ENDPOINT_MAX 32 // The iso machinery (5th DD word + packet-size memory) costs USB RAM on every build; -// compile it only when a class that can open an iso endpoint is enabled. -#define DCD_ISO_ENABLED (CFG_TUD_AUDIO || CFG_TUD_VIDEO || CFG_TUD_VENDOR) +// compile it only when a class that can open an iso endpoint is enabled. Keep this in +// sync with the classes that actually arm an iso endpoint: audio, video, BTH (voice), +// and vendor (its optional CFG_TUD_VENDOR_EP_ISO_* endpoints, exercised by usbtest). +#define DCD_ISO_ENABLED (CFG_TUD_AUDIO || CFG_TUD_VIDEO || CFG_TUD_VENDOR || CFG_TUD_BTH) typedef struct TU_ATTR_ALIGNED(4) { @@ -64,7 +66,9 @@ TU_VERIFY_STATIC( sizeof(dma_desc_t) == (DCD_ISO_ENABLED ? 20 : 16), "size is no // Hardware fixes endpoint type by number: 3, 6, 9, 12 are the iso-capable ones. // Constant per ep_id (= 2*epnum + dir) — unlike dd->isochronous, which dcd_edpt_xfer // transiently zeroes while rebuilding the DD, this is safe to dispatch on from the ISR. -TU_ATTR_ALWAYS_INLINE static inline bool ep_id_is_iso(uint8_t ep_id) { +// TU_ATTR_UNUSED: every caller is under #if DCD_ISO_ENABLED, so non-iso builds don't +// reference it and clang -Wunused-function (fatal) would otherwise reject the build. +TU_ATTR_UNUSED TU_ATTR_ALWAYS_INLINE static inline bool ep_id_is_iso(uint8_t ep_id) { uint8_t const epnum = (uint8_t)(ep_id >> 1); return (epnum % 3) == 0 && (epnum != 0) && (epnum != 15); } @@ -360,8 +364,9 @@ bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc) uint8_t const epnum = tu_edpt_number(p_endpoint_desc->bEndpointAddress); uint8_t const ep_id = ep_addr2idx(p_endpoint_desc->bEndpointAddress); - // Endpoint type is fixed to endpoint number - // 1: interrupt, 2: Bulk, 3: Iso and so on + // Endpoint type is fixed to endpoint number (1 interrupt, 2 bulk, 3 iso, ...). + // Iso endpoints are armed via dcd_edpt_iso_alloc/activate, never through here + // (TUP_DCD_EDPT_ISO_ALLOC is defined for this IP), so only bulk/interrupt land here. switch ( p_endpoint_desc->bmAttributes.xfer ) { case TUSB_XFER_INTERRUPT: @@ -372,11 +377,6 @@ bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc) TU_ASSERT((epnum % 3) == 2 || (epnum == 15)); break; - case TUSB_XFER_ISOCHRONOUS: - // iso machinery is compiled out when no iso-capable class is enabled - TU_ASSERT(DCD_ISO_ENABLED && (epnum % 3) == 0 && (epnum != 0) && (epnum != 15)); - break; - default: break; } @@ -387,9 +387,7 @@ bool dcd_edpt_open(uint8_t rhport, tusb_desc_endpoint_t const * p_endpoint_desc) //------------- first DD prepare -------------// dma_desc_t* const dd = &_dcd.dd[ep_id]; - tu_memclr(dd, sizeof(dma_desc_t)); - - dd->isochronous = (p_endpoint_desc->bmAttributes.xfer == TUSB_XFER_ISOCHRONOUS) ? 1 : 0; + tu_memclr(dd, sizeof(dma_desc_t)); // non-iso: isochronous stays 0 dd->max_packet_size = ep_size; dd->retired = 1; // invalid at first -- cgit v1.3.1