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@xxxxxxxxxxxxxxx> wrote:
> John Stultz <john.stultz@xxxxxxxxxx> 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