| Message ID | 70555c2202529c6e0bdd23124003d0d4bc637cdc.1588025916.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: gadget: Handle streams | expand |
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > With the new usb_request->is_last field, now the function drivers can > inform which request is the end of a transfer, dwc3 can program its TRBs > to let the controller know when to free its resources when a transfer > completes. This is required for stream transfers. The controller needs > to know where one stream starts and ends to properly allocate resources > for different streams. > > Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> from a quick read, it looks like this can be broken down into smaller patches. > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 7204a838ec06..b11183a715a7 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -701,6 +701,7 @@ struct dwc3_ep { > #define DWC3_EP_END_TRANSFER_PENDING BIT(4) > #define DWC3_EP_PENDING_REQUEST BIT(5) > #define DWC3_EP_DELAY_START BIT(6) > +#define DWC3_EP_WAIT_TRANSFER_COMPLETE BIT(7) this could be its own patch with proper description (and usage) > > /* This last one is specific to EP0 */ > #define DWC3_EP0_DIR_IN BIT(31) > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 865e6fbb7360..628f9d142876 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -579,6 +579,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action) > > if (usb_ss_max_streams(comp_desc) && usb_endpoint_xfer_bulk(desc)) { > params.param1 |= DWC3_DEPCFG_STREAM_CAPABLE > + | DWC3_DEPCFG_XFER_COMPLETE_EN adding this bit for stream endpoints could be a separate commit. > | DWC3_DEPCFG_STREAM_EVENT_EN; > dep->stream_capable = true; > } > @@ -917,8 +918,9 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) > } > > static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb, > - dma_addr_t dma, unsigned length, unsigned chain, unsigned node, > - unsigned stream_id, unsigned short_not_ok, unsigned no_interrupt) > + dma_addr_t dma, unsigned length, unsigned chain, > + unsigned is_last, unsigned node, unsigned stream_id, > + unsigned short_not_ok, unsigned no_interrupt) if you add "is_last" as the last argument, this hunk will look smaller. Nitpicking, I know. > @@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) > > if (!dwc3_calc_trbs_left(dep)) > return; > + > + /* Don't prepare ahead. This is not an option for DWC_usb32. */ > + if (req->request.is_last) > + return; this requires some better description. Why isn't it an option for dwc_usb32? > @@ -2648,37 +2666,22 @@ static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep) > return !dwc3_gadget_ep_request_completed(req); > } > > -static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, > - const struct dwc3_event_depevt *event) > -{ > - dep->frame_number = event->parameters; > -} moving these functions around could be a separate patch. The way it's done now takes away from review. > @@ -2704,6 +2707,45 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, > > dwc->u1u2 = 0; > } > + > + return no_started_trb; > +} > + > +static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, > + const struct dwc3_event_depevt *event) > +{ > + dep->frame_number = event->parameters; > +} > + > +static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, > + const struct dwc3_event_depevt *event) > +{ > + int status = 0; > + > + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) > + dwc3_gadget_endpoint_frame_from_event(dep, event); > + > + if (event->status & DEPEVT_STATUS_BUSERR) > + status = -ECONNRESET; > + > + if (event->status & DEPEVT_STATUS_MISSED_ISOC) > + status = -EXDEV; > + > + dwc3_gadget_endpoint_trbs_complete(dep, event, status); > +} > + > +static void dwc3_gadget_endpoint_transfer_complete(struct dwc3_ep *dep, > + const struct dwc3_event_depevt *event) > +{ > + int status = 0; > + > + dep->flags &= ~DWC3_EP_TRANSFER_STARTED; > + > + if (event->status & DEPEVT_STATUS_BUSERR) > + status = -ECONNRESET; > + > + if (dwc3_gadget_endpoint_trbs_complete(dep, event, status)) > + dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE; > } > > static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep, > @@ -2770,7 +2812,10 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, > } > break; > case DWC3_DEPEVT_STREAMEVT: > + break; Swap these around. Keep all the "nothing to do here" cases together.
Hi, Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > >> With the new usb_request->is_last field, now the function drivers can >> inform which request is the end of a transfer, dwc3 can program its TRBs >> to let the controller know when to free its resources when a transfer >> completes. This is required for stream transfers. The controller needs >> to know where one stream starts and ends to properly allocate resources >> for different streams. >> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > from a quick read, it looks like this can be broken down into smaller > patches. Ok. > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 7204a838ec06..b11183a715a7 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -701,6 +701,7 @@ struct dwc3_ep { >> #define DWC3_EP_END_TRANSFER_PENDING BIT(4) >> #define DWC3_EP_PENDING_REQUEST BIT(5) >> #define DWC3_EP_DELAY_START BIT(6) >> +#define DWC3_EP_WAIT_TRANSFER_COMPLETE BIT(7) > this could be its own patch with proper description (and usage) Will do. >> >> /* This last one is specific to EP0 */ >> #define DWC3_EP0_DIR_IN BIT(31) >> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >> index 865e6fbb7360..628f9d142876 100644 >> --- a/drivers/usb/dwc3/gadget.c >> +++ b/drivers/usb/dwc3/gadget.c >> @@ -579,6 +579,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action) >> >> if (usb_ss_max_streams(comp_desc) && usb_endpoint_xfer_bulk(desc)) { >> params.param1 |= DWC3_DEPCFG_STREAM_CAPABLE >> + | DWC3_DEPCFG_XFER_COMPLETE_EN > adding this bit for stream endpoints could be a separate commit. Will do. > >> | DWC3_DEPCFG_STREAM_EVENT_EN; >> dep->stream_capable = true; >> } >> @@ -917,8 +918,9 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) >> } >> >> static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb, >> - dma_addr_t dma, unsigned length, unsigned chain, unsigned node, >> - unsigned stream_id, unsigned short_not_ok, unsigned no_interrupt) >> + dma_addr_t dma, unsigned length, unsigned chain, >> + unsigned is_last, unsigned node, unsigned stream_id, >> + unsigned short_not_ok, unsigned no_interrupt) > if you add "is_last" as the last argument, this hunk will look > smaller. Nitpicking, I know. No problem :) > >> @@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) >> >> if (!dwc3_calc_trbs_left(dep)) >> return; >> + >> + /* Don't prepare ahead. This is not an option for DWC_usb32. */ >> + if (req->request.is_last) >> + return; > this requires some better description. Why isn't it an option for dwc_usb32? Internally, DWC_usb32 does some advance caching and burst that we should not prepare more TRB until the transfer is completed. This doesn't apply for isoc, missed a check here. Need to apply on the next version. > >> @@ -2648,37 +2666,22 @@ static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep) >> return !dwc3_gadget_ep_request_completed(req); >> } >> >> -static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, >> - const struct dwc3_event_depevt *event) >> -{ >> - dep->frame_number = event->parameters; >> -} > moving these functions around could be a separate patch. The way it's > done now takes away from review. You're right, it's a bit difficult to review as is. > >> @@ -2704,6 +2707,45 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, >> >> dwc->u1u2 = 0; >> } >> + >> + return no_started_trb; >> +} >> + >> +static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, >> + const struct dwc3_event_depevt *event) >> +{ >> + dep->frame_number = event->parameters; >> +} >> + >> +static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, >> + const struct dwc3_event_depevt *event) >> +{ >> + int status = 0; >> + >> + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) >> + dwc3_gadget_endpoint_frame_from_event(dep, event); >> + >> + if (event->status & DEPEVT_STATUS_BUSERR) >> + status = -ECONNRESET; >> + >> + if (event->status & DEPEVT_STATUS_MISSED_ISOC) >> + status = -EXDEV; >> + >> + dwc3_gadget_endpoint_trbs_complete(dep, event, status); >> +} >> + >> +static void dwc3_gadget_endpoint_transfer_complete(struct dwc3_ep *dep, >> + const struct dwc3_event_depevt *event) >> +{ >> + int status = 0; >> + >> + dep->flags &= ~DWC3_EP_TRANSFER_STARTED; >> + >> + if (event->status & DEPEVT_STATUS_BUSERR) >> + status = -ECONNRESET; >> + >> + if (dwc3_gadget_endpoint_trbs_complete(dep, event, status)) >> + dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE; >> } >> >> static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep, >> @@ -2770,7 +2812,10 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, >> } >> break; >> case DWC3_DEPEVT_STREAMEVT: >> + break; > Swap these around. Keep all the "nothing to do here" cases > together. > Thanks, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>> @@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) >>> >>> if (!dwc3_calc_trbs_left(dep)) >>> return; >>> + >>> + /* Don't prepare ahead. This is not an option for DWC_usb32. */ >>> + if (req->request.is_last) >>> + return; >> this requires some better description. Why isn't it an option for dwc_usb32? > > Internally, DWC_usb32 does some advance caching and burst that we should > not prepare more TRB until the transfer is completed. > This doesn't apply for isoc, missed a check here. Need to apply on the > next version. Do you mind re-wording this statement as a comment in the code?
Felipe Balbi wrote: > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>> @@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) >>>> >>>> if (!dwc3_calc_trbs_left(dep)) >>>> return; >>>> + >>>> + /* Don't prepare ahead. This is not an option for DWC_usb32. */ >>>> + if (req->request.is_last) >>>> + return; >>> this requires some better description. Why isn't it an option for dwc_usb32? >> Internally, DWC_usb32 does some advance caching and burst that we should >> not prepare more TRB until the transfer is completed. >> This doesn't apply for isoc, missed a check here. Need to apply on the >> next version. > Do you mind re-wording this statement as a comment in the code? Yes, I reworded it and added the comment in the code and commit message in the v2. You already have it on your testing/next branch. Thanks, Thinh
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 7204a838ec06..b11183a715a7 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -701,6 +701,7 @@ struct dwc3_ep { #define DWC3_EP_END_TRANSFER_PENDING BIT(4) #define DWC3_EP_PENDING_REQUEST BIT(5) #define DWC3_EP_DELAY_START BIT(6) +#define DWC3_EP_WAIT_TRANSFER_COMPLETE BIT(7) /* This last one is specific to EP0 */ #define DWC3_EP0_DIR_IN BIT(31) diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 865e6fbb7360..628f9d142876 100644 --- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -579,6 +579,7 @@ static int dwc3_gadget_set_ep_config(struct dwc3_ep *dep, unsigned int action) if (usb_ss_max_streams(comp_desc) && usb_endpoint_xfer_bulk(desc)) { params.param1 |= DWC3_DEPCFG_STREAM_CAPABLE + | DWC3_DEPCFG_XFER_COMPLETE_EN | DWC3_DEPCFG_STREAM_EVENT_EN; dep->stream_capable = true; } @@ -917,8 +918,9 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep) } static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb, - dma_addr_t dma, unsigned length, unsigned chain, unsigned node, - unsigned stream_id, unsigned short_not_ok, unsigned no_interrupt) + dma_addr_t dma, unsigned length, unsigned chain, + unsigned is_last, unsigned node, unsigned stream_id, + unsigned short_not_ok, unsigned no_interrupt) { struct dwc3 *dwc = dep->dwc; struct usb_gadget *gadget = &dwc->gadget; @@ -1011,6 +1013,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep, struct dwc3_trb *trb, if (chain) trb->ctrl |= DWC3_TRB_CTRL_CHN; + else if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) && is_last) + trb->ctrl |= DWC3_TRB_CTRL_LST; if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable) trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id); @@ -1038,6 +1042,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep, unsigned stream_id = req->request.stream_id; unsigned short_not_ok = req->request.short_not_ok; unsigned no_interrupt = req->request.no_interrupt; + unsigned is_last = req->request.is_last; if (req->request.num_sgs > 0) { length = sg_dma_len(req->start_sg); @@ -1057,7 +1062,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep, req->num_trbs++; - __dwc3_prepare_one_trb(dep, trb, dma, length, chain, node, + __dwc3_prepare_one_trb(dep, trb, dma, length, chain, is_last, node, stream_id, short_not_ok, no_interrupt); } @@ -1100,7 +1105,8 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep, trb = &dep->trb_pool[dep->trb_enqueue]; req->num_trbs++; __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, - maxp - rem, false, 1, + maxp - rem, false, + req->request.is_last, 1, req->request.stream_id, req->request.short_not_ok, req->request.no_interrupt); @@ -1145,7 +1151,8 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep, trb = &dep->trb_pool[dep->trb_enqueue]; req->num_trbs++; __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, maxp - rem, - false, 1, req->request.stream_id, + false, req->request.is_last, 1, + req->request.stream_id, req->request.short_not_ok, req->request.no_interrupt); } else if (req->request.zero && req->request.length && @@ -1162,7 +1169,8 @@ static void dwc3_prepare_one_trb_linear(struct dwc3_ep *dep, trb = &dep->trb_pool[dep->trb_enqueue]; req->num_trbs++; __dwc3_prepare_one_trb(dep, trb, dwc->bounce_addr, 0, - false, 1, req->request.stream_id, + false, req->request.is_last, 1, + req->request.stream_id, req->request.short_not_ok, req->request.no_interrupt); } else { @@ -1223,6 +1231,10 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep) if (!dwc3_calc_trbs_left(dep)) return; + + /* Don't prepare ahead. This is not an option for DWC_usb32. */ + if (req->request.is_last) + return; } } @@ -1284,6 +1296,9 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep) return ret; } + if (req->request.is_last) + dep->flags |= DWC3_EP_WAIT_TRANSFER_COMPLETE; + return 0; } @@ -1490,6 +1505,9 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req) list_add_tail(&req->list, &dep->pending_list); req->status = DWC3_REQUEST_STATUS_QUEUED; + if (dep->flags & DWC3_EP_WAIT_TRANSFER_COMPLETE) + return 0; + /* Start the transfer only after the END_TRANSFER is completed */ if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) { dep->flags |= DWC3_EP_DELAY_START; @@ -2648,37 +2666,22 @@ static bool dwc3_gadget_ep_should_continue(struct dwc3_ep *dep) return !dwc3_gadget_ep_request_completed(req); } -static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, - const struct dwc3_event_depevt *event) -{ - dep->frame_number = event->parameters; -} - -static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, - const struct dwc3_event_depevt *event) +static bool dwc3_gadget_endpoint_trbs_complete(struct dwc3_ep *dep, + const struct dwc3_event_depevt *event, int status) { struct dwc3 *dwc = dep->dwc; - unsigned status = 0; - bool stop = false; - - dwc3_gadget_endpoint_frame_from_event(dep, event); - - if (event->status & DEPEVT_STATUS_BUSERR) - status = -ECONNRESET; - - if (event->status & DEPEVT_STATUS_MISSED_ISOC) { - status = -EXDEV; - - if (list_empty(&dep->started_list)) - stop = true; - } + bool no_started_trb = true; dwc3_gadget_ep_cleanup_completed_requests(dep, event, status); - if (stop) + if (dep->flags & DWC3_EP_END_TRANSFER_PENDING) + return no_started_trb; + + if (status == -EXDEV && list_empty(&dep->started_list)) dwc3_stop_active_transfer(dep, true, true); else if (dwc3_gadget_ep_should_continue(dep)) - __dwc3_gadget_kick_transfer(dep); + if (__dwc3_gadget_kick_transfer(dep) == 0) + no_started_trb = false; /* * WORKAROUND: This is the 2nd half of U1/U2 -> U0 workaround. @@ -2695,7 +2698,7 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, continue; if (!list_empty(&dep->started_list)) - return; + return no_started_trb; } reg = dwc3_readl(dwc->regs, DWC3_DCTL); @@ -2704,6 +2707,45 @@ static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, dwc->u1u2 = 0; } + + return no_started_trb; +} + +static void dwc3_gadget_endpoint_frame_from_event(struct dwc3_ep *dep, + const struct dwc3_event_depevt *event) +{ + dep->frame_number = event->parameters; +} + +static void dwc3_gadget_endpoint_transfer_in_progress(struct dwc3_ep *dep, + const struct dwc3_event_depevt *event) +{ + int status = 0; + + if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) + dwc3_gadget_endpoint_frame_from_event(dep, event); + + if (event->status & DEPEVT_STATUS_BUSERR) + status = -ECONNRESET; + + if (event->status & DEPEVT_STATUS_MISSED_ISOC) + status = -EXDEV; + + dwc3_gadget_endpoint_trbs_complete(dep, event, status); +} + +static void dwc3_gadget_endpoint_transfer_complete(struct dwc3_ep *dep, + const struct dwc3_event_depevt *event) +{ + int status = 0; + + dep->flags &= ~DWC3_EP_TRANSFER_STARTED; + + if (event->status & DEPEVT_STATUS_BUSERR) + status = -ECONNRESET; + + if (dwc3_gadget_endpoint_trbs_complete(dep, event, status)) + dep->flags &= ~DWC3_EP_WAIT_TRANSFER_COMPLETE; } static void dwc3_gadget_endpoint_transfer_not_ready(struct dwc3_ep *dep, @@ -2770,7 +2812,10 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, } break; case DWC3_DEPEVT_STREAMEVT: + break; case DWC3_DEPEVT_XFERCOMPLETE: + dwc3_gadget_endpoint_transfer_complete(dep, event); + break; case DWC3_DEPEVT_RXTXFIFOEVT: break; }
With the new usb_request->is_last field, now the function drivers can inform which request is the end of a transfer, dwc3 can program its TRBs to let the controller know when to free its resources when a transfer completes. This is required for stream transfers. The controller needs to know where one stream starts and ends to properly allocate resources for different streams. Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> --- drivers/usb/dwc3/core.h | 1 + drivers/usb/dwc3/gadget.c | 107 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 77 insertions(+), 31 deletions(-)