[RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset

[RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset

[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 on reset, 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.

I don't know if this is actually the right fix, but it seems
to work well. Feedback would be greatly appreciated!

Cc: Wei Xu <xuwei5@hisilicon.com>
Cc: Guodong Xu <guodong.xu@linaro.org>
Cc: Chen Yu <chenyu56@huawei.com>
Cc: Amit Pundir <amit.pundir@linaro.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: John Youn <johnyoun@synopsys.com>
Cc: Douglas Anderson <dianders@chromium.org>
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/gadget.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 24fbebc..5505001 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
 
 	/* Kill any ep0 requests as controller will be reinitialized */
 	kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
+	/* Make sure everything is disconnected */
+	dwc2_hsotg_disconnect(hsotg);
 
 	if (!is_usb_reset)
 		if (dwc2_core_reset(hsotg))
-- 
2.7.4

Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset

[John Youn]

On 10/19/2016 11:00 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.
> 
> Ends up If we don't disconnect the gadget state on reset, 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.
> 
> I don't know if this is actually the right fix, but it seems
> to work well. Feedback would be greatly appreciated!
> 
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Guodong Xu <guodong.xu@linaro.org>
> Cc: Chen Yu <chenyu56@huawei.com>
> Cc: Amit Pundir <amit.pundir@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: John Youn <johnyoun@synopsys.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> 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/gadget.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 24fbebc..5505001 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>  
>  	/* Kill any ep0 requests as controller will be reinitialized */
>  	kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
> +	/* Make sure everything is disconnected */
> +	dwc2_hsotg_disconnect(hsotg);
>  
>  	if (!is_usb_reset)
>  		if (dwc2_core_reset(hsotg))
> 

Seems fine with our testing.

Acked-by: John Youn <johnyoun@synopsys.com>

Regards,
John

Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset

[John Stultz]

On Tue, Oct 25, 2016 at 2:29 PM, John Youn <John.Youn@synopsys.com> wrote:
> On 10/19/2016 11:00 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.
>>
>> Ends up If we don't disconnect the gadget state on reset, 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.
>>
>> I don't know if this is actually the right fix, but it seems
>> to work well. Feedback would be greatly appreciated!
>>
>> Cc: Wei Xu <xuwei5@hisilicon.com>
>> Cc: Guodong Xu <guodong.xu@linaro.org>
>> Cc: Chen Yu <chenyu56@huawei.com>
>> Cc: Amit Pundir <amit.pundir@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: John Youn <johnyoun@synopsys.com>
>> Cc: Douglas Anderson <dianders@chromium.org>
>> 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/gadget.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 24fbebc..5505001 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>>
>>       /* Kill any ep0 requests as controller will be reinitialized */
>>       kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
>> +     /* Make sure everything is disconnected */
>> +     dwc2_hsotg_disconnect(hsotg);
>>
>>       if (!is_usb_reset)
>>               if (dwc2_core_reset(hsotg))
>>
>
> Seems fine with our testing.
>
> Acked-by: John Youn <johnyoun@synopsys.com>

Awesome! Thanks so much for the review and testing!

I'm curious, did you happen to have any thoughts or objections on the
"dwc2: Force port resume on switching to device mode" patch
(https://patchwork.kernel.org/patch/9375965/ ) as well?

thanks
-john

Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset

[John Youn]

On 10/25/2016 2:56 PM, John Stultz wrote:
> On Tue, Oct 25, 2016 at 2:29 PM, John Youn <John.Youn@synopsys.com> wrote:
>> On 10/19/2016 11:00 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.
>>>
>>> Ends up If we don't disconnect the gadget state on reset, 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.
>>>
>>> I don't know if this is actually the right fix, but it seems
>>> to work well. Feedback would be greatly appreciated!
>>>
>>> Cc: Wei Xu <xuwei5@hisilicon.com>
>>> Cc: Guodong Xu <guodong.xu@linaro.org>
>>> Cc: Chen Yu <chenyu56@huawei.com>
>>> Cc: Amit Pundir <amit.pundir@linaro.org>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: John Youn <johnyoun@synopsys.com>
>>> Cc: Douglas Anderson <dianders@chromium.org>
>>> 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/gadget.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>> index 24fbebc..5505001 100644
>>> --- a/drivers/usb/dwc2/gadget.c
>>> +++ b/drivers/usb/dwc2/gadget.c
>>> @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>>>
>>>       /* Kill any ep0 requests as controller will be reinitialized */
>>>       kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
>>> +     /* Make sure everything is disconnected */
>>> +     dwc2_hsotg_disconnect(hsotg);
>>>
>>>       if (!is_usb_reset)
>>>               if (dwc2_core_reset(hsotg))
>>>
>>
>> Seems fine with our testing.
>>
>> Acked-by: John Youn <johnyoun@synopsys.com>
> 
> Awesome! Thanks so much for the review and testing!
> 
> I'm curious, did you happen to have any thoughts or objections on the
> "dwc2: Force port resume on switching to device mode" patch
> (https://patchwork.kernel.org/patch/9375965/ ) as well?

Sorry, must have missed that one. We'll take a look.

Regards,
John

Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 6836 bytes --]


Hi,

John Stultz <john.stultz@linaro.org> writes:
> 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 on reset, 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.
>
> I don't know if this is actually the right fix, but it seems
> to work well. Feedback would be greatly appreciated!
>
> Cc: Wei Xu <xuwei5@hisilicon.com>
> Cc: Guodong Xu <guodong.xu@linaro.org>
> Cc: Chen Yu <chenyu56@huawei.com>
> Cc: Amit Pundir <amit.pundir@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: John Youn <johnyoun@synopsys.com>
> Cc: Douglas Anderson <dianders@chromium.org>
> 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/gadget.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 24fbebc..5505001 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>  
>  	/* Kill any ep0 requests as controller will be reinitialized */
>  	kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
> +	/* Make sure everything is disconnected */
> +	dwc2_hsotg_disconnect(hsotg);

Dunno, seems like you're actually working around a different
bug. Looking at USB Reset handler we have:

	if (gintsts & (GINTSTS_USBRST | GINTSTS_RESETDET)) {

		u32 usb_status = dwc2_readl(hsotg->regs + GOTGCTL);
		u32 connected = hsotg->connected;

		dev_dbg(hsotg->dev, "%s: USBRst\n", __func__);
		dev_dbg(hsotg->dev, "GNPTXSTS=%08x\n",
			dwc2_readl(hsotg->regs + GNPTXSTS));

		dwc2_writel(GINTSTS_USBRST, hsotg->regs + GINTSTS);

		/* Report disconnection if it is not already done. */
		dwc2_hsotg_disconnect(hsotg);

		if (usb_status & GOTGCTL_BSESVLD && connected)
			dwc2_hsotg_core_init_disconnected(hsotg, true);
	}

so, dwc2_hsotg_disconnect() is already called from Reset path. What
you're doing here is that you could, potentially, call
driver->disconnect() twice.

The real problem could be your implementation for ->pullup() which
duplicates part of what ->udc_start() does:

static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
{
	struct dwc2_hsotg *hsotg = to_hsotg(gadget);
	unsigned long flags = 0;

	dev_dbg(hsotg->dev, "%s: is_on: %d op_state: %d\n", __func__, is_on,
			hsotg->op_state);

	/* Don't modify pullup state while in host mode */
	if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
		hsotg->enabled = is_on;
		return 0;
	}

	spin_lock_irqsave(&hsotg->lock, flags);
	if (is_on) {
		hsotg->enabled = 1;
		dwc2_hsotg_core_init_disconnected(hsotg, false);
		dwc2_hsotg_core_connect(hsotg);
	} else {
		dwc2_hsotg_core_disconnect(hsotg);
		dwc2_hsotg_disconnect(hsotg);
		hsotg->enabled = 0;
	}

	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
	spin_unlock_irqrestore(&hsotg->lock, flags);

	return 0;
}

Here's what I think dwc2_hsotg_pullup() and dwc2_hsotg_udc_start()
should contain:


diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 9dc6c482b89e..dbe28947d3a9 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3388,6 +3388,7 @@ static void dwc2_hsotg_init(struct dwc2_hsotg *hsotg)
 	dwc2_writel(0, hsotg->regs + DAINTMSK);
 
 	/* Be in disconnected state until gadget is registered */
+	/* REVISIT!!!! This should be done in probe()!!!! */
 	__orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);
 
 	/* setup fifos */
@@ -3428,26 +3429,6 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget,
 	unsigned long flags;
 	int ret;
 
-	if (!hsotg) {
-		pr_err("%s: called with no device\n", __func__);
-		return -ENODEV;
-	}
-
-	if (!driver) {
-		dev_err(hsotg->dev, "%s: no driver\n", __func__);
-		return -EINVAL;
-	}
-
-	if (driver->max_speed < USB_SPEED_FULL)
-		dev_err(hsotg->dev, "%s: bad speed\n", __func__);
-
-	if (!driver->setup) {
-		dev_err(hsotg->dev, "%s: missing entry points\n", __func__);
-		return -EINVAL;
-	}
-
-	WARN_ON(hsotg->driver);
-
 	driver->driver.bus = NULL;
 	hsotg->driver = driver;
 	hsotg->gadget.dev.of_node = hsotg->dev->of_node;
@@ -3550,17 +3531,15 @@ static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
 	/* Don't modify pullup state while in host mode */
 	if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
 		hsotg->enabled = is_on;
-		return 0;
+		return -EINVAL;
 	}
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 	if (is_on) {
 		hsotg->enabled = 1;
-		dwc2_hsotg_core_init_disconnected(hsotg, false);
 		dwc2_hsotg_core_connect(hsotg);
 	} else {
 		dwc2_hsotg_core_disconnect(hsotg);
-		dwc2_hsotg_disconnect(hsotg);
 		hsotg->enabled = 0;
 	}
 

And BTW, your usb_gadget_ops had indentation problems, here's a fix for
that:

@@ -3617,10 +3596,10 @@ static int dwc2_hsotg_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 }
 
 static const struct usb_gadget_ops dwc2_hsotg_gadget_ops = {
-       .get_frame      = dwc2_hsotg_gadget_getframe,
+       .get_frame              = dwc2_hsotg_gadget_getframe,
        .udc_start              = dwc2_hsotg_udc_start,
        .udc_stop               = dwc2_hsotg_udc_stop,
-       .pullup                 = dwc2_hsotg_pullup,
+       .pullup                 = dwc2_hsotg_pullup,
        .vbus_session           = dwc2_hsotg_vbus_session,
        .vbus_draw              = dwc2_hsotg_vbus_draw,
 };

