[PATCH v3 0/3] usb: dwc3: xilinx: error path and teardown fixes

[PATCH v3 0/3] usb: dwc3: xilinx: error path and teardown fixes

[Radhey Shyam Pandey]

dwc3-xilinx does not release what it acquires: system suspend exits the
USB3 PHY without powering it off, the ZynqMP init error paths leave the
core resets deasserted, and neither probe failure nor remove undoes the
reset and PHY state at all.  These three patches fix each in turn.

Changes in v3:
The usb_resets_released flag and the pltfm_exit pointer are gone.  The
teardown is now a devm_add_action_or_reset() action, guarded on usb3-phy
so it only undoes the reset cycle init performed, and invoked explicitly
before the clocks are gated on both the probe error path and remove, so
.shutdown gets it too.

Changes in v2:
Split out of a five patch series so the fixes can be sent and backported
on their own, and reordered ahead of the platform-data cleanups.  Patch
3 was reworked so it no longer depends on that cleanup, and all three
are tagged for stable.

All three are tagged for stable.
Tested on ZCU102: unbind and rebind cycle cleanly with the device
re-enumerating at SuperSpeed, and reboot is clean.

Link to v1:
https://lore.kernel.org/all/20260810174713.2325292-1-radhey.shyam.pandey@amd.com/
Link to v2:
https://lore.kernel.org/all/20260922182125.11067-1-radhey.shyam.pandey@amd.com/

Radhey Shyam Pandey (3):
  usb: dwc3: xilinx: fix system suspend and resume PHY handling
  usb: dwc3: xilinx: re-assert resets on ZynqMP init error paths
  usb: dwc3: xilinx: unwind ZynqMP platform init on probe failure and
    remove

 drivers/usb/dwc3/dwc3-xilinx.c | 123 +++++++++++++++++++++++++++------
 1 file changed, 100 insertions(+), 23 deletions(-)


base-commit: abc36cbda29d8f19cf3a580cd86ca9e865186a41
-- 
2.44.4

[PATCH v3 1/3] usb: dwc3: xilinx: fix system suspend and resume PHY handling

[Radhey Shyam Pandey]

System suspend and resume error paths do not handle PHY and clock
resources correctly. Suspend calls phy_exit() without first powering
off the PHY and ignores failures, while resume can leave clocks
enabled if PHY reinitialization fails.

Propagate errors to the PM core and unwind resources to ensure a
consistent state on suspend and resume failures.

Fixes: d6edcdc1ef06 ("usb: dwc3: xilinx: fix usb3 non-wakeup source resume failure")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
Changes in v3:
- None

Changes in v2:
- Split out of the combined five patch series so the fixes can be sent
  and backported on their own, per review feedback.
- Reordered ahead of the platform-data cleanups.
- Log a failure of the phy_power_on() rollback.  phy_exit() leaves
  init_count untouched when it fails, so if the rollback also fails the
  PHY is left with power_count and init_count out of step; that is now
  at least visible in the log.
- Added Cc: stable.
---
 drivers/usb/dwc3/dwc3-xilinx.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
index b832505e1b04..8c63e02575f1 100644
--- a/drivers/usb/dwc3/dwc3-xilinx.c
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -383,13 +383,26 @@ static int __maybe_unused dwc3_xlnx_runtime_idle(struct device *dev)
 static int __maybe_unused dwc3_xlnx_suspend(struct device *dev)
 {
 	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
+	int ret;
 
-	phy_exit(priv_data->usb3_phy);
+	ret = phy_power_off(priv_data->usb3_phy);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_exit(priv_data->usb3_phy);
+	if (ret < 0)
+		goto err_phy_power_on;
 
 	/* Disable the clocks */
 	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
 
 	return 0;
+
+err_phy_power_on:
+	if (phy_power_on(priv_data->usb3_phy))
+		dev_err(dev, "failed to restore PHY power after suspend error\n");
+
+	return ret;
 }
 
 static int __maybe_unused dwc3_xlnx_resume(struct device *dev)
@@ -403,15 +416,20 @@ static int __maybe_unused dwc3_xlnx_resume(struct device *dev)
 
 	ret = phy_init(priv_data->usb3_phy);
 	if (ret < 0)
-		return ret;
+		goto err_clk_disable;
 
 	ret = phy_power_on(priv_data->usb3_phy);
 	if (ret < 0) {
 		phy_exit(priv_data->usb3_phy);
-		return ret;
+		goto err_clk_disable;
 	}
 
 	return 0;
