Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S942373AbcJaMye (ORCPT ); Mon, 31 Oct 2016 08:54:34 -0400 Received: from mga07.intel.com ([134.134.136.100]:7893 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S941925AbcJaMyT (ORCPT ); Mon, 31 Oct 2016 08:54:19 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,575,1473145200"; d="asc'?scan'208";a="25658086" From: Felipe Balbi To: Baolin Wang Cc: gregkh@linuxfoundation.org, broonie@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, baolin.wang@linaro.org Subject: Re: [PATCH v1] usb: dwc3: gadget: wait for End Transfer to complete In-Reply-To: <6fdb5cd4216755f07bb1a1e5d081e269046c60c6.1476949020.git.baolin.wang@linaro.org> References: <6fdb5cd4216755f07bb1a1e5d081e269046c60c6.1476949020.git.baolin.wang@linaro.org> Date: Mon, 31 Oct 2016 14:53:56 +0200 Message-ID: <87h97sr863.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: 7138 Lines: 237 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Baolin Wang writes: > Instead of just delaying for 100us, we should > actually wait for End Transfer Command Complete > interrupt before moving on. Note that this should > only be done if we're dealing with one of the core > revisions that actually require the interrupt before > moving on. > > [ felipe.balbi@linux.intel.com: minor improvements ] > > Signed-off-by: Baolin Wang I made one extra modification to prevent us from checking for pm_runtime_suspended() twice: commit 8f48e8d6d3dfe75b5582c8a7b1ee5739a393748c Author: Baolin Wang Date: Mon Oct 31 19:38:36 2016 +0800 usb: dwc3: gadget: wait for End Transfer to complete =20=20=20=20 Instead of just delaying for 100us, we should actually wait for End Transfer Command Complete interrupt before moving on. Note that this should only be done if we're dealing with one of the core revisions that actually require the interrupt before moving on. =20=20=20=20 [ felipe.balbi@linux.intel.com: minor improvements ] =20=20=20=20 Signed-off-by: Baolin Wang Signed-off-by: Felipe Balbi diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 5fc437021ac7..c2b86856e85d 100644 =2D-- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -26,6 +26,7 @@ #include #include #include +#include =20 #include #include @@ -505,6 +506,7 @@ struct dwc3_event_buffer { * @endpoint: usb endpoint * @pending_list: list of pending requests for this endpoint * @started_list: list of started requests on this endpoint + * @wait_end_transfer: wait_queue_head_t for waiting on End Transfer compl= ete * @lock: spinlock for endpoint request queue traversal * @regs: pointer to first endpoint register * @trb_pool: array of transaction buffers @@ -530,6 +532,8 @@ struct dwc3_ep { struct list_head pending_list; struct list_head started_list; =20 + wait_queue_head_t wait_end_transfer; + spinlock_t lock; void __iomem *regs; =20 @@ -546,6 +550,7 @@ struct dwc3_ep { #define DWC3_EP_BUSY (1 << 4) #define DWC3_EP_PENDING_REQUEST (1 << 5) #define DWC3_EP_MISSED_ISOC (1 << 6) +#define DWC3_EP_END_TRANSFER_PENDING (1 << 7) =20 /* This last one is specific to EP0 */ #define DWC3_EP0_DIR_IN (1 << 31) @@ -1050,6 +1055,9 @@ struct dwc3_event_depevt { #define DEPEVT_TRANSFER_BUS_EXPIRY 2 =20 u32 parameters:16; + +/* For Command Complete Events */ +#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 8)) >> 8) } __packed; =20 /** diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index 3b53a5714df4..d544e7369776 100644 =2D-- a/drivers/usb/dwc3/gadget.c +++ b/drivers/usb/dwc3/gadget.c @@ -593,11 +593,14 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *de= p, dep->comp_desc =3D comp_desc; dep->type =3D usb_endpoint_type(desc); dep->flags |=3D DWC3_EP_ENABLED; + dep->flags &=3D ~DWC3_EP_END_TRANSFER_PENDING; =20 reg =3D dwc3_readl(dwc->regs, DWC3_DALEPENA); reg |=3D DWC3_DALEPENA_EP(dep->number); dwc3_writel(dwc->regs, DWC3_DALEPENA, reg); =20 + init_waitqueue_head(&dep->wait_end_transfer); + if (usb_endpoint_xfer_control(desc)) return 0; =20 @@ -699,7 +702,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) dep->endpoint.desc =3D NULL; dep->comp_desc =3D NULL; dep->type =3D 0; =2D dep->flags =3D 0; + dep->flags &=3D DWC3_EP_END_TRANSFER_PENDING; =20 return 0; } @@ -1783,9 +1786,6 @@ static int dwc3_gadget_start(struct usb_gadget *g, =20 static void __dwc3_gadget_stop(struct dwc3 *dwc) { =2D if (pm_runtime_suspended(dwc->dev)) =2D return; =2D dwc3_gadget_disable_irq(dwc); __dwc3_gadget_ep_disable(dwc->eps[0]); __dwc3_gadget_ep_disable(dwc->eps[1]); @@ -1795,10 +1795,29 @@ static int dwc3_gadget_stop(struct usb_gadget *g) { struct dwc3 *dwc =3D gadget_to_dwc(g); unsigned long flags; + int epnum; + + if (pm_runtime_suspended(dwc->dev)) + return 0; =20 spin_lock_irqsave(&dwc->lock, flags); __dwc3_gadget_stop(dwc); dwc->gadget_driver =3D NULL; + + for (epnum =3D 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) { + struct dwc3_ep *dep =3D dwc->eps[epnum]; + + if (!dep) + continue; + + if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) + continue; + + wait_event_lock_irq(dep->wait_end_transfer, + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING), + dwc->lock); + } + spin_unlock_irqrestore(&dwc->lock, flags); =20 free_irq(dwc->irq_gadget, dwc->ev_buf); @@ -2171,10 +2190,12 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dw= c, { struct dwc3_ep *dep; u8 epnum =3D event->endpoint_number; + u8 cmd; =20 dep =3D dwc->eps[epnum]; =20 =2D if (!(dep->flags & DWC3_EP_ENABLED)) + if (!(dep->flags & DWC3_EP_ENABLED) && + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) return; =20 if (epnum =3D=3D 0 || epnum =3D=3D 1) { @@ -2215,8 +2236,15 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, return; } break; =2D case DWC3_DEPEVT_RXTXFIFOEVT: case DWC3_DEPEVT_EPCMDCMPLT: + cmd =3D DEPEVT_PARAMETER_CMD(event->parameters); + + if (cmd =3D=3D DWC3_DEPCMD_ENDTRANSFER) { + dep->flags &=3D ~DWC3_EP_END_TRANSFER_PENDING; + wake_up(&dep->wait_end_transfer); + } + break; + case DWC3_DEPEVT_RXTXFIFOEVT: break; } } @@ -2269,7 +2297,8 @@ static void dwc3_stop_active_transfer(struct dwc3 *dw= c, u32 epnum, bool force) =20 dep =3D dwc->eps[epnum]; =20 =2D if (!dep->resource_index) + if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) || + !dep->resource_index) return; =20 /* @@ -2313,8 +2342,10 @@ static void dwc3_stop_active_transfer(struct dwc3 *d= wc, u32 epnum, bool force) dep->resource_index =3D 0; dep->flags &=3D ~DWC3_EP_BUSY; =20 =2D if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) + if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) { + dep->flags |=3D DWC3_EP_END_TRANSFER_PENDING; udelay(100); + } } =20 static void dwc3_clear_stall_all_ep(struct dwc3 *dwc) =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYFz7kAAoJEMy+uJnhGpkG2AwP/27TIkIfnd7b43076QqmtegE aUD1JwRr5ApQqwIrV+hQdYIWR24WFOpvVcynqc/WqqCPSDO+CbvKFV0jnVPwMsU/ WWD1k0qXsxfYO15/LLqZ+kBVgfgiP3EYMl/fvTfkmxzHOd5giNkkgf0rBEmPTVbx 5AHTYNkXbkCgkp5XdbIWEuOFwTTZyf5ALfVXt3dQzYqwRWSCNbCO0WtZYXyyzBBO fa4nduLsyDyCAo7du395SxomKIdDN/Jn7g3V/wganzOjrBfksUKmcRs2yXJflAzT JyBcy3xkVzqNeeP4Yv4HmSN24NmB4hxLyuQfa3AphLHqLKeT1cr0mGsoNadfHzw4 iqr2lQAACGeIXs/d8qWINEYX/XGTef7Som9eSGV9WsbMwq0IbPj6Fw6Dt3LpxIhx TpSuZhja06oRaI4f07x3vzONHUJ537nnwLDOh/FhY+AUDqUfDGN0aC26KiXbw9RG 1icBRnk/jA90As6paoNOND7LgtEa+YDMm8PfmCRvQBnoP5N7DiZUkA6xEp1yM2X0 nCqFp7LV5dchfL4maWp8MEt7zS3vtL/swEG4KoYt8OUglBeYEfVn/UkKOF2pOEkC Zk+w5t3idyP/HBk/gs2CNHykY8sT7OPWfYqkSsktJNutz5FsgQyNFX9Ow593mwF4 yjmGiv8gMuhu0y2v5tWt =n0EF -----END PGP SIGNATURE----- --=-=-=--