John Y, it sees like dwc2 needs quite a bit of work when it comes to its
linkage to the Gadget API, please try to schedule some time to revisit
all of that. Also, the way debugging is done with dwc2 is rather bad,
with a few #ifdef DEBUG in the driver. Some of that information could be
exposed via tracepoints and some would make more sense with debugfs.

Anyway, I don't think $subject is the right fix for the problem
described so I'm not taking it, at least not at this moment or without a
better explanation of how we got to the conclusion that $subject is the
right fix ;-)

cheers

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 800 bytes --]

Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset

[John Youn]

On 10/31/2016 4:19 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> John Stultz <john.stultz@linaro.org> writes:
>> 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 on reset, 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.
>>
>> I don't know if this is actually the right fix, but it seems
>> to work well. Feedback would be greatly appreciated!
>>
>> Cc: Wei Xu <xuwei5@hisilicon.com>
>> Cc: Guodong Xu <guodong.xu@linaro.org>
>> Cc: Chen Yu <chenyu56@huawei.com>
>> Cc: Amit Pundir <amit.pundir@linaro.org>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: John Youn <johnyoun@synopsys.com>
>> Cc: Douglas Anderson <dianders@chromium.org>
>> 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/gadget.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 24fbebc..5505001 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>>  
>>  	/* Kill any ep0 requests as controller will be reinitialized */
>>  	kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
>> +	/* Make sure everything is disconnected */
>> +	dwc2_hsotg_disconnect(hsotg);
> 
> Dunno, seems like you're actually working around a different
> bug. Looking at USB Reset handler we have:
> 
> 	if (gintsts & (GINTSTS_USBRST | GINTSTS_RESETDET)) {
> 
> 		u32 usb_status = dwc2_readl(hsotg->regs + GOTGCTL);
> 		u32 connected = hsotg->connected;
> 
> 		dev_dbg(hsotg->dev, "%s: USBRst\n", __func__);
> 		dev_dbg(hsotg->dev, "GNPTXSTS=%08x\n",
> 			dwc2_readl(hsotg->regs + GNPTXSTS));
> 
> 		dwc2_writel(GINTSTS_USBRST, hsotg->regs + GINTSTS);
> 
> 		/* Report disconnection if it is not already done. */
> 		dwc2_hsotg_disconnect(hsotg);
> 
> 		if (usb_status & GOTGCTL_BSESVLD && connected)
> 			dwc2_hsotg_core_init_disconnected(hsotg, true);
> 	}
> 
> so, dwc2_hsotg_disconnect() is already called from Reset path. What
> you're doing here is that you could, potentially, call
> driver->disconnect() twice.
> 
> The real problem could be your implementation for ->pullup() which
> duplicates part of what ->udc_start() does:
> 
> static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> {
> 	struct dwc2_hsotg *hsotg = to_hsotg(gadget);
> 	unsigned long flags = 0;
> 
> 	dev_dbg(hsotg->dev, "%s: is_on: %d op_state: %d\n", __func__, is_on,
> 			hsotg->op_state);
> 
> 	/* Don't modify pullup state while in host mode */
> 	if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
> 		hsotg->enabled = is_on;
> 		return 0;
> 	}
> 
> 	spin_lock_irqsave(&hsotg->lock, flags);
> 	if (is_on) {
> 		hsotg->enabled = 1;
> 		dwc2_hsotg_core_init_disconnected(hsotg, false);
> 		dwc2_hsotg_core_connect(hsotg);
> 	} else {
> 		dwc2_hsotg_core_disconnect(hsotg);
> 		dwc2_hsotg_disconnect(hsotg);
> 		hsotg->enabled = 0;
> 	}
> 
> 	hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> 	spin_unlock_irqrestore(&hsotg->lock, flags);
> 
> 	return 0;
> }
> 
> Here's what I think dwc2_hsotg_pullup() and dwc2_hsotg_udc_start()
> should contain:
> 
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 9dc6c482b89e..dbe28947d3a9 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3388,6 +3388,7 @@ static void dwc2_hsotg_init(struct dwc2_hsotg *hsotg)
>  	dwc2_writel(0, hsotg->regs + DAINTMSK);
>  
>  	/* Be in disconnected state until gadget is registered */
> +	/* REVISIT!!!! This should be done in probe()!!!! */
>  	__orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);
>  
>  	/* setup fifos */
> @@ -3428,26 +3429,6 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget,
>  	unsigned long flags;
>  	int ret;
>  
> -	if (!hsotg) {
> -		pr_err("%s: called with no device\n", __func__);
> -		return -ENODEV;
> -	}
> -
> -	if (!driver) {
> -		dev_err(hsotg->dev, "%s: no driver\n", __func__);
> -		return -EINVAL;
> -	}
> -
> -	if (driver->max_speed < USB_SPEED_FULL)
> -		dev_err(hsotg->dev, "%s: bad speed\n", __func__);
> -
> -	if (!driver->setup) {
> -		dev_err(hsotg->dev, "%s: missing entry points\n", __func__);
> -		return -EINVAL;
> -	}
> -
> -	WARN_ON(hsotg->driver);
> -
>  	driver->driver.bus = NULL;
>  	hsotg->driver = driver;
>  	hsotg->gadget.dev.of_node = hsotg->dev->of_node;
> @@ -3550,17 +3531,15 @@ static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>  	/* Don't modify pullup state while in host mode */
>  	if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
>  		hsotg->enabled = is_on;
> -		return 0;
> +		return -EINVAL;
>  	}
>  
>  	spin_lock_irqsave(&hsotg->lock, flags);
>  	if (is_on) {
>  		hsotg->enabled = 1;
> -		dwc2_hsotg_core_init_disconnected(hsotg, false);
>  		dwc2_hsotg_core_connect(hsotg);
>  	} else {
>  		dwc2_hsotg_core_disconnect(hsotg);
> -		dwc2_hsotg_disconnect(hsotg);
>  		hsotg->enabled = 0;
>  	}
>  
> 
> And BTW, your usb_gadget_ops had indentation problems, here's a fix for
> that:
> 
> @@ -3617,10 +3596,10 @@ static int dwc2_hsotg_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>  }
>  
>  static const struct usb_gadget_ops dwc2_hsotg_gadget_ops = {
> -       .get_frame      = dwc2_hsotg_gadget_getframe,
> +       .get_frame              = dwc2_hsotg_gadget_getframe,
>         .udc_start              = dwc2_hsotg_udc_start,
>         .udc_stop               = dwc2_hsotg_udc_stop,
> -       .pullup                 = dwc2_hsotg_pullup,
> +       .pullup                 = dwc2_hsotg_pullup,
>         .vbus_session           = dwc2_hsotg_vbus_session,
>         .vbus_draw              = dwc2_hsotg_vbus_draw,
>  };
> 
> John Y, it sees like dwc2 needs quite a bit of work when it comes to its
> linkage to the Gadget API, please try to schedule some time to revisit
> all of that.

