[PATCH v2 0/5] usb: dwc2: fix low power issues found on stm32mp1

[PATCH v2 0/5] 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 two 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 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 (5):
      usb: dwc2: fix recovery after PM domain off
      usb: dwc2: fix a possible interrupt storm on stm32mp1
      usb: dwc2: hcd: fix possible successive vbus supply enables
      usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume
      usb: dwc2: platform: fix stm32mp15 detection logic power management

 drivers/usb/dwc2/core.c      |   1 -
 drivers/usb/dwc2/core.h      |   2 -
 drivers/usb/dwc2/core_intr.c |   9 +-
 drivers/usb/dwc2/hcd.c       |  12 +--
 drivers/usb/dwc2/platform.c  | 228 ++++++++++++++++++++++---------------------
 5 files changed, 128 insertions(+), 124 deletions(-)
---
base-commit: b634294f400bc2500de5acd9c4961649c8d904c9
change-id: 20261007-dwc2-low-power-fixes-stm32-0c1e5e68b252

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

[PATCH v2 1/5] usb: dwc2: fix recovery after PM domain off

[Fabrice Gasnier]

The commit ba6e518d136b ("usb: dwc2: Implement recovery after PM domain
off") introduces a regression on stm32mp1x, which freeze the platform
when going to low power.

When the controller has been put into clock gating, reading some of
the registers to backup them from the suspend() routine in platform.c
lead to platform hang.
On stm32, the issue is systematic in host mode:
- _dwc2_hcd_suspend() calls dwc2_host_enter_clock_gating() every time.
- dwc2_suspend() calls dwc2_host_backup_critical_registers() that freezes.
This may happen in device mode too, if the host has suspended the bus.
Then clock gating has been entered, before calling dwc2_suspend().

So rather than backing-up registers that can lead to freeze when in
clock gating mode upon suspend (to restore then later):
- keep the canary detection logic (GUSBCFG TOUCAL)
- move it after the host/device mode has been restored, e.g.
  after dwc2_force_dr_mode() or drd resume code
- re-initialize the core as done during probe, when calling
  dwc2_hcd_init()
- re-enable the global interrupts.

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 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     |  2 --
 drivers/usb/dwc2/platform.c | 57 +++++++++++++++------------------------------
 3 files changed, 19 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index f375c5185bfe..368da20ae14d 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 767251aa1aa3..1b6441164c6c 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;
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index ef0d73077034..5aa552672078 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,24 +701,6 @@ 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);
@@ -742,18 +716,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,6 +748,25 @@ 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.
+	 */
+	if (!(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK)) {
+		if (dwc2->dr_mode != USB_DR_MODE_PERIPHERAL) {
+			ret = dwc2_core_init(dwc2, true);
+			if (ret)
+				return ret;
+			dwc2_enable_global_interrupts(dwc2);
+		}
+		/* Also ensure device mode resume takes place in dwc2_hsotg_resume() */
+		if (dwc2_is_device_mode(dwc2))
+			dwc2->lx_state = DWC2_L0;
+	}
+
 	if (dwc2_is_device_mode(dwc2))
 		ret = dwc2_hsotg_resume(dwc2);
 

-- 
2.43.0

Re: [PATCH v2 1/5] usb: dwc2: fix recovery after PM domain off

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: dwc2: missing host registers init on resume
- [Medium] usb: dwc2: race condition in dwc2_resume between global interrupt enable and lx_state update
--

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

usb: dwc2: fix recovery after PM domain off

