dwc2 postfixes#3666
Conversation
There was a problem hiding this comment.
Pull request overview
Adjusts the DWC2 device controller (DCD) EP0 SETUP handling in DMA mode to cope with back-to-back SETUP packets by allocating room for multiple SETUP packets and selecting the most recent packet via DOEPDMA0, aligning behavior with the DWC2 manual’s guidance.
Changes:
- Expand the EP0 SETUP DMA buffer from 8 bytes to 24 bytes (3 packets).
- Program EP0 OUT to receive back-to-back SETUP packets (via
STUPCNT). - Use
DOEPDMA0to locate and forward the latest SETUP packet to the TinyUSB stack.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: HiFiPhile <admin@hifiphile.com>
|
| target | .text | .rodata | .data | .bss | total | % diff |
|---|---|---|---|---|---|---|
| raspberrypi_zero/mtp | 61,528 → 61,524 (-4) | — | — | — | 68,851 → 68,847 (-4) | -0.0% |
|
@claude why your auto review does not work with this PR, it is not from fork |
|
Claude finished @hathach's task in 3m 32s —— View job Auto-review Investigation
The auto-review did actually run on this PR. Here's what I found: Workflow run: Claude Code Review #26847730488
Why you may not see a review comment: The job completed, but there was 1 permission denial during the Claude session. This likely caused the sticky comment ( The
To diagnose further, you could temporarily add To re-trigger now (without changing the workflow), push a no-op commit to |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
TinyUSB Average Code Size Metrics
Input files
|
|
@codex review |
|
@claude review |
|
Claude finished @hathach's task in 3m 33s —— View job Code Review: dwc2 postfixes
SummaryThe PR fixes EP0 SETUP handling in DWC2 DMA mode: the hardware always buffers up to 3 back-to-back SETUP packets before firing Issues1.
// new
dwc2->epout[0].doeptsiz = (3 << DOEPTSIZ_STUPCNT_Pos); // PKTCNT=0, XFRSIZ=0
// old
dwc2->epout[0].doeptsiz = (1 << DOEPTSIZ_STUPCNT_Pos) | (1 << DOEPTSIZ_PKTCNT_Pos) | (8 << DOEPTSIZ_XFRSIZ_Pos);The DWC2 databook distinguishes SETUP packet reception (controlled by That said, Linux's Suggestion: either add 2. No bounds check before
tusb_control_request_t *setup_packet = (tusb_control_request_t *) (epout0->doepdma - 8);If TU_ASSERT(epout0->doepdma >= (uintptr_t)_dcd_usbbuf.setup_buffer + 8, );
tusb_control_request_t *setup_packet = (tusb_control_request_t *) (epout0->doepdma - 8);3.
if (setup_packet->wLength == 0) {
if (tu_le16toh(setup_packet->wLength) == 0) {Positive Changes
The core logic is correct and the test evidence (3/4 back-to-back SETUP packets handled) gives confidence. Items 1 and 2 are worth addressing before merge. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5e080732e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Receive only 1 packet | ||
| dwc2->epout[0].doeptsiz = (1 << DOEPTSIZ_STUPCNT_Pos) | (1 << DOEPTSIZ_PKTCNT_Pos) | (8 << DOEPTSIZ_XFRSIZ_Pos); | ||
| // Receive back-to-back setup packets | ||
| dwc2->epout[0].doeptsiz = (3 << DOEPTSIZ_STUPCNT_Pos); |
There was a problem hiding this comment.
On internal-DMA DWC2 parts this arms EP0 with XFRSIZ=0 and PKTCNT=0, because the assignment now writes only STUPCNT. The DWC2/STM32 EP0 OUT start sequence still needs a nonzero packet count and transfer size for the setup DMA buffer (ST's USB_EP0_OutStart() programs PKTCNT=1, XFRSIZ=3*8, and STUPCNT); otherwise the core is enabled for a zero-length OUT transfer and can fail to DMA/complete the first SETUP packet, breaking enumeration in DMA mode.
Useful? React with 👍 / 👎.
Cast DOEPDMA0 through uintptr_t and use sizeof(tusb_control_request_t) instead of the magic constant 8, matching project convention. Add a reference to Programming Guide v4.20a 9.1.2.1 for the DOEPDMAn-8 rule. Addresses Copilot review comment; no functional change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review triage (Codex + Copilot)Thanks for the reviews. Cross-checked each against the Synopsys DWC2 USB 2.0 HS-OTG Databook / Programming Guide v4.20a. P1 —
The SETUP DMA write to memory is governed by
Pointer cast / magic |
hathach
left a comment
hathach
left a comment
There was a problem hiding this comment.
perfect, thank you @HiFiPhile . Sorry for the noise with AI bot, I am testing it out with non-fokred contributor to see if it works (may only work with my own pr). These bots can be useful for trivial fix and testing.
PS: My hil pool is growing large and seem like either kernel usb has issues with large amount of devices at the same time or I got issue with power (overcurrent), re-run hil a few times with less active boards works.
Even if
STUPCNT=1the DMA always receives 3 SETUP packets before address rewind, a buffer of 3 packets is needed.Follow the manual to use
DOEPDMA0to get the latest packet.For testing it's not easy with PC, actually I hacked USBH to send 3/4 SETUP packets:
