Skip to contentSkip to conversation

PCI: rockchip: Improve driver quality

PATCH RESEND Applied Geraldo Nascimento 5 participants 12 messages Jun 30, 2025 – Jul 22, 2025

Subsystems (5)
PCI Arm Rockchip Generic PHY framework PCI native host bridge and endpoint drivers +1 more subsystems
lore
Share & tools
lore

Geraldo Nascimento

author
#0 lore raw

v9 cover PCI: rockchip: Improve driver quality

During a 30-day debugging-run fighting quirky PCIe devices on RK3399 some quality improvements began to take form and after feedback from community they reached more polished state.

This will ensure maximum chance of retraining to 5.0GT/s, on all four lanes and fix async strobe TEST_WRITE disablement. On top of this, standard PCIe defines are now used to reference registers from offset at Capabilities Register.

Unfortunately, it seems Rockchip-IP PCIe is unable to handle 16-bit register writes and there's risk of corrupting state of RW1C registers, an issue raised by Bjorn Helgaas. There's little I could do to fix that, so on this issue the situation remains the same.

---
V8 -> V9: modify third patch to better reflect authorship by Valmantas V7 -> V8: add Valmantas Paliksa Signed-off-by to third patch V6 -> V7: drop RFC tag as per Heiko Stuebner's reminder, update cover letter
V5 -> V6: reflow to 75 cols, use 5.0GTs instead of Gen2 nomenclature, clarify strobe write adjustment and remove PHY_CFG_RD_MASK 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 (3):
PCI: rockchip: Use standard PCIe defines
PCI: rockchip: Set Target Link Speed before retraining
phy: rockchip-pcie: Properly disable TEST_WRITE strobe signal

Valmantas Paliksa (1):
phy: rockchip-pcie: Enable all four lanes if required

 drivers/pci/controller/pcie-rockchip-ep.c   |  4 +-
 drivers/pci/controller/pcie-rockchip-host.c | 48 +++++++++++----------
 drivers/pci/controller/pcie-rockchip.h      | 12 +-----
 drivers/phy/rockchip/phy-rockchip-pcie.c    | 15 +++----
 4 files changed, 36 insertions(+), 43 deletions(-)
Signature · 1 line
2.49.0
author
#1 lore raw

PATCH RESEND v9 1/4 PCI: rockchip: Use standard PCIe defines

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 <[email protected]>
Signed-off-by Geraldo Nascimento <[email protected]>
---
 drivers/pci/controller/pcie-rockchip-ep.c   |  4 +-
 drivers/pci/controller/pcie-rockchip-host.c | 44 ++++++++++-----------
 drivers/pci/controller/pcie-rockchip.h      | 12 +-----
 3 files changed, 25 insertions(+), 35 deletions(-)
drivers/pci/controller/pcie-rockchip-ep.c +2 -2
index 55416b8311dd..300cd85fa035 100644
@@ -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)
drivers/pci/controller/pcie-rockchip-host.c +22 -22

95 lines not on this page · raw message

drivers/pci/controller/pcie-rockchip.h +1 -11
index 5864a20323f2..f5cbf3c9d2d9 100644
@@ -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)
Signature · 1 line
2.49.0
#2 lore raw

Re: [RESEND PATCH v9 1/4] PCI: rockchip: Use standard PCIe defines

Quoted 5 lines · 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.

This could be phrased a bit more cleanly. At least I don't get exactly what "from offset" means. You mean you replace the unused custom ones? But if they're unused, why are they even being replaced?

Quoted 2 lines
Suggested-By Bjorn Helgaas <[email protected]>

s/By/by

P.

Quoted 192 lines
Signed-off-by Geraldo Nascimento <[email protected]>

---
 drivers/pci/controller/pcie-rockchip-ep.c   |  4 +- drivers/pci/controller/pcie-rockchip-host.c | 44 ++++++++++--------- --
 drivers/pci/controller/pcie-rockchip.h      | 12 +-----

 3 files changed, 25 insertions(+), 35 deletions(-)
drivers/pci/controller/pcie-rockchip-ep.c +0 -0
index 55416b8311dd..300cd85fa035 100644
@@ -518,9 +518,9 @@ static void rockchip_pcie_ep_retrain_link(struct
 rockchip_pcie *rockchip)
  {
  	u32 status;

drivers/pci/controller/pcie-rockchip-ep.c +1 -1
-	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;

drivers/pci/controller/pcie-rockchip-ep.c +1 -1
-	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)

drivers/pci/controller/pcie-rockchip-host.c +0 -0
index b9e7a8710cf0..65653218b9ab 100644
@@ -40,18 +40,18 @@ static void rockchip_pcie_enable_bw_int(struct
 rockchip_pcie *rockchip)
  {
  	u32 status;

drivers/pci/controller/pcie-rockchip-host.c +1 -1
-	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);

drivers/pci/controller/pcie-rockchip-host.c +1 -1
-	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;

drivers/pci/controller/pcie-rockchip-host.c +1 -1
-	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;

drivers/pci/controller/pcie-rockchip-host.c +1 -1
-	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)

drivers/pci/controller/pcie-rockchip-host.c +0 -0
@@ -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 */

drivers/pci/controller/pcie-rockchip-host.c +1 -1
-	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;

drivers/pci/controller/pcie-rockchip-host.c +0 -0
@@ -278,10 +278,10 @@ static void
 rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
  		power = power / 10;
  	}

drivers/pci/controller/pcie-rockchip-host.c +1 -4
-	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);

drivers/pci/controller/pcie-rockchip-host.c +3 -0
+	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);
 }

 /**

drivers/pci/controller/pcie-rockchip-host.c +0 -0
@@ -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 */

drivers/pci/controller/pcie-rockchip-host.c +1 -1
-	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;

drivers/pci/controller/pcie-rockchip-host.c +1 -1
-	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 */

drivers/pci/controller/pcie-rockchip-host.c +1 -1
-	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;

drivers/pci/controller/pcie-rockchip-host.c +1 -1
-	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,

drivers/pci/controller/pcie-rockchip-host.c +0 -0
@@ -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.
  */

drivers/pci/controller/pcie-rockchip-host.c +0 -1
-		status = rockchip_pcie_read(rockchip,

PCIE_RC_CONFIG_LCS);

drivers/pci/controller/pcie-rockchip-host.c +1 -0
+		status = rockchip_pcie_read(rockchip,

PCIE_RC_CONFIG_CR + PCI_EXP_LNKCTL);
  status |= PCI_EXP_LNKCTL_RL;

drivers/pci/controller/pcie-rockchip-host.c +0 -1
-		rockchip_pcie_write(rockchip, status,

PCIE_RC_CONFIG_LCS);

drivers/pci/controller/pcie-rockchip-host.c +1 -0
+		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,

drivers/pci/controller/pcie-rockchip-host.c +0 -0
@@ -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")) {

drivers/pci/controller/pcie-rockchip-host.c +0 -1
-		status = rockchip_pcie_read(rockchip,

PCIE_RC_CONFIG_LINK_CAP);

drivers/pci/controller/pcie-rockchip-host.c +0 -2
-		status &= ~PCIE_RC_CONFIG_LINK_CAP_L0S;
-		rockchip_pcie_write(rockchip, status,

PCIE_RC_CONFIG_LINK_CAP);

drivers/pci/controller/pcie-rockchip-host.c +1 -0
+		status = rockchip_pcie_read(rockchip,

PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);

drivers/pci/controller/pcie-rockchip-host.c +2 -0
+		status &= ~PCI_EXP_LNKCAP_ASPM_L0S;
+		rockchip_pcie_write(rockchip, status,

PCIE_RC_CONFIG_CR + PCI_EXP_LNKCAP);
  }

drivers/pci/controller/pcie-rockchip-host.c +1 -4
-	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);

drivers/pci/controller/pcie-rockchip-host.c +3 -0
+	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:

drivers/pci/controller/pcie-rockchip.h +2 -11
index 5864a20323f2..f5cbf3c9d2d9 100644
@@ -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)

author
#3 lore raw

Re: [RESEND PATCH v9 1/4] PCI: rockchip: Use standard PCIe defines

Quoted 10 lines · Philipp Stanner
Quoted 5 lines · 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.

This could be phrased a bit more cleanly. At least I don't get exactly what "from offset" means. You mean you replace the unused custom ones? But if they're unused, why are they even being replaced?

Hi Philipp!

"from offset" means we use standard PCIe defines for registers that are adjacent to Capabilities Register, and we reference them from the offset at Capabilities Register.

No, all registers replaced are in use, unused in that context means they (the custom-defined registers which can be referenced starting from Capabilities Register address) become unused after the change, only.

