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

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

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.

Did you try with the recent update for isoc? (e.i., after 5 
START_TRANSFER failures, the driver will issue END_TRANSFER to retry on 
the next XferNotReady event)

Let me know if you still run into issues with that.

>
> 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
>
>   drivers/usb/dwc3/gadget.c | 37 +++++++++++++++++++++++++------------
>   1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 421a0f73110a40b..0962ddd7fbf6ae6 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1276,7 +1276,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
>   
>   static void dwc3_gadget_ep_cleanup_cancelled_requests(struct dwc3_ep *dep);
>   
> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req)

Any reason to have keep_req? With the recent changes, if 
dwc3_send_gadget_ep_cmd() returns -EAGAIN, then the controller driver 
won't give back the request.

>   {
>   	struct dwc3_gadget_ep_cmd_params params;
>   	struct dwc3_request		*req;
> @@ -1314,12 +1314,9 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>   	}
>   
>   	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> -	if (ret < 0) {
> +	if (ret < 0 && (!keep_req || ret != -EAGAIN)) {
>   		struct dwc3_request *tmp;
>   
> -		if (ret == -EAGAIN)
> -			return ret;
> -
>   		dwc3_stop_active_transfer(dep, true, true);
>   
>   		list_for_each_entry_safe(req, tmp, &dep->started_list, list)
> @@ -1335,7 +1332,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>   	if (dep->stream_capable && req->request.is_last)
>   		dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE;
>   
> -	return 0;
> +	return ret;
>   }
>   
>   static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
> @@ -1458,7 +1455,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
>   	dep->start_cmd_status = 0;
>   	dep->combo_num = 0;
>   
> -	return __dwc3_gadget_kick_transfer(dep);
> +	return __dwc3_gadget_kick_transfer(dep, false);
>   }
>   
>   static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
> @@ -1481,9 +1478,25 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>   	}
>   
>   	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
> -		dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
> +		/*
> +		 * 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.
> +		 */
> +		u32 frame = __dwc3_gadget_get_frame(dwc);
> +		bool rollover = frame < (dep->frame_number & 0x3fff);
> +
> +		dep->frame_number = (dep->frame_number & ~0x3fff) | frame;
> +		if (rollover)
> +			dep->frame_number += BIT(14);
> +
> +		dep->frame_number = DWC3_ALIGN_FRAME(dep, 1);
>   
> -		ret = __dwc3_gadget_kick_transfer(dep);
> +		ret = __dwc3_gadget_kick_transfer(dep,
> +				i + 1 < DWC3_ISOC_MAX_RETRIES);
>   		if (ret != -EAGAIN)
>   			break;
>   	}
> @@ -1567,7 +1580,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
>   		}
>   	}
>   
> -	return __dwc3_gadget_kick_transfer(dep);
> +	return __dwc3_gadget_kick_transfer(dep, false);
>   }
>   
>   static int dwc3_gadget_ep_queue(struct usb_ep *ep, struct usb_request *request,
> @@ -2719,7 +2732,7 @@ static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep,
>   	if (status == -EXDEV && list_empty(&dep->started_list))
>   		dwc3_stop_active_transfer(dep, true, true);
>   	else if (dwc3_gadget_ep_should_continue(dep))
> -		if (__dwc3_gadget_kick_transfer(dep) == 0)
> +		if (__dwc3_gadget_kick_transfer(dep, false) == 0)
>   			no_started_trb = false;
>   
>   out:
> @@ -2904,7 +2917,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
>   			dwc3_gadget_ep_cleanup_cancelled_requests(dep);
>   			if ((dep->flags & DWC3_EP_DELAY_START) &&
>   			    !usb_endpoint_xfer_isoc(dep->endpoint.desc))
> -				__dwc3_gadget_kick_transfer(dep);
> +				__dwc3_gadget_kick_transfer(dep, false);
>   
>   			dep->flags &= ~DWC3_EP_DELAY_START;
>   		}

BR,
Thinh