PCI: rockchip: 5.0 GT/s speed discouraged by Rockchip
v4 cover PCI: rockchip: 5.0 GT/s speed discouraged by Rockchip
Dragan Simic already had warned me of potential issues with 5.0 GT/s speed operation in Rockchip PCIe. However, in recent interactions with Shawn Lin from Rockchip it came to my attention there's grave danger in the unknown errata regarding 5.0 GT/s operational speed of their PCIe core.
Drop all code related to 5.0 GT/s operational speed from this driver.
Endpoint Mode driver was not tested. There's a bit of redundancy in the commit messages but it's the best I could do to ensure people will actually understand why perfectly working code is being dropped.
---
Changes in v4:
- Incorporate suggestion by Bjorn and refined by Dragan to drop the
"catastrophic" code
- Link to v3: https://lore.kernel.org/linux-rockchip/[email protected]/T/
Changes in v3:
- Clarify warning message even though Rockchip won't disclose details
- Drop DT changes as they were applied as subset by Heiko
- Link to v2: https://lore.kernel.org/all/[email protected]/T/
Changes in v2:
- hard limit to 2.5 GT/s, not just warn
- add Reported-by: and Reviewed-by: Dragan Simic
- remove redundant declaration of max-link-speed from helios64 dts
- fix Link: of helios64 patch
- simplify RC mode comment
- Link to v1: https://lore.kernel.org/all/aRhR79u5BPtRRFw3@geday/T/
---
Geraldo Nascimento (4):
PCI: rockchip: drop 2.5 GT/s defines
PCI: rockchip: drive at 2.5 GT/s only and error out other speeds
PCI: rockchip-host: do not attempt 5.0 GT/s retraining
PCI: rockchip-ep: do not attempt 5.0 GT/s retraining
drivers/pci/controller/pcie-rockchip-ep.c | 13 ------------- drivers/pci/controller/pcie-rockchip-host.c | 20 -------------------- drivers/pci/controller/pcie-rockchip.c | 13 +++++-------- drivers/pci/controller/pcie-rockchip.h | 3 --- 4 files changed, 5 insertions(+), 44 deletions(-)
Signature · 1 line
2.52.0
PATCH v4 1/4 PCI: rockchip: drop 2.5 GT/s defines
Drop the 2.5 GT/s Link Speed defines from Rockchip PCIe header definitions. 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
--- drivers/pci/controller/pcie-rockchip.h | 3 --- 1 file changed, 3 deletions(-)
drivers/pci/controller/pcie-rockchip.h +0 -3
@@ -42,7 +42,6 @@#define PCIE_CLIENT_MODE_RC HWORD_SET_BIT(0x0040)#define PCIE_CLIENT_MODE_EP HWORD_CLR_BIT(0x0040)#define PCIE_CLIENT_GEN_SEL_1 HWORD_CLR_BIT(0x0080) -#define PCIE_CLIENT_GEN_SEL_2 HWORD_SET_BIT(0x0080)#define PCIE_CLIENT_LEGACY_INT_CTRL (PCIE_CLIENT_BASE + 0x0c)#define PCIE_CLIENT_INT_IN_ASSERT HWORD_SET_BIT(0x0002)#define PCIE_CLIENT_INT_IN_DEASSERT HWORD_CLR_BIT(0x0002)
@@ -197,8 +196,6 @@ (((x) & PCIE_CORE_PL_CONF_LS_MASK) == PCIE_CORE_PL_CONF_LS_READY)#define PCIE_LINK_UP (x) \ (((x) & PCIE_CLIENT_LINK_STATUS_MASK) == PCIE_CLIENT_LINK_STATUS_UP) -#define PCIE_LINK_IS_GEN2 (x) \ - (((x) & PCIE_CORE_PL_CONF_SPEED_MASK) == PCIE_CORE_PL_CONF_SPEED_5G)#define RC_REGION_0_ADDR_TRANS_H 0x00000000#define RC_REGION_0_ADDR_TRANS_L 0x00000000
Signature · 1 line
2.52.0
Re: [PATCH v4 1/4] PCI: rockchip: drop 2.5 GT/s defines
Quoted 29 lines · Geraldo Nascimento
Drop the 2.5 GT/s Link Speed defines from Rockchip PCIe header definitions. 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")Reported-by Dragan Simic <[email protected]>Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip.h | 3 --- 1 file changed, 3 deletions(-)drivers/pci/controller/pcie-rockchip.h +0
-0
drivers/pci/controller/pcie-rockchip.h line 45
Quoted 4 lines
drivers/pci/controller/pcie-rockchip.h +0
-1@@ -42,7 +42,6 @@#define PCIE_CLIENT_MODE_RC HWORD_SET_BIT(0x0040)#define PCIE_CLIENT_MODE_EP HWORD_CLR_BIT(0x0040)#define PCIE_CLIENT_GEN_SEL_1 HWORD_CLR_BIT(0x0080) -#define PCIE_CLIENT_GEN_SEL_2 HWORD_SET_BIT(0x0080)
This commit alone won't compile right? PCIE_CLIENT_GEN_SEL_2 is still referenced by rockchip_pcie_init_port():
if (rockchip->link_gen == 2)
rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2,
PCIE_CLIENT_CONFIG);This reference is removed by the next patch in the series but not sure if we want to do it here instead.
drivers/pci/controller/pcie-rockchip.h line 201
Quoted 9 lines
drivers/pci/controller/pcie-rockchip.h +0
-2#define PCIE_CLIENT_LEGACY_INT_CTRL (PCIE_CLIENT_BASE + 0x0c)#define PCIE_CLIENT_INT_IN_ASSERT HWORD_SET_BIT(0x0002)#define PCIE_CLIENT_INT_IN_DEASSERT HWORD_CLR_BIT(0x0002)@@ -197,8 +196,6 @@ (((x) & PCIE_CORE_PL_CONF_LS_MASK) == PCIE_CORE_PL_CONF_LS_READY)#define PCIE_LINK_UP (x) \ (((x) & PCIE_CLIENT_LINK_STATUS_MASK) == PCIE_CLIENT_LINK_STATUS_UP) -#define PCIE_LINK_IS_GEN2 (x) \ - (((x) & PCIE_CORE_PL_CONF_SPEED_MASK) == PCIE_CORE_PL_CONF_SPEED_5G)
Same for this, references elsewhere tho.
Quoted 3 lines
drivers/pci/controller/pcie-rockchip.h +0
-0#define RC_REGION_0_ADDR_TRANS_H 0x00000000#define RC_REGION_0_ADDR_TRANS_L 0x00000000
Re: [PATCH v4 1/4] PCI: rockchip: drop 2.5 GT/s defines
Hi Charalampos,
Quoted 26 lines · Charalampos Mitrodimas
Quoted 24 lines · Geraldo Nascimento
Drop the 2.5 GT/s Link Speed defines from Rockchip PCIe header definitions. 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")Reported-by Dragan Simic <[email protected]>Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip.h | 3 --- 1 file changed, 3 deletions(-)
Quoted 12 lines
drivers/pci/controller/pcie-rockchip.h line 45
Quoted 9 lines
drivers/pci/controller/pcie-rockchip.h +0
-1@@ -42,7 +42,6 @@#define PCIE_CLIENT_MODE_RC HWORD_SET_BIT(0x0040)#define PCIE_CLIENT_MODE_EP HWORD_CLR_BIT(0x0040)#define PCIE_CLIENT_GEN_SEL_1 HWORD_CLR_BIT(0x0080) -#define PCIE_CLIENT_GEN_SEL_2 HWORD_SET_BIT(0x0080)This commit alone won't compile right?
PCIE_CLIENT_GEN_SEL_2is still referenced byrockchip_pcie_init_port():
You are correct, thanks for catching it.
Quoted 8 lines
if (rockchip->link_gen == 2) rockchip_pcie_write(rockchip, PCIE_CLIENT_GEN_SEL_2, PCIE_CLIENT_CONFIG);This reference is removed by the next patch in the series but not sure if we want to do it here instead.
Quoted 12 lines
drivers/pci/controller/pcie-rockchip.h line 201
Quoted 9 lines
drivers/pci/controller/pcie-rockchip.h +0
-2#define PCIE_CLIENT_LEGACY_INT_CTRL (PCIE_CLIENT_BASE + 0x0c)#define PCIE_CLIENT_INT_IN_ASSERT HWORD_SET_BIT(0x0002)#define PCIE_CLIENT_INT_IN_DEASSERT HWORD_CLR_BIT(0x0002)@@ -197,8 +196,6 @@ (((x) & PCIE_CORE_PL_CONF_LS_MASK) == PCIE_CORE_PL_CONF_LS_READY)#define PCIE_LINK_UP (x) \ (((x) & PCIE_CLIENT_LINK_STATUS_MASK) == PCIE_CLIENT_LINK_STATUS_UP) -#define PCIE_LINK_IS_GEN2 (x) \ - (((x) & PCIE_CORE_PL_CONF_SPEED_MASK) == PCIE_CORE_PL_CONF_SPEED_5G)Same for this, references elsewhere tho.
Thanks for pointing this out. I'll do better for v5.
Geraldo Nascimento
Quoted 3 lines
Quoted 3 lines
drivers/pci/controller/pcie-rockchip.h +0
-0#define RC_REGION_0_ADDR_TRANS_H 0x00000000#define RC_REGION_0_ADDR_TRANS_L 0x00000000
PATCH v4 2/4 PCI: rockchip: drive at 2.5 GT/s only and error out other speeds
In reply to Geraldo NascimentoConfigure the core to be driven at 2.5 GT/s Link Speed and error out on any other speed. 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
--- drivers/pci/controller/pcie-rockchip.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)
drivers/pci/controller/pcie-rockchip.c +5 -8
@@ -66,8 +66,9 @@ 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 ) +return dev_err_probe (dev, rockchip->link_gen, +"Invalid Link Speed\n" );for (i =0 ; i <ROCKCHIP_NUM_PM_RSTS ; i++) rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];
@@ -147,12 +148,8 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) goto err_exit_phy; } -if (rockchip->link_gen ==2 ) -rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_2 , -PCIE_CLIENT_CONFIG ); -else -rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_1 , -PCIE_CLIENT_CONFIG ); +rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_1 , +PCIE_CLIENT_CONFIG ); regs =PCIE_CLIENT_ARI_ENABLE |PCIE_CLIENT_CONF_LANE_NUM (rockchip->lanes);
Signature · 1 line
2.52.0
Re: [PATCH v4 2/4] PCI: rockchip: drive at 2.5 GT/s only and error out other speeds
Hello Geraldo,
Quoted 29 lines · Geraldo Nascimento
Configure the core to be driven at 2.5 GT/s Link Speed and error out on any other speed. 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")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 | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)drivers/pci/controller/pcie-rockchip.c +0
-0
drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) · line 71
Quoted 8 lines
drivers/pci/controller/pcie-rockchip.c +3
-2@@ -66,8 +66,9 @@ 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 ) +return dev_err_probe (dev, rockchip->link_gen, +"Invalid Link Speed\n" );
I'm quite surprised to see what happened here in the v4? The changes introduced in this diff block in the v3 were perfectly fine, i.e. we need to correct any runtime occurrences of Gen2 speed setting in the rockchip_ pcie_parse_dt() function, together with emitting a warning that urges the users and the board dtb maintainer to fix the board dtb. I'll get back to this below.
Quoted 7 lines
drivers/pci/controller/pcie-rockchip.c +0
-1for (i =0 ; i <ROCKCHIP_NUM_PM_RSTS ; i++) rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];@@ -147,12 +148,8 @@ 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 155
Quoted 10 lines
drivers/pci/controller/pcie-rockchip.c +2
-5-rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_2 , -PCIE_CLIENT_CONFIG ); - else -rockchip_pcie_write (rockchip ,PCIE_CLIENT_GEN_SEL_1 , -PCIE_CLIENT_CONFIG ); +rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_1 , +PCIE_CLIENT_CONFIG ); regs =PCIE_CLIENT_ARI_ENABLE |PCIE_CLIENT_CONF_LANE_NUM (rockchip->lanes);
At this point we need to check and bail out if no longer supported Gen2 speed setting is received, which also applies to the appropriate spot in the PCIe endpoint driver.
To clarify, it's the responsibility of the rockchip_pcie_parse_dt() function to validate and possibly correct the speed setting, because it is what receives that setting as a value from the board dtb, while the consumers of that validated speed setting should check it, to make sure the speed is right because they no longer can handle requests for Gen2 speed, and simply bail out if the received speed isn't right, i.e. if it differs from Gen1.
Re: [PATCH v4 2/4] PCI: rockchip: drive at 2.5 GT/s only and error out other speeds
Quoted 2 lines · Dragan Simic
Hello Geraldo,
Hi Dragan,
Quoted 32 lines · Geraldo Nascimento
Quoted 31 lines
Configure the core to be driven at 2.5 GT/s Link Speed and error out on any other speed. 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")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 | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)drivers/pci/controller/pcie-rockchip.c +0
-0@@ -66,8 +66,9 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) }
Quoted 13 lines
drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) · line 71
Quoted 6 lines
drivers/pci/controller/pcie-rockchip.c +3
-2rockchip->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 ) +return dev_err_probe (dev, rockchip->link_gen, +"Invalid Link Speed\n" );I'm quite surprised to see what happened here in the v4? The changes introduced in this diff block in the v3 were perfectly fine, i.e. we need to correct any runtime occurrences of Gen2 speed setting in the
rockchip_pcie_parse_dt()function, together with emitting a warning that urges the users and the board dtb maintainer to fix the board dtb. I'll get back to this below.
I see what you mean now. Sure, this could be incorporated for v5 without a problem and is the more proper way to solve it.
Quoted 16 lines
Quoted 16 lines
drivers/pci/controller/pcie-rockchip.c +2
-6for (i =0 ; i <ROCKCHIP_NUM_PM_RSTS ; i++) rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];@@ -147,12 +148,8 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) goto err_exit_phy; } -if (rockchip->link_gen ==2 ) -rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_2 , -PCIE_CLIENT_CONFIG ); -else -rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_1 , -PCIE_CLIENT_CONFIG ); +rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_1 , +PCIE_CLIENT_CONFIG );
Quoted 6 lines
drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) · line 155
Quoted 2 lines
drivers/pci/controller/pcie-rockchip.c +0
-0regs =PCIE_CLIENT_ARI_ENABLE |PCIE_CLIENT_CONF_LANE_NUM (rockchip->lanes);At this point we need to check and bail out if no longer supported Gen2 speed setting is received, which also applies to the appropriate spot in the PCIe endpoint driver.
Right, it is a bit of a double-check I think but it can't hurt.
Quoted 9 lines
To clarify, it's the responsibility of the
rockchip_pcie_parse_dt()function to validate and possibly correct the speed setting, because it is what receives that setting as a value from the board dtb, while the consumers of that validated speed setting should check it, to make sure the speed is right because they no longer can handle requests for Gen2 speed, and simply bail out if the received speed isn't right, i.e. if it differs from Gen1.
Like I said, it ends being a double-check, because we can't let probe progress if link_gen ends up being different than 1. Either we force 2.5 GT/s with a fair warning or we error out on the probe already.
Thank you,
Geraldo Nascimento
Re: [PATCH v4 2/4] PCI: rockchip: drive at 2.5 GT/s only and error out other speeds
Quoted 47 lines · Geraldo Nascimento
Quoted 32 lines · Dragan Simic
Quoted 31 lines · Geraldo Nascimento
Configure the core to be driven at 2.5 GT/s Link Speed and error out on any other speed. 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")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 | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)drivers/pci/controller/pcie-rockchip.c +0
-0@@ -66,8 +66,9 @@ int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) }Quoted 13 lines
drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) · line 71
Quoted 6 lines
drivers/pci/controller/pcie-rockchip.c +3
-2rockchip->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 ) +return dev_err_probe (dev, rockchip->link_gen, +"Invalid Link Speed\n" );I'm quite surprised to see what happened here in the v4? The changes introduced in this diff block in the v3 were perfectly fine, i.e. we need to correct any runtime occurrences of Gen2 speed setting in the
rockchip_pcie_parse_dt()function, together with emitting a warning that urges the users and the board dtb maintainer to fix the board dtb. I'll get back to this below.
Quoted 2 lines
I see what you mean now. Sure, this could be incorporated for v5 without a problem and is the more proper way to solve it.
Actually, we need to do it the way I suggested above, i.e. the v3 way, because doing it like in the v4 diff chunk above could break PCIe on some boards that currently have Gen2 configured in their dtb files, which may get their kernels updated without updating their dtb files as well, for whatever reason. Basically, we need to avoid such breakage scenarios.
Quoted 32 lines
Quoted 15 lines
Quoted 15 lines
drivers/pci/controller/pcie-rockchip.c +2
-6for (i =0 ; i <ROCKCHIP_NUM_PM_RSTS ; i++) rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];@@ -147,12 +148,8 @@ int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) goto err_exit_phy; } -if (rockchip->link_gen ==2 ) -rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_2 , -PCIE_CLIENT_CONFIG ); -else -rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_1 , -PCIE_CLIENT_CONFIG ); +rockchip_pcie_write (rockchip,PCIE_CLIENT_GEN_SEL_1 , +PCIE_CLIENT_CONFIG );Quoted 6 lines
drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_init_port(struct rockchip_pcie *rockchip) · line 155
Quoted 2 lines
drivers/pci/controller/pcie-rockchip.c +0
-0regs =PCIE_CLIENT_ARI_ENABLE |PCIE_CLIENT_CONF_LANE_NUM (rockchip->lanes);At this point we need to check and bail out if no longer supported Gen2 speed setting is received, which also applies to the appropriate spot in the PCIe endpoint driver.
Right, it is a bit of a double-check I think but it can't hurt.
Quoted 7 lines
To clarify, it's the responsibility of the
rockchip_pcie_parse_dt()function to validate and possibly correct the speed setting, because it is what receives that setting as a value from the board dtb, while the consumers of that validated speed setting should check it, to make sure the speed is right because they no longer can handle requests for Gen2 speed, and simply bail out if the received speed isn't right, i.e. if it differs from Gen1.
Quoted 3 lines
Like I said, it ends being a double-check, because we can't let probe progress if
link_genends up being different than 1. Either we force 2.5 GT/s with a fair warning or we error out on the probe already.
Ah, sorry, I somehow missed that both rockchip_pcie_host_init_port() and rockchip_pcie_ep_link_training() are defined as static functions, so you're right that checking for "link_gen == 1" in them again would be redundant.
PATCH v4 3/4 PCI: rockchip-host: do not attempt 5.0 GT/s retraining
In reply to Geraldo NascimentoDrop the 5.0 GT/s Link Speed retraining from Rockchip PCIe Root Complex Mode Operation, so called host driver.
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
--- drivers/pci/controller/pcie-rockchip-host.c | 20 -------------------- 1 file changed, 20 deletions(-)
drivers/pci/controller/pcie-rockchip-host.c +0 -20
@@ -328,26 +328,6 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip) goto err_power_off_phy; } -if (rockchip->link_gen ==2 ) { -/* -* 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 ); - - err =readl_poll_timeout (rockchip->apb_base +PCIE_CORE_CTRL , - status,PCIE_LINK_IS_GEN2 (status),20 , -500 *USEC_PER_MSEC ); -if (err) -dev_dbg (dev,"PCIe link training gen2 timeout, fall back to gen1!\n" ); - } -/* Check the final link width from negotiated lane counter from MGMT */ status =rockchip_pcie_read (rockchip,PCIE_CORE_CTRL ); status = 0x1 << ((status & PCIE_CORE_PL_CONF_LANE_MASK) >>
Signature · 1 line
2.52.0
PATCH v4 4/4 PCI: rockchip-ep: do not attempt 5.0 GT/s retraining
In reply to Geraldo NascimentoDrop the 5.0 GT/s Link Speed retraining code block from Rockchip PCIe header definitions. 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
--- drivers/pci/controller/pcie-rockchip-ep.c | 13 ------------- 1 file changed, 13 deletions(-)
drivers/pci/controller/pcie-rockchip-ep.c +0 -13
@@ -553,19 +553,6 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work)if (ret) goto again; -/* -* Check the current speed: if gen2 speed was requested and we are not -* at gen2 speed yet, retrain again for gen2. -*/ - val =rockchip_pcie_read (rockchip,PCIE_CORE_CTRL ); -if (!PCIE_LINK_IS_GEN2 (val) && rockchip->link_gen ==2 ) { -/* Enable retrain for gen2 */ -rockchip_pcie_ep_retrain_link (rockchip); -readl_poll_timeout (rockchip->apb_base +PCIE_CORE_CTRL , - val,PCIE_LINK_IS_GEN2 (val),50 , -LINK_TRAIN_TIMEOUT ); - } -/* Check again that the link is up */ if (!rockchip_pcie_ep_link_up (rockchip)) goto again;
Signature · 1 line
2.52.0
Re: [PATCH v4 4/4] PCI: rockchip-ep: do not attempt 5.0 GT/s retraining
Quoted 4 lines · Geraldo Nascimento
Drop the 5.0 GT/s Link Speed retraining code block from Rockchip PCIe header definitions. 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.
Patch body says "header definitions" but the change is to the drivers' source. Maybe you need to reword in a way that is clear as to what happened?
Cheers!
Quoted 43 lines
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
Reported-by Dragan Simic <[email protected]>Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip-ep.c | 13 ------------- 1 file changed, 13 deletions(-)drivers/pci/controller/pcie-rockchip-ep.c +0
-13@@ -553,19 +553,6 @@ static void rockchip_pcie_ep_link_training(struct work_struct *work)if (ret) goto again; -/* -* Check the current speed: if gen2 speed was requested and we are not -* at gen2 speed yet, retrain again for gen2. -*/ - val =rockchip_pcie_read (rockchip,PCIE_CORE_CTRL ); -if (!PCIE_LINK_IS_GEN2 (val) && rockchip->link_gen ==2 ) { -/* Enable retrain for gen2 */ -rockchip_pcie_ep_retrain_link (rockchip); -readl_poll_timeout (rockchip->apb_base +PCIE_CORE_CTRL , - val,PCIE_LINK_IS_GEN2 (val),50 , -LINK_TRAIN_TIMEOUT ); - } -/* Check again that the link is up */ if (!rockchip_pcie_ep_link_up (rockchip)) goto again;
Re: [PATCH v4 4/4] PCI: rockchip-ep: do not attempt 5.0 GT/s retraining
Quoted 7 lines · Charalampos Mitrodimas
Quoted 4 lines · Geraldo Nascimento
Drop the 5.0 GT/s Link Speed retraining code block from Rockchip PCIe header definitions. 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.
Quoted 5 lines
Patch body says "header definitions" but the change is to the drivers' source. Maybe you need to reword in a way that is clear as to what happened?
Cheers!
Indeed, I was the victim of reckless copy pasting here.
Will fix for v5.
Thanks,
Geraldo Nascimento