+
+err_clk_disable:
+	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
+
+	return ret;
 }
 
 static const struct dev_pm_ops dwc3_xlnx_dev_pm_ops = {
-- 
2.44.4

[PATCH v3 2/3] usb: dwc3: xilinx: re-assert resets on ZynqMP init error paths

[Radhey Shyam Pandey]

If reset deassert or PHY setup fails partway through
dwc3_xlnx_init_zynqmp(), re-assert the resets that were already
released before unwinding the PHY.

Only undo the reset cycle the function actually performed. The cycle is
skipped when no usb3-phy is present, because asserting the core resets
can break a USB3 configuration that is live but missing from the device
tree, so the error path skips it for the same reason. Asserting an
already asserted reset is harmless, so one block covers however far the
deasserts progressed.

Save the ZynqMP reset handles in driver private data so later probe
teardown can re-assert released resets.

Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
Changes in v3:
- Dropped the usb_resets_released field.  Nothing in this patch read it;
  it only existed for the conditional teardown in patch 3, which no
  longer needs it.
- Collapsed the three cascading assert labels into one err_reset_assert
  block guarded on priv_data->usb3_phy.  v2 asserted unconditionally,
  which would have put a no-PHY board into reset that init never
  asserted; the guard now matches the init-time cycle.

Changes in v2:
- Split out of the combined five patch series; see patch 1.
- Reordered ahead of the platform-data cleanups.
- Added Cc: stable.
- No functional change to the patch itself.
---
 drivers/usb/dwc3/dwc3-xilinx.c | 55 ++++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
index 8c63e02575f1..d2315573b3d3 100644
--- a/drivers/usb/dwc3/dwc3-xilinx.c
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -48,6 +48,9 @@ struct dwc3_xlnx {
 	void __iomem			*regs;
 	int				(*pltfm_init)(struct dwc3_xlnx *data);
 	struct phy			*usb3_phy;
+	struct reset_control		*usb_crst;
+	struct reset_control		*usb_hibrst;
+	struct reset_control		*usb_apbrst;
 };
 
 static void dwc3_xlnx_mask_phy_rst(struct dwc3_xlnx *priv_data, bool mask)
@@ -112,7 +115,6 @@ static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
 static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 {
 	struct device		*dev = priv_data->dev;
-	struct reset_control	*crst, *hibrst, *apbrst;
 	struct gpio_desc	*reset_gpio;
 	int			ret = 0;
 
@@ -124,25 +126,25 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 		goto err;
 	}
 
-	crst = devm_reset_control_get_exclusive(dev, "usb_crst");
-	if (IS_ERR(crst)) {
-		ret = PTR_ERR(crst);
+	priv_data->usb_crst = devm_reset_control_get_exclusive(dev, "usb_crst");
+	if (IS_ERR(priv_data->usb_crst)) {
+		ret = PTR_ERR(priv_data->usb_crst);
 		dev_err_probe(dev, ret,
 			      "failed to get core reset signal\n");
 		goto err;
 	}
 
-	hibrst = devm_reset_control_get_exclusive(dev, "usb_hibrst");
-	if (IS_ERR(hibrst)) {
-		ret = PTR_ERR(hibrst);
+	priv_data->usb_hibrst = devm_reset_control_get_exclusive(dev, "usb_hibrst");
+	if (IS_ERR(priv_data->usb_hibrst)) {
+		ret = PTR_ERR(priv_data->usb_hibrst);
 		dev_err_probe(dev, ret,
 			      "failed to get hibernation reset signal\n");
 		goto err;
 	}
 
-	apbrst = devm_reset_control_get_exclusive(dev, "usb_apbrst");
-	if (IS_ERR(apbrst)) {
-		ret = PTR_ERR(apbrst);
+	priv_data->usb_apbrst = devm_reset_control_get_exclusive(dev, "usb_apbrst");
+	if (IS_ERR(priv_data->usb_apbrst)) {
+		ret = PTR_ERR(priv_data->usb_apbrst);
 		dev_err_probe(dev, ret,
 			      "failed to get APB reset signal\n");
 		goto err;
@@ -156,19 +158,19 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 	 * absent.
 	 */
 	if (priv_data->usb3_phy) {
-		ret = reset_control_assert(crst);
+		ret = reset_control_assert(priv_data->usb_crst);
 		if (ret < 0) {
 			dev_err(dev, "Failed to assert core reset\n");
 			goto err;
 		}
 
-		ret = reset_control_assert(hibrst);
+		ret = reset_control_assert(priv_data->usb_hibrst);
 		if (ret < 0) {
 			dev_err(dev, "Failed to assert hibernation reset\n");
 			goto err;
 		}
 
-		ret = reset_control_assert(apbrst);
+		ret = reset_control_assert(priv_data->usb_apbrst);
 		if (ret < 0) {
 			dev_err(dev, "Failed to assert APB reset\n");
 			goto err;
@@ -179,7 +181,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 	if (ret < 0)
 		goto err;
 
-	ret = reset_control_deassert(apbrst);
+	ret = reset_control_deassert(priv_data->usb_apbrst);
 	if (ret < 0) {
 		dev_err(dev, "Failed to release APB reset\n");
 		goto err_phy_exit;
@@ -195,21 +197,21 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 		writel(PIPE_CLK_DESELECT, priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
 	}
 
-	ret = reset_control_deassert(crst);
+	ret = reset_control_deassert(priv_data->usb_crst);
 	if (ret < 0) {
 		dev_err(dev, "Failed to release core reset\n");
-		goto err_phy_exit;
+		goto err_reset_assert;
 	}
 
-	ret = reset_control_deassert(hibrst);
+	ret = reset_control_deassert(priv_data->usb_hibrst);
 	if (ret < 0) {
 		dev_err(dev, "Failed to release hibernation reset\n");
-		goto err_phy_exit;
+		goto err_reset_assert;
 	}
 
 	ret = phy_power_on(priv_data->usb3_phy);
 	if (ret < 0)
-		goto err_phy_exit;
+		goto err_reset_assert;
 
 	/* ulpi reset via gpio-modepin or gpio-framework driver */
 	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
@@ -230,6 +232,21 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 
 err_phy_power_off:
 	phy_power_off(priv_data->usb3_phy);
+err_reset_assert:
+	/*
+	 * Undo only the reset cycle performed above.  That cycle is skipped
+	 * when there is no usb3-phy.  The deasserts stay unconditional so a
+	 * core the bootloader left in reset still comes up, but they are
+	 * deliberately not paired with an assert here: resetting a USB3
+	 * configuration that is live but missing from the device tree could
+	 * break it.  Asserting an already asserted reset is harmless, so one
+	 * block covers however far the cycle's deasserts got.
+	 */
+	if (priv_data->usb3_phy) {
+		reset_control_assert(priv_data->usb_hibrst);
+		reset_control_assert(priv_data->usb_crst);
+		reset_control_assert(priv_data->usb_apbrst);
+	}
 err_phy_exit:
 	phy_exit(priv_data->usb3_phy);
 err:
-- 
2.44.4

[PATCH v3 3/3] usb: dwc3: xilinx: unwind ZynqMP platform init on probe failure and remove

[Radhey Shyam Pandey]

dwc3_xlnx_init_zynqmp() leaves the controller out of reset with the PHY
powered on, but nothing undoes that if probe fails later or when the
driver is removed. The resets stay deasserted and the PHY stays on with
no driver bound.

Register the teardown with devm_add_action_or_reset() once init has
completed, so it runs on probe failure without the driver having to
track how far init progressed. The teardown only undoes the reset cycle
init performed, so it carries the same usb3-phy guard: with no PHY the
resets were never asserted, and asserting them could break a USB3
configuration that is live but missing from the device tree.

The resets have to be asserted while the clocks are still running, so
both the probe error path and remove() run the teardown explicitly
before gating the clocks rather than leaving it to devres, which would
otherwise run it afterwards. devm_release_action() unlinks the action
before calling it, so devres cannot run it a second time.

Running it from remove() also covers .shutdown, which shares
dwc3_xlnx_remove(): device_shutdown() does not call
devres_release_all(), so a teardown left to devres would never run
there.

Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
Cc: stable@vger.kernel.org
Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
Assisted-by: Claude:claude-opus-5
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
Changes in v3:
- Register the teardown with devm_add_action_or_reset() instead of a
  pltfm_exit pointer and the usb_resets_released flag, per Philipp.
- Guard the teardown's reset asserts on usb3_phy; v2 asserted
  unconditionally, resetting a no-PHY board that init never asserted.
- Replace v2's err_pltfm_exit label with dwc3_xlnx_release_teardown(),
  called from both the probe error path and remove() so the resets are
  unwound before the clocks are gated in either.
- Explain in remove() why the teardown is not left to devres:
  device_shutdown() does not call devres_release_all().
- Clocks left unmanaged: devm_pm_runtime_enable()'s cleanup runs
  runtime_suspend() before a clock devres node would release, which
  double-disables on ZynqMP.

Changes in v2:
- Reworked so the fix no longer depends on the platform-data cleanup.
  v1 registered the teardown as plat->exit in struct dwc3_xlnx_platdata,
  which is introduced by one of the cleanup patches; that made the fix
  unbackportable.  It now uses a pltfm_exit pointer alongside the
  existing pltfm_init in struct dwc3_xlnx, assigned by
  dwc3_xlnx_init_zynqmp() once init has succeeded.  The follow-up
  cleanup series folds both pointers into the platform data struct.
- Made dwc3_xlnx_exit_zynqmp() idempotent by returning early when
  usb_resets_released is clear, rather than guarding only the reset
  assertions.  phy_power_off() and phy_exit() decrement their counts
  unconditionally, so an unbalanced second call would underflow them.
- Rewrote the commit message to describe the bug rather than the
  implementation, since the callback it referred to no longer exists at
  this point in the series.
- Added Cc: stable.
---
 drivers/usb/dwc3/dwc3-xilinx.c | 44 +++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
index d2315573b3d3..475bc19a1746 100644
--- a/drivers/usb/dwc3/dwc3-xilinx.c
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -112,6 +112,39 @@ static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
 	return 0;
 }
 
+static void dwc3_xlnx_zynqmp_teardown(void *data)
+{
+	struct dwc3_xlnx *priv_data = data;
+
+	phy_power_off(priv_data->usb3_phy);
+
+	/*
+	 * Undo only the reset cycle init performed.  As on the init error
+	 * path, the unconditional deasserts are not paired with an assert
+	 * here: with no usb3-phy, resetting the core could break a USB3
+	 * configuration that is live but missing from the device tree.
+	 */
+	if (priv_data->usb3_phy) {
+		reset_control_assert(priv_data->usb_hibrst);
+		reset_control_assert(priv_data->usb_crst);
+		reset_control_assert(priv_data->usb_apbrst);
+	}
+
+	phy_exit(priv_data->usb3_phy);
+}
+
+/*
+ * Run the platform teardown explicitly, so the resets are asserted while the
+ * clocks are still running.  devm_release_action() unlinks the action before
+ * calling it, so devres will not run it a second time.
+ */
+static void dwc3_xlnx_release_teardown(struct device *dev,
+				       struct dwc3_xlnx *priv_data)
+{
+	if (devm_is_action_added(dev, dwc3_xlnx_zynqmp_teardown, priv_data))
+		devm_release_action(dev, dwc3_xlnx_zynqmp_teardown, priv_data);
+}
+
 static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 {
 	struct device		*dev = priv_data->dev;
@@ -228,7 +261,8 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
 
 	dwc3_xlnx_set_coherency(priv_data, XLNX_USB_TRAFFIC_ROUTE_CONFIG);
 
-	return 0;
+	return devm_add_action_or_reset(dev, dwc3_xlnx_zynqmp_teardown,
+					priv_data);
 
 err_phy_power_off:
 	phy_power_off(priv_data->usb3_phy);
@@ -355,6 +389,7 @@ static int dwc3_xlnx_probe(struct platform_device *pdev)
 	pm_runtime_set_suspended(dev);
 
 err_clk_put:
+	dwc3_xlnx_release_teardown(dev, priv_data);
 	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
 
 	return ret;
@@ -367,6 +402,13 @@ static void dwc3_xlnx_remove(struct platform_device *pdev)
 
 	of_platform_depopulate(dev);
 
+	/*
+	 * This is also what makes .shutdown behave the same as .remove:
+	 * device_shutdown() does not call devres_release_all(), so a teardown
+	 * left to devres would never run on the shutdown path.
+	 */
+	dwc3_xlnx_release_teardown(dev, priv_data);
+
 	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
 	priv_data->num_clocks = 0;
 
-- 
2.44.4

Re: [PATCH v3 0/3] usb: dwc3: xilinx: error path and teardown fixes

[Thinh Nguyen]

On Mon, Sep 28, 2026, Radhey Shyam Pandey wrote:
> dwc3-xilinx does not release what it acquires: system suspend exits the
> USB3 PHY without powering it off, the ZynqMP init error paths leave the
> core resets deasserted, and neither probe failure nor remove undoes the
> reset and PHY state at all.  These three patches fix each in turn.
> 
> Changes in v3:
> The usb_resets_released flag and the pltfm_exit pointer are gone.  The
> teardown is now a devm_add_action_or_reset() action, guarded on usb3-phy
> so it only undoes the reset cycle init performed, and invoked explicitly
> before the clocks are gated on both the probe error path and remove, so
> .shutdown gets it too.
> 
> Changes in v2:
> Split out of a five patch series so the fixes can be sent and backported
> on their own, and reordered ahead of the platform-data cleanups.  Patch
> 3 was reworked so it no longer depends on that cleanup, and all three
> are tagged for stable.
> 
> All three are tagged for stable.
> Tested on ZCU102: unbind and rebind cycle cleanly with the device
> re-enumerating at SuperSpeed, and reboot is clean.
> 
> Link to v1:
> https://urldefense.com/v3/__https://lore.kernel.org/all/20260810174713.2325292-1-radhey.shyam.pandey@amd.com/__;!!A4F2R9G_pg!d4dH4RYBRKYYxRO-3MGmHJZ86UXN4JYGusxmhX4jDPLX6H5W62iLJiLacptn5Rp4xREH-Vz1JK6yyLXc6Xfuob4dk7gorw$ 
> Link to v2:
> https://urldefense.com/v3/__https://lore.kernel.org/all/20260922182125.11067-1-radhey.shyam.pandey@amd.com/__;!!A4F2R9G_pg!d4dH4RYBRKYYxRO-3MGmHJZ86UXN4JYGusxmhX4jDPLX6H5W62iLJiLacptn5Rp4xREH-Vz1JK6yyLXc6Xfuob7oSPp7Kw$ 
> 
> Radhey Shyam Pandey (3):
>   usb: dwc3: xilinx: fix system suspend and resume PHY handling
>   usb: dwc3: xilinx: re-assert resets on ZynqMP init error paths
>   usb: dwc3: xilinx: unwind ZynqMP platform init on probe failure and
>     remove
> 
>  drivers/usb/dwc3/dwc3-xilinx.c | 123 +++++++++++++++++++++++++++------
>  1 file changed, 100 insertions(+), 23 deletions(-)
> 
> 
> base-commit: abc36cbda29d8f19cf3a580cd86ca9e865186a41
> -- 
> 2.44.4
> 

I missed seeing the v3 of this series. Looking through the changes now.

Thanks,
Thinh

Re: [PATCH v3 1/3] usb: dwc3: xilinx: fix system suspend and resume PHY handling

[Thinh Nguyen]

On Mon, Sep 28, 2026, Radhey Shyam Pandey wrote:
> System suspend and resume error paths do not handle PHY and clock
> resources correctly. Suspend calls phy_exit() without first powering
> off the PHY and ignores failures, while resume can leave clocks
> enabled if PHY reinitialization fails.
> 
> Propagate errors to the PM core and unwind resources to ensure a
> consistent state on suspend and resume failures.
> 
> Fixes: d6edcdc1ef06 ("usb: dwc3: xilinx: fix usb3 non-wakeup source resume failure")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> Changes in v3:
> - None
> 
> Changes in v2:
> - Split out of the combined five patch series so the fixes can be sent
>   and backported on their own, per review feedback.
> - Reordered ahead of the platform-data cleanups.
> - Log a failure of the phy_power_on() rollback.  phy_exit() leaves
>   init_count untouched when it fails, so if the rollback also fails the
>   PHY is left with power_count and init_count out of step; that is now
>   at least visible in the log.
> - Added Cc: stable.
> ---
>  drivers/usb/dwc3/dwc3-xilinx.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index b832505e1b04..8c63e02575f1 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -383,13 +383,26 @@ static int __maybe_unused dwc3_xlnx_runtime_idle(struct device *dev)
>  static int __maybe_unused dwc3_xlnx_suspend(struct device *dev)
>  {
>  	struct dwc3_xlnx *priv_data = dev_get_drvdata(dev);
> +	int ret;
>  
> -	phy_exit(priv_data->usb3_phy);
> +	ret = phy_power_off(priv_data->usb3_phy);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_exit(priv_data->usb3_phy);
> +	if (ret < 0)
> +		goto err_phy_power_on;
>  
>  	/* Disable the clocks */
>  	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
>  
>  	return 0;
> +
> +err_phy_power_on:
> +	if (phy_power_on(priv_data->usb3_phy))
> +		dev_err(dev, "failed to restore PHY power after suspend error\n");
> +
> +	return ret;
>  }
>  
>  static int __maybe_unused dwc3_xlnx_resume(struct device *dev)
> @@ -403,15 +416,20 @@ static int __maybe_unused dwc3_xlnx_resume(struct device *dev)
>  
>  	ret = phy_init(priv_data->usb3_phy);
>  	if (ret < 0)
> -		return ret;
> +		goto err_clk_disable;
>  
>  	ret = phy_power_on(priv_data->usb3_phy);
>  	if (ret < 0) {
>  		phy_exit(priv_data->usb3_phy);
> -		return ret;
> +		goto err_clk_disable;
>  	}
>  
>  	return 0;
> +
> +err_clk_disable:
> +	clk_bulk_disable(priv_data->num_clocks, priv_data->clks);
> +
> +	return ret;
>  }
>  
>  static const struct dev_pm_ops dwc3_xlnx_dev_pm_ops = {
> -- 
> 2.44.4
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

Thanks,
Thinh

Re: [PATCH v3 2/3] usb: dwc3: xilinx: re-assert resets on ZynqMP init error paths

[Thinh Nguyen]

On Mon, Sep 28, 2026, Radhey Shyam Pandey wrote:
> If reset deassert or PHY setup fails partway through
> dwc3_xlnx_init_zynqmp(), re-assert the resets that were already
> released before unwinding the PHY.
> 
> Only undo the reset cycle the function actually performed. The cycle is
> skipped when no usb3-phy is present, because asserting the core resets
> can break a USB3 configuration that is live but missing from the device
> tree, so the error path skips it for the same reason. Asserting an
> already asserted reset is harmless, so one block covers however far the
> deasserts progressed.
> 
> Save the ZynqMP reset handles in driver private data so later probe
> teardown can re-assert released resets.
> 
> Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> Changes in v3:
> - Dropped the usb_resets_released field.  Nothing in this patch read it;
>   it only existed for the conditional teardown in patch 3, which no
>   longer needs it.
> - Collapsed the three cascading assert labels into one err_reset_assert
>   block guarded on priv_data->usb3_phy.  v2 asserted unconditionally,
>   which would have put a no-PHY board into reset that init never
>   asserted; the guard now matches the init-time cycle.
> 
> Changes in v2:
> - Split out of the combined five patch series; see patch 1.
> - Reordered ahead of the platform-data cleanups.
> - Added Cc: stable.
> - No functional change to the patch itself.
> ---
>  drivers/usb/dwc3/dwc3-xilinx.c | 55 ++++++++++++++++++++++------------
>  1 file changed, 36 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index 8c63e02575f1..d2315573b3d3 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -48,6 +48,9 @@ struct dwc3_xlnx {
>  	void __iomem			*regs;
>  	int				(*pltfm_init)(struct dwc3_xlnx *data);
>  	struct phy			*usb3_phy;
> +	struct reset_control		*usb_crst;
> +	struct reset_control		*usb_hibrst;
> +	struct reset_control		*usb_apbrst;
>  };
>  
>  static void dwc3_xlnx_mask_phy_rst(struct dwc3_xlnx *priv_data, bool mask)
> @@ -112,7 +115,6 @@ static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
>  static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  {
>  	struct device		*dev = priv_data->dev;
> -	struct reset_control	*crst, *hibrst, *apbrst;
>  	struct gpio_desc	*reset_gpio;
>  	int			ret = 0;
>  
> @@ -124,25 +126,25 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  		goto err;
>  	}
>  
> -	crst = devm_reset_control_get_exclusive(dev, "usb_crst");
> -	if (IS_ERR(crst)) {
> -		ret = PTR_ERR(crst);
> +	priv_data->usb_crst = devm_reset_control_get_exclusive(dev, "usb_crst");
> +	if (IS_ERR(priv_data->usb_crst)) {
> +		ret = PTR_ERR(priv_data->usb_crst);
>  		dev_err_probe(dev, ret,
>  			      "failed to get core reset signal\n");
>  		goto err;
>  	}
>  
> -	hibrst = devm_reset_control_get_exclusive(dev, "usb_hibrst");
> -	if (IS_ERR(hibrst)) {
> -		ret = PTR_ERR(hibrst);
> +	priv_data->usb_hibrst = devm_reset_control_get_exclusive(dev, "usb_hibrst");
> +	if (IS_ERR(priv_data->usb_hibrst)) {
> +		ret = PTR_ERR(priv_data->usb_hibrst);
>  		dev_err_probe(dev, ret,
>  			      "failed to get hibernation reset signal\n");
>  		goto err;
>  	}
>  
> -	apbrst = devm_reset_control_get_exclusive(dev, "usb_apbrst");
> -	if (IS_ERR(apbrst)) {
> -		ret = PTR_ERR(apbrst);
> +	priv_data->usb_apbrst = devm_reset_control_get_exclusive(dev, "usb_apbrst");
> +	if (IS_ERR(priv_data->usb_apbrst)) {
> +		ret = PTR_ERR(priv_data->usb_apbrst);
>  		dev_err_probe(dev, ret,
>  			      "failed to get APB reset signal\n");
>  		goto err;
> @@ -156,19 +158,19 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  	 * absent.
>  	 */
>  	if (priv_data->usb3_phy) {
> -		ret = reset_control_assert(crst);
> +		ret = reset_control_assert(priv_data->usb_crst);
>  		if (ret < 0) {
>  			dev_err(dev, "Failed to assert core reset\n");
>  			goto err;
>  		}
>  
> -		ret = reset_control_assert(hibrst);
> +		ret = reset_control_assert(priv_data->usb_hibrst);
>  		if (ret < 0) {
>  			dev_err(dev, "Failed to assert hibernation reset\n");
>  			goto err;
>  		}
>  
> -		ret = reset_control_assert(apbrst);
> +		ret = reset_control_assert(priv_data->usb_apbrst);
>  		if (ret < 0) {
>  			dev_err(dev, "Failed to assert APB reset\n");
>  			goto err;
> @@ -179,7 +181,7 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  	if (ret < 0)
>  		goto err;
>  
> -	ret = reset_control_deassert(apbrst);
> +	ret = reset_control_deassert(priv_data->usb_apbrst);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to release APB reset\n");
>  		goto err_phy_exit;
> @@ -195,21 +197,21 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  		writel(PIPE_CLK_DESELECT, priv_data->regs + XLNX_USB_FPD_PIPE_CLK);
>  	}
>  
> -	ret = reset_control_deassert(crst);
> +	ret = reset_control_deassert(priv_data->usb_crst);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to release core reset\n");
> -		goto err_phy_exit;
> +		goto err_reset_assert;
>  	}
>  
> -	ret = reset_control_deassert(hibrst);
> +	ret = reset_control_deassert(priv_data->usb_hibrst);
>  	if (ret < 0) {
>  		dev_err(dev, "Failed to release hibernation reset\n");
> -		goto err_phy_exit;
> +		goto err_reset_assert;
>  	}
>  
>  	ret = phy_power_on(priv_data->usb3_phy);
>  	if (ret < 0)
> -		goto err_phy_exit;
> +		goto err_reset_assert;
>  
>  	/* ulpi reset via gpio-modepin or gpio-framework driver */
>  	reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> @@ -230,6 +232,21 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  
>  err_phy_power_off:
>  	phy_power_off(priv_data->usb3_phy);
> +err_reset_assert:
> +	/*
> +	 * Undo only the reset cycle performed above.  That cycle is skipped
> +	 * when there is no usb3-phy.  The deasserts stay unconditional so a
> +	 * core the bootloader left in reset still comes up, but they are
> +	 * deliberately not paired with an assert here: resetting a USB3
> +	 * configuration that is live but missing from the device tree could
> +	 * break it.  Asserting an already asserted reset is harmless, so one
> +	 * block covers however far the cycle's deasserts got.
> +	 */

This is overly verbose. I'd reserve the comment in the commit message
only, although I suppose having it doesn't hurt..

> +	if (priv_data->usb3_phy) {
> +		reset_control_assert(priv_data->usb_hibrst);
> +		reset_control_assert(priv_data->usb_crst);
> +		reset_control_assert(priv_data->usb_apbrst);
> +	}
>  err_phy_exit:
>  	phy_exit(priv_data->usb3_phy);
>  err:
> -- 
> 2.44.4
> 

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

BR,
Thinh

Re: [PATCH v3 3/3] usb: dwc3: xilinx: unwind ZynqMP platform init on probe failure and remove

[Thinh Nguyen]

On Mon, Sep 28, 2026, Radhey Shyam Pandey wrote:
> dwc3_xlnx_init_zynqmp() leaves the controller out of reset with the PHY
> powered on, but nothing undoes that if probe fails later or when the
> driver is removed. The resets stay deasserted and the PHY stays on with
> no driver bound.
> 
> Register the teardown with devm_add_action_or_reset() once init has
> completed, so it runs on probe failure without the driver having to
> track how far init progressed. The teardown only undoes the reset cycle
> init performed, so it carries the same usb3-phy guard: with no PHY the
> resets were never asserted, and asserting them could break a USB3
> configuration that is live but missing from the device tree.
> 
> The resets have to be asserted while the clocks are still running, so
> both the probe error path and remove() run the teardown explicitly
> before gating the clocks rather than leaving it to devres, which would
> otherwise run it afterwards. devm_release_action() unlinks the action
> before calling it, so devres cannot run it a second time.
> 
> Running it from remove() also covers .shutdown, which shares
> dwc3_xlnx_remove(): device_shutdown() does not call
> devres_release_all(), so a teardown left to devres would never run
> there.
> 
> Fixes: 84770f028fab ("usb: dwc3: Add driver for Xilinx platforms")
> Cc: stable@vger.kernel.org
> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> ---
> Changes in v3:
> - Register the teardown with devm_add_action_or_reset() instead of a
>   pltfm_exit pointer and the usb_resets_released flag, per Philipp.
> - Guard the teardown's reset asserts on usb3_phy; v2 asserted
>   unconditionally, resetting a no-PHY board that init never asserted.
> - Replace v2's err_pltfm_exit label with dwc3_xlnx_release_teardown(),
>   called from both the probe error path and remove() so the resets are
>   unwound before the clocks are gated in either.
> - Explain in remove() why the teardown is not left to devres:
>   device_shutdown() does not call devres_release_all().
> - Clocks left unmanaged: devm_pm_runtime_enable()'s cleanup runs
>   runtime_suspend() before a clock devres node would release, which
>   double-disables on ZynqMP.
> 
> Changes in v2:
> - Reworked so the fix no longer depends on the platform-data cleanup.
>   v1 registered the teardown as plat->exit in struct dwc3_xlnx_platdata,
>   which is introduced by one of the cleanup patches; that made the fix
>   unbackportable.  It now uses a pltfm_exit pointer alongside the
>   existing pltfm_init in struct dwc3_xlnx, assigned by
>   dwc3_xlnx_init_zynqmp() once init has succeeded.  The follow-up
>   cleanup series folds both pointers into the platform data struct.
> - Made dwc3_xlnx_exit_zynqmp() idempotent by returning early when
>   usb_resets_released is clear, rather than guarding only the reset
>   assertions.  phy_power_off() and phy_exit() decrement their counts
>   unconditionally, so an unbalanced second call would underflow them.
> - Rewrote the commit message to describe the bug rather than the
>   implementation, since the callback it referred to no longer exists at
>   this point in the series.
> - Added Cc: stable.
> ---
>  drivers/usb/dwc3/dwc3-xilinx.c | 44 +++++++++++++++++++++++++++++++++-
>  1 file changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> index d2315573b3d3..475bc19a1746 100644
> --- a/drivers/usb/dwc3/dwc3-xilinx.c
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -112,6 +112,39 @@ static int dwc3_xlnx_init_versal(struct dwc3_xlnx *priv_data)
>  	return 0;
>  }
>  
> +static void dwc3_xlnx_zynqmp_teardown(void *data)
> +{
> +	struct dwc3_xlnx *priv_data = data;
> +
> +	phy_power_off(priv_data->usb3_phy);
> +
> +	/*
> +	 * Undo only the reset cycle init performed.  As on the init error
> +	 * path, the unconditional deasserts are not paired with an assert
> +	 * here: with no usb3-phy, resetting the core could break a USB3
> +	 * configuration that is live but missing from the device tree.
> +	 */
> +	if (priv_data->usb3_phy) {
> +		reset_control_assert(priv_data->usb_hibrst);
> +		reset_control_assert(priv_data->usb_crst);
> +		reset_control_assert(priv_data->usb_apbrst);
> +	}
> +
> +	phy_exit(priv_data->usb3_phy);
> +}
> +
> +/*
> + * Run the platform teardown explicitly, so the resets are asserted while the
> + * clocks are still running.  devm_release_action() unlinks the action before
> + * calling it, so devres will not run it a second time.
> + */
> +static void dwc3_xlnx_release_teardown(struct device *dev,
> +				       struct dwc3_xlnx *priv_data)
> +{
> +	if (devm_is_action_added(dev, dwc3_xlnx_zynqmp_teardown, priv_data))
> +		devm_release_action(dev, dwc3_xlnx_zynqmp_teardown, priv_data);
> +}
> +
>  static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  {
>  	struct device		*dev = priv_data->dev;
> @@ -228,7 +261,8 @@ static int dwc3_xlnx_init_zynqmp(struct dwc3_xlnx *priv_data)
>  
>  	dwc3_xlnx_set_coherency(priv_data, XLNX_USB_TRAFFIC_ROUTE_CONFIG);
>  
> -	return 0;
> +	return devm_add_action_or_reset(dev, dwc3_xlnx_zynqmp_teardown,
> +					priv_data);
>  
>  err_phy_power_off:
>  	phy_power_off(priv_data->usb3_phy);
> @@ -355,6 +389,7 @@ static int dwc3_xlnx_probe(struct platform_device *pdev)
>  	pm_runtime_set_suspended(dev);
>  
>  err_clk_put:
> +	dwc3_xlnx_release_teardown(dev, priv_data);
>  	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
>  
>  	return ret;
> @@ -367,6 +402,13 @@ static void dwc3_xlnx_remove(struct platform_device *pdev)
>  
>  	of_platform_depopulate(dev);
>  
> +	/*
> +	 * This is also what makes .shutdown behave the same as .remove:
> +	 * device_shutdown() does not call devres_release_all(), so a teardown
> +	 * left to devres would never run on the shutdown path.
> +	 */
> +	dwc3_xlnx_release_teardown(dev, priv_data);
> +
>  	clk_bulk_disable_unprepare(priv_data->num_clocks, priv_data->clks);
>  	priv_data->num_clocks = 0;
>  
> -- 
> 2.44.4
> 

All the inline comments are quite verbose. Unless necessary, I'd prefer
keeping the detailed explanations in the commit message. But it's fine.

Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>

BR,
Thinh