-
Notifications
You must be signed in to change notification settings - Fork 1.3k
dcd/dwc2: support ISO IN transfer when bInterval > 1 #3275
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.
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances the DWC2 USB device controller driver to properly support isochronous IN transfers when the bInterval is greater than 1, which is commonly used in audio applications. The changes address hardware limitations where the Even/Odd frame toggling mechanism needs special handling for different DWC2 controller variants.
Key changes include:
- Enhanced ISO IN transfer scheduling to work with any bInterval value
- Added incomplete isochronous transfer interrupt handling for robust error recovery
- Optimized single-packet transfers by writing directly to FIFO without enabling TX empty interrupts
dwc2 requires manually toggle Even/Odd bit manually for ISO IN transfer, that's poses a problem when bInterval > 1 mainly for audio class, as the moment the transfer is scheduled, we don't know when the host will issue IN token (bInterval vs bRefresh schenanigans). Linux driver use NAK interrupt to detect when the host is sending IN token and toggle the Even/Odd bit accordingly based on the current frame number and bInterval. However on ST's stripped down DWC2 FS controller (e.g STM32F4, STM32F7), NAK interrupt is not supported, even it's marked as always present in DWC2 databook. NAK interrupt is only supported on HS controller with external PHY. Instead I schedule all ISO IN transfer for next frame, if the transfer failed, incomplete isochronous IN transfer interrupt will be triggered and we can relaunch the transfer. Signed-off-by: HiFiPhile <admin@hifiphile.com>
94cc340 to
f8690ab
Compare
|
@codex review this pr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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
f8690ab to
e0a589c
Compare
|
@hathach |
|
@HiFiPhile retrying iso with flush and re-write packet seems a bit heavy, with high enough bInterval we may end up doing it mostly every frame. Since dwc2 requires I think in the future we have to improve how we manage ISO transfer e.g
|
That's what should be done ideally (kernel does something similar), but in real it's not always bInterval who decides the transfer interval. I wrote an explanation here: https://github.com/HiFiPhile/tinyusb/blob/9f1a86c0cbf2974775687848a95a126c6503c1cf/examples/device/uac2_speaker_fb/src/usb_descriptors.h#L137 |
|
Thanks @HiFiPhile for the detail info, I think we should follow the bInterval, I don't understand UAC1, but I think bInterval is the hard limit between the ISO transfer, while bRefresh is probably more flexible maybe bRefresh = n*bInterval. Mabye the class driver can have both with the same value to make every OS happy. For dwc2 driver, I think sticking with bInterval is easier and cleaner. |
|
@hathach I remembered why we can't do scheduling with bInterval, due to Nak interrupt is unavailable on some devices (OTG_FS). Nak interrupt is needed in order to align the frame number with the host. For example when bInterval=4 kernel does this:
Without knowing the starting frame, if we schedule the frame on 3,7,11,15 then the transfer won't work. Also Nak is a global interrupt which consumes a lot of CPU. Actually instead of retry 255 times (max bInerval allowed), we can limit to actual bInerval value. For UAC1 if bRefresh >0 it will generate some transfer failed event but harmless. I figured out how to restart the transfer without disable/reenable the endpoint. |
Signed-off-by: HiFiPhile <admin@hifiphile.com>
4f76c00 to
b86a2e5
Compare
b86a2e5 to
a2af502
Compare
a2af502 to
cd14dd2
Compare
Signed-off-by: HiFiPhile <admin@hifiphile.com>
cd14dd2 to
beea882
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gates Failed
Prevent hotspot decline
(1 hotspot with Complex Method, Complex Conditional)
Enforce advisory code health rules
(1 file with Complex Method, Complex Conditional)
Gates Passed
2 Quality Gates Passed
See analysis details in CodeScene
Reason for failure
| Prevent hotspot decline | Violations | Code Health Impact | |
|---|---|---|---|
| dcd_dwc2.c | 2 rules in this hotspot | 6.63 → 6.21 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| dcd_dwc2.c | 2 advisory rules | 6.63 → 6.21 | Suppress |
Quality Gate Profile: Clean Code Collective
Want more control? Customize Code Health rules or catch issues early with our IDE extension and CLI tool.
|
@HiFiPhile does it need to use global NAK, I see there is DIEPINTi bit 13 that is available in stm32f4.
|
|
@hathach It's only available for OTG_HS, OTG_FS doesn't have it . |
|
@HiFiPhile which mcu in your screenshot, mine is otg fs stm32f405/407
|
|
@HiFiPhile yeah, I think using incomplete iso interrupt is much better than NAK. I guess maybe this interrupt is probably introduced in later version of dwc2 where NAK is already implemented. Also I think we only need to do this once per ISO activate for syncing the first, however we cannot trust host to keep the bInterval right, so yeah sync target/last frame numb everytime ISO is transfer complete or incomplete is great idea. |
|
@hathach can we merge it ? |
|
@HiFiPhile oops, I thought there is still more changes to the PR. Didn't know that it is all implemented, give me a bit of time to do some basic testing. |
hathach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HiFiPhile PR stil haven't implemented the target frame yet. So the application need to call the edpt_xfer() 1 frame before the bInterval, otherwise we will miss that IN token and send data in the next bInterval (late timing) ?
I think in the future we have to improve how we manage ISO transfer e.g
- iso transfer --> compute target frame_num = last xferred frame_num + bInterval
- enable sof interrupt, loop for iso in check target frame, if there is transfer next --> write data, set odd/even bit.
- iso incomplete still requires, but I guess host will mostly respect the iso bInterval so the number of rety would be much less often.
- sof interrupt can be disabled when there is no active iso transfer.
| // Read DSTS and DIEPCTLn for all isochronous endpoints. If the current EP is enabled | ||
| // and the read value of DSTS.SOFFN is the targeted uframe number for this EP, then | ||
| // this EP has an incomplete transfer. | ||
| if (depctl.enable && depctl.type == DEPCTL_EPTYPE_ISOCHRONOUS && depctl.dpid_iso_odd == odd_now) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any chances that 2 ISO is schedule at the same uframe, and 1 is complete and 1 is incomplete and we detect the wrong endpoint ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hathach Both audio and video are real-time streaming data it's ok to return transfer failed if the host didn't read the data in the specified interval. edpt_xfer() needs to be called not earlier than bInterval*frames before the IN token:
|
|
@HiFiPhile thanks for the explanation, I know ISO data can be missed. Let's say bInterval = 10, and we recevie setInterface at frame=100, assume host send the 1st IN token as well. So we will have
So ideally video/audio must prepare the next iso data within the bInterval and call edpt_xfer() it as soon as the data is ready. let's say at frame 105th, the stack would then ideally only fetch data to fifo at frame 109. with the current implementation, what would happen at frame 106,107 ? |
@hathach If edpt_xfer is called at 105th data will be pushed into fifo and the transfer will be scheduled for 106th. At 106th incomplete interrupt fires and rearm the endpoint for 107th. At 107th same thing happens, until the transfer succeed in 110th. It's the procedure of step 5 chapter 8.2.5, except the guild forgot to mention that DIEPTSIZ needs to be rewritten (how I failed to get it work initially) |
perfect explanation, also thanks for citation. Each time I open the guide, it feel like completely new to me |
hathach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excelllent PR as always. Thank you very much, I extract the function into its own handle_() to make it easier to read.






Isochronous IN transfer handling:
dwc2 requires manually toggle Even/Odd bit manually for ISO IN transfer, that's poses a problem when bInterval > 1 mainly for audio class, as the moment the transfer is scheduled, we don't know when the host will issue IN token (bInterval vs bRefresh shenanigans).
Linux driver use NAK interrupt to detect when the host is sending IN token and toggle the Even/Odd bit accordingly based on the current frame number and bInterval.
However on ST's stripped down DWC2 FS controller (e.g STM32F4, STM32F7), NAK interrupt is not supported, even it's marked as always present in DWC2 databook. NAK interrupt is only supported on HS controller.
Instead I schedule all ISO IN transfer for next frame, if the transfer failed, incomplete isochronous IN transfer interrupt will be triggered and we can relaunch the transfer.
Fix some issues in #3270