Ok we'll take a closer look at this reset/disconnect issue.

And the usage of the gadget API is already on our list to
address. There's other issues with regard to that as well. We're
focusing on addressing these outstanding issues now, but we need to
first get a bunch stuff upstreamed that have been queued internally
for a while.

> Also, the way debugging is done with dwc2 is rather bad,
> with a few #ifdef DEBUG in the driver. Some of that information could be
> exposed via tracepoints and some would make more sense with debugfs.

Agree, also among the many things on our list :)

> 
> Anyway, I don't think $subject is the right fix for the problem
> described so I'm not taking it, at least not at this moment or without a
> better explanation of how we got to the conclusion that $subject is the
> right fix ;-)

Ok thanks.

Regards,
John

Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset

[John Stultz]

On Mon, Oct 31, 2016 at 4:18 AM, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
> John Stultz <john.stultz@linaro.org> writes:
>> 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 on reset, 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.
>>
>> I don't know if this is actually the right fix, but it seems
>> to work well. Feedback would be greatly appreciated!
>>
[snip]
>> ---
>>  drivers/usb/dwc2/gadget.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 24fbebc..5505001 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> @@ -2519,6 +2519,8 @@ void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *hsotg,
>>
>>       /* Kill any ep0 requests as controller will be reinitialized */
>>       kill_all_requests(hsotg, hsotg->eps_out[0], -ECONNRESET);
>> +     /* Make sure everything is disconnected */
>> +     dwc2_hsotg_disconnect(hsotg);
>
> Dunno, seems like you're actually working around a different
> bug. Looking at USB Reset handler we have:
>
>         if (gintsts & (GINTSTS_USBRST | GINTSTS_RESETDET)) {
>
>                 u32 usb_status = dwc2_readl(hsotg->regs + GOTGCTL);
>                 u32 connected = hsotg->connected;
>
>                 dev_dbg(hsotg->dev, "%s: USBRst\n", __func__);
>                 dev_dbg(hsotg->dev, "GNPTXSTS=%08x\n",
>                         dwc2_readl(hsotg->regs + GNPTXSTS));
>
>                 dwc2_writel(GINTSTS_USBRST, hsotg->regs + GINTSTS);
>
>                 /* Report disconnection if it is not already done. */
>                 dwc2_hsotg_disconnect(hsotg);
>
>                 if (usb_status & GOTGCTL_BSESVLD && connected)
>                         dwc2_hsotg_core_init_disconnected(hsotg, true);
>         }
>
> so, dwc2_hsotg_disconnect() is already called from Reset path. What
> you're doing here is that you could, potentially, call
> driver->disconnect() twice.
>
> The real problem could be your implementation for ->pullup() which
> duplicates part of what ->udc_start() does:
>
> static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
> {
>         struct dwc2_hsotg *hsotg = to_hsotg(gadget);
>         unsigned long flags = 0;
>
>         dev_dbg(hsotg->dev, "%s: is_on: %d op_state: %d\n", __func__, is_on,
>                         hsotg->op_state);
>
>         /* Don't modify pullup state while in host mode */
>         if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
>                 hsotg->enabled = is_on;
>                 return 0;
>         }
>
>         spin_lock_irqsave(&hsotg->lock, flags);
>         if (is_on) {
>                 hsotg->enabled = 1;
>                 dwc2_hsotg_core_init_disconnected(hsotg, false);
>                 dwc2_hsotg_core_connect(hsotg);
>         } else {
>                 dwc2_hsotg_core_disconnect(hsotg);
>                 dwc2_hsotg_disconnect(hsotg);
>                 hsotg->enabled = 0;
>         }
>
>         hsotg->gadget.speed = USB_SPEED_UNKNOWN;
>         spin_unlock_irqrestore(&hsotg->lock, flags);
>
>         return 0;
> }
>
> Here's what I think dwc2_hsotg_pullup() and dwc2_hsotg_udc_start()
> should contain:
>
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 9dc6c482b89e..dbe28947d3a9 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3388,6 +3388,7 @@ static void dwc2_hsotg_init(struct dwc2_hsotg *hsotg)
>         dwc2_writel(0, hsotg->regs + DAINTMSK);
>
>         /* Be in disconnected state until gadget is registered */
> +       /* REVISIT!!!! This should be done in probe()!!!! */
>         __orr32(hsotg->regs + DCTL, DCTL_SFTDISCON);
>
>         /* setup fifos */
> @@ -3428,26 +3429,6 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget,
>         unsigned long flags;
>         int ret;
>
> -       if (!hsotg) {
> -               pr_err("%s: called with no device\n", __func__);
> -               return -ENODEV;
> -       }
> -
> -       if (!driver) {
> -               dev_err(hsotg->dev, "%s: no driver\n", __func__);
> -               return -EINVAL;
> -       }
> -
> -       if (driver->max_speed < USB_SPEED_FULL)
> -               dev_err(hsotg->dev, "%s: bad speed\n", __func__);
> -
> -       if (!driver->setup) {
> -               dev_err(hsotg->dev, "%s: missing entry points\n", __func__);
> -               return -EINVAL;
> -       }
> -
> -       WARN_ON(hsotg->driver);
> -
>         driver->driver.bus = NULL;
>         hsotg->driver = driver;
>         hsotg->gadget.dev.of_node = hsotg->dev->of_node;
> @@ -3550,17 +3531,15 @@ static int dwc2_hsotg_pullup(struct usb_gadget *gadget, int is_on)
>         /* Don't modify pullup state while in host mode */
>         if (hsotg->op_state != OTG_STATE_B_PERIPHERAL) {
>                 hsotg->enabled = is_on;
> -               return 0;
> +               return -EINVAL;
>         }
>
>         spin_lock_irqsave(&hsotg->lock, flags);
>         if (is_on) {
>                 hsotg->enabled = 1;
> -               dwc2_hsotg_core_init_disconnected(hsotg, false);
>                 dwc2_hsotg_core_connect(hsotg);
>         } else {
>                 dwc2_hsotg_core_disconnect(hsotg);
> -               dwc2_hsotg_disconnect(hsotg);
>                 hsotg->enabled = 0;
>         }


So I reverted my proposed change and gave the above patch a try on my
HiKey device, but this didn't change the issue I'm seeing.  I still
get WARN_ONs in dwc2_hsotg_init_fifo() about the fifo_map not being
clear.  Adding my proposed change still helps this issue for me.

Though I do see the potential for calling dwc2_hsotg_disconnect()
twice as its already called in the  dwc2_hsotg_irq() path before
calling dwc2_hsotg_core_init_disconnected.

But if its of more help, the error I'm seeing seems to be triggered
from the dwc2_conn_id_status_change() code path calling
dwc2_hsotg_core_init_disconnected().

Would the proper fix to be calling dwc2_hsotg_disconnect() from
dwc2_conn_id_status_change() instead?

thanks
-john