Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754365AbcKHUh2 (ORCPT ); Tue, 8 Nov 2016 15:37:28 -0500 Received: from mail-it0-f46.google.com ([209.85.214.46]:37505 "EHLO mail-it0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754088AbcKHUhZ (ORCPT ); Tue, 8 Nov 2016 15:37:25 -0500 MIME-Version: 1.0 In-Reply-To: <87mvhkrckz.fsf@linux.intel.com> References: <1476943241-31810-1-git-send-email-john.stultz@linaro.org> <87mvhkrckz.fsf@linux.intel.com> From: John Stultz Date: Tue, 8 Nov 2016 12:37:19 -0800 Message-ID: Subject: Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset To: Felipe Balbi Cc: lkml , Wei Xu , Guodong Xu , Chen Yu , Amit Pundir , Rob Herring , Mark Rutland , John Youn , Douglas Anderson , Greg Kroah-Hartman , Linux USB List Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6699 Lines: 179 On Mon, Oct 31, 2016 at 4:18 AM, Felipe Balbi wrote: > John Stultz 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