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

[PATCH] 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.

This leaves PHY2's clk2 enabled and also drops an extra reference from
the current PHY's clk2, causing the prepare/enable accounting to become
unbalanced.

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

Signed-off-by: Xu Rao <raoxu@uniontech.com>
---
 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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

Re: [PATCH] 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] Concurrent usage of PHY2 results in hardware malfunction because `phy2->reset` is requested as an exclusive reset control.
- [High] The clock and reset teardown order for `phy2` in `sun4i_usb_phy_exit()` is inverted and asymmetric with its initialization.
--

commit 9c33e00d3375b4a5cb06dbdb11bc874c8960fcff
Author: Xu Rao <raoxu@uniontech.com>

phy: allwinner: sun4i-usb: disable the PHY2 PMU clock after SIDDQ setup

This commit fixes a clock accounting imbalance by ensuring the same PHY2
clk2 that was enabled for auxiliary PMU access is correctly disabled,
rather than disabling the current PHY's clk2.

> 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);
>  	}

[Severity: High]
This is a pre-existing issue, but since this patch deals with PHY2 resource
accounting, does the reset control for PHY2 also have a concurrent usage
issue?

In sun4i_usb_phy_probe(), the reset control is fetched exclusively:

sun4i_usb_phy_probe()
    ...
    phy->reset = devm_reset_control_get(dev, name);

Because it is an exclusive reset control, it does not maintain a reference
count for assertions. When multiple PHYs (like PHY0 and PHY1) independently
use PHY2, won't one PHY exiting unconditionally assert phy2->reset and
instantly break the other active PHYs?

sun4i_usb_phy_exit()
    ...
    reset_control_assert(phy2->reset);

[Severity: High]
This is also a pre-existing issue, but the clock and reset teardown order
for phy2 appears inverted in sun4i_usb_phy_exit() compared to how it is
safely initialized here in sun4i_usb_phy_init().

In sun4i_usb_phy_init(), phy2->clk is enabled before phy2->reset is
deasserted. However, in sun4i_usb_phy_exit(), the clock is disabled before
the reset is asserted:

sun4i_usb_phy_exit()
    ...
    clk_disable_unprepare(phy2->clk);
    reset_control_assert(phy2->reset);

Could asserting the reset on an unclocked hardware block leave the PHY2
block in an undefined or stuck state?

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

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Andre Przywara]

Hi,

On 7/6/26 11:35, raoxu wrote:
> 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.
> 
> This leaves PHY2's clk2 enabled and also drops an extra reference from
> the current PHY's clk2, causing the prepare/enable accounting to become
> unbalanced.
> 
> Disable the same PHY2 clk2 that was enabled for the auxiliary PMU access.
> 
> Signed-off-by: Xu Rao <raoxu@uniontech.com>
> ---
>   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);

Interesting, this looks about right, and matches the comment above, 
noting that phy2->clk2 is just temporarily needed. I don't remember 
further details, only that this workaround was quite annoying and messy ;-)

However I am wondering how this worked so far: This should sabotage the 
access to the local REG_HCI_PHY_CTL access in the next few lines ...
Any idea why this worked nevertheless?

Thanks,
Andre

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


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Xu Rao]

Hi Andre,

> > - clk_disable_unprepare(phy->clk2);
> > + clk_disable_unprepare(phy2->clk2);
>
> Interesting, this looks about right, and matches the comment above,
> noting that phy2->clk2 is just temporarily needed. I don't remember
> further details, only that this workaround was quite annoying and messy ;-)
>
> However I am wondering how this worked so far: This should sabotage the
> access to the local REG_HCI_PHY_CTL access in the next few lines ...
> Any idea why this worked nevertheless?

Yes, the current code is wrong, but the visible effect is limited.

In sun4i_usb_phy_init(), the auxiliary SIDDQ path explicitly enables
phy2->clk2 and then accesses phy2->pmu + REG_HCI_PHY_CTL. So the clock
that has to be disabled at the end of that block is phy2->clk2.

The current code disables phy->clk2 instead, which is a different object.
That does not undo the temporary phy2->clk2 enable.

The reason this has not shown up as an obvious functional failure is that
phy->clk2 is an optional PMU clock. On systems where the non-PHY2 PMU
clock is not provided, the wrong clk_disable_unprepare(phy->clk2) does
not actually disable a local PMU clock. In that case, the practical
visible issue is simply that the temporary phy2->clk2 enable is not
matched by a corresponding disable.

So the failure is not that the following local REG_HCI_PHY_CTL access is
known to break today. The concrete bug is that the auxiliary path enables
one clock and disables another one.

Thanks,
Xu Rao

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy

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

[Andre Przywara]

Hi,

On 7/7/26 05:21, Xu Rao wrote:
> Hi Andre,
> 
>>> - clk_disable_unprepare(phy->clk2);
>>> + clk_disable_unprepare(phy2->clk2);
>>
>> Interesting, this looks about right, and matches the comment above,
>> noting that phy2->clk2 is just temporarily needed. I don't remember
>> further details, only that this workaround was quite annoying and messy ;-)

I checked it again, and indeed we need this clock to access the PMU for 
PHY2, otherwise those PMU registers are RAZ/WI. Interestingly this only 
applies to PHY2, the other PHY's PMU area can be accessed without any 
extra clocks. And also this extra clock is actually CLK_BUS_EHCI2, so 
it's already enabled by the EHCI driver (if this port is enabled).

>> However I am wondering how this worked so far: This should sabotage the
>> access to the local REG_HCI_PHY_CTL access in the next few lines ...
>> Any idea why this worked nevertheless?
> 
> Yes, the current code is wrong, but the visible effect is limited.
> 
> In sun4i_usb_phy_init(), the auxiliary SIDDQ path explicitly enables
> phy2->clk2 and then accesses phy2->pmu + REG_HCI_PHY_CTL. So the clock
> that has to be disabled at the end of that block is phy2->clk2.
> 
> The current code disables phy->clk2 instead, which is a different object.
> That does not undo the temporary phy2->clk2 enable.
> 
> The reason this has not shown up as an obvious functional failure is that
> phy->clk2 is an optional PMU clock. On systems where the non-PHY2 PMU
> clock is not provided, the wrong clk_disable_unprepare(phy->clk2) does
> not actually disable a local PMU clock.

To be more precise: it's only PHY2 on the H616 that has this "clk2", for 
everyone else it's NULL and so any clock operations are NOPs. That's why 
disabling clk2 of PHY0, PHY1, or PHY3 does not do anything at all.

> In that case, the practical
> visible issue is simply that the temporary phy2->clk2 enable is not
> matched by a corresponding disable.

Yes, it purely a leak of this clock that's happening here. That explains 
why it worked before, and makes the fix good to go:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Can you add a sentence with the explanation of why disabling the wrong 
clock did not do any harm? Like: "No other PHY uses a clk2 clock, so the 
pointer was NULL, and disabling the wrong clock did nothing."

Cheers,
Andre.




> So the failure is not that the following local REG_HCI_PHY_CTL access is
> known to break today. The concrete bug is that the auxiliary path enables
> one clock and disables another one.
> 
> Thanks,
> Xu Rao


-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy