[RFC PATCH v5 0/4] PCI: rockchip: Improve driver quality

[RFC PATCH v5 0/4] PCI: rockchip: Improve driver quality

[Geraldo Nascimento]

During a 30-day debugging-run fighting quirky PCIe devices on RK3399
some quality improvements began to take form and this is my attempt
at upstreaming it. It will ensure maximum chance of retraining to Gen2
5.0GT/s, on all four lanes and plus if anybody is debugging the PHY
they'll now get real values from TEST_I[3:0] for every TEST_ADDR[4:0]
without risk of locking up kernel like with present broken async
strobe TEST_WRITE.

---
V4 -> V5: fix build failure, reflow commit messages and also convert
registers for EP operation, all suggested by Ilpo
V3 -> V4: fix setting-up of TLS in Link Control and Status Register 2,
also adjust commit titles
V2 -> V3: correctly clean-up with standard PCIe defines as per Bjorn's
suggestion
V1 -> V2: use standard PCIe defines as suggested by Bjorn


Geraldo Nascimento (4):
  PCI: rockchip: Use standard PCIe defines
  PCI: rockchip: Set Target Link Speed before retraining
  phy: rockchip-pcie: Enable all four lanes
  phy: rockchip-pcie: Adjust read mask and write

 drivers/pci/controller/pcie-rockchip-ep.c   |  4 +-
 drivers/pci/controller/pcie-rockchip-host.c | 49 ++++++++++++---------
 drivers/pci/controller/pcie-rockchip.h      | 12 +----
 drivers/phy/rockchip/phy-rockchip-pcie.c    | 16 ++++---
 4 files changed, 39 insertions(+), 42 deletions(-)

-- 
2.49.0

[RFC PATCH v5 1/4] PCI: rockchip: Use standard PCIe defines

[Geraldo Nascimento]

Current code uses custom-defined register offsets and bitfields
for standard PCIe registers. Change to using standard PCIe
defines. Since we are now using standard PCIe defines, drop
unused custom-defined ones, which are now referenced from
offset at added Capabilities Register.

Suggested-By: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
---
 drivers/pci/controller/pcie-rockchip-ep.c   |  4 +-
 drivers/pci/controller/pcie-rockchip-host.c | 45 +++++++++++----------
 drivers/pci/controller/pcie-rockchip.h      | 12 +-----
 3 files changed, 26 insertions(+), 35 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-ep.c b/drivers/pci/controller/pcie-rockchip-ep.c
index 55416b8311dd..300cd85fa035 100644
--- a/drivers/pci/controller/pcie-rockchip-ep.c
+++ b/drivers/pci/controller/pcie-rockchip-ep.c
@@ -518,9 +518,9 @@ static void rockchip_pcie_ep_retrain_link(struct rockchip_pcie *rockchip)
 {
 	u32 status;
 
-	status = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_LCS);
+	status = rockchip_pcie_read(rockchip, PCIE_EP_CONFIG_BASE + PCI_EXP_LNKCTL);
 	status |= PCI_EXP_LNKCTL_RL;
-	rockchip_pcie_write(rockchip, status, PCIE_EP_CONFIG_LCS);
+	rockchip_pcie_write(rockchip, status, PCIE_EP_CONFIG_BASE + PCI_EXP_LNKCTL);
 }
 
 static bool rockchip_pcie_ep_link_up(struct rockchip_pcie *rockchip)
diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index b9e7a8710cf0..8489d51e01ca 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -40,18 +40,18 @@ static void rockchip_pcie_enable_bw_int(struct rockchip_pcie *rockchip)
 {
 	u32 status;
 
-	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
+	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 	status |= (PCI_EXP_LNKCTL_LBMIE | PCI_EXP_LNKCTL_LABIE);
-	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 }
 
 static void rockchip_pcie_clr_bw_int(struct rockchip_pcie *rockchip)
 {
 	u32 status;
 
-	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
+	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 	status |= (PCI_EXP_LNKSTA_LBMS | PCI_EXP_LNKSTA_LABS) << 16;
-	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 }
 
 static void rockchip_pcie_update_txcredit_mui(struct rockchip_pcie *rockchip)
