Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758210AbbKSQhg (ORCPT ); Thu, 19 Nov 2015 11:37:36 -0500 Received: from mail-yk0-f171.google.com ([209.85.160.171]:33373 "EHLO mail-yk0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755519AbbKSQhd (ORCPT ); Thu, 19 Nov 2015 11:37:33 -0500 MIME-Version: 1.0 In-Reply-To: <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> <2B3535C5ECE8B5419E3ECBE30077290901DC3D696B@US01WEMBX2.internal.synopsys.com> Date: Thu, 19 Nov 2015 08:37:33 -0800 X-Google-Sender-Auth: Wyfa4OQNAlVSEQqULfjLiRnQ-y0 Message-ID: Subject: Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them From: Doug Anderson To: John Youn Cc: Felipe Balbi , Yunzhi Li , =?UTF-8?Q?Heiko_St=C3=BCbner?= , "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" 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: 4102 Lines: 107 Hi, On Wed, Nov 18, 2015 at 5:43 PM, John Youn wrote: > 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. How do you guys want this handled? Should I send up a new version of this patch? ...or should I send a followon patch that does this removal? -Doug -- 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/