[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

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
> 

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

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