[v5] PCI: rockchip: Improve driver quality

[RFC PATCH v5 3/4] phy: rockchip-pcie: Enable all four lanes

Posted by Geraldo Nascimento 3 months, 4 weeks ago

Current code enables only Lane 0 because pwr_cnt will be incremented
on first call to the function. Use for-loop to enable all 4 lanes
through GRF.

Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
index bd44af36c67a..48bcc7d2b33b 100644
--- a/drivers/phy/rockchip/phy-rockchip-pcie.c
+++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
@@ -176,11 +176,13 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 				   PHY_CFG_ADDR_MASK,
 				   PHY_CFG_ADDR_SHIFT));
 
-	regmap_write(rk_phy->reg_base,
-		     rk_phy->phy_data->pcie_laneoff,
-		     HIWORD_UPDATE(!PHY_LANE_IDLE_OFF,
-				   PHY_LANE_IDLE_MASK,
-				   PHY_LANE_IDLE_A_SHIFT + inst->index));
+	for (int i=0; i < PHY_MAX_LANE_NUM; i++) {
+		regmap_write(rk_phy->reg_base,
+			     rk_phy->phy_data->pcie_laneoff,
+			     HIWORD_UPDATE(!PHY_LANE_IDLE_OFF,
+					   PHY_LANE_IDLE_MASK,
+					   PHY_LANE_IDLE_A_SHIFT + i));
+	}
 
 	/*
 	 * No documented timeout value for phy operation below,
-- 
2.49.0

Re: [RFC PATCH v5 3/4] phy: rockchip-pcie: Enable all four lanes

Posted by Robin Murphy 3 months, 3 weeks ago

On 2025-06-13 6:03 pm, Geraldo Nascimento wrote:
> Current code enables only Lane 0 because pwr_cnt will be incremented
> on first call to the function. Use for-loop to enable all 4 lanes
> through GRF.

If this was really necessary, then surely it would also need the 
equivalent changes in rockchip_pcie_phy_power_off() too?

However, I'm not sure it *is* necessary - the NVMe on my RK3399 board 
happily claims to be using an x4 link, so I stuck a print of inst->index 
in this function, and sure enough I do see it being called for each 
instance already:

[    1.737479] phy phy-ff770000.syscon:pcie-phy.1: power_on 0
[    1.738810] phy phy-ff770000.syscon:pcie-phy.2: power_on 1
[    1.745193] phy phy-ff770000.syscon:pcie-phy.3: power_on 2
[    1.745196] phy phy-ff770000.syscon:pcie-phy.4: power_on 3

Thanks,
Robin.

> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> ---
>   drivers/phy/rockchip/phy-rockchip-pcie.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index bd44af36c67a..48bcc7d2b33b 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -176,11 +176,13 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
>   				   PHY_CFG_ADDR_MASK,
>   				   PHY_CFG_ADDR_SHIFT));
>   
> -	regmap_write(rk_phy->reg_base,
> -		     rk_phy->phy_data->pcie_laneoff,
> -		     HIWORD_UPDATE(!PHY_LANE_IDLE_OFF,
> -				   PHY_LANE_IDLE_MASK,
> -				   PHY_LANE_IDLE_A_SHIFT + inst->index));
> +	for (int i=0; i < PHY_MAX_LANE_NUM; i++) {
> +		regmap_write(rk_phy->reg_base,
> +			     rk_phy->phy_data->pcie_laneoff,
> +			     HIWORD_UPDATE(!PHY_LANE_IDLE_OFF,
> +					   PHY_LANE_IDLE_MASK,
> +					   PHY_LANE_IDLE_A_SHIFT + i));
> +	}
>   
>   	/*
>   	 * No documented timeout value for phy operation below,

Re: [RFC PATCH v5 3/4] phy: rockchip-pcie: Enable all four lanes

Posted by Geraldo Nascimento 3 months, 3 weeks ago

On Fri, Jun 20, 2025 at 01:04:46PM +0100, Robin Murphy wrote:
> On 2025-06-13 6:03 pm, Geraldo Nascimento wrote:
> > Current code enables only Lane 0 because pwr_cnt will be incremented
> > on first call to the function. Use for-loop to enable all 4 lanes
> > through GRF.
> 
> If this was really necessary, then surely it would also need the 
> equivalent changes in rockchip_pcie_phy_power_off() too?
> 
> However, I'm not sure it *is* necessary - the NVMe on my RK3399 board 
> happily claims to be using an x4 link, so I stuck a print of inst->index 
> in this function, and sure enough I do see it being called for each 
> instance already:
> 
> [    1.737479] phy phy-ff770000.syscon:pcie-phy.1: power_on 0
> [    1.738810] phy phy-ff770000.syscon:pcie-phy.2: power_on 1
> [    1.745193] phy phy-ff770000.syscon:pcie-phy.3: power_on 2
> [    1.745196] phy phy-ff770000.syscon:pcie-phy.4: power_on 3
> 

Hi Robin, and thanks for caring, it's excellent to rely on your
extensive expertise on ARM in general and RK3399 specifically!

However, on my board I'm positive it does not work without proposed
patch and I get stuck with x1 link without it.

There are currently very similar patches applied downstream to Armbian
and OpenWRT so at least I'm confident that is not only my board which is
quirky and other people experienced the same problem.

Thanks,
Geraldo Nascimento

> Thanks,
> Robin.

Re: [RFC PATCH v5 3/4] phy: rockchip-pcie: Enable all four lanes

Posted by Geraldo Nascimento 3 months, 3 weeks ago

On Fri, Jun 20, 2025 at 09:26:36AM -0300, Geraldo Nascimento wrote:
> On Fri, Jun 20, 2025 at 01:04:46PM +0100, Robin Murphy wrote:
> > On 2025-06-13 6:03 pm, Geraldo Nascimento wrote:
> > > Current code enables only Lane 0 because pwr_cnt will be incremented
> > > on first call to the function. Use for-loop to enable all 4 lanes
> > > through GRF.
> > 
> > If this was really necessary, then surely it would also need the 
> > equivalent changes in rockchip_pcie_phy_power_off() too?
> > 
> > However, I'm not sure it *is* necessary - the NVMe on my RK3399 board 
> > happily claims to be using an x4 link, so I stuck a print of inst->index 
> > in this function, and sure enough I do see it being called for each 
> > instance already:
> > 
> > [    1.737479] phy phy-ff770000.syscon:pcie-phy.1: power_on 0
> > [    1.738810] phy phy-ff770000.syscon:pcie-phy.2: power_on 1
> > [    1.745193] phy phy-ff770000.syscon:pcie-phy.3: power_on 2
> > [    1.745196] phy phy-ff770000.syscon:pcie-phy.4: power_on 3
> > 
> 
> Hi Robin, and thanks for caring, it's excellent to rely on your
> extensive expertise on ARM in general and RK3399 specifically!
> 
> However, on my board I'm positive it does not work without proposed
> patch and I get stuck with x1 link without it.
> 
> There are currently very similar patches applied downstream to Armbian
> and OpenWRT so at least I'm confident that is not only my board which is
> quirky and other people experienced the same problem.
> 
> Thanks,
> Geraldo Nascimento

Hello again Robin,

for reference, here's the commit for OpenWRT, originally from Armbian:
https://github.com/openwrt/openwrt/commit/2dc9801fe81ab3c092d2ca75e4c63f8d5eea46f5

Please note that the author of that commit specifically mentions a warm
reboot is needed to trigger the "stuck on x1" behavior. That author took
a different strategy than me, just reordering instead of using for-loop.
I'm open for different strategies, but the report is real I assure you.

Geraldo Nascimento

> 
> > Thanks,
> > Robin.

Re: [RFC PATCH v5 3/4] phy: rockchip-pcie: Enable all four lanes

Posted by Robin Murphy 3 months, 3 weeks ago

On 2025-06-20 1:26 pm, Geraldo Nascimento wrote:
> On Fri, Jun 20, 2025 at 01:04:46PM +0100, Robin Murphy wrote:
>> On 2025-06-13 6:03 pm, Geraldo Nascimento wrote:
>>> Current code enables only Lane 0 because pwr_cnt will be incremented
>>> on first call to the function. Use for-loop to enable all 4 lanes
>>> through GRF.
>>
>> If this was really necessary, then surely it would also need the
>> equivalent changes in rockchip_pcie_phy_power_off() too?
>>
>> However, I'm not sure it *is* necessary - the NVMe on my RK3399 board
>> happily claims to be using an x4 link, so I stuck a print of inst->index
>> in this function, and sure enough I do see it being called for each
>> instance already:
>>
>> [    1.737479] phy phy-ff770000.syscon:pcie-phy.1: power_on 0
>> [    1.738810] phy phy-ff770000.syscon:pcie-phy.2: power_on 1
>> [    1.745193] phy phy-ff770000.syscon:pcie-phy.3: power_on 2
>> [    1.745196] phy phy-ff770000.syscon:pcie-phy.4: power_on 3
>>
> 
> Hi Robin, and thanks for caring, it's excellent to rely on your
> extensive expertise on ARM in general and RK3399 specifically!
> 
> However, on my board I'm positive it does not work without proposed
> patch and I get stuck with x1 link without it.
> 
> There are currently very similar patches applied downstream to Armbian
> and OpenWRT so at least I'm confident that is not only my board which is
> quirky and other people experienced the same problem.

Ah, I put that print at the top of the function - on second look now I
see that there's an awkward mix of per-lane and global data, and pwr_cnt
is actually the latter. Sure enough, moving the print past that check I
only see it once.

However, I still don't think blindly enabling all the lanes is the right
thing to do either; I'd imagine something like the (untested) diff below
would be more appropriate. That would then seem to balance with what
power_off is doing.

Thanks,
Robin.

----->8-----
diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
index bd44af36c67a..a34a983db16c 100644
--- a/drivers/phy/rockchip/phy-rockchip-pcie.c
+++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
@@ -160,11 +160,8 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
  
  	guard(mutex)(&rk_phy->pcie_mutex);
  
-	if (rk_phy->pwr_cnt++) {
-		return 0;
-	}
-
-	err = reset_control_deassert(rk_phy->phy_rst);
+	if (rk_phy->pwr_cnt++)
+		err = reset_control_deassert(rk_phy->phy_rst);
  	if (err) {
  		dev_err(&phy->dev, "deassert phy_rst err %d\n", err);
  		rk_phy->pwr_cnt--;
@@ -181,6 +178,8 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
  		     HIWORD_UPDATE(!PHY_LANE_IDLE_OFF,
  				   PHY_LANE_IDLE_MASK,
  				   PHY_LANE_IDLE_A_SHIFT + inst->index));
+	if (rk_phy->pwr_cnt)
+		return 0;
  
  	/*
  	 * No documented timeout value for phy operation below,

Re: [RFC PATCH v5 3/4] phy: rockchip-pcie: Enable all four lanes

Posted by Geraldo Nascimento 3 months, 3 weeks ago

On Fri, Jun 20, 2025 at 01:47:35PM +0100, Robin Murphy wrote:
> Ah, I put that print at the top of the function - on second look now I
> see that there's an awkward mix of per-lane and global data, and pwr_cnt
> is actually the latter. Sure enough, moving the print past that check I
> only see it once.

Hi Robin, thanks for re-testing and no worries.

> 
> However, I still don't think blindly enabling all the lanes is the right
> thing to do either; I'd imagine something like the (untested) diff below
> would be more appropriate. That would then seem to balance with what
> power_off is doing.

Thanks for the suggestion, I'll make sure to test it appropriately
before sending v6.

Thanks!
Geraldo Nascimento

> 
> Thanks,
> Robin.
> 
> ----->8-----
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index bd44af36c67a..a34a983db16c 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -160,11 +160,8 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
>   
>   	guard(mutex)(&rk_phy->pcie_mutex);
>   
> -	if (rk_phy->pwr_cnt++) {
> -		return 0;
> -	}
> -
> -	err = reset_control_deassert(rk_phy->phy_rst);
> +	if (rk_phy->pwr_cnt++)
> +		err = reset_control_deassert(rk_phy->phy_rst);
>   	if (err) {
>   		dev_err(&phy->dev, "deassert phy_rst err %d\n", err);
>   		rk_phy->pwr_cnt--;
> @@ -181,6 +178,8 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
>   		     HIWORD_UPDATE(!PHY_LANE_IDLE_OFF,
>   				   PHY_LANE_IDLE_MASK,
>   				   PHY_LANE_IDLE_A_SHIFT + inst->index));
> +	if (rk_phy->pwr_cnt)
> +		return 0;
>   
>   	/*
>   	 * No documented timeout value for phy operation below,
>