[PATCH v3 0/2] usb: dwc3: gadget: improve isoc handling

[PATCH v3 0/2] usb: dwc3: gadget: improve isoc handling

[Michael Grzeschik]

These two patches improve the isoc handling and make the dwc3 gadget
driver somewhat usable with the UVC gadget for isochronous endpoints.

The first patch makes starting isochronous transfers more reliable. I
think it's more less, what Thinh suggested some time ago[1]. It's still
not perfect because the first request must still be queued within 2
seconds but it's a lot better than the current situation.

The second patch makes it possible to have gaps in the data stream. The
UVC gadget relies on such behaviour. Without this, using the UVC gadget
with a live stream stops after the first frame that needs more time to
be scheduled.

[1] https://marc.info/?l=linux-usb&m=156088170723824&w=4

Michael Grzeschik (1):
  usb: dwc3: gadget: when the started list is empty stop the active xfer

Michael Olbrich (1):
  usb: dwc3: gadget: make starting isoc transfers more robust

 drivers/usb/dwc3/gadget.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH v3 1/2] usb: dwc3: gadget: make starting isoc transfers more robust

[Michael Grzeschik]

From: Michael Olbrich <m.olbrich@pengutronix.de>

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@pengutronix.de>
Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
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);
 
-- 
2.27.0

[PATCH v3 2/2] usb: dwc3: gadget: when the started list is empty stop the active xfer

[Michael Grzeschik]

When we have nothing left to be queued after handling the last trb
we have to stop the current transfer. This way we can ensure that
the next request will be queued with an new and valid timestamp
and will not directly run into an missed xfer.

Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

---
v1 -> v2: - This Patch replaces the following patch by Michael Olbrich:
            usb: dwc3: gadget: restart the transfer if a isoc request is queued too late
v2 -> v3: - rearranged condition for easier read (suggested by Tinh Nguyen)

 drivers/usb/dwc3/gadget.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 7ad85a7d06f70bf..2e15878aa8ae781 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2735,7 +2735,9 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
 	if (dep->flags & DWC3_EP_END_TRANSFER_PENDING)
 		goto out;
 
-	if (status == -EXDEV && list_empty(&dep->started_list))
+	if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+		list_empty(&dep->started_list) &&
+		(list_empty(&dep->pending_list) || status == -EXDEV))
 		dwc3_stop_active_transfer(dep, true, true);
 	else if (dwc3_gadget_ep_should_continue(dep))
 		if (__dwc3_gadget_kick_transfer(dep) == 0)
-- 
2.27.0

Re: [PATCH v3 1/2] usb: dwc3: gadget: make starting isoc transfers more robust

[Thinh Nguyen]

Hi,

Michael Grzeschik wrote:
> From: Michael Olbrich <m.olbrich@pengutronix.de>
>
> 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@pengutronix.de>
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>
> ---
> 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.

For both patches in this series:
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>

Thanks,
Thinh

Re: [PATCH v3 1/2] usb: dwc3: gadget: make starting isoc transfers more robust

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 4932 bytes --]

On Mon, Jun 29, 2020 at 09:20:02PM +0000, Thinh Nguyen wrote:
>Hi,
>
>Michael Grzeschik wrote:
>> From: Michael Olbrich <m.olbrich@pengutronix.de>
>>
>> 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@pengutronix.de>
>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>
>> ---
>> 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?

>For both patches in this series:
>Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
>
>Thanks,
>Thinh

Thanks!
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/2] usb: dwc3: gadget: make starting isoc transfers more robust

[Thinh Nguyen]

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@pengutronix.de>
>>>
>>> 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@pengutronix.de>
>>> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>>> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>>>
>>> ---
>>> 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

Re: [PATCH v3 1/2] usb: dwc3: gadget: make starting isoc transfers more robust

[Peter Chen]

On Tue, Jun 30, 2020 at 3:15 PM Michael Grzeschik <mgr@pengutronix.de> wrote:
>
> On Mon, Jun 29, 2020 at 09:20:02PM +0000, Thinh Nguyen wrote:
> >Hi,
> >
> >Michael Grzeschik wrote:
> >> From: Michael Olbrich <m.olbrich@pengutronix.de>
> >>
> >> 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@pengutronix.de>
> >> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> >>
> >> ---
> >> 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);

Hi Michael,

I can't find the above code for chipidea, where did you get?
The frame index value register is a standard EHCI register,
but for UDC, it is not a common register. UDC design is varied
for vendors.

