[RFC][PATCH 0/3] DWC2 fixes and hacks to make things work on HiKey

[RFC][PATCH 0/3] DWC2 fixes and hacks to make things work on HiKey

[John Stultz]

Hey folks,
  I wanted to send out these patches again for comment.
These three patches are basically required to get otg/gadget
mode working reliably on HiKey.

I admit they do feel a bit hackish, but without them we run
into a number of problems related to autosuspend and incorrect
connection state handling.

So I wanted to resend these for feedback and guidance.

I suspect that some of these issues may be due to the phy driver
on hikey being somewhat minimal:
https://git.linaro.org/people/john.stultz/android-dev.git/plain/drivers/phy/phy-hi6220-usb.c?h=dev/hikey-mainline-WIP

So I've tried to hacked together extconn support into the phy
driver, and I'm getting phy notifications, but I've not yet
sorted out how to get the phy->gadget signaling to work
properly (or if my appraoch is even correct).

See: 
https://git.linaro.org/people/john.stultz/android-dev.git/commit/?h=dev/hikey-mainline-WIP&id=c608f263240f7c54dbf5439f2210cba1ab8e0136

I've had a difficult time trying to understand how the
usb contoller driver, phy driver and extcon shoudl properly
fit together, and I'm not sure what would be a good reference
driver to follow.

So suggestions or pointers here would be greatly appreciated.

Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Cc: Amit Pundir <amit.pundir@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: John Youn <johnyoun@synopsys.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Chen Yu <chenyu56@huawei.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org

Chen Yu (1):
  usb: dwc2: Force port resume on switching to device mode

John Stultz (2):
  usb: dwc2: Avoid suspending if we're in gadget mode
  usb: dwc2: Make sure we disconnect the gadget state

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

-- 
2.7.4

[RFC][PATCH 1/3] usb: dwc2: Force port resume on switching to device mode

[John Stultz]

From: Chen Yu <chenyu56@huawei.com>

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.

Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Cc: Amit Pundir <amit.pundir@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: John Youn <johnyoun@synopsys.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Chen Yu <chenyu56@huawei.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: Chen Yu <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/usb/dwc2/hcd.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index df5a065..b374e60 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -54,6 +54,9 @@
 #include "core.h"
 #include "hcd.h"
 
