Re: [PATCH v2 2/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY RX probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The phy_configure callback accesses hardware registers while the device is still in reset and without ensuring the PM domain is active.
- [High] The driver acquires and configures clocks but never enables them.
--

--- Patch [2]: [PATCH v2 2/2] phy: starfive: Fix runtime PM cleanup in JH7110 DPHY RX probe ---
commit fe8a50ded4b6e0393fd6d6a27d22735266427a91
Author: Can Peng <pengcan@kylinos.cn>

phy: starfive: Fix runtime PM cleanup in JH7110 DPHY RX probe

stf_dphy_probe() enables runtime PM before registering the PHY provider.
If devm_of_phy_provider_register() fails, probe returns with runtime PM
still enabled. This commit uses devm_pm_runtime_enable() to ensure
runtime PM is disabled automatically on later probe failures or unbind.

> diff --git a/drivers/phy/starfive/phy-jh7110-dphy-rx.c b/drivers/phy/starfive/phy-jh7110-dphy-rx.c
> index 0b039e1f71c55..d06f21ad63325 100644
> --- a/drivers/phy/starfive/phy-jh7110-dphy-rx.c
> +++ b/drivers/phy/starfive/phy-jh7110-dphy-rx.c
> @@ -150,6 +150,7 @@ static int stf_dphy_probe(struct platform_device *pdev)
>  {
>  	struct phy_provider *phy_provider;
>  	struct stf_dphy *dphy;
> +	int ret;
>  
>  	dphy = devm_kzalloc(&pdev->dev, sizeof(*dphy), GFP_KERNEL);
>  	if (!dphy)
[ ... ]
> @@ -190,7 +191,9 @@ static int stf_dphy_probe(struct platform_device *pdev)
>  		return PTR_ERR(dphy->phy);
>  	}
>  
> -	pm_runtime_enable(&pdev->dev);
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
>  
>  	phy_set_drvdata(dphy->phy, dphy);
>  	phy_provider = devm_of_phy_provider_register(&pdev->dev,

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the PM
changes, I noticed an issue in stf_dphy_configure().

The phy_configure callback accesses hardware registers directly:

stf_dphy_configure() {
    ...
    writel(FIELD_PREP(STF_DPHY_ENABLE_CLK, 1) |
           FIELD_PREP(STF_DPHY_ENABLE_CLK1, 1) |
    ...
}

However, the PM domain is resumed and the reset is deasserted later in
stf_dphy_power_on(). 

Because the RX driver lacks a phy_init() callback, if a consumer (like
a MIPI CSI-2/DSI controller) calls phy_configure() prior to phy_power_on(),
will this cause a synchronous external abort or silent write failure by
accessing registers of a device that is still held in reset and unpowered?

[Severity: High]
This is also a pre-existing issue, but I noticed a potential problem in
stf_dphy_power_on() regarding clock enablement.

The driver acquires cfg_clk, ref_clk, and tx_clk in probe() and sets their
rates in stf_dphy_power_on():

stf_dphy_power_on() {
    ...
    clk_set_rate(dphy->cfg_clk, 99000000);
    clk_set_rate(dphy->ref_clk, 49500000);
    clk_set_rate(dphy->tx_clk, 19800000);
    ...
}

Are these clocks ever actually enabled? I do not see any corresponding
clk_prepare_enable() calls in the driver. Will the PHY hardware fail to
operate or hang on register access if the clocks are not implicitly kept on
by the bootloader or a PM domain?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260718090054.444513-1-pengcan@kylinos.cn?part=2

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