Skip to content

pbio/drv/usb_nxt: Fix hangs during USB communication. #512

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
laurensvalk merged 2 commits into from
Oct 23, 2026
Merged

pbio/drv/usb_nxt: Fix hangs during USB communication. #512

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf4bb7d
pbio/drv/usb_nxt: Skip masked endpoints in the interrupt handler.
fuzic Oct 5, 2026
279a5e1
pbio/drv/usb_nxt: Don't poll CSR after writes.
fuzic Oct 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 24 additions & 36 deletions lib/pbio/drv/usb/usb_nxt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

@laurensvalk laurensvalk Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a blocking wait though. We are gradually converting the nxos drivers to use the cooperative multitasking of pbio. Might be quite a bit of refactoring work though since this is driven from the ISR.

@dlech dlech Oct 6, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should just be on the order of 100 ns or so though, so not really harmful blocking. I think we are talking less than 10 cpu cycles. Doesn't seem worth it to yield here to me.

@fuzic fuzic Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with David that yielding isn't worth it for ~6 cycles, and this runs inside the UDP ISR so it can't yield anyway. Happy to look at moving the endpoint handling out of the ISR into a pbio process as a separate PR.

}

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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -727,8 +709,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;
}
}
Expand Down
Loading