Re: [PATCH v3] usb: dwc3: Stop active transfers before halting the controller

[Felipe Balbi <balbi@kernel.org>]

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


Hi,

Wesley Cheng <wcheng@codeaurora.org> writes:
> diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
> index 59f2e8c31bd1..456aa87e8778 100644
> --- a/drivers/usb/dwc3/ep0.c
> +++ b/drivers/usb/dwc3/ep0.c
> @@ -197,7 +197,7 @@ int dwc3_gadget_ep0_queue(struct usb_ep *ep, struct usb_request *request,
>  	int				ret;
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
> -	if (!dep->endpoint.desc) {
> +	if (!dep->endpoint.desc || !dwc->pullups_connected) {

this looks odd. If we don't have pullups connected, we shouldn't have a
descriptor, likewise if we don't have a a description, we haven't been
enumerated, therefore we shouldn't have pullups connected.

What am I missing here?

> @@ -1926,6 +1926,21 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
>  	return 0;
>  }
>  
> +static void dwc3_stop_active_transfers(struct dwc3 *dwc)
> +{
> +	u32 epnum;
> +
> +	for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {

dwc3 knows the number of endpoints available in the HW. Use dwc->num_eps
instead.

> @@ -1971,6 +1986,8 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
>  	return 0;
>  }
>  
> +static void __dwc3_gadget_stop(struct dwc3 *dwc);
> +
>  static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  {
>  	struct dwc3		*dwc = gadget_to_dwc(g);
> @@ -1994,9 +2011,37 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  		}
>  	}
>  
> +	/*
> +	 * Synchronize and disable any further event handling while controller
> +	 * is being enabled/disabled.
> +	 */
> +	disable_irq(dwc->irq_gadget);

why isn't dwc3_gadget_disable_irq() enough?

>  	spin_lock_irqsave(&dwc->lock, flags);

spin_lock_irqsave() will disable interrupts, why disable_irq() above?

> +	/* Controller is not halted until pending events are acknowledged */
> +	if (!is_on) {
> +		u32 count;
> +
> +		/*
> +		 * The databook explicitly mentions for a device-initiated
> +		 * disconnect sequence, the SW needs to ensure that it ends any
> +		 * active transfers.
> +		 */

make this a little better by mentioning the version and section of the
databook you're reading. That makes it easier for future
reference. Also, use an actual quote from the databook, along the lines
of:

		/*
                 * Synopsys DesignWare Cores USB3 Databook Revision
                 * X.YYa states in section W.Z that "device-initiated
                 * disconnect ...."
                 */

> +		dwc3_stop_active_transfers(dwc);
> +		__dwc3_gadget_stop(dwc);
> +
> +		count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> +		count &= DWC3_GEVNTCOUNT_MASK;
> +		if (count > 0) {
> +			dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
> +			dwc->ev_buf->lpos = (dwc->ev_buf->lpos + count) %
> +						dwc->ev_buf->length;
> +		}

don't duplicate code. Add a patch before this extracting this into
helper and use it for both irq handler and gadget pullup.

> +	}
> +
>  	ret = dwc3_gadget_run_stop(dwc, is_on, false);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
> +	enable_irq(dwc->irq_gadget);
>  
>  	return ret;
>  }
> @@ -3100,6 +3145,8 @@ static void dwc3_gadget_reset_interrupt(struct dwc3 *dwc)
>  	}
>  
>  	dwc3_reset_gadget(dwc);
> +	/* Stop any active/pending transfers when receiving bus reset */

unnecessary comment. We're calling a function named "stop active
transfers" from within the "bus reset handler".

-- 
balbi

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