PCI: rockchip: Improve driver quality

Current code uses custom-defined register offsets
and bitfields for standard PCIe registers. Change
to using standard PCIe defines.

drivers/pci/controller/pcie-rockchip-host.c +1 -2

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

drivers/pci/controller/pcie-rockchip-host.c +2 -2

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

drivers/pci/controller/pcie-rockchip-host.c +10 -9

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

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

Quoted 3 lines · Geraldo Nascimento

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;

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.

Quoted 13 lines · Bjorn Helgaas

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

Quoted 3 lines · Geraldo Nascimento

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;

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.

Quoted 5 lines

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

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

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

Quoted 3 lines · Geraldo Nascimento

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;

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.

drivers/pci/controller/pcie-rockchip-host.c static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip) · line 391

Quoted 9 lines

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

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.

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

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

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 <[email protected]>

---
 drivers/phy/rockchip/phy-rockchip-pcie.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/phy/rockchip/phy-rockchip-pcie.c +2 -2

index 48bcc7d2b33b..35d2523ee776 100644
@@ -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

Signature · 1 line

2.49.0

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

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.