[PATCH v4 0/6] usb: dwc2: fix low power issues found on stm32mp1

[PATCH v4 0/6] usb: dwc2: fix low power issues found on stm32mp1

[Fabrice Gasnier]

This series addresses several hang, bad_irq or re-initialisation issues
found on stm32mp1 when going to system low power (stop mode):
- First three fixes can be reproduced on stm32mp13 DK board, stm32mp15 DK
  and EVAL boards.
- Subsequent fixes can be reproduced on stm32mp15 EVAL board that uses
  an external Vbus regulator and stm32 dedicated detection logic.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v4:
- Address Sashiko feedback on register-loss recovery: add
  dwc2_hcd_restore() to reinitialize the host and clear stale state;
  disconnect the gadget and clear stale endpoint and low-power state.
- Fix the HCD resume race by holding hsotg->lock with interrupts disabled
  across global interrupt enable and dwc2_hcd_reinit().
- Rework VBUS handling: track the regulator enable reference with
  vbus_supply_enabled and a mutex, keep external VBUS enabled across
  system suspend, and drop dwc2_vbus_supply_restore().
- Track STM32 ID/VBUS detection enablement with
  stm32_id_vb_detection_enabled.

Changes in v3:
- Update since Sashiko comments
- Add a paramter (need_restore) to gadget resume function based on
  canary detection to avoir possible race on lx_state as raised by
  Sashiko
- Add a dwc2_vbus_supply_restore() to check regulator enable state
  during hcd resume, to enforce it gets restored along with PWR bit
  if it has been turned off earlier.
- Introduced a new patch for a possible race in hcd resume, with
  HCD_FLAG_HW_ACCESSIBLE not being set, but host interrupts are active.
- Refactor stm32 vb/id detection, to rool back in case of regulator
  disable fails.
- Link to v2: https://patch.msgid.link/20261013-dwc2-low-power-fixes-stm32-v2-0-79865a4f1b63@foss.st.com

Changes in v2:
- Several fixes or commit messages updates since Sashiko review (see
  changelog in each patch).
- Add Thinh instead of Minas to the recipients
- Link to v1: https://patch.msgid.link/20261008-dwc2-low-power-fixes-stm32-v1-0-5b303df762e6@foss.st.com

---
Fabrice Gasnier (6):
      usb: dwc2: fix recovery after PM domain off
      usb: dwc2: fix a possible interrupt storm in hcd resume
      usb: dwc2: fix a possible interrupt storm on stm32mp1
      usb: dwc2: hcd: fix possible successive vbus supply enables
      usb: dwc2: hcd: don't disable external vbus in system suspend
      usb: dwc2: platform: fix stm32mp15 detection logic power management

 drivers/usb/dwc2/core.c      |   1 -
 drivers/usb/dwc2/core.h      |  17 ++-
 drivers/usb/dwc2/core_intr.c |   9 +-
 drivers/usb/dwc2/gadget.c    |  18 +++-
 drivers/usb/dwc2/hcd.c       | 101 ++++++++++++-----
 drivers/usb/dwc2/platform.c  | 250 +++++++++++++++++++++++--------------------
 6 files changed, 246 insertions(+), 150 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261007-dwc2-low-power-fixes-stm32-0c1e5e68b252

Best regards,
--  
Fabrice Gasnier <fabrice.gasnier@foss.st.com>

[PATCH v4 1/6] usb: dwc2: fix recovery after PM domain off

[Fabrice Gasnier]

When the power domain containing the DWC2 controller is disabled during
suspend, its registers are lost. Reading registers to back them up from
dwc2_suspend() after clock gating can hang STM32MP1 in both host and
device mode. Host suspend enters clock gating directly; in device mode,
this can happen if the host suspended the USB bus before system suspend.

Check GUSBCFG.TOUTCAL after restoring the controller's mode to detect
register loss, then reinitialize the controller and role-specific state.
For host mode, reinitialize the HCD, discard stale register backups, and
restore HCD accessibility after setup. For gadget mode, force a
disconnect, disable non-EP0 endpoints, clear stale low-power state, and
mark gadget speed unknown. Pass the register-loss state to gadget resume
so recovery also runs when the host had suspended the bus.

Host resume observes HPRT0_CONNSTS cleared after register loss, so it
does not skip the recovery path. Ordinary resume and USB-reset behavior
remain unchanged.

Fixes: ba6e518d136b ("usb: dwc2: Implement recovery after PM domain off")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v4:
- Disconnect the gadget and clear stale endpoint and low-power state.
  Sashiko comment "usb: dwc2: incomplete gadget state cleanup in
  dwc2_hsotg_resume()"
- Reinitialize host state and clear stale backups after register loss.
  Sashiko comment "usb: dwc2: incomplete host mode recovery in
  dwc2_resume() after PM domain off"
- Revisit commit message

Changes in v3:
- Add a note in the commit message since Sashiko comment on
  _dwc2_hcd_resume() check on HPRT0_CONNSTS bit.
- Add need_restore flag and pass it to dwc2_hsotg_resume() to avoid
  possible interrupt race with lx_state update as raised by Sashiko,
  instead of updating lx_state to DWC2_L0.

Changes in v2:
- In case registers have been lost, ensure the device mode resume takes
  place by updating the lx_state to DWC2_L0, as reported by Sashiko. Note
  in case of error in dwc2_core_init() choice is made to leave the low
  level HW init, as it is the expected state out of the resume routine.
---
 drivers/usb/dwc2/core.c     |  1 -
 drivers/usb/dwc2/core.h     |  9 ++++---
 drivers/usb/dwc2/gadget.c   | 18 +++++++++++--
 drivers/usb/dwc2/hcd.c      | 47 +++++++++++++++++++++++++++++++++
 drivers/usb/dwc2/platform.c | 64 ++++++++++++++++++---------------------------
 5 files changed, 93 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 6a0135e06b00..50627c2baf1d 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -43,7 +43,6 @@ int dwc2_backup_global_registers(struct dwc2_hsotg *hsotg)
 	/* Backup global regs */
 	gr = &hsotg->gr_backup;
 
-	gr->gintsts = dwc2_readl(hsotg, GINTSTS);
 	gr->gotgctl = dwc2_readl(hsotg, GOTGCTL);
 	gr->gintmsk = dwc2_readl(hsotg, GINTMSK);
 	gr->gahbcfg = dwc2_readl(hsotg, GAHBCFG);
diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 015a5fb7842d..32662cb3d3eb 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -668,7 +668,6 @@ struct dwc2_hw_params {
 /**
  * struct dwc2_gregs_backup - Holds global registers state before
  * entering partial power down
- * @gintsts:		Backup of GINTSTS register
  * @gotgctl:		Backup of GOTGCTL register
  * @gintmsk:		Backup of GINTMSK register
  * @gahbcfg:		Backup of GAHBCFG register
@@ -685,7 +684,6 @@ struct dwc2_hw_params {
  * @valid:		True if registers values backuped.
  */
 struct dwc2_gregs_backup {
-	u32 gintsts;
 	u32 gotgctl;
 	u32 gintmsk;
 	u32 gahbcfg;
@@ -1415,7 +1413,7 @@ void dwc2_dump_global_registers(struct dwc2_hsotg *hsotg);
 	IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
 int dwc2_hsotg_remove(struct dwc2_hsotg *hsotg);
 int dwc2_hsotg_suspend(struct dwc2_hsotg *dwc2);
-int dwc2_hsotg_resume(struct dwc2_hsotg *dwc2);
+int dwc2_hsotg_resume(struct dwc2_hsotg *dwc2, bool need_restore);
 int dwc2_gadget_init(struct dwc2_hsotg *hsotg);
 void dwc2_hsotg_core_init_disconnected(struct dwc2_hsotg *dwc2,
 				       bool reset);
@@ -1451,7 +1449,7 @@ static inline int dwc2_hsotg_remove(struct dwc2_hsotg *dwc2)
 { return 0; }
 static inline int dwc2_hsotg_suspend(struct dwc2_hsotg *dwc2)
 { return 0; }
-static inline int dwc2_hsotg_resume(struct dwc2_hsotg *dwc2)
+static inline int dwc2_hsotg_resume(struct dwc2_hsotg *dwc2, bool need_restore)
 { return 0; }
 static inline int dwc2_gadget_init(struct dwc2_hsotg *hsotg)
 { return 0; }
@@ -1502,6 +1500,7 @@ static inline void dwc2_clear_fifo_map(struct dwc2_hsotg *hsotg) {}
 #if IS_ENABLED(CONFIG_USB_DWC2_HOST) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
 int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg);
 int dwc2_hcd_get_future_frame_number(struct dwc2_hsotg *hsotg, int us);
+int dwc2_hcd_restore(struct dwc2_hsotg *hsotg);
 void dwc2_hcd_connect(struct dwc2_hsotg *hsotg);
 void dwc2_hcd_disconnect(struct dwc2_hsotg *hsotg, bool force);
 void dwc2_hcd_start(struct dwc2_hsotg *hsotg);
@@ -1529,6 +1528,8 @@ static inline int dwc2_hcd_get_frame_number(struct dwc2_hsotg *hsotg)
 static inline int dwc2_hcd_get_future_frame_number(struct dwc2_hsotg *hsotg,
 						   int us)
 { return 0; }
+static inline int dwc2_hcd_restore(struct dwc2_hsotg *hsotg)
+{ return 0; }
 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) {}
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6c8a9f05fff0..225442471aa9 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -5200,11 +5200,12 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
 	return 0;
 }
 
-int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg)
+int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg, bool need_restore)
 {
 	unsigned long flags;
+	int ep;
 
-	if (hsotg->lx_state == DWC2_L2)
+	if (!need_restore && hsotg->lx_state == DWC2_L2)
 		return 0;
 
 	if (hsotg->driver) {
@@ -5212,6 +5213,19 @@ int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg)
 			 hsotg->driver->driver.name);
 
 		spin_lock_irqsave(&hsotg->lock, flags);
+		if (need_restore) {
+			hsotg->hibernated = 0;
+			hsotg->in_ppd = 0;
+			hsotg->bus_suspended = false;
+			dwc2_hsotg_disconnect(hsotg);
+			for (ep = 1; ep < hsotg->num_of_eps; ep++) {
+				if (hsotg->eps_in[ep])
+					dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
+				if (hsotg->eps_out[ep])
+					dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
+			}
+			hsotg->gadget.speed = USB_SPEED_UNKNOWN;
+		}
 		dwc2_hsotg_core_init_disconnected(hsotg, false);
 		if (hsotg->enabled) {
 			/* Enable ACG feature in device mode,if supported */
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index d5275cd658d4..3f9adfa7c630 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -2302,6 +2302,53 @@ static void dwc2_hcd_reinit(struct dwc2_hsotg *hsotg)
 	dwc2_core_host_init(hsotg);
 }
 
+/**
+ * dwc2_hcd_restore - reinitialize host state after register loss
+ * @hsotg: Programming view of the DWC_otg controller
+ *
+ * Return: 0 on success, or a negative error code on failure.
+ */
+int dwc2_hcd_restore(struct dwc2_hsotg *hsotg)
+{
+	struct usb_hcd *hcd = dwc2_hsotg_to_hcd(hsotg);
+	unsigned long flags;
+	int ret;
+
+	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+	dwc2_disable_global_interrupts(hsotg);
+
+	if (!IS_ERR_OR_NULL(hsotg->uphy))
+		usb_phy_set_suspend(hsotg->uphy, false);
+
+	ret = dwc2_vbus_supply_init(hsotg);
+	if (ret)
+		return ret;
+
+	ret = dwc2_core_init(hsotg, true);
+	if (ret) {
+		dwc2_vbus_supply_exit(hsotg);
+		return ret;
+	}
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+	dwc2_hcd_reinit(hsotg);
+	hsotg->gr_backup.valid = false;
+	hsotg->hr_backup.valid = false;
+	hsotg->dr_backup.valid = false;
+	hsotg->hibernated = 0;
+	hsotg->in_ppd = 0;
+	hsotg->bus_suspended = false;
+	hsotg->lx_state = DWC2_L0;
+	hsotg->flags.b.port_connect_status_change = 1;
+	hsotg->flags.b.port_suspend_change = 1;
+	set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+	spin_unlock_irqrestore(&hsotg->lock, flags);
+
+	dwc2_enable_global_interrupts(hsotg);
+
+	return 0;
+}
+
 static void dwc2_hc_init_split(struct dwc2_hsotg *hsotg,
 			       struct dwc2_host_chan *chan,
 			       struct dwc2_qtd *qtd, struct dwc2_hcd_urb *urb)
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index ef0d73077034..72214593f431 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -692,14 +692,6 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 		regulator_disable(dwc2->usb33d);
 	}
 
-	if (is_device_mode)
-		ret = dwc2_gadget_backup_critical_registers(dwc2);
-	else
-		ret = dwc2_host_backup_critical_registers(dwc2);
-
-	if (ret)
-		return ret;
-
 	if (dwc2->ll_hw_enabled &&
 	    (is_device_mode || dwc2_host_can_poweroff_phy(dwc2))) {
 		ret = __dwc2_lowlevel_hw_disable(dwc2);
@@ -709,27 +701,10 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 	return ret;
 }
 
-static int dwc2_restore_critical_registers(struct dwc2_hsotg *hsotg)
-{
-	struct dwc2_gregs_backup *gr;
-
-	gr = &hsotg->gr_backup;
-
-	if (!gr->valid) {
-		dev_err(hsotg->dev, "No valid register backup, failed to restore\n");
-		return -EINVAL;
-	}
-
-	if (gr->gintsts & GINTSTS_CURMODE_HOST)
-		return dwc2_host_restore_critical_registers(hsotg);
-
-	return dwc2_gadget_restore_critical_registers(hsotg, DWC2_RESTORE_DCTL |
-						      DWC2_RESTORE_DCFG);
-}
-
 static int __maybe_unused dwc2_resume(struct device *dev)
 {
 	struct dwc2_hsotg *dwc2 = dev_get_drvdata(dev);
+	bool need_restore;
 	int ret = 0;
 
 	if (!dwc2->ll_hw_enabled)
@@ -742,18 +717,6 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 	}
 	dwc2->phy_off_for_suspend = false;
 
-	/*
-	 * During suspend it's possible that the power domain for the
-	 * DWC2 controller is disabled and all register values get lost.
-	 * In case the GUSBCFG register is not initialized, it's clear the
-	 * registers must be restored.
-	 */
-	if (!(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK)) {
-		ret = dwc2_restore_critical_registers(dwc2);
-		if (ret)
-			return ret;
-	}
-
 	if (dwc2->params.activate_stm_id_vb_detection) {
 		unsigned long flags;
 		u32 ggpio, gotgctl;
@@ -786,8 +749,31 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 		dwc2_drd_resume(dwc2);
 	}
 
+	/*
+	 * During suspend it's possible that the power domain for the
+	 * DWC2 controller is disabled and all register values get lost.
+	 * In case the GUSBCFG register is not initialized, it's clear the
+	 * registers must be restored. So invoke here the necessary init
+	 * as done by dwc2_hcd_init() during probe.
+	 */
+	need_restore = !(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK);
+	if (need_restore) {
+		if (dwc2->dr_mode != USB_DR_MODE_PERIPHERAL) {
+			if (dwc2_is_device_mode(dwc2)) {
+				ret = dwc2_core_init(dwc2, true);
+				if (ret)
+					return ret;
+				dwc2_enable_global_interrupts(dwc2);
+			} else {
+				ret = dwc2_hcd_restore(dwc2);
+				if (ret)
+					return ret;
+			}
+		}
+	}
+
 	if (dwc2_is_device_mode(dwc2))