This commit addresses a freeze during platform low power transitions
by modifying how registers are recovered after the PM domain turns off.
Instead of backing up registers that cause hangs during clock gating,
it re-initializes the core via dwc2_core_init() and restores the canary
detection logic later in the resume sequence.

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index ef0d730770347..5aa5526720783 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
[ ... ]
> @@ -786,6 +748,25 @@ 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.
> +	 */
> +	if (!(dwc2_readl(dwc2, GUSBCFG) & GUSBCFG_TOUTCAL_MASK)) {
> +		if (dwc2->dr_mode != USB_DR_MODE_PERIPHERAL) {
> +			ret = dwc2_core_init(dwc2, true);
> +			if (ret)
> +				return ret;

[Severity: High]
Does this adequately initialize the host registers? dwc2_core_init()
only performs global initialization and doesn't configure host-specific
registers like HCFG, FIFO sizing, or the GUSBCFG_TOUTCAL canary.

While _dwc2_hcd_resume() does call dwc2_core_host_init() via
dwc2_hcd_reinit(), it unconditionally skips this step if the PHY
detects a connected device:

drivers/usb/dwc2/hcd.c:_dwc2_hcd_resume() {
    ...
    hprt0 = dwc2_read_hprt0(hsotg);

    /*
     * Added port connection status checking which prevents exiting from
     * Partial Power Down mode from _dwc2_hcd_resume() if not in Partial
     * Power Down mode.
     */
    if (hprt0 & HPRT0_CONNSTS) {
        set_bit(HCD_FLAG_HW_ACCESSIBLE, &hcd->flags);
        hsotg->lx_state = DWC2_L0;
        goto unlock;
    }
    ...
}

Will this cause host registers to never be initialized if a device is
connected during resume, potentially breaking host mode?

Also, since the GUSBCFG_TOUTCAL canary is never restored to 7, will
subsequent resumes repeatedly misinterpret the canary as missing and trigger
redundant resets?

> +			dwc2_enable_global_interrupts(dwc2);
> +		}
> +		/* Also ensure device mode resume takes place in dwc2_hsotg_resume() */
> +		if (dwc2_is_device_mode(dwc2))
> +			dwc2->lx_state = DWC2_L0;

[Severity: Medium]
Can this introduce a race condition? Global interrupts are unmasked before
the lockless update of lx_state to DWC2_L0.

If an interrupt fires immediately, the handler (dwc2_handle_common_intr)
could take the lock and read a stale DWC2_L2 state for dwc2->lx_state.
Could this lead to incorrect state transitions or a controller freeze?

> +	}
> +
>  	if (dwc2_is_device_mode(dwc2))

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-dwc2-low-power-fixes-stm32-v2-0-79865a4f1b63@foss.st.com?part=1

[PATCH v2 2/5] 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 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..f6d5983d468a 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");
+				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

Re: [PATCH v2 2/5] 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. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-dwc2-low-power-fixes-stm32-v2-0-79865a4f1b63@foss.st.com?part=2

[PATCH v2 3/5] usb: dwc2: hcd: fix possible successive vbus supply enables

[Fabrice Gasnier]

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).
This results in vbus enable count to increase every time this occurs.
As consequence, dwc2_vbus_supply_exit won't disable vbus upon suspend
request.
Fix this by reading PWR bit before and after dwc2_hcd_reinit() call.

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 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/hcd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index cd0dc876b421..05d5d7265e42 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4249,6 +4249,7 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
 	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
 	struct usb_bus *bus = hcd_to_bus(hcd);
 	unsigned long flags;
+	bool vbus_was_off;
 	u32 hprt0;
 	int ret;
 
@@ -4264,11 +4265,13 @@ static int _dwc2_hcd_start(struct usb_hcd *hcd)
 		return 0;	/* why 0 ?? */
 	}
 
+	vbus_was_off = !(dwc2_read_hprt0(hsotg) & HPRT0_PWR);
+
 	dwc2_hcd_reinit(hsotg);
 
 	hprt0 = dwc2_read_hprt0(hsotg);
 	/* Has vbus power been turned on in dwc2_core_host_init ? */
-	if (hprt0 & HPRT0_PWR) {
+	if (vbus_was_off && (hprt0 & HPRT0_PWR)) {
 		/* Enable external vbus supply before resuming root hub */
 		spin_unlock_irqrestore(&hsotg->lock, flags);
 		ret = dwc2_vbus_supply_init(hsotg);

-- 
2.43.0

Re: [PATCH v2 3/5] usb: dwc2: hcd: fix possible successive vbus supply enables

[sashiko-bot]

> 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).
> This results in vbus enable count to increase every time this occurs.
> As consequence, dwc2_vbus_supply_exit won't disable vbus upon suspend
> request.
> Fix this by reading PWR bit before and after dwc2_hcd_reinit() call.
> 
> 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. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-dwc2-low-power-fixes-stm32-v2-0-79865a4f1b63@foss.st.com?part=3

[PATCH v2 4/5] usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume

[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).

PM suspend flow doesn't disable vbus while in host mode (neither by
clearing HPRT0_PWR nor by calling dwc2_vbus_supply_exit()). In sequence:
- dwc2_port_suspend() - enters clock gating (set lx_state to DWC2_L2)
- _dwc2_hcd_suspend() - nothing more to do as already in L2.

PM resume flow after wakeup (two cases as registers may have been lost)
- _dwc2_hcd_resume() - checks HPRT0_CONNSTS
1 - if set (registers not lost) - nothing more to do.
  - followed by dwc2_port_resume() - exits clock gating (sets lx_state to
    DWC2_L0)
2 - if NOT set - exits clock gating, re-initializes the controller,
    restores HPRT0_PWR and calls dwc2_vbus_supply_init() unbalanced.

For the 2nd case, it's unbalanced / enabled each time it happens: the
vbus regulator enable count constantly increase. As a result, vbus
won't be turned off in case the role changes later to device mode.

