PCI: rockchip: Improve driver quality

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

---
 drivers/pci/controller/pcie-rockchip-host.c | 4 ++++
 1 file changed, 4 insertions(+)

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

index 8489d51e01ca..467e3fc377f7 100644

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

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

Signature · 1 line

2.49.0

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 <[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 8489d51e01ca..467e3fc377f7 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);
  		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;

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.

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

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

---
  drivers/phy/rockchip/phy-rockchip-pcie.c | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

drivers/phy/rockchip/phy-rockchip-pcie.c +7 -5

index bd44af36c67a..48bcc7d2b33b 100644
@@ -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,

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

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.

Quoted 13 lines · Robin Murphy

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

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:

Quoted 5 lines

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

drivers/phy/rockchip/phy-rockchip-pcie.c +4 -5

index bd44af36c67a..a34a983db16c 100644
@@ -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,

Quoted 13 lines · Robin Murphy

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

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:

Quoted 5 lines

[    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

Quoted 3 lines

Thanks,
Robin.

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

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

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)

Quoted 2 lines

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.

Quoted 21 lines

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

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.