PCI: rockchip: 5.0 GT/s speed discouraged by Rockchip

Configure the core to be driven at 2.5 GT/s Link Speed and ignore any other speed with a warning. The reason is that Shawn Lin from Rockchip has reiterated that there may be danger of "catastrophic failure" in using their PCIe with 5.0 GT/s speeds.

While Rockchip has done so informally without issuing a proper errata, and the particulars are thus unknown, this may cause data loss or worse.

This change is corroborated by RK3399 official datasheet [1], which states maximum link speed for this platform is 2.5 GT/s.

[1] https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf

Fixes 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")

Link https://lore.kernel.org/all/[email protected]/ open here

Cc [email protected]

Reported-by Dragan Simic <[email protected]>

Reported-by Shawn Lin <[email protected]>

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

---
 drivers/pci/controller/pcie-rockchip.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

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

index 0f88da378805..2f211d1f4c7c 100644

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

 	for (i = 0; i < ROCKCHIP_NUM_PM_RSTS; i++)
 		rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];

@@ -147,12 +149,13 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		goto err_exit_phy;
 	}
 
-	if (rockchip->link_gen == 2)

Hello Geraldo,

Quoted 29 lines

Configure the core to be driven at 2.5 GT/s Link Speed and ignore any other speed with a warning. The reason is that Shawn Lin from Rockchip has reiterated that there may be danger of "catastrophic failure" in using their PCIe with 5.0 GT/s speeds.

While Rockchip has done so informally without issuing a proper errata, and the particulars are thus unknown, this may cause data loss or worse.

This change is corroborated by RK3399 official datasheet [1], which states maximum link speed for this platform is 2.5 GT/s.

[1] https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf

Fixes 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")

Link https://lore.kernel.org/all/[email protected]/ open here

Cc [email protected]

Reported-by Dragan Simic <[email protected]>

Reported-by Shawn Lin <[email protected]>

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

---
 drivers/pci/controller/pcie-rockchip.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

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

index 0f88da378805..2f211d1f4c7c 100644

drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) · line 72

Quoted 9 lines

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

@@ -66,8 +66,10 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 	}
 
 	rockchip->link_gen = of_pci_get_max_link_speed(node);
-	if (rockchip->link_gen < 0 || rockchip->link_gen > 2)
-		rockchip->link_gen = 2;
+	if (rockchip->link_gen < 0 || rockchip->link_gen >= 2) {
+		rockchip->link_gen = 1;
+		dev_warn(dev, "invalid max-link-speed, fix your DT\n");
+	}

I'd suggest using a bit more formal message here, like the one below, which would also avoid addressing the user directly:

"Invalid max-link-speed found, limited to Gen1 to avoid data corruption"

Quoted 7 lines

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

 	for (i = 0; i < ROCKCHIP_NUM_PM_RSTS; i++)
 		rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];

@@ -147,12 +149,13 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		goto err_exit_phy;
 	}
 

drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) · line 158

Quoted 11 lines

drivers/pci/controller/pcie-rockchip.c +5 -4

-	if (rockchip->link_gen == 2)
-		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2,
-				    PCIE_CLIENT_CONFIG);
-	else
+	if (rockchip->link_gen == 2) {
+		/* 5.0 GT/s may cause catastrophic failure for this core */
+		dev_warn(dev, "5.0 GT/s may cause data loss or worse\n");
+	} else {
 		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
 				    PCIE_CLIENT_CONFIG);
+	}

I don't think we need to emit a warning here, because, as we've already established through earlier discussions, the rockchip_pcie_init_port() function should never receive an invalid speed value.

drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) · line 161

Quoted 3 lines

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

 	regs = PCIE_CLIENT_ARI_ENABLE |
 	       PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes);

It would make most sense to squash all three patches in this series into a single patch, because they all form a single logical unit, which introduces changes that are just going to be harder to track down later if it's all scattered into multiple separate patches.

The only possible issue with the squashing comes from the inability to have different patch subject prefixes for different driver changes, but I think that's actually not an issue. The long-term benefits of having everything as a single patch outweighs the benefits of having different patch subjects with separate patches.

Quoted 43 lines · Dragan Simic

Quoted 29 lines · Geraldo Nascimento

Configure the core to be driven at 2.5 GT/s Link Speed and ignore any other speed with a warning. The reason is that Shawn Lin from Rockchip has reiterated that there may be danger of "catastrophic failure" in using their PCIe with 5.0 GT/s speeds.

While Rockchip has done so informally without issuing a proper errata, and the particulars are thus unknown, this may cause data loss or worse.

This change is corroborated by RK3399 official datasheet [1], which states maximum link speed for this platform is 2.5 GT/s.

[1] https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf

Fixes 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")

Link https://lore.kernel.org/all/[email protected]/ open here

Cc [email protected]

Reported-by Dragan Simic <[email protected]>

Reported-by Shawn Lin <[email protected]>

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

---
 drivers/pci/controller/pcie-rockchip.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

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

index 0f88da378805..2f211d1f4c7c 100644

drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) · line 72

Quoted 9 lines

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

@@ -66,8 +66,10 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 	}
 
 	rockchip->link_gen = of_pci_get_max_link_speed(node);
-	if (rockchip->link_gen < 0 || rockchip->link_gen > 2)
-		rockchip->link_gen = 2;
+	if (rockchip->link_gen < 0 || rockchip->link_gen >= 2) {
+		rockchip->link_gen = 1;
+		dev_warn(dev, "invalid max-link-speed, fix your DT\n");
+	}

I'd suggest using a bit more formal message here, like the one below, which would also avoid addressing the user directly:

Quoted 1 line

"Invalid max-link-speed found, limited to Gen1 to avoid data corruption"

We really should spare on characters here, but I see your point and will try to cook up a better way.

Quoted 6 lines

Quoted 6 lines

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

 	for (i = 0; i < ROCKCHIP_NUM_PM_RSTS; i++)
 		rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];

@@ -147,12 +149,13 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		goto err_exit_phy;
 	}
 

Quoted 15 lines

drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) · line 158

Quoted 11 lines

drivers/pci/controller/pcie-rockchip.c +5 -4

-	if (rockchip->link_gen == 2)
-		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2,
-				    PCIE_CLIENT_CONFIG);
-	else
+	if (rockchip->link_gen == 2) {
+		/* 5.0 GT/s may cause catastrophic failure for this core */
+		dev_warn(dev, "5.0 GT/s may cause data loss or worse\n");
+	} else {
 		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
 				    PCIE_CLIENT_CONFIG);
+	}

I don't think we need to emit a warning here, because, as we've already established through earlier discussions, the rockchip_pcie_init_port() function should never receive an invalid speed value.

Just as a lame excuse, those messages were everywhere in the mid of my development, this is one that escaped deletion, will drop.

Quoted 7 lines

drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) · line 161

Quoted 2 lines

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

 	regs = PCIE_CLIENT_ARI_ENABLE |
 	       PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes);

It would make most sense to squash all three patches in this series into a single patch, because they all form a single logical unit, which introduces changes that are just going to be harder to track down later if it's all scattered into multiple separate patches.

I agree, having all drops in one big patch is the better tactic here.

Quoted 5 lines

The only possible issue with the squashing comes from the inability to have different patch subject prefixes for different driver changes, but I think that's actually not an issue. The long-term benefits of having everything as a single patch outweighs the benefits of having different patch subjects with separate patches.

Sure, will do so for v6.

Hello Geraldo,

Quoted 29 lines · Geraldo Nascimento

Configure the core to be driven at 2.5 GT/s Link Speed and ignore any other speed with a warning. The reason is that Shawn Lin from Rockchip has reiterated that there may be danger of "catastrophic failure" in using their PCIe with 5.0 GT/s speeds.

While Rockchip has done so informally without issuing a proper errata, and the particulars are thus unknown, this may cause data loss or worse.

This change is corroborated by RK3399 official datasheet [1], which states maximum link speed for this platform is 2.5 GT/s.

[1] https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf

Fixes 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")

Link https://lore.kernel.org/all/[email protected]/ open here

Cc [email protected]

Reported-by Dragan Simic <[email protected]>

Reported-by Shawn Lin <[email protected]>

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

---
 drivers/pci/controller/pcie-rockchip.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

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

index 0f88da378805..2f211d1f4c7c 100644

drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) · line 72

Quoted 9 lines

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

@@ -66,8 +66,10 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip)
 	}
 
 	rockchip->link_gen = of_pci_get_max_link_speed(node);
-	if (rockchip->link_gen < 0 || rockchip->link_gen > 2)
-		rockchip->link_gen = 2;
+	if (rockchip->link_gen < 0 || rockchip->link_gen >= 2) {
+		rockchip->link_gen = 1;
+		dev_warn(dev, "invalid max-link-speed, fix your DT\n");
+	}

I'd suggest using a bit more formal message here, like the one below, which would also avoid addressing the user directly:

"Invalid max-link-speed found, limited to Gen1 to avoid data corruption"

Quoted 7 lines

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

 	for (i = 0; i < ROCKCHIP_NUM_PM_RSTS; i++)
 		rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];

@@ -147,12 +149,13 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip)
 		goto err_exit_phy;
 	}
 
-	if (rockchip->link_gen == 2)

drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) · line 158

Quoted 10 lines

drivers/pci/controller/pcie-rockchip.c +5 -3

-		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2,
-				    PCIE_CLIENT_CONFIG);
-	else
+	if (rockchip->link_gen == 2) {
+		/* 5.0 GT/s may cause catastrophic failure for this core */
+		dev_warn(dev, "5.0 GT/s may cause data loss or worse\n");
+	} else {
 		rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_1,
 				    PCIE_CLIENT_CONFIG);
+	}

I don't think we need to emit a warning here, because, as we've already established through earlier discussions, the rockchip_pcie_init_port() function should never receive an invalid speed value.

drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) · line 161

Quoted 2 lines

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

 	regs = PCIE_CLIENT_ARI_ENABLE |
 	       PCIE_CLIENT_CONF_LANE_NUM(rockchip->lanes);

It would make most sense to squash all three patches in this series into a single patch, because they all form a single logical unit, which introduces changes that are just going to be harder to track down later if it's all scattered into multiple separate patches.

The only possible issue with the squashing comes from the inability to have different patch subject prefixes for different driver changes, but I think that's actually not an issue. The long-term benefits of having everything as a single patch outweighs the benefits of having different patch subjects with separate patches.

Patches 3&4 can be squashed together, but the rest should be in separate patches as they target different drivers. If we were to revert any of these in the future, it becomes easy.

- Mani

Signature · 1 line

மணிவண்ணன் சதாசிவம்