-		ret = dwc2_hsotg_resume(dwc2);
+		ret = dwc2_hsotg_resume(dwc2, need_restore);
 
 	return ret;
 }

-- 
2.43.0

[PATCH v4 2/6] usb: dwc2: fix a possible interrupt storm in hcd resume

[Fabrice Gasnier]

The host interrupts are re-enabled in dwc2_hcd_reinit(). So there is a
race during resume between a possible hcd IRQ, and the HW accessible bit
to be set. This can result in a bad_irq to be reported (nobody cared).

Note dwc2_hcd_reinit() "Must be called with interrupt disabled and
spinlock held." So, move dwc2_hcd_reinit() and the glocal irq enabling
inside the critical section, after HCD_FLAG_HW_ACCESSIBLE has been set.

Fixes: ef5e0eec4764 ("usb: dwc2: Add clock gating exiting flow by system resume")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Note it's been found while debugging with additional mdelay after
dwc2_hcd_reinit() call.
---
Changes in v4:
- Address Sashiko comment "usb: dwc2: unlocked dwc2_hcd_reinit() call in
  _dwc2_hcd_resume()": rather move the dwc2_hcd_reinit() and irq enabling
  under the critical section, after HCD_FLAG_HW_ACCESSIBLE has been set.
- Update commit msg.

Changes in v3:
- Introduce this patch since debug session (since sashiko comment on
  patch 1)
---
 drivers/usb/dwc2/hcd.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 3f9adfa7c630..7e4202c145f2 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4515,15 +4515,14 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 		 * the global interrupts are disabled.
 		 */
 		dwc2_core_init(hsotg, false);
-		dwc2_enable_global_interrupts(hsotg);
-		dwc2_hcd_reinit(hsotg);
 		spin_lock_irqsave(&hsotg->lock, flags);
-
 		/*
 		 * Set HW accessible bit before powering on the controller
 		 * since an interrupt may rise.
 		 */
 		set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+		dwc2_enable_global_interrupts(hsotg);
+		dwc2_hcd_reinit(hsotg);
 		break;
 	default:
 		hsotg->lx_state = DWC2_L0;

-- 
2.43.0

[PATCH v4 3/6] usb: dwc2: fix a possible interrupt storm on stm32mp1

[Fabrice Gasnier]

On stm32mp1, upon resume from low power (stop) mode, in host mode, a
port interrupt storm may be seen before the hardware is accessible.

The core is able to generate interrupts during system resume in
platform.c when calling dwc2_resume()->__dwc2_lowlevel_hw_enable(). As
the PHY and clocks become available, a resume IRQ can be seen with a
Host Port Interrupt (PrtInt).

The wakeup interrupt handler make the controller to exit power down mode,
but the HCD handler returns IRQ_NONE, due to the HW accessible flag has
been cleared in _dwc2_hcd_suspend() earlier. It ends-up with the bad_irq
handler.

So set the HCD_FLAG_HW_ACCESSIBLE flag when the resume IRQ is handled to
exit from power down mode.

Fixes: ef5e0eec4764 ("usb: dwc2: Add clock gating exiting flow by system resume")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v3:
- Set HCD_FLAG_HW_ACCESSIBLE only when dwc2_exit_partial_power_down()
  succeeds.

Changes in v2:
- Add check on hcd before setting HCD_FLAG_HW_ACCESSIBLE flag, to avoid a
  NULL pointer dereference on hcd in peripheral-only mode as raised by
  Sashiko.
- Add same handling in PPD mode as raised by Sashiko. Only handle clock
  gating and partial power down mode (PPD) as pre-existing wakeup handler
  manages these two modes. I don't have Hibernation capable hardware to
  cross-check this mode.
---
 drivers/usb/dwc2/core_intr.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 9565e6a52725..df9f6b963fb8 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -453,18 +453,25 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 		}
 	} else {
 		if (hsotg->lx_state == DWC2_L2) {
+			struct usb_hcd *hcd = hsotg->priv;
+
 			if (hsotg->in_ppd) {
 				ret = dwc2_exit_partial_power_down(hsotg, 1,
 								   true);
 				if (ret)
 					dev_err(hsotg->dev,
 						"exit partial_power_down failed\n");
+				else if (hcd)
+					set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 			}
 
 			if (hsotg->params.power_down ==
 			    DWC2_POWER_DOWN_PARAM_NONE && hsotg->bus_suspended &&
-			    !hsotg->params.no_clock_gating)
+			    !hsotg->params.no_clock_gating) {
 				dwc2_host_exit_clock_gating(hsotg, 1);
+				if (hcd)
+					set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
+			}
 
 			/*
 			 * If we've got this quirk then the PHY is stuck upon

-- 
2.43.0

[PATCH v4 4/6] usb: dwc2: hcd: fix possible successive vbus supply enables

[Fabrice Gasnier]

Relying on HPRT0_PWR value to call dwc2_vbus_supply_init() shows some
corner case resulting in vbus enable count to increase.
As consequence, dwc2_vbus_supply_exit call done when clearing
PWR bit won't disable vbus (for example upon suspend request).

Address two case that has been seen:
- In _dwc2_hcd_start(), external VBUS supply may have already been enabled,
  before entering dwc2_hcd_reinit (e.g. the controller is in host mode,
  without any device plugged).
- Upon resume from low power in case registers have been lost. In clock
  gating mode PM suspend flow doesn't disable vbus while in host mode:
  - dwc2_port_suspend() enters clock gating (it sets lx_state to DWC2_L2)
  - _dwc2_hcd_suspend() nothing more to do as already in L2.
  - _dwc2_hcd_resume() - checks HPRT0_CONNSTS. If it's not set (register
    loss) it sets HPRT0_PWR and call dwc2_vbus_supply_init().

Fix this by adding a boolean flag to balance vbus enable/disable calls
instead of relying on HPRT0_PWR value. Clean the existing checks on pwr
bit. A mutex is added as dwc2_vbus_supply_init/_exit calls can run
concurrently from hub_control and dwc2_conn_id_status_change.

Fixes: cd7cd0e6cedf ("usb: dwc2: fix unbalanced use of external vbus-supply")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v4:
- drop previous vbus_was_off boolean. Introduce vbus_supply_enabled flag
  instead and vbus_supply_lock.
- remove checks on pwr when possible as now replaced by this flag to keep
  balanced calls.

Changes in v2:
- Add a vbus_was_off boolean to check explicitly an off-on transition
  on port power, to enable vbus supply, instead of relying an XOR check
  that would be symmetric, e.g. wrongly enable it in case it gets switched
  off (1 to 0) as reported by Sashiko.
---
 drivers/usb/dwc2/core.h     |  6 ++++++
 drivers/usb/dwc2/hcd.c      | 45 ++++++++++++++++++++++++---------------------
 drivers/usb/dwc2/platform.c |  1 +
 3 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 32662cb3d3eb..c97c958b19a5 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -10,6 +10,7 @@
 
 #include <linux/acpi.h>
 #include <linux/device-id/pci.h>
+#include <linux/mutex.h>
 #include <linux/phy/phy.h>
 #include <linux/regulator/consumer.h>
 #include <linux/usb/gadget.h>
@@ -886,6 +887,9 @@ struct dwc2_hregs_backup {
  *                      removed once all SoCs support usb transceiver.
  * @supplies:           Definition of USB power supplies
  * @vbus_supply:        Regulator supplying vbus.
+ * @vbus_supply_lock:	Protects vbus_supply state and regulator operations.
+ * @vbus_supply_enabled: True if this driver holds an enable reference to
+ *			vbus_supply.
  * @usb33d:		Optional 3.3v regulator used on some stm32 devices to
  *			supply ID and VBUS detection hardware.
  * @lock:		Spinlock that protects all the driver data structures
@@ -1079,6 +1083,8 @@ struct dwc2_hsotg {
 	struct dwc2_hsotg_plat *plat;
 	struct regulator_bulk_data supplies[DWC2_NUM_SUPPLIES];
 	struct regulator *vbus_supply;
+	struct mutex vbus_supply_lock;
+	bool vbus_supply_enabled;
 	struct regulator *usb33d;
 
 	spinlock_t lock;
diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 7e4202c145f2..053d1234884b 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -133,18 +133,32 @@ static void dwc2_gusbcfg_init(struct dwc2_hsotg *hsotg)
 
 static int dwc2_vbus_supply_init(struct dwc2_hsotg *hsotg)
 {
-	if (hsotg->vbus_supply)
-		return regulator_enable(hsotg->vbus_supply);
+	int ret = 0;
 
-	return 0;
+	scoped_guard(mutex, &hsotg->vbus_supply_lock) {
+		if (hsotg->vbus_supply && !hsotg->vbus_supply_enabled) {
+			ret = regulator_enable(hsotg->vbus_supply);
+			if (!ret)
+				hsotg->vbus_supply_enabled = true;
+		}
+	}
+
+	return ret;
 }
 
 static int dwc2_vbus_supply_exit(struct dwc2_hsotg *hsotg)
 {
-	if (hsotg->vbus_supply)
-		return regulator_disable(hsotg->vbus_supply);
+	int ret = 0;
 
-	return 0;
+	scoped_guard(mutex, &hsotg->vbus_supply_lock) {
+		if (hsotg->vbus_supply && hsotg->vbus_supply_enabled) {
+			ret = regulator_disable(hsotg->vbus_supply);
+			if (!ret)
+				hsotg->vbus_supply_enabled = false;
+		}
+	}
+
+	return ret;
 }
 
 /**
@@ -3435,7 +3449,6 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
 	u32 port_status;
 	u32 speed;
 	u32 pcgctl;
-	u32 pwr;
 
 	switch (typereq) {
 	case ClearHubFeature:
@@ -3480,11 +3493,9 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
 			dev_dbg(hsotg->dev,
 				"ClearPortFeature USB_PORT_FEAT_POWER\n");
 			hprt0 = dwc2_read_hprt0(hsotg);
-			pwr = hprt0 & HPRT0_PWR;
 			hprt0 &= ~HPRT0_PWR;
 			dwc2_writel(hsotg, hprt0, HPRT0);
-			if (pwr)
-				dwc2_vbus_supply_exit(hsotg);
+			dwc2_vbus_supply_exit(hsotg);
 			break;
 
 		case USB_PORT_FEAT_INDICATOR:
@@ -3688,11 +3699,9 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
 			dev_dbg(hsotg->dev,
 				"SetPortFeature - USB_PORT_FEAT_POWER\n");
 			hprt0 = dwc2_read_hprt0(hsotg);
-			pwr = hprt0 & HPRT0_PWR;
 			hprt0 |= HPRT0_PWR;
 			dwc2_writel(hsotg, hprt0, HPRT0);
-			if (!pwr)
-				dwc2_vbus_supply_init(hsotg);
+			dwc2_vbus_supply_init(hsotg);
 			break;
 
 		case USB_PORT_FEAT_RESET:
@@ -3727,7 +3736,6 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
 			dwc2_writel(hsotg, 0, PCGCTL);
 
 			hprt0 = dwc2_read_hprt0(hsotg);
-			pwr = hprt0 & HPRT0_PWR;
 			/* Clear suspend bit if resetting from suspend state */
 			hprt0 &= ~HPRT0_SUSP;
 
@@ -3741,8 +3749,7 @@ static int dwc2_hcd_hub_control(struct dwc2_hsotg *hsotg, u16 typereq,
 				dev_dbg(hsotg->dev,
 					"In host mode, hprt0=%08x\n", hprt0);
 				dwc2_writel(hsotg, hprt0, HPRT0);
-				if (!pwr)
-					dwc2_vbus_supply_init(hsotg);
+				dwc2_vbus_supply_init(hsotg);
 			}
 
 			/* Clear reset bit in 10ms (FS/LS) or 50ms (HS) */
@@ -4344,7 +4351,6 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
 {
 	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
 	unsigned long flags;
-	u32 hprt0;
 
 	/* Turn off all host-specific interrupts */
 	dwc2_disable_host_interrupts(hsotg);
@@ -4353,7 +4359,6 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
 	synchronize_irq(hcd->irq);
 
 	spin_lock_irqsave(&hsotg->lock, flags);
-	hprt0 = dwc2_read_hprt0(hsotg);
 	/* Ensure hcd is disconnected */
 	dwc2_hcd_disconnect(hsotg, true);
 	dwc2_hcd_stop(hsotg);
@@ -4362,9 +4367,7 @@ static void _dwc2_hcd_stop(struct usb_hcd *hcd)
 	clear_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
 	spin_unlock_irqrestore(&hsotg->lock, flags);
 
-	/* keep balanced supply init/exit by checking HPRT0_PWR */
-	if (hprt0 & HPRT0_PWR)
-		dwc2_vbus_supply_exit(hsotg);
+	dwc2_vbus_supply_exit(hsotg);
 
 	usleep_range(1000, 3000);
 }
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 72214593f431..968b4ba6d814 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -471,6 +471,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		return retval;
 
 	spin_lock_init(&hsotg->lock);
+	mutex_init(&hsotg->vbus_supply_lock);
 
 	hsotg->vbus_supply = devm_regulator_get_optional(hsotg->dev, "vbus");
 	if (IS_ERR(hsotg->vbus_supply)) {

-- 
2.43.0

[PATCH v4 5/6] usb: dwc2: hcd: don't disable external vbus in system suspend

[Fabrice Gasnier]

It's not required to remove port power upon system suspend, as explained
in commit 113f86d0c302 ("usb: dwc2: Update partial power down entering by
system suspend"). It can be addressed with hub ctrl (USB_PORT_FEAT_POWER).

When PM suspend flow disables Vbus while in host mode by
calling dwc2_vbus_supply_exit()) in _dwc2_hcd_suspend() and the core has
been put into clock gating mode, it triggers a disconnect and a resume
IRQ (during suspend):
  ++Disconnect Detected Interrupt++ (Host) a_host
  ++Resume or Remote Wakeup Detected Interrupt++

That starts the wkp_timer in dwc2_host_exit_clock_gating() that will
finally resume the port, e.g. call dwc2_wakeup_detected() and set
lx_state = DWC2_L0 after ~70ms.

It races with PM suspend/resume flow. During _dwc2_hcd_resume() the port
may already be in DWC2_L0 state skipping the dwc2_vbus_supply_init(),
leaving the Vbus disabled.

So remove the disable of external Vbus in bus suspend to avoid this.

Fixes: 113f86d0c302 ("usb: dwc2: Update partial power down entering by system suspend")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v4:
- Rename the commit title and re-visit the commit message.
- Drop dwc2_vbus_supply_restore() but keep dwc2_vbus_supply_init() to
  enforce vbus is present when setting PWR bit.

Changes in v3:
- Sashiko comment (state desync leaves VBUS regulator permanently off)
  Add a dwc2_vbus_supply_restore() to check regulator enable state
  during hcd resume, if it has been turned off earlier, enable it,
  so it is kept sync'ed with PWR bit that gets set.

Changes in v2:
- rephrase commit message since Sashiko review. Hopefully it better
  explains the issue, and why HPRT0_PWR is kept synchronized with
  external vbus supply.
---
 drivers/usb/dwc2/hcd.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 053d1234884b..40450b0a565d 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4435,10 +4435,6 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 		goto skip_power_saving;
 	}
 
-	spin_unlock_irqrestore(&hsotg->lock, flags);
-	dwc2_vbus_supply_exit(hsotg);
-	spin_lock_irqsave(&hsotg->lock, flags);
-
 	/* Ask phy to be suspended */
 	if (!IS_ERR_OR_NULL(hsotg->uphy)) {
 		spin_unlock_irqrestore(&hsotg->lock, flags);

-- 
2.43.0

[PATCH v4 6/6] usb: dwc2: platform: fix stm32mp15 detection logic power management

[Fabrice Gasnier]

On stm32mp15, the STM32 detection logic can remain enabled even though
the other hardware resources are disabled during probe in peripheral
mode:
    /* Gadget code manages lowlevel hw on its own */
    if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL)
        dwc2_lowlevel_hw_disable(hsotg);

When entering system low power, the detection logic resources would then
remain enabled because dwc2_suspend() checks:
    if (!dwc2->ll_hw_enabled)
        return 0;

If the stm32mp15 enters standby, the register contents can be lost and
not restored, leaving dwc2 unusable. This can happen when no gadget
driver has been bound.

Move the STM32 detection logic enable/disable procedures into dedicated
dwc2_stm32_hw_[enable/disable]() routines and call them from the
__dwc2_lowlevel_hw_[enable/disable]() routines.

Factorize the probe code by replacing regulator_enable() and GGPIO
register access with a call to dwc2_stm32_hw_enable(). The earlier
dwc2_lowlevel_hw_enable() call during probe cannot do this because
activate_stm_id_vb_detection is only known after dwc2_init_params() runs
and the usb33d supply is acquired.

Track VBUS/ID detection state in stm32_id_vb_detection_enabled so it
is disabled only after successful enablement.

Fixes: 2b94b054ac49 ("usb: dwc2: fix hang during suspend if set as peripheral")
Fixes: a415083a11cc ("usb: dwc2: add support for STM32MP15 SoCs USB OTG HS and FS")
Cc: stable@vger.kernel.org
Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
Changes in v4:
- Track successful STM32 ID/VBUS detection enablement with
  stm32_id_vb_detection_enabled, avoiding an invalid regulator disable
  during probe error cleanup, as reported by Sashiko.

Changes in v3:
- Roll back if regulator_disable() fails in dwc2_stm32_hw_disable(), as
  reported by Sashiko.
- Add a note in response to Sashiko's v2 review.

Changes in v2:
- Fix the probe error path if devm_regulator_get() or
  dwc2_stm32_hw_enable() fails, as reported by Sashiko.
- Describe the hardware-disable error handling in the commit message.

Notes:
- If dwc2_stm32_hw_disable() fails in __dwc2_lowlevel_hw_disable(),
  there is nothing to roll back, so return immediately. Alternatively,
  the error could be ignored to continue disabling other resources.
  Similar behavior already exists for phy shutdown/exit/power_off errors;
  this could be addressed separately.
- In host mode with remote wakeup enabled and a descendant enabled to
  wake the system, phy_off_for_suspend is false. The STM32 power domain
  is not expected to turn off, so registers should not be lost.
---
 drivers/usb/dwc2/core.h     |   2 +
 drivers/usb/dwc2/platform.c | 185 ++++++++++++++++++++++++++------------------
 2 files changed, 112 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index c97c958b19a5..7de080507b5a 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -875,6 +875,7 @@ struct dwc2_hregs_backup {
  * @reset_phy_on_wake:	Quirk saying that we should assert PHY reset on a
  *			remote wakeup.
  * @phy_off_for_suspend: Status of whether we turned the PHY off at suspend.
+ * @stm32_id_vb_detection_enabled: True if STM32 ID/VBUS detection is enabled.
  * @need_phy_for_wake:	Quirk saying that we should keep the PHY on at
  *			suspend if we need USB to wake us up.
  * @frame_number:       Frame number read from the core. For both device
@@ -1076,6 +1077,7 @@ struct dwc2_hsotg {
 	unsigned int reset_phy_on_wake:1;
 	unsigned int need_phy_for_wake:1;
 	unsigned int phy_off_for_suspend:1;
+	bool stm32_id_vb_detection_enabled;
 	u16 frame_number;
 
 	struct phy *phy;
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 968b4ba6d814..7aa8d7c58e75 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -91,6 +91,95 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
 	return 0;
 }
 
+static void dwc2_stm32_hw_activate_id_vb_detection(struct dwc2_hsotg *hsotg)
+{
+	unsigned long flags;
+	u32 ggpio, gotgctl;
+
+	ggpio = dwc2_readl(hsotg, GGPIO);
+	ggpio |= GGPIO_STM32_OTG_GCCFG_IDEN;
+	ggpio |= GGPIO_STM32_OTG_GCCFG_VBDEN;
+	dwc2_writel(hsotg, ggpio, GGPIO);
+
+	/* ID/VBUS detection startup time */
+	usleep_range(5000, 7000);
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+	/* Remove overrides when resuming from the disable */
+	gotgctl = dwc2_readl(hsotg, GOTGCTL);
+	gotgctl &= ~GOTGCTL_DBNCE_FLTR_BYPASS;
+	gotgctl &= ~(GOTGCTL_BVALOEN | GOTGCTL_AVALOEN |
+		     GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL);
+	dwc2_writel(hsotg, gotgctl, GOTGCTL);
+	spin_unlock_irqrestore(&hsotg->lock, flags);
+}
+
+static int dwc2_stm32_hw_enable(struct dwc2_hsotg *hsotg)
+{
+	int ret;
+
+	/* params are only known, once dwc2_init_params() has been probed */
+	if (!hsotg->params.activate_stm_id_vb_detection)
+		return 0;
+
+	if (hsotg->stm32_id_vb_detection_enabled)
+		return 0;
+
+	ret = regulator_enable(hsotg->usb33d);
+	if (ret)
+		return ret;
+
+	dwc2_stm32_hw_activate_id_vb_detection(hsotg);
+	hsotg->stm32_id_vb_detection_enabled = true;
+
+	return 0;
+}
+
+static int dwc2_stm32_hw_disable(struct dwc2_hsotg *hsotg)
+{
+	unsigned long flags;
+	u32 ggpio, gotgctl;
+	int ret;
+
+	if (!hsotg->stm32_id_vb_detection_enabled)
+		return 0;
+
+	/*
+	 * Need to force the mode to the current mode to avoid Mode
+	 * Mismatch Interrupt when ID detection will be disabled.
+	 */
+	dwc2_force_mode(hsotg, !dwc2_is_device_mode(hsotg));
+
+	spin_lock_irqsave(&hsotg->lock, flags);
+	gotgctl = dwc2_readl(hsotg, GOTGCTL);
+	/* bypass debounce filter, enable overrides */
+	gotgctl |= GOTGCTL_DBNCE_FLTR_BYPASS;
+	gotgctl |= GOTGCTL_BVALOEN | GOTGCTL_AVALOEN;
+	/* Force A / B session if needed */
+	if (gotgctl & GOTGCTL_ASESVLD)
+		gotgctl |= GOTGCTL_AVALOVAL;
+	if (gotgctl & GOTGCTL_BSESVLD)
+		gotgctl |= GOTGCTL_BVALOVAL;
+	dwc2_writel(hsotg, gotgctl, GOTGCTL);
+	spin_unlock_irqrestore(&hsotg->lock, flags);
+
+	ggpio = dwc2_readl(hsotg, GGPIO);
+	ggpio &= ~GGPIO_STM32_OTG_GCCFG_IDEN;
+	ggpio &= ~GGPIO_STM32_OTG_GCCFG_VBDEN;
+	dwc2_writel(hsotg, ggpio, GGPIO);
+
+	ret = regulator_disable(hsotg->usb33d);
+	if (ret) {
+		dwc2_stm32_hw_activate_id_vb_detection(hsotg);
+		dwc2_force_dr_mode(hsotg);
+		return ret;
+	}
+
+	hsotg->stm32_id_vb_detection_enabled = false;
+
+	return 0;
+}
+
 static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
 {
 	struct platform_device *pdev = to_platform_device(hsotg->dev);
@@ -129,8 +218,22 @@ static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
 	if (ret)
 		goto err_dis_clk;
 
+	ret = dwc2_stm32_hw_enable(hsotg);
+	if (ret)
+		goto err_dis_phy;
+
 	return 0;
 
+err_dis_phy:
+	if (hsotg->uphy) {
+		usb_phy_shutdown(hsotg->uphy);
+	} else if (hsotg->plat && hsotg->plat->phy_exit) {
+		hsotg->plat->phy_exit(pdev, hsotg->plat->phy_type);
+	} else {
+		phy_power_off(hsotg->phy);
+		phy_exit(hsotg->phy);
+	}
+
 err_dis_clk:
 	if (hsotg->clk)
 		clk_disable_unprepare(hsotg->clk);
@@ -166,6 +269,10 @@ static int __dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg)
 	struct platform_device *pdev = to_platform_device(hsotg->dev);
 	int ret = 0;
 
+	ret = dwc2_stm32_hw_disable(hsotg);
+	if (ret)
+		return ret;
+
 	if (hsotg->uphy) {
 		usb_phy_shutdown(hsotg->uphy);
 	} else if (hsotg->plat && hsotg->plat->phy_exit) {
@@ -346,9 +453,6 @@ static void dwc2_driver_remove(struct platform_device *dev)
 
 	dwc2_drd_exit(hsotg);
 
-	if (hsotg->params.activate_stm_id_vb_detection)
-		regulator_disable(hsotg->usb33d);
-
 	if (hsotg->ll_hw_enabled)
 		dwc2_lowlevel_hw_disable(hsotg);
 }
@@ -542,33 +646,23 @@ static int dwc2_driver_probe(struct platform_device *dev)
 		goto error;
 
 	if (hsotg->params.activate_stm_id_vb_detection) {
-		u32 ggpio;
-
 		hsotg->usb33d = devm_regulator_get(hsotg->dev, "usb33d");
 		if (IS_ERR(hsotg->usb33d)) {
 			retval = PTR_ERR(hsotg->usb33d);
 			dev_err_probe(hsotg->dev, retval, "failed to request usb33d supply\n");
 			goto error;
 		}
-		retval = regulator_enable(hsotg->usb33d);
+		retval = dwc2_stm32_hw_enable(hsotg);
 		if (retval) {
-			dev_err_probe(hsotg->dev, retval, "failed to enable usb33d supply\n");
+			dev_err_probe(hsotg->dev, retval, "dwc2_stm32_hw_enable failed\n");
 			goto error;
 		}
-
-		ggpio = dwc2_readl(hsotg, GGPIO);
-		ggpio |= GGPIO_STM32_OTG_GCCFG_IDEN;
-		ggpio |= GGPIO_STM32_OTG_GCCFG_VBDEN;
-		dwc2_writel(hsotg, ggpio, GGPIO);
-
-		/* ID/VBUS detection startup time */
-		usleep_range(5000, 7000);
 	}
 
 	retval = dwc2_drd_init(hsotg);
 	if (retval) {
 		dev_err_probe(hsotg->dev, retval, "failed to initialize dual-role\n");
-		goto error_init;
+		goto error;
 	}
 
 	if (hsotg->dr_mode != USB_DR_MODE_HOST) {
@@ -638,9 +732,6 @@ static int dwc2_driver_probe(struct platform_device *dev)
 error_drd:
 	dwc2_drd_exit(hsotg);
 
-error_init:
-	if (hsotg->params.activate_stm_id_vb_detection)
-		regulator_disable(hsotg->usb33d);
 error:
 	if (hsotg->ll_hw_enabled)
 		dwc2_lowlevel_hw_disable(hsotg);
@@ -662,37 +753,6 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
 
 	dwc2_drd_suspend(dwc2);
 
-	if (dwc2->params.activate_stm_id_vb_detection) {
-		unsigned long flags;
-		u32 ggpio, gotgctl;
-
-		/*
-		 * Need to force the mode to the current mode to avoid Mode
-		 * Mismatch Interrupt when ID detection will be disabled.
-		 */
-		dwc2_force_mode(dwc2, !is_device_mode);
-
-		spin_lock_irqsave(&dwc2->lock, flags);
-		gotgctl = dwc2_readl(dwc2, GOTGCTL);
-		/* bypass debounce filter, enable overrides */
-		gotgctl |= GOTGCTL_DBNCE_FLTR_BYPASS;
-		gotgctl |= GOTGCTL_BVALOEN | GOTGCTL_AVALOEN;
-		/* Force A / B session if needed */
-		if (gotgctl & GOTGCTL_ASESVLD)
-			gotgctl |= GOTGCTL_AVALOVAL;
-		if (gotgctl & GOTGCTL_BSESVLD)
-			gotgctl |= GOTGCTL_BVALOVAL;
-		dwc2_writel(dwc2, gotgctl, GOTGCTL);
-		spin_unlock_irqrestore(&dwc2->lock, flags);
-
-		ggpio = dwc2_readl(dwc2, GGPIO);
-		ggpio &= ~GGPIO_STM32_OTG_GCCFG_IDEN;
-		ggpio &= ~GGPIO_STM32_OTG_GCCFG_VBDEN;
-		dwc2_writel(dwc2, ggpio, GGPIO);
-
-		regulator_disable(dwc2->usb33d);
-	}
-
 	if (dwc2->ll_hw_enabled &&
 	    (is_device_mode || dwc2_host_can_poweroff_phy(dwc2))) {
 		ret = __dwc2_lowlevel_hw_disable(dwc2);
@@ -718,31 +778,6 @@ static int __maybe_unused dwc2_resume(struct device *dev)
 	}
 	dwc2->phy_off_for_suspend = false;
 
-	if (dwc2->params.activate_stm_id_vb_detection) {
-		unsigned long flags;
-		u32 ggpio, gotgctl;
-
-		ret = regulator_enable(dwc2->usb33d);
-		if (ret)
-			return ret;
-
-		ggpio = dwc2_readl(dwc2, GGPIO);
-		ggpio |= GGPIO_STM32_OTG_GCCFG_IDEN;
-		ggpio |= GGPIO_STM32_OTG_GCCFG_VBDEN;
-		dwc2_writel(dwc2, ggpio, GGPIO);
-
-		/* ID/VBUS detection startup time */
-		usleep_range(5000, 7000);
-
-		spin_lock_irqsave(&dwc2->lock, flags);
-		gotgctl = dwc2_readl(dwc2, GOTGCTL);
-		gotgctl &= ~GOTGCTL_DBNCE_FLTR_BYPASS;
-		gotgctl &= ~(GOTGCTL_BVALOEN | GOTGCTL_AVALOEN |
-			     GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL);
-		dwc2_writel(dwc2, gotgctl, GOTGCTL);
-		spin_unlock_irqrestore(&dwc2->lock, flags);
-	}
-
 	if (!dwc2->role_sw) {
 		/* Need to restore FORCEDEVMODE/FORCEHOSTMODE */
 		dwc2_force_dr_mode(dwc2);

-- 
2.43.0

Re: [PATCH v4 1/6] usb: dwc2: fix recovery after PM domain off

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] usb: dwc2: premature IRQ enable and incomplete cleanup in
  device resume
- [Low] usb: dwc2: stale commit message note on HPRT0_CONNSTS in host
  resume
--

commit 3fc088190fe8f18420841b384b9d96f416a409a2
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

usb: dwc2: fix recovery after PM domain off

This commit removes the critical register backup from dwc2_suspend() and
instead reinitializes the DWC2 controller, HCD, and gadget state during
dwc2_resume() when GUSBCFG.TOUTCAL indicates that registers were lost.

> Host resume observes HPRT0_CONNSTS cleared after register loss, so it
> does not skip the recovery path. Ordinary resume and USB-reset behavior
> remain unchanged.

[Severity: Low]
Is this note about HPRT0_CONNSTS still accurate?

dwc2_hcd_restore() now sets hsotg->lx_state = DWC2_L0 during platform
resume, so when _dwc2_hcd_resume() runs later it returns early at the
hsotg->lx_state != DWC2_L2 check before reading HPRT0:

drivers/usb/dwc2/hcd.c:_dwc2_hcd_resume() {
    ...
	if (hsotg->lx_state != DWC2_L2)
		goto unlock;

	hprt0 = dwc2_read_hprt0(hsotg);
    ...
	if (hprt0 & HPRT0_CONNSTS) {
		hsotg->lx_state = DWC2_L0;
		goto unlock;
	}
    ...
}

> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 6c8a9f05fff00..225442471aa95 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -5200,11 +5200,12 @@ int dwc2_hsotg_suspend(struct dwc2_hsotg *hsotg)
>  	return 0;
>  }
>  
> -int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg)
> +int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg, bool need_restore)
>  {
>  	unsigned long flags;
> +	int ep;
>  
> -	if (hsotg->lx_state == DWC2_L2)
> +	if (!need_restore && hsotg->lx_state == DWC2_L2)
>  		return 0;
>  
>  	if (hsotg->driver) {
> @@ -5212,6 +5213,19 @@ int dwc2_hsotg_resume(struct dwc2_hsotg *hsotg)
>  			 hsotg->driver->driver.name);
>  
>  		spin_lock_irqsave(&hsotg->lock, flags);
> +		if (need_restore) {
> +			hsotg->hibernated = 0;
> +			hsotg->in_ppd = 0;
> +			hsotg->bus_suspended = false;
> +			dwc2_hsotg_disconnect(hsotg);
> +			for (ep = 1; ep < hsotg->num_of_eps; ep++) {
> +				if (hsotg->eps_in[ep])
> +					dwc2_hsotg_ep_disable(&hsotg->eps_in[ep]->ep);
> +				if (hsotg->eps_out[ep])
> +					dwc2_hsotg_ep_disable(&hsotg->eps_out[ep]->ep);
> +			}
> +			hsotg->gadget.speed = USB_SPEED_UNKNOWN;
> +		}
>  		dwc2_hsotg_core_init_disconnected(hsotg, false);
>  		if (hsotg->enabled) {
>  			/* Enable ACG feature in device mode,if supported */

[ ... ]

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index ef0d730770347..72214593f4312 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c

[ ... ]

> @@ -786,8 +749,31 @@ static int __maybe_unused dwc2_resume(struct device *dev)
>  		dwc2_drd_resume(dwc2);
>  	}
>  
> +	/*
> +	 * During suspend it's possible that the power domain for the
> +	 * DWC2 controller is disabled and all register values get lost.
> +	 * In case the GUSBCFG register is not initialized, it's clear the
> +	 * registers must be restored. So invoke here the necessary init
> +	 * as done by dwc2_hcd_init() during probe.
> +	 */
> +	need_restore = !(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK);
> +	if (need_restore) {
> +		if (dwc2->dr_mode != USB_DR_MODE_PERIPHERAL) {
> +			if (dwc2_is_device_mode(dwc2)) {
> +				ret = dwc2_core_init(dwc2, true);
> +				if (ret)
> +					return ret;
> +				dwc2_enable_global_interrupts(dwc2);
> +			} else {
> +				ret = dwc2_hcd_restore(dwc2);
> +				if (ret)
> +					return ret;
> +			}
> +		}
> +	}
> +
>  	if (dwc2_is_device_mode(dwc2))
> -		ret = dwc2_hsotg_resume(dwc2);
> +		ret = dwc2_hsotg_resume(dwc2, need_restore);
>  
>  	return ret;
>  }

