Re: [PATCH 4/5] usb: dwc3: gadget: Handle transfer completion

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4687 bytes --]


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.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]