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.
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@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
>
> 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, ¶ms);
> - 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
