usb: device_next: uac2: Support configurable polling rates #2910
Conversation
|
none Note: This comment is automatically posted and updated by the Contribs GitHub Action. |
vbrzeski
force-pushed
the
uac2-binterval
branch
2 times, most recently
from
1bcbd0b to
d66bfbe
Compare
| Input or output type reports polling period in microseconds. For USB full | ||
| speed this could be clamped to 1ms or 255ms depending on the value. |
There was a problem hiding this comment.
Isochronous endpoint bInterval range 1 to 255 ms clamping is not valid.
For devices complying with USB 1.x specification, isochronous endpoint bInterval must be 1 (see Universal Serial Bus Specification Revision 1.1 Table 9-10. Standard Endpoint Descriptor bInterval).
For devices complying with USB 2 specification be it Full-Speed or High-Speed, isochronous endpoint bInterval range is from 1 to 16 where bInterval is used as exponent 2**(bInterval-1) (see Universal Serial Bus Specification Revision 2.0 Table 9-13. Standard Endpoint Descriptor bInterval). Therefore the largest interval in ms for Full-Speed isochronous endpoint is 2**(16-1)=32768 ms (and for High-Speed it is (2**(16-1))/8=4096 ms).
There was a problem hiding this comment.
This same approach is taken in the HID driver, but will take another look :)
There was a problem hiding this comment.
Interrupt endpoints are defined differently. See bInterval field description in Table 9-13. Standard Endpoint Descriptor in Universal Serial Bus Specification Revision 2.0.
Isochronous endpoints do have the power-of-two both at Full-Speed and High-Speed, while interrupt has 1 to 255 at Full-Speed and power-of-two at High-Speed. I believe this is inherited from USB 1.x due to backwards compatibility. In USB 1.x isochronous endpoints must have bInterval=1 while interrupt endpoints were using 1 to 255. Therefore without breaking backwards compatibility it was possible to add the power-of-two to isochronous endpoints (the only allowed value was 1 which has the same behavior in both USB 1.x and USB 2 Full-Speed operation), but not for interrupt endpoints.
vbrzeski
force-pushed
the
uac2-binterval
branch
6 times, most recently
from
9eeb868 to
616e9c5
Compare
Don't commit this, this is a proof of concept for now.
|
@tmon-nordic can you please take a look at the 2nd commit as without these changes, the bInterval change is moot: With this PR, we have acceptable performance for standard audio, though the changes in this commit are significant, and a little hacky. The most recent change is working well, though I had to deviate from the Linux gadget implementation given the "OUT Token on Disabled Endpoint" IRQ doesn't work for me. All the documentation for other MCU families with this USB Controller suggests this IRQ will only trigger for EP0. Otherwise, the changes are a hacky grafting of this linux driver: |
|
|
|
||
| return (priv->enumspd == USB_DWC2_DSTS_ENUMSPD_HS3060) ? | ||
| USB_DWC2_DSTS_SOFFN_LIMIT : | ||
| USB_DWC2_DSTS_SOFFN_LIMIT >> 3; |
There was a problem hiding this comment.
There is no need, and in fact it is wrong, to divide SOFFN by 8 at Full-Speed. SOFFN contains microframe number when operating at High-Speed and Frame number when operating at Full-Speed.
| return (priv->enumspd == USB_DWC2_DSTS_ENUMSPD_HS3060) ? | ||
| (1U << (cfg->interval - 1)) : | ||
| cfg->interval; |
There was a problem hiding this comment.
This is not standard compliant. It is valid for High-Speed, but not valid for Full-Speed. Using the same equation for High-Speed and Full-Speed will be standard compliant.
| sys_clear_bits((mem_addr_t)&base->dctl, USB_DWC2_DCTL_SFTDISCON); | ||
|
|
||
| // Disable frame number thing (out of curiousity) | ||
| // sys_clear_bits((mem_addr_t)&base->dctl, USB_DWC2_DCTL_IGNRFRMNUM); |
There was a problem hiding this comment.
IgnrFrmNum value after reset is 0, so clearing this bit has no effect. If you want to enable Periodic Transfer Interrupt then this bit should be set.
Note that this is only valid in the DMA modes (Buffer DMA or Descriptor DMA) and it is not valid in Completer mode. When IgnrFrmNum is set to 1, then IncompISOCIN interrupt should be masked (GINTMSK register should not have USB_DWC2_GINTSTS_INCOMPISOIN set).
|
|
||
| sys_write32(doepmsk, (mem_addr_t)&base->doepmsk); | ||
| sys_set_bits((mem_addr_t)&base->diepmsk, USB_DWC2_DIEPINT_XFERCOMPL); | ||
| sys_set_bits((mem_addr_t)&base->diepmsk, USB_DWC2_DIEPINT_XFERCOMPL | USB_DWC2_DIEPINT_NAKINTRPT); |
There was a problem hiding this comment.
This will be performance killer if e.g. CDC ACM is used. On Linux xHCI host, opening ttyACM instance for given CDC ACM results in 5 IN/NAK every microframe.
| uint32_t doepmsk; | ||
|
|
||
| /* Set the NAK bit for all OUT endpoints */ | ||
| sys_clear_bits((mem_addr_t)&base->diepmsk, USB_DWC2_DIEPINT_NAKINTRPT); |
There was a problem hiding this comment.
Why clear bit to later set it?
|
|
||
| doeptsiz = sys_read32((mem_addr_t)&base->out_ep[ep_idx].doeptsiz); | ||
|
|
||
| // HACK: since it seems like OUTTKNEPDIS doesn't work for non-control endpoints, |
There was a problem hiding this comment.
I have no idea how the isochronous endpoint synchronization mechanism using OUTTKNEPDIS works/ever worked in Linux. If anything, that would be abusing some undocumented behavior because DWC2 documentation is clear that this applies only to control OUT endpoints.
There was a problem hiding this comment.
In my testing, it never seems to trigger. So I'm at a loss there. My next steps would be to trace ISO in the Linux kernel, but I'm losing faith in it as the Bespoke defacto driver.
| dwc2_handle_out_xfercompl(dev, n); | ||
| } | ||
|
|
||
| // Why does this work for linux gadget when docs suggest its only for control EP |
There was a problem hiding this comment.
However weird it may sound, are you sure that this part of the Linux gadget driver really works as described in comments?
|
Going to address your feedback, cleanup the simpler commit, and create a PR for github. |
3 participants
The UAC2 device class doesn't support configurable polling rates.