hcd/dwc2: fix FIFO allocation and periodic transfer - #3815
Conversation
Hardware-in-the-loop (HIL) Test Reporthfp.jsonSkipped by PR selection: no affected boards on this rig. tinyusb-esp.jsonScoped run: 2 board(s) — espressif_p4_function_ev, espressif_s3_devkitm. Boards/tests not listed were not run. ✅ 0 passed · ❌ 8 failed · ⚪ 0 skipped · blank not run
tinyusb.jsonScoped run: 1 board(s) — stm32f723disco. Boards/tests not listed were not run. ✅ 7 passed · ❌ 1 failed · ⚪ 0 skipped · blank not run
|
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| feather_stm32f405/hid_controller | 21,564 → 22,192 (+628) | — | — | — | 22,904 → 23,532 (+628) | +2.7% |
| feather_stm32f405/device_info | 22,336 → 22,964 (+628) | — | — | — | 23,800 → 24,428 (+628) | +2.6% |
| feather_stm32f405/bare_api | 22,920 → 23,548 (+628) | — | — | — | 24,472 → 25,100 (+628) | +2.6% |
| feather_stm32f405/midi_rx | 23,492 → 24,120 (+628) | — | — | — | 24,620 → 25,248 (+628) | +2.6% |
| stlinkv3mini/hid_controller | 22,736 → 23,348 (+612) | — | — | — | 24,204 → 24,816 (+612) | +2.5% |
| feather_stm32f405/midi2_host | 24,004 → 24,632 (+628) | — | — | — | 25,404 → 26,032 (+628) | +2.5% |
| stlinkv3mini/device_info | 23,504 → 24,116 (+612) | — | — | — | 25,096 → 25,708 (+612) | +2.4% |
| stlinkv3mini/bare_api | 24,092 → 24,704 (+612) | — | — | — | 25,772 → 26,384 (+612) | +2.4% |
| stlinkv3mini/midi_rx | 24,676 → 25,288 (+612) | — | — | — | 25,932 → 26,544 (+612) | +2.4% |
| stlinkv3mini/midi2_host | 25,136 → 25,748 (+612) | — | — | — | 26,664 → 27,276 (+612) | +2.3% |
There was a problem hiding this comment.
Pull request overview
This PR fixes DWC2 host DFIFO layout in the Synopsys DWC2 HCD driver by ensuring the host RxFIFO remains anchored at address 0 and by correctly sizing per-host-channel reserved words using the decoded host channel count.
Changes:
- Use
dwc2_channel_count()to size reserved DMA metadata space and per-channel RxFIFO status words. - Stop subtracting
rxfsizfrom the top-of-DFIFO allocator (so PTX/NPTX no longer get placed at address 0 and overlap RxFIFO).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Isochronous endpoints do not use the normal data-toggle sequence. Avoid saving or advancing HCTSIZ PID state on completion and retry so later transfers cannot be submitted with an invalid toggled PID. Signed-off-by: HiFiPHile <admin@hifiphile.com>
The core has already halted a periodic IN channel when every packet completes. Report the transfer at that point instead of requesting another halt interrupt, allowing the next service interval to be queued without delay. Signed-off-by: HiFiPHile <admin@hifiphile.com>
Anchor resubmitted periodic transfers to the endpoint service interval and defer early submissions through SOF. This prevents callback latency from shifting the cadence or causing intervals to be skipped, while keeping pending transfers abortable. Signed-off-by: HiFiPHile <admin@hifiphile.com>
A frame overrun ends an isochronous service opportunity without transferring data. Complete that interval with zero bytes so the class driver can queue the next packet instead of leaving the endpoint busy. Retry interrupt transfers normally and handle the condition for slave and DMA, IN and OUT paths.
Transfer completion releases channels and updates HAINTMSK in interrupt context while new transfers allocate channels from task context. Prevent the two read-modify-write sequences from losing an interrupt mask bit, which otherwise leaves a completed transfer permanently busy.
Popping an IN transfer-completion entry from GRXSTSP asserts HCINT.XferCompl. Drain the receive FIFO first, then read the live masked global status so the newly asserted channel completion is handled without waiting for another interrupt.
There was a problem hiding this comment.
🟡 Changes recommended
hcd_edpt_abort_xfer() has a confirmed race with the SOF ISR and returns success even when no transfer is aborted, which can break abort semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
| if (edpt->xfer_pending) { | ||
| edpt->xfer_pending = 0; | ||
| edpt->uframe_countdown = 0; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🟡 Changes recommended
There are confirmed correctness issues (frame-number masking width and a race in aborting deferred periodic transfers) that can mis-schedule transfers or allow aborted transfers to still start.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
src/portable/synopsys/dwc2/hcd_dwc2.c:799
- Clearing xfer_pending/uframe_countdown without an interrupt-critical section can race with the SOF ISR (which decrements uframe_countdown and can kick off the transfer). This can result in abort returning true while the deferred transfer still starts.
if (edpt->xfer_pending) {
edpt->xfer_pending = 0;
edpt->uframe_countdown = 0;
return true;
}
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Lite
This pull request introduces significant improvements and bug fixes to the periodic transfer scheduling and error handling logic in the DWC2 USB host controller driver. The changes improve the accuracy and reliability of periodic (interrupt and isochronous) transfers, especially in handling missed service intervals, transfer deferral, and completion. Additionally, several code cleanups and minor enhancements have been made.
Periodic Transfer Scheduling and Deferral Improvements:
periodic_phase,xfer_pending,periodic_frame) tohcd_endpoint_tto track the phase and deferral status of periodic transfers, and expandeduframe_intervalanduframe_countdownto 19 bits for better interval tracking.periodic_xfer_countdownandperiodic_xfer_defer. Transfers are now only kicked off when the correct interval is reached. [1] [2]Error Handling and Completion for Missed Periodic Intervals:
HCINT_FARME_OVERRUN(missed frame/interval) in both IN and OUT directions for both slave and DMA modes. Isochronous endpoints now report successful completion with empty data, while interrupt endpoints retry the transfer. [1] [2] [3] [4]General Code and Logic Enhancements:
HCD_FRAME_NUMBER_MASKfor safer frame number calculations and masking.Interrupt Handling Adjustments:
These changes collectively make periodic transfer scheduling more robust and reliable, particularly for isochronous and interrupt endpoints, and improve the driver's compliance with USB host controller requirements.