[RESEND RFC PATCH v4 0/5] PCI: rockchip: Improve driver quality

[RESEND RFC PATCH v4 0/5] 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.

---
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 (5):
  PCI: rockchip: Use standard PCIe defines
  PCI: rockchip: Drop unused custom registers and bitfields
  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-host.c | 48 +++++++++++----------
 drivers/pci/controller/pcie-rockchip.h      | 11 +----
 drivers/phy/rockchip/phy-rockchip-pcie.c    | 16 ++++---
 3 files changed, 36 insertions(+), 39 deletions(-)

-- 
2.49.0

[RESEND RFC PATCH v4 1/5] 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.

Suggested-By: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Geraldo Nascimento <geraldogabriel@gmail.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 44 ++++++++++-----------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index b9e7a8710cf0..65653218b9ab 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,9 @@ 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);
+		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 +380,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:
-- 
2.49.0

[RESEND RFC PATCH v4 2/5] PCI: rockchip: Drop unused custom registers and bitfields

[Geraldo Nascimento]

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.h | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index 5864a20323f2..f611599988d7 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -155,16 +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_RC_CONFIG_CR		(PCIE_RC_CONFIG_BASE + 0xc0)
 #define PCIE_EP_CONFIG_LCS		(PCIE_EP_CONFIG_BASE + 0xd0)
 #define PCIE_RC_CONFIG_L1_SUBSTATE_CTRL2 (PCIE_RC_CONFIG_BASE + 0x90c)
 #define PCIE_RC_CONFIG_THP_CAP		(PCIE_RC_CONFIG_BASE + 0x274)
-- 
2.49.0

[RESEND RFC PATCH v4 3/5] 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 65653218b9ab..7a0b6ebb7c27 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);
 		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_CR + PCI_EXP_LNKCTL);
-- 
2.49.0

[RESEND RFC PATCH v4 4/5] 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

[RESEND RFC PATCH v4 5/5] 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: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines

[Bjorn Helgaas]

On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote:
> Current code uses custom-defined register offsets
> and bitfields for standard PCIe registers. Change
> to using standard PCIe defines.

Wrap to fill 75 columns so there's space for "git log" to add
indentation.

> @@ -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;

It looks funny to write PCI_EXP_LNKCTL with bits from PCI_EXP_LNKSTA.
I guess this is because rockchip_pcie_write() does 32-bit writes, but
PCI_EXP_LNKCTL and PCI_EXP_LNKSTA are adjacent 16-bit registers.

If the hardware supports it, adding rockchip_pcie_readw() and
rockchip_pcie_writew() for 16-bit accesses would make this read
better.

Hopefully the hardware *does* support this (it's required per spec at
least for config accesses, which would be a different path in the
hardware).  Doing the 32-bit write of PCI_EXP_LNKCTL above is
problematic because writes PCI_EXP_LNKSTA as well, and PCI_EXP_LNKSTA
includes some RW1C bits that may be unintentionally cleared.

> -	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);

This assumes the value you read from PCI_EXP_DEVCAP had zeroes in
these bits.  It might, but it would look safer to do:

  status &= ~(PCI_EXP_DEVCAP_PWR_VAL | PCI_EXP_DEVCAP_PWR_SCL);
  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,9 @@ 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);
> +		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 +380,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);

Similar problem here; PCI_EXP_DEVCTL is only 16 bits, and writing the
adjacent PCI_EXP_DEVSTA may clear RW1C bits you didn't want to clear.

Bjorn

Re: [RESEND RFC PATCH v4 3/5] PCI: rockchip: Set Target Link Speed before retraining

[Bjorn Helgaas]

On Fri, Jun 13, 2025 at 12:06:00PM -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.

Nit: I don't know what "Gen2" means (and the spec warns against
assuming spec rev maps one-to-one to a speed), so try to use the
actual speed instead of "GenX".

Bjorn

Re: [RESEND RFC PATCH v4 5/5] phy: rockchip-pcie: Adjust read mask and write

[Bjorn Helgaas]

On Fri, Jun 13, 2025 at 12:06:28PM -0300, 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.
> While at it, adjust read mask which should be the same as write mask.

Not a PCI patch, but "adjust" doesn't tell us what's happening.

From reading the patch, I assume that since PHY_CFG_WR_ENABLE and
PHY_CFG_WR_DISABLE were both defined to be 1, this code:

        regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
                     HIWORD_UPDATE(PHY_CFG_WR_DISABLE,
                                   PHY_CFG_WR_MASK,
                                   PHY_CFG_WR_SHIFT));

