[PATCH v5 0/3] usb: dwc2: Fix core reset and force mode delays

[PATCH v5 0/3] usb: dwc2: Fix core reset and force mode delays

[John Youn]

This series accounts for the delay from the IDDIG debounce filter when
switching modes. This delay is a function of the PHY clock speed and
can range from 5-50 ms. This delay must be taken into account on core
reset and force modes. A full explanation is provided in the patch
commit log and code comments.

This revision of the series increases the IDDIG delay to 100 ms. Some
rockchip platforms seem to timeout even with 50 ms so I have doubled
this.

Appreciate any testing on RK3188 platforms.

v5:
* Added Stefan Wahren's tested-by
* Dropped RFT

v4:
* Increased the IDDIG delay to 110ms.
* Removed tested-by for patch 2 since I have changed the delays.

v3:
* Added tested-bys for patch 1-2
* Fixed an issue where a function was not returning a value
* Dropped patch 4

v2:
* Broke up the last patch of the original series

Regards,
John

John Youn (3):
  usb: dwc2: gadget: Only initialize device if in device mode
  usb: dwc2: Add delay to core soft reset
  usb: dwc2: Properly account for the force mode delays

 drivers/usb/dwc2/core.c   | 126 +++++++++++++++++++++++++++++++++++++++-------
 drivers/usb/dwc2/core.h   |   1 +
 drivers/usb/dwc2/gadget.c |   7 ++-
 drivers/usb/dwc2/hw.h     |   1 +
 4 files changed, 116 insertions(+), 19 deletions(-)

-- 
2.9.0

[PATCH v5 0/3] usb: dwc2: Fix core reset and force mode delays

[John Youn]

This series accounts for the delay from the IDDIG debounce filter when
switching modes. This delay is a function of the PHY clock speed and
can range from 5-50 ms. This delay must be taken into account on core
reset and force modes. A full explanation is provided in the patch
commit log and code comments.

This revision of the series increases the IDDIG delay to 100 ms. Some
rockchip platforms seem to timeout even with 50 ms so I have doubled
this.

Appreciate any testing on RK3188 platforms.

v5:
* Added Stefan Wahren's tested-by
* Dropped RFT

v4:
* Increased the IDDIG delay to 110ms.
* Removed tested-by for patch 2 since I have changed the delays.

v3:
* Added tested-bys for patch 1-2
* Fixed an issue where a function was not returning a value
* Dropped patch 4

v2:
* Broke up the last patch of the original series

Regards,
John

John Youn (3):
  usb: dwc2: gadget: Only initialize device if in device mode
  usb: dwc2: Add delay to core soft reset
  usb: dwc2: Properly account for the force mode delays

 drivers/usb/dwc2/core.c   | 126 +++++++++++++++++++++++++++++++++++++++-------
 drivers/usb/dwc2/core.h   |   1 +
 drivers/usb/dwc2/gadget.c |   7 ++-
 drivers/usb/dwc2/hw.h     |   1 +
 4 files changed, 116 insertions(+), 19 deletions(-)

-- 
2.9.0

[PATCH v5 1/3] usb: dwc2: gadget: Only initialize device if in device mode

[John Youn]

In dwc2_hsotg_udc_start(), don't initialize the controller for device
mode unless we are actually in device mode.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: John Youn <johnyoun@synopsys.com>
---
 drivers/usb/dwc2/gadget.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 94bd19a..4cd6403 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3466,8 +3466,11 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget,
 		otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget);
 
 	spin_lock_irqsave(&hsotg->lock, flags);
-	dwc2_hsotg_init(hsotg);
-	dwc2_hsotg_core_init_disconnected(hsotg, false);
+	if (dwc2_hw_is_device(hsotg)) {
+		dwc2_hsotg_init(hsotg);
+		dwc2_hsotg_core_init_disconnected(hsotg, false);
+	}
+
 	hsotg->enabled = 0;
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
-- 
2.9.0

[PATCH v5 1/3] usb: dwc2: gadget: Only initialize device if in device mode

[John Youn]

In dwc2_hsotg_udc_start(), don't initialize the controller for device
mode unless we are actually in device mode.

Tested-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: John Youn <johnyoun@synopsys.com>
---
 drivers/usb/dwc2/gadget.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 94bd19a..4cd6403 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3466,8 +3466,11 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget,
 		otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget);
 
 	spin_lock_irqsave(&hsotg->lock, flags);
-	dwc2_hsotg_init(hsotg);
-	dwc2_hsotg_core_init_disconnected(hsotg, false);
+	if (dwc2_hw_is_device(hsotg)) {
+		dwc2_hsotg_init(hsotg);
+		dwc2_hsotg_core_init_disconnected(hsotg, false);
+	}
+
 	hsotg->enabled = 0;
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
-- 
2.9.0

[PATCH v5 2/3] usb: dwc2: Add delay to core soft reset

[John Youn]

Add a delay to the core soft reset function to account for the IDDIG
debounce filter.

If the current mode is host, either due to the force mode bit being
set (which persists after core reset) or the connector id pin, a core
soft reset will temporarily reset the mode to device and a delay from
the IDDIG debounce filter will occur before going back to host mode.

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: John Youn <johnyoun@synopsys.com>
---
 drivers/usb/dwc2/core.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/core.h |  1 +
 drivers/usb/dwc2/hw.h   |  1 +
 3 files changed, 97 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 4135a5f..a3068e0 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -238,6 +238,77 @@ int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg)
 	return ret;
 }
 
+/**
+ * dwc2_wait_for_mode() - Waits for the controller mode.
+ * @hsotg:	Programming view of the DWC_otg controller.
+ * @host_mode:	If true, waits for host mode, otherwise device mode.
+ */
+static void dwc2_wait_for_mode(struct dwc2_hsotg *hsotg,
+			       bool host_mode)
+{
+	ktime_t start;
+	ktime_t end;
+	unsigned int timeout = 110;
+
+	dev_vdbg(hsotg->dev, "Waiting for %s mode\n",
+		 host_mode ? "host" : "device");
+
+	start = ktime_get();
+
+	while (1) {
+		s64 ms;
+
+		if (dwc2_is_host_mode(hsotg) == host_mode) {
+			dev_vdbg(hsotg->dev, "%s mode set\n",
+				 host_mode ? "Host" : "Device");
+			break;
+		}
+
+		end = ktime_get();
+		ms = ktime_to_ms(ktime_sub(end, start));
+
+		if (ms >= (s64)timeout) {
+			dev_warn(hsotg->dev, "%s: Couldn't set %s mode\n",
+				 __func__, host_mode ? "host" : "device");
+			break;
+		}
+
+		usleep_range(1000, 2000);
+	}
+}
+
+/**
+ * dwc2_iddig_filter_enabled() - Returns true if the IDDIG debounce
+ * filter is enabled.
+ */
+static bool dwc2_iddig_filter_enabled(struct dwc2_hsotg *hsotg)
+{
+	u32 gsnpsid;
+	u32 ghwcfg4;
+
+	if (!dwc2_hw_is_otg(hsotg))
+		return false;
+
+	/* Check if core configuration includes the IDDIG filter. */
+	ghwcfg4 = dwc2_readl(hsotg->regs + GHWCFG4);
+	if (!(ghwcfg4 & GHWCFG4_IDDIG_FILT_EN))
+		return false;
+
+	/*
+	 * Check if the IDDIG debounce filter is bypassed. Available
+	 * in core version >= 3.10a.
+	 */
+	gsnpsid = dwc2_readl(hsotg->regs + GSNPSID);
+	if (gsnpsid >= DWC2_CORE_REV_3_10a) {
+		u32 gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+
+		if (gotgctl & GOTGCTL_DBNCE_FLTR_BYPASS)
+			return false;
+	}
+
+	return true;
+}
+
 /*
  * Do core a soft reset of the core.  Be careful with this because it
  * resets all the internal state machines of the core.
@@ -246,9 +317,30 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 {
 	u32 greset;
 	int count = 0;
+	bool wait_for_host_mode = false;
 
 	dev_vdbg(hsotg->dev, "%s()\n", __func__);
 
+	/*
+	 * If the current mode is host, either due to the force mode
+	 * bit being set (which persists after core reset) or the
+	 * connector id pin, a core soft reset will temporarily reset
+	 * the mode to device. A delay from the IDDIG debounce filter
+	 * will occur before going back to host mode.
+	 *
+	 * Determine whether we will go back into host mode after a
+	 * reset and account for this delay after the reset.
+	 */
+	if (dwc2_iddig_filter_enabled(hsotg)) {
+		u32 gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+		u32 gusbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
+
+		if (!(gotgctl & GOTGCTL_CONID_B) ||
+		    (gusbcfg & GUSBCFG_FORCEHOSTMODE)) {
+			wait_for_host_mode = true;
+		}
+	}
+
 	/* Core Soft Reset */
 	greset = dwc2_readl(hsotg->regs + GRSTCTL);
 	greset |= GRSTCTL_CSFTRST;
