From cf4bb7d52a87a17c2bfa4693de31fb2c40914a7d Mon Sep 17 00:00:00 2001 From: Mark Richardson Date: Mon, 5 Oct 2026 10:52:10 -0700 Subject: [PATCH 1/2] pbio/drv/usb_nxt: Skip masked endpoints in the interrupt handler. UDP_ISR still reports endpoints whose interrupt is masked (e.g. the bulk OUT endpoint while a received packet waits for the process). The handler picked the lowest one, so a masked endpoint could repeatedly sidetrack handling of the real interrupt (e.g. a completed bulk IN packet), which stayed asserted and re-entered the handler forever, hanging the hub. Select the endpoint from UDP_ISR & UDP_IMR, as the Linux at91_udc driver does. Reproduced by sending stdin bursts while the hub was printing, with the CSR fix from the next commit applied. A watchdog on the system tick caught the UDP interrupt firing 141086 times per second, in the masked bulk OUT branch of the handler. With this change, 15 of 15 rounds completed over three runs. --- lib/pbio/drv/usb/usb_nxt.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lib/pbio/drv/usb/usb_nxt.c b/lib/pbio/drv/usb/usb_nxt.c index 71d309b01..aedc57a54 100644 --- a/lib/pbio/drv/usb/usb_nxt.c +++ b/lib/pbio/drv/usb/usb_nxt.c @@ -727,8 +727,14 @@ static void pbdrv_usb_nxt_isr(void) { isr &= ~AT91C_UDP_RXRSM; } + /* Skip endpoints whose interrupt is masked (e.g. the bulk OUT endpoint + * while a received packet waits for the process), which UDP_ISR still + * reports. These can repeatedly sidetrack handling of a real interrupt, + * causing a hang. + */ + uint32_t pending = isr & *AT91C_UDP_IMR; for (endpoint = 0; endpoint < PBDRV_USB_NXT_N_ENDPOINTS; endpoint++) { - if (isr & (1 << endpoint)) { + if (pending & (1 << endpoint)) { break; } } From 279a5e1a247ba8fa6c1e8e8690c61f8ea95771bc Mon Sep 17 00:00:00 2001 From: Mark Richardson Date: Mon, 5 Oct 2026 10:52:10 -0700 Subject: [PATCH 2/2] pbio/drv/usb_nxt: Don't poll CSR after writes. The CSR helpers write a flag and then poll until the controller reports the change. Some flags are changed back by the controller itself (e.g. TXPKTRDY is cleared once the packet is sent). If an interrupt delays the poll for longer, the loop never sees the flag set and just waits indefinitely. This happened with an I2C sensor in use, since the bit-banged I2C interrupt runs every 26us. While reading the ultrasonic sensor, the hub froze within a few minutes. Recording the interrupted program counter from the system tick showed it spinning in pbdrv_usb_nxt_csr_set_flag() (which was called to set TXPKTRDY for a bulk IN packet). A CSR write only needs a fixed time to take effect (3 UDPCK and 3 MCK cycles), so wait that long instead of polling. This also replaces the loops waiting for endpoint configuration writes, which could hang the same way. With this change, the same sensor test ran 7 times without a freeze (it previously froze within 1-3 runs, 4 times out of 4). --- lib/pbio/drv/usb/usb_nxt.c | 52 +++++++++++++------------------------- 1 file changed, 17 insertions(+), 35 deletions(-) diff --git a/lib/pbio/drv/usb/usb_nxt.c b/lib/pbio/drv/usb/usb_nxt.c index aedc57a54..5a64dcda1 100644 --- a/lib/pbio/drv/usb/usb_nxt.c +++ b/lib/pbio/drv/usb/usb_nxt.c @@ -278,32 +278,29 @@ static bool pbdrv_usb_nxt_tx_zlp[PBDRV_USB_NXT_N_ENDPOINTS]; #define CSR_NO_EFFECT_1_ALL (AT91C_UDP_TXCOMP | AT91C_UDP_RX_DATA_BK0 | \ AT91C_UDP_RXSETUP | AT91C_UDP_ISOERROR | AT91C_UDP_RX_DATA_BK1) -/* The flags in the UDP_CSR register are a little strange: writing to - * them does not instantly change their value. Their value will change - * to reflect the write when the USB controller has taken the change - * into account. The driver must wait until the controller - * acknowledges changes to CSR. - * - * These helpers set/clear CSR flags, and then loop waiting for the - * controller to synchronize +/* A CSR write takes effect only once it has crossed into the USB clock + * domain. The datasheet (doc6175, UDP_CSR) says to wait 3 UDPCK and 3 MCK + * cycles (125 ns) instead of polling for the new value. The controller may + * already have changed it back (e.g. TXPKTRDY once the packet is sent). */ +#define PBDRV_USB_NXT_CSR_SYNC_NS 150 + +static void pbdrv_usb_nxt_csr_write(uint8_t endpoint, uint32_t csr) { + AT91C_UDP_CSR[endpoint] = csr; + nx_systick_wait_ns(PBDRV_USB_NXT_CSR_SYNC_NS); +} + static void pbdrv_usb_nxt_csr_clear_flag(uint8_t endpoint, uint32_t flags) { uint32_t csr = AT91C_UDP_CSR[endpoint]; csr |= CSR_NO_EFFECT_1_ALL; csr &= ~(flags); - AT91C_UDP_CSR[endpoint] = csr; - while (AT91C_UDP_CSR[endpoint] & (flags)) { - ; - } + pbdrv_usb_nxt_csr_write(endpoint, csr); } static void pbdrv_usb_nxt_csr_set_flag(uint8_t endpoint, uint32_t flags) { uint32_t csr = AT91C_UDP_CSR[endpoint]; csr |= CSR_NO_EFFECT_1_ALL | (flags); - AT91C_UDP_CSR[endpoint] = csr; - while ((AT91C_UDP_CSR[endpoint] & (flags)) != (flags)) { - ; - } + pbdrv_usb_nxt_csr_write(endpoint, csr); } /* Starts sending data to the host. If the data cannot fit into a @@ -552,20 +549,9 @@ static void pbdrv_usb_handle_std_request(pbdrv_usb_setup_packet_t *packet) { /* Enable the CDC data and notification endpoints. */ pbdrv_usb_rx_bank = AT91C_UDP_RX_DATA_BK0; - AT91C_UDP_CSR[EP_BULK_OUT] = AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_BULK_OUT; - while (AT91C_UDP_CSR[EP_BULK_OUT] != (AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_BULK_OUT)) { - ; - } - - AT91C_UDP_CSR[EP_BULK_IN] = AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_BULK_IN; - while (AT91C_UDP_CSR[EP_BULK_IN] != (AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_BULK_IN)) { - ; - } - - AT91C_UDP_CSR[EP_NOTIF] = AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_INT_IN; - while (AT91C_UDP_CSR[EP_NOTIF] != (AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_INT_IN)) { - ; - } + pbdrv_usb_nxt_csr_write(EP_BULK_OUT, AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_BULK_OUT); + pbdrv_usb_nxt_csr_write(EP_BULK_IN, AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_BULK_IN); + pbdrv_usb_nxt_csr_write(EP_NOTIF, AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_INT_IN); pbdrv_usb_nxt_configured = packet->wValue > 0; break; @@ -685,11 +671,7 @@ static void pbdrv_usb_nxt_isr(void) { pbdrv_usb_rx_len = 0; /* Reset EP0 to a basic control endpoint. */ - /* TODO: The while is ugly. Fix it. */ - AT91C_UDP_CSR[0] = AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_CTRL; - while (AT91C_UDP_CSR[0] != (AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_CTRL)) { - ; - } + pbdrv_usb_nxt_csr_write(EP_CONTROL, AT91C_UDP_EPEDS | AT91C_UDP_EPTYPE_CTRL); /* Enable interrupt handling for all three endpoints, as well as * suspend/resume.