Fixes and workarounds for dwc2 on HiKey board

Hope everyone had a happy new years!

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

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.

Any feedback would be greatly appreciated!

thanks
-john

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

New in v2:
* Reworked goto logic in patch #2, as suggested by Vardan

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   |  9 +++++-
 drivers/usb/dwc2/gadget.c |  2 +-
 drivers/usb/dwc2/hcd.c    | 79 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/params.c | 19 ++++++++++++
 5 files changed, 110 insertions(+), 5 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 11d8ae9..2d0dac2 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 9548d3e..eb2d628 100644
@@ -1101,7 +1101,7 @@ static inline bool dwc2_is_hs_iot(struct dwc2_hsotg *hsotg)
  * 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 c55db4a..cbba471 100644
@@ -3158,7 +3158,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!

---
v2: Rework goto logic suggested by Vardan, and add a comment

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

index 911c3b3..b60307a 100644
@@ -3241,6 +3241,14 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 				 dwc2_is_host_mode(hsotg) ? "Host" :
 				 "Peripheral");
 			usleep_range(20000, 40000);
+			/*
+			 * Sometimes the initial GOTGCTRL read is wrong, so
+			 * check it again and jump to host mode if that was
+			 * the case.
+			 */
+			gotgctl = dwc2_readl(hsotg->regs + GOTGCTL);
+			if (!(gotgctl & GOTGCTL_CONID_B))
+				goto host;
 			if (++count > 250)
 				break;
 		}

@@ -3255,6 +3263,7 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 		spin_unlock_irqrestore(&hsotg->lock, flags);
 		dwc2_hsotg_core_connect(hsotg);
 	} else {
+host:
 		/* A-Device connector (Host Mode) */
 		dev_dbg(hsotg->dev, "connId A\n");
 		while (!dwc2_is_host_mode(hsotg)) {

2.7.4

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 b60307a..a089946 100644
@@ -54,6 +54,8 @@
 #include "core.h"
 #include "hcd.h"
 
+static void dwc2_port_resume(struct dwc2_hsotg *hsotg);
+
 /*
  * =========================================================================
  *  Host Core Layer Functions

@@ -3235,6 +3237,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 a089946..6e4ec8a 100644
@@ -4383,6 +4383,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->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:
* Reworked to adapt around changes that landed in 4.10-rc

 drivers/usb/dwc2/core.h   |  7 ++++++
 drivers/usb/dwc2/hcd.c    | 60 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/params.c | 19 +++++++++++++++
 3 files changed, 86 insertions(+)

index eb2d628..4074752 100644
@@ -444,6 +444,11 @@ enum dwc2_ep0_state {
  *			in DWORDS with possible values from from
  *			16-32768 (default: 256, 256, 256, 256, 768,
  *			768, 768, 768, 0, 0, 0, 0, 0, 0, 0).
+ * @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

@@ -516,6 +521,8 @@ struct dwc2_core_params {
 	u16 g_rx_fifo_size;
 	u16 g_np_tx_fifo_size;
 	u32 g_tx_fifo_size[MAX_EPS_CHANNELS];
+
+	int change_speed_quirk;
 };
 
 /**

index a786256..71ca9c2 100644
@@ -67,6 +67,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 = {

@@ -227,6 +228,7 @@ static const struct dwc2_core_params params_default = {
 	.uframe_sched			= -1,
 	.external_id_pin_ctl		= -1,
 	.hibernation			= -1,
+	.change_speed_quirk		= -1,
 };
 
 const struct of_device_id dwc2_of_match_table[] = {

@@ -1110,6 +1112,22 @@ static void dwc2_set_gadget_dma(struct dwc2_hsotg *hsotg)
 			    !!hw->dma_desc_enable);
 }
 
+static void dwc2_set_change_speed_quirk(struct dwc2_hsotg *hsotg, int val)
+{
+	if (DWC2_OUT_OF_BOUNDS(val, 0, 1)) {
+		if (val >= 0) {
+			dev_err(hsotg->dev,
+				"'%d' invalid for parameter change_speed_quirk\n",
+				val);
+			dev_err(hsotg->dev, "change_speed_quirk must be 0 or 1\n");
+		}
+		val = 0;
+		dev_dbg(hsotg->dev, "Setting change_speed_quirk to %d\n", val);
+	}
+
+	hsotg->params.change_speed_quirk = val;
+}
+
 /**
  * dwc2_set_parameters() - Set all core parameters.
  *

@@ -1170,6 +1188,7 @@ static void dwc2_set_parameters(struct dwc2_hsotg *hsotg,
 	dwc2_set_param_uframe_sched(hsotg, params->uframe_sched);
 	dwc2_set_param_external_id_pin_ctl(hsotg, params->external_id_pin_ctl);
 	dwc2_set_param_hibernation(hsotg, params->hibernation);
+	dwc2_set_change_speed_quirk(hsotg, params->change_speed_quirk);
 
 	/*
 	 * Set devicetree-only parameters. These parameters do not

2.7.4

John/Vardan: Just as a heads up, along with these patches, I'm also still using the "fix flags for DMA descriptor allocation in dwc2_hsotg_ep_enable" from Marek Szyprowski, as its not landed upstream, and I'm also still seeing the matching warning on dwc2_hsotg_ep_disable() on the free side, as I've not seen a fix for that yet.

thanks
-john

This just hit Greg's tree and should be in -rc3.

Most of the team is out this week, so it might not be ready until next week. Unfortunately we had some infrastructure issues which prevented us from getting this out before the holidays.

Regards,
John

No worries. I don't mean to stress folks after vacations. Just wanted to make sure it wasn't forgotten.

thanks
-john

Hi John,

Do you need these in 4.10-rc or is 4.11 ok?

John

So I was submitting these for 4.11.

The only one which was sort of a regression fix (though introduced in 4.9 rather then 4.10) was the "Avoid suspending if we're in gadget mode" one, which you seemed to submit for 4.10-rc already.

Let me know if there is anything you need prior to queuing the rest for 4.11.

thanks
-john

Ok. The only one that will have a problem will be patch 5 due to my recent series [1].

Can you rebase against that?

Patch 1-4 of this series:

I'll also add these to our 4.11 branch locally as there are a lot of interdependent changes. I'll send everything at once to Felipe to make it easier for him.

Regards,
John

[1] https://www.spinics.net/lists/linux-usb/msg151693.html

Do you have a git branch for that series to rebase against?

thanks
-john

https://github.com/synopsys-usb/linux.git

branch 'next'

Regards,
John