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

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 <[email protected]>
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/[email protected]?part=1

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy