Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761100AbbKTWsd (ORCPT ); Fri, 20 Nov 2015 17:48:33 -0500 Received: from smtprelay.synopsys.com ([198.182.60.111]:42103 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761014AbbKTWsc convert rfc822-to-8bit (ORCPT ); Fri, 20 Nov 2015 17:48:32 -0500 From: John Youn To: Douglas Anderson , John Youn , "balbi@ti.com" CC: Yunzhi Li , =?iso-8859-1?Q?Heiko_St=FCbner?= , "linux-rockchip@lists.infradead.org" , Julius Werner , "gregory.herrero@intel.com" , "yousaf.kaukab@intel.com" , "dinguyen@opensource.altera.com" , "gregkh@linuxfoundation.org" , "linux-usb@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v5 2/2] usb: dwc2: host: Clear interrupts before handling them Thread-Topic: [PATCH v5 2/2] usb: dwc2: host: Clear interrupts before handling them Thread-Index: AQHRI7XUjvxq5dQnHkKu6th2MDIjFg== Date: Fri, 20 Nov 2015 22:48:29 +0000 Message-ID: <2B3535C5ECE8B5419E3ECBE30077290901DC3DAFF7@US01WEMBX2.internal.synopsys.com> References: <1448039188-14296-1-git-send-email-dianders@chromium.org> <1448039188-14296-2-git-send-email-dianders@chromium.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.10.161.90] Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 10211 Lines: 299 On 11/20/2015 9:06 AM, Douglas Anderson wrote: > 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. > > Note that for dwc2_handle_usb_port_intr(), instead of moving the clear > of PRTINT to the beginning of the function we remove it completely. The > only way to clear PRTINT is to clear the sources that set it in the > first place. > > Signed-off-by: Douglas Anderson > --- > Changes in v5: > - Rebased upon testing/next from 15-11-20. > - Fixed missing reset as found by code inspection during rebase. > - Now atop fix for missing spinlock in reset. > - Dropped John Youn's ack / tested by since there's a bugfix now. > > Changes in v4: > - Don't replace dwc2_writel with writel (Antti Sepp?l?). > - Update description to explain why we remove PRTINT clear. > > Changes in v3: > - Don't (uselessly) clear the PRTINT anymore (Felipe Balbi). > > drivers/usb/dwc2/core_intr.c | 49 +++++++++++++++++++++----------------------- > drivers/usb/dwc2/hcd_intr.c | 18 ++++++++-------- > 2 files changed, 32 insertions(+), 35 deletions(-) > > diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c > index 61601d16e233..d85c5c9f96c1 100644 > --- a/drivers/usb/dwc2/core_intr.c > +++ b/drivers/usb/dwc2/core_intr.c > @@ -86,9 +86,6 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) > hprt0 &= ~HPRT0_ENA; > dwc2_writel(hprt0, hsotg->regs + HPRT0); > } > - > - /* Clear interrupt */ > - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS); > } > > /** > @@ -98,11 +95,11 @@ static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg) > */ > static void dwc2_handle_mode_mismatch_intr(struct dwc2_hsotg *hsotg) > { > - dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n", > - dwc2_is_host_mode(hsotg) ? "Host" : "Device"); > - > /* Clear interrupt */ > dwc2_writel(GINTSTS_MODEMIS, hsotg->regs + GINTSTS); > + > + dev_warn(hsotg->dev, "Mode Mismatch Interrupt: currently in %s mode\n", > + dwc2_is_host_mode(hsotg) ? "Host" : "Device"); > } > > /** > @@ -276,9 +273,13 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg) > */ > static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg) > { > - u32 gintmsk = dwc2_readl(hsotg->regs + GINTMSK); > + u32 gintmsk; > + > + /* Clear interrupt */ > + dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS); > > /* Need to disable SOF interrupt immediately */ > + gintmsk = dwc2_readl(hsotg->regs + GINTMSK); > gintmsk &= ~GINTSTS_SOF; > dwc2_writel(gintmsk, hsotg->regs + GINTMSK); > > @@ -295,9 +296,6 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg) > queue_work(hsotg->wq_otg, &hsotg->wf_otg); > spin_lock(&hsotg->lock); > } > - > - /* Clear interrupt */ > - dwc2_writel(GINTSTS_CONIDSTSCHNG, hsotg->regs + GINTSTS); > } > > /** > @@ -315,12 +313,12 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg) > { > int ret; > > - dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n", > - hsotg->lx_state); > - > /* Clear interrupt */ > dwc2_writel(GINTSTS_SESSREQINT, hsotg->regs + GINTSTS); > > + dev_dbg(hsotg->dev, "Session request interrupt - lx_state=%d\n", > + hsotg->lx_state); > + > if (dwc2_is_device_mode(hsotg)) { > if (hsotg->lx_state == DWC2_L2) { > ret = dwc2_exit_hibernation(hsotg, true); > @@ -347,6 +345,10 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg) > static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) > { > int ret; > + > + /* Clear interrupt */ > + dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS); > + > dev_dbg(hsotg->dev, "++Resume or Remote Wakeup Detected Interrupt++\n"); > dev_dbg(hsotg->dev, "%s lxstate = %d\n", __func__, hsotg->lx_state); > > @@ -368,10 +370,9 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) > /* Change to L0 state */ > hsotg->lx_state = DWC2_L0; > } else { > - if (hsotg->core_params->hibernation) { > - dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS); > + if (hsotg->core_params->hibernation) > return; > - } > + > if (hsotg->lx_state != DWC2_L1) { > u32 pcgcctl = dwc2_readl(hsotg->regs + PCGCTL); > > @@ -385,9 +386,6 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) > hsotg->lx_state = DWC2_L0; > } > } > - > - /* Clear interrupt */ > - dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS); > } > > /* > @@ -396,14 +394,14 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg) > */ > static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg) > { > + dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS); > + > dev_dbg(hsotg->dev, "++Disconnect Detected Interrupt++ (%s) %s\n", > dwc2_is_host_mode(hsotg) ? "Host" : "Device", > dwc2_op_state_str(hsotg)); > > if (hsotg->op_state == OTG_STATE_A_HOST) > dwc2_hcd_disconnect(hsotg, false); > - > - dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS); > } > > /* > @@ -419,6 +417,9 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg) > u32 dsts; > int ret; > > + /* Clear interrupt */ > + dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS); > + > dev_dbg(hsotg->dev, "USB SUSPEND\n"); > > if (dwc2_is_device_mode(hsotg)) { > @@ -437,7 +438,7 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg) > if (!dwc2_is_device_connected(hsotg)) { > dev_dbg(hsotg->dev, > "ignore suspend request before enumeration\n"); > - goto clear_int; > + return; > } > > ret = dwc2_enter_hibernation(hsotg); > @@ -476,10 +477,6 @@ skip_power_saving: > hsotg->op_state = OTG_STATE_A_HOST; > } > } > - > -clear_int: > - /* Clear interrupt */ > - dwc2_writel(GINTSTS_USBSUSP, hsotg->regs + GINTSTS); > } > > #define GINTMSK_COMMON (GINTSTS_WKUPINT | GINTSTS_SESSREQINT | \ > diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c > index a1eb48e54858..f8253803a050 100644 > --- a/drivers/usb/dwc2/hcd_intr.c > +++ b/drivers/usb/dwc2/hcd_intr.c > @@ -122,6 +122,9 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg) > struct dwc2_qh *qh; > enum dwc2_transaction_type tr_type; > > + /* Clear interrupt */ > + dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS); > + > #ifdef DEBUG_SOF > dev_vdbg(hsotg->dev, "--Start of Frame Interrupt--\n"); > #endif > @@ -146,9 +149,6 @@ static void dwc2_sof_intr(struct dwc2_hsotg *hsotg) > tr_type = dwc2_hcd_select_transactions(hsotg); > if (tr_type != DWC2_TRANSACTION_NONE) > dwc2_hcd_queue_transactions(hsotg, tr_type); > - > - /* Clear interrupt */ > - dwc2_writel(GINTSTS_SOF, hsotg->regs + GINTSTS); > } > > /* > @@ -312,6 +312,7 @@ static void dwc2_hprt0_enable(struct dwc2_hsotg *hsotg, u32 hprt0, > > if (do_reset) { > *hprt0_modify |= HPRT0_RST; > + dwc2_writel(*hprt0_modify, hsotg->regs + HPRT0); > queue_delayed_work(hsotg->wq_otg, &hsotg->reset_work, > msecs_to_jiffies(60)); > } else { > @@ -347,11 +348,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) > * Set flag and clear if detected > */ > if (hprt0 & HPRT0_CONNDET) { > + dwc2_writel(hprt0_modify | HPRT0_CONNDET, hsotg->regs + HPRT0); > + > dev_vdbg(hsotg->dev, > "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n", > hprt0); > dwc2_hcd_connect(hsotg); > - hprt0_modify |= HPRT0_CONNDET; > > /* > * The Hub driver asserts a reset when it sees port connect > @@ -364,10 +366,10 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) > * Clear if detected - Set internal flag if disabled > */ > if (hprt0 & HPRT0_ENACHG) { > + dwc2_writel(hprt0_modify | HPRT0_ENACHG, hsotg->regs + HPRT0); > dev_vdbg(hsotg->dev, > " --Port Interrupt HPRT0=0x%08x Port Enable Changed (now %d)--\n", > hprt0, !!(hprt0 & HPRT0_ENA)); > - hprt0_modify |= HPRT0_ENACHG; > if (hprt0 & HPRT0_ENA) { > hsotg->new_connection = true; > dwc2_hprt0_enable(hsotg, hprt0, &hprt0_modify); > @@ -387,15 +389,13 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg) > > /* Overcurrent Change Interrupt */ > if (hprt0 & HPRT0_OVRCURRCHG) { > + dwc2_writel(hprt0_modify | HPRT0_OVRCURRCHG, > + hsotg->regs + HPRT0); > dev_vdbg(hsotg->dev, > " --Port Interrupt HPRT0=0x%08x Port Overcurrent Changed--\n", > hprt0); > hsotg->flags.b.port_over_current_change = 1; > - hprt0_modify |= HPRT0_OVRCURRCHG; > } > - > - /* Clear Port Interrupts */ > - dwc2_writel(hprt0_modify, hsotg->regs + HPRT0); > } > > /* > Acked-by: John Youn Tested-by: John Youn Tested on IP v3.20 on HAPS PCI system. Regards, 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/