Hi,
Michael Grzeschik wrote:
> On Mon, Jun 29, 2020 at 09:20:02PM +0000, Thinh Nguyen wrote:
>> Hi,
>>
>> Michael Grzeschik wrote:
>>> From: Michael Olbrich <m.olbrich@xxxxxxxxxxxxxx>
>>>
>>> Currently __dwc3_gadget_start_isoc must be called very shortly after
>>> XferNotReady. Otherwise the frame number is outdated and start transfer
>>> will fail, even with several retries.
>>>
>>> DSTS provides the lower 14 bit of the frame number. Use it in
>>> combination
>>> with the frame number provided by XferNotReady to guess the current
>>> frame
>>> number. This will succeed unless more than one 14 rollover has happened
>>> since XferNotReady.
>>>
>>> Start transfer might still fail if the frame number is increased
>>> immediately after DSTS is read. So retries are still needed.
>>> Don't drop the current request if this happens. This way it is not
>>> lost and
>>> can be used immediately to try again with the next frame number.
>>>
>>> With this change, __dwc3_gadget_start_isoc is still not successfully
>>> in all
>>> cases bit it increases the acceptable delay after XferNotReady
>>> significantly.
>>>
>>> Signed-off-by: Michael Olbrich <m.olbrich@xxxxxxxxxxxxxx>
>>> Signed-off-by: Michael Tretter <m.tretter@xxxxxxxxxxxxxx>
>>> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
>>>
>>> ---
>>> v1 -> v2: - removed last_frame_number from struct
>>> - included rollover variable
>>> v2 -> v3: - moved the calculation before the retry loop
>>> - skipping the calculation if bInterval > 14
>>>
>>> drivers/usb/dwc3/gadget.c | 19 +++++++++++++++++++
>>> 1 file changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 8de6f10d335e1c0..7ad85a7d06f70bf 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1463,6 +1463,7 @@ static int dwc3_gadget_start_isoc_quirk(struct
>>> dwc3_ep *dep)
>>>
>>> static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>> {
>>> + const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
>>> struct dwc3 *dwc = dep->dwc;
>>> int ret;
>>> int i;
>>> @@ -1480,6 +1481,24 @@ static int __dwc3_gadget_start_isoc(struct
>>> dwc3_ep *dep)
>>> return dwc3_gadget_start_isoc_quirk(dep);
>>> }
>>>
>>> + if (desc->bInterval <= 14) {
>>> + u32 frame = __dwc3_gadget_get_frame(dwc);
>>> + bool rollover = frame < (dep->frame_number & 0x3fff);
>>> +
>>> + /*
>>> + * frame_number is set from XferNotReady and may be already
>>> + * out of date. DSTS only provides the lower 14 bit of the
>>> + * current frame number. So add the upper two bits of
>>> + * frame_number and handle a possible rollover.
>>> + * This will provide the correct frame_number unless more than
>>> + * rollover has happened since XferNotReady.
>>> + */
>>> +
>>> + dep->frame_number = (dep->frame_number & ~0x3fff) | frame;
>>> + if (rollover)
>>> + dep->frame_number += BIT(14);
>>> + }
>>> +
>>> for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>>> dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
>>>
>>
>> I think it's cleaner to create a mask for 0x3fff, but I can see how it
>> can arguably clearer for not using a macro also. It's fine to me
>> either way.
>
> This mask is used on many usb controllers and
> everyone is defining their own.
>
> drivers/usb/gadget/udc/mv_udc.h
> 34:#define USB_FRINDEX_MASKS 0x3fff
>
> drivers/usb/gadget/udc/fsl_usb2_udc.h
> 116:#define USB_FRINDEX_MASKS 0x3fff
>
>
> Even other drivers using the mask without defining it:
>
> drivers/usb/misc/ftdi-elan.c
> 2224: hc_fminterval &= 0x3fff;
> 2313: ((9 *hc_fminterval) / 10) & 0x3fff);
>
> drivers/usb/chipidea/udc.c
> 1634: ret = hw_read(ci, OP_FRINDEX, 0x3fff);
>
> drivers/usb/dwc3/gadget.c
> 1406: test_frame_number = dep->frame_number & 0x3fff;
> 1453: dep->frame_number &= 0x3fff;
>
> drivers/usb/host/ohci-hcd.c
> 557: ohci->fminterval = val & 0x3fff;
>
> drivers/usb/host/xhci-ring.c
> 3980: start_frame &= 0x3fff;
>
> drivers/usb/host/ohci-dbg.c
> 635: rdata & 0x3fff);
> 641: rdata & 0x3fff);
> 647: rdata & 0x3fff);
>
> drivers/usb/host/u132-hcd.c
> 1535: ((9 * fi) / 10) & 0x3fff);
> 1605: u132->hc_fminterval = temp & 0x3fff;
>
> drivers/usb/host/ohci.h
> 701: ohci_writel (ohci, ((9 * fi) / 10) & 0x3fff,
>
> I could write a patch changing them all or at least
> make a common gadget define.
>
> What could be a common place for that?
>
> On device level only it could be include/linux/usb/gadget.h
> On whole usb level this could be include/uapi/linux/usb/ch9.h
>
> Ideas?
>
It maybe common for usb controllers to report frame number as a 14-bit
value (probably because of SOF/ITP packet format). But different
controller may behave differently here. For DWC3 controller, this is
read from the DSTS register, and it doesn't have to follow any standard.
Only DWC3 driver knows the format (e.g., if the device is full-speed,
DSTS.SOFFN is only valid as a 10 bit frame number and not uframe). So,
we shouldn't create a common mask for all controllers.
Talking about full speed, you should also ignore this calculation if the
device is in full-speed. Please make a fix in v4.
With full-speed, there should be plenty of time for the system to handle
XferNotReady since it's in frame and not uframe (with retry still apply
of course).
BR,
Thinh