[Severity: Medium]
Can enabling global interrupts here race with the state cleanup in
dwc2_hsotg_resume()?

dwc2_core_init() unmasks GINTMSK_COMMON (including GINTSTS_USBSUSP and
GINTSTS_WKUPINT), and dwc2_enable_global_interrupts() sets
GAHBCFG_GLBL_INTR_EN before dwc2_hsotg_resume() takes hsotg->lock and
clears hsotg->hibernated, hsotg->in_ppd, hsotg->bus_suspended, and
hsotg->connected.  In addition, dwc2_hsotg_disconnect() drops and
reacquires hsotg->lock before dwc2_hsotg_core_init_disconnected()
reinitializes the device registers:

dwc2_resume()
  dwc2_core_init(dwc2, true)
  dwc2_enable_global_interrupts(dwc2)
  <hardirq>
    dwc2_handle_common_intr()
      ...
        dwc2_handle_usb_suspend_intr()
          dwc2_gadget_enter_clock_gating()  <-- sees connected == 1, gates clk
  dwc2_hsotg_resume(dwc2, true)
    spin_lock_irqsave(&hsotg->lock, flags)
    ...
    dwc2_hsotg_ep_disable()
      dwc2_readl(hsotg, DIEPCTL(index))     <-- hangs with clock gated