So remove the disable/enable of external vbus in bus suspend/resume
routines to avoid this. Moreover, setting/restoring HPRT0_PWR for the
2nd case will simply make the vbus regulator enable count to be aligned
with it.

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 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 | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 05d5d7265e42..bedd1300e62e 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4388,10 +4388,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);
@@ -4500,10 +4496,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 		spin_lock_irqsave(&hsotg->lock, flags);
 	}
 
-	/* Enable external vbus supply after resuming the port. */
 	spin_unlock_irqrestore(&hsotg->lock, flags);
-	dwc2_vbus_supply_init(hsotg);
-
 	/* Wait for controller to correctly update D+/D- level */
 	usleep_range(3000, 5000);
 	spin_lock_irqsave(&hsotg->lock, flags);

-- 
2.43.0

Re: [PATCH v2 4/5] usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: dwc2: state desync leaves VBUS regulator permanently off
--

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

usb: dwc2: hcd: don't disable/enable external vbus by system suspend/resume

This commit removes the disable and enable calls for the external vbus in
the bus suspend and resume routines. It aims to avoid an unbalanced regulator
enable count that occurs when registers are lost during system suspend.

> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index c966c08cd7d82..263713ebd4801 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c

[ ... ]

> @@ -4500,10 +4496,7 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>  		spin_lock_irqsave(&hsotg->lock, flags);
>  	}
>  
> -	/* Enable external vbus supply after resuming the port. */
>  	spin_unlock_irqrestore(&hsotg->lock, flags);
> -	dwc2_vbus_supply_init(hsotg);
> -

[Severity: Medium]
Will this leave the physical regulator permanently off if the port power was 
cleared before system suspend?

If the port is powered off by the hub driver before suspend, the external
regulator count is 0 and HPRT0_PWR is 0. During system resume, if controller
registers are lost, _dwc2_hcd_resume() unconditionally sets HPRT0_PWR to 1
further down in this function:

    dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET |
                    HPRT0_ENACHG, HPRT0);

By removing the dwc2_vbus_supply_init() call here, the physical regulator
remains off, but the HPRT0_PWR bit is forced to 1, causing a state desync.

When the hub driver later tries to power on the port in dwc2_hcd_hub_control(),
it checks the current state:

    case USB_PORT_FEAT_POWER:
        ...
        hprt0 = dwc2_read_hprt0(hsotg);
        pwr = hprt0 & HPRT0_PWR;
        hprt0 |= HPRT0_PWR;
        dwc2_writel(hsotg, hprt0, HPRT0);
        if (!pwr)
            dwc2_vbus_supply_init(hsotg);
        break;

Since HPRT0_PWR is already 1, the check for !pwr evaluates to false, and the
external regulator is silently skipped.

Does this lock the port in an permanently unpowered state where it is unable
to detect or enumerate devices? Could this be avoided by only setting HPRT0_PWR
during resume if the regulator is also being enabled, or by restoring the
exact prior port power state instead of unconditionally setting it?

>  	/* Wait for controller to correctly update D+/D- level */
>  	usleep_range(3000, 5000);
>  	spin_lock_irqsave(&hsotg->lock, flags);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-dwc2-low-power-fixes-stm32-v2-0-79865a4f1b63@foss.st.com?part=4

[PATCH v2 5/5] usb: dwc2: platform: fix stm32mp15 detection logic power management

[Fabrice Gasnier]

On stm32mp15, the stm detection logic can be left enabled even though the
other hw resources gets disabled from the probe (peripheral mode only):
	/* Gadget code manages lowlevel hw on its own */
	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL)
		dwc2_lowlevel_hw_disable(hsotg);

Then when going to system low power, stm detection logic resources now
remains enabled since dwc2_suspend() checks:
	if (!dwc2->ll_hw_enabled)
		return 0;

In case the stm32mp15 goes standby, the registers contents gets lost
and may not be restored so dwc2 becomes unusable. This is the case when
no gadget driver has been bound until that point.

So move the stm detection logic enable/disable procedures to dedicated
dwc2_stm32_hw_[enable/disable] routines. Call these routines
from the __dwc2_lowlevel_hw_[enable/disable] routines.

Factorize some of the probing code. Replace regulator_enable() and
GGPIO register poking by direct call to dwc2_stm32_hw_enable().
The dwc2_lowlevel_hw_enable() call done earlier during probing isn't
able to do it: the activate_stm_id_vb_detection is only known after
the dwc2_init_params() has run (and probe the usb33d-supply).

In __dwc2_lowlevel_hw_disable() in case the dwc2_stm32_hw_disable() fails,
return the error, without disabling other HW resources to avoid possible
inconsistent state (e.g. usb33d supply not disabled, so other resources
are kept in the same state).

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 v2:
- fix probe error path in case devm_regulator_get or dwc2_stm32_hw_enable
  fails as raised by Sashiko. Clear activate_stm_id_vb_detection flag so
  the dwc2_stm32_hw_disable() routine won't call regulator_disable()
  respectively with the ERR_PTR returned or unbalanced.