@@ -277,6 +369,9 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 		}
 	} while (!(greset & GRSTCTL_AHBIDLE));
 
+	if (wait_for_host_mode)
+		dwc2_wait_for_mode(hsotg, true);
+
 	return 0;
 }
 
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 78526ee..466c220 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -882,6 +882,7 @@ struct dwc2_hsotg {
 #define DWC2_CORE_REV_2_92a	0x4f54292a
 #define DWC2_CORE_REV_2_94a	0x4f54294a
 #define DWC2_CORE_REV_3_00a	0x4f54300a
+#define DWC2_CORE_REV_3_10a	0x4f54310a
 
 #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
 	union dwc2_hcd_internal_flags {
diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
index efc3bcd..9105844 100644
--- a/drivers/usb/dwc2/hw.h
+++ b/drivers/usb/dwc2/hw.h
@@ -48,6 +48,7 @@
 #define GOTGCTL_ASESVLD			(1 << 18)
 #define GOTGCTL_DBNC_SHORT		(1 << 17)
 #define GOTGCTL_CONID_B			(1 << 16)
+#define GOTGCTL_DBNCE_FLTR_BYPASS	(1 << 15)
 #define GOTGCTL_DEVHNPEN		(1 << 11)
 #define GOTGCTL_HSTSETHNPEN		(1 << 10)
 #define GOTGCTL_HNPREQ			(1 << 9)
-- 
2.9.0

[PATCH v5 2/3] usb: dwc2: Add delay to core soft reset

[John Youn]

Add a delay to the core soft reset function to account for the IDDIG
debounce filter.

If the current mode is host, either due to the force mode bit being
set (which persists after core reset) or the connector id pin, a core
soft reset will temporarily reset the mode to device and a delay from
the IDDIG debounce filter will occur before going back to host mode.

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: John Youn <johnyoun@synopsys.com>
---
 drivers/usb/dwc2/core.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/core.h |  1 +
 drivers/usb/dwc2/hw.h   |  1 +
 3 files changed, 97 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 4135a5f..a3068e0 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -238,6 +238,77 @@ int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg)
 	return ret;
 }
 
+/**
+ * dwc2_wait_for_mode() - Waits for the controller mode.
+ * @hsotg:	Programming view of the DWC_otg controller.
+ * @host_mode:	If true, waits for host mode, otherwise device mode.
+ */
+static void dwc2_wait_for_mode(struct dwc2_hsotg *hsotg,
+			       bool host_mode)
+{
+	ktime_t start;
+	ktime_t end;
+	unsigned int timeout = 110;
+
+	dev_vdbg(hsotg->dev, "Waiting for %s mode\n",
+		 host_mode ? "host" : "device");
+
+	start = ktime_get();
+
+	while (1) {
+		s64 ms;
+
+		if (dwc2_is_host_mode(hsotg) == host_mode) {
+			dev_vdbg(hsotg->dev, "%s mode set\n",
+				 host_mode ? "Host" : "Device");
+			break;
+		}
+
+		end = ktime_get();
+		ms = ktime_to_ms(ktime_sub(end, start));
+
+		if (ms >= (s64)timeout) {
+			dev_warn(hsotg->dev, "%s: Couldn't set %s mode\n",
+				 __func__, host_mode ? "host" : "device");
+			break;
+		}
+
+		usleep_range(1000, 2000);
+	}
+}
+
+/**
+ * dwc2_iddig_filter_enabled() - Returns true if the IDDIG debounce
+ * filter is enabled.
+ */
+static bool dwc2_iddig_filter_enabled(struct dwc2_hsotg *hsotg)
+{
+	u32 gsnpsid;
+	u32 ghwcfg4;
+
+	if (!dwc2_hw_is_otg(hsotg))
+		return false;
+
+	/* Check if core configuration includes the IDDIG filter. */
+	ghwcfg4 = dwc2_readl(hsotg->regs + GHWCFG4);
+	if (!(ghwcfg4 & GHWCFG4_IDDIG_FILT_EN))
+		return false;
+
+	/*
+	 * Check if the IDDIG debounce filter is bypassed. Available
+	 * in core version >= 3.10a.
+	 */
+	gsnpsid = dwc2_readl(hsotg->regs + GSNPSID);
+	if (gsnpsid >= DWC2_CORE_REV_3_10a) {
+		u32 gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+
+		if (gotgctl & GOTGCTL_DBNCE_FLTR_BYPASS)
+			return false;
+	}
+
+	return true;
+}
+
 /*
  * Do core a soft reset of the core.  Be careful with this because it
  * resets all the internal state machines of the core.
@@ -246,9 +317,30 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 {
 	u32 greset;
 	int count = 0;
+	bool wait_for_host_mode = false;
 
 	dev_vdbg(hsotg->dev, "%s()\n", __func__);
 
+	/*
+	 * If the current mode is host, either due to the force mode
+	 * bit being set (which persists after core reset) or the
+	 * connector id pin, a core soft reset will temporarily reset
+	 * the mode to device. A delay from the IDDIG debounce filter
+	 * will occur before going back to host mode.
+	 *
+	 * Determine whether we will go back into host mode after a
+	 * reset and account for this delay after the reset.
+	 */
+	if (dwc2_iddig_filter_enabled(hsotg)) {
+		u32 gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+		u32 gusbcfg = dwc2_readl(hsotg->regs + GUSBCFG);
+
+		if (!(gotgctl & GOTGCTL_CONID_B) ||
+		    (gusbcfg & GUSBCFG_FORCEHOSTMODE)) {
+			wait_for_host_mode = true;
+		}
+	}
+
 	/* Core Soft Reset */
 	greset = dwc2_readl(hsotg->regs + GRSTCTL);
 	greset |= GRSTCTL_CSFTRST;