If GINTSTS_USBSUSP fires while hsotg->connected is still 1 and DSTS_SUSPSTS
is set after reset, dwc2_handle_usb_suspend_intr() enters clock gating or
partial power down right before dwc2_hsotg_ep_disable() accesses endpoint
registers, hanging the platform.  Similarly, an early wakeup or common
interrupt while hsotg->hibernated or hsotg->in_ppd is still 1 will run the
hibernation or partial-power-down exit paths using stale register backups.

Also, unlike dwc2_hcd_restore(), the device-mode restore path does not
clear gr_backup.valid, dr_backup.valid, or hr_backup.valid, and if
hsotg->driver is NULL, dwc2_hsotg_resume() skips the need_restore cleanup
entirely, leaving hibernated, in_ppd, bus_suspended, and lx_state stale.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-dwc2-low-power-fixes-stm32-v4-0-08a8c962a09e@foss.st.com?part=1

Re: [PATCH v4 2/6] usb: dwc2: fix a possible interrupt storm in hcd resume

[sashiko-bot]

> The host interrupts are re-enabled in dwc2_hcd_reinit(). So there is a
> race during resume between a possible hcd IRQ, and the HW accessible bit
> to be set. This can result in a bad_irq to be reported (nobody cared).
> 
> Note dwc2_hcd_reinit() "Must be called with interrupt disabled and
> spinlock held." So, move dwc2_hcd_reinit() and the glocal irq enabling
> inside the critical section, after HCD_FLAG_HW_ACCESSIBLE has been set.
> 
> Fixes: ef5e0eec4764 ("usb: dwc2: Add clock gating exiting flow by system resume")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-dwc2-low-power-fixes-stm32-v4-0-08a8c962a09e@foss.st.com?part=2