actually left something *enabled* when it meant to disable it.

Maybe the subject/commit log could say something about actually
disabling whatever this is instead of leaving it enabled?

PHY_CFG_RD_MASK appears unused, so maybe it should be just removed.

> 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: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines

[Geraldo Nascimento]

On Fri, Jun 13, 2025 at 03:14:09PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote:
> > -	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;
> 
> It looks funny to write PCI_EXP_LNKCTL with bits from PCI_EXP_LNKSTA.
> I guess this is because rockchip_pcie_write() does 32-bit writes, but
> PCI_EXP_LNKCTL and PCI_EXP_LNKSTA are adjacent 16-bit registers.
> 
> If the hardware supports it, adding rockchip_pcie_readw() and
> rockchip_pcie_writew() for 16-bit accesses would make this read
> better.
> 
> Hopefully the hardware *does* support this (it's required per spec at
> least for config accesses, which would be a different path in the
> hardware).  Doing the 32-bit write of PCI_EXP_LNKCTL above is
> problematic because writes PCI_EXP_LNKSTA as well, and PCI_EXP_LNKSTA
> includes some RW1C bits that may be unintentionally cleared.

Hi Bjorn and thank you for the review,

while your rationale is correct per PCIe spec, per RK3399 TRM
those registers are indeed 32 bits in the Rockchip-IP PCIe, so
I'm forced to work with that, but without fear that other
registers get messed-up. (See for example Section 17.6.6.1.30
of RK3399 TRM, Part 2)

Regards,
Geraldo Nascimento

Re: [RESEND RFC PATCH v4 3/5] PCI: rockchip: Set Target Link Speed before retraining

[Geraldo Nascimento]

On Fri, Jun 13, 2025 at 03:15:43PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 13, 2025 at 12:06:00PM -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.
> 
> Nit: I don't know what "Gen2" means (and the spec warns against
> assuming spec rev maps one-to-one to a speed), so try to use the
> actual speed instead of "GenX".

Ah, excellent catch. I'll make sure to adjust the commit message!

Geraldo Nascimento

> 
> Bjorn

Re: [RESEND RFC PATCH v4 5/5] phy: rockchip-pcie: Adjust read mask and write

[Geraldo Nascimento]

On Fri, Jun 13, 2025 at 03:20:56PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 13, 2025 at 12:06:28PM -0300, 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.
> > While at it, adjust read mask which should be the same as write mask.
> 
> Not a PCI patch, but "adjust" doesn't tell us what's happening.
> 
> From reading the patch, I assume that since PHY_CFG_WR_ENABLE and
> PHY_CFG_WR_DISABLE were both defined to be 1, this code:
> 
>         regmap_write(rk_phy->reg_base, rk_phy->phy_data->pcie_conf,
>                      HIWORD_UPDATE(PHY_CFG_WR_DISABLE,
>                                    PHY_CFG_WR_MASK,
>                                    PHY_CFG_WR_SHIFT));
> 
> actually left something *enabled* when it meant to disable it.
> 
> Maybe the subject/commit log could say something about actually
> disabling whatever this is instead of leaving it enabled?
> 
> PHY_CFG_RD_MASK appears unused, so maybe it should be just removed.

Your line of reasoning is correct regarding the TEST_WRITE async strobe
register, and there's a picture of the flow in Section 17.5.3
(PCIe PHY Configuration) of the RK3399 TRM, Part 2.

I'll make sure to be more clear in the commit message.

Regarding PHY_CFG_RD_MASK, yes, it is unused AFAICT and can be removed.
It's leftover from BSP where the debugging function phy_rd_cfg exists.

Thanks,
Geraldo Nascimento

Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines

[Bjorn Helgaas]

