Hi, Michael Olbrich wrote: > 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> > --- > drivers/usb/dwc3/core.h | 1 + > drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++-------- > 2 files changed, 24 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 3dd783b889cb..c5b223656e08 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -709,6 +709,7 @@ struct dwc3_ep { > u8 type; > u8 resource_index; > u32 frame_number; > + u32 last_frame_number; There's no need to add a new field for last_frame_number. Just store the value in a local variable in __dwc3_gadget_start_isoc(). > u32 interval; > > char name[20]; > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 173f5329d3d9..ac4673d91939 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(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) > { > struct dwc3_gadget_ep_cmd_params params; > struct dwc3_request *req; > @@ -1242,7 +1242,7 @@ 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)) { > /* > * FIXME we need to iterate over the list of requests > * here and stop, unmap, free and del each of the linked > @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) > return ret; > } > > - return 0; > + return ret; > } > > static int __dwc3_gadget_get_frame(struct dwc3 *dwc) > @@ -1377,7 +1377,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) > @@ -1402,9 +1402,23 @@ 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); > + /* > + * last_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 > + * last_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); > + > + dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame; > + if (frame < (dep->last_frame_number & 0x3fff)) > + dep->frame_number += 0x4000; Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We started using 0x3fff in multiple places now, can we create a macro for that? Also, add an empty line here. > + 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; > } > @@ -1461,7 +1475,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, > @@ -2467,7 +2481,7 @@ static int dwc3_gadget_ep_cleanup_completed_request(struct dwc3_ep *dep, > > if (!dwc3_gadget_ep_request_completed(req) && > req->num_pending_sgs) { > - __dwc3_gadget_kick_transfer(dep); > + __dwc3_gadget_kick_transfer(dep, false); > goto out; > } > > @@ -2497,6 +2511,7 @@ static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, > const struct dwc3_event_depevt *event) > { > dep->frame_number = event->parameters; > + dep->last_frame_number = event->parameters; > } > > static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, Other than the comments I provided, this patch looks fine to me. BR, Thinh