Re: [PATCH v4 3/6] usb: dwc2: fix a possible interrupt storm on stm32mp1

[sashiko-bot]

> On stm32mp1, upon resume from low power (stop) mode, in host mode, a
> port interrupt storm may be seen before the hardware is accessible.
> 
> The core is able to generate interrupts during system resume in
> platform.c when calling dwc2_resume()->__dwc2_lowlevel_hw_enable(). As
> the PHY and clocks become available, a resume IRQ can be seen with a
> Host Port Interrupt (PrtInt).
> 
> The wakeup interrupt handler make the controller to exit power down mode,
> but the HCD handler returns IRQ_NONE, due to the HW accessible flag has
> been cleared in _dwc2_hcd_suspend() earlier. It ends-up with the bad_irq
> handler.
> 
> So set the HCD_FLAG_HW_ACCESSIBLE flag when the resume IRQ is handled to
> exit from power down mode.
> 
> Fixes: ef5e0eec4764 ("usb: dwc2: Add clock gating exiting flow by system resume")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-dwc2-low-power-fixes-stm32-v4-0-08a8c962a09e@foss.st.com?part=3

Re: [PATCH v4 4/6] usb: dwc2: hcd: fix possible successive vbus supply enables

[sashiko-bot]

> Relying on HPRT0_PWR value to call dwc2_vbus_supply_init() shows some
> corner case resulting in vbus enable count to increase.
> As consequence, dwc2_vbus_supply_exit call done when clearing
> PWR bit won't disable vbus (for example upon suspend request).
> 
> Address two case that has been seen:
> - In _dwc2_hcd_start(), external VBUS supply may have already been enabled,
>   before entering dwc2_hcd_reinit (e.g. the controller is in host mode,
>   without any device plugged).
> - Upon resume from low power in case registers have been lost. In clock
>   gating mode PM suspend flow doesn't disable vbus while in host mode:
>   - dwc2_port_suspend() enters clock gating (it sets lx_state to DWC2_L2)
>   - _dwc2_hcd_suspend() nothing more to do as already in L2.
>   - _dwc2_hcd_resume() - checks HPRT0_CONNSTS. If it's not set (register
>     loss) it sets HPRT0_PWR and call dwc2_vbus_supply_init().
> 
> Fix this by adding a boolean flag to balance vbus enable/disable calls
> instead of relying on HPRT0_PWR value. Clean the existing checks on pwr
> bit. A mutex is added as dwc2_vbus_supply_init/_exit calls can run
> concurrently from hub_control and dwc2_conn_id_status_change.
> 
> Fixes: cd7cd0e6cedf ("usb: dwc2: fix unbalanced use of external vbus-supply")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-dwc2-low-power-fixes-stm32-v4-0-08a8c962a09e@foss.st.com?part=4

