PCI: rockchip: 5.0 GT/s speed may be dangerous
Subsystems (5)
cover PCI: rockchip: 5.0 GT/s speed may be dangerous
In recent interactions with Shawn Lin from Rockchip it came to my attention there's an unknown errata regarding 5.0 GT/s operational speed of their PCIe core. According to Shawn there's grave danger even if the odds are low. To contain any damage, let's cover the remaining corner-cases where the default would lead to 5.0 GT/s operation as well as add a comment to Root Complex driver core, documenting this danger.
Geraldo Nascimento (3):
PCI: rockchip: warn of danger of 5.0 GT/s speeds
PCI: rockchip-host: comment danger of 5.0 GT/s speed
arm64: dts: rockchip: drop max-link-speed = <2> in helios64 PCIe
arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 1 - drivers/pci/controller/pcie-rockchip-host.c | 5 +++++ drivers/pci/controller/pcie-rockchip.c | 8 ++++++-- 3 files changed, 11 insertions(+), 3 deletions(-)
Signature · 1 line
2.49.0
PATCH 1/3 PCI: rockchip: warn of danger of 5.0 GT/s speeds
Shawn Lin from Rockchip has reiterated that there may be danger in using their PCIe with 5.0 GT/s speeds. Warn the user if they make a DT change from the default and at the same time, drive at 2.5 GT/s only, in case the DT max-link-speed property is invalid or inexistent.
--- drivers/pci/controller/pcie-rockchip.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/pci/controller/pcie-rockchip.c +6 -2
@@ -66,8 +66,12 @@ 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, set to 2.5 GT/s\n" ); + } +else if (rockchip->link_gen ==2 ) +dev_warn (dev,"5.0 GT/s may lead to catastrophic failure\n" );for (i =0 ; i <ROCKCHIP_NUM_PM_RSTS ; i++) rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];
Signature · 1 line
2.49.0
Re: [PATCH 1/3] PCI: rockchip: warn of danger of 5.0 GT/s speeds
Hello Geraldo,
Quoted 22 lines · Geraldo Nascimento
Shawn Lin from Rockchip has reiterated that there may be danger in using their PCIe with 5.0 GT/s speeds. Warn the user if they make a DT change from the default and at the same time, drive at 2.5 GT/s only, in case the DT max-link-speed property is invalid or inexistent.
Fixes 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/pci/controller/pcie-rockchip.c +0
-1@@ -66,8 +66,12 @@ 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 )
drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) · line 77
Quoted 10 lines
drivers/pci/controller/pcie-rockchip.c +6
-1- rockchip->link_gen =2 ; +if (rockchip->link_gen <0 || rockchip->link_gen >2 ) { + rockchip->link_gen =1 ; +dev_warn (dev,"invalid max-link-speed, set to 2.5 GT/s\n" ); + } +else if (rockchip->link_gen ==2 ) +dev_warn (dev,"5.0 GT/s may lead to catastrophic failure\n" );for (i =0 ; i <ROCKCHIP_NUM_PM_RSTS ; i++) rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];
Looking good to me, thanks for this patch! People often declare PCIe Gen2 on RK3399-based boards as "works for me", which actually happens very often, but such simple evaluations cannot be taken as a proof of Gen2 correctness.
Furthermore, RK3399's internal interconnects limit the effective I/O speed of PCIe transfers already, so switching to PCIe Gen1 actually doesn't result in some large I/O performance penalties.
With all that in mind, please feel free to include
Re: [PATCH 1/3] PCI: rockchip: warn of danger of 5.0 GT/s speeds
Quoted 41 lines · Dragan Simic
Hello Geraldo,
Quoted 22 lines · Geraldo Nascimento
Shawn Lin from Rockchip has reiterated that there may be danger in using their PCIe with 5.0 GT/s speeds. Warn the user if they make a DT change from the default and at the same time, drive at 2.5 GT/s only, in case the DT max-link-speed property is invalid or inexistent.
Fixes 956cd99b35a8 ("PCI: rockchip: Separate common code from RC driver")Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/pci/controller/pcie-rockchip.c +0
-1@@ -66,8 +66,12 @@ 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 )drivers/pci/controller/pcie-rockchip.c int rockchip_pcie_parse_dt(struct rockchip_pcie *rockchip) · line 77
Quoted 10 lines
drivers/pci/controller/pcie-rockchip.c +6
-1- rockchip->link_gen =2 ; +if (rockchip->link_gen <0 || rockchip->link_gen >2 ) { + rockchip->link_gen =1 ; +dev_warn (dev,"invalid max-link-speed, set to 2.5 GT/s\n" ); + } +else if (rockchip->link_gen ==2 ) +dev_warn (dev,"5.0 GT/s may lead to catastrophic failure\n" );for (i =0 ; i <ROCKCHIP_NUM_PM_RSTS ; i++) rockchip->pm_rsts[i].id = rockchip_pci_pm_rsts[i];Looking good to me, thanks for this patch! People often declare PCIe Gen2 on RK3399-based boards as "works for me", which actually happens very often, but such simple evaluations cannot be taken as a proof of Gen2 correctness.
Quoted 7 lines
Furthermore, RK3399's internal interconnects limit the effective I/O speed of PCIe transfers already, so switching to PCIe Gen1 actually doesn't result in some large I/O performance penalties.
With all that in mind, please feel free to include
Reviewed-by Dragan Simic <[email protected]>
Hi Dragan,
I think I'll include a Reported-By: you too, since you were the first to warn me of unknown errata regarding Rockchip PCIe.
I told you it was a test of mine but I didn't realize the grave danger implied by Shawn Lin declarations.
Once I realized this could end badly the only way I could retribute was to send a few corrections :)
Thank you,
Geraldo Nascimento
PATCH 2/3 PCI: rockchip-host: comment danger of 5.0 GT/s speed
In reply to Geraldo NascimentoAccording to Rockchip sources, there is grave danger in enabling 5.0 GT/s speed for this core. Add a comment documenting that danger and discouraging end-users from forcing higher speed through DT changes.
--- drivers/pci/controller/pcie-rockchip-host.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/pci/controller/pcie-rockchip-host.c +5 -0
@@ -332,6 +332,11 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)/* * Enable retrain for gen2. This should be configured only after * gen1 finished. +* +* According to Rockchip this path is dangerous and may lead to +* catastrophic failure. Even if the odds are small, users are +* still discouraged to engage the corresponding DT option. +* */ status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL2 ); status &= ~PCI_EXP_LNKCTL2_TLS ;
Signature · 1 line
2.49.0
Re: [PATCH 2/3] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Hello Geraldo,
Quoted 17 lines · Geraldo Nascimento
According to Rockchip sources, there is grave danger in enabling 5.0 GT/s speed for this core. Add a comment documenting that danger and discouraging end-users from forcing higher speed through DT changes.
Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip-host.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/pci/controller/pcie-rockchip-host.c +0
-0@@ -332,6 +332,11 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)/*
drivers/pci/controller/pcie-rockchip-host.c static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip) · line 342
Quoted 10 lines
drivers/pci/controller/pcie-rockchip-host.c +5
-0* Enable retrain for gen2. This should be configured only after * gen1 finished. +* +* According to Rockchip this path is dangerous and may lead to +* catastrophic failure. Even if the odds are small, users are +* still discouraged to engage the corresponding DT option. +* */ status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL2 ); status &= ~PCI_EXP_LNKCTL2_TLS ;
Looking good to me, thanks for this patch! There's no need to emit warnings here, because they'd be emitted already in the rockchip_pcie_parse_dt() function.
Please feel free to include
Re: [PATCH 2/3] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Quoted 35 lines · Dragan Simic
Hello Geraldo,
Quoted 17 lines · Geraldo Nascimento
According to Rockchip sources, there is grave danger in enabling 5.0 GT/s speed for this core. Add a comment documenting that danger and discouraging end-users from forcing higher speed through DT changes.
Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip-host.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/pci/controller/pcie-rockchip-host.c +0
-0@@ -332,6 +332,11 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)/* drivers/pci/controller/pcie-rockchip-host.c static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip) · line 342
Quoted 10 lines
drivers/pci/controller/pcie-rockchip-host.c +5
-0* Enable retrain for gen2. This should be configured only after * gen1 finished. +* +* According to Rockchip this path is dangerous and may lead to +* catastrophic failure. Even if the odds are small, users are +* still discouraged to engage the corresponding DT option. +* */ status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL2 ); status &= ~PCI_EXP_LNKCTL2_TLS ;Looking good to me, thanks for this patch! There's no need to emit warnings here, because they'd be emitted already in the
rockchip_pcie_parse_dt()function.
Quoted 4 lines
Please feel free to include
Reviewed-by Dragan Simic <[email protected]>
I disagree, I think the comment stands.
Even if we reduce to one line, ex:
+ May cause damage
Regards,
Geraldo Nascimento
Re: [PATCH 2/3] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Quoted 36 lines · Geraldo Nascimento
Quoted 35 lines · Dragan Simic
Quoted 17 lines · Geraldo Nascimento
According to Rockchip sources, there is grave danger in enabling 5.0 GT/s speed for this core. Add a comment documenting that danger and discouraging end-users from forcing higher speed through DT changes.
Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip-host.c | 5 +++++ 1 file changed, 5 insertions(+)drivers/pci/controller/pcie-rockchip-host.c +0
-0@@ -332,6 +332,11 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)/* drivers/pci/controller/pcie-rockchip-host.c static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip) · line 342
Quoted 10 lines
drivers/pci/controller/pcie-rockchip-host.c +5
-0* Enable retrain for gen2. This should be configured only after * gen1 finished. +* +* According to Rockchip this path is dangerous and may lead to +* catastrophic failure. Even if the odds are small, users are +* still discouraged to engage the corresponding DT option. +* */ status =rockchip_pcie_read (rockchip,PCIE_RC_CONFIG_CR +PCI_EXP_LNKCTL2 ); status &= ~PCI_EXP_LNKCTL2_TLS ;Looking good to me, thanks for this patch! There's no need to emit warnings here, because they'd be emitted already in the
rockchip_pcie_parse_dt()function.Please feel free to include
Quoted 8 lines
Quoted 2 lines
Reviewed-by Dragan Simic <[email protected]>I disagree, I think the comment stands.
Even if we reduce to one line, ex:
+ May cause damage
Ah, perhaps I wasn't clear enough, so let me clarify a bit. The comment you added is fine, I just referred to no need for emitting a warning at that point, because it would be emitted already.
Re: [PATCH 2/3] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Quoted 13 lines · Dragan Simic
Quoted 12 lines · Geraldo Nascimento
Quoted 8 lines · Dragan Simic
Looking good to me, thanks for this patch! There's no need to emit warnings here, because they'd be emitted already in the
rockchip_pcie_parse_dt()function.Please feel free to include
Reviewed-by Dragan Simic <[email protected]>I disagree, I think the comment stands.
Quoted 7 lines
Quoted 3 lines
Even if we reduce to one line, ex:
+ May cause damage
Ah, perhaps I wasn't clear enough, so let me clarify a bit. The comment you added is fine, I just referred to no need for emitting a warning at that point, because it would be emitted already.
OK, I get it now so I think it's time to send v2 with all that in mind :)
Thanks,
Geraldo Nascimento
Re: [PATCH 2/3] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Quoted 16 lines · Geraldo Nascimento
Quoted 15 lines · Dragan Simic
Quoted 14 lines · Geraldo Nascimento
Quoted 8 lines · Dragan Simic
Looking good to me, thanks for this patch! There's no need to emit warnings here, because they'd be emitted already in the
rockchip_pcie_parse_dt()function.Please feel free to include
Reviewed-by Dragan Simic <[email protected]>I disagree, I think the comment stands.
Even if we reduce to one line, ex:
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
+ May cause damage
Ah, perhaps I wasn't clear enough, so let me clarify a bit. The comment you added is fine, I just referred to no need for emitting a warning at that point, because it would be emitted already.
OK, I get it now so I think it's time to send v2 with all that in mind :)
Please, wait a day or two before sending the v2, because that's standard procedure. Sending more than one version in the same day is highly discouraged, because it doesn't give enough time to people for reviewing, and may cause people to look at a wrong version.
Re: [PATCH 2/3] PCI: rockchip-host: comment danger of 5.0 GT/s speed
Quoted 4 lines · Dragan Simic
Please, wait a day or two before sending the v2, because that's standard procedure. Sending more than one version in the same day is highly discouraged, because it doesn't give enough time to people for reviewing, and may cause people to look at a wrong version.
Yeah, you're right, sorry.
PATCH 3/3 arm64: dts: rockchip: drop max-link-speed = <2> in helios64 PCIe
In reply to Geraldo NascimentoShawn Lin from Rockchip strongly discourages attempts to use their RK3399 PCIe core at 5.0 GT/s speed, citing concerns about catastrophic failures that may happen. Even if the odds are low, drop from last user of this property for the RK3399 platform, helios64.
--- arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 1 - 1 file changed, 1 deletion(-)
arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0 -1
@@ -424,7 +424,6 @@ &pcie_phy { &pcie0 { ep-gpios = <&gpio2 RK_PD4 GPIO_ACTIVE_HIGH>; - max-link-speed = <2>; num-lanes = <2>; pinctrl-names = "default"; status = "okay";
Signature · 1 line
2.49.0
Re: [PATCH 3/3] arm64: dts: rockchip: drop max-link-speed = <2> in helios64 PCIe
Hello Geraldo,
Quoted 18 lines · Geraldo Nascimento
Shawn Lin from Rockchip strongly discourages attempts to use their RK3399 PCIe core at 5.0 GT/s speed, citing concerns about catastrophic failures that may happen. Even if the odds are low, drop from last user of this property for the RK3399 platform, helios64.
Fixes 755fff528b1b ("arm64: dts: rockchip: add variables for pcie completion to helios64")Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 1 - 1 file changed, 1 deletion(-)arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-0
arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts &pcie_phy { · line 429
Quoted 7 lines
arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-1@@ -424,7 +424,6 @@ &pcie_phy { &pcie0 { ep-gpios = <&gpio2 RK_PD4 GPIO_ACTIVE_HIGH>; - max-link-speed = <2>; num-lanes = <2>; pinctrl-names = "default"; status = "okay";
Looking good to me, this rounds up the prevention of issues coming from buggy PCIe Gen2 on RK3399.
Please feel free to include
Though, could you, please, add patch 4/3 to this series, which would remove the redundant parameter "max-link-speed = <1>" from rk3399-nanopi-r4s.dtsi?
Re: [PATCH 3/3] arm64: dts: rockchip: drop max-link-speed = <2> in helios64 PCIe
Quoted 30 lines · Dragan Simic
Quoted 18 lines · Geraldo Nascimento
Shawn Lin from Rockchip strongly discourages attempts to use their RK3399 PCIe core at 5.0 GT/s speed, citing concerns about catastrophic failures that may happen. Even if the odds are low, drop from last user of this property for the RK3399 platform, helios64.
Fixes 755fff528b1b ("arm64: dts: rockchip: add variables for pcie completion to helios64")Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 1 - 1 file changed, 1 deletion(-)arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-0arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts &pcie_phy { · line 429
Quoted 7 lines
arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-1@@ -424,7 +424,6 @@ &pcie_phy { &pcie0 { ep-gpios = <&gpio2 RK_PD4 GPIO_ACTIVE_HIGH>; - max-link-speed = <2>; num-lanes = <2>; pinctrl-names = "default"; status = "okay";Looking good to me, this rounds up the prevention of issues coming from buggy PCIe Gen2 on RK3399.
Quoted 7 lines
Please feel free to include
Reviewed-by Dragan Simic <[email protected]>Though, could you, please, add patch 4/3 to this series, which would remove the redundant parameter "max-link-speed = <1>" from
rk3399-nanopi-r4s.dtsi?
Sorry, I forgot to note that the patch summary would read a bit better if it were reworded like this:
arm64: dts: rockchip: Remove redundant max-link-speed from helios64 board dts
Re: [PATCH 3/3] arm64: dts: rockchip: drop max-link-speed = <2> in helios64 PCIe
Quoted 35 lines · Dragan Simic
Quoted 34 lines · Dragan Simic
Quoted 18 lines · Geraldo Nascimento
Shawn Lin from Rockchip strongly discourages attempts to use their RK3399 PCIe core at 5.0 GT/s speed, citing concerns about catastrophic failures that may happen. Even if the odds are low, drop from last user of this property for the RK3399 platform, helios64.
Fixes 755fff528b1b ("arm64: dts: rockchip: add variables for pcie completion to helios64")Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 1 - 1 file changed, 1 deletion(-)arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-0arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts &pcie_phy { · line 429
Quoted 7 lines
arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-1@@ -424,7 +424,6 @@ &pcie_phy { &pcie0 { ep-gpios = <&gpio2 RK_PD4 GPIO_ACTIVE_HIGH>; - max-link-speed = <2>; num-lanes = <2>; pinctrl-names = "default"; status = "okay";Looking good to me, this rounds up the prevention of issues coming from buggy PCIe Gen2 on RK3399.
Please feel free to include
Reviewed-by Dragan Simic <[email protected]>
Quoted 9 lines
Quoted 3 lines
Though, could you, please, add patch 4/3 to this series, which would remove the redundant parameter "max-link-speed = <1>" from
rk3399-nanopi-r4s.dtsi?Sorry, I forgot to note that the patch summary would read a bit better if it were reworded like this:
arm64: dts: rockchip: Remove redundant max-link-speed from helios64 board dts
No, I think:
arm64: dts: rockchip: Remove dangerous max-link-speed from helios64 board dts
Is more accurately described. With focus on the dangerous, we're not liable.
Redudant max-link-speed only for nanopi-r4s include definitions I think!
Thanks,
Geraldo Nascimento
Re: [PATCH 3/3] arm64: dts: rockchip: drop max-link-speed = <2> in helios64 PCIe
Quoted 45 lines · Geraldo Nascimento
Quoted 35 lines · Dragan Simic
Quoted 34 lines · Dragan Simic
Quoted 18 lines · Geraldo Nascimento
Shawn Lin from Rockchip strongly discourages attempts to use their RK3399 PCIe core at 5.0 GT/s speed, citing concerns about catastrophic failures that may happen. Even if the odds are low, drop from last user of this property for the RK3399 platform, helios64.
Fixes 755fff528b1b ("arm64: dts: rockchip: add variables for pcie completion to helios64")Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 1 - 1 file changed, 1 deletion(-)arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-0arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts &pcie_phy { · line 429
Quoted 7 lines
arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-1@@ -424,7 +424,6 @@ &pcie_phy { &pcie0 { ep-gpios = <&gpio2 RK_PD4 GPIO_ACTIVE_HIGH>; - max-link-speed = <2>; num-lanes = <2>; pinctrl-names = "default"; status = "okay";Looking good to me, this rounds up the prevention of issues coming from buggy PCIe Gen2 on RK3399.
Please feel free to include
Reviewed-by Dragan Simic <[email protected]>Quoted 8 lines
Quoted 3 lines
Though, could you, please, add patch 4/3 to this series, which would remove the redundant parameter "max-link-speed = <1>" from
rk3399-nanopi-r4s.dtsi?Sorry, I forgot to note that the patch summary would read a bit better if it were reworded like this:
arm64: dts: rockchip: Remove redundant max-link-speed from helios64 board dts
Quoted 8 lines
No, I think:
arm64: dts: rockchip: Remove dangerous max-link-speed from helios64 board dts
Is more accurately described. With focus on the dangerous, we're not liable.
Redudant max-link-speed only for nanopi-r4s include definitions I think!
Indeed, you're right. It was obviously an -ENOCOFFEE on my part. :)
Re: [PATCH 3/3] arm64: dts: rockchip: drop max-link-speed = <2> in helios64 PCIe
In reply to Dragan SimicQuoted 32 lines · Dragan Simic
Hello Geraldo,
Quoted 18 lines · Geraldo Nascimento
Shawn Lin from Rockchip strongly discourages attempts to use their RK3399 PCIe core at 5.0 GT/s speed, citing concerns about catastrophic failures that may happen. Even if the odds are low, drop from last user of this property for the RK3399 platform, helios64.
Fixes 755fff528b1b ("arm64: dts: rockchip: add variables for pcie completion to helios64")Reported-by Shawn Lin <[email protected]>Signed-off-by Geraldo Nascimento <[email protected]>--- arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 1 - 1 file changed, 1 deletion(-)arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-0arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts &pcie_phy { · line 429
Quoted 7 lines
arch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts +0
-1@@ -424,7 +424,6 @@ &pcie_phy { &pcie0 { ep-gpios = <&gpio2 RK_PD4 GPIO_ACTIVE_HIGH>; - max-link-speed = <2>; num-lanes = <2>; pinctrl-names = "default"; status = "okay";Looking good to me, this rounds up the prevention of issues coming from buggy PCIe Gen2 on RK3399.
Quoted 8 lines
Please feel free to include
Reviewed-by Dragan Simic <[email protected]>Though, could you, please, add patch 4/3 to this series, which would remove the redundant parameter "max-link-speed = <1>" from
rk3399-nanopi-r4s.dtsi?
Thanks for catch, will certainly be included in v2 after I get a few more reviews.
Geraldo Nascimento
Shawn Lin
maintainerHi Geraldo,
Quoted 8 lines · Geraldo Nascimento
In recent interactions with Shawn Lin from Rockchip it came to my attention there's an unknown errata regarding 5.0 GT/s operational speed of their PCIe core. According to Shawn there's grave danger even if the odds are low. To contain any damage, let's cover the remaining corner-cases where the default would lead to 5.0 GT/s operation as well as add a comment to Root Complex driver core, documenting this danger.
I'm not sure just adding a warn would be a good choice. Could we totally force to use gen1 and add a warn if trying to use Gen2.
Meanwhile amend the commit message to add a reference
of RK3399 official datesheet[1] which says PCIe on RK3399 should only support 2.5GT/s?
[1]https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf
Quoted 10 lines
Geraldo Nascimento (3):
PCI: rockchip: warn of danger of 5.0 GT/s speeds
PCI: rockchip-host: comment danger of 5.0 GT/s speed
arm64: dts: rockchip: drop max-link-speed = <2> in helios64 PCIearch/arm64/boot/dts/rockchip/rk3399-kobol-helios64.dts | 1 - drivers/pci/controller/pcie-rockchip-host.c | 5 +++++ drivers/pci/controller/pcie-rockchip.c | 8 ++++++-- 3 files changed, 11 insertions(+), 3 deletions(-)
Hello Shawn and Geraldo,
Quoted 11 lines · Shawn Lin
Quoted 7 lines · Geraldo Nascimento
In recent interactions with Shawn Lin from Rockchip it came to my attention there's an unknown errata regarding 5.0 GT/s operational speed of their PCIe core. According to Shawn there's grave danger even if the odds are low. To contain any damage, let's cover the remaining corner-cases where the default would lead to 5.0 GT/s operation as well as add a comment to Root Complex driver core, documenting this danger.
I'm not sure just adding a warn would be a good choice. Could we totally force to use gen1 and add a warn if trying to use Gen2.
I think that forcing 2.5 GT/s with an appropriate warning message is a good idea. That would be like some quirk that gets applied automatically, to prevent data corruption, while warning people who attempt to "overclock" the PCIe interface.
Quoted 5 lines
Meanwhile amend the commit message to add a reference
of RK3399 official datesheet[1] which says PCIe on RK3399 should only support 2.5GT/s?[1]https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf
Also, rewording the patch summary as follows below may be good, because that would provide more details:
PCI: rockchip: Warn about Gen2 5.0 GT/s on RK3399 being unsafe
Or, if we'll go with the automatic downgrading, like this:
PCI: rockchip: Limit RK3399 to Gen1 2.5 GT/s to prevent breakage
Quoted 12 lines · Dragan Simic
Hello Shawn and Geraldo,
Quoted 9 lines · Shawn Lin
Quoted 7 lines · Geraldo Nascimento
In recent interactions with Shawn Lin from Rockchip it came to my attention there's an unknown errata regarding 5.0 GT/s operational speed of their PCIe core. According to Shawn there's grave danger even if the odds are low. To contain any damage, let's cover the remaining corner-cases where the default would lead to 5.0 GT/s operation as well as add a comment to Root Complex driver core, documenting this danger.
Quoted 7 lines
Quoted 2 lines
I'm not sure just adding a warn would be a good choice. Could we totally force to use gen1 and add a warn if trying to use Gen2.
I think that forcing 2.5 GT/s with an appropriate warning message is a good idea. That would be like some quirk that gets applied automatically, to prevent data corruption, while warning people who attempt to "overclock" the PCIe interface.
Hi Shawn and Dragan,
Alright, I'll send v2 with this suggestion in mind. So that driving the core at 5.0 GT/s will require patching and compiling own kernel.
Quoted 7 lines
Quoted 6 lines
Meanwhile amend the commit message to add a reference
of RK3399 official datesheet[1] which says PCIe on RK3399 should only support 2.5GT/s?[1]https://opensource.rock-chips.com/images/d/d7/Rockchip_RK3399_Datasheet_V2.1-20200323.pdf
Shawn, URLs have the bad habit of changing or simply disappearing, so I don't think it's a good idea to put URL in the commit message.
Also, the datasheet just mentions that RK3399 supports only 2.5 GT/s, it does not mention possible damage from driving the core at 5.0 GT/s.
Quoted 9 lines
Also, rewording the patch summary as follows below may be good, because that would provide more details:
PCI: rockchip: Warn about Gen2 5.0 GT/s on RK3399 being unsafe
Or, if we'll go with the automatic downgrading, like this:
PCI: rockchip: Limit RK3399 to Gen1 2.5 GT/s to prevent breakage
Dragan, these are good ones, thanks. Though I think I'll omit Gen1/Gen2 wording since I know how much Bjorn dislikes those terms.
Thanks,
Geraldo Nascimento
Hello Geraldo,
Quoted 26 lines · Geraldo Nascimento
Quoted 10 lines · Dragan Simic
Quoted 9 lines · Shawn Lin
Quoted 7 lines · Geraldo Nascimento
In recent interactions with Shawn Lin from Rockchip it came to my attention there's an unknown errata regarding 5.0 GT/s operational speed of their PCIe core. According to Shawn there's grave danger even if the odds are low. To contain any damage, let's cover the remaining corner-cases where the default would lead to 5.0 GT/s operation as well as add a comment to Root Complex driver core, documenting this danger.
Quoted 7 lines
Quoted 2 lines
I'm not sure just adding a warn would be a good choice. Could we totally force to use gen1 and add a warn if trying to use Gen2.
I think that forcing 2.5 GT/s with an appropriate warning message is a good idea. That would be like some quirk that gets applied automatically, to prevent data corruption, while warning people who attempt to "overclock" the PCIe interface.
Alright, I'll send v2 with this suggestion in mind. So that driving the core at 5.0 GT/s will require patching and compiling own kernel.
Quoted 4 lines
Quoted 4 lines
Meanwhile amend the commit message to add a reference
of RK3399 official datesheet[1] which says PCIe on RK3399 should only support 2.5GT/s?
Quoted 5 lines
Quoted 2 lines
Shawn, URLs have the bad habit of changing or simply disappearing, so I don't think it's a good idea to put URL in the commit message.
Ah, it's actually perfectly fine, there's always Wayback Machibe to rescue references/URLs that may disappear over time. Wikipedia relies heavily on exactly that mechanism, for example.
Quoted 2 lines
Also, the datasheet just mentions that RK3399 supports only 2.5 GT/s, it does not mention possible damage from driving the core at 5.0 GT/s.
True, but having an additional reference doesn't hurt, especially because the revision history mentions, albeit vaguely, an update to the PCIe specification in version 1.1.of the datasheet. Though, if we had the version 1.0 publicly available for comparison, that would've been much better. :)
Quoted 11 lines
Quoted 8 lines
Also, rewording the patch summary as follows below may be good, because that would provide more details:
PCI: rockchip: Warn about Gen2 5.0 GT/s on RK3399 being unsafe
Or, if we'll go with the automatic downgrading, like this:
PCI: rockchip: Limit RK3399 to Gen1 2.5 GT/s to prevent breakage
Dragan, these are good ones, thanks. Though I think I'll omit Gen1/Gen2 wording since I know how much Bjorn dislikes those terms.
I'm glad that you like those, and I also thought about not including the GenX parts, because they're basically a bit redundant. :)