On Fri, Jun 13, 2025 at 05:26:46PM -0300, Geraldo Nascimento wrote:
> On Fri, Jun 13, 2025 at 03:14:09PM -0500, Bjorn Helgaas wrote:
> > On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote:
> > > -	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;
> > 
> > It looks funny to write PCI_EXP_LNKCTL with bits from PCI_EXP_LNKSTA.
> > I guess this is because rockchip_pcie_write() does 32-bit writes, but
> > PCI_EXP_LNKCTL and PCI_EXP_LNKSTA are adjacent 16-bit registers.
> > 
> > If the hardware supports it, adding rockchip_pcie_readw() and
> > rockchip_pcie_writew() for 16-bit accesses would make this read
> > better.
> > 
> > Hopefully the hardware *does* support this (it's required per spec at
> > least for config accesses, which would be a different path in the
> > hardware).  Doing the 32-bit write of PCI_EXP_LNKCTL above is
> > problematic because writes PCI_EXP_LNKSTA as well, and PCI_EXP_LNKSTA
> > includes some RW1C bits that may be unintentionally cleared.
> 
> Hi Bjorn and thank you for the review,
> 
> while your rationale is correct per PCIe spec, per RK3399 TRM
> those registers are indeed 32 bits in the Rockchip-IP PCIe, so
> I'm forced to work with that, but without fear that other
> registers get messed-up. (See for example Section 17.6.6.1.30
> of RK3399 TRM, Part 2)

I don't have access to any of these TRMs, so I only know what's in the
driver.

When you say "without fear", are you saying there's a way to do that
32-bit write such that the LNKSTA bits are discarded by the hardware?
Or just that the hardware forces us to accept this potential status
register corruption?

Is this something that could be written using the config access path?
I guess probably not, based on this:

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-rockchip-host.c?id=v6.15#n141

Bjorn

Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines

[Geraldo Nascimento]

On Fri, Jun 13, 2025 at 03:50:23PM -0500, Bjorn Helgaas wrote:
> I don't have access to any of these TRMs, so I only know what's in the
> driver.
> 

They are not under NDA and can be obtained though Rockchip's
official site:
https://rockchip.fr/Rockchip%20RK3399%20TRM%20V1.3%20Part2.pdf

> When you say "without fear", are you saying there's a way to do that
> 32-bit write such that the LNKSTA bits are discarded by the hardware?
> Or just that the hardware forces us to accept this potential status
> register corruption?

I meant to say those registers themselves are defined in TRM as 32 bits.

> 
> Is this something that could be written using the config access path?
> I guess probably not, based on this:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pcie-rockchip-host.c?id=v6.15#n141
> 

That certainly looks frightening.

> Bjorn

Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines

[Geraldo Nascimento]

On Fri, Jun 13, 2025 at 03:14:09PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 13, 2025 at 12:05:31PM -0300, Geraldo Nascimento wrote:
> > -	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;
> 
> It looks funny to write PCI_EXP_LNKCTL with bits from PCI_EXP_LNKSTA.
> I guess this is because rockchip_pcie_write() does 32-bit writes, but
> PCI_EXP_LNKCTL and PCI_EXP_LNKSTA are adjacent 16-bit registers.
> 
> If the hardware supports it, adding rockchip_pcie_readw() and
> rockchip_pcie_writew() for 16-bit accesses would make this read
> better.
> 
> Hopefully the hardware *does* support this (it's required per spec at
> least for config accesses, which would be a different path in the
> hardware).  Doing the 32-bit write of PCI_EXP_LNKCTL above is
> problematic because writes PCI_EXP_LNKSTA as well, and PCI_EXP_LNKSTA
> includes some RW1C bits that may be unintentionally cleared.

Hi Bjorn,

unfortunately Rockchip PCIe IP does not support 16-bit accesses,
I tried and it only rendered the kernel unbootable, which made
people in my house angry since the RK3399 box is my Internet Gateway!

:-)

For thit particular case, it is OK since LABS and LBMS are precisely
the only RW1C bits in LNKSTA as far as I know. But see below.
> >  
> > -	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);
> 
> Similar problem here; PCI_EXP_DEVCTL is only 16 bits, and writing the
> adjacent PCI_EXP_DEVSTA may clear RW1C bits you didn't want to clear.
> 

This is a bit more concerning then above. I'm out of ideas regarding
this particular issue you raised.

Geraldo Nascimento
> Bjorn

Re: [RESEND RFC PATCH v4 1/5] PCI: rockchip: Use standard PCIe defines

[Geraldo Nascimento]

On Fri, Jun 13, 2025 at 06:01:40PM -0300, Geraldo Nascimento wrote:
> They are not under NDA and can be obtained though Rockchip's
> official site:
> https://rockchip.fr/Rockchip%20RK3399%20TRM%20V1.3%20Part2.pdf

Hm, sorry about the confusion Bjorn, that is not an official website
at all it seems, so I assume these are leaked?

If so, it's a real pity, there's not particularly confidential about
these docs, and they are essential for working with Rockchip chips.

Sorry,
Geraldo Nascimento