Fixes and workarounds for dwc2 on HiKey board

I just wanted to send out my current queue of patches for dwc2 controller on the HiKey board.

This does exclude my patchset[1] to add extcon support to dwc2, which John Youn suspects a pending rework of the dwc2 fifo init logic might make unnecssary.

So in the meantime, I wanted to send out the other patches I have to use, so that they can get feedback and be consdiered for the next (4.11) merge window.

Feedback would be greatly appreciated!

thanks
-john

[1] https://lkml.org/lkml/2016/12/6/69

Chen Yu (2):
usb: dwc2: Force port resume on switching to device mode
usb: dwc2: Add a quirk to allow speed negotiation for Hisilicon Hi6220

John Stultz (3):
usb: dwc2: Avoid sleeping while holding hsotg->lock
usb: dwc2: Workaround case where GOTGCTL state is wrong
usb: dwc2: Avoid suspending if we're in gadget mode

 drivers/usb/dwc2/core.c     |  6 ++--
 drivers/usb/dwc2/core.h     |  8 ++++-
 drivers/usb/dwc2/gadget.c   |  2 +-
 drivers/usb/dwc2/hcd.c      | 75 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/usb/dwc2/platform.c |  1 +
 5 files changed, 86 insertions(+), 6 deletions(-)

2.7.4

Basically when plugging in various cables in different orders, I'm occasionally seeing the following BUG splat:

[   86.215403] BUG: scheduling while atomic: kworker/u16:2/53/0x00000002
[   86.219164] usb 1-1: USB disconnect, device number 9
[   86.226845] Preemption disabled at:[   86.230218]
[<ffffff8008673558>] dwc2_conn_id_status_change+0x120/0x250
[   86.236894] CPU: 0 PID: 53 Comm: kworker/u16:2 Tainted: G        W
     4.9.0-rc8-00051-gd5a7979-dirty #1702
[   86.246836] Hardware name: HiKey Development Board (DT)
[   86.252100] Workqueue: dwc2 dwc2_conn_id_status_change
[   86.257279] Call trace:
[   86.259771] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
[   86.265210] [<ffffff8008087ddc>] show_stack+0x14/0x20
[   86.270308] [<ffffff80084343f0>] dump_stack+0x90/0xb0
[   86.275401] [<ffffff80080d8d94>] __schedule_bug+0x6c/0xb8
[   86.280841] [<ffffff8008a07220>] __schedule+0x4f8/0x5b0
[   86.286099] [<ffffff8008a073e8>] schedule+0x38/0xa0
[   86.291017] [<ffffff8008a0a6cc>] schedule_hrtimeout_range_clock+0x8c/0xf0
[   86.297846] [<ffffff8008a0a740>] schedule_hrtimeout_range+0x10/0x18
[   86.304150] [<ffffff8008a0a4a0>] usleep_range+0x50/0x58
[   86.309418] [<ffffff800866d8dc>] dwc2_wait_for_mode.isra.4+0x54/0xd0
[   86.315815] [<ffffff800866f058>] dwc2_core_reset+0xe0/0x168
[   86.321431] [<ffffff800867e364>] dwc2_hsotg_core_init_disconnected+0x2c/0x310
[   86.328602] [<ffffff8008673568>] dwc2_conn_id_status_change+0x130/0x250
[   86.335254] [<ffffff80080ccd48>] process_one_work+0x118/0x370
[   86.341035] [<ffffff80080ccfe8>] worker_thread+0x48/0x498
[   86.346473] [<ffffff80080d2eb0>] kthread+0xd0/0xe8
[   86.351299] [<ffffff8008082e80>] ret_from_fork+0x10/0x50

This seems to be caused by the dwc2_wait_for_mode() calling usleep_range() while the hstog->lock spinlock is held, since we take that before calling dwc2_hsotg_core_init_disconnected().

This patch avoids the issue by adding an extra argument to dwc2_core_reset(), as suggested by John Youn, which allows us to skip the waiting, which should be unnecessary when calling from dwc2_hsotg_core_init_disconnected().

---
 drivers/usb/dwc2/core.c   | 6 +++---
 drivers/usb/dwc2/core.h   | 2 +-
 drivers/usb/dwc2/gadget.c | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

index 4c0fa0b..2e83545 100644
@@ -313,7 +313,7 @@ static bool dwc2_iddig_filter_enabled(struct dwc2_hsotg *hsotg)
  * Do core a soft reset of the core.  Be careful with this because it
  * resets all the internal state machines of the core.
  */
