Re: [PATCH] phy: rockchip: snps-pcie3: Raise the SRAM init timeout

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] phy: rockchip: 20ms busy-wait loop in SRAM init polling stalls CPU
--

commit b3f51ba06ba11802539460b6b1bb17b8564adf62
Author: Shawn Lin <shawn.lin@rock-chips.com>
Date:   Thu Oct 15 07:02:55 2026 +0000

phy: rockchip: snps-pcie3: Raise the SRAM init timeout

This commit increases the SRAM initialization timeout for the PCIe3 PHY from
500us to 20ms. The longer timeout accommodates slower power-up sequences,
such as those at low temperatures, to prevent false lock failure reports.

> diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> index 4e8ffd173096a..bbbfd6d2e369b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
> +++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c

[ ... ]

> @@ -127,7 +129,7 @@ static int rockchip_p3phy_rk3568_init(struct rockchip_p3phy_priv *priv)
>  	ret = regmap_read_poll_timeout(priv->phy_grf,
>  				       GRF_PCIE30PHY_STATUS0,
>  				       reg, SRAM_INIT_DONE(reg),
> -				       0, 500);
> +				       0, RK_PCIE_SRAM_INIT_TIMEOUT);

[Severity: Medium]
When the sleep_us parameter is 0, regmap_read_poll_timeout() uses cpu_relax()
and tight-loops instead of sleeping.

With the timeout increased from 500us to 20,000us (20ms), does this create an
excessively long busy-wait that could stall the CPU?

Since this initialization runs during driver probe or PHY power-on, which
happen under a mutex where sleeping is permitted, would it be better to provide
a non-zero sleep interval (like 1000) so the CPU can be yielded during this
extended 20ms wait?

>  	if (ret)
>  		dev_err(&priv->phy->dev, "%s: lock failed 0x%x, check input refclk and power supply\n",
>  		       __func__, reg);

[ ... ]

> @@ -187,11 +189,11 @@ static int rockchip_p3phy_rk3588_init(struct rockchip_p3phy_priv *priv)
>  	ret = regmap_read_poll_timeout(priv->phy_grf,
>  				       RK3588_PCIE3PHY_GRF_PHY0_STATUS1,
>  				       reg, RK3588_SRAM_INIT_DONE(reg),
> -				       0, 500);
> +				       0, RK_PCIE_SRAM_INIT_TIMEOUT);

[Severity: Medium]
Similarly here, for the RK3588 init path, does leaving the sleep interval at 0
mean we could spend up to 20ms busy-waiting?

>  	ret |= regmap_read_poll_timeout(priv->phy_grf,
>  					RK3588_PCIE3PHY_GRF_PHY1_STATUS1,
>  					reg, RK3588_SRAM_INIT_DONE(reg),
> -					0, 500);
> +					0, RK_PCIE_SRAM_INIT_TIMEOUT);

[Severity: Medium]
And since this second poll runs immediately after the first one, could we end
up busy-waiting for up to 40ms in total if both lanes are slow to lock?

>  	if (ret)
>  		dev_err(&priv->phy->dev, "lock failed 0x%x, check input refclk and power supply\n",
>  			reg);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1792047775-77954-1-git-send-email-shawn.lin@rock-chips.com?part=1

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