- update commit message to describe error handling on HW disable.
---
 drivers/usb/dwc2/platform.c | 173 +++++++++++++++++++++++++-------------------
 1 file changed, 98 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 5aa552672078..5841efef6b35 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -91,6 +91,80 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
 	return 0;
 }
 
+static int dwc2_stm32_hw_enable(struct dwc2_hsotg *hsotg)
+{
+	unsigned long flags;
+	u32 ggpio, gotgctl;
+	int ret;
+
+	/* params are only known, once dwc2_init_params() has been probed */
+	if (!hsotg->params.activate_stm_id_vb_detection)
+		return 0;
+
+	ret = regulator_enable(hsotg->usb33d);
+	if (ret)
+		return ret;
+
+	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);
+
+	return 0;
+}
+
+static int dwc2_stm32_hw_disable(struct dwc2_hsotg *hsotg)
+{
+	unsigned long flags;
+	u32 ggpio, gotgctl;
+	int ret;
+
+	if (!hsotg->params.activate_stm_id_vb_detection)
+		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)
+		return ret;
+
+	return 0;
+}
+
 static int __dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg)
 {
 	struct platform_device *pdev = to_platform_device(hsotg->dev);
@@ -129,8 +203,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 +254,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 +438,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);
 }
@@ -541,33 +630,26 @@ 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);
+			/* skips low level dwc2_stm32_hw_disable() in case of error */
+			hsotg->params.activate_stm_id_vb_detection = 0;
 			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");
+			hsotg->params.activate_stm_id_vb_detection = 0;
+			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) {
@@ -637,9 +719,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);
@@ -661,37 +740,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);
@@ -716,31 +764,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 v2 5/5] usb: dwc2: platform: fix stm32mp15 detection logic power management

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] usb: dwc2: resource leak on dwc2_stm32_hw_disable error
- [High] usb: dwc2: stm32 GGPIO registers not restored on resume in host mode
- [High] usb: dwc2: platform: resource leak and hardware state corruption on regulator_disable failure
- [Medium] usb: dwc2: hardware left in inconsistent state on regulator_disable failure
- [Medium] usb: dwc2: unlocked RMW of GUSBCFG in dwc2_force_mode() races with IRQ handler
--

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

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

This patch moves the STM32 detection logic enable and disable procedures
into dedicated routines. These routines are now called from the low-level
hardware enable and disable functions, ensuring that the detection
registers are properly handled during power management transitions.

Cc: stable@vger.kernel.org

> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 5aa5526720783..5841efef6b351 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
[ ... ]
> @@ -91,6 +91,80 @@ static int dwc2_get_dr_mode(struct dwc2_hsotg *hsotg)
[ ... ]
> +static int dwc2_stm32_hw_disable(struct dwc2_hsotg *hsotg)
> +{
> +	unsigned long flags;
> +	u32 ggpio, gotgctl;
> +	int ret;
> +
> +	if (!hsotg->params.activate_stm_id_vb_detection)
> +		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);

[Severity: Medium]
Since dwc2_force_mode() performs an unlocked read-modify-write on the
GUSBCFG register, can this race with a concurrent interrupt handler
(such as dwc2_port_intr() running under hsotg->lock) and silently
overwrite the handler's changes?

[ ... ]
> +	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)
> +		return ret;

[Severity: Medium]
If regulator_disable() fails and returns an error, the OTG detection
registers GOTGCTL and GGPIO have already been modified. Does this leave
the hardware in a permanently inconsistent state with its OTG detection
broken, since the register modifications are not rolled back on this
error path?

[ ... ]
> @@ -166,6 +254,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);

[Severity: High]
If dwc2_stm32_hw_disable() fails (for example, due to the
regulator_disable failure), this early return abruptly halts the
teardown. When this happens during dwc2_driver_remove(), does this
bypass the disablement of the remaining PHY, clock, and bulk regulator
resources, causing a permanent leak?

[ ... ]
> @@ -716,31 +764,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);

[Severity: High]
This STM32 hardware enable logic was previously executed unconditionally
on resume, but is now moved inside __dwc2_lowlevel_hw_enable().

In host mode with remote wakeup enabled, dwc2_host_can_poweroff_phy()
returns false, which skips __dwc2_lowlevel_hw_disable() during suspend
and leaves phy_off_for_suspend = false.

During dwc2_resume(), because phy_off_for_suspend is false,
__dwc2_lowlevel_hw_enable() is skipped entirely.

If the controller's power domain was disabled during suspend (losing
register values), doesn't skipping the STM32 hardware enable leave the
GGPIO register uninitialized, breaking VBUS and ID detection on resume?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013-dwc2-low-power-fixes-stm32-v2-0-79865a4f1b63@foss.st.com?part=5