diff mbox

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

Message ID 1479246456-21652-4-git-send-email-john.stultz@linaro.org
State Superseded
Headers show

Commit Message

John Stultz Nov. 15, 2016, 9:47 p.m. UTC 
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(+)

-- 
2.7.4

Comments

John Youn Nov. 22, 2016, 3:23 a.m. UTC | #1 
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);

>
John Stultz Nov. 22, 2016, 6:48 a.m. UTC | #2 
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
diff mbox

Patch

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