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@xxxxxxx>

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