Re: [PATCH v3] usb: dwc3: core: Do core softreset when switch mode

[Wesley Cheng <wcheng@codeaurora.org>]

On 4/15/2021 3:20 PM, Thinh Nguyen wrote:
> From: Yu Chen <chenyu56@huawei.com>
> From: John Stultz <john.stultz@linaro.org>
> 
> According to the programming guide, to switch mode for DRD controller,
> the driver needs to do the following.
> 
> To switch from device to host:
> 1. Reset controller with GCTL.CoreSoftReset
> 2. Set GCTL.PrtCapDir(host mode)
> 3. Reset the host with USBCMD.HCRESET
> 4. Then follow up with the initializing host registers sequence
> 
> To switch from host to device:
> 1. Reset controller with GCTL.CoreSoftReset
> 2. Set GCTL.PrtCapDir(device mode)
> 3. Reset the device with DCTL.CSftRst
> 4. Then follow up with the initializing registers sequence
> 
> Currently we're missing step 1) to do GCTL.CoreSoftReset and step 3) of
> switching from host to device. John Stult reported a lockup issue seen
> with HiKey960 platform without these steps[1]. Similar issue is observed
> with Ferry's testing platform[2].
> 
> So, apply the required steps along with some fixes to Yu Chen's and John
> Stultz's version. The main fixes to their versions are the missing wait
> for clocks synchronization before clearing GCTL.CoreSoftReset and only
> apply DCTL.CSftRst when switching from host to device.
> 
> [1] https://lore.kernel.org/linux-usb/20210108015115.27920-1-john.stultz@linaro.org/
> [2] https://lore.kernel.org/linux-usb/0ba7a6ba-e6a7-9cd4-0695-64fc927e01f1@gmail.com/
> 
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Ferry Toth <fntoth@gmail.com>
> Cc: Wesley Cheng <wcheng@codeaurora.org>
> Cc: <stable@vger.kernel.org>
> Fixes: 41ce1456e1db ("usb: dwc3: core: make dwc3_set_mode() work properly")
> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
> Changes in v3:
> - Check if the desired mode is OTG, then keep the old flow
> - Remove condition for OTG support only since the device can still be
>   configured DRD host/device mode only
> - Remove redundant hw_mode check since __dwc3_set_mode() only applies when
>   hw_mode is DRD
> Changes in v2:
> - Initialize mutex per device and not as global mutex.
> - Add additional checks for DRD only mode
> 
>  drivers/usb/dwc3/core.c | 27 +++++++++++++++++++++++++++
>  drivers/usb/dwc3/core.h |  5 +++++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 5c25e6a72dbd..2f118ad43571 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -114,6 +114,8 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>  	dwc->current_dr_role = mode;
>  }
>  
> +static int dwc3_core_soft_reset(struct dwc3 *dwc);
> +
>  static void __dwc3_set_mode(struct work_struct *work)
>  {
>  	struct dwc3 *dwc = work_to_dwc(work);
> @@ -121,6 +123,8 @@ static void __dwc3_set_mode(struct work_struct *work)
>  	int ret;
>  	u32 reg;
>  
> +	mutex_lock(&dwc->mutex);
> +
>  	pm_runtime_get_sync(dwc->dev);
>  
>  	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG)
> @@ -154,6 +158,25 @@ static void __dwc3_set_mode(struct work_struct *work)
>  		break;
>  	}
>  
> +	/* For DRD host or device mode only */
> +	if (dwc->desired_dr_role != DWC3_GCTL_PRTCAP_OTG) {
> +		reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +		reg |= DWC3_GCTL_CORESOFTRESET;
> +		dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> +		/*
> +		 * Wait for internal clocks to synchronized. DWC_usb31 and
> +		 * DWC_usb32 may need at least 50ms (less for DWC_usb3). To
> +		 * keep it consistent across different IPs, let's wait up to
> +		 * 100ms before clearing GCTL.CORESOFTRESET.
> +		 */
> +		msleep(100);
> +
> +		reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +		reg &= ~DWC3_GCTL_CORESOFTRESET;
> +		dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +	}
> +
>  	spin_lock_irqsave(&dwc->lock, flags);
>  
>  	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
> @@ -178,6 +201,8 @@ static void __dwc3_set_mode(struct work_struct *work)
>  		}
>  		break;
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> +		dwc3_core_soft_reset(dwc);
> +
>  		dwc3_event_buffers_setup(dwc);
>  
>  		if (dwc->usb2_phy)
> @@ -200,6 +225,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>  out:
>  	pm_runtime_mark_last_busy(dwc->dev);
>  	pm_runtime_put_autosuspend(dwc->dev);
> +	mutex_unlock(&dwc->mutex);
>  }
>  
>  void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
> @@ -1553,6 +1579,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	dwc3_cache_hwparams(dwc);
>  
>  	spin_lock_init(&dwc->lock);
> +	mutex_init(&dwc->mutex);
>  
>  	pm_runtime_set_active(dev);
>  	pm_runtime_use_autosuspend(dev);
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 695ff2d791e4..7e3afa5378e8 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -13,6 +13,7 @@
>  
>  #include <linux/device.h>
>  #include <linux/spinlock.h>
> +#include <linux/mutex.h>
>  #include <linux/ioport.h>
>  #include <linux/list.h>
>  #include <linux/bitops.h>
> @@ -947,6 +948,7 @@ struct dwc3_scratchpad_array {
>   * @scratch_addr: dma address of scratchbuf
>   * @ep0_in_setup: one control transfer is completed and enter setup phase
>   * @lock: for synchronizing
> + * @mutex: for mode switching
>   * @dev: pointer to our struct device
>   * @sysdev: pointer to the DMA-capable device
>   * @xhci: pointer to our xHCI child
> @@ -1088,6 +1090,9 @@ struct dwc3 {
>  	/* device lock */
>  	spinlock_t		lock;
>  
> +	/* mode switching lock */
> +	struct mutex		mutex;
> +
>  	struct device		*dev;
>  	struct device		*sysdev;
>  
> 
> base-commit: 4b853c236c7b5161a2e444bd8b3c76fe5aa5ddcb
> 
Hi Thinh,

Thanks for this change! Tested this on our platform w/ a DRD mode switch
loop and it looks good.

Tested-by: Wesley Cheng <wcheng@codeaurora.org>

Thanks
Wesley Cheng
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project