Skip to contentSkip to conversation

usb: dwc2: Fix core reset and force mode delays

PATCH In review John Youn 4 participants 15 messages Sep 8, 2016 – Sep 16, 2016 6 messages not in the archive

Subsystems (3)
lore
Share & tools
lore

John Youn

author
#0 lore raw

v5 cover usb: dwc2: Fix core reset and force mode delays

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(-)
Signature · 1 line
2.9.0
author
#1 lore raw

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

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

---
 drivers/usb/dwc2/gadget.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/usb/dwc2/gadget.c +5 -2
index 94bd19a..4cd6403 100644
@@ -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);
 
Signature · 1 line
2.9.0
author
#2 lore raw

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

In reply to 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.

---
 drivers/usb/dwc2/core.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/core.h |  1 +
 drivers/usb/dwc2/hw.h   |  1 +
 3 files changed, 97 insertions(+)
drivers/usb/dwc2/core.c +95 -0
index 4135a5f..a3068e0 100644
@@ -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;
 }
 
drivers/usb/dwc2/core.h +1 -0
index 78526ee..466c220 100644
@@ -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 {
drivers/usb/dwc2/hw.h +1 -0
index efc3bcd..9105844 100644
@@ -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)
Signature · 1 line
2.9.0
author
#3 lore raw

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

In reply to 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.

---
 drivers/usb/dwc2/core.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)