Peter

>
> 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?
>
> >For both patches in this series:
> >Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
> >
> >Thanks,
> >Thinh
>
> Thanks!
> Michael
>
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 1/2] usb: dwc3: gadget: make starting isoc transfers more robust

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 6670 bytes --]

Hi Peter,

On Wed, Jul 01, 2020 at 10:26:56AM +0800, Peter Chen wrote:
>On Tue, Jun 30, 2020 at 3:15 PM Michael Grzeschik <mgr@pengutronix.de> wrote:
>>
>> On Mon, Jun 29, 2020 at 09:20:02PM +0000, Thinh Nguyen wrote:
>> >Hi,
>> >
>> >Michael Grzeschik wrote:
>> >> From: Michael Olbrich <m.olbrich@pengutronix.de>
>> >>
>> >> 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@pengutronix.de>
>> >> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
>> >> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
>> >>
>> >> ---
>> >> 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);
>
>Hi Michael,
>
>I can't find the above code for chipidea, where did you get?

I was greping my usb-wip stack where I have an extra
Patch for the get_frame callback implementation
of the chipidea/udc.c. I think I will have to post it.

>The frame index value register is a standard EHCI register,
>but for UDC, it is not a common register. UDC design is varied
>for vendors.

Good to know. Is there any measure to find out if it is
there? I would add that limitation to the above mentioned patch.

Thanks,
Michael

>>
>> 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?
>>
>> >For both patches in this series:
>> >Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
>> >
>> >Thanks,
>> >Thinh
>>
>> Thanks!
>> Michael
>>
>> --
>> Pengutronix e.K.                           |                             |
>> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
>> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v3 1/2] usb: dwc3: gadget: make starting isoc transfers more robust

[Peter Chen]

  
> >> drivers/usb/chipidea/udc.c
> >> 1634:   ret = hw_read(ci, OP_FRINDEX, 0x3fff);
> >
> >Hi Michael,
> >
> >I can't find the above code for chipidea, where did you get?
> 
> I was greping my usb-wip stack where I have an extra Patch for the get_frame
> callback implementation of the chipidea/udc.c. I think I will have to post it.
> 

Welcome to that.

> >The frame index value register is a standard EHCI register, but for
> >UDC, it is not a common register. UDC design is varied for vendors.
> 
> Good to know. Is there any measure to find out if it is there? I would add that
> limitation to the above mentioned patch.
> 

Sorry, I can't get your point. My point was 0x3fff is the mask for Frame Index Register
for standard EHCI controller, but this value may not the same meaning
for other controllers.  So, I suggest only defining for dwc3 in your patch.

Peter

> Thanks,
> Michael
> 
> >>
> >> 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?
> >>
> >> >For both patches in this series:
> >> >Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>
> >> >
> >> >Thanks,
> >> >Thinh
> >>
> >> Thanks!
> >> Michael
> >>
> >> --
> >> Pengutronix e.K.                           |                             |
> >> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> >> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> >
> 
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 1/2] usb: dwc3: gadget: make starting isoc transfers more robust

[Michael Grzeschik]

[-- Attachment #1: Type: text/plain, Size: 1443 bytes --]

On Wed, Jul 01, 2020 at 08:45:19AM +0000, Peter Chen wrote:
>
>> >> drivers/usb/chipidea/udc.c
>> >> 1634:   ret = hw_read(ci, OP_FRINDEX, 0x3fff);
>> >
>> >Hi Michael,
>> >
>> >I can't find the above code for chipidea, where did you get?
>>
>> I was greping my usb-wip stack where I have an extra Patch for the get_frame
>> callback implementation of the chipidea/udc.c. I think I will have to post it.
>>
>
>Welcome to that.
>
>> >The frame index value register is a standard EHCI register, but for
>> >UDC, it is not a common register. UDC design is varied for vendors.
>>
>> Good to know. Is there any measure to find out if it is there? I would add that
>> limitation to the above mentioned patch.
>>
>
>Sorry, I can't get your point. My point was 0x3fff is the mask for Frame Index Register
>for standard EHCI controller, but this value may not the same meaning
>for other controllers.  So, I suggest only defining for dwc3 in your patch.

Yes I got that now, I somehow missread it for being also not common in
chipidea udc versions. Now that it is clear, I can post the patch.

Thanks,
Michael

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]