Quoted 6 lines
Quoted 4 lines
Suggested-By Bjorn Helgaas <[email protected]>

s/By/by

Thanks for the capitalization catch. Unfortunately there's little I can do now that Mani went ahead and applied the first two patches (directly related to PCI subsystem).

Thanks,
Geraldo Nascimento

Quoted 3 lines

P.

maintainer
#4 lore raw

Re: [RESEND PATCH v9 1/4] PCI: rockchip: Use standard PCIe defines

Quoted 14 lines · Geraldo Nascimento
Quoted 10 lines · Philipp Stanner
Quoted 5 lines · 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.

This could be phrased a bit more cleanly. At least I don't get exactly what "from offset" means. You mean you replace the unused custom ones? But if they're unused, why are they even being replaced?

Hi Philipp!

Quoted 8 lines

"from offset" means we use standard PCIe defines for registers that are adjacent to Capabilities Register, and we reference them from the offset at Capabilities Register.

No, all registers replaced are in use, unused in that context means they (the custom-defined registers which can be referenced starting from Capabilities Register address) become unused after the change, only.

I've reworded the commit message:
https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?h=controller/rockchip&id=04e740dfe153f2b6530ce99c0e346600d3fb2ef7

Quoted 11 lines
Quoted 6 lines
Quoted 4 lines
Suggested-By Bjorn Helgaas <[email protected]>

s/By/by

Thanks for the capitalization catch. Unfortunately there's little I can do now that Mani went ahead and applied the first two patches (directly related to PCI subsystem).

This was already taken care!

- Mani

Signature · 1 line
மணிவண்ணன் சதாசிவம்
#5 lore raw

Re: [RESEND PATCH v9 1/4] PCI: rockchip: Use standard PCIe defines

In reply to Geraldo Nascimento
Quoted 6 lines · 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.

drivers/pci/controller/pcie-rockchip-host.c static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip) · line 283

Quoted 10 lines
drivers/pci/controller/pcie-rockchip-host.c +3 -4
@@ -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);

Added #include <linux/bitfield.h> for this:

CC drivers/pci/controller/pcie-rockchip-host.o
drivers/pci/controller/pcie-rockchip-host.c: In function ‘rockchip_pcie_set_power_limit’:
drivers/pci/controller/pcie-rockchip-host.c:272:24: error: implicit declaration of function ‘FIELD_MAX’ [-Werror=implicit-function-declaration]

  272 |         while (power > FIELD_MAX(PCI_EXP_DEVCAP_PWR_VAL)) {
      |                        ^~~~~~~~~
drivers/pci/controller/pcie-rockchip-host.c:282:19: error: implicit declaration of function ‘FIELD_PREP’ [-Werror=implicit-function-declaration]
  282 |         status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_VAL, power);
      |                   ^~~~~~~~~~
author
#6 lore raw

Re: [RESEND PATCH v9 1/4] PCI: rockchip: Use standard PCIe defines

Quoted 20 lines · Bjorn Helgaas
Quoted 6 lines · 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.

drivers/pci/controller/pcie-rockchip-host.c static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip) · line 283

Quoted 10 lines
drivers/pci/controller/pcie-rockchip-host.c +3 -4
@@ -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);

Added #include <linux/bitfield.h> for this:

Quoted 9 lines

CC drivers/pci/controller/pcie-rockchip-host.o
drivers/pci/controller/pcie-rockchip-host.c: In function ‘rockchip_pcie_set_power_limit’:
drivers/pci/controller/pcie-rockchip-host.c:272:24: error: implicit declaration of function ‘FIELD_MAX’ [-Werror=implicit-function-declaration]

  272 |         while (power > FIELD_MAX(PCI_EXP_DEVCAP_PWR_VAL)) {
      |                        ^~~~~~~~~
drivers/pci/controller/pcie-rockchip-host.c:282:19: error: implicit declaration of function ‘FIELD_PREP’ [-Werror=implicit-function-declaration]
  282 |         status |= FIELD_PREP(PCI_EXP_DEVCAP_PWR_VAL, power);
      |                   ^~~~~~~~~~

Hi Bjorn,

Ugh, what a miss! Thank you for taking care of this!

Geraldo Nascimento

author
#7 lore raw

PATCH RESEND v9 2/4 PCI: rockchip: Set Target Link Speed before retraining

In reply to Geraldo Nascimento

Current code may fail 5.0GT/s 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.

Tested-by Robin Murphy <[email protected]>
Signed-off-by Geraldo Nascimento <[email protected]>
---
 drivers/pci/controller/pcie-rockchip-host.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/pci/controller/pcie-rockchip-host.c +4 -0
index 65653218b9ab..25890f6c0e17 100644
@@ -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);
Signature · 1 line
2.49.0
author
#8 lore raw

