[PATCH v2] phy: allwinner: sun4i-usb: disable the PHY2 PMU clock after SIDDQ setup

[PATCH v2] phy: allwinner: sun4i-usb: disable the PHY2 PMU clock after SIDDQ setup

[raoxu]

From: Xu Rao <raoxu@uniontech.com>

sun4i_usb_phy_init() temporarily enables PHY2's clk2 when a SoC needs
PHY2 SIDDQ setup while initializing another PHY. However, after updating
PHY2's PMU register it disables the clk2 pointer from the PHY currently
being initialized instead.

Only PHY2 on H616 has a clk2 clock. No other PHY on H616 has a clk2
clock, so phy->clk2 is NULL in this path and disabling it is a no-op.
This is why disabling the wrong clock did not cause an additional
functional failure.

The temporary phy2->clk2 enable therefore remains unmatched, leaking a
clock enable reference each time this path is executed.

Disable the same PHY2 clk2 that was enabled for the auxiliary PMU access.

Fixes: b45c6d80325b ("phy: sun4i-usb: Introduce port2 SIDDQ quirk")
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
Changes in v2:
- Explain why disabling phy->clk2 did not cause additional harm: no
  other PHY on H616 has clk2, so phy->clk2 is NULL and the operation is
  a no-op.
- Clarify that the concrete issue is the unmatched PHY2 clk2 enable.
- Add the Fixes tag and Andre's Reviewed-by.

 drivers/phy/allwinner/phy-sun4i-usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
index e2fbf8ccf99e..839856c09e30 100644
--- a/drivers/phy/allwinner/phy-sun4i-usb.c
+++ b/drivers/phy/allwinner/phy-sun4i-usb.c
@@ -318,7 +318,7 @@ static int sun4i_usb_phy_init(struct phy *_phy)
 			writel(val, phy2->pmu + REG_HCI_PHY_CTL);
 		}

-		clk_disable_unprepare(phy->clk2);
+		clk_disable_unprepare(phy2->clk2);
 	}

 	if (phy->pmu && data->cfg->hci_phy_ctl_clear) {
--
2.50.1

Re: [PATCH v2] phy: allwinner: sun4i-usb: disable the PHY2 PMU clock after SIDDQ setup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Exclusive reset `phy2->reset` is forcefully asserted on exit, breaking other concurrently active PHYs that rely on the shared PHY2 hardware.
- [Medium] Incorrect teardown order for `phy2` in `sun4i_usb_phy_exit()` disables the clock before asserting the reset.
--

--- Patch [1]: [PATCH v2] phy: allwinner: sun4i-usb: disable the PHY2 PMU clock after SIDDQ setup ---
commit c3b10e6864aadde53d10b7e0b2a5fcb7d0d76e08
Author: Xu Rao <raoxu@uniontech.com>
Subject: phy: allwinner: sun4i-usb: disable the PHY2 PMU clock after SIDDQ setup

This patch fixes a clock enable leak in sun4i_usb_phy_init() by properly
disabling PHY2's clk2 instead of the currently initialized PHY's clk2. It
corrects an unmatched clk_prepare_enable() for the auxiliary PMU access.

> diff --git a/drivers/phy/allwinner/phy-sun4i-usb.c b/drivers/phy/allwinner/phy-sun4i-usb.c
> index e2fbf8ccf99e1..839856c09e305 100644
> --- a/drivers/phy/allwinner/phy-sun4i-usb.c
> +++ b/drivers/phy/allwinner/phy-sun4i-usb.c
> @@ -318,7 +318,7 @@ static int sun4i_usb_phy_init(struct phy *_phy)
>  			writel(val, phy2->pmu + REG_HCI_PHY_CTL);
>  		}
>  
> -		clk_disable_unprepare(phy->clk2);
> +		clk_disable_unprepare(phy2->clk2);
>  	}
>  
>  	if (phy->pmu && data->cfg->hci_phy_ctl_clear) {

[Severity: Medium]
This isn't a bug introduced by this patch, but looking at how PHY2 is torn
down later in sun4i_usb_phy_exit():

drivers/phy/allwinner/phy-sun4i-usb.c:sun4i_usb_phy_exit() {
	if (data->cfg->needs_phy2_siddq && phy->index != 2) {
		struct sun4i_usb_phy *phy2 = &data->phys[2];

		clk_disable_unprepare(phy2->clk);
		reset_control_assert(phy2->reset);
	}
[ ... ]
}

Is the clock and reset teardown order correct here?

The teardown order disables the clock before asserting the reset, which
reverses the initialization order.

Asserting a hardware reset without the IP block's clock running can sometimes
fail to propagate the reset state or cause a bus lockup.

[Severity: High]
This is also a pre-existing issue, but does asserting this shared
phy2->reset line on exit break other concurrently active PHYs that rely on
PHY2?

Because phy->reset is requested in sun4i_usb_phy_probe() as an exclusive
reset:

drivers/phy/allwinner/phy-sun4i-usb.c:sun4i_usb_phy_probe() {
[ ... ]
		snprintf(name, sizeof(name), "usb%d_reset", i);
		phy->reset = devm_reset_control_get(dev, name);
[ ... ]
}

The reset framework won't refcount it, so unconditionally calling
reset_control_assert(phy2->reset) will force the hardware line into reset
immediately.

This could tear down the shared hardware resource regardless of other active
PHYs using it.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/CE85F490F5646A2C+20260813130542.2425068-1-raoxu@uniontech.com?part=1