Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933392AbbKSBoB (ORCPT ); Wed, 18 Nov 2015 20:44:01 -0500 Received: from smtprelay.synopsys.com ([198.182.60.111]:51404 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932461AbbKSBni convert rfc822-to-8bit (ORCPT ); Wed, 18 Nov 2015 20:43:38 -0500 From: John Youn To: Doug Anderson , Felipe Balbi CC: John Youn , Yunzhi Li , =?iso-8859-1?Q?Heiko_St=FCbner?= , "open list:ARM/Rockchip SoC..." , Julius Werner , "Herrero, Gregory" , "Kaukab, Yousaf" , Dinh Nguyen , Greg Kroah-Hartman , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them Thread-Topic: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them Thread-Index: AQHRFnai8Um1uMkwQEqCFc/AlKfO/A== Date: Thu, 19 Nov 2015 01:43:35 +0000 Message-ID: <2B3535C5ECE8B5419E3ECBE30077290901DC3D696B@US01WEMBX2.internal.synopsys.com> References: <1446582629-30091-1-git-send-email-dianders@chromium.org> <1446582629-30091-2-git-send-email-dianders@chromium.org> <87wpthnc2e.fsf@saruman.tx.rr.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.9.140.20] Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3788 Lines: 102 On 11/16/2015 9:22 AM, Doug Anderson wrote: > Felipe, > > On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi wrote: >> >> Hi, >> >> Douglas Anderson writes: >>> In general it is wise to clear interrupts before processing them. If >>> you don't do that, you can get: >>> 1. Interrupt happens >>> 2. You look at system state and process interrupt >>> 3. A new interrupt happens >>> 4. You clear interrupt without processing it. >>> >>> This patch was actually a first attempt to fix missing device insertions >>> as described in (usb: dwc2: host: Fix missing device insertions) and it >>> did solve some of the signal bouncing problems but not all of >>> them (which is why I submitted the other patch). Specifically, this >>> patch itself would sometimes change: >>> 1. hardware sees connect >>> 2. hardware sees disconnect >>> 3. hardware sees connect >>> 4. dwc2_port_intr() - clears connect interrupt >>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() >>> >>> ...to: >>> 1. hardware sees connect >>> 2. hardware sees disconnect >>> 3. dwc2_port_intr() - clears connect interrupt >>> 4. hardware sees connect >>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect() >>> >>> ...but with different timing then sometimes we'd still miss cable >>> insertions. >>> >>> In any case, though this patch doesn't fix any (known) problems, it >>> still seems wise as a general policy to clear interrupt before handling >>> them. >>> >>> Signed-off-by: Douglas Anderson >>> --- >>> Changes in v2: None >>> >>> drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++---------------------- >>> drivers/usb/dwc2/hcd_intr.c | 16 ++++++------- >>> 2 files changed, 35 insertions(+), 36 deletions(-) >>> >>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c >>> index 61601d16e233..2a166b7eec41 100644 >>> --- a/drivers/usb/dwc2/core_intr.c >>> +++ b/drivers/usb/dwc2/core_intr.c >>> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg) >>> */ >>> static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) >>> { >>> - u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0); >>> + u32 hprt0; >>> >>> + /* Clear interrupt */ >>> + dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS); >>> + >>> + hprt0 = dwc2_readl(hsotg->regs + HPRT0); >>> if (hprt0 & HPRT0_ENACHG) { >>> hprt0 &= ~HPRT0_ENA; >>> dwc2_writel(hprt0, hsotg->regs + HPRT0); >>> } >>> - >>> - /* Clear interrupt */ >>> - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS); >> >> isn't this a regression ? You're first clearing the interrupts and only >> then reading to check what's pending, however, what's pending has just >> been cleared. Seems like this should be: >> >> hprt0 = dwc2_readl(HPRT0); >> dwc2_writeal(PRTINT, GINTSTS); > > Actually, we could probably remove the setting of GINTSTS_PRTINT > completely. The docs I have say that the GINTSTS_PRTINT is read only > and that: > >> The core sets this bit to indicate a change in port status of one of the >> DWC_otg core ports in Host mode. The application must read the >> Host Port Control and Status (HPRT) register to determine the exact >> event that caused this interrupt. The application must clear the >> appropriate status bit in the Host Port Control and Status register to >> clear this bit. > > ...so writing PRTINT is probably useless, but John can confirm. > Yup, it seems it can be removed. John -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/