PATCH RESEND v9 3/4 phy: rockchip-pcie: Enable all four lanes if required

In reply to Geraldo Nascimento

From: Valmantas Paliksa <[email protected]>

Current code enables only Lane 0 because pwr_cnt will be incremented on first call to the function. Let's reorder the enablement code to enable all 4 lanes through GRF.

Reviewed-by Neil Armstrong <[email protected]>
Reviewed-by Robin Murphy <[email protected]>
Signed-off-by Valmantas Paliksa <[email protected]>
Signed-off-by Geraldo Nascimento <[email protected]>
---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/phy/rockchip/phy-rockchip-pcie.c +6 -6
index bd44af36c67a..f22ffb41cdc2 100644
@@ -160,6 +160,12 @@ static int rockchip_pcie_phy_power_on(struct phy *phy)
 
 	guard(mutex)(&rk_phy->pcie_mutex);
 
+	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));
+
 	if (rk_phy->pwr_cnt++) {
 		return 0;
 	}

@@ -176,12 +182,6 @@ 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));
-
 	/*
 	 * No documented timeout value for phy operation below,
 	 * so we make it large enough here. And we use loop-break
Signature · 1 line
2.49.0
author
#9 lore raw

PATCH RESEND v9 4/4 phy: rockchip-pcie: Properly disable TEST_WRITE strobe signal

In reply to Geraldo Nascimento

pcie_conf is used to touch TEST_WRITE strobe signal. This signal should be enabled, a little time waited, and then disabled. Current code clearly was copy-pasted and never disables the strobe signal. Adjust the define. While at it, remove PHY_CFG_RD_MASK which has been unused since 64cdc0360811 ("phy: rockchip-pcie: remove unused phy_rd_cfg function").

Reviewed-by Neil Armstrong <[email protected]>
Signed-off-by Geraldo Nascimento <[email protected]>
---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/phy/rockchip/phy-rockchip-pcie.c +1 -2
index f22ffb41cdc2..4e2dfd01adf2 100644
@@ -30,9 +30,8 @@
 #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_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
Signature · 1 line
2.49.0
maintainer Applied
#10 lore raw

Re: (subset) [RESEND PATCH v9 0/4] PCI: rockchip: Improve driver quality

In reply to Geraldo Nascimento
Quoted 10 lines · Geraldo Nascimento

During a 30-day debugging-run fighting quirky PCIe devices on RK3399 some quality improvements began to take form and after feedback from community they reached more polished state.

This will ensure maximum chance of retraining to 5.0GT/s, on all four lanes and fix async strobe TEST_WRITE disablement. On top of this, standard PCIe defines are now used to reference registers from offset at Capabilities Register.

[...]

Applied, thanks!

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

      commit: a54fa9e656b38d64761478d06aa8679eae074ca1
[2/4] PCI: rockchip: Set Target Link Speed before retraining
      commit: 7a886fbf4004a990cb7231d64370c622d0eb741f

Best regards,

Signature · 1 line
Manivannan Sadhasivam <[email protected]>
maintainer Applied
#11 lore raw

Re: (subset) [RESEND PATCH v9 0/4] PCI: rockchip: Improve driver quality

In reply to Geraldo Nascimento
Quoted 10 lines · Geraldo Nascimento

During a 30-day debugging-run fighting quirky PCIe devices on RK3399 some quality improvements began to take form and after feedback from community they reached more polished state.

This will ensure maximum chance of retraining to 5.0GT/s, on all four lanes and fix async strobe TEST_WRITE disablement. On top of this, standard PCIe defines are now used to reference registers from offset at Capabilities Register.

[...]

Applied, thanks!

[3/4] phy: rockchip-pcie: Enable all four lanes if required

      commit: c3fe7071e196e25789ecf90dbc9e8491a98884d7
[4/4] phy: rockchip-pcie: Properly disable TEST_WRITE strobe signal
      commit: 25facbabc3fc33c794ad09d73f73268c0f8cbc7d

Best regards,

Signature · 1 line
~Vinod