| Message ID | 1447968195-32097-2-git-send-email-dianders@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, Douglas Anderson <dianders@chromium.org> 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. > > 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 <dianders@chromium.org> > Acked-by: John Youn <johnyoun@synopsys.com> > Tested-by: John Youn <johnyoun@synopsys.com> $ patch -p1 --dry-run < patch.diff checking file drivers/usb/dwc2/core_intr.c checking file drivers/usb/dwc2/hcd_intr.c Hunk #4 FAILED at 365. Hunk #5 succeeded at 388 (offset 11 lines). 1 out of 5 hunks FAILED Care to rebase on top of my testing/next ?
Felipe, On Fri, Nov 20, 2015 at 7:40 AM, Felipe Balbi <balbi@ti.com> wrote: > > Hi, > > Douglas Anderson <dianders@chromium.org> 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. >> >> 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 <dianders@chromium.org> >> Acked-by: John Youn <johnyoun@synopsys.com> >> Tested-by: John Youn <johnyoun@synopsys.com> > > $ patch -p1 --dry-run < patch.diff > checking file drivers/usb/dwc2/core_intr.c > checking file drivers/usb/dwc2/hcd_intr.c > Hunk #4 FAILED at 365. > Hunk #5 succeeded at 388 (offset 11 lines). > 1 out of 5 hunks FAILED > > Care to rebase on top of my testing/next ? No problem. ...though when I did that, I actually found (by code inspection) a bug in the original patch, so I guess it's a good thing it didn't apply... ...and then that led me to another bug that was pre-existing. I'll send up two new patches shortly. I'll remove John's Ack and Tested tags from the patch since it contains a change. It looks like you already landed part 1 of this series (usb: dwc2: host: Fix missing device insertions) so I won't resend that. -Doug
Hi, On Fri, Nov 20, 2015 at 8:49 AM, Doug Anderson <dianders@chromium.org> wrote: > Felipe, > > On Fri, Nov 20, 2015 at 7:40 AM, Felipe Balbi <balbi@ti.com> wrote: >> >> Hi, >> >> Douglas Anderson <dianders@chromium.org> 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. >>> >>> 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 <dianders@chromium.org> >>> Acked-by: John Youn <johnyoun@synopsys.com> >>> Tested-by: John Youn <johnyoun@synopsys.com> >> >> $ patch -p1 --dry-run < patch.diff >> checking file drivers/usb/dwc2/core_intr.c >> checking file drivers/usb/dwc2/hcd_intr.c >> Hunk #4 FAILED at 365. >> Hunk #5 succeeded at 388 (offset 11 lines). >> 1 out of 5 hunks FAILED >> >> Care to rebase on top of my testing/next ? > > No problem. > > ...though when I did that, I actually found (by code inspection) a bug > in the original patch, so I guess it's a good thing it didn't apply... > ...and then that led me to another bug that was pre-existing. I'll > send up two new patches shortly. I'll remove John's Ack and Tested > tags from the patch since it contains a change. > > It looks like you already landed part 1 of this series (usb: dwc2: > host: Fix missing device insertions) so I won't resend that. Ah, OK. Now I see why nobody found these problems in testing. The code path is completely disabled by all current parameters as checked in to mainline. In any case, it's good to fix. -Doug
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 03504ac2fecc..bc3452355cfd 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); } /* @@ -347,11 +347,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 +365,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) dwc2_hprt0_enable(hsotg, hprt0, &hprt0_modify); else @@ -376,15 +377,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); } /*