+
+static void dwc2_port_resume(struct dwc2_hsotg *hsotg);
+
 /*
  * =========================================================================
  *  Host Core Layer Functions
@@ -3204,6 +3207,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

[RFC][PATCH 2/3] usb: dwc2: Avoid suspending if we're in gadget mode

[John Stultz]

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.

Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Cc: Amit Pundir <amit.pundir@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: John Youn <johnyoun@synopsys.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Chen Yu <chenyu56@huawei.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Suggested-by: Chen Yu <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/usb/dwc2/hcd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index b374e60..8c980fd 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4373,6 +4373,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

[RFC][PATCH 3/3] usb: dwc2: Make sure we disconnect the gadget state

[John Stultz]

I had seen some odd behavior with HiKey's usb-gadget interface
that I finally seemed to have chased down. Basically every other
time I pluged in the OTG port, the gadget interface would
properly initialize. The other times, I'd get a big WARN_ON
in dwc2_hsotg_init_fifo() about the fifo_map not being clear.

Ends up if we don't disconnect the gadget state, the fifo-map
doesn't get cleared properly, which causes WARN_ON messages and
also results in the device not properly being setup as a gadget
every other time the OTG port is connected.

So this patch adds a call to dwc2_hsotg_disconnect() in the
reset path so the state is properly cleared.

With it, the gadget interface initializes properly on every
plug in.

Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Cc: Amit Pundir <amit.pundir@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: John Youn <johnyoun@synopsys.com>
Cc: Douglas Anderson <dianders@chromium.org>
Cc: Chen Yu <chenyu56@huawei.com>
Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/usb/dwc2/hcd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 8c980fd..d2557b7 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3228,6 +3228,7 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 		dwc2_core_init(hsotg, false);
 		dwc2_enable_global_interrupts(hsotg);
 		spin_lock_irqsave(&hsotg->lock, flags);
+		dwc2_hsotg_disconnect(hsotg);
 		dwc2_hsotg_core_init_disconnected(hsotg, false);
 		spin_unlock_irqrestore(&hsotg->lock, flags);
 		dwc2_hsotg_core_connect(hsotg);
-- 
2.7.4

Re: [RFC][PATCH 3/3] usb: dwc2: Make sure we disconnect the gadget state

[John Youn]

On 11/15/2016 1:47 PM, John Stultz wrote:
> I had seen some odd behavior with HiKey's usb-gadget interface
> that I finally seemed to have chased down. Basically every other
> time I pluged in the OTG port, the gadget interface would
> properly initialize. The other times, I'd get a big WARN_ON
> in dwc2_hsotg_init_fifo() about the fifo_map not being clear.

Hi,

The fifo_map could end up not being clear when disconnect is never
sent to the UDC framework. That unsets the configuration and the
endpoints get disabled, which clears the FIFO map.

Looks like the problem happens when going from A-device to B-device.

If you come up as an A-Device, the gadget wouldn't have been
configured so it shouldn't warn going A->B.

If you go B->A, you will get a session end detected, which triggers
the udc disconnect. Then A->B should not warn here either.

Can you determine why this doesn't happen on your system? It sounds
like there might be some race condition that we need to identify. If
you can provide logs with DEBUG enabled that would be helpful too.

Regards,
John


> 
> Ends up if we don't disconnect the gadget state, the fifo-map
> doesn't get cleared properly, which causes WARN_ON messages and
> also results in the device not properly being setup as a gadget
> every other time the OTG port is connected.
> 
> So this patch adds a call to dwc2_hsotg_disconnect() in the
> reset path so the state is properly cleared.
> 
> With it, the gadget interface initializes properly on every
> plug in.
> 
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Guodong Xu <guodong.xu@linaro.org>
> Cc: Amit Pundir <amit.pundir@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: John Youn <johnyoun@synopsys.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> Cc: Chen Yu <chenyu56@huawei.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-usb@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/usb/dwc2/hcd.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 8c980fd..d2557b7 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -3228,6 +3228,7 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
>  		dwc2_core_init(hsotg, false);
>  		dwc2_enable_global_interrupts(hsotg);
>  		spin_lock_irqsave(&hsotg->lock, flags);
> +		dwc2_hsotg_disconnect(hsotg);
>  		dwc2_hsotg_core_init_disconnected(hsotg, false);
>  		spin_unlock_irqrestore(&hsotg->lock, flags);
>  		dwc2_hsotg_core_connect(hsotg);
> 

Re: [RFC][PATCH 3/3] usb: dwc2: Make sure we disconnect the gadget state

[John Stultz]

On Mon, Nov 21, 2016 at 7:23 PM, John Youn <John.Youn@synopsys.com> wrote:
> On 11/15/2016 1:47 PM, John Stultz wrote:
>> I had seen some odd behavior with HiKey's usb-gadget interface
>> that I finally seemed to have chased down. Basically every other
>> time I pluged in the OTG port, the gadget interface would
>> properly initialize. The other times, I'd get a big WARN_ON
>> in dwc2_hsotg_init_fifo() about the fifo_map not being clear.
>
> The fifo_map could end up not being clear when disconnect is never
> sent to the UDC framework. That unsets the configuration and the
> endpoints get disabled, which clears the FIFO map.
>
> Looks like the problem happens when going from A-device to B-device.
>
> If you come up as an A-Device, the gadget wouldn't have been
> configured so it shouldn't warn going A->B.
>
> If you go B->A, you will get a session end detected, which triggers
> the udc disconnect. Then A->B should not warn here either.

Yea. From the udc perspective, it seems like we see:

usb_ep_enable()
gadget works fine...

unplug gadget cable, it switches into host mode.

re-plug gadget cable, and as it switches into gadget mode:

[   74.241737] JDB: udc-core usb_ep_disable!
[   74.245812] JDB: udc-core usb_ep_disable!
[   74.302390] dwc2 f72c0000.usb: new device is high-speed
[   74.474131] dwc2 f72c0000.usb: new device is high-speed
[   74.550185] dwc2 f72c0000.usb: new device is high-speed
[   74.621953] dwc2 f72c0000.usb: new address 74
[   74.651824] configfs-gadget gadget: high-speed config #1: b
[   74.657467] JDB: udc-core usb_ep_enable!
[   74.661422] JDB: udc-core usb_ep_enable!

This is why I suspect that the overly simplistic phy driver is part of
the problem here.  However, in trying to extend it to be more
functional (I've got extcon wired up and see events when I plug in and
out), I'm still having trouble, as its not clear how the generic phy
(not usb phy) driver is supposed to interact with the UDC/dwc2 driver.

Part of the issue I've seen is that while the hikey generic phy driver
tries to mimic some of the usb-phy drivers, creating an otg device and
registering it:

        priv->dev = &pdev->dev;
        priv->phy.dev = priv->dev;
        priv->phy.label = "hi6220_usb_phy";
        priv->phy.otg = otg;
        priv->phy.type = USB_PHY_TYPE_USB2;
        otg->set_host = hi6220_otg_set_host;
        otg->set_peripheral = hi6220_otg_set_peripheral;
        otg->usb_phy = &priv->phy;

        platform_set_drvdata(pdev, priv);

        phy_set_drvdata(phy, priv);

        usb_add_phy_dev(&priv->phy);

The trouble is the dwc2 driver doesn't seem to call
set_host/peripheral() because it never sets the uphy pointer in
dwc2_lowlevel_hw_init(), as the hikey generic phy driver is found
before that point.

So I'm not really sure how to get the generic phy -> usb_gadget
connection established so I can call
usb_gadget_vbus_connect/disconnect at the right time..

Suggestions?

> Can you determine why this doesn't happen on your system? It sounds
> like there might be some race condition that we need to identify. If
> you can provide logs with DEBUG enabled that would be helpful too.

I'll try to capture some more data. I've got my own debug printouts
littering the logs, so I'll try to pull those out of the way.

thanks
-john

Re: [RFC][PATCH 0/3] DWC2 fixes and hacks to make things work on HiKey

[John Stultz]

On Tue, Nov 15, 2016 at 1:47 PM, John Stultz <john.stultz@linaro.org> wrote:
> I suspect that some of these issues may be due to the phy driver
> on hikey being somewhat minimal:
> https://git.linaro.org/people/john.stultz/android-dev.git/plain/drivers/phy/phy-hi6220-usb.c?h=dev/hikey-mainline-WIP

Whoops. Sorry, that link should have been the upstream version, not
the modified one:
https://git.linaro.org/people/john.stultz/android-dev.git/plain/drivers/phy/phy-hi6220-usb.c?id=a25f0944ba9b1d8a6813fd6f1a86f1bd59ac25a6


thanks
-john