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

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

[Shawn Lin]

The SRAM initialization of the PCIe3 PHY may take longer than the 500us
currently allowed, especially on a slow power-up such as at low
temperature. In that case the driver reports

  lock failed 0x..., check input refclk and power supply

even though the input refclk and power supply are fine.

Raise the timeout to 20ms and use a named constant for it, applied to
both the RK3568 and RK3588 paths.

A non-zero sleep interval (1ms) is passed to the polling helpers so the
CPU is yielded between reads. With sleep_us == 0 the helpers tight-loop,
which would otherwise burn the CPU during the now much longer wait.

The two PHY lanes share most of their circuitry in both aggregation and
bifurcation mode, so in practice once the first PHY locks the second one
can be assumed to lock as well. The two sequential polls therefore do
not add any noticeable delay.

Signed-off-by: Shawn Lin <shawn.lin@rock-chips.com>

---

Changes in v2:
- address Sashiko's comment

 drivers/phy/rockchip/phy-rockchip-snps-pcie3.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
index 4e8ffd1..69bda8b 100644
--- a/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
+++ b/drivers/phy/rockchip/phy-rockchip-snps-pcie3.c
@@ -19,6 +19,8 @@
 #include <linux/regmap.h>
 #include <linux/reset.h>
 
+#define RK_PCIE_SRAM_INIT_TIMEOUT		20000
+
 /* Register for RK3568 */
 #define GRF_PCIE30PHY_CON1			0x4
 #define GRF_PCIE30PHY_CON6			0x18
@@ -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);
+				       1000, RK_PCIE_SRAM_INIT_TIMEOUT);
 	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);
+				       1000, RK_PCIE_SRAM_INIT_TIMEOUT);
 	ret |= regmap_read_poll_timeout(priv->phy_grf,
 					RK3588_PCIE3PHY_GRF_PHY1_STATUS1,
 					reg, RK3588_SRAM_INIT_DONE(reg),
-					0, 500);
+					1000, RK_PCIE_SRAM_INIT_TIMEOUT);
 	if (ret)
 		dev_err(&priv->phy->dev, "lock failed 0x%x, check input refclk and power supply\n",
 			reg);
-- 
2.7.4


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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] phy: rockchip: misleading log and bitwise OR of error codes
--

commit 3bcb5033387e2e5fffc56e29d4197fecdb178968
Author: Shawn Lin <shawn.lin@rock-chips.com>

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

The SRAM initialization of the PCIe3 PHY may take longer than the 500us
currently allowed. This commit raises the timeout to 20ms and uses a non-zero
sleep interval to yield the CPU instead of tight-looping.

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

[ ... ]

> @@ -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);
> +				       1000, RK_PCIE_SRAM_INIT_TIMEOUT);
>  	ret |= regmap_read_poll_timeout(priv->phy_grf,
>  					RK3588_PCIE3PHY_GRF_PHY1_STATUS1,
>  					reg, RK3588_SRAM_INIT_DONE(reg),
> -					0, 500);
> +					1000, RK_PCIE_SRAM_INIT_TIMEOUT);
>  	if (ret)
>  		dev_err(&priv->phy->dev, "lock failed 0x%x, check input refclk and power supply\n",
>  			reg);

[Severity: Low]
Does this sequence safely handle errors when the first poll times out but the
second one succeeds?

Looking at rockchip_p3phy_rk3588_init(), if the first
regmap_read_poll_timeout() fails, it will return an error code and leave reg
at its failing value. However, the bitwise OR assignment executes the second
poll regardless of the first poll's outcome.

If the second poll succeeds, reg is overwritten with a successful state.
The bitwise OR will combine the error code with the second poll's return
value, which might corrupt the error code returned to the caller.

Additionally, the error check evaluates if the combined ret is non-zero,
triggering the error log. The log will print the potentially successful reg
value from the second lane, which could mislead developers investigating lock
failures.

Should the second poll be skipped if the first one fails, or should separate
variables be used to track the status of each lane?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/1792051843-93245-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