@@ -269,7 +269,7 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 	scale = 3; /* 0.001x */
 	curr = curr / 1000; /* convert to mA */
 	power = (curr * 3300) / 1000; /* milliwatt */
-	while (power > PCIE_RC_CONFIG_DCR_CSPL_LIMIT) {
+	while (power > FIELD_MAX(PCI_EXP_DEVCAP_PWR_VAL)) {
 		if (!scale) {
 			dev_warn(rockchip->dev, "invalid power supply\n");
 			return;
@@ -278,10 +278,10 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 		power = power / 10;
 	}
 
-	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCR);
-	status |= (power << PCIE_RC_CONFIG_DCR_CSPL_SHIFT) |
-		  (scale << PCIE_RC_CONFIG_DCR_CPLS_SHIFT);
-	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCR);
+	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCAP);
+	status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_VAL, power);
+	status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_SCL, scale);
+	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCAP);
 }
 
 /**
@@ -309,14 +309,14 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 	rockchip_pcie_set_power_limit(rockchip);
 
 	/* Set RC's clock architecture as common clock */
-	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
+	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 	status |= PCI_EXP_LNKSTA_SLC << 16;
-	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 
 	/* Set RC's RCB to 128 */
-	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
+	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 	status |= PCI_EXP_LNKCTL_RCB;
-	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 
 	/* Enable Gen1 training */
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
@@ -341,9 +341,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 		 * Enable retrain for gen2. This should be configured only after
 		 * gen1 finished.
 		 */
-		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LCS);
+		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);
+		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 		status |= PCI_EXP_LNKCTL_RL;
-		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LCS);
+		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 
 		err = readl_poll_timeout(rockchip->apb_base + PCIE_CORE_CTRL,
 					 status, PCIE_LINK_IS_GEN2(status), 20,
@@ -380,15 +381,15 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 
 	/* Clear L0s from RC's link cap */
 	if (of_property_read_bool(dev->of_node, "aspm-no-l0s")) {
-		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_LINK_CAP);
-		status &= ~PCIE_RC_CONFIG_LINK_CAP_L0S;
-		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_LINK_CAP);
+		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
+		status &= ~PCI_EXP_LNKCAP_ASPM_L0S;
+		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
 	}
 
-	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_DCSR);
-	status &= ~PCIE_RC_CONFIG_DCSR_MPS_MASK;
-	status |= PCIE_RC_CONFIG_DCSR_MPS_256;
-	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_DCSR);
+	status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL);
+	status &= ~PCI_EXP_DEVCTL_PAYLOAD;
+	status |= PCI_EXP_DEVCTL_PAYLOAD_256B;
+	rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_DEVCTL);
 
 	return 0;
 err_power_off_phy:
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 5864a20323f2..f5cbf3c9d2d9 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -155,17 +155,7 @@
 #define PCIE_EP_CONFIG_DID_VID		(PCIE_EP_CONFIG_BASE + 0x00)
 #define PCIE_EP_CONFIG_LCS		(PCIE_EP_CONFIG_BASE + 0xd0)
 #define PCIE_RC_CONFIG_RID_CCR		(PCIE_RC_CONFIG_BASE + 0x08)
-#define PCIE_RC_CONFIG_DCR		(PCIE_RC_CONFIG_BASE + 0xc4)
-#define   PCIE_RC_CONFIG_DCR_CSPL_SHIFT		18
-#define   PCIE_RC_CONFIG_DCR_CSPL_LIMIT		0xff
-#define   PCIE_RC_CONFIG_DCR_CPLS_SHIFT		26
-#define PCIE_RC_CONFIG_DCSR		(PCIE_RC_CONFIG_BASE + 0xc8)
-#define   PCIE_RC_CONFIG_DCSR_MPS_MASK		GENMASK(7, 5)
-#define   PCIE_RC_CONFIG_DCSR_MPS_256		(0x1 << 5)
-#define PCIE_RC_CONFIG_LINK_CAP		(PCIE_RC_CONFIG_BASE + 0xcc)
-#define   PCIE_RC_CONFIG_LINK_CAP_L0S		BIT(10)
-#define PCIE_RC_CONFIG_LCS		(PCIE_RC_CONFIG_BASE + 0xd0)
-#define PCIE_EP_CONFIG_LCS		(PCIE_EP_CONFIG_BASE + 0xd0)
+#define PCIE_RC_CONFIG_CR		(PCIE_RC_CONFIG_BASE + 0xc0)
 #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
 #define PCIE_RC_CONFIG_THP_CAP		(PCIE_RC_CONFIG_BASE + 0x274)
 #define   PCIE_RC_CONFIG_THP_CAP_NEXT_MASK	GENMASK(31, 20)
