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

[sashiko-bot@kernel.org]

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