drivers/usb/dwc2/core.c +14 -17
index a3068e0..fa9b26b 100644
@@ -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);
 }
 
 /*
Signature · 1 line
2.9.0

A message that is not in the archive (sent off-list or not yet received): b4aac4b197241b8199c5300656d245692eb45856.1473302184.git.johnyoun-HKixBCOQz3hWk0Htik3J/[email protected]

#5 lore raw

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

Hi John,

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 13 lines

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.

---

[...]

drivers/usb/dwc2/core.c void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg) · line 481

Quoted 11 lines
drivers/usb/dwc2/core.c +0 -6
@@ -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

#6 lore raw

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

Hi Heiko,

Quoted 38 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:

Hi John,

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 13 lines

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.

---

[...]

drivers/usb/dwc2/core.c void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg) · line 481

Quoted 11 lines
drivers/usb/dwc2/core.c +0 -6
@@ -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

Quoted 1 line
	[    4.875570] usb usb2-port1: connect-debounce failed

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

Regards
Stefan

A message that is not in the archive (sent off-list or not yet received): 1336050364.27663.591ba3c4-a2b5-418f-9999-6b3cd631f440.open-xchange-7tX72C7vayboQLBSYMtkGA@public.gmane.org

#8 lore raw

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

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:

Quoted 40 lines
Quoted 40 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:
Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 13 lines

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.

---

[...]

Quoted 6 lines
drivers/usb/dwc2/core.c +0 -0
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
 
 			 __func__, hsotg->dr_mode);
 
 		break;
 

drivers/usb/dwc2/core.c void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)

Quoted 10 lines
drivers/usb/dwc2/core.c +0 -6
 	}
 
-
-	/*
-	 * 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

Quoted 3 lines
Quoted 1 line
	[    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

#9 lore raw

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

Hi Heiko,

Quoted 49 lines

Heiko Stuebner <[email protected]> hat am 12. September 2016 um 13:05 geschrieben:

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:

Quoted 42 lines
Quoted 41 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:
Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 13 lines

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.

---

[...]

Quoted 6 lines
drivers/usb/dwc2/core.c +0 -0
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)
 
 			 __func__, hsotg->dr_mode);
 
 		break;
 

drivers/usb/dwc2/core.c void dwc2_force_dr_mode(struct dwc2_hsotg *hsotg)

Quoted 10 lines
drivers/usb/dwc2/core.c +0 -6
 	}
 
-
-	/*
-	 * 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
Quoted 8 lines
Quoted 1 line

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

Quoted 11 lines

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

A message that is not in the archive (sent off-list or not yet received): 19473401.122600.97acfbc9-df96-4427-b533-911010ce5c3f.open-xchange-7tX72C7vayboQLBSYMtkGA@public.gmane.org

#11 lore raw

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

Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:

Quoted 64 lines

Hi Heiko,

Quoted 62 lines

Heiko Stuebner <[email protected]> hat am 12. September 2016 um 13:05 geschrieben:

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:

Quoted 49 lines
Quoted 49 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 19 lines

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.

---

[...]

Quoted 7 lines
(quoted diff) +0 -0
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
 *hsotg)
 
 			 __func__, hsotg->dr_mode);
 
 		break;
 

Review void dwc2_force_dr_mode(struct dwc2_hsotg

Quoted 10 lines
(quoted diff) +0 -6
 	}
 
-
-	/*
-	 * 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

Quoted 3 lines
Quoted 1 line
	[    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

Quoted 7 lines
Quoted 4 lines

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

#12 lore raw

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

Quoted 68 lines · Heiko Stübner

Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:

Quoted 67 lines

Hi Heiko,

Quoted 65 lines

Heiko Stuebner <[email protected]> hat am 12. September 2016 um 13:05 geschrieben:

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:

Quoted 49 lines
Quoted 49 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 19 lines

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.

---

[...]

Quoted 7 lines
(quoted diff) +0 -0
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
 *hsotg)
 
 			 __func__, hsotg->dr_mode);
 
 		break;
 

Review void dwc2_force_dr_mode(struct dwc2_hsotg

Quoted 10 lines
(quoted diff) +0 -6
 	}
 
-
-	/*
-	 * 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

Quoted 3 lines
Quoted 1 line
	[    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

Quoted 9 lines
Quoted 4 lines
Quoted 1 line

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

A message that is not in the archive (sent off-list or not yet received): 97a3539d-7176-086f-71f9-371d8d4b016c-HKixBCOQz3hWk0Htik3J/[email protected]

#14 lore raw

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

Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:

Quoted 79 lines · Heiko Stübner
Quoted 70 lines

Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:

Quoted 69 lines

Hi Heiko,

Quoted 66 lines

Heiko Stuebner <[email protected]> hat am 12. September 2016 um 13:05 geschrieben:

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:

Quoted 49 lines
Quoted 49 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 19 lines

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.

---

[...]

Quoted 7 lines
(quoted diff) +0 -0
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
 *hsotg)
 
 			 __func__, hsotg->dr_mode);
 
 		break;
 

Review void dwc2_force_dr_mode(struct dwc2_hsotg

Quoted 10 lines
(quoted diff) +0 -6
 	}
 
-
-	/*
-	 * 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

Quoted 3 lines
Quoted 1 line
	[    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.

Quoted 7 lines
Quoted 2 lines

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.

Quoted 5 lines

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< -------------------

drivers/usb/dwc2/core.c +10 -1
index fa9b26b..4c0fa0b 100644
@@ -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

#15 lore raw

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

Quoted 121 lines · Heiko Stübner

Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:

Quoted 79 lines · Heiko Stübner
Quoted 70 lines

Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:

Quoted 69 lines

Hi Heiko,

Quoted 66 lines

Heiko Stuebner <[email protected]> hat am 12. September 2016 um 13:05 geschrieben:

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:

Quoted 49 lines
Quoted 49 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 19 lines

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.

---

[...]

Quoted 7 lines
(quoted diff) +0 -0
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
 *hsotg)
 
 			 __func__, hsotg->dr_mode);
 
 		break;
 

Review void dwc2_force_dr_mode(struct dwc2_hsotg

Quoted 10 lines
(quoted diff) +0 -6
 	}
 
-
-	/*
-	 * 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

Quoted 3 lines
Quoted 1 line
	[    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.

Quoted 7 lines
Quoted 2 lines

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.

Quoted 5 lines

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< -------------------

drivers/usb/dwc2/core.c +10 -2
index fa9b26b..4c0fa0b 100644
@@ -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< -------------------
Quoted 5 lines

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

A message that is not in the archive (sent off-list or not yet received): 6b0c9143-63b3-c5c3-7122-bbbb0c9f610b-HKixBCOQz3hWk0Htik3J/[email protected]

#17 lore raw

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

Hi John,

[add Felipe and Greg]

Am 14.09.2016 um 23:10 schrieb John Youn:

Quoted 114 lines · Heiko Stübner
Quoted 113 lines

Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:

Quoted 72 lines · Heiko Stübner
Quoted 64 lines

Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:

Quoted 63 lines

Hi Heiko,

Quoted 61 lines

Heiko Stuebner <[email protected]> hat am 12. September 2016 um 13:05 geschrieben:

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:

Quoted 41 lines
Quoted 41 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 19 lines

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.

---

[...]

Quoted 16 lines
(quoted diff) +0 -6
@@ -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);
Quoted 8 lines
Quoted 7 lines
Quoted 1 line
  }

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

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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.

Quoted 5 lines

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< -------------------

drivers/usb/dwc2/core.c +10 -2
index fa9b26b..4c0fa0b 100644
@@ -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< -------------------
Quoted 8 lines
Quoted 5 lines

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

Quoted 3 lines

Regards,
John

_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

A message that is not in the archive (sent off-list or not yet received): ad02126f-6c0e-9216-f7ab-ec528eeda2e2-eS4NqCHxEME@public.gmane.org

#19 lore raw

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

Quoted 126 lines · Stefan Wahren

Hi John,

[add Felipe and Greg]

Am 14.09.2016 um 23:10 schrieb John Youn:

Quoted 121 lines · Heiko Stübner
Quoted 113 lines

Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:

Quoted 72 lines · Heiko Stübner
Quoted 64 lines

Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:

Quoted 63 lines

Hi Heiko,

Quoted 61 lines

Heiko Stuebner <[email protected]> hat am 12. September 2016 um 13:05 geschrieben:

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:

Quoted 41 lines
Quoted 41 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 19 lines

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.

---

[...]

Quoted 16 lines
(quoted diff) +0 -6
@@ -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);
Quoted 8 lines
Quoted 7 lines
Quoted 1 line
  }

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

Quoted 7 lines
Quoted 3 lines
Quoted 1 line

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.

Quoted 5 lines

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< -------------------

drivers/usb/dwc2/core.c +10 -2
index fa9b26b..4c0fa0b 100644
@@ -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< -------------------
Quoted 5 lines

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.

Quoted 5 lines
Quoted 1 line

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
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

#20 lore raw

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

In reply to Heiko Stübner
Quoted 121 lines · Heiko Stübner

Am Dienstag, 13. September 2016, 12:04:12 schrieb John Youn:

Quoted 79 lines · Heiko Stübner
Quoted 70 lines

Am Dienstag, 13. September 2016, 20:07:25 schrieb Stefan Wahren:

Quoted 69 lines

Hi Heiko,

Quoted 66 lines

Heiko Stuebner <[email protected]> hat am 12. September 2016 um 13:05 geschrieben:

Hi Stefan,

Am Montag, 12. September 2016, 07:20:44 CEST schrieb Stefan Wahren:

Quoted 49 lines
Quoted 49 lines

Heiko Stuebner <[email protected]> hat am 11. September 2016 um 23:19 geschrieben:

Am Mittwoch, 7. September 2016, 19:39:43 CEST schrieb John Youn:

Quoted 19 lines

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.

---

[...]

Quoted 7 lines
(quoted diff) +0 -0
@@ -475,12 +478,6 @@ void dwc2_force_dr_mode(struct dwc2_hsotg
 *hsotg)
 
 			 __func__, hsotg->dr_mode);
 
 		break;
 

Review void dwc2_force_dr_mode(struct dwc2_hsotg

Quoted 10 lines
(quoted diff) +0 -6
 	}
 
-
-	/*
-	 * 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

Quoted 3 lines
Quoted 1 line
	[    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.

Quoted 7 lines
Quoted 2 lines

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.

Quoted 5 lines

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< -------------------

drivers/usb/dwc2/core.c +10 -2
index fa9b26b..4c0fa0b 100644
@@ -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< -------------------
Quoted 6 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html