-- 
2.49.0

[RFC PATCH v5 2/4] PCI: rockchip: Set Target Link Speed before retraining

[Geraldo Nascimento]

Current code may fail Gen2 retraining if Target Link Speed
is set to 2.5 GT/s in Link Control and Status Register 2.
Set it to 5.0 GT/s accordingly.

Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 8489d51e01ca..467e3fc377f7 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -341,6 +341,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 		 * Enable retrain for gen2. This should be configured only after
 		 * gen1 finished.
 		 */
+		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);
+		status &= ~PCI_EXP_LNKCTL2_TLS;
+		status |= PCI_EXP_LNKCTL2_TLS_5_0GT;
+		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);
 		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);
 		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
 		status |= PCI_EXP_LNKCTL_RL;
-- 
2.49.0

Re: [RFC PATCH v5 2/4] PCI: rockchip: Set Target Link Speed before retraining

[Geraldo Nascimento]

On Fri, Jun 13, 2025 at 02:03:50PM -0300, Geraldo Nascimento wrote:
> Current code may fail Gen2 retraining if Target Link Speed
> is set to 2.5 GT/s in Link Control and Status Register 2.
> Set it to 5.0 GT/s accordingly.
> 
> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> ---
>  drivers/pci/controller/pcie-rockchip-host.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 8489d51e01ca..467e3fc377f7 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -341,6 +341,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>  		 * Enable retrain for gen2. This should be configured only after
>  		 * gen1 finished.
>  		 */
> +		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);
> +		status &= ~PCI_EXP_LNKCTL2_TLS;
> +		status |= PCI_EXP_LNKCTL2_TLS_5_0GT;
> +		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);
>  		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);

Hi,

I see rockchip_pcie_write() was added twice, in this patch and also in
1/4.

I'll send v6 with correction after I get some reviews.

Thank you,
Geraldo Nascimento

>  		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>  		status |= PCI_EXP_LNKCTL_RL;
> -- 
> 2.49.0
> 

Re: [RFC PATCH v5 2/4] PCI: rockchip: Set Target Link Speed before retraining

[Robin Murphy]

On 2025-06-13 6:03 pm, Geraldo Nascimento wrote:
> Current code may fail Gen2 retraining if Target Link Speed
> is set to 2.5 GT/s in Link Control and Status Register 2.
> Set it to 5.0 GT/s accordingly.

I have max-link-speed overridden to 2 in my local DTB, and indeed this 
seems to make my NVMe report a 5.0 GT/s link where previously it was 
still downgrading to 2.5, so:

