Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1168805AbcKALBx (ORCPT ); Tue, 1 Nov 2016 07:01:53 -0400 Received: from mga07.intel.com ([134.134.136.100]:14935 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1168772AbcKALBv (ORCPT ); Tue, 1 Nov 2016 07:01:51 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,579,1473145200"; d="asc'?scan'208";a="26420859" 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 v2] usb: dwc3: gadget: wait for End Transfer to complete In-Reply-To: References: Date: Tue, 01 Nov 2016 13:01:28 +0200 Message-ID: <87twbrpipj.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: 7432 Lines: 254 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Baolin Wang writes: > Changes since v1: > - Move the suspend checking to right place to avoid checking twice. there is still one problem > @@ -1736,12 +1739,38 @@ static int dwc3_gadget_stop(struct usb_gadget *g) > { > struct dwc3 *dwc =3D gadget_to_dwc(g); > unsigned long flags; > + int epnum; >=20=20 > spin_lock_irqsave(&dwc->lock, flags); > __dwc3_gadget_stop(dwc); this tries to access registers. If __dwc3_gadget_stop() runs while clocks are gated, we will get Data Abort exception. How about this version, instead? 8<-------------------------------------------------------------------------= ----- From=20e21260f349271ff1d12e919be20a9ee47e29e4b4 Mon Sep 17 00:00:00 2001 From: Baolin Wang Date: Mon, 31 Oct 2016 19:38:36 +0800 Subject: [PATCH] usb: dwc3: gadget: wait for End Transfer to complete 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 Signed-off-by: Felipe Balbi =2D-- drivers/usb/dwc3/core.h | 8 ++++++++ drivers/usb/dwc3/gadget.c | 49 +++++++++++++++++++++++++++++++++++++++----= ---- 2 files changed, 49 insertions(+), 8 deletions(-) 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 4743e53cc295..a375fd28ed96 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,31 @@ static int dwc3_gadget_stop(struct usb_gadget *g) { struct dwc3 *dwc =3D gadget_to_dwc(g); unsigned long flags; + int epnum; =20 spin_lock_irqsave(&dwc->lock, flags); + + if (pm_runtime_suspended(dwc->dev)) + goto out; + __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); + } + +out: spin_unlock_irqrestore(&dwc->lock, flags); =20 free_irq(dwc->irq_gadget, dwc->ev_buf); @@ -2171,10 +2192,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 +2238,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 +2299,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 +2344,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 2.10.1 =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYGHYIAAoJEMy+uJnhGpkGfE8P/2BxC+rsAJ+AudyxBcF3YnDy lAmS4gK4pANzhh6Qt2aaWHpHOtLN53+4cphazIrc8WfO75MYqlZK2BNjr4C2xY/P F9c7MbuogeOuO29K3/8BVdAoJcuvxxj7C60kNxxzbE7kGJ2YS3K+fARFiLfawrSB gUItPntjW2iXXWkk/LG2QzIVheVwTMp0wMBmkQIKW4RsWidQoSHhsQ+9l6eih3Sb Znh7ZIdel3asXJgGQ926YV5eN8LormuBW72kIDd2ZR69v/dkh+if6OybxS/2sSnE 7sivTpqJfPo5lfBilK3UOQggDKVmhkDTX4EL53UhVwiq4mGQILUm2XXv1ecXiZQz I6cPgn8oWGwKRRuU67Wfrp7CX07eysc5FCV9BTJmywjGNdSAFoLyOZWSjV5a7nkC CCywyhfiodXbawvXD7FD274rAYuq1+lsmQmFwEVQtECxfC94PQ+i3/97IT9EJOlG 5Wf1iaz1eXltT6xgaZ+kExC6mLHdKGbh9K5I81oGdDtwS1mhowJT1aqsrYMAzvfV UuXhuWcabBplJsSIIXY5m4v98FkDJf23ObeLUohnDA5rt1MGUZbibuaH4Ahq0Vlb Ue9MOEbwIJ9EzqoFkt9FL/yvEu/+8SRrE3EI0DpltVPiHCFN7+OyrZVyf0MTNUUh ZmbR4YKEnZhcpzoi8pHe =OUDP -----END PGP SIGNATURE----- --=-=-=--