Re: [PATCH v4 5/6] usb: dwc2: hcd: don't disable external vbus in system suspend

[sashiko-bot]

> It's not required to remove port power upon system suspend, as explained
> in commit 113f86d0c302 ("usb: dwc2: Update partial power down entering by
> system suspend"). It can be addressed with hub ctrl (USB_PORT_FEAT_POWER).
> 
> When PM suspend flow disables Vbus while in host mode by
> calling dwc2_vbus_supply_exit()) in _dwc2_hcd_suspend() and the core has
> been put into clock gating mode, it triggers a disconnect and a resume
> IRQ (during suspend):
>   ++Disconnect Detected Interrupt++ (Host) a_host
>   ++Resume or Remote Wakeup Detected Interrupt++
> 
> That starts the wkp_timer in dwc2_host_exit_clock_gating() that will
> finally resume the port, e.g. call dwc2_wakeup_detected() and set
> lx_state = DWC2_L0 after ~70ms.
> 
> It races with PM suspend/resume flow. During _dwc2_hcd_resume() the port
> may already be in DWC2_L0 state skipping the dwc2_vbus_supply_init(),
> leaving the Vbus disabled.
> 
> So remove the disable of external Vbus in bus suspend to avoid this.
> 
> Fixes: 113f86d0c302 ("usb: dwc2: Update partial power down entering by system suspend")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-dwc2-low-power-fixes-stm32-v4-0-08a8c962a09e@foss.st.com?part=5

Re: [PATCH v4 6/6] usb: dwc2: platform: fix stm32mp15 detection logic power management

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: dwc2: missing forced mode restore in
  dwc2_stm32_hw_enable()
--

commit faf6843d2e18eaa77a4b2f6b54f7e23489971e73
Author: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

usb: dwc2: platform: fix stm32mp15 detection logic power management

This commit moves the STM32 ID/VBUS detection enable and disable logic into
dedicated helpers called from the low-level hardware enable/disable paths,
and tracks its state in stm32_id_vb_detection_enabled.

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 968b4ba6d8141..7aa8d7c58e75f 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -91,6 +91,95 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
>  	return 0;
>  }
>  
> +static void dwc2_stm32_hw_activate_id_vb_detection(struct dwc2_hsotg *hsotg)
> +{
> +	unsigned long flags;
> +	u32 ggpio, gotgctl;
> +
> +	ggpio = dwc2_readl(hsotg, GGPIO);
> +	ggpio |= GGPIO_STM32_OTG_GCCFG_IDEN;
> +	ggpio |= GGPIO_STM32_OTG_GCCFG_VBDEN;
> +	dwc2_writel(hsotg, ggpio, GGPIO);
> +
> +	/* ID/VBUS detection startup time */
> +	usleep_range(5000, 7000);
> +
> +	spin_lock_irqsave(&hsotg->lock, flags);
> +	/* Remove overrides when resuming from the disable */
> +	gotgctl = dwc2_readl(hsotg, GOTGCTL);
> +	gotgctl &= ~GOTGCTL_DBNCE_FLTR_BYPASS;
> +	gotgctl &= ~(GOTGCTL_BVALOEN | GOTGCTL_AVALOEN |
> +		     GOTGCTL_BVALOVAL | GOTGCTL_AVALOVAL);
> +	dwc2_writel(hsotg, gotgctl, GOTGCTL);
> +	spin_unlock_irqrestore(&hsotg->lock, flags);
> +}
> +
> +static int dwc2_stm32_hw_enable(struct dwc2_hsotg *hsotg)
> +{
> +	int ret;
> +
> +	/* params are only known, once dwc2_init_params() has been probed */
> +	if (!hsotg->params.activate_stm_id_vb_detection)
> +		return 0;
> +
> +	if (hsotg->stm32_id_vb_detection_enabled)
> +		return 0;
> +
> +	ret = regulator_enable(hsotg->usb33d);
> +	if (ret)
> +		return ret;
> +
> +	dwc2_stm32_hw_activate_id_vb_detection(hsotg);
> +	hsotg->stm32_id_vb_detection_enabled = true;
> +
> +	return 0;
> +}

[Severity: Medium]
Does dwc2_stm32_hw_enable() leave GUSBCFG_FORCEDEVMODE cleared when a gadget
driver is bound after resuming from standby in peripheral mode?

When the system enters standby in USB_DR_MODE_PERIPHERAL with no gadget
driver bound, ll_hw_enabled is false, so dwc2_resume() returns early without
calling dwc2_force_dr_mode() and the controller registers reset during
standby.  When a gadget driver is later bound, dwc2_hsotg_udc_start() ->
dwc2_lowlevel_hw_enable() -> dwc2_stm32_hw_enable() enables
GGPIO_STM32_OTG_GCCFG_IDEN and clears the GOTGCTL overrides without
restoring GUSBCFG_FORCEDEVMODE.

If the ID pin is low, the OTG controller switches into host mode instead of
staying forced into device mode, breaking peripheral operation and hitting
WARN_ON(host && hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) in
dwc2_force_mode() on the next dwc2_stm32_hw_disable() call.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-dwc2-low-power-fixes-stm32-v4-0-08a8c962a09e@foss.st.com?part=6