Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S938902AbcJaLTi (ORCPT ); Mon, 31 Oct 2016 07:19:38 -0400 Received: from mga02.intel.com ([134.134.136.20]:11554 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934692AbcJaLTg (ORCPT ); Mon, 31 Oct 2016 07:19:36 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,426,1473145200"; d="asc'?scan'208";a="1078390389" From: Felipe Balbi To: John Stultz , lkml Cc: John Stultz , Wei Xu , Guodong Xu , Chen Yu , Amit Pundir , Rob Herring , Mark Rutland , John Youn , Douglas Anderson , Greg Kroah-Hartman , linux-usb@vger.kernel.org Subject: Re: [RFC][PATCH] usb: dwc2: Make sure we disconnect the gadget state on reset In-Reply-To: <1476943241-31810-1-git-send-email-john.stultz@linaro.org> References: <1476943241-31810-1-git-send-email-john.stultz@linaro.org> Date: Mon, 31 Oct 2016 13:18:36 +0200 Message-ID: <87mvhkrckz.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7725 Lines: 233 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, 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! > > Cc: Wei Xu > Cc: Guodong Xu > Cc: Chen Yu > Cc: Amit Pundir > Cc: Rob Herring > Cc: Mark Rutland > Cc: John Youn > Cc: Douglas Anderson > Cc: Greg Kroah-Hartman > Cc: linux-usb@vger.kernel.org > Signed-off-by: John Stultz > --- > 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, >=20=20 > /* 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 =3D dwc2_readl(hsotg->regs + GOTGCTL); u32 connected =3D hsotg->connected; dev_dbg(hsotg->dev, "%s: USBRst\n", __func__); dev_dbg(hsotg->dev, "GNPTXSTS=3D%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 =3D to_hsotg(gadget); unsigned long flags =3D 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 !=3D OTG_STATE_B_PERIPHERAL) { hsotg->enabled =3D is_on; return 0; } spin_lock_irqsave(&hsotg->lock, flags); if (is_on) { hsotg->enabled =3D 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 =3D 0; } hsotg->gadget.speed =3D 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 =2D-- 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); =20 /* Be in disconnected state until gadget is registered */ + /* REVISIT!!!! This should be done in probe()!!!! */ __orr32(hsotg->regs + DCTL, DCTL_SFTDISCON); =20 /* setup fifos */ @@ -3428,26 +3429,6 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *g= adget, unsigned long flags; int ret; =20 =2D if (!hsotg) { =2D pr_err("%s: called with no device\n", __func__); =2D return -ENODEV; =2D } =2D =2D if (!driver) { =2D dev_err(hsotg->dev, "%s: no driver\n", __func__); =2D return -EINVAL; =2D } =2D =2D if (driver->max_speed < USB_SPEED_FULL) =2D dev_err(hsotg->dev, "%s: bad speed\n", __func__); =2D =2D if (!driver->setup) { =2D dev_err(hsotg->dev, "%s: missing entry points\n", __func__); =2D return -EINVAL; =2D } =2D =2D WARN_ON(hsotg->driver); =2D driver->driver.bus =3D NULL; hsotg->driver =3D driver; hsotg->gadget.dev.of_node =3D hsotg->dev->of_node; @@ -3550,17 +3531,15 @@ static int dwc2_hsotg_pullup(struct usb_gadget *gad= get, int is_on) /* Don't modify pullup state while in host mode */ if (hsotg->op_state !=3D OTG_STATE_B_PERIPHERAL) { hsotg->enabled =3D is_on; =2D return 0; + return -EINVAL; } =20 spin_lock_irqsave(&hsotg->lock, flags); if (is_on) { hsotg->enabled =3D 1; =2D dwc2_hsotg_core_init_disconnected(hsotg, false); dwc2_hsotg_core_connect(hsotg); } else { dwc2_hsotg_core_disconnect(hsotg); =2D dwc2_hsotg_disconnect(hsotg); hsotg->enabled =3D 0; } =20 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) } =20 static const struct usb_gadget_ops dwc2_hsotg_gadget_ops =3D { =2D .get_frame =3D dwc2_hsotg_gadget_getframe, + .get_frame =3D dwc2_hsotg_gadget_getframe, .udc_start =3D dwc2_hsotg_udc_start, .udc_stop =3D dwc2_hsotg_udc_stop, =2D .pullup =3D dwc2_hsotg_pullup, + .pullup =3D dwc2_hsotg_pullup, .vbus_session =3D dwc2_hsotg_vbus_session, .vbus_draw =3D 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 =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYFyiMAAoJEMy+uJnhGpkGAhIQAKuGtVEErneg6IeTAvyavLVI 1ijjdqBb7Pj7+bX0T6MONhOV1C8pFjqN4+IumlutodPqTX/xiERqAjJ80gARnLS+ u3qCf/a/y6TfHm/zB9Q4C4YIJgRlUiJKLd89y+hhG6O5IH9AEpyiLW7AR5BqFXGW AwALxCqhybMP+MYbVMW1NKX6BJ3cPQcbbLP1k/pkLMTeVw25LHaGepe9cTxuIudX F9vf4lKNxQ9KH37YEZskp6FECPF3//KhOLeRa1mBZkfPiXH8uxl7Fdm2/7ZY0ybU ApaUjILS5ocpyyb4CoKeHoDvwUKdvfgYQMxwyy0GIU68z/ErAOFl9AF3tM09tjbu RIk+6POG36IhD2CJlxfeO6ZNkPvnfzg7FrV0T0dbtT0jC77UTH1SRUTuNkT4U3T6 uZ2/NoBlvmZj/Is3ZTXVYZ0hINIUreP936v5e1Las2H7OMy2iHSRS3IxxblayRHr Od80xrgNhZwDMqSg3VEbnZaOiH51YBGYkBXUtDjvdfVgZS3EpFQYf10sXUBlrp63 ov5OW3aARNf0hpHEHr4xct62I2nVpZxKHCFYqPIH4ps5iaire+nlB5+83XLt1k4W zmWx9bSNHdEx0YUJXNlBj2O0OYBxG2eJvkRPs25Rb7bkwKbIE9feMf8eGfVxMlag JjAM8YHVQO9Q+RI2WVJA =7ax+ -----END PGP SIGNATURE----- --=-=-=--