-int dwc2_core_reset(struct dwc2_hsotg *hsotg)
+int dwc2_core_reset(struct dwc2_hsotg *hsotg, bool skip_wait)
 {
 	u32 greset;
 	int count = 0;

@@ -369,7 +369,7 @@ int dwc2_core_reset(struct dwc2_hsotg *hsotg)
 		}
 	} while (!(greset & GRSTCTL_AHBIDLE));
 
-	if (wait_for_host_mode)
+	if (wait_for_host_mode && !skip_wait)
 		dwc2_wait_for_mode(hsotg, true);
 
 	return 0;

@@ -500,7 +500,7 @@ int dwc2_core_reset_and_force_dr_mode(struct dwc2_hsotg *hsotg)
 {
 	int retval;
 
-	retval = dwc2_core_reset(hsotg);
+	retval = dwc2_core_reset(hsotg, false);
 	if (retval)
 		return retval;
 

index 2a21a04..963ea1b 100644
@@ -1020,7 +1020,7 @@ enum dwc2_halt_status {
  * The following functions support initialization of the core driver component
  * and the DWC_otg controller
  */
-extern int dwc2_core_reset(struct dwc2_hsotg *hsotg);
+extern int dwc2_core_reset(struct dwc2_hsotg *hsotg, bool skip_wait);
 extern int dwc2_core_reset_and_force_dr_mode(struct dwc2_hsotg *hsotg);
 extern int dwc2_enter_hibernation(struct dwc2_hsotg *hsotg);
 extern int dwc2_exit_hibernation(struct dwc2_hsotg *hsotg, bool restore);

index 24fbebc..0bda799 100644
@@ -2521,7 +2521,7 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
 	kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
 
 	if (!is_usb_reset)
-		if (dwc2_core_reset(hsotg))
+		if (dwc2_core_reset(hsotg, true))
 			return;
 
 	/*

2.7.4

When removing a USB-A to USB-otg adapter cable, we get a change status irq, and then in dwc2_conn_id_status_change, we erroniously see the GOTGCTL_CONID_B flag set. This causes us to get stuck in the "while (!dwc2_is_device_mode(hsotg))" loop, spitting out "Waiting for Peripheral Mode, Mode=Host" warnings until it fails out many seconds later.

This patch works around the issue by re-reading the GOTGCTL state to check if the GOTGCTL_CONID_B is still set and if not restarting the change status logic.

I suspect this isn't the best solution, but it seems to work well for me.

Feedback would be greatly appreciated!

---
 drivers/usb/dwc2/hcd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index df5a065..a3f34dd 100644
@@ -3199,7 +3199,7 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 	dev_dbg(hsotg->dev, "gotgctl=%0x\n", gotgctl);
 	dev_dbg(hsotg->dev, "gotgctl.b.conidsts=%d\n",
 		!!(gotgctl & GOTGCTL_CONID_B));
-
+again:
 	/* B-Device connector (Device Mode) */
 	if (gotgctl & GOTGCTL_CONID_B) {
 		/* Wait for switch to device mode */

@@ -3210,6 +3210,9 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 				 dwc2_is_host_mode(hsotg) ? "Host" :
 				 "Peripheral");
 			usleep_range(20000, 40000);
+			gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+			if (!(gotgctl & GOTGCTL_CONID_B))
+				goto again;
 			if (++count > 250)
 				break;
 		}

2.7.4

Sounds good. I've made the change.

Thanks so much for your review!
-john

From: Chen Yu <[email protected]>

We've seen failures when switching between host and gadget mode, which was diagnosed as being caused due to the bus being auto-suspended when we switched.

So this patch forces a port resume when switching to device mode if the bus is suspended.

---
 drivers/usb/dwc2/hcd.c | 7 +++++++
 1 file changed, 7 insertions(+)

index a3f34dd..24db997 100644
@@ -54,6 +54,8 @@
 #include "core.h"
 #include "hcd.h"
 
+static void dwc2_port_resume(struct dwc2_hsotg *hsotg);
+
 /*
  * =========================================================================
  *  Host Core Layer Functions

@@ -3204,6 +3206,11 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 	if (gotgctl & GOTGCTL_CONID_B) {
 		/* Wait for switch to device mode */
 		dev_dbg(hsotg->dev, "connId B\n");
+		if (hsotg->bus_suspended) {
+			dev_info(hsotg->dev,
+				 "Do port resume before switching to device mode\n");
+			dwc2_port_resume(hsotg);
+		}
 		while (!dwc2_is_device_mode(hsotg)) {
 			dev_info(hsotg->dev,
 				 "Waiting for Peripheral Mode, Mode=%s\n",

2.7.4

I've found when booting HiKey with the usb gadget cable attached if I then try to connect via adb, I get an infinite spew of:

dwc2 f72c0000.usb: dwc2_hsotg_ep_sethalt(ep ffffffc0790ecb18 ep1out, 0) dwc2 f72c0000.usb: dwc2_hsotg_ep_sethalt(ep ffffffc0790eca18 ep1in, 0)

It seems that the usb autosuspend is suspending the bus shortly after bootup when the gadget cable is attached. So when adbd then tries to use the device, it doesn't work and it then tries to restart it over and over via the ep_sethalt calls (via FUNCTIONFS_CLEAR_HALT ioctl).

Chen Yu suggested this patch to avoid suspending if we're in device mode, and it avoids the problem.

This doesn't remove the need for the previous patch, to resume the port when we switch to gadget mode from host mode. But it does seem to resolve the issue.

---
 drivers/usb/dwc2/hcd.c | 3 +++
 1 file changed, 3 insertions(+)

index 24db997..008f5bc 100644
@@ -4375,6 +4375,9 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 	if (!HCD_HW_ACCESSIBLE(hcd))
 		goto unlock;
 
+	if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
+		goto unlock;
+
 	if (!hsotg->core_params->hibernation)
 		goto skip_power_saving;
 

2.7.4

From: Chen Yu <[email protected]>

The Hi6220's usb controller is limited in that it does not support "Split Transactions", so it does not support communicating with low-speed and full-speed devices behind a high-speed hub.

Thus it requires a quirk so that we can manually drop the usb speed when low/full-speed are attached, and bump back to high speed when they are removed.

[jstultz: Reworked to simplify the patch, and made
commit log to be more specific about the issue]

---
v2:
* Fix build issue reported by kbuildbot
* Rework to avoid using new dts entry suggested by RobH
* Further tweaks from Chen Yu to try to address comments from John Youn
* Further simplified logic

---
 drivers/usb/dwc2/core.h     |  6 +++++
 drivers/usb/dwc2/hcd.c      | 60 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/platform.c |  1 +
 3 files changed, 67 insertions(+)

index 963ea1b..de1fa0c 100644
@@ -417,6 +417,11 @@ enum dwc2_ep0_state {
  *			needed.
  *			0 - No (default)
  *			1 - Yes
+ * @change_speed_quirk: Change speed configuration to DWC2_SPEED_PARAM_FULL
+ *                      while full&low speed device connect. And change speed
+ *                      back to DWC2_SPEED_PARAM_HIGH while device is gone.
+ *			0 - No (default)
+ *			1 - Yes
  *
  * The following parameters may be specified when starting the module. These
  * parameters define how the DWC_otg controller should be configured. A

@@ -457,6 +462,7 @@ struct dwc2_core_params {
 	int uframe_sched;
 	int external_id_pin_ctl;
 	int hibernation;
+	int change_speed_quirk;
 };
 
 /**

index 8e1728b..17eb5fd 100644
@@ -85,6 +85,7 @@ static const struct dwc2_core_params params_hi6220 = {
 	.uframe_sched			= 0,
 	.external_id_pin_ctl		= -1,
 	.hibernation			= -1,
+	.change_speed_quirk		= 1,
 };
 
 static const struct dwc2_core_params params_bcm2835 = {

2.7.4

Just out of curiosity (I know nothing about this hardware), what happens if there is a high-speed hub plugged into the host controller and both a high-speed and a full-speed device plugged into the hub?

Do you end up forcing the high-speed device to run at full speed?

Alan Stern

If your interested in details, page 12 of the pdf here has some details:
https://github.com/96boards/documentation/blob/master/ConsumerEdition/HiKey/AdditionalDocs/HiKey_Hardware_User_Manual_Rev0.2.pdf

There's also schematics for the board available (if you are interested in that sort of stuff). You can find the USB bits on Page 4 here:
https://github.com/96boards/documentation/blob/master/ConsumerEdition/HiKey/HardwareDocs/HiKey_schematics_LeMaker_version_Rev_A1.pdf

Yes. It drops back to full-speed.

thanks
-john