* [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-03 20:30 Douglas Anderson 2015-11-03 20:30 ` Douglas Anderson ` (2 more replies) 0 siblings, 3 replies; 39+ messages in thread From: Douglas Anderson @ 2015-11-03 20:30 UTC (permalink / raw) To: John Youn, balbi Cc: Yunzhi Li, Heiko Stübner, linux-rockchip, Julius Werner, gregory.herrero, yousaf.kaukab, dinguyen, Douglas Anderson, johnyoun, gregkh, linux-usb, linux-kernel 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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-03 20:30 ` Douglas Anderson 0 siblings, 0 replies; 39+ messages in thread From: Douglas Anderson @ 2015-11-03 20:30 UTC (permalink / raw) To: John Youn, balbi Cc: Yunzhi Li, Heiko Stübner, linux-rockchip, Julius Werner, gregory.herrero, yousaf.kaukab, dinguyen, Douglas Anderson, johnyoun, gregkh, linux-usb, linux-kernel 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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-03 20:30 ` Douglas Anderson 0 siblings, 0 replies; 39+ messages in thread From: Douglas Anderson @ 2015-11-03 20:30 UTC (permalink / raw) To: John Youn, balbi-l0cyMroinI0 Cc: gregory.herrero-ral2JQCrhuEAvxtiuMwx3w, Heiko Stübner, johnyoun-HKixBCOQz3hWk0Htik3J/w, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, linux-usb-u79uwXL29TY76Z2rM5mHXA, Douglas Anderson, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, yousaf.kaukab-ral2JQCrhuEAvxtiuMwx3w, Yunzhi Li, Julius Werner, dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx 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-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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 ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them 2015-11-03 20:30 ` Douglas Anderson (?) @ 2015-11-05 3:08 ` John Youn -1 siblings, 0 replies; 39+ messages in thread From: John Youn @ 2015-11-05 3:08 UTC (permalink / raw) To: Douglas Anderson, John Youn, balbi Cc: Yunzhi Li, Heiko Stübner, linux-rockchip, Julius Werner, gregory.herrero, yousaf.kaukab, dinguyen, gregkh, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-16 16:28 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-16 16:28 UTC (permalink / raw) To: Douglas Anderson, John Youn Cc: Yunzhi Li, Heiko Stübner, linux-rockchip, Julius Werner, gregory.herrero, yousaf.kaukab, dinguyen, Douglas Anderson, johnyoun, gregkh, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 2898 bytes --] 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. > > 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); 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); Also, these two patches look very large for the -rc. I'll move them to v4.5 merge window unless you can convince me that they are, indeed, the smallest change possible to fix the problem you're facing and that they are, indeed, regressions. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-16 16:28 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-16 16:28 UTC (permalink / raw) To: John Youn Cc: gregory.herrero-ral2JQCrhuEAvxtiuMwx3w, Heiko Stübner, johnyoun-HKixBCOQz3hWk0Htik3J/w, gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r, linux-usb-u79uwXL29TY76Z2rM5mHXA, Douglas Anderson, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, yousaf.kaukab-ral2JQCrhuEAvxtiuMwx3w, Yunzhi Li, Julius Werner, dinguyen-yzvPICuk2ABMcg4IHK0kFoH6Mc4MB0Vx [-- Attachment #1.1: Type: text/plain, Size: 2952 bytes --] Hi, Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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. > > Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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); 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); Also, these two patches look very large for the -rc. I'll move them to v4.5 merge window unless you can convince me that they are, indeed, the smallest change possible to fix the problem you're facing and that they are, indeed, regressions. -- balbi [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] [-- Attachment #2: Type: text/plain, Size: 200 bytes --] _______________________________________________ Linux-rockchip mailing list Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-16 17:22 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-16 17:22 UTC (permalink / raw) To: Felipe Balbi Cc: John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel Felipe, On Mon, Nov 16, 2015 at 8:28 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. >> >> 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); > > 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. ...if it wasn't useless to clear PRTINT it would depend on how things were implemented. One (purely speculative) case where my patch would be more correct: 1. Interrupt source 1 in HPRT fires. PRTINT is latched to 1 and interrupt handler is called. 2. Read HPRT 3. Interrupt source 2 in HPRT fires. PRTINT is already 1, so no new interrupt handler called. 4. Clear PRTINT 5. Handle interrupt source 1 ...now we'll never get an interrupt for interrupt source 2. In any case, I think this particular change is a no-op, but other changes in this patch (probably) aren't. > Also, these two patches look very large for the -rc. I'll move them to > v4.5 merge window unless you can convince me that they are, indeed, the > smallest change possible to fix the problem you're facing and that they > are, indeed, regressions. Patch #2 should definitely _not_ go into the RC. It is a relatively intrusive patch, fixes no known issues, and has only had light testing. I haven't even landed this change locally in ChromeOS since it's a bit scary to me I haven't found any good use for it. I do keep testing with it to see if it fixes any of my issues, though. ;) I'd like to see patch #1 should go into the RC since it fixes problems that are really easy to reproduce. IMHO It's also neither big nor scary. FWIW it's already landed in the ChromeOS tree as of Oct 20th (almost a month). While it hasn't hit stable channel yet, it's been on beta channel and nobody has discovered any issues with it yet. Thanks! :) -Doug ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-16 17:22 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-16 17:22 UTC (permalink / raw) To: Felipe Balbi Cc: John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA Felipe, On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote: > > Hi, > > Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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. >> >> Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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); > > 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. ...if it wasn't useless to clear PRTINT it would depend on how things were implemented. One (purely speculative) case where my patch would be more correct: 1. Interrupt source 1 in HPRT fires. PRTINT is latched to 1 and interrupt handler is called. 2. Read HPRT 3. Interrupt source 2 in HPRT fires. PRTINT is already 1, so no new interrupt handler called. 4. Clear PRTINT 5. Handle interrupt source 1 ...now we'll never get an interrupt for interrupt source 2. In any case, I think this particular change is a no-op, but other changes in this patch (probably) aren't. > Also, these two patches look very large for the -rc. I'll move them to > v4.5 merge window unless you can convince me that they are, indeed, the > smallest change possible to fix the problem you're facing and that they > are, indeed, regressions. Patch #2 should definitely _not_ go into the RC. It is a relatively intrusive patch, fixes no known issues, and has only had light testing. I haven't even landed this change locally in ChromeOS since it's a bit scary to me I haven't found any good use for it. I do keep testing with it to see if it fixes any of my issues, though. ;) I'd like to see patch #1 should go into the RC since it fixes problems that are really easy to reproduce. IMHO It's also neither big nor scary. FWIW it's already landed in the ChromeOS tree as of Oct 20th (almost a month). While it hasn't hit stable channel yet, it's been on beta channel and nobody has discovered any issues with it yet. Thanks! :) -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them 2015-11-16 17:22 ` Doug Anderson @ 2015-11-19 1:43 ` John Youn -1 siblings, 0 replies; 39+ messages in thread From: John Youn @ 2015-11-19 1:43 UTC (permalink / raw) To: Doug Anderson, Felipe Balbi Cc: John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, Greg Kroah-Hartman, linux-usb, linux-kernel On 11/16/2015 9:22 AM, Doug Anderson wrote: > Felipe, > > On Mon, Nov 16, 2015 at 8:28 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. >>> >>> 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); >> >> 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. John ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-19 1:43 ` John Youn 0 siblings, 0 replies; 39+ messages in thread From: John Youn @ 2015-11-19 1:43 UTC (permalink / raw) To: Doug Anderson, Felipe Balbi Cc: Herrero, Gregory, Heiko Stübner, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, John Youn, open list:ARM/Rockchip SoC..., Kaukab, Yousaf, Yunzhi Li, Julius Werner, Dinh Nguyen On 11/16/2015 9:22 AM, Doug Anderson wrote: > Felipe, > > On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote: >> >> Hi, >> >> Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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. >>> >>> Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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); >> >> 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. John ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-19 16:37 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-19 16:37 UTC (permalink / raw) To: John Youn Cc: Felipe Balbi, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, Greg Kroah-Hartman, linux-usb, linux-kernel Hi, On Wed, Nov 18, 2015 at 5:43 PM, John Youn <John.Youn@synopsys.com> wrote: > On 11/16/2015 9:22 AM, Doug Anderson wrote: >> Felipe, >> >> On Mon, Nov 16, 2015 at 8:28 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. >>>> >>>> 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); >>> >>> 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-19 16:37 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-19 16:37 UTC (permalink / raw) To: John Youn Cc: Felipe Balbi, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA Hi, On Wed, Nov 18, 2015 at 5:43 PM, John Youn <John.Youn-HKixBCOQz3hWk0Htik3J/w@public.gmane.org> wrote: > On 11/16/2015 9:22 AM, Doug Anderson wrote: >> Felipe, >> >> On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote: >>> >>> Hi, >>> >>> Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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. >>>> >>>> Signed-off-by: Douglas Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.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); >>> >>> 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-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-19 18:19 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-19 18:19 UTC (permalink / raw) To: Doug Anderson, John Youn Cc: Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, Greg Kroah-Hartman, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 1301 bytes --] Hi, Doug Anderson <dianders@chromium.org> writes: >>>> 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? I'll leave the final decision to John, but my opinion is that a new version of the patch would be preferrable. -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them @ 2015-11-19 18:19 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-19 18:19 UTC (permalink / raw) To: Doug Anderson, John Youn Cc: Herrero, Gregory, Heiko Stübner, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, open list:ARM/Rockchip SoC..., Kaukab, Yousaf, Yunzhi Li, Julius Werner, Dinh Nguyen [-- Attachment #1.1: Type: text/plain, Size: 1328 bytes --] Hi, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> writes: >>>> 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? I'll leave the final decision to John, but my opinion is that a new version of the patch would be preferrable. -- balbi [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] [-- Attachment #2: Type: text/plain, Size: 200 bytes --] _______________________________________________ Linux-rockchip mailing list Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them 2015-11-19 18:19 ` Felipe Balbi (?) @ 2015-11-19 19:09 ` John Youn -1 siblings, 0 replies; 39+ messages in thread From: John Youn @ 2015-11-19 19:09 UTC (permalink / raw) To: Felipe Balbi, Doug Anderson, John Youn Cc: Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, Greg Kroah-Hartman, linux-usb, linux-kernel On 11/19/2015 10:19 AM, Felipe Balbi wrote: > > Hi, > > Doug Anderson <dianders@chromium.org> writes: >>>>> 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? > > I'll leave the final decision to John, but my opinion is that a new > version of the patch would be preferrable. > Hi Doug, Could you resend with the change? Regards, John ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions 2015-11-03 20:30 [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions Douglas Anderson 2015-11-03 20:30 ` Douglas Anderson @ 2015-11-05 2:52 ` John Youn 2015-11-16 17:44 ` Felipe Balbi 2 siblings, 0 replies; 39+ messages in thread From: John Youn @ 2015-11-05 2:52 UTC (permalink / raw) To: Douglas Anderson, John Youn, balbi Cc: Yunzhi Li, Heiko Stübner, linux-rockchip, Julius Werner, gregory.herrero, yousaf.kaukab, dinguyen, gregkh, linux-usb, linux-kernel 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 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions 2015-11-03 20:30 [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions Douglas Anderson @ 2015-11-16 17:44 ` Felipe Balbi 2015-11-05 2:52 ` [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions John Youn 2015-11-16 17:44 ` Felipe Balbi 2 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-16 17:44 UTC (permalink / raw) To: Douglas Anderson, John Youn Cc: Yunzhi Li, Heiko Stübner, linux-rockchip, Julius Werner, gregory.herrero, yousaf.kaukab, dinguyen, Douglas Anderson, johnyoun, gregkh, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 8061 bytes --] Hi, (replying here for the context of why I think this is NOT the smallest patch possible for the -rc) Douglas Anderson <dianders@chromium.org> writes: > 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); this 'force' parameter is new and looks unnecessary for the -rc. > 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); because force is unnecessary (it seems to be just an optimization to me), this change is unnecessary. > 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); and so is this. > 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; > +} you make no mention of why this is needed. This is basically a refactor, not a fix. > +/** > * 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) as mentioned before, unnecessary 'force' parameter. > @@ -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); what's inside this 'force' branch is what you really need to fix the problem, right ? It seems like for the -rc cycle, all you need is: diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 571c21727ff9..967d1e686efe 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) */ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) { + u32 hprt0; u32 intr; /* Set status flags for the hub driver */ @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) dwc2_hcd_cleanup_channels(hsotg); dwc2_host_disconnect(hsotg); + + hprt0 = dwc2_readl(hsotg->regs + HPRT0); + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) { + if (hsotg->lx_state != DWC2_L0) + usb_hcd_resume_roothub(hsotg->priv); + + hsotg->flags.b.port_connect_status_change = 1; + hsotg->flags.b.port_connect_status = 1; + } } /** This has some code duplication and it'll *always* check for CONNDET | CONNSTS, but that's okay for the -rc. Follow-up patches (for v4.5 merge window) can refactor the code duplication and introduce 'force' parameter. > @@ -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); unnecessary change. > 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); unnecessary change. Do you agree or do you think 'force' is really necessary for this fix ? -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 17:44 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-16 17:44 UTC (permalink / raw) To: John Youn Cc: Yunzhi Li, Heiko Stübner, linux-rockchip, Julius Werner, gregory.herrero, yousaf.kaukab, dinguyen, Douglas Anderson, johnyoun, gregkh, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 8061 bytes --] Hi, (replying here for the context of why I think this is NOT the smallest patch possible for the -rc) Douglas Anderson <dianders@chromium.org> writes: > 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); this 'force' parameter is new and looks unnecessary for the -rc. > 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); because force is unnecessary (it seems to be just an optimization to me), this change is unnecessary. > 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); and so is this. > 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; > +} you make no mention of why this is needed. This is basically a refactor, not a fix. > +/** > * 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) as mentioned before, unnecessary 'force' parameter. > @@ -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); what's inside this 'force' branch is what you really need to fix the problem, right ? It seems like for the -rc cycle, all you need is: diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c index 571c21727ff9..967d1e686efe 100644 --- a/drivers/usb/dwc2/hcd.c +++ b/drivers/usb/dwc2/hcd.c @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) */ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) { + u32 hprt0; u32 intr; /* Set status flags for the hub driver */ @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) dwc2_hcd_cleanup_channels(hsotg); dwc2_host_disconnect(hsotg); + + hprt0 = dwc2_readl(hsotg->regs + HPRT0); + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) { + if (hsotg->lx_state != DWC2_L0) + usb_hcd_resume_roothub(hsotg->priv); + + hsotg->flags.b.port_connect_status_change = 1; + hsotg->flags.b.port_connect_status = 1; + } } /** This has some code duplication and it'll *always* check for CONNDET | CONNSTS, but that's okay for the -rc. Follow-up patches (for v4.5 merge window) can refactor the code duplication and introduce 'force' parameter. > @@ -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); unnecessary change. > 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); unnecessary change. Do you agree or do you think 'force' is really necessary for this fix ? -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply related [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions 2015-11-16 17:44 ` Felipe Balbi (?) @ 2015-11-16 18:13 ` Doug Anderson 2015-11-16 18:22 ` Felipe Balbi -1 siblings, 1 reply; 39+ messages in thread From: Doug Anderson @ 2015-11-16 18:13 UTC (permalink / raw) To: Felipe Balbi Cc: John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel Felipe, On Mon, Nov 16, 2015 at 9:44 AM, Felipe Balbi <balbi@ti.com> wrote: >> 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); > > because force is unnecessary (it seems to be just an optimization to > me), this change is unnecessary. I added "force" in v2 of the patch in response to John's feedback to v1. He pointed out that when you unload the module when you have a device connected that my v1 patch would not properly disconnect the device (or, rather, it would disconnect it and then reconnect it). That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force of "true". >> /** >> + * 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; >> +} > > you make no mention of why this is needed. This is basically a refactor, > not a fix. This new function is called from two places now, isn't it? Basically this is a little bit of code that used to be directly in dwc2_port_intr(). I still want it there, but I also want to call the same bit of code after a disconnect if I detect that the device has been inserted again. >> @@ -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); > > what's inside this 'force' branch is what you really need to fix the > problem, right ? It seems like for the -rc cycle, all you need is: > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c > index 571c21727ff9..967d1e686efe 100644 > --- a/drivers/usb/dwc2/hcd.c > +++ b/drivers/usb/dwc2/hcd.c > @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) > */ > void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) > { > + u32 hprt0; > u32 intr; > > /* Set status flags for the hub driver */ > @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) > dwc2_hcd_cleanup_channels(hsotg); > > dwc2_host_disconnect(hsotg); > + > + hprt0 = dwc2_readl(hsotg->regs + HPRT0); > + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) { > + if (hsotg->lx_state != DWC2_L0) > + usb_hcd_resume_roothub(hsotg->priv); > + > + hsotg->flags.b.port_connect_status_change = 1; > + hsotg->flags.b.port_connect_status = 1; > + } I'd really rather not add the duplication unless you insist. To me it makes it clearer to include the (small) refactor in the same patch. If the refactor makes this change too big for an RC, then it's OK with me to just skip this for the RC. It's not fixing a regression or anything. I have no requirements to have this land in 4.4. It fixes a bug and I thought that the fix was pretty small and safe (despite having a diffstat that's bigger than the bare minimum). I will leave it to your judgement. >> 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); > > unnecessary change. > > Do you agree or do you think 'force' is really necessary for this fix ? As per above, John thought it was a good idea to really make the disconnect happen upon module unload. If you disagree then we could probably just totally remove the "dwc2_hcd_disconnect(hsotg);" from the _dwc2_hcd_stop() function. -Doug ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 18:22 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-16 18:22 UTC (permalink / raw) To: Doug Anderson Cc: John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 7470 bytes --] Hi, Doug Anderson <dianders@chromium.org> writes: > Felipe, > > On Mon, Nov 16, 2015 at 9:44 AM, Felipe Balbi <balbi@ti.com> wrote: >>> 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); >> >> because force is unnecessary (it seems to be just an optimization to >> me), this change is unnecessary. > > I added "force" in v2 of the patch in response to John's feedback to > v1. He pointed out that when you unload the module when you have a > device connected that my v1 patch would not properly disconnect the > device (or, rather, it would disconnect it and then reconnect it). > > That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force > of "true". There's no mention of this in commit log. It would be great to add a: "while at that, also make sure that we don't try to detect a device on module unload because of foo bar baz as pointed out by John Youn". Or something along these lines. >>> /** >>> + * 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; >>> +} >> >> you make no mention of why this is needed. This is basically a refactor, >> not a fix. > > This new function is called from two places now, isn't it? > > Basically this is a little bit of code that used to be directly in > dwc2_port_intr(). I still want it there, but I also want to call the > same bit of code after a disconnect if I detect that the device has > been inserted again. I got that :-) But it's not mentioned in commit and it's apparently unnecessary for fixing the bug :-) Another "we're also adding a new hsotg_disconnect() function by means of refactoring to avoid code duplication" would've been enough. >>> @@ -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); >> >> what's inside this 'force' branch is what you really need to fix the >> problem, right ? It seems like for the -rc cycle, all you need is: >> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >> index 571c21727ff9..967d1e686efe 100644 >> --- a/drivers/usb/dwc2/hcd.c >> +++ b/drivers/usb/dwc2/hcd.c >> @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) >> */ >> void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) >> { >> + u32 hprt0; >> u32 intr; >> >> /* Set status flags for the hub driver */ >> @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) >> dwc2_hcd_cleanup_channels(hsotg); >> >> dwc2_host_disconnect(hsotg); >> + >> + hprt0 = dwc2_readl(hsotg->regs + HPRT0); >> + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) { >> + if (hsotg->lx_state != DWC2_L0) >> + usb_hcd_resume_roothub(hsotg->priv); >> + >> + hsotg->flags.b.port_connect_status_change = 1; >> + hsotg->flags.b.port_connect_status = 1; >> + } > > I'd really rather not add the duplication unless you insist. To me it > makes it clearer to include the (small) refactor in the same patch. > > If the refactor makes this change too big for an RC, then it's OK with > me to just skip this for the RC. It's not fixing a regression or > anything. I have no requirements to have this land in 4.4. It fixes > a bug and I thought that the fix was pretty small and safe (despite > having a diffstat that's bigger than the bare minimum). I will leave > it to your judgement. let's at least modify commit log to make all these extra changes clear that they are needed because of reason (a) or (b) or whatever. If you just send a patch doing much more than it apparently should without no mention as to why it was done this way, I can't know for sure those changes are needed; next thing you know, Greg refuses to apply my pull request because the change is too large :-) We don't want that to happen :-) >>> 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); >> >> unnecessary change. >> >> Do you agree or do you think 'force' is really necessary for this fix ? > > As per above, John thought it was a good idea to really make the > disconnect happen upon module unload. If you disagree then we could > probably just totally remove the "dwc2_hcd_disconnect(hsotg);" from > the _dwc2_hcd_stop() function. see above. thanks -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 18:22 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-16 18:22 UTC (permalink / raw) To: Doug Anderson Cc: Herrero, Gregory, Heiko Stübner, John Youn, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, John Youn, open list:ARM/Rockchip SoC..., Kaukab, Yousaf, Yunzhi Li, Julius Werner, Dinh Nguyen [-- Attachment #1.1: Type: text/plain, Size: 7519 bytes --] Hi, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> writes: > Felipe, > > On Mon, Nov 16, 2015 at 9:44 AM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote: >>> 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); >> >> because force is unnecessary (it seems to be just an optimization to >> me), this change is unnecessary. > > I added "force" in v2 of the patch in response to John's feedback to > v1. He pointed out that when you unload the module when you have a > device connected that my v1 patch would not properly disconnect the > device (or, rather, it would disconnect it and then reconnect it). > > That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force > of "true". There's no mention of this in commit log. It would be great to add a: "while at that, also make sure that we don't try to detect a device on module unload because of foo bar baz as pointed out by John Youn". Or something along these lines. >>> /** >>> + * 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; >>> +} >> >> you make no mention of why this is needed. This is basically a refactor, >> not a fix. > > This new function is called from two places now, isn't it? > > Basically this is a little bit of code that used to be directly in > dwc2_port_intr(). I still want it there, but I also want to call the > same bit of code after a disconnect if I detect that the device has > been inserted again. I got that :-) But it's not mentioned in commit and it's apparently unnecessary for fixing the bug :-) Another "we're also adding a new hsotg_disconnect() function by means of refactoring to avoid code duplication" would've been enough. >>> @@ -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); >> >> what's inside this 'force' branch is what you really need to fix the >> problem, right ? It seems like for the -rc cycle, all you need is: >> >> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c >> index 571c21727ff9..967d1e686efe 100644 >> --- a/drivers/usb/dwc2/hcd.c >> +++ b/drivers/usb/dwc2/hcd.c >> @@ -276,6 +276,7 @@ static void dwc2_hcd_cleanup_channels(struct dwc2_hsotg *hsotg) >> */ >> void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) >> { >> + u32 hprt0; >> u32 intr; >> >> /* Set status flags for the hub driver */ >> @@ -315,6 +316,15 @@ void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg) >> dwc2_hcd_cleanup_channels(hsotg); >> >> dwc2_host_disconnect(hsotg); >> + >> + hprt0 = dwc2_readl(hsotg->regs + HPRT0); >> + if (!(hprt0 & (HPRT0_CONNDET | HPRT0_CONNSTS))) { >> + if (hsotg->lx_state != DWC2_L0) >> + usb_hcd_resume_roothub(hsotg->priv); >> + >> + hsotg->flags.b.port_connect_status_change = 1; >> + hsotg->flags.b.port_connect_status = 1; >> + } > > I'd really rather not add the duplication unless you insist. To me it > makes it clearer to include the (small) refactor in the same patch. > > If the refactor makes this change too big for an RC, then it's OK with > me to just skip this for the RC. It's not fixing a regression or > anything. I have no requirements to have this land in 4.4. It fixes > a bug and I thought that the fix was pretty small and safe (despite > having a diffstat that's bigger than the bare minimum). I will leave > it to your judgement. let's at least modify commit log to make all these extra changes clear that they are needed because of reason (a) or (b) or whatever. If you just send a patch doing much more than it apparently should without no mention as to why it was done this way, I can't know for sure those changes are needed; next thing you know, Greg refuses to apply my pull request because the change is too large :-) We don't want that to happen :-) >>> 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); >> >> unnecessary change. >> >> Do you agree or do you think 'force' is really necessary for this fix ? > > As per above, John thought it was a good idea to really make the > disconnect happen upon module unload. If you disagree then we could > probably just totally remove the "dwc2_hcd_disconnect(hsotg);" from > the _dwc2_hcd_stop() function. see above. thanks -- balbi [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] [-- Attachment #2: Type: text/plain, Size: 200 bytes --] _______________________________________________ Linux-rockchip mailing list Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 18:44 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-16 18:44 UTC (permalink / raw) To: Felipe Balbi Cc: John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel Felipe, On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <balbi@ti.com> wrote: >> I added "force" in v2 of the patch in response to John's feedback to >> v1. He pointed out that when you unload the module when you have a >> device connected that my v1 patch would not properly disconnect the >> device (or, rather, it would disconnect it and then reconnect it). >> >> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force >> of "true". > > There's no mention of this in commit log. It would be great to add a: > > "while at that, also make sure that we don't try to detect a device on > module unload because of foo bar baz as pointed out by John Youn". > > Or something along these lines. ...well, except that it's not new behavior. In other words: * Without my patch at all: we don't try to detect a device on module unload. * With my v1 patch: we (incorrectly) did try to detect a device on module unload. * With my v2 patch: we're back to not trying to detect a device on module unload. In other words: my v2 patch (correctly) doesn't change the behavior on module unload. That's why I didn't mention it in the commit message. It's in the "v2" changelog though. I'll try to come up with something for the commit message though. See below for new proposed commit message. >>> you make no mention of why this is needed. This is basically a refactor, >>> not a fix. >> >> This new function is called from two places now, isn't it? >> >> Basically this is a little bit of code that used to be directly in >> dwc2_port_intr(). I still want it there, but I also want to call the >> same bit of code after a disconnect if I detect that the device has >> been inserted again. > > I got that :-) But it's not mentioned in commit and it's apparently > unnecessary for fixing the bug :-) Another "we're also adding a new > hsotg_disconnect() function by means of refactoring to avoid code > duplication" would've been enough. OK, sure. >> I'd really rather not add the duplication unless you insist. To me it >> makes it clearer to include the (small) refactor in the same patch. >> >> If the refactor makes this change too big for an RC, then it's OK with >> me to just skip this for the RC. It's not fixing a regression or >> anything. I have no requirements to have this land in 4.4. It fixes >> a bug and I thought that the fix was pretty small and safe (despite >> having a diffstat that's bigger than the bare minimum). I will leave >> it to your judgement. > > let's at least modify commit log to make all these extra changes clear > that they are needed because of reason (a) or (b) or whatever. If you > just send a patch doing much more than it apparently should without no > mention as to why it was done this way, I can't know for sure those > changes are needed; next thing you know, Greg refuses to apply my pull > request because the change is too large :-) > > We don't want that to happen :-) Totally understand. It's your butt on the line for the pull request to Greg, so definitely want to make sure you're comfortable with anything you pass on. As always I definitely appreciate your reviews and your time. How about if we just add a "Notes" to the end of the patch description. I can repost a patch or you can feel free to change the description as per below (just let me know). ...so in total: --- usb: dwc2: host: Fix missing device insertions 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. Notes: 1. As part of this change we add a "force" parameter to dwc2_hcd_disconnect() so that when we're unloading the module we avoid the new behavior. The need for this was pointed out by John Youn. 2. The bit of code needed at the end of dwc2_hcd_disconnect() is exactly the same bit of code from dwc2_port_intr(). To avoid duplication, we refactor that code out into a new function dwc2_hcd_connect(). -Doug ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 18:44 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-16 18:44 UTC (permalink / raw) To: Felipe Balbi Cc: John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA Felipe, On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote: >> I added "force" in v2 of the patch in response to John's feedback to >> v1. He pointed out that when you unload the module when you have a >> device connected that my v1 patch would not properly disconnect the >> device (or, rather, it would disconnect it and then reconnect it). >> >> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force >> of "true". > > There's no mention of this in commit log. It would be great to add a: > > "while at that, also make sure that we don't try to detect a device on > module unload because of foo bar baz as pointed out by John Youn". > > Or something along these lines. ...well, except that it's not new behavior. In other words: * Without my patch at all: we don't try to detect a device on module unload. * With my v1 patch: we (incorrectly) did try to detect a device on module unload. * With my v2 patch: we're back to not trying to detect a device on module unload. In other words: my v2 patch (correctly) doesn't change the behavior on module unload. That's why I didn't mention it in the commit message. It's in the "v2" changelog though. I'll try to come up with something for the commit message though. See below for new proposed commit message. >>> you make no mention of why this is needed. This is basically a refactor, >>> not a fix. >> >> This new function is called from two places now, isn't it? >> >> Basically this is a little bit of code that used to be directly in >> dwc2_port_intr(). I still want it there, but I also want to call the >> same bit of code after a disconnect if I detect that the device has >> been inserted again. > > I got that :-) But it's not mentioned in commit and it's apparently > unnecessary for fixing the bug :-) Another "we're also adding a new > hsotg_disconnect() function by means of refactoring to avoid code > duplication" would've been enough. OK, sure. >> I'd really rather not add the duplication unless you insist. To me it >> makes it clearer to include the (small) refactor in the same patch. >> >> If the refactor makes this change too big for an RC, then it's OK with >> me to just skip this for the RC. It's not fixing a regression or >> anything. I have no requirements to have this land in 4.4. It fixes >> a bug and I thought that the fix was pretty small and safe (despite >> having a diffstat that's bigger than the bare minimum). I will leave >> it to your judgement. > > let's at least modify commit log to make all these extra changes clear > that they are needed because of reason (a) or (b) or whatever. If you > just send a patch doing much more than it apparently should without no > mention as to why it was done this way, I can't know for sure those > changes are needed; next thing you know, Greg refuses to apply my pull > request because the change is too large :-) > > We don't want that to happen :-) Totally understand. It's your butt on the line for the pull request to Greg, so definitely want to make sure you're comfortable with anything you pass on. As always I definitely appreciate your reviews and your time. How about if we just add a "Notes" to the end of the patch description. I can repost a patch or you can feel free to change the description as per below (just let me know). ...so in total: --- usb: dwc2: host: Fix missing device insertions 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. Notes: 1. As part of this change we add a "force" parameter to dwc2_hcd_disconnect() so that when we're unloading the module we avoid the new behavior. The need for this was pointed out by John Youn. 2. The bit of code needed at the end of dwc2_hcd_disconnect() is exactly the same bit of code from dwc2_port_intr(). To avoid duplication, we refactor that code out into a new function dwc2_hcd_connect(). -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 19:09 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-16 19:09 UTC (permalink / raw) To: Doug Anderson Cc: John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel [-- Attachment #1: Type: text/plain, Size: 5004 bytes --] hi, Doug Anderson <dianders@chromium.org> writes: > Felipe, > > On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <balbi@ti.com> wrote: >>> I added "force" in v2 of the patch in response to John's feedback to >>> v1. He pointed out that when you unload the module when you have a >>> device connected that my v1 patch would not properly disconnect the >>> device (or, rather, it would disconnect it and then reconnect it). >>> >>> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force >>> of "true". >> >> There's no mention of this in commit log. It would be great to add a: >> >> "while at that, also make sure that we don't try to detect a device on >> module unload because of foo bar baz as pointed out by John Youn". >> >> Or something along these lines. > > ...well, except that it's not new behavior. In other words: > > * Without my patch at all: we don't try to detect a device on module unload. > > * With my v1 patch: we (incorrectly) did try to detect a device on > module unload. > > * With my v2 patch: we're back to not trying to detect a device on > module unload. > > In other words: my v2 patch (correctly) doesn't change the behavior on > module unload. That's why I didn't mention it in the commit message. > It's in the "v2" changelog though. > > > I'll try to come up with something for the commit message though. See > below for new proposed commit message. > > >>>> you make no mention of why this is needed. This is basically a refactor, >>>> not a fix. >>> >>> This new function is called from two places now, isn't it? >>> >>> Basically this is a little bit of code that used to be directly in >>> dwc2_port_intr(). I still want it there, but I also want to call the >>> same bit of code after a disconnect if I detect that the device has >>> been inserted again. >> >> I got that :-) But it's not mentioned in commit and it's apparently >> unnecessary for fixing the bug :-) Another "we're also adding a new >> hsotg_disconnect() function by means of refactoring to avoid code >> duplication" would've been enough. > > OK, sure. > > >>> I'd really rather not add the duplication unless you insist. To me it >>> makes it clearer to include the (small) refactor in the same patch. >>> >>> If the refactor makes this change too big for an RC, then it's OK with >>> me to just skip this for the RC. It's not fixing a regression or >>> anything. I have no requirements to have this land in 4.4. It fixes >>> a bug and I thought that the fix was pretty small and safe (despite >>> having a diffstat that's bigger than the bare minimum). I will leave >>> it to your judgement. >> >> let's at least modify commit log to make all these extra changes clear >> that they are needed because of reason (a) or (b) or whatever. If you >> just send a patch doing much more than it apparently should without no >> mention as to why it was done this way, I can't know for sure those >> changes are needed; next thing you know, Greg refuses to apply my pull >> request because the change is too large :-) >> >> We don't want that to happen :-) > > Totally understand. It's your butt on the line for the pull request > to Greg, so definitely want to make sure you're comfortable with > anything you pass on. As always I definitely appreciate your reviews > and your time. > > > How about if we just add a "Notes" to the end of the patch > description. I can repost a patch or you can feel free to change the > description as per below (just let me know). ...so in total: > > --- > > usb: dwc2: host: Fix missing device insertions > > 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. > > Notes: > > 1. As part of this change we add a "force" parameter to > dwc2_hcd_disconnect() so that when we're unloading the module we > avoid the new behavior. The need for this was pointed out by John > Youn. > 2. The bit of code needed at the end of dwc2_hcd_disconnect() is > exactly the same bit of code from dwc2_port_intr(). To avoid > duplication, we refactor that code out into a new function > dwc2_hcd_connect(). this should be enough, thanks for being so responsive -- balbi [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 19:09 ` Felipe Balbi 0 siblings, 0 replies; 39+ messages in thread From: Felipe Balbi @ 2015-11-16 19:09 UTC (permalink / raw) To: Doug Anderson Cc: Herrero, Gregory, Heiko Stübner, John Youn, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, John Youn, open list:ARM/Rockchip SoC..., Kaukab, Yousaf, Yunzhi Li, Julius Werner, Dinh Nguyen [-- Attachment #1.1: Type: text/plain, Size: 5053 bytes --] hi, Doug Anderson <dianders-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> writes: > Felipe, > > On Mon, Nov 16, 2015 at 10:22 AM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote: >>> I added "force" in v2 of the patch in response to John's feedback to >>> v1. He pointed out that when you unload the module when you have a >>> device connected that my v1 patch would not properly disconnect the >>> device (or, rather, it would disconnect it and then reconnect it). >>> >>> That's why _dwc2_hcd_stop() calls dwc2_hcd_disconnect() with a force >>> of "true". >> >> There's no mention of this in commit log. It would be great to add a: >> >> "while at that, also make sure that we don't try to detect a device on >> module unload because of foo bar baz as pointed out by John Youn". >> >> Or something along these lines. > > ...well, except that it's not new behavior. In other words: > > * Without my patch at all: we don't try to detect a device on module unload. > > * With my v1 patch: we (incorrectly) did try to detect a device on > module unload. > > * With my v2 patch: we're back to not trying to detect a device on > module unload. > > In other words: my v2 patch (correctly) doesn't change the behavior on > module unload. That's why I didn't mention it in the commit message. > It's in the "v2" changelog though. > > > I'll try to come up with something for the commit message though. See > below for new proposed commit message. > > >>>> you make no mention of why this is needed. This is basically a refactor, >>>> not a fix. >>> >>> This new function is called from two places now, isn't it? >>> >>> Basically this is a little bit of code that used to be directly in >>> dwc2_port_intr(). I still want it there, but I also want to call the >>> same bit of code after a disconnect if I detect that the device has >>> been inserted again. >> >> I got that :-) But it's not mentioned in commit and it's apparently >> unnecessary for fixing the bug :-) Another "we're also adding a new >> hsotg_disconnect() function by means of refactoring to avoid code >> duplication" would've been enough. > > OK, sure. > > >>> I'd really rather not add the duplication unless you insist. To me it >>> makes it clearer to include the (small) refactor in the same patch. >>> >>> If the refactor makes this change too big for an RC, then it's OK with >>> me to just skip this for the RC. It's not fixing a regression or >>> anything. I have no requirements to have this land in 4.4. It fixes >>> a bug and I thought that the fix was pretty small and safe (despite >>> having a diffstat that's bigger than the bare minimum). I will leave >>> it to your judgement. >> >> let's at least modify commit log to make all these extra changes clear >> that they are needed because of reason (a) or (b) or whatever. If you >> just send a patch doing much more than it apparently should without no >> mention as to why it was done this way, I can't know for sure those >> changes are needed; next thing you know, Greg refuses to apply my pull >> request because the change is too large :-) >> >> We don't want that to happen :-) > > Totally understand. It's your butt on the line for the pull request > to Greg, so definitely want to make sure you're comfortable with > anything you pass on. As always I definitely appreciate your reviews > and your time. > > > How about if we just add a "Notes" to the end of the patch > description. I can repost a patch or you can feel free to change the > description as per below (just let me know). ...so in total: > > --- > > usb: dwc2: host: Fix missing device insertions > > 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. > > Notes: > > 1. As part of this change we add a "force" parameter to > dwc2_hcd_disconnect() so that when we're unloading the module we > avoid the new behavior. The need for this was pointed out by John > Youn. > 2. The bit of code needed at the end of dwc2_hcd_disconnect() is > exactly the same bit of code from dwc2_port_intr(). To avoid > duplication, we refactor that code out into a new function > dwc2_hcd_connect(). this should be enough, thanks for being so responsive -- balbi [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 818 bytes --] [-- Attachment #2: Type: text/plain, Size: 200 bytes --] _______________________________________________ Linux-rockchip mailing list Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org http://lists.infradead.org/mailman/listinfo/linux-rockchip ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 19:31 ` Alan Stern 0 siblings, 0 replies; 39+ messages in thread From: Alan Stern @ 2015-11-16 19:31 UTC (permalink / raw) To: Doug Anderson Cc: Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel On Mon, 16 Nov 2015, Doug Anderson wrote: > --- > > usb: dwc2: host: Fix missing device insertions > > 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. Why do you need to do anything special here? Normally a driver's interrupt handler should query the hardware status after clearing the interrupt source. That way no transitions ever get missed. In your example, at step 5 the dwc2 driver would check the port status and see that it currently is connected. Therefore the driver would pass a "connect status changed" event to the USB core and set the port status to "connected". No extra checking is needed, and transitory connects or disconnects get handled correctly. Alan Stern ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 19:31 ` Alan Stern 0 siblings, 0 replies; 39+ messages in thread From: Alan Stern @ 2015-11-16 19:31 UTC (permalink / raw) To: Doug Anderson Cc: Herrero, Gregory, Heiko Stübner, John Youn, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, Felipe Balbi, John Youn, open list:ARM/Rockchip SoC..., Kaukab, Yousaf, Yunzhi Li, Julius Werner, Dinh Nguyen On Mon, 16 Nov 2015, Doug Anderson wrote: > --- > > usb: dwc2: host: Fix missing device insertions > > 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. Why do you need to do anything special here? Normally a driver's interrupt handler should query the hardware status after clearing the interrupt source. That way no transitions ever get missed. In your example, at step 5 the dwc2 driver would check the port status and see that it currently is connected. Therefore the driver would pass a "connect status changed" event to the USB core and set the port status to "connected". No extra checking is needed, and transitory connects or disconnects get handled correctly. Alan Stern ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 19:46 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-16 19:46 UTC (permalink / raw) To: Alan Stern Cc: Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel Alan, On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 16 Nov 2015, Doug Anderson wrote: > >> --- >> >> usb: dwc2: host: Fix missing device insertions >> >> 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. > > Why do you need to do anything special here? Normally a driver's > interrupt handler should query the hardware status after clearing the > interrupt source. That way no transitions ever get missed. > > In your example, at step 5 the dwc2 driver would check the port status > and see that it currently is connected. Therefore the driver would > pass a "connect status changed" event to the USB core and set the port > status to "connected". No extra checking is needed, and transitory > connects or disconnects get handled correctly. Things are pretty ugly at the moment because the dwc2 interrupt handler is split in two. There's dwc2_handle_hcd_intr() and dwc2_handle_common_intr(). Both are registered for the same "shared" IRQ. If I had to guess, I'd say that this is probably because someone wanted to assign the ".irq" field in the "struct hc_driver" for the host controller but that they also needed the other interrupt handler to handle things shared between host and gadget mode. In any case, one of these two interrupt routines handles connect and the other disconnect. That's pretty ugly but means that you can't just rely on standard techniques for keeping things in sync. It would probably be a pretty reasonable idea to try to clean that up more, but that would be a very major and intrusive change. -Doug ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 19:46 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-16 19:46 UTC (permalink / raw) To: Alan Stern Cc: Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA Alan, On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote: > On Mon, 16 Nov 2015, Doug Anderson wrote: > >> --- >> >> usb: dwc2: host: Fix missing device insertions >> >> 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. > > Why do you need to do anything special here? Normally a driver's > interrupt handler should query the hardware status after clearing the > interrupt source. That way no transitions ever get missed. > > In your example, at step 5 the dwc2 driver would check the port status > and see that it currently is connected. Therefore the driver would > pass a "connect status changed" event to the USB core and set the port > status to "connected". No extra checking is needed, and transitory > connects or disconnects get handled correctly. Things are pretty ugly at the moment because the dwc2 interrupt handler is split in two. There's dwc2_handle_hcd_intr() and dwc2_handle_common_intr(). Both are registered for the same "shared" IRQ. If I had to guess, I'd say that this is probably because someone wanted to assign the ".irq" field in the "struct hc_driver" for the host controller but that they also needed the other interrupt handler to handle things shared between host and gadget mode. In any case, one of these two interrupt routines handles connect and the other disconnect. That's pretty ugly but means that you can't just rely on standard techniques for keeping things in sync. It would probably be a pretty reasonable idea to try to clean that up more, but that would be a very major and intrusive change. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions 2015-11-16 19:46 ` Doug Anderson (?) @ 2015-11-16 20:26 ` Julius Werner 2015-11-16 20:38 ` Alan Stern -1 siblings, 1 reply; 39+ messages in thread From: Julius Werner @ 2015-11-16 20:26 UTC (permalink / raw) To: Doug Anderson Cc: Alan Stern, Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel Another point to note, which I think is what prevents the flow Alan suggested from working, is this little snippet in DWC2's hub_control GetPortStatus callback: if (!hsotg->flags.b.port_connect_status) { /* * The port is disconnected, which means the core is * either in device mode or it soon will be. Just * return 0's for the remainder of the port status * since the port register can't be read if the core * is in device mode. */ *(__le32 *)buf = cpu_to_le32(port_status); break; } This is before it actually checks the register state of the port. So it relies on dwc2_hcd_connect() and dwc2_hcd_disconnect() to be called in the right order to give the correct answer for USB_PORT_STAT_CONNECTION. This could maybe be improved but I don't know what kind of weird interactions with device mode operation made that snippet necessary in the first place. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 20:38 ` Alan Stern 0 siblings, 0 replies; 39+ messages in thread From: Alan Stern @ 2015-11-16 20:38 UTC (permalink / raw) To: Julius Werner Cc: Doug Anderson, Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel On Mon, 16 Nov 2015, Julius Werner wrote: > Another point to note, which I think is what prevents the flow Alan > suggested from working, is this little snippet in DWC2's hub_control > GetPortStatus callback: > > if (!hsotg->flags.b.port_connect_status) { > /* > * The port is disconnected, which means the core is > * either in device mode or it soon will be. > Just > * return 0's for the remainder of the port status > * since the port register can't be read if the core > * is in device mode. > */ > *(__le32 *)buf = cpu_to_le32(port_status); > break; > } > > This is before it actually checks the register state of the port. So > it relies on dwc2_hcd_connect() and dwc2_hcd_disconnect() to be called > in the right order to give the correct answer for > USB_PORT_STAT_CONNECTION. This could maybe be improved but I don't > know what kind of weird interactions with device mode operation made > that snippet necessary in the first place. Why does this test hsotg->flags.b.port_connect_status? What it really needs to know is whether the core is in device mode. It should test for that instead. Then it could accurately report the true connection state whenever the core is in host mode, regardless of the order in which dwc2_hcd_connect() and dwc2_hcd_disconnect() get called. No? My guess would be that using the wrong test was simply a misguided attempt at optimization. Alan Stern ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 20:38 ` Alan Stern 0 siblings, 0 replies; 39+ messages in thread From: Alan Stern @ 2015-11-16 20:38 UTC (permalink / raw) To: Julius Werner Cc: Doug Anderson, Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA On Mon, 16 Nov 2015, Julius Werner wrote: > Another point to note, which I think is what prevents the flow Alan > suggested from working, is this little snippet in DWC2's hub_control > GetPortStatus callback: > > if (!hsotg->flags.b.port_connect_status) { > /* > * The port is disconnected, which means the core is > * either in device mode or it soon will be. > Just > * return 0's for the remainder of the port status > * since the port register can't be read if the core > * is in device mode. > */ > *(__le32 *)buf = cpu_to_le32(port_status); > break; > } > > This is before it actually checks the register state of the port. So > it relies on dwc2_hcd_connect() and dwc2_hcd_disconnect() to be called > in the right order to give the correct answer for > USB_PORT_STAT_CONNECTION. This could maybe be improved but I don't > know what kind of weird interactions with device mode operation made > that snippet necessary in the first place. Why does this test hsotg->flags.b.port_connect_status? What it really needs to know is whether the core is in device mode. It should test for that instead. Then it could accurately report the true connection state whenever the core is in host mode, regardless of the order in which dwc2_hcd_connect() and dwc2_hcd_disconnect() get called. No? My guess would be that using the wrong test was simply a misguided attempt at optimization. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions 2015-11-16 19:46 ` Doug Anderson (?) (?) @ 2015-11-16 20:31 ` Alan Stern 2015-11-16 23:14 ` Doug Anderson -1 siblings, 1 reply; 39+ messages in thread From: Alan Stern @ 2015-11-16 20:31 UTC (permalink / raw) To: Doug Anderson Cc: Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel On Mon, 16 Nov 2015, Doug Anderson wrote: > Alan, > > On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Mon, 16 Nov 2015, Doug Anderson wrote: > > > >> --- > >> > >> usb: dwc2: host: Fix missing device insertions > >> > >> 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. > > > > Why do you need to do anything special here? Normally a driver's > > interrupt handler should query the hardware status after clearing the > > interrupt source. That way no transitions ever get missed. > > > > In your example, at step 5 the dwc2 driver would check the port status > > and see that it currently is connected. Therefore the driver would > > pass a "connect status changed" event to the USB core and set the port > > status to "connected". No extra checking is needed, and transitory > > connects or disconnects get handled correctly. > > Things are pretty ugly at the moment because the dwc2 interrupt > handler is split in two. There's dwc2_handle_hcd_intr() and > dwc2_handle_common_intr(). Both are registered for the same "shared" > IRQ. If I had to guess, I'd say that this is probably because someone > wanted to assign the ".irq" field in the "struct hc_driver" for the > host controller but that they also needed the other interrupt handler > to handle things shared between host and gadget mode. > > In any case, one of these two interrupt routines handles connect and > the other disconnect. That's pretty ugly but means that you can't > just rely on standard techniques for keeping things in sync. Okay, that is rather weird. Still, I don't see why it should matter. Fundamentally there's no difference between a "connect" interrupt and a "disconnect" interrupt. They should both do exactly the same things: clear the interrupt source, post a "connection change" event, and set the driver's connect status based on the hardware's current state. The second and third parts can be handled by a shared subroutine. If you think of these things as "connect changed" interrupts rather than as "connect" and "disconnect" interrupts, it makes a lot of sense. > It would probably be a pretty reasonable idea to try to clean that up > more, but that would be a very major and intrusive change. Maybe so -- I'll take your word for it since I'm not at all familiar with the dwc2 code. Alan Stern ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 23:14 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-16 23:14 UTC (permalink / raw) To: Alan Stern Cc: Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel Alan, On Mon, Nov 16, 2015 at 12:31 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 16 Nov 2015, Doug Anderson wrote: > >> Alan, >> >> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Mon, 16 Nov 2015, Doug Anderson wrote: >> > >> >> --- >> >> >> >> usb: dwc2: host: Fix missing device insertions >> >> >> >> 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. >> > >> > Why do you need to do anything special here? Normally a driver's >> > interrupt handler should query the hardware status after clearing the >> > interrupt source. That way no transitions ever get missed. >> > >> > In your example, at step 5 the dwc2 driver would check the port status >> > and see that it currently is connected. Therefore the driver would >> > pass a "connect status changed" event to the USB core and set the port >> > status to "connected". No extra checking is needed, and transitory >> > connects or disconnects get handled correctly. >> >> Things are pretty ugly at the moment because the dwc2 interrupt >> handler is split in two. There's dwc2_handle_hcd_intr() and >> dwc2_handle_common_intr(). Both are registered for the same "shared" >> IRQ. If I had to guess, I'd say that this is probably because someone >> wanted to assign the ".irq" field in the "struct hc_driver" for the >> host controller but that they also needed the other interrupt handler >> to handle things shared between host and gadget mode. >> >> In any case, one of these two interrupt routines handles connect and >> the other disconnect. That's pretty ugly but means that you can't >> just rely on standard techniques for keeping things in sync. > > Okay, that is rather weird. Still, I don't see why it should matter. > > Fundamentally there's no difference between a "connect" interrupt and a > "disconnect" interrupt. They should both do exactly the same things: > clear the interrupt source, post a "connection change" event, and set > the driver's connect status based on the hardware's current state. > The second and third parts can be handled by a shared subroutine. Ah, sorry I misunderstood. OK, fair enough. So you're saying that the problem is that dwc2_hcd_disconnect() has a line that looks like this: hsotg->flags.b.port_connect_status = 0; ...and the dwc2_port_intr() has a line that looks like this: hsotg->flags.b.port_connect_status = 1; ...and both should just query the status. Do you think we should to block landing this patch on cleaning up how dwc2 handles port_connect_status? I'm not sure what side effects changing port_connect_status will have, so I'll need to test and dig a bit... I'm currently working on trying to fix the microframe scheduler and was planning to post the next series of patches there pretty soon. I'm also planning to keep digging to figure out how to overall increase compatibility with devices (and compatibility with many devices plugged in). If it were up to me, I'd prefer to land this patch in either 4.4 or 4.5 since it does seem to work. ...then put seeing what we can do to cleanup all of the port_connect_status on the todo list. Julius points out in his response that there are comments saying that HPRT0 can't be read in device mode. John: since you're setup to test device mode, can you check if my patch (which now adds a read of HPRT0) will cause problems? Maybe holding this off and keeping it out of the RC is a good idea after all... -Doug ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions @ 2015-11-16 23:14 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-16 23:14 UTC (permalink / raw) To: Alan Stern Cc: Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA Alan, On Mon, Nov 16, 2015 at 12:31 PM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote: > On Mon, 16 Nov 2015, Doug Anderson wrote: > >> Alan, >> >> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org> wrote: >> > On Mon, 16 Nov 2015, Doug Anderson wrote: >> > >> >> --- >> >> >> >> usb: dwc2: host: Fix missing device insertions >> >> >> >> 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. >> > >> > Why do you need to do anything special here? Normally a driver's >> > interrupt handler should query the hardware status after clearing the >> > interrupt source. That way no transitions ever get missed. >> > >> > In your example, at step 5 the dwc2 driver would check the port status >> > and see that it currently is connected. Therefore the driver would >> > pass a "connect status changed" event to the USB core and set the port >> > status to "connected". No extra checking is needed, and transitory >> > connects or disconnects get handled correctly. >> >> Things are pretty ugly at the moment because the dwc2 interrupt >> handler is split in two. There's dwc2_handle_hcd_intr() and >> dwc2_handle_common_intr(). Both are registered for the same "shared" >> IRQ. If I had to guess, I'd say that this is probably because someone >> wanted to assign the ".irq" field in the "struct hc_driver" for the >> host controller but that they also needed the other interrupt handler >> to handle things shared between host and gadget mode. >> >> In any case, one of these two interrupt routines handles connect and >> the other disconnect. That's pretty ugly but means that you can't >> just rely on standard techniques for keeping things in sync. > > Okay, that is rather weird. Still, I don't see why it should matter. > > Fundamentally there's no difference between a "connect" interrupt and a > "disconnect" interrupt. They should both do exactly the same things: > clear the interrupt source, post a "connection change" event, and set > the driver's connect status based on the hardware's current state. > The second and third parts can be handled by a shared subroutine. Ah, sorry I misunderstood. OK, fair enough. So you're saying that the problem is that dwc2_hcd_disconnect() has a line that looks like this: hsotg->flags.b.port_connect_status = 0; ...and the dwc2_port_intr() has a line that looks like this: hsotg->flags.b.port_connect_status = 1; ...and both should just query the status. Do you think we should to block landing this patch on cleaning up how dwc2 handles port_connect_status? I'm not sure what side effects changing port_connect_status will have, so I'll need to test and dig a bit... I'm currently working on trying to fix the microframe scheduler and was planning to post the next series of patches there pretty soon. I'm also planning to keep digging to figure out how to overall increase compatibility with devices (and compatibility with many devices plugged in). If it were up to me, I'd prefer to land this patch in either 4.4 or 4.5 since it does seem to work. ...then put seeing what we can do to cleanup all of the port_connect_status on the todo list. Julius points out in his response that there are comments saying that HPRT0 can't be read in device mode. John: since you're setup to test device mode, can you check if my patch (which now adds a read of HPRT0) will cause problems? Maybe holding this off and keeping it out of the RC is a good idea after all... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions 2015-11-16 23:14 ` Doug Anderson (?) @ 2015-11-17 1:53 ` John Youn 2015-11-17 2:37 ` Doug Anderson -1 siblings, 1 reply; 39+ messages in thread From: John Youn @ 2015-11-17 1:53 UTC (permalink / raw) To: Doug Anderson, Alan Stern Cc: Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, Greg Kroah-Hartman, linux-usb, linux-kernel On 11/16/2015 3:14 PM, Doug Anderson wrote: > Alan, > > On Mon, Nov 16, 2015 at 12:31 PM, Alan Stern <stern@rowland.harvard.edu> wrote: >> On Mon, 16 Nov 2015, Doug Anderson wrote: >> >>> Alan, >>> >>> On Mon, Nov 16, 2015 at 11:31 AM, Alan Stern <stern@rowland.harvard.edu> wrote: >>>> On Mon, 16 Nov 2015, Doug Anderson wrote: >>>> >>>>> --- >>>>> >>>>> usb: dwc2: host: Fix missing device insertions >>>>> >>>>> 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. >>>> >>>> Why do you need to do anything special here? Normally a driver's >>>> interrupt handler should query the hardware status after clearing the >>>> interrupt source. That way no transitions ever get missed. >>>> >>>> In your example, at step 5 the dwc2 driver would check the port status >>>> and see that it currently is connected. Therefore the driver would >>>> pass a "connect status changed" event to the USB core and set the port >>>> status to "connected". No extra checking is needed, and transitory >>>> connects or disconnects get handled correctly. >>> >>> Things are pretty ugly at the moment because the dwc2 interrupt >>> handler is split in two. There's dwc2_handle_hcd_intr() and >>> dwc2_handle_common_intr(). Both are registered for the same "shared" >>> IRQ. If I had to guess, I'd say that this is probably because someone >>> wanted to assign the ".irq" field in the "struct hc_driver" for the >>> host controller but that they also needed the other interrupt handler >>> to handle things shared between host and gadget mode. >>> >>> In any case, one of these two interrupt routines handles connect and >>> the other disconnect. That's pretty ugly but means that you can't >>> just rely on standard techniques for keeping things in sync. >> >> Okay, that is rather weird. Still, I don't see why it should matter. >> >> Fundamentally there's no difference between a "connect" interrupt and a >> "disconnect" interrupt. They should both do exactly the same things: >> clear the interrupt source, post a "connection change" event, and set >> the driver's connect status based on the hardware's current state. >> The second and third parts can be handled by a shared subroutine. > > Ah, sorry I misunderstood. OK, fair enough. So you're saying that > the problem is that dwc2_hcd_disconnect() has a line that looks like > this: > > hsotg->flags.b.port_connect_status = 0; > > ...and the dwc2_port_intr() has a line that looks like this: > > hsotg->flags.b.port_connect_status = 1; > > ...and both should just query the status. > > > Do you think we should to block landing this patch on cleaning up how > dwc2 handles port_connect_status? I'm not sure what side effects > changing port_connect_status will have, so I'll need to test and dig a > bit... > > I'm currently working on trying to fix the microframe scheduler and > was planning to post the next series of patches there pretty soon. > I'm also planning to keep digging to figure out how to overall > increase compatibility with devices (and compatibility with many > devices plugged in). > > If it were up to me, I'd prefer to land this patch in either 4.4 or > 4.5 since it does seem to work. ...then put seeing what we can do to > cleanup all of the port_connect_status on the todo list. That sound good to me. Though I'd prefer to see this series queued for 4.5 for more testing. > > Julius points out in his response that there are comments saying that > HPRT0 can't be read in device mode. John: since you're setup to test > device mode, can you check if my patch (which now adds a read of > HPRT0) will cause problems? Maybe holding this off and keeping it out > of the RC is a good idea after all... > Yup it's only available in host mode. The same with all the host-mode registers. You will get a ModeMis interrupt if you try to access them in device mode. I gave my test-by on the v2 patches earlier, no problems here. John ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions 2015-11-17 1:53 ` John Youn @ 2015-11-17 2:37 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-17 2:37 UTC (permalink / raw) To: John Youn Cc: Alan Stern, Felipe Balbi, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, Greg Kroah-Hartman, linux-usb, linux-kernel John, On Mon, Nov 16, 2015 at 5:53 PM, John Youn <John.Youn@synopsys.com> wrote: > Yup it's only available in host mode. The same with all the > host-mode registers. You will get a ModeMis interrupt if you > try to access them in device mode. > > I gave my test-by on the v2 patches earlier, no problems here. Yup, I appreciate it! I just wanted to confirm that you tested this particular case in gadget mode. ;) ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions 2015-11-16 23:14 ` Doug Anderson (?) (?) @ 2015-11-17 15:40 ` Alan Stern 2015-11-17 16:13 ` Doug Anderson -1 siblings, 1 reply; 39+ messages in thread From: Alan Stern @ 2015-11-17 15:40 UTC (permalink / raw) To: Doug Anderson Cc: Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel On Mon, 16 Nov 2015, Doug Anderson wrote: > > Fundamentally there's no difference between a "connect" interrupt and a > > "disconnect" interrupt. They should both do exactly the same things: > > clear the interrupt source, post a "connection change" event, and set > > the driver's connect status based on the hardware's current state. > > The second and third parts can be handled by a shared subroutine. > > Ah, sorry I misunderstood. OK, fair enough. So you're saying that > the problem is that dwc2_hcd_disconnect() has a line that looks like > this: > > hsotg->flags.b.port_connect_status = 0; > > ...and the dwc2_port_intr() has a line that looks like this: > > hsotg->flags.b.port_connect_status = 1; > > ...and both should just query the status. Well, I don't know how the driver uses flags.b.port_connect_status. In principle it could do away with that flag completely and always query the hardware status. > Do you think we should to block landing this patch on cleaning up how > dwc2 handles port_connect_status? I'm not sure what side effects > changing port_connect_status will have, so I'll need to test and dig a > bit... > > I'm currently working on trying to fix the microframe scheduler and > was planning to post the next series of patches there pretty soon. > I'm also planning to keep digging to figure out how to overall > increase compatibility with devices (and compatibility with many > devices plugged in). > > If it were up to me, I'd prefer to land this patch in either 4.4 or > 4.5 since it does seem to work. ...then put seeing what we can do to > cleanup all of the port_connect_status on the todo list. It's up to you guys. All I've been doing here is pointing out that your proposed approach didn't seem like the best. Alan Stern ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions 2015-11-17 15:40 ` Alan Stern @ 2015-11-17 16:13 ` Doug Anderson 0 siblings, 0 replies; 39+ messages in thread From: Doug Anderson @ 2015-11-17 16:13 UTC (permalink / raw) To: Alan Stern Cc: Felipe Balbi, John Youn, Yunzhi Li, Heiko Stübner, open list:ARM/Rockchip SoC..., Julius Werner, Herrero, Gregory, Kaukab, Yousaf, Dinh Nguyen, John Youn, Greg Kroah-Hartman, linux-usb, linux-kernel Alan, On Tue, Nov 17, 2015 at 7:40 AM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Mon, 16 Nov 2015, Doug Anderson wrote: > >> > Fundamentally there's no difference between a "connect" interrupt and a >> > "disconnect" interrupt. They should both do exactly the same things: >> > clear the interrupt source, post a "connection change" event, and set >> > the driver's connect status based on the hardware's current state. >> > The second and third parts can be handled by a shared subroutine. >> >> Ah, sorry I misunderstood. OK, fair enough. So you're saying that >> the problem is that dwc2_hcd_disconnect() has a line that looks like >> this: >> >> hsotg->flags.b.port_connect_status = 0; >> >> ...and the dwc2_port_intr() has a line that looks like this: >> >> hsotg->flags.b.port_connect_status = 1; >> >> ...and both should just query the status. > > Well, I don't know how the driver uses flags.b.port_connect_status. In > principle it could do away with that flag completely and always query > the hardware status. > >> Do you think we should to block landing this patch on cleaning up how >> dwc2 handles port_connect_status? I'm not sure what side effects >> changing port_connect_status will have, so I'll need to test and dig a >> bit... >> >> I'm currently working on trying to fix the microframe scheduler and >> was planning to post the next series of patches there pretty soon. >> I'm also planning to keep digging to figure out how to overall >> increase compatibility with devices (and compatibility with many >> devices plugged in). >> >> If it were up to me, I'd prefer to land this patch in either 4.4 or >> 4.5 since it does seem to work. ...then put seeing what we can do to >> cleanup all of the port_connect_status on the todo list. > > It's up to you guys. All I've been doing here is pointing out that > your proposed approach didn't seem like the best. Thanks! Just wanted to make sure you know that I'm very very appreciative of your reviews and suggestions here. Having someone intimately familiar with how other USB host drivers work that's willing to point out how dwc2 can do things better will be very helpful in helping dwc2 grow. -Doug ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2015-11-19 19:09 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-03 20:30 [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions Douglas Anderson 2015-11-03 20:30 ` [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them Douglas Anderson 2015-11-03 20:30 ` Douglas Anderson 2015-11-05 3:08 ` John Youn 2015-11-16 16:28 ` Felipe Balbi 2015-11-16 16:28 ` Felipe Balbi 2015-11-16 17:22 ` Doug Anderson 2015-11-16 17:22 ` Doug Anderson 2015-11-19 1:43 ` John Youn 2015-11-19 1:43 ` John Youn 2015-11-19 16:37 ` Doug Anderson 2015-11-19 16:37 ` Doug Anderson 2015-11-19 18:19 ` Felipe Balbi 2015-11-19 18:19 ` Felipe Balbi 2015-11-19 19:09 ` John Youn 2015-11-05 2:52 ` [PATCH v2 1/2] usb: dwc2: host: Fix missing device insertions John Youn 2015-11-16 17:44 ` Felipe Balbi 2015-11-16 17:44 ` Felipe Balbi 2015-11-16 18:13 ` Doug Anderson 2015-11-16 18:22 ` Felipe Balbi 2015-11-16 18:22 ` Felipe Balbi 2015-11-16 18:44 ` Doug Anderson 2015-11-16 18:44 ` Doug Anderson 2015-11-16 19:09 ` Felipe Balbi 2015-11-16 19:09 ` Felipe Balbi 2015-11-16 19:31 ` Alan Stern 2015-11-16 19:31 ` Alan Stern 2015-11-16 19:46 ` Doug Anderson 2015-11-16 19:46 ` Doug Anderson 2015-11-16 20:26 ` Julius Werner 2015-11-16 20:38 ` Alan Stern 2015-11-16 20:38 ` Alan Stern 2015-11-16 20:31 ` Alan Stern 2015-11-16 23:14 ` Doug Anderson 2015-11-16 23:14 ` Doug Anderson 2015-11-17 1:53 ` John Youn 2015-11-17 2:37 ` Doug Anderson 2015-11-17 15:40 ` Alan Stern 2015-11-17 16:13 ` Doug Anderson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.