Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161551AbcKADS4 (ORCPT ); Mon, 31 Oct 2016 23:18:56 -0400 Received: from mail-yw0-f180.google.com ([209.85.161.180]:35586 "EHLO mail-yw0-f180.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965427AbcKADSz (ORCPT ); Mon, 31 Oct 2016 23:18:55 -0400 MIME-Version: 1.0 In-Reply-To: <87h97sr863.fsf@linux.intel.com> References: <6fdb5cd4216755f07bb1a1e5d081e269046c60c6.1476949020.git.baolin.wang@linaro.org> <87h97sr863.fsf@linux.intel.com> From: Baolin Wang Date: Tue, 1 Nov 2016 11:18:53 +0800 Message-ID: Subject: Re: [PATCH v1] usb: dwc3: gadget: wait for End Transfer to complete To: Felipe Balbi Cc: Greg KH , Mark Brown , USB , LKML 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: 7865 Lines: 232 Hi, On 31 October 2016 at 20:53, Felipe Balbi wrote: > > 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 > > 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 > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 5fc437021ac7..c2b86856e85d 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -26,6 +26,7 @@ > #include > #include > #include > +#include > > #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 complete > * @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; > > + wait_queue_head_t wait_end_transfer; > + > spinlock_t lock; > void __iomem *regs; > > @@ -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) > > /* 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 > > u32 parameters:16; > + > +/* For Command Complete Events */ > +#define DEPEVT_PARAMETER_CMD(n) (((n) & (0xf << 8)) >> 8) > } __packed; > > /** > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index 3b53a5714df4..d544e7369776 100644 > --- 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 *dep, > dep->comp_desc = comp_desc; > dep->type = usb_endpoint_type(desc); > dep->flags |= DWC3_EP_ENABLED; > + dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING; > > reg = dwc3_readl(dwc->regs, DWC3_DALEPENA); > reg |= DWC3_DALEPENA_EP(dep->number); > dwc3_writel(dwc->regs, DWC3_DALEPENA, reg); > > + init_waitqueue_head(&dep->wait_end_transfer); > + > if (usb_endpoint_xfer_control(desc)) > return 0; > > @@ -699,7 +702,7 @@ static int __dwc3_gadget_ep_disable(struct dwc3_ep *dep) > dep->endpoint.desc = NULL; > dep->comp_desc = NULL; > dep->type = 0; > - dep->flags = 0; > + dep->flags &= DWC3_EP_END_TRANSFER_PENDING; > > return 0; > } > @@ -1783,9 +1786,6 @@ static int dwc3_gadget_start(struct usb_gadget *g, > > static void __dwc3_gadget_stop(struct dwc3 *dwc) > { > - if (pm_runtime_suspended(dwc->dev)) > - return; > - > 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 = gadget_to_dwc(g); > unsigned long flags; > + int epnum; > + > + if (pm_runtime_suspended(dwc->dev)) > + return 0; If we set the suspend checking at here, we will lose the chance to free gadget irq when stopping gadget. > > spin_lock_irqsave(&dwc->lock, flags); > __dwc3_gadget_stop(dwc); > dwc->gadget_driver = NULL; Can we set the suspend checking at here? And we should not delete the suspend checking in __dwc3_gadget_stop() function. + if (pm_runtime_suspended(dwc->dev)) { + spin_unlock_irqrestore(&dwc->lock, flags); + goto out; + } > + > + for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) { > + struct dwc3_ep *dep = 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); Add one tag here: + out: > free_irq(dwc->irq_gadget, dwc->ev_buf); > @@ -2171,10 +2190,12 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, > { > struct dwc3_ep *dep; > u8 epnum = event->endpoint_number; > + u8 cmd; > > dep = dwc->eps[epnum]; > > - if (!(dep->flags & DWC3_EP_ENABLED)) > + if (!(dep->flags & DWC3_EP_ENABLED) && > + !(dep->flags & DWC3_EP_END_TRANSFER_PENDING)) > return; > > if (epnum == 0 || epnum == 1) { > @@ -2215,8 +2236,15 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc, > return; > } > break; > - case DWC3_DEPEVT_RXTXFIFOEVT: > case DWC3_DEPEVT_EPCMDCMPLT: > + cmd = DEPEVT_PARAMETER_CMD(event->parameters); > + > + if (cmd == DWC3_DEPCMD_ENDTRANSFER) { > + dep->flags &= ~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 *dwc, u32 epnum, bool force) > > dep = dwc->eps[epnum]; > > - if (!dep->resource_index) > + if ((dep->flags & DWC3_EP_END_TRANSFER_PENDING) || > + !dep->resource_index) > return; > > /* > @@ -2313,8 +2342,10 @@ static void dwc3_stop_active_transfer(struct dwc3 *dwc, u32 epnum, bool force) > dep->resource_index = 0; > dep->flags &= ~DWC3_EP_BUSY; > > - if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) > + if (dwc3_is_usb31(dwc) || dwc->revision < DWC3_REVISION_310A) { > + dep->flags |= DWC3_EP_END_TRANSFER_PENDING; > udelay(100); > + } > } > > static void dwc3_clear_stall_all_ep(struct dwc3 *dwc) > > -- > balbi -- Baolin.wang Best Regards