@@ -277,6 +369,9 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 		}
 	} while (!(greset & GRSTCTL_AHBIDLE));
 
+	if (wait_for_host_mode)
+		dwc2_wait_for_mode(hsotg, true);
+
 	return 0;
 }
 
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 78526ee..466c220 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -882,6 +882,7 @@ struct dwc2_hsotg {
 #define DWC2_CORE_REV_2_92a	0x4f54292a
 #define DWC2_CORE_REV_2_94a	0x4f54294a
 #define DWC2_CORE_REV_3_00a	0x4f54300a
+#define DWC2_CORE_REV_3_10a	0x4f54310a
 
 #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
 	union dwc2_hcd_internal_flags {
diff --git a/drivers/usb/dwc2/hw.h b/drivers/usb/dwc2/hw.h
index efc3bcd..9105844 100644
--- a/drivers/usb/dwc2/hw.h
+++ b/drivers/usb/dwc2/hw.h
@@ -48,6 +48,7 @@
 #define GOTGCTL_ASESVLD			(1 << 18)
 #define GOTGCTL_DBNC_SHORT		(1 << 17)
 #define GOTGCTL_CONID_B			(1 << 16)
+#define GOTGCTL_DBNCE_FLTR_BYPASS	(1 << 15)
 #define GOTGCTL_DEVHNPEN		(1 << 11)
 #define GOTGCTL_HSTSETHNPEN		(1 << 10)
 #define GOTGCTL_HNPREQ			(1 << 9)
-- 
2.9.0

[PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[John Youn]

When a force mode bit is set and the IDDIG debounce filter is enabled,
there is a delay for the forced mode to take effect. This delay is due
to the IDDIG debounce filter and is variable depending on the platform's
PHY clock speed. To account for this delay we can poll for the expected
mode.

On a clear force mode, since we don't know what mode to poll for, delay
for a fixed 100 ms. This is the maximum delay based on the slowest PHY
clock speed.

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: John Youn <johnyoun@synopsys.com>
---
 drivers/usb/dwc2/core.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index a3068e0..fa9b26b 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -395,9 +395,9 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
  * Checks are done in this function to determine whether doing a force
  * would be valid or not.
  *
- * If a force is done, it requires a 25ms delay to take effect.
- *
- * Returns true if the mode was forced.
+ * If a force is done, it requires a IDDIG debounce filter delay if
+ * the filter is configured and enabled. We poll the current mode of
+ * the controller to account for this delay.
  */
 static bool dwc2_force_mode(struct dwc2_hsotg *hsotg, bool host)
 {
@@ -432,12 +432,18 @@ static bool dwc2_force_mode(struct dwc2_hsotg *hsotg, bool host)
 	gusbcfg |= set;
 	dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
 
-	msleep(25);
+	dwc2_wait_for_mode(hsotg, host);
 	return true;
 }
 
-/*
- * Clears the force mode bits.
+/**
+ * dwc2_clear_force_mode() - Clears the force mode bits.
+ *
+ * After clearing the bits, wait up to 100 ms to account for any
+ * potential IDDIG filter delay. We can't know if we expect this delay
+ * or not because the value of the connector ID status is affected by
+ * the force mode. We only need to call this once during probe if
+ * dr_mode == OTG.
  */
 static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
 {
@@ -448,11 +454,8 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
 	gusbcfg &= ~GUSBCFG_FORCEDEVMODE;
 	dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
 
-	/*
-	 * NOTE: This long sleep is _very_ important, otherwise the core will
-	 * not stay in host mode after a connector ID change!
-	 */
-	msleep(25);
+	if (dwc2_iddig_filter_enabled(hsotg))
+		usleep_range(100000, 110000);
 }
 
 /*
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
 			 __func__, hsotg->dr_mode);
 		break;
 	}
-
-	/*
-	 * NOTE: This is required for some rockchip soc based
-	 * platforms.
-	 */
-	msleep(50);
 }
 
 /*
-- 
2.9.0

[PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[John Youn]

When a force mode bit is set and the IDDIG debounce filter is enabled,
there is a delay for the forced mode to take effect. This delay is due
to the IDDIG debounce filter and is variable depending on the platform's
PHY clock speed. To account for this delay we can poll for the expected
mode.

On a clear force mode, since we don't know what mode to poll for, delay
for a fixed 100 ms. This is the maximum delay based on the slowest PHY
clock speed.

Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
Signed-off-by: John Youn <johnyoun@synopsys.com>
---
 drivers/usb/dwc2/core.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index a3068e0..fa9b26b 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -395,9 +395,9 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
  * Checks are done in this function to determine whether doing a force
  * would be valid or not.
  *
- * If a force is done, it requires a 25ms delay to take effect.
- *
- * Returns true if the mode was forced.
+ * If a force is done, it requires a IDDIG debounce filter delay if
+ * the filter is configured and enabled. We poll the current mode of
+ * the controller to account for this delay.
  */
 static bool dwc2_force_mode(struct dwc2_hsotg *hsotg, bool host)
 {
@@ -432,12 +432,18 @@ static bool dwc2_force_mode(struct dwc2_hsotg *hsotg, bool host)
 	gusbcfg |= set;
 	dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
 
-	msleep(25);
+	dwc2_wait_for_mode(hsotg, host);
 	return true;
 }
 
-/*
- * Clears the force mode bits.
+/**
+ * dwc2_clear_force_mode() - Clears the force mode bits.
+ *
+ * After clearing the bits, wait up to 100 ms to account for any
+ * potential IDDIG filter delay. We can't know if we expect this delay
+ * or not because the value of the connector ID status is affected by
+ * the force mode. We only need to call this once during probe if
+ * dr_mode == OTG.
  */
 static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
 {
@@ -448,11 +454,8 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
 	gusbcfg &= ~GUSBCFG_FORCEDEVMODE;
 	dwc2_writel(gusbcfg, hsotg->regs + GUSBCFG);
 
-	/*
-	 * NOTE: This long sleep is _very_ important, otherwise the core will
-	 * not stay in host mode after a connector ID change!
-	 */
-	msleep(25);
+	if (dwc2_iddig_filter_enabled(hsotg))
+		usleep_range(100000, 110000);
 }
 
 /*
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
 			 __func__, hsotg->dr_mode);
 		break;
 	}
-
-	/*
-	 * NOTE: This is required for some rockchip soc based
-	 * platforms.
-	 */
-	msleep(50);
 }
 
 /*
-- 
2.9.0

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[Heiko Stuebner]

Hi John,

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
> When a force mode bit is set and the IDDIG debounce filter is enabled,
> there is a delay for the forced mode to take effect. This delay is due
> to the IDDIG debounce filter and is variable depending on the platform's
> PHY clock speed. To account for this delay we can poll for the expected
> mode.
> 
> On a clear force mode, since we don't know what mode to poll for, delay
> for a fixed 100 ms. This is the maximum delay based on the slowest PHY
> clock speed.
> 
> Tested-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> ---

[...]

> @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
>  			 __func__, hsotg->dr_mode);
>  		break;
>  	}
> -
> -	/*
> -	 * NOTE: This is required for some rockchip soc based
> -	 * platforms.
> -	 */
> -	msleep(50);
>  }

sorry for not finding the time to test your subsequent versions, but this still 
acts up on my Rockchip boards, as I'm still running into errors like
	[    4.875570] usb usb2-port1: connect-debounce failed

And it still requires the 50ms default sleep to work properly. But it seems I 
was able to find some interesting things when testing the individual parts of 
your patch. The port that is affected is a host-only port, so I can also get

[    3.862440] dwc2 101c0000.usb: dwc2_force_dr_mode() to mode 1
{custom debug in dwc2_force_dr_mode}
[    3.868223] dwc2 101c0000.usb: dwc2_force_mode() no OTG controller
{custom debug in dwc2_force_mode at if (!dwc2_hw_is_otg) }

I remember that I also did my previous tests on the host-only ports (since the 
otg ones are often also used as power-supply) but sadly I only have remote 
access to my boards this week, so cannot change the cabling to actually try 
with a real otg dwc2.


Heiko

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[Stefan Wahren]

Hi Heiko,

> Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 11. September 2016 um 23:19
> geschrieben:
> 
> 
> Hi John,
> 
> Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
> > When a force mode bit is set and the IDDIG debounce filter is enabled,
> > there is a delay for the forced mode to take effect. This delay is due
> > to the IDDIG debounce filter and is variable depending on the platform's
> > PHY clock speed. To account for this delay we can poll for the expected
> > mode.
> > 
> > On a clear force mode, since we don't know what mode to poll for, delay
> > for a fixed 100 ms. This is the maximum delay based on the slowest PHY
> > clock speed.
> > 
> > Tested-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> > Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > ---
> 
> [...]
> 
> > @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
> >  			 __func__, hsotg->dr_mode);
> >  		break;
> >  	}
> > -
> > -	/*
> > -	 * NOTE: This is required for some rockchip soc based
> > -	 * platforms.
> > -	 */
> > -	msleep(50);
> >  }
> 
> sorry for not finding the time to test your subsequent versions, but this
> still 
> acts up on my Rockchip boards, as I'm still running into errors like
> 	[    4.875570] usb usb2-port1: connect-debounce failed

could you please name the relevant DTS file of the affected boards?

Regards
Stefan

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[Heiko Stuebner]

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
> > Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 11. September 2016 um 23:19
> > geschrieben:
> > Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
> > > When a force mode bit is set and the IDDIG debounce filter is enabled,
> > > there is a delay for the forced mode to take effect. This delay is due
> > > to the IDDIG debounce filter and is variable depending on the platform's
> > > PHY clock speed. To account for this delay we can poll for the expected
> > > mode.
> > > 
> > > On a clear force mode, since we don't know what mode to poll for, delay
> > > for a fixed 100 ms. This is the maximum delay based on the slowest PHY
> > > clock speed.
> > > 
> > > Tested-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> > > Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > > ---
> > 
> > [...]
> > 
> > > @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
> > > 
> > >  			 __func__, hsotg->dr_mode);
> > >  		
> > >  		break;
> > >  	
> > >  	}
> > > 
> > > -
> > > -	/*
> > > -	 * NOTE: This is required for some rockchip soc based
> > > -	 * platforms.
> > > -	 */
> > > -	msleep(50);
> > > 
> > >  }
> > 
> > sorry for not finding the time to test your subsequent versions, but this
> > still
> > acts up on my Rockchip boards, as I'm still running into errors like
> > 
> > 	[    4.875570] usb usb2-port1: connect-debounce failed
> 
> could you please name the relevant DTS file of the affected boards?

So far I've been able to see that on

rk3188-radxarock
rk3036-kylin

both on host-only dwc2 controllers.

Checking my rk3288-veyron-pinky, I see my usb-ethernet coming up correctly and 
if I'm not mistaken (some sort of blindness when reading dmesg), there the 
adapter is connected to the real otg dwc2 on there.

So right now it really seems to be limited to host-only dwc2 controllers, but 
I guess I should double check that once I can switch cables around again.


Heiko

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[Stefan Wahren]

Hi Heiko,

> Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 12. September 2016 um 13:05
> geschrieben:
> 
> 
> Hi Stefan,
> 
> Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
> > > Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 11. September 2016 um 23:19
> > > geschrieben:
> > > Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
> > > > When a force mode bit is set and the IDDIG debounce filter is enabled,
> > > > there is a delay for the forced mode to take effect. This delay is due
> > > > to the IDDIG debounce filter and is variable depending on the platform's
> > > > PHY clock speed. To account for this delay we can poll for the expected
> > > > mode.
> > > > 
> > > > On a clear force mode, since we don't know what mode to poll for, delay
> > > > for a fixed 100 ms. This is the maximum delay based on the slowest PHY
> > > > clock speed.
> > > > 
> > > > Tested-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> > > > Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > > > ---
> > > 
> > > [...]
> > > 
> > > > @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
> > > > 
> > > >  			 __func__, hsotg->dr_mode);
> > > >  		
> > > >  		break;
> > > >  	
> > > >  	}
> > > > 
> > > > -
> > > > -	/*
> > > > -	 * NOTE: This is required for some rockchip soc based
> > > > -	 * platforms.
> > > > -	 */
> > > > -	msleep(50);
> > > > 
> > > >  }
> > > 
> > > sorry for not finding the time to test your subsequent versions, but this
> > > still
> > > acts up on my Rockchip boards, as I'm still running into errors like
> > > 
> > > 	[    4.875570] usb usb2-port1: connect-debounce failed
> > 
> > could you please name the relevant DTS file of the affected boards?
> 
> So far I've been able to see that on
> 
> rk3188-radxarock
> rk3036-kylin
> 
> both on host-only dwc2 controllers.

thanks, does patch 1 & 2 already have a negative effect on these controllers?

Stefan

> 
> Checking my rk3288-veyron-pinky, I see my usb-ethernet coming up correctly and
> 
> if I'm not mistaken (some sort of blindness when reading dmesg), there the 
> adapter is connected to the real otg dwc2 on there.
> 
> So right now it really seems to be limited to host-only dwc2 controllers, but 
> I guess I should double check that once I can switch cables around again.
> 
> 
> Heiko

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[Heiko Stübner]

Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:
> Hi Heiko,
> 
> > Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 12. September 2016 um 13:05
> > geschrieben:
> > 
> > 
> > Hi Stefan,
> > 
> > Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
> > > > Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 11. September 2016 um 23:19
> > > > geschrieben:
> > > > 
> > > > Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
> > > > > When a force mode bit is set and the IDDIG debounce filter is
> > > > > enabled,
> > > > > there is a delay for the forced mode to take effect. This delay is
> > > > > due
> > > > > to the IDDIG debounce filter and is variable depending on the
> > > > > platform's
> > > > > PHY clock speed. To account for this delay we can poll for the
> > > > > expected
> > > > > mode.
> > > > > 
> > > > > On a clear force mode, since we don't know what mode to poll for,
> > > > > delay
> > > > > for a fixed 100 ms. This is the maximum delay based on the slowest
> > > > > PHY
> > > > > clock speed.
> > > > > 
> > > > > Tested-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> > > > > Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> > > > > ---
> > > > 
> > > > [...]
> > > > 
> > > > > @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
> > > > > *hsotg)
> > > > > 
> > > > >  			 __func__, hsotg->dr_mode);
> > > > >  		
> > > > >  		break;
> > > > >  	
> > > > >  	}
> > > > > 
> > > > > -
> > > > > -	/*
> > > > > -	 * NOTE: This is required for some rockchip soc based
> > > > > -	 * platforms.
> > > > > -	 */
> > > > > -	msleep(50);
> > > > > 
> > > > >  }
> > > > 
> > > > sorry for not finding the time to test your subsequent versions, but
> > > > this
> > > > still
> > > > acts up on my Rockchip boards, as I'm still running into errors like
> > > > 
> > > > 	[    4.875570] usb usb2-port1: connect-debounce failed
> > > 
> > > could you please name the relevant DTS file of the affected boards?
> > 
> > So far I've been able to see that on
> > 
> > rk3188-radxarock
> > rk3036-kylin
> > 
> > both on host-only dwc2 controllers.
> 
> thanks, does patch 1 & 2 already have a negative effect on these
> controllers?

nope, patches 1+2 alone are ok and only the msleep(50) going away causes 
problems. I'm back home now, so I'll hopefully have time to check host-only 
vs. otg dwc2 controllers tomorrow as well.


Heiko

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[John Youn]

On 9/13/2016 11:39 AM, Heiko Stübner wrote:
> Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:
>> Hi Heiko,
>>
>>> Heiko Stuebner <heiko@sntech.de> hat am 12. September 2016 um 13:05
>>> geschrieben:
>>>
>>>
>>> Hi Stefan,
>>>
>>> Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
>>>>> Heiko Stuebner <heiko@sntech.de> hat am 11. September 2016 um 23:19
>>>>> geschrieben:
>>>>>
>>>>> Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
>>>>>> When a force mode bit is set and the IDDIG debounce filter is
>>>>>> enabled,
>>>>>> there is a delay for the forced mode to take effect. This delay is
>>>>>> due
>>>>>> to the IDDIG debounce filter and is variable depending on the
>>>>>> platform's
>>>>>> PHY clock speed. To account for this delay we can poll for the
>>>>>> expected
>>>>>> mode.
>>>>>>
>>>>>> On a clear force mode, since we don't know what mode to poll for,
>>>>>> delay
>>>>>> for a fixed 100 ms. This is the maximum delay based on the slowest
>>>>>> PHY
>>>>>> clock speed.
>>>>>>
>>>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>>> Signed-off-by: John Youn <johnyoun@synopsys.com>
>>>>>> ---
>>>>>
>>>>> [...]
>>>>>
>>>>>> @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
>>>>>> *hsotg)
>>>>>>
>>>>>>  			 __func__, hsotg->dr_mode);
>>>>>>  		
>>>>>>  		break;
>>>>>>  	
>>>>>>  	}
>>>>>>
>>>>>> -
>>>>>> -	/*
>>>>>> -	 * NOTE: This is required for some rockchip soc based
>>>>>> -	 * platforms.
>>>>>> -	 */
>>>>>> -	msleep(50);
>>>>>>
>>>>>>  }
>>>>>
>>>>> sorry for not finding the time to test your subsequent versions, but
>>>>> this
>>>>> still
>>>>> acts up on my Rockchip boards, as I'm still running into errors like
>>>>>
>>>>> 	[    4.875570] usb usb2-port1: connect-debounce failed
>>>>
>>>> could you please name the relevant DTS file of the affected boards?
>>>
>>> So far I've been able to see that on
>>>
>>> rk3188-radxarock
>>> rk3036-kylin
>>>
>>> both on host-only dwc2 controllers.
>>
>> thanks, does patch 1 & 2 already have a negative effect on these
>> controllers?
> 
> nope, patches 1+2 alone are ok and only the msleep(50) going away causes 
> problems. I'm back home now, so I'll hopefully have time to check host-only 
> vs. otg dwc2 controllers tomorrow as well.
> 

Ok, thanks for testing Heiko.

Given that, let's try to fix this in the next -rc (or before) because
I think the Raspberry Pi needs the other parts of that last patch. We
can just revert the msleep(50) if needed.

I'll also try to get my hands on a rockchip platform. I think that
will help the testing along.

Regards,
John

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[Heiko Stübner]

Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:
> On 9/13/2016 11:39 AM, Heiko Stübner wrote:
> > Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:
> >> Hi Heiko,
> >> 
> >>> Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 12. September 2016 um 13:05
> >>> geschrieben:
> >>> 
> >>> 
> >>> Hi Stefan,
> >>> 
> >>> Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
> >>>>> Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 11. September 2016 um 23:19
> >>>>> geschrieben:
> >>>>> 
> >>>>> Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
> >>>>>> When a force mode bit is set and the IDDIG debounce filter is
> >>>>>> enabled,
> >>>>>> there is a delay for the forced mode to take effect. This delay is
> >>>>>> due
> >>>>>> to the IDDIG debounce filter and is variable depending on the
> >>>>>> platform's
> >>>>>> PHY clock speed. To account for this delay we can poll for the
> >>>>>> expected
> >>>>>> mode.
> >>>>>> 
> >>>>>> On a clear force mode, since we don't know what mode to poll for,
> >>>>>> delay
> >>>>>> for a fixed 100 ms. This is the maximum delay based on the slowest
> >>>>>> PHY
> >>>>>> clock speed.
> >>>>>> 
> >>>>>> Tested-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
> >>>>>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
> >>>>>> ---
> >>>>> 
> >>>>> [...]
> >>>>> 
> >>>>>> @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
> >>>>>> *hsotg)
> >>>>>> 
> >>>>>>  			 __func__, hsotg->dr_mode);
> >>>>>>  		
> >>>>>>  		break;
> >>>>>>  	
> >>>>>>  	}
> >>>>>> 
> >>>>>> -
> >>>>>> -	/*
> >>>>>> -	 * NOTE: This is required for some rockchip soc based
> >>>>>> -	 * platforms.
> >>>>>> -	 */
> >>>>>> -	msleep(50);
> >>>>>> 
> >>>>>>  }
> >>>>> 
> >>>>> sorry for not finding the time to test your subsequent versions, but
> >>>>> this
> >>>>> still
> >>>>> acts up on my Rockchip boards, as I'm still running into errors like
> >>>>> 
> >>>>> 	[    4.875570] usb usb2-port1: connect-debounce failed
> >>>> 
> >>>> could you please name the relevant DTS file of the affected boards?
> >>> 
> >>> So far I've been able to see that on
> >>> 
> >>> rk3188-radxarock
> >>> rk3036-kylin
> >>> 
> >>> both on host-only dwc2 controllers.
> >> 
> >> thanks, does patch 1 & 2 already have a negative effect on these
> >> controllers?
> > 
> > nope, patches 1+2 alone are ok and only the msleep(50) going away causes
> > problems. I'm back home now, so I'll hopefully have time to check
> > host-only
> > vs. otg dwc2 controllers tomorrow as well.
> 
> Ok, thanks for testing Heiko.
> 
> Given that, let's try to fix this in the next -rc (or before) because
> I think the Raspberry Pi needs the other parts of that last patch. We
> can just revert the msleep(50) if needed.

I could nicely confirm, that only the host-only controller seems affected by
this. Same kernel image that fails on the host-only one with the "connect-
debounce failed" message seems to work once I switch that over to the  otg
controller and the system comes up nicely again.

So if we don't find the root cause in the short term, maybe we could at least
limit the delay for host-only variants, like

---------------------- 8< -------------------
diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index fa9b26b..4c0fa0b 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -463,9 +463,18 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
  */
 void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
 {
+	bool ret;
+
 	switch (hsotg->dr_mode) {
 	case USB_DR_MODE_HOST:
-		dwc2_force_mode(hsotg, true);
+		ret = dwc2_force_mode(hsotg, true);
+		/*
+		 * NOTE: This is required for some rockchip soc based
+		 * platforms on their host-only dwc2.
+		 */
+		if (!ret)
+			msleep(50);
+
 		break;
 	case USB_DR_MODE_PERIPHERAL:
 		dwc2_force_mode(hsotg, false);
---------------------- 8< -------------------

I've been running with that change for some time now on both the
rk3036-kylin and rk3188-radxarock. Plugged and unplugged different
combinations of usb-devices on the usb otg and host ports, so far
seems to work.


Heiko

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[John Youn]

On 9/14/2016 7:05 AM, Heiko Stübner wrote:
> Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:
>> On 9/13/2016 11:39 AM, Heiko Stübner wrote:
>>> Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:
>>>> Hi Heiko,
>>>>
>>>>> Heiko Stuebner <heiko@sntech.de> hat am 12. September 2016 um 13:05
>>>>> geschrieben:
>>>>>
>>>>>
>>>>> Hi Stefan,
>>>>>
>>>>> Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
>>>>>>> Heiko Stuebner <heiko@sntech.de> hat am 11. September 2016 um 23:19
>>>>>>> geschrieben:
>>>>>>>
>>>>>>> Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
>>>>>>>> When a force mode bit is set and the IDDIG debounce filter is
>>>>>>>> enabled,
>>>>>>>> there is a delay for the forced mode to take effect. This delay is
>>>>>>>> due
>>>>>>>> to the IDDIG debounce filter and is variable depending on the
>>>>>>>> platform's
>>>>>>>> PHY clock speed. To account for this delay we can poll for the
>>>>>>>> expected
>>>>>>>> mode.
>>>>>>>>
>>>>>>>> On a clear force mode, since we don't know what mode to poll for,
>>>>>>>> delay
>>>>>>>> for a fixed 100 ms. This is the maximum delay based on the slowest
>>>>>>>> PHY
>>>>>>>> clock speed.
>>>>>>>>
>>>>>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>>>>> Signed-off-by: John Youn <johnyoun@synopsys.com>
>>>>>>>> ---
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
>>>>>>>> *hsotg)
>>>>>>>>
>>>>>>>>  			 __func__, hsotg->dr_mode);
>>>>>>>>  		
>>>>>>>>  		break;
>>>>>>>>  	
>>>>>>>>  	}
>>>>>>>>
>>>>>>>> -
>>>>>>>> -	/*
>>>>>>>> -	 * NOTE: This is required for some rockchip soc based
>>>>>>>> -	 * platforms.
>>>>>>>> -	 */
>>>>>>>> -	msleep(50);
>>>>>>>>
>>>>>>>>  }
>>>>>>>
>>>>>>> sorry for not finding the time to test your subsequent versions, but
>>>>>>> this
>>>>>>> still
>>>>>>> acts up on my Rockchip boards, as I'm still running into errors like
>>>>>>>
>>>>>>> 	[    4.875570] usb usb2-port1: connect-debounce failed
>>>>>>
>>>>>> could you please name the relevant DTS file of the affected boards?
>>>>>
>>>>> So far I've been able to see that on
>>>>>
>>>>> rk3188-radxarock
>>>>> rk3036-kylin
>>>>>
>>>>> both on host-only dwc2 controllers.
>>>>
>>>> thanks, does patch 1 & 2 already have a negative effect on these
>>>> controllers?
>>>
>>> nope, patches 1+2 alone are ok and only the msleep(50) going away causes
>>> problems. I'm back home now, so I'll hopefully have time to check
>>> host-only
>>> vs. otg dwc2 controllers tomorrow as well.
>>
>> Ok, thanks for testing Heiko.
>>
>> Given that, let's try to fix this in the next -rc (or before) because
>> I think the Raspberry Pi needs the other parts of that last patch. We
>> can just revert the msleep(50) if needed.
> 
> I could nicely confirm, that only the host-only controller seems affected by
> this. Same kernel image that fails on the host-only one with the "connect-
> debounce failed" message seems to work once I switch that over to the  otg
> controller and the system comes up nicely again.
> 
> So if we don't find the root cause in the short term, maybe we could at least
> limit the delay for host-only variants, like
> 
> ---------------------- 8< -------------------
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index fa9b26b..4c0fa0b 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -463,9 +463,18 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
>   */
>  void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
>  {
> +	bool ret;
> +
>  	switch (hsotg->dr_mode) {
>  	case USB_DR_MODE_HOST:
> -		dwc2_force_mode(hsotg, true);
> +		ret = dwc2_force_mode(hsotg, true);
> +		/*
> +		 * NOTE: This is required for some rockchip soc based
> +		 * platforms on their host-only dwc2.
> +		 */
> +		if (!ret)
> +			msleep(50);
> +
>  		break;
>  	case USB_DR_MODE_PERIPHERAL:
>  		dwc2_force_mode(hsotg, false);
> ---------------------- 8< -------------------
> 
> I've been running with that change for some time now on both the
> rk3036-kylin and rk3188-radxarock. Plugged and unplugged different
> combinations of usb-devices on the usb otg and host ports, so far
> seems to work.
> 

Ok great. We'll use that for the -rc if we can't find the root cause.

Then hopefully we can apply the final dropped patch for 4.10.

Regards,
John

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[Stefan Wahren]

Hi John,

[add Felipe and Greg]

Am 14.09.2016 um 23:10 schrieb John Youn:
> On 9/14/2016 7:05 AM, Heiko Stübner wrote:
>> Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:
>>> On 9/13/2016 11:39 AM, Heiko Stübner wrote:
>>>> Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:
>>>>> Hi Heiko,
>>>>>
>>>>>> Heiko Stuebner <heiko@sntech.de> hat am 12. September 2016 um 13:05
>>>>>> geschrieben:
>>>>>>
>>>>>>
>>>>>> Hi Stefan,
>>>>>>
>>>>>> Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
>>>>>>>> Heiko Stuebner <heiko@sntech.de> hat am 11. September 2016 um 23:19
>>>>>>>> geschrieben:
>>>>>>>>
>>>>>>>> Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
>>>>>>>>> When a force mode bit is set and the IDDIG debounce filter is
>>>>>>>>> enabled,
>>>>>>>>> there is a delay for the forced mode to take effect. This delay is
>>>>>>>>> due
>>>>>>>>> to the IDDIG debounce filter and is variable depending on the
>>>>>>>>> platform's
>>>>>>>>> PHY clock speed. To account for this delay we can poll for the
>>>>>>>>> expected
>>>>>>>>> mode.
>>>>>>>>>
>>>>>>>>> On a clear force mode, since we don't know what mode to poll for,
>>>>>>>>> delay
>>>>>>>>> for a fixed 100 ms. This is the maximum delay based on the slowest
>>>>>>>>> PHY
>>>>>>>>> clock speed.
>>>>>>>>>
>>>>>>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>>>>>> Signed-off-by: John Youn <johnyoun@synopsys.com>
>>>>>>>>> ---
>>>>>>>> [...]
>>>>>>>>
>>>>>>>>> @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
>>>>>>>>> *hsotg)
>>>>>>>>>
>>>>>>>>>   			 __func__, hsotg->dr_mode);
>>>>>>>>>   		
>>>>>>>>>   		break;
>>>>>>>>>   	
>>>>>>>>>   	}
>>>>>>>>>
>>>>>>>>> -
>>>>>>>>> -	/*
>>>>>>>>> -	 * NOTE: This is required for some rockchip soc based
>>>>>>>>> -	 * platforms.
>>>>>>>>> -	 */
>>>>>>>>> -	msleep(50);
>>>>>>>>>
>>>>>>>>>   }
>>>>>>>> sorry for not finding the time to test your subsequent versions, but
>>>>>>>> this
>>>>>>>> still
>>>>>>>> acts up on my Rockchip boards, as I'm still running into errors like
>>>>>>>>
>>>>>>>> 	[    4.875570] usb usb2-port1: connect-debounce failed
>>>>>>> could you please name the relevant DTS file of the affected boards?
>>>>>> So far I've been able to see that on
>>>>>>
>>>>>> rk3188-radxarock
>>>>>> rk3036-kylin
>>>>>>
>>>>>> both on host-only dwc2 controllers.
>>>>> thanks, does patch 1 & 2 already have a negative effect on these
>>>>> controllers?
>>>> nope, patches 1+2 alone are ok and only the msleep(50) going away causes
>>>> problems. I'm back home now, so I'll hopefully have time to check
>>>> host-only
>>>> vs. otg dwc2 controllers tomorrow as well.
>>> Ok, thanks for testing Heiko.
>>>
>>> Given that, let's try to fix this in the next -rc (or before) because
>>> I think the Raspberry Pi needs the other parts of that last patch. We
>>> can just revert the msleep(50) if needed.
>> I could nicely confirm, that only the host-only controller seems affected by
>> this. Same kernel image that fails on the host-only one with the "connect-
>> debounce failed" message seems to work once I switch that over to the  otg
>> controller and the system comes up nicely again.
>>
>> So if we don't find the root cause in the short term, maybe we could at least
>> limit the delay for host-only variants, like
>>
>> ---------------------- 8< -------------------
>> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
>> index fa9b26b..4c0fa0b 100644
>> --- a/drivers/usb/dwc2/core.c
>> +++ b/drivers/usb/dwc2/core.c
>> @@ -463,9 +463,18 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
>>    */
>>   void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
>>   {
>> +	bool ret;
>> +
>>   	switch (hsotg->dr_mode) {
>>   	case USB_DR_MODE_HOST:
>> -		dwc2_force_mode(hsotg, true);
>> +		ret = dwc2_force_mode(hsotg, true);
>> +		/*
>> +		 * NOTE: This is required for some rockchip soc based
>> +		 * platforms on their host-only dwc2.
>> +		 */
>> +		if (!ret)
>> +			msleep(50);
>> +
>>   		break;
>>   	case USB_DR_MODE_PERIPHERAL:
>>   		dwc2_force_mode(hsotg, false);
>> ---------------------- 8< -------------------
>>
>> I've been running with that change for some time now on both the
>> rk3036-kylin and rk3188-radxarock. Plugged and unplugged different
>> combinations of usb-devices on the usb otg and host ports, so far
>> seems to work.
>>
> Ok great. We'll use that for the -rc if we can't find the root cause.
>
> Then hopefully we can apply the final dropped patch for 4.10.

unfortunately i noticed too late that Felipe and Greg didn't get the 
whole conversation. The series v5 has been merged in Greg's USB tree.

Stefan

>
> Regards,
> John


_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[John Youn]

On 9/15/2016 12:08 AM, Stefan Wahren wrote:
> Hi John,
> 
> [add Felipe and Greg]
> 
> Am 14.09.2016 um 23:10 schrieb John Youn:
>> On 9/14/2016 7:05 AM, Heiko Stübner wrote:
>>> Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:
>>>> On 9/13/2016 11:39 AM, Heiko Stübner wrote:
>>>>> Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:
>>>>>> Hi Heiko,
>>>>>>
>>>>>>> Heiko Stuebner <heiko@sntech.de> hat am 12. September 2016 um 13:05
>>>>>>> geschrieben:
>>>>>>>
>>>>>>>
>>>>>>> Hi Stefan,
>>>>>>>
>>>>>>> Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
>>>>>>>>> Heiko Stuebner <heiko@sntech.de> hat am 11. September 2016 um 23:19
>>>>>>>>> geschrieben:
>>>>>>>>>
>>>>>>>>> Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
>>>>>>>>>> When a force mode bit is set and the IDDIG debounce filter is
>>>>>>>>>> enabled,
>>>>>>>>>> there is a delay for the forced mode to take effect. This delay is
>>>>>>>>>> due
>>>>>>>>>> to the IDDIG debounce filter and is variable depending on the
>>>>>>>>>> platform's
>>>>>>>>>> PHY clock speed. To account for this delay we can poll for the
>>>>>>>>>> expected
>>>>>>>>>> mode.
>>>>>>>>>>
>>>>>>>>>> On a clear force mode, since we don't know what mode to poll for,
>>>>>>>>>> delay
>>>>>>>>>> for a fixed 100 ms. This is the maximum delay based on the slowest
>>>>>>>>>> PHY
>>>>>>>>>> clock speed.
>>>>>>>>>>
>>>>>>>>>> Tested-by: Stefan Wahren <stefan.wahren@i2se.com>
>>>>>>>>>> Signed-off-by: John Youn <johnyoun@synopsys.com>
>>>>>>>>>> ---
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>> @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
>>>>>>>>>> *hsotg)
>>>>>>>>>>
>>>>>>>>>>   			 __func__, hsotg->dr_mode);
>>>>>>>>>>   		
>>>>>>>>>>   		break;
>>>>>>>>>>   	
>>>>>>>>>>   	}
>>>>>>>>>>
>>>>>>>>>> -
>>>>>>>>>> -	/*
>>>>>>>>>> -	 * NOTE: This is required for some rockchip soc based
>>>>>>>>>> -	 * platforms.
>>>>>>>>>> -	 */
>>>>>>>>>> -	msleep(50);
>>>>>>>>>>
>>>>>>>>>>   }
>>>>>>>>> sorry for not finding the time to test your subsequent versions, but
>>>>>>>>> this
>>>>>>>>> still
>>>>>>>>> acts up on my Rockchip boards, as I'm still running into errors like
>>>>>>>>>
>>>>>>>>> 	[    4.875570] usb usb2-port1: connect-debounce failed
>>>>>>>> could you please name the relevant DTS file of the affected boards?
>>>>>>> So far I've been able to see that on
>>>>>>>
>>>>>>> rk3188-radxarock
>>>>>>> rk3036-kylin
>>>>>>>
>>>>>>> both on host-only dwc2 controllers.
>>>>>> thanks, does patch 1 & 2 already have a negative effect on these
>>>>>> controllers?
>>>>> nope, patches 1+2 alone are ok and only the msleep(50) going away causes
>>>>> problems. I'm back home now, so I'll hopefully have time to check
>>>>> host-only
>>>>> vs. otg dwc2 controllers tomorrow as well.
>>>> Ok, thanks for testing Heiko.
>>>>
>>>> Given that, let's try to fix this in the next -rc (or before) because
>>>> I think the Raspberry Pi needs the other parts of that last patch. We
>>>> can just revert the msleep(50) if needed.
>>> I could nicely confirm, that only the host-only controller seems affected by
>>> this. Same kernel image that fails on the host-only one with the "connect-
>>> debounce failed" message seems to work once I switch that over to the  otg
>>> controller and the system comes up nicely again.
>>>
>>> So if we don't find the root cause in the short term, maybe we could at least
>>> limit the delay for host-only variants, like
>>>
>>> ---------------------- 8< -------------------
>>> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
>>> index fa9b26b..4c0fa0b 100644
>>> --- a/drivers/usb/dwc2/core.c
>>> +++ b/drivers/usb/dwc2/core.c
>>> @@ -463,9 +463,18 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
>>>    */
>>>   void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
>>>   {
>>> +	bool ret;
>>> +
>>>   	switch (hsotg->dr_mode) {
>>>   	case USB_DR_MODE_HOST:
>>> -		dwc2_force_mode(hsotg, true);
>>> +		ret = dwc2_force_mode(hsotg, true);
>>> +		/*
>>> +		 * NOTE: This is required for some rockchip soc based
>>> +		 * platforms on their host-only dwc2.
>>> +		 */
>>> +		if (!ret)
>>> +			msleep(50);
>>> +
>>>   		break;
>>>   	case USB_DR_MODE_PERIPHERAL:
>>>   		dwc2_force_mode(hsotg, false);
>>> ---------------------- 8< -------------------
>>>
>>> I've been running with that change for some time now on both the
>>> rk3036-kylin and rk3188-radxarock. Plugged and unplugged different
>>> combinations of usb-devices on the usb otg and host ports, so far
>>> seems to work.
>>>
>> Ok great. We'll use that for the -rc if we can't find the root cause.
>>
>> Then hopefully we can apply the final dropped patch for 4.10.
> 
> unfortunately i noticed too late that Felipe and Greg didn't get the 
> whole conversation. The series v5 has been merged in Greg's USB tree.
> 

Yeah, we'll have to fix in -rc. Either revert the last patch or add
the msleep.

Regards,
John

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v5 3/3] usb: dwc2: Properly account for the force mode delays

[John Youn]

On 9/14/2016 7:05 AM, Heiko Stübner wrote:
> Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:
>> On 9/13/2016 11:39 AM, Heiko Stübner wrote:
>>> Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:
>>>> Hi Heiko,
>>>>
>>>>> Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 12. September 2016 um 13:05
>>>>> geschrieben:
>>>>>
>>>>>
>>>>> Hi Stefan,
>>>>>
>>>>> Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:
>>>>>>> Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> hat am 11. September 2016 um 23:19
>>>>>>> geschrieben:
>>>>>>>
>>>>>>> Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:
>>>>>>>> When a force mode bit is set and the IDDIG debounce filter is
>>>>>>>> enabled,
>>>>>>>> there is a delay for the forced mode to take effect. This delay is
>>>>>>>> due
>>>>>>>> to the IDDIG debounce filter and is variable depending on the
>>>>>>>> platform's
>>>>>>>> PHY clock speed. To account for this delay we can poll for the
>>>>>>>> expected
>>>>>>>> mode.
>>>>>>>>
>>>>>>>> On a clear force mode, since we don't know what mode to poll for,
>>>>>>>> delay
>>>>>>>> for a fixed 100 ms. This is the maximum delay based on the slowest
>>>>>>>> PHY
>>>>>>>> clock speed.
>>>>>>>>
>>>>>>>> Tested-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
>>>>>>>> Signed-off-by: John Youn <johnyoun-HKixBCOQz3hWk0Htik3J/w@public.gmane.org>
>>>>>>>> ---
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> @@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
>>>>>>>> *hsotg)
>>>>>>>>
>>>>>>>>  			 __func__, hsotg->dr_mode);
>>>>>>>>  		
>>>>>>>>  		break;
>>>>>>>>  	
>>>>>>>>  	}
>>>>>>>>
>>>>>>>> -
>>>>>>>> -	/*
>>>>>>>> -	 * NOTE: This is required for some rockchip soc based
>>>>>>>> -	 * platforms.
>>>>>>>> -	 */
>>>>>>>> -	msleep(50);
>>>>>>>>
>>>>>>>>  }
>>>>>>>
>>>>>>> sorry for not finding the time to test your subsequent versions, but
>>>>>>> this
>>>>>>> still
>>>>>>> acts up on my Rockchip boards, as I'm still running into errors like
>>>>>>>
>>>>>>> 	[    4.875570] usb usb2-port1: connect-debounce failed
>>>>>>
>>>>>> could you please name the relevant DTS file of the affected boards?
>>>>>
>>>>> So far I've been able to see that on
>>>>>
>>>>> rk3188-radxarock
>>>>> rk3036-kylin
>>>>>
>>>>> both on host-only dwc2 controllers.
>>>>
>>>> thanks, does patch 1 & 2 already have a negative effect on these
>>>> controllers?
>>>
>>> nope, patches 1+2 alone are ok and only the msleep(50) going away causes
>>> problems. I'm back home now, so I'll hopefully have time to check
>>> host-only
>>> vs. otg dwc2 controllers tomorrow as well.
>>
>> Ok, thanks for testing Heiko.
>>
>> Given that, let's try to fix this in the next -rc (or before) because
>> I think the Raspberry Pi needs the other parts of that last patch. We
>> can just revert the msleep(50) if needed.
> 
> I could nicely confirm, that only the host-only controller seems affected by
> this. Same kernel image that fails on the host-only one with the "connect-
> debounce failed" message seems to work once I switch that over to the  otg
> controller and the system comes up nicely again.
> 
> So if we don't find the root cause in the short term, maybe we could at least
> limit the delay for host-only variants, like
> 
> ---------------------- 8< -------------------
> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
> index fa9b26b..4c0fa0b 100644
> --- a/drivers/usb/dwc2/core.c
> +++ b/drivers/usb/dwc2/core.c
> @@ -463,9 +463,18 @@ static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
>   */
>  void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
>  {
> +	bool ret;
> +
>  	switch (hsotg->dr_mode) {
>  	case USB_DR_MODE_HOST:
> -		dwc2_force_mode(hsotg, true);
> +		ret = dwc2_force_mode(hsotg, true);
> +		/*
> +		 * NOTE: This is required for some rockchip soc based
> +		 * platforms on their host-only dwc2.
> +		 */
> +		if (!ret)
> +			msleep(50);
> +
>  		break;
>  	case USB_DR_MODE_PERIPHERAL:
>  		dwc2_force_mode(hsotg, false);
> ---------------------- 8< -------------------
> 
> I've been running with that change for some time now on both the
> rk3036-kylin and rk3188-radxarock. Plugged and unplugged different
> combinations of usb-devices on the usb otg and host ports, so far
> seems to work.
> 
> 

Hi Heiko,

Let's add the delay just as you have it here.

I'm not sure how the delay comes into play in host-only hardware
though. I will see if the RTL engineers know why that might be needed.

Regards,
John


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html