Hi! On Wed, Jun 24, 2020 at 07:26:42PM +0000, Thinh Nguyen wrote: >Michael Grzeschik wrote: >> From: Michael Olbrich >> >> 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. Yes. Without my patch I still see the issue. Event with the retry machanism. It is even worse. I even missed one additional patch, I had on top this one. See my note below. >> 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 >> Signed-off-by: Michael Tretter >> Signed-off-by: Michael Grzeschik >> >> --- >> 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. Yes, we don't need the keep_req. I tried this and it worked without. I will remove the parameter in v3. >> { >> 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); This is not enough, we have to add i into the alignment to ensure that the stream is not failing: dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 2); I am even thiking to move the frame number calculation into the DWC3_DEPCMD_STARTTRANSFER code path of dwc3_send_gadget_ep_cmd. But this will break the dwc3_gadget_start_isoc_quirk function. What do you think? >> >> - 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; >> } 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 |