[PATCH v4 2/2] usb: dwc2: host: Clear interrupts before handling them
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
Acked-by: John Youn
Tested-by: John Youn
---
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).
Changes in v2: None
drivers/usb/dwc2/core_intr.c | 49 +---
drivers/usb/dwc2/hcd_intr.c | 17 ---
2 files changed, 31 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)
/* Chan
Re: [PATCH v4 2/2] usb: dwc2: host: Clear interrupts before handling them
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. > > 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 > Acked-by: John Youn > Tested-by: John Youn $ 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 ? -- balbi signature.asc Description: PGP signature
Re: [PATCH v4 2/2] usb: dwc2: host: Clear interrupts before handling them
Felipe, On Fri, Nov 20, 2015 at 7:40 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. >> >> 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 >> Acked-by: John Youn >> Tested-by: John Youn > > $ 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 2/2] usb: dwc2: host: Clear interrupts before handling them
Hi, On Fri, Nov 20, 2015 at 8:49 AM, Doug Anderson wrote: > Felipe, > > On Fri, Nov 20, 2015 at 7:40 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. >>> >>> 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 >>> Acked-by: John Youn >>> Tested-by: John Youn >> >> $ 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