Tested-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> ---
>   drivers/pci/controller/pcie-rockchip-host.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
> index 8489d51e01ca..467e3fc377f7 100644
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -341,6 +341,10 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
>   		 * Enable retrain for gen2. This should be configured only after
>   		 * gen1 finished.
>   		 */
> +		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);
> +		status &= ~PCI_EXP_LNKCTL2_TLS;
> +		status |= PCI_EXP_LNKCTL2_TLS_5_0GT;
> +		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);
>   		rockchip_pcie_write(rockchip, status, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL2);
>   		status = rockchip_pcie_read(rockchip, PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
>   		status |= PCI_EXP_LNKCTL_RL;

Re: [RFC PATCH v5 2/4] PCI: rockchip: Set Target Link Speed before retraining

[Geraldo Nascimento]

On Fri, Jun 20, 2025 at 01:33:11PM +0100, Robin Murphy wrote:
> On 2025-06-13 6:03 pm, Geraldo Nascimento wrote:
> > Current code may fail Gen2 retraining if Target Link Speed
> > is set to 2.5 GT/s in Link Control and Status Register 2.
> > Set it to 5.0 GT/s accordingly.
> 
> I have max-link-speed overridden to 2 in my local DTB, and indeed this 
> seems to make my NVMe report a 5.0 GT/s link where previously it was 
> still downgrading to 2.5, so:
> 
> Tested-by: Robin Murphy <robin.murphy@arm.com>
>

Hi Robin,

thanks for the testing, I'll include the tag in v6 once Bjorn gets back
to me on the 16-bit adjacent registers problem.

Geraldo Nascimento

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

[Geraldo Nascimento]

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

[Robin Murphy]

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

[Geraldo Nascimento]

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

[Robin Murphy]

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

[Geraldo Nascimento]

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,
> 

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

[Geraldo Nascimento]

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.

[RFC PATCH v5 4/4] phy: rockchip-pcie: Adjust read mask and write

[Geraldo Nascimento]

Section 17.6.10 of the RK3399 TRM "PCIe PIPE PHY registers Description"
defines asynchronous strobe TEST_WRITE which should be enabled then
disabled and seems to have been copy-pasted as of current. Adjust it.
While at it, adjust read mask which should be the same as write mask.

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

diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
index 48bcc7d2b33b..35d2523ee776 100644
--- a/drivers/phy/rockchip/phy-rockchip-pcie.c
+++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
@@ -30,9 +30,9 @@
 #define PHY_CFG_ADDR_SHIFT    1
 #define PHY_CFG_DATA_MASK     0xf
 #define PHY_CFG_ADDR_MASK     0x3f
-#define PHY_CFG_RD_MASK       0x3ff
+#define PHY_CFG_RD_MASK       0x3f
 #define PHY_CFG_WR_ENABLE     1
-#define PHY_CFG_WR_DISABLE    1
+#define PHY_CFG_WR_DISABLE    0
 #define PHY_CFG_WR_SHIFT      0
 #define PHY_CFG_WR_MASK       1
 #define PHY_CFG_PLL_LOCK      0x10
-- 
2.49.0

Re: [RFC PATCH v5 4/4] phy: rockchip-pcie: Adjust read mask and write

[Robin Murphy]

On 2025-06-13 6:04 pm, Geraldo Nascimento wrote:
> Section 17.6.10 of the RK3399 TRM "PCIe PIPE PHY registers Description"
> defines asynchronous strobe TEST_WRITE which should be enabled then
> disabled and seems to have been copy-pasted as of current. Adjust it.

FWIW that's a bit hard to make sense of, given that it bears no relation 
whatsoever to the naming used in the code :/

(Not least because the mapping of register fields to phy signals here is 
really a property of GRF_SOC_CON8 rather than the phy itself)

> While at it, adjust read mask which should be the same as write mask.

Which write mask? Certainly not PHY_CFG_WR_MASK... However as this 
definition is unused since 64cdc0360811 ("phy: rockchip-pcie: remove 
unused phy_rd_cfg function"), I don't see much point in touching it 
other than to remove it entirely. If it is the case that only the 
address field is significant for whatever a "read" operation actually 
means, well then that's just another job for ADDR_MASK (which I guess is 
what the open-coded business with PHY_CFG_PLL_LOCK is actually doing...)

Thanks,
Robin.

> Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> ---
>   drivers/phy/rockchip/phy-rockchip-pcie.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
> index 48bcc7d2b33b..35d2523ee776 100644
> --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> @@ -30,9 +30,9 @@
>   #define PHY_CFG_ADDR_SHIFT    1
>   #define PHY_CFG_DATA_MASK     0xf
>   #define PHY_CFG_ADDR_MASK     0x3f
> -#define PHY_CFG_RD_MASK       0x3ff
> +#define PHY_CFG_RD_MASK       0x3f
>   #define PHY_CFG_WR_ENABLE     1
> -#define PHY_CFG_WR_DISABLE    1
> +#define PHY_CFG_WR_DISABLE    0
>   #define PHY_CFG_WR_SHIFT      0
>   #define PHY_CFG_WR_MASK       1
>   #define PHY_CFG_PLL_LOCK      0x10

Re: [RFC PATCH v5 4/4] phy: rockchip-pcie: Adjust read mask and write

[Geraldo Nascimento]

On Fri, Jun 20, 2025 at 03:19:06PM +0100, Robin Murphy wrote:
> On 2025-06-13 6:04 pm, Geraldo Nascimento wrote:
> > Section 17.6.10 of the RK3399 TRM "PCIe PIPE PHY registers Description"
> > defines asynchronous strobe TEST_WRITE which should be enabled then
> > disabled and seems to have been copy-pasted as of current. Adjust it.
> 
> FWIW that's a bit hard to make sense of, given that it bears no relation 
> whatsoever to the naming used in the code :/
> 
> (Not least because the mapping of register fields to phy signals here is 
> really a property of GRF_SOC_CON8 rather than the phy itself)

Hi Robin,

will adjust for a better commit message, thank you.

> 
> > While at it, adjust read mask which should be the same as write mask.
> 
> Which write mask? Certainly not PHY_CFG_WR_MASK... However as this 
> definition is unused since 64cdc0360811 ("phy: rockchip-pcie: remove 
> unused phy_rd_cfg function"), I don't see much point in touching it 
> other than to remove it entirely. If it is the case that only the 
> address field is significant for whatever a "read" operation actually 
> means, well then that's just another job for ADDR_MASK (which I guess is 
> what the open-coded business with PHY_CFG_PLL_LOCK is actually doing...)

Oh, I already had agreed on Bjorn's suggestion to drop PHY_CFG_RD_MASK
for good from code, but I appreciate your input too.

Thanks,
Geraldo Nascimento

> 
> Thanks,
> Robin.
> 
> > Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
> > ---
> >   drivers/phy/rockchip/phy-rockchip-pcie.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/phy/rockchip/phy-rockchip-pcie.c b/drivers/phy/rockchip/phy-rockchip-pcie.c
> > index 48bcc7d2b33b..35d2523ee776 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-pcie.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-pcie.c
> > @@ -30,9 +30,9 @@
> >   #define PHY_CFG_ADDR_SHIFT    1
> >   #define PHY_CFG_DATA_MASK     0xf
> >   #define PHY_CFG_ADDR_MASK     0x3f
> > -#define PHY_CFG_RD_MASK       0x3ff
> > +#define PHY_CFG_RD_MASK       0x3f
> >   #define PHY_CFG_WR_ENABLE     1
> > -#define PHY_CFG_WR_DISABLE    1
> > +#define PHY_CFG_WR_DISABLE    0
> >   #define PHY_CFG_WR_SHIFT      0
> >   #define PHY_CFG_WR_MASK       1
> >   #define PHY_CFG_PLL_LOCK      0x10
> 

Re: [RFC PATCH v5 4/4] phy: rockchip-pcie: Adjust read mask and write

[Geraldo Nascimento]

On Fri, Jun 20, 2025 at 03:19:06PM +0100, Robin Murphy wrote:
> Which write mask? Certainly not PHY_CFG_WR_MASK... However as this 
> definition is unused since 64cdc0360811 ("phy: rockchip-pcie: remove 
> unused phy_rd_cfg function"), I don't see much point in touching it 
> other than to remove it entirely. If it is the case that only the 
> address field is significant for whatever a "read" operation actually 
> means, well then that's just another job for ADDR_MASK (which I guess is 
> what the open-coded business with PHY_CFG_PLL_LOCK is actually doing...)

Just for the sake of posterity, Robin is right here, PHY_CFG_WR_MASK is
just hardcoded to 1, and PHY_CFG_RD_MASK should have been the same
as PHY_CFG_ADDR_MASK as Robin correctly pointed out.

Moot point since I already agreed with Bjorn and Robin to drop the read
define, and Robin was kind enough to track the exact commit where the
corresponding read function was removed. I re-injected that function
from BSP into mainline for my own debugging though, that's why I caught
the typo.

Thanks,
Geraldo Nascimento

> 
> Thanks,
> Robin.