Hi,
Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
