diff mbox series

[v2] usb: dwc3: core: Do core softreset when switch mode

Message ID 2cb4e704b059a8cc91f37081c8ceb95c6492e416.1618503587.git.Thinh.Nguyen@synopsys.com (mailing list archive)
State Superseded
Headers show
Series [v2] usb: dwc3: core: Do core softreset when switch mode | expand

Commit Message

Thinh Nguyen April 15, 2021, 4:29 p.m. UTC
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 v2:
- Initialize mutex per device and not as global mutex.
- Add additional checks for DRD only mode

 drivers/usb/dwc3/core.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/usb/dwc3/core.h |  5 +++++
 2 files changed, 39 insertions(+)


base-commit: 4b853c236c7b5161a2e444bd8b3c76fe5aa5ddcb

Comments

John Stultz April 15, 2021, 7:54 p.m. UTC | #1
On Thu, Apr 15, 2021 at 9:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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 v2:
> - Initialize mutex per device and not as global mutex.
> - Add additional checks for DRD only mode


Hey Thinh!

  Thanks so much for your persisting effort on this issue! Its
something I'd love to see finally resolved!

 >  static void __dwc3_set_mode(struct work_struct *work)
>  {
>         struct dwc3 *dwc = work_to_dwc(work);
>         unsigned long flags;
> +       unsigned int hw_mode;
> +       bool otg_enabled = false;
>         int ret;
>         u32 reg;
>
> +       mutex_lock(&dwc->mutex);
> +
> +       hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> +       if (DWC3_VER_IS_PRIOR(DWC3, 330A) &&
> +           (dwc->hwparams.hwparams6 & DWC3_GHWPARAMS6_SRPSUPPORT))
> +               otg_enabled = true;

Unfortunately on HiKey960, this check ends up being true, and that
basically disables the needed (on HiKey960 at least) soft reset logic
below, so we still end up hitting the issue.

The revision/hwparams6 values on the board are:
  revision: 0x5533300a hwparams6: 0xfeaec20

Just to make sure, I did test disabling the check here, and it does
seem to avoid the !COREIDLE stuck problem seen frequently on the
board.

thanks
-john
Thinh Nguyen April 15, 2021, 8:11 p.m. UTC | #2
John Stultz wrote:
> On Thu, Apr 15, 2021 at 9:29 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> 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://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20210108015115.27920-1-john.stultz@linaro.org/__;!!A4F2R9G_pg!JQx84Y3U9KGy3dVpW72cQCGg_UMbRwObcCBtqBh0SWpbyQ2nzz1OF23IkfKkMKRZ3Xlq$ 
>> [2] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/0ba7a6ba-e6a7-9cd4-0695-64fc927e01f1@gmail.com/__;!!A4F2R9G_pg!JQx84Y3U9KGy3dVpW72cQCGg_UMbRwObcCBtqBh0SWpbyQ2nzz1OF23IkfKkMM1Jy_99$ 
>>
>> 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 v2:
>> - Initialize mutex per device and not as global mutex.
>> - Add additional checks for DRD only mode
> 
> 
> Hey Thinh!
> 
>   Thanks so much for your persisting effort on this issue! Its
> something I'd love to see finally resolved!
> 
>  >  static void __dwc3_set_mode(struct work_struct *work)
>>  {
>>         struct dwc3 *dwc = work_to_dwc(work);
>>         unsigned long flags;
>> +       unsigned int hw_mode;
>> +       bool otg_enabled = false;
>>         int ret;
>>         u32 reg;
>>
>> +       mutex_lock(&dwc->mutex);
>> +
>> +       hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>> +       if (DWC3_VER_IS_PRIOR(DWC3, 330A) &&
>> +           (dwc->hwparams.hwparams6 & DWC3_GHWPARAMS6_SRPSUPPORT))
>> +               otg_enabled = true;
> 
> Unfortunately on HiKey960, this check ends up being true, and that
> basically disables the needed (on HiKey960 at least) soft reset logic
> below, so we still end up hitting the issue.
> 
> The revision/hwparams6 values on the board are:
>   revision: 0x5533300a hwparams6: 0xfeaec20
> 
> Just to make sure, I did test disabling the check here, and it does
> seem to avoid the !COREIDLE stuck problem seen frequently on the
> board.
> 

Hi John,

That extra check for OTG support is unnecessary (and I believe is
incorrect), but I wanted to completely alleviate Felipe's concern. With
static host-only/device-only DRD mode, we don't care about OTG since
PrtCapDir is not set to OTG.

Thanks for the test. I'll make the fix and discuss further with Felipe
if necessary.

Thanks,
Thinh
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 5c25e6a72dbd..8eb6242e6bce 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -114,13 +114,24 @@  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);
 	unsigned long flags;
+	unsigned int hw_mode;
+	bool otg_enabled = false;
 	int ret;
 	u32 reg;
 
+	mutex_lock(&dwc->mutex);
+
+	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+	if (DWC3_VER_IS_PRIOR(DWC3, 330A) &&
+	    (dwc->hwparams.hwparams6 & DWC3_GHWPARAMS6_SRPSUPPORT))
+		otg_enabled = true;
+
 	pm_runtime_get_sync(dwc->dev);
 
 	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_OTG)
@@ -154,6 +165,24 @@  static void __dwc3_set_mode(struct work_struct *work)
 		break;
 	}
 
+	if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD && !otg_enabled) {
+		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 +207,9 @@  static void __dwc3_set_mode(struct work_struct *work)
 		}
 		break;
 	case DWC3_GCTL_PRTCAP_DEVICE:
+		if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD && !otg_enabled)
+			dwc3_core_soft_reset(dwc);
+
 		dwc3_event_buffers_setup(dwc);
 
 		if (dwc->usb2_phy)
@@ -200,6 +232,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 +1586,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;