Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1261875 > unrolled thread

[PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions

Started byDouglas Anderson <dianders@chromium.org>
First post2015-11-03 21:40 +0100
Last post2015-11-05 04:00 +0100
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions Douglas Anderson <dianders@chromium.org> - 2015-11-03 21:40 +0100
    [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them Douglas Anderson <dianders@chromium.org> - 2015-11-03 21:40 +0100
      Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before  handling them John Youn <John.Youn@synopsys.com> - 2015-11-05 04:10 +0100
    Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions John Youn <John.Youn@synopsys.com> - 2015-11-05 04:00 +0100

#1261875 — [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions

FromDouglas Anderson <dianders@chromium.org>
Date2015-11-03 21:40 +0100
Subject[PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions
Message-ID<qqQx4-7Ef-7@gated-at.bofh.it>
If you've got your interrupt signals bouncing a bit as you insert your
USB device, you might end up in a state when the device is connected but
the driver doesn't know it.

Specifically, the observed order is:
 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()

Now you'll be stuck with the cable plugged in and no further interrupts
coming in but the driver will think we're disconnected.

We'll fix this by checking for the missing connect interrupt and
re-connecting after the disconnect is posted.  We don't skip the
disconnect because if there is a transitory disconnect we really want to
de-enumerate and re-enumerate.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v2:
- Don't reconnect when called from _dwc2_hcd_stop() (John Youn)

 drivers/usb/dwc2/core.h      |  6 ++++--
 drivers/usb/dwc2/core_intr.c |  4 ++--
 drivers/usb/dwc2/hcd.c       | 40 ++++++++++++++++++++++++++++++++++++++--
 drivers/usb/dwc2/hcd_intr.c  |  6 +-----
 4 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index a66d3cb62b65..daa1c342cdac 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -1154,12 +1154,14 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg,
 
 #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
 extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg);
-extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg);
+extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
+extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
 extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
 #else
 static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
 { return 0; }
-static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {}
+static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
+static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
 static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
 static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
 static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 27daa42788b1..61601d16e233 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
 			dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n",
 				hsotg->op_state);
 			spin_unlock(&hsotg->lock);
-			dwc2_hcd_disconnect(hsotg);
+			dwc2_hcd_disconnect(hsotg, false);
 			spin_lock(&hsotg->lock);
 			hsotg->op_state = OTG_STATE_A_PERIPHERAL;
 		} else {
@@ -401,7 +401,7 @@ static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
 		dwc2_op_state_str(hsotg));
 
 	if (hsotg->op_state == OTG_STATE_A_HOST)
-		dwc2_hcd_disconnect(hsotg);
+		dwc2_hcd_disconnect(hsotg, false);
 
 	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
 }
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index e79baf73c234..e208eac7ff57 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -268,15 +268,33 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
 }
 
 /**
+ * dwc2_hcd_connect() - Handles connect of the HCD
+ *
+ * @hsotg: Pointer to struct dwc2_hsotg
+ *
+ * Must be called with interrupt disabled and spinlock held
+ */
+void dwc2_hcd_connect(struct dwc2_hsotg *hsotg)
+{
+	if (hsotg->lx_state != DWC2_L0)
+		usb_hcd_resume_root_hub(hsotg->priv);
+
+	hsotg->flags.b.port_connect_status_change = 1;
+	hsotg->flags.b.port_connect_status = 1;
+}
+
+/**
  * dwc2_hcd_disconnect() - Handles disconnect of the HCD
  *
  * @hsotg: Pointer to struct dwc2_hsotg
+ * @force: If true, we won't try to reconnect even if we see device connected.
  *
  * Must be called with interrupt disabled and spinlock held
  */
-void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
+void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force)
 {
 	u32 intr;
+	u32 hprt0;
 
 	/* Set status flags for the hub driver */
 	hsotg->flags.b.port_connect_status_change = 1;
@@ -315,6 +333,24 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
 		dwc2_hcd_cleanup_channels(hsotg);
 
 	dwc2_host_disconnect(hsotg);
+
+	/*
+	 * Add an extra check here to see if we're actually connected but
+	 * we don't have a detection interrupt pending.  This can happen if:
+	 *   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 here
+	 *
+	 * Without the extra check here we will end calling disconnect
+	 * and won't get any future interrupts to handle the connect.
+	 */
+	if (!force) {
+		hprt0 = dwc2_readl(hsotg->regs + HPRT0);
+		if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS))
+			dwc2_hcd_connect(hsotg);
+	}
 }
 
 /**
@@ -2365,7 +2401,7 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 	/* Ensure hcd is disconnected */
-	dwc2_hcd_disconnect(hsotg);
+	dwc2_hcd_disconnect(hsotg, true);
 	dwc2_hcd_stop(hsotg);
 	hsotg->lx_state = DWC2_L3;
 	hcd->state = HC_STATE_HALT;
diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
index bda0b21b850f..03504ac2fecc 100644
--- a/drivers/usb/dwc2/hcd_intr.c
+++ b/drivers/usb/dwc2/hcd_intr.c
@@ -350,11 +350,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
 		dev_vdbg(hsotg->dev,
 			 "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
 			 hprt0);
-		if (hsotg->lx_state != DWC2_L0)
-			usb_hcd_resume_root_hub(hsotg->priv);
-
-		hsotg->flags.b.port_connect_status_change = 1;
-		hsotg->flags.b.port_connect_status = 1;
+		dwc2_hcd_connect(hsotg);
 		hprt0_modify |= HPRT0_CONNDET;
 
 		/*
-- 
2.6.0.rc2.230.g3dd15c0

--
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/

[toc] | [next] | [standalone]

#1261881 — [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them

FromDouglas Anderson <dianders@chromium.org>
Date2015-11-03 21:40 +0100
Subject[PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them
Message-ID<qqQx4-7Ef-21@gated-at.bofh.it>
In reply to#1261875
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 <dianders@chromium.org>
---
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);
 }
 
 /**
@@ -98,11 +99,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 +277,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 +300,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 +317,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 +349,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 +374,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 +390,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 +398,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 +421,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 +442,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 +481,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..4c7f709b8182 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) {
+		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) {
+		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,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
 
 	/* Overcurrent Change Interrupt */
 	if (hprt0 & HPRT0_OVRCURRCHG) {
+		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);
 }
 
 /*
-- 
2.6.0.rc2.230.g3dd15c0

--
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/

[toc] | [prev] | [next] | [standalone]

#1262859 — Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them

FromJohn Youn <John.Youn@synopsys.com>
Date2015-11-05 04:10 +0100
SubjectRe: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them
Message-ID<qrj62-Vz-9@gated-at.bofh.it>
In reply to#1261881
On 11/3/2015 12:31 PM, 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.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 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);
>  }
>  
>  /**
> @@ -98,11 +99,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 +277,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 +300,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 +317,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 +349,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 +374,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 +390,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 +398,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 +421,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 +442,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 +481,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..4c7f709b8182 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) {
> +		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) {
> +		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,12 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>  
>  	/* Overcurrent Change Interrupt */
>  	if (hprt0 & HPRT0_OVRCURRCHG) {
> +		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 <johnyoun@synopsys.com>
Tested-by: John Youn <johnyoun@synopsys.com>

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/

[toc] | [prev] | [next] | [standalone]

#1262854

FromJohn Youn <John.Youn@synopsys.com>
Date2015-11-05 04:00 +0100
Message-ID<qriWl-BZ-9@gated-at.bofh.it>
In reply to#1261875
On 11/3/2015 12:31 PM, Douglas Anderson wrote:
> If you've got your interrupt signals bouncing a bit as you insert your
> USB device, you might end up in a state when the device is connected but
> the driver doesn't know it.
> 
> Specifically, the observed order is:
>  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()
> 
> Now you'll be stuck with the cable plugged in and no further interrupts
> coming in but the driver will think we're disconnected.
> 
> We'll fix this by checking for the missing connect interrupt and
> re-connecting after the disconnect is posted.  We don't skip the
> disconnect because if there is a transitory disconnect we really want to
> de-enumerate and re-enumerate.
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Don't reconnect when called from _dwc2_hcd_stop() (John Youn)
> 
>  drivers/usb/dwc2/core.h      |  6 ++++--
>  drivers/usb/dwc2/core_intr.c |  4 ++--
>  drivers/usb/dwc2/hcd.c       | 40 ++++++++++++++++++++++++++++++++++++++--
>  drivers/usb/dwc2/hcd_intr.c  |  6 +-----
>  4 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index a66d3cb62b65..daa1c342cdac 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -1154,12 +1154,14 @@ static inline int dwc2_hsotg_set_test_mode(struct dwc2_hsotg *hsotg,
>  
>  #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
>  extern int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg);
> -extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg);
> +extern void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
> +extern void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
>  extern void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
>  #else
>  static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
>  { return 0; }
> -static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) {}
> +static inline void dwc2_hcd_connect(struct dwc2_hsotg *hsotg) {}
> +static inline void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force) {}
>  static inline void dwc2_hcd_start(struct dwc2_hsotg *hsotg) {}
>  static inline void dwc2_hcd_remove(struct dwc2_hsotg *hsotg) {}
>  static inline int dwc2_hcd_init(struct dwc2_hsotg *hsotg, int irq)
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 27daa42788b1..61601d16e233 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -239,7 +239,7 @@ static void dwc2_handle_otg_intr(struct dwc2_hsotg *hsotg)
>  			dev_dbg(hsotg->dev, "a_suspend->a_peripheral (%d)\n",
>  				hsotg->op_state);
>  			spin_unlock(&hsotg->lock);
> -			dwc2_hcd_disconnect(hsotg);
> +			dwc2_hcd_disconnect(hsotg, false);
>  			spin_lock(&hsotg->lock);
>  			hsotg->op_state = OTG_STATE_A_PERIPHERAL;
>  		} else {
> @@ -401,7 +401,7 @@ static void dwc2_handle_disconnect_intr(struct dwc2_hsotg *hsotg)
>  		dwc2_op_state_str(hsotg));
>  
>  	if (hsotg->op_state == OTG_STATE_A_HOST)
> -		dwc2_hcd_disconnect(hsotg);
> +		dwc2_hcd_disconnect(hsotg, false);
>  
>  	dwc2_writel(GINTSTS_DISCONNINT, hsotg->regs + GINTSTS);
>  }
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index e79baf73c234..e208eac7ff57 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -268,15 +268,33 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg)
>  }
>  
>  /**
> + * dwc2_hcd_connect() - Handles connect of the HCD
> + *
> + * @hsotg: Pointer to struct dwc2_hsotg
> + *
> + * Must be called with interrupt disabled and spinlock held
> + */
> +void dwc2_hcd_connect(struct dwc2_hsotg *hsotg)
> +{
> +	if (hsotg->lx_state != DWC2_L0)
> +		usb_hcd_resume_root_hub(hsotg->priv);
> +
> +	hsotg->flags.b.port_connect_status_change = 1;
> +	hsotg->flags.b.port_connect_status = 1;
> +}
> +
> +/**
>   * dwc2_hcd_disconnect() - Handles disconnect of the HCD
>   *
>   * @hsotg: Pointer to struct dwc2_hsotg
> + * @force: If true, we won't try to reconnect even if we see device connected.
>   *
>   * Must be called with interrupt disabled and spinlock held
>   */
> -void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
> +void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force)
>  {
>  	u32 intr;
> +	u32 hprt0;
>  
>  	/* Set status flags for the hub driver */
>  	hsotg->flags.b.port_connect_status_change = 1;
> @@ -315,6 +333,24 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg)
>  		dwc2_hcd_cleanup_channels(hsotg);
>  
>  	dwc2_host_disconnect(hsotg);
> +
> +	/*
> +	 * Add an extra check here to see if we're actually connected but
> +	 * we don't have a detection interrupt pending.  This can happen if:
> +	 *   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 here
> +	 *
> +	 * Without the extra check here we will end calling disconnect
> +	 * and won't get any future interrupts to handle the connect.
> +	 */
> +	if (!force) {
> +		hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> +		if (!(hprt0 & HPRT0_CONNDET) && (hprt0 & HPRT0_CONNSTS))
> +			dwc2_hcd_connect(hsotg);
> +	}
>  }
>  
>  /**
> @@ -2365,7 +2401,7 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
>  
>  	spin_lock_irqsave(&hsotg->lock, flags);
>  	/* Ensure hcd is disconnected */
> -	dwc2_hcd_disconnect(hsotg);
> +	dwc2_hcd_disconnect(hsotg, true);
>  	dwc2_hcd_stop(hsotg);
>  	hsotg->lx_state = DWC2_L3;
>  	hcd->state = HC_STATE_HALT;
> diff --git a/drivers/usb/dwc2/hcd_intr.c b/drivers/usb/dwc2/hcd_intr.c
> index bda0b21b850f..03504ac2fecc 100644
> --- a/drivers/usb/dwc2/hcd_intr.c
> +++ b/drivers/usb/dwc2/hcd_intr.c
> @@ -350,11 +350,7 @@ static void dwc2_port_intr(struct dwc2_hsotg *hsotg)
>  		dev_vdbg(hsotg->dev,
>  			 "--Port Interrupt HPRT0=0x%08x Port Connect Detected--\n",
>  			 hprt0);
> -		if (hsotg->lx_state != DWC2_L0)
> -			usb_hcd_resume_root_hub(hsotg->priv);
> -
> -		hsotg->flags.b.port_connect_status_change = 1;
> -		hsotg->flags.b.port_connect_status = 1;
> +		dwc2_hcd_connect(hsotg);
>  		hprt0_modify |= HPRT0_CONNDET;
>  
>  		/*
> 


Acked-by: John Youn <johnyoun@synopsys.com>
Tested-by: John Youn <johnyoun@synopsys.com>

Tested on Synopsys HAPS platform IP v3.20a.

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/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web