Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932391AbdGJOOi (ORCPT ); Mon, 10 Jul 2017 10:14:38 -0400 Received: from mga09.intel.com ([134.134.136.24]:23592 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932217AbdGJOOg (ORCPT ); Mon, 10 Jul 2017 10:14:36 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,340,1496127600"; d="asc'?scan'208";a="1170721376" From: Felipe Balbi To: Minas Harutyunyan , John Youn , Greg Kroah-Hartman , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Minas Harutyunyan Subject: Re: [PATCH] usb: dwc2: gadget: On disconnect reset device address to zero In-Reply-To: <87d19875fc.fsf@linux.intel.com> References: <87d19875fc.fsf@linux.intel.com> Date: Mon, 10 Jul 2017 17:14:18 +0300 Message-ID: <87a84c751h.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: 4806 Lines: 141 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi again, Felipe Balbi writes: > Hi, > > Minas Harutyunyan writes: >> USB CV driver stack doesn't perform USB RESET after device disconnect/ >> connect, so need to reset to zero DEVADDR field in DCFG to pass >> enumeration again. >> >> Signed-off-by: Minas Harutyunyan >> --- >> drivers/usb/dwc2/gadget.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 98a4a79e7f6e..deb3d901b99d 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -3174,6 +3174,9 @@ void dwc2_hsotg_disconnect(struct dwc2_hsotg *hsot= g) >> return; >>=20=20 >> hsotg->connected =3D 0; >> + /* On disconnect reset device address to zero */ >> + __bic32(hsotg->regs + DCFG, DCFG_DEVADDR_MASK); >> + > > Which of the tests are you talking about? Which particular USB CV are > you running? > > I don't remember ever seeing this with dwc3. How should I go about > triggering this problem? If this was really the case, then we would have > this on *all* UDCs. > > I just did a fresh install of USB 3 Gen X CV (that I just download from > usb.org). Ran Chapter 9 tests against a HS dwc3 board I have around and > I can't see the problem you're talking about. > > Here are all my non-endpoint interrupt events in order. Test passes > fine. If disconnect, reconnect and run the tests again, then Reset will > be driven on the bus which will cause address to be reset. > > Can you share more details about the problem you're facing? I've been looking at dwc2 for a while and I think this is a bug in dwc2_hsotg_irq() on the branch for GINTSTS_USBRST. I don't have docs for dwc2. Nowhere in that function is driver making sure DCFG_DEVADDR is cleared to 0. In fact, there nowhere at all in the driver where you're making sure to reset device address: $ git --no-pager grep --color -nH -e DCFG_DEVADDR_ -- drivers/usb/dwc2/ drivers/usb/dwc2/gadget.c:1837: dcfg &=3D ~DCFG_DEVADDR_MASK; drivers/usb/dwc2/gadget.c:1839: DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK; drivers/usb/dwc2/hw.h:426:#define DCFG_DEVADDR_MASK (0x7f << 4) drivers/usb/dwc2/hw.h:427:#define DCFG_DEVADDR_SHIFT 4 drivers/usb/dwc2/hw.h:428:#define DCFG_DEVADDR_LIMIT 0x7f If we look into gadget.c on those lines, here's what we have: static void dwc2_hsotg_process_control(struct dwc2_hsotg *hsotg, struct usb_ctrlrequest *ctrl) { [...] struct dwc2_hsotg_ep *ep0 =3D hsotg->eps_out[0]; int ret =3D 0; u32 dcfg; dev_dbg(hsotg->dev, "ctrl Type=3D%02x, Req=3D%02x, V=3D%04x, I=3D%04x, L=3D%04x\n", ctrl->bRequestType, ctrl->bRequest, ctrl->wValue, ctrl->wIndex, ctrl->wLength); if (ctrl->wLength =3D=3D 0) { ep0->dir_in =3D 1; hsotg->ep0_state =3D DWC2_EP0_STATUS_IN; } else if (ctrl->bRequestType & USB_DIR_IN) { ep0->dir_in =3D 1; hsotg->ep0_state =3D DWC2_EP0_DATA_IN; } else { ep0->dir_in =3D 0; hsotg->ep0_state =3D DWC2_EP0_DATA_OUT; } if ((ctrl->bRequestType & USB_TYPE_MASK) =3D=3D USB_TYPE_STANDARD) { switch (ctrl->bRequest) { case USB_REQ_SET_ADDRESS: hsotg->connected =3D 1; dcfg =3D dwc2_readl(hsotg->regs + DCFG); dcfg &=3D ~DCFG_DEVADDR_MASK; dcfg |=3D (le16_to_cpu(ctrl->wValue) << DCFG_DEVADDR_SHIFT) & DCFG_DEVADDR_MASK; dwc2_writel(dcfg, hsotg->regs + DCFG); dev_info(hsotg->dev, "new address %d\n", ctrl->wValue); ret =3D dwc2_hsotg_send_reply(hsotg, ep0, NULL, 0); return; [...] } So you only touch DEVADDR from SetAddress. That's clearly a bug elsewhere in the driver and is *NOT* related to USB CV at all. Please fix this driver properly instead of hacking around unrelated functions. Disconnect IRQ is *NOT* supposed to clear device address. That's a job for Reset IRQ. =2D-=20 pbalbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAllji7oACgkQzL64meEa mQYkbQ//QZ9cmDnVdylZxwy2cOXVFXobsLHzY+vlk2bLnitCeDKItdMLUZdBy9M+ aYqEZUM+wXkJYppOuPnGqjtXH0zdPX1Nn/f6s+cBgfVqSZZaQQM16t1zgDlq16+I Xi0K7rrBbs4fN1ksUGnwFMGiThtdGDW5dO0fylZngeHzPgMTWiDsC9XXR396V4jd V+LMq93bdlTa3YGe0PPnVKPn5b9xnSlIcReGXPf3xoKULxmn11KkMMbvH8HiSzOx ab8gF4oJAkIfSzJQJsHjIhAnhBXPryE1aoiKQZuVmGAqGvXJpxjNVlQmsQAZkwG+ vTieoVdM/wGqoMveN6bA94UVJ5AVSx4GcbMjTTKiIzZfM8mpVA7aXcUkcAUK3jk/ FeljJLPG0muQ3DiWodgJTw7Ld3qgcOvutgqfk2oBtmasklSoMeBA0clwKKa3kDEb IOyQRJg36HCTvSk3L8JRwMLzsrS94xbNJO2J9AEurCfoIR2CC9ubNznUg3VRU/k5 7e7GabkBewohw/Xq44CjDFX4UNWly15ALKhmmX1wZlaT/6Bv6PHZ4oqQwD8HWLhm KkCzoig/dx5qMFuOxoHRIePBvc1/afkhgjgEXeWhWPsdOF84O2XyBZpJ50RlFE/G TfM9RI3rJIaxZ7aVUluy58K/AbyNSO2a5i60OmU771DrqBHiIzI= =N/6t -----END PGP SIGNATURE----- --=-=-=--