Re: [PATCH 2/2] usb: dwc3: gadget: usb: dwc3: run/stop metastability workaround

[Felipe Balbi <balbi@kernel.org>]

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

Roger Quadros <rogerq@ti.com> writes:

> [ text/plain ]
> The existing workaround of forcing DEVSPD to SUPER_SPEED
> for HIGH_SPEED ports is causing another side effect
> which causes erratic interrupts and delayed gadget
> enumeration of upto 2 seconds.
>
> Work around the run/stop issue by detecting if
> it happened using debug LTSSM state and issuing
> soft reset to the device controller when changing RUN_STOP
> from 0 to 1.
>
> We apply the workaround only if PRTCAP is DEVICE mode
> as we don't yet support this workaround in OTG mode.
>
> Use USB RESET event as exit condition for workaround.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/usb/dwc3/core.h   |   1 +
>  drivers/usb/dwc3/gadget.c | 175 +++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 144 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2bea1ac..a724c0d 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -762,6 +762,7 @@ struct dwc3 {
>  
>  	struct usb_gadget	gadget;
>  	struct usb_gadget_driver *gadget_driver;
> +	struct completion	reset_event; /* used for run/stop workaround */

the fact that you needed a struct completion here already points to the
fact this is really, really bad :-)

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 3ac170f..03418b8 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -35,6 +35,9 @@
>  #include "gadget.h"
>  #include "io.h"
>  
> +static void dwc3_gadget_disable_irq(struct dwc3 *dwc);
> +static int dwc3_gadget_restart(struct dwc3 *dwc);
> +
>  /**
>   * dwc3_gadget_set_test_mode - Enables USB2 Test Modes
>   * @dwc: pointer to our context structure
> @@ -1570,13 +1573,100 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  	struct dwc3		*dwc = gadget_to_dwc(g);
>  	unsigned long		flags;
>  	int			ret;
> +	int			trys = 0;
>  
>  	is_on = !!is_on;
>  
> +	if (is_on)
> +		reinit_completion(&dwc->reset_event);
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  	ret = dwc3_gadget_run_stop(dwc, is_on, false);
>  	spin_unlock_irqrestore(&dwc->lock, flags);
>  
> +try:
> +	/**
> +	 * WORKAROUND: DWC3 revision < 2.20a have an issue
> +	 * which would cause metastability state on Run/Stop
> +	 * bit if we try to force the IP to USB2-only mode.
> +	 *
> +	 * Because of that, we check if we hit that issue and
> +	 * reset core and retry if we did.
> +	 *
> +	 * We only attempt this workaround if we are in
> +	 * DEVICE mode (i.e. not OTG).
> +	 *
> +	 * Refers to:
> +	 *
> +	 * STAR#9000525659: Clock Domain Crossing on DCTL in
> +	 * USB 2.0 Mode
> +	 */
> +	if (is_on && dwc->revision < DWC3_REVISION_220A &&
> +	    dwc->prtcap_mode == DWC3_GCTL_PRTCAP_DEVICE) {
> +		u32 devspd, ltssm;
> +		unsigned long t;
> +
> +		/* only applicable if devspd != SUPERSPEED */
> +		devspd = dwc3_readl(dwc->regs, DWC3_DCFG) & DWC3_DCFG_SPEED_MASK;
> +		if (devspd == DWC3_DCFG_SUPERSPEED)
> +			goto done;
> +
> +		/* get link state */
> +		ltssm = dwc3_readl(dwc->regs, DWC3_GDBGLTSSM);
> +		ltssm = (ltssm >> 22) & 0xf;
> +
> +		/**
> +		 * Need to wait for 100ms and check if ltssm != 4 to detect
> +		 * metastability issue. If we got a reset event then we are
> +		 * safe and can continue.
> +		 */
> +		t = wait_for_completion_timeout(&dwc->reset_event,
> +						msecs_to_jiffies(100));
> +		if (t)
> +			goto done;
> +
> +		/**
> +		 * If link state != 4 we've hit the metastability issue, soft reset.
> +		 */
> +		if (ltssm == 4)
> +			goto done;
> +
> +		dwc3_trace(trace_dwc3_gadget,
> +			   "applying metastability workaround\n");
> +		trys++;
> +		if (trys == 2) {
> +			dev_WARN_ONCE(dwc->dev, true,
> +				      "metastability workaround failed!\n");
> +			return -ETIMEDOUT;
> +		}
> +
> +		spin_lock_irqsave(&dwc->lock, flags);
> +		/* stop gadget */
> +		dwc3_gadget_disable_irq(dwc);
> +		__dwc3_gadget_ep_disable(dwc->eps[0]);
> +		__dwc3_gadget_ep_disable(dwc->eps[1]);
> +
> +		/* soft reset device and restart */
> +		ret = dwc3_device_reinit(dwc);
> +		if (ret) {
> +			dev_err(dwc->dev, "device reinit failed\n");
> +			return ret;
> +		}
> +
> +		reinit_completion(&dwc->reset_event);
> +		/* restart gadget */
> +		ret = dwc3_gadget_restart(dwc);
> +		if (ret) {
> +			dev_err(dwc->dev, "failed to re-init gadget\n");
> +			return ret;
> +		}
> +
> +		spin_unlock_irqrestore(&dwc->lock, flags);
> +		goto try;
> +	}

holy crap dude ?!? Now ->pullup() has to reinitialize dwc3 completely ? 

-- 
balbi

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