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

[Roger Quadros]

On 16/03/16 15:14, Felipe Balbi wrote:
> Roger Quadros <rogerq@xxxxxx> 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@xxxxxx>
>> ---
>>  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 ? 
> 

Only if it hits the error state. We're only re-initializing the device controller
not complete dwc3.

cheers,
-roger