hcd(ch32_usbfs): pace isochronous transfers with SOF - #3839
Open
zjzhang-cn wants to merge 1 commit into
Open
Conversation
The USBFS SIE executes one transaction the moment HOST_EP_PID is written, with no frame-boundary alignment, so a re-submitted isochronous endpoint is polled at the task-loop rate instead of once per 1 ms USB frame. Rework the host driver: - Schedule isochronous transfers from the SOF interrupt: each ISO endpoint is allowed at most one transaction per USB frame, armed from the SOF tick and from the ISO completion handler, with a one-slot pending queue per endpoint so an early same-frame re-submission cannot starve the opposite direction. - Retry NAK'd control/bulk transfers once per frame from the SOF interrupt with progressive backoff (1 -> 2 -> ... -> 64 frames) instead of a tight task-loop retry that floods the device. - Point the DMA directly at the application buffer instead of the 64-byte internal TX/RX buffers, which would be overflowed by larger packets (e.g. full-speed isochronous); the internal buffers are kept only for setup, status-stage and zero-length transfers. - Ignore DETECT edges asserted during port reset (SE0) so the reset is not reported as a device removal that kills enumeration. - Keep UIE_TRANSFER enabled for the driver lifetime and STOP (HOST_EP_PID = 0) before clearing UIF_TRANSFER to avoid re-arming the SIE with a stale PID; route completions by an xfer_type snapshot so a control transfer can never complete through the ISO path with a stale RX_LEN. - Implement hcd_edpt_close() and hcd_edpt_abort_xfer(); return the SOF frame counter from hcd_frame_number(). Validated on nanoch32v203: all 34 board targets build (34 OK, 0 failed) and the host_info_to_device_cdc, host_hid_to_device_cdc, device_info and msc_file_explorer examples run on hardware.
Contributor
There was a problem hiding this comment.
Pull request overview
Reworks the CH32V20x USBFS host driver with SOF-paced isochronous scheduling and improved transfer lifecycle handling.
Changes:
- Adds SOF-based ISO scheduling, queuing, and frame tracking.
- Adds NAK retry backoff, direct DMA, and reset/interrupt safeguards.
- Implements endpoint close/abort APIs and frame-number reporting.
Suppressed comments (10)
src/portable/wch/hcd_ch32_usbfs.c:446
- For a NAKed SETUP stage,
hcd_int_handleridentifies the record as EP0 OUT, so this derivesUSB_PID_OUTfor the retry. A SETUP token must be retried asUSB_PID_SETUP; otherwise a NAK during enumeration is retried as an ordinary data OUT packet and the control transfer cannot progress. Preserve the armed request PID in the NAK stash/current slot and use it here.
uint8_t pid = (tu_edpt_dir(best->ep_addr) == TUSB_DIR_IN) ? USB_PID_IN : USB_PID_OUT;
bool prev = usbfs_irq_save();
src/portable/wch/hcd_ch32_usbfs.c:472
- Isochronous transfers have no ACK/NAK handshake, but they can still complete with a CRC, timeout, or other host error. This path ignores the response/status entirely and always reports success; for a failed IN transaction
RX_LENcan also be stale, so old data may be delivered as a fresh packet. Check the transfer status and report a failed/zero-length completion for non-success results.
// ISO transfer completion. ISO has no handshake (no NAK/STALL): every armed
// transaction completes successfully. RX_LEN holds received bytes for IN;
// for OUT the whole queued packet was sent.
static void iso_transfer_complete(uint8_t request_pid, usb_edpt_t *edpt) {
uint16_t done_len = (request_pid == USB_PID_IN) ? (uint16_t)USBOTG_H_FS->RX_LEN : usb_current_xfer_info.bufferlen;
src/portable/wch/hcd_ch32_usbfs.c:373
- The frame gate only compares against the current frame, and the endpoint record does not retain
bInterval. Full-speed isochronous endpoints may specify service intervals greater than one frame, so this schedules tokens every frame instead of the interval from the descriptor. Store and honor the endpoint interval when deciding whether a queued ISO transfer is due.
if (cur->configured && cur->xfer_type == TUSB_XFER_ISOCHRONOUS && cur->iso_queued && !cur->iso_active &&
cur->iso_last_frame != g_sof_frame) {
src/portable/wch/hcd_ch32_usbfs.c:740
- On disconnect this only clears the global in-flight slot; endpoint records still retain
iso_queued/iso_activeandis_nak_pending, while SOF generation remains enabled. Before the deferred remove event callshcd_device_close(), a subsequent SOF can therefore arm a transfer for the detached device and emit a stale completion. Cancel the endpoint queues or gate the SOF armers as part of disconnect handling.
// Drop any in-flight / queued ISO on disconnect.
bool prev = usbfs_irq_save();
usb_current_xfer_info.is_busy = false;
USBOTG_H_FS->HOST_EP_PID = 0;
usbfs_irq_restore(prev);
src/portable/wch/hcd_ch32_usbfs.c:898
- Resetting
nak_backoffhere covers data/status transfers, buthcd_setup_send()does not reset the same endpoint record's backoff. After one NAKed SETUP, subsequent control SETUP requests inherit the old exponential delay instead of starting with the documented one-frame retry.
edpt_info->nak_backoff = 1; // fresh transfer: reset the progressive NAK backoff
src/portable/wch/hcd_ch32_usbfs.c:889
- The non-ISO path does not reserve the shared in-flight slot atomically with its busy check. An SOF can arrive after the
whileexits but beforeis_busyis set, arm a queued ISO transfer, and then this code overwrites the global slot and USB registers with the control/bulk transfer, losing the ISO completion. Publish the non-ISO slot underusbfs_irq_save()before any preemptible work, then arm after restoring the IRQ.
while (usb_current_xfer_info.is_busy) {}
hardware_set_port_address_speed(dev_addr);
usb_current_xfer_info.is_busy = true;
src/portable/wch/hcd_ch32_usbfs.c:933
- This setup submission has the same check-then-publish race as the endpoint transfer path: a SOF can arm a queued ISO between the busy-wait and this assignment, after which the setup state overwrites the shared slot. Reserve
is_busyatomically with the wait before returning to interruptible code.
while (usb_current_xfer_info.is_busy) {}
usb_current_xfer_info.is_busy = true;
src/portable/wch/hcd_ch32_usbfs.c:756
hcd_int_handler()supportsin_isr == falsefor polling, but this always passestrue. A polled SOF handler consequently skipsarm_iso_drain()'s IRQ critical section and can race an actual USB IRQ while publishing and arming the ISO slot. Propagate the handler's context flag instead of assuming every invocation is an ISR.
arm_iso_drain(true);
src/portable/wch/hcd_ch32_usbfs.c:757
- With a continuously queued ISO endpoint,
arm_iso_drain()setsis_busyat every SOF, soarm_nak_retry()immediately returns. Since ISO completion only callsarm_iso_drain()again, a NAK-pending control/bulk transfer can be starved indefinitely whenever ISO traffic is continuous. The scheduler needs fair arbitration or a reserved retry opportunity.
// ISO has priority over the NAK retry: isochronous transfers must keep
// their 1/frame cadence even while a control transfer is NAKing (the retry
// happens in the gaps). Reversing this starves ISO under a persistent
// control NAK.
arm_iso_drain(true);
arm_nak_retry();
src/portable/wch/hcd_ch32_usbfs.c:419
- The ISO path treats
iso_lenas one hardware packet and reports the whole requested length on OUT, but it never checks it againstmax_packet_sizeor retains a remaining length. A caller using the HCD's total-length transfer API with more than one packet will program an oversized ISO transaction or complete after only the first packet with an incorrect byte count. Either explicitly restrict ISO submissions to one packet or implement per-frame remainder handling.
USBOTG_H_FS->HOST_TX_LEN = arm_len;
hardware_start_xfer(USB_PID_OUT, arm_ep, 0, true);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+588
to
+592
| edpt->is_nak_pending = true; | ||
| edpt->buflen = usb_current_xfer_info.bufferlen; | ||
| edpt->buf = usb_current_xfer_info.buffer; | ||
| edpt->nak_xferred = usb_current_xfer_info.xferred_len; | ||
| edpt->nak_backoff = TU_MIN(edpt->nak_backoff * 2, 64); |
Comment on lines
+913
to
+915
| usb_edpt_t *edpt = get_edpt_record(dev_addr, ep_addr); | ||
| if (edpt == NULL || edpt->xfer_type != TUSB_XFER_ISOCHRONOUS) { | ||
| return false; |
| edpt->iso_active = false; | ||
| usbfs_irq_restore(prev); | ||
|
|
||
| hcd_event_xfer_complete(dev_addr, ep_addr, done_len, XFER_RESULT_SUCCESS, true); |
Comment on lines
+639
to
+641
| // Drop any in-flight / queued ISO so it does not resume after the reset. | ||
| usb_current_xfer_info.is_busy = false; | ||
| USBOTG_H_FS->HOST_EP_PID = 0; |
| } usb_edpt_t; | ||
|
|
||
| static usb_edpt_t usb_edpt_list[CFG_TUH_DEVICE_MAX * 6] = {}; | ||
| static usb_edpt_t usb_edpt_list[CFG_TUH_DEVICE_MAX * CFG_TUH_ENDPOINT_MAX] = {}; |
Comment on lines
+283
to
+288
| if (pid == USB_PID_IN) { | ||
| USBOTG_H_FS->HOST_RX_DMA = | ||
| (uint32_t)(usb_current_xfer_info.buffer != NULL ? usb_current_xfer_info.buffer : (uint8_t *)USBFS_RX_Buf); | ||
| } else { | ||
| USBOTG_H_FS->HOST_TX_DMA = | ||
| (uint32_t)(usb_current_xfer_info.buffer != NULL ? usb_current_xfer_info.buffer : (uint8_t *)USBFS_TX_Buf); |
Comment on lines
+408
to
+412
| if (!in_isr) { | ||
| usbfs_irq_restore(prev); | ||
| } | ||
|
|
||
| if (armed) { |
Size Difference ReportBecause TinyUSB code size varies by port and configuration, the metrics below represent the averaged totals across all example builds. Note: If there is no change, only one value is shown. Changes >1% in size
Changes <1% in size
No changes
|
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| ch32v203c_r0_1v0/device_info | 19,144 → 20,408 (+1,264) | — | — | 2,436 → 4,356 (+1,920) | 24,228 → 27,412 (+3,184) | +13.1% |
| ch32v203c_r0_1v0/midi_rx | 20,540 → 21,832 (+1,292) | — | — | 3,340 → 5,260 (+1,920) | 26,544 → 29,756 (+3,212) | +12.1% |
| ch32v203c_r0_1v0/midi2_host | 21,680 → 22,912 (+1,232) | — | — | 6,864 → 8,784 (+1,920) | 31,196 → 34,348 (+3,152) | +10.1% |
| ch32v203c_r0_1v0/host_info_to_device_cdc | 31,296 → 32,628 (+1,332) | — | — | 3,212 → 5,132 (+1,920) | 37,248 → 40,500 (+3,252) | +8.7% |
| ch32v203c_r0_1v0/host_hid_to_device_cdc | 31,360 → 32,600 (+1,240) | — | — | 4,632 → 6,552 (+1,920) | 38,728 → 41,888 (+3,160) | +8.2% |
| ch32v203c_r0_1v0/cdc_msc_hid | 32,344 → 33,560 (+1,216) | — | — | 5,316 → 7,236 (+1,920) | 40,340 → 43,476 (+3,136) | +7.8% |
| ch32v203c_r0_1v0/bare_api | 20,032 → 21,232 (+1,200) | — | — | 3,060 → 3,828 (+768) | 25,740 → 27,708 (+1,968) | +7.6% |
| ch32v203c_r0_1v0/hid_controller | 18,388 → 19,592 (+1,204) | — | — | 1,716 → 2,196 (+480) | 22,688 → 24,372 (+1,684) | +7.4% |
| ch32v203c_r0_1v0/msc_file_explorer | 40,164 → 41,380 (+1,216) | — | — | 11,000 → 12,920 (+1,920) | 53,816 → 56,952 (+3,136) | +5.8% |
Hardware-in-the-loop (HIL) Test ReportNo HIL run for this push (no affected boards, or hardware testing did not run). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The USBFS SIE executes one transaction the moment
HOST_EP_PIDiswritten, with no frame-boundary alignment, so a re-submitted
isochronous endpoint is polled at the task-loop rate instead of once
per 1 ms USB frame. This reworks the CH32V20x USBFS host driver so
isochronous transfers are paced by the SOF interrupt.
What changed
at most one transaction per USB frame, armed from the SOF tick and
from the ISO completion handler, with a one-slot pending queue per
endpoint so an early same-frame re-submission cannot starve the
opposite direction.
transfers are re-armed once per frame from the SOF interrupt
(1 → 2 → … → 64 frames) instead of a tight task-loop retry that
floods the device's control endpoint and starves both sides.
TX/RX buffers would be overflowed by larger packets (e.g. FS
isochronous); they are now used only for setup, status-stage and
zero-length transfers.
asserted while the bus is in reset (SE0) are ignored, otherwise the
reset is reported as a device removal and kills enumeration.
UIE_TRANSFERstays enabled for thedriver lifetime; the ISR STOPS (
HOST_EP_PID = 0) before clearingUIF_TRANSFERto avoid re-arming the SIE with a stale PID;completions are routed by an
xfer_typesnapshot so a controltransfer can never complete through the ISO path with a stale
RX_LEN.hcd_edpt_close()andhcd_edpt_abort_xfer();hcd_frame_number()returns the SOF framecounter.
Motivation
Host enumeration on CH32V20x USBFS was unreliable after the 0.20.0 →
0.21.0 window: a HID device that mounted fine with 0.20.0 failed to
mount with 0.21.0 on the same hardware.
Test / build evidence
./tools/build.py -b nanoch32v203 -s make: 34 OK, 0 failedhost_info_to_device_cdc,host_hid_to_device_cdc,device_info,msc_file_explorerhost/device_infobuildwith
riscv32-wch-elf-gcc15 passes