PCI: rockchip-host: support quirky devices
Subsystems (5)
cover PCI: rockchip-host: support quirky devices
With these two changes I'm able to work with a Samsung PM981a OEM SSD that is known to be not-working with Rockchip-IP PCIe.
Previously I attempted a contrived solution that mostly worked for my simple purposes but was rather inelegant and impractical.
Now I have isolated the behavior to the three lines in the two commits. Omit those three lines and you get a working set with the kernel.
I have no idea how to actually implement this in a way that makes sense and doesn't break the PCIe spec but it is my sincere wish that interested RK3399 parties test the change and report any regressions with already-working devices and specifically, successes or failures of initial link-training with these changes.
Geraldo Nascimento (2):
arm64: dts: rockchip: drop PCIe 3v3 always-on/boot-on
PCI: rockchip-host: drop wait on PERST# toggle
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi | 2 -- drivers/pci/controller/pcie-rockchip-host.c | 1 - 2 files changed, 3 deletions(-)
Signature · 1 line
2.49.0
RFC PATCH 1/2 arm64: dts: rockchip: drop PCIe 3v3 always-on/boot-on
Example commit of needed dropping of regulator always-on/boot-on declarations to make sure quirky devices known to not be working on RK3399 are able to enumerate on the PCI bus.
One example only, tested on my ROCK PI N10 board, to avoid patch-bomb
--- arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi | 2 -- 1 file changed, 2 deletions(-)
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +0 -2
@@ -25,8 +25,6 @@ vcc3v3_pcie: regulator-vcc-pcie { pinctrl-names = "default"; pinctrl-0 = <&pcie_pwr>; regulator-name = "vcc3v3_pcie"; - regulator-always-on; - regulator-boot-on; vin-supply = <&vcc5v0_sys>; };
Signature · 1 line
2.49.0
RFC PATCH 2/2 PCI: rockchip-host: drop wait on PERST# toggle
In reply to Geraldo NascimentoWith this change PCIe will complete link-training with a known quirky device - Samsung OEM PM981a SSD. This is completely against the PCIe spec and yet it works as long as the power regulator for 3v3 PCIe power is not defined as always-on or boot-on.
--- drivers/pci/controller/pcie-rockchip-host.c | 1 - 1 file changed, 1 deletion(-)
drivers/pci/controller/pcie-rockchip-host.c +0 -1
@@ -314,7 +314,6 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)rockchip_pcie_write (rockchip,PCIE_CLIENT_LINK_TRAIN_ENABLE ,PCIE_CLIENT_CONFIG ); -msleep (PCIE_T_PVPERL_MS );gpiod_set_value_cansleep (rockchip->perst_gpio,1 );msleep (PCIE_RESET_CONFIG_WAIT_MS );
Signature · 1 line
2.49.0
Re: [RFC PATCH 2/2] PCI: rockchip-host: drop wait on PERST# toggle
Quoted 4 lines · Geraldo Nascimento
With this change PCIe will complete link-training with a known quirky device - Samsung OEM PM981a SSD. This is completely against the PCIe spec and yet it works as long as the power regulator for 3v3 PCIe power is not defined as always-on or boot-on.
What is against the spec? In what way is this SSD "known quirky"? Is there a published erratum for it?
Removing this delay might make this SSD work, but if this delay is required per PCIe spec, how can we be confident that other devices will still work?
Reports of devices that still work is not really enough to move this from the "hack that makes one device work" column to the "safe and effective for all devices" column.
It's easy to see how *lack* of a delay can break something, but much harder to imagine how *removing* a delay can make something work. Devices must be able to tolerate pretty much arbitrary delays.
Quoted 25 lines
Signed-off-by Geraldo Nascimento <[email protected]>--- drivers/pci/controller/pcie-rockchip-host.c | 1 - 1 file changed, 1 deletion(-)drivers/pci/controller/pcie-rockchip-host.c +0
-1@@ -314,7 +314,6 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)rockchip_pcie_write (rockchip,PCIE_CLIENT_LINK_TRAIN_ENABLE ,PCIE_CLIENT_CONFIG ); -msleep (PCIE_T_PVPERL_MS );gpiod_set_value_cansleep (rockchip->perst_gpio,1 );msleep (PCIE_RESET_CONFIG_WAIT_MS );Signature · 7 lines
2.49.0 _______________________________________________ Linux-rockchip mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [RFC PATCH 2/2] PCI: rockchip-host: drop wait on PERST# toggle
Quoted 8 lines · Bjorn Helgaas
Quoted 4 lines · Geraldo Nascimento
With this change PCIe will complete link-training with a known quirky device - Samsung OEM PM981a SSD. This is completely against the PCIe spec and yet it works as long as the power regulator for 3v3 PCIe power is not defined as always-on or boot-on.
What is against the spec? In what way is this SSD "known quirky"? Is there a published erratum for it?
Hi Bjorn!
My proposed change itself is against the spec.
This SSD is known to simply not complete initial link-training with Rockchip-IP PCIe. This is not officialy documented but based on reports across boards (like the Armbian one).
I think it's the other way around though, it's the Rockchip-IP PCIe controller that is quirky somehow and doesn't play nice with many, many devices.
Quoted 4 lines
Removing this delay might make this SSD work, but if this delay is required per PCIe spec, how can we be confident that other devices will still work?
We really can't be sure there will be no side-effects to other devices.
Quoted 4 lines
Reports of devices that still work is not really enough to move this from the "hack that makes one device work" column to the "safe and effective for all devices" column.
I agree, and I knew from the start I would not get encouragements from the community relative to this change. Still, it seemed selfish not to share the workaround.
Quoted 4 lines
It's easy to see how *lack* of a delay can break something, but much harder to imagine how *removing* a delay can make something work. Devices must be able to tolerate pretty much arbitrary delays.
That said, the problem of quirky Rockchip-IP PCIe remains. The fact that removing the delay makes it work with devices that previously did not complete initial link training must at least point to a resolution somehow, even if the present change is unacceptable.
I'll continue to hack around this, see if I can pinpoint exactly why that delay screws up my initial link training.
Thanks,
Geraldo Nascimento
Re: [RFC PATCH 2/2] PCI: rockchip-host: drop wait on PERST# toggle
In reply to Bjorn HelgaasQuoted 13 lines · Bjorn Helgaas
Quoted 4 lines · Geraldo Nascimento
With this change PCIe will complete link-training with a known quirky device - Samsung OEM PM981a SSD. This is completely against the PCIe spec and yet it works as long as the power regulator for 3v3 PCIe power is not defined as always-on or boot-on.
What is against the spec? In what way is this SSD "known quirky"? Is there a published erratum for it?
Removing this delay might make this SSD work, but if this delay is required per PCIe spec, how can we be confident that other devices will still work?
Quoted 7 lines
Reports of devices that still work is not really enough to move this from the "hack that makes one device work" column to the "safe and effective for all devices" column.
It's easy to see how *lack* of a delay can break something, but much harder to imagine how *removing* a delay can make something work. Devices must be able to tolerate pretty much arbitrary delays.
Hi Bjorn!
I did some more testing, intrigued by why would a delay of more than 5 ms after the enablement of the power rails trigger failure in initial link-training.
Something in my intuition kept telling me this was PERST# related, and so I followed that rabbit-hole.
It seems the following change will allow the SSD to work with the Rockchip-IP PCIe core without any other changes. So it is purely a DT change and we are able to keep the mandatory 100ms delay after driving PERST# low, as well as the always-on/boot-on properties of the 3v3 power regulator.
This time everything is within the PCIe spec AFAICT, PERST# indeed is an Open Drain signal, and indeed it does requires pull-up resistor to maintain the drive after driving it high.
I'm still testing the overall stability of this, let's hope for the best!
Thanks,
Geraldo Nascimento
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +7 -2
@@ -383,13 +383,14 @@ &pcie_phy { }; &pcie0 { - ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>; + ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; num-lanes = <4>; - pinctrl-0 = <&pcie_clkreqnb_cpm>; + pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>; pinctrl-names = "default"; vpcie0v9-supply = <&vcca_0v9>; /* VCC_0V9_S0 */ vpcie1v8-supply = <&vcca_1v8>; /* VCC_1V8_S0 */ vpcie3v3-supply = <&vcc3v3_pcie>; + max-link-speed = <2>; status = "okay"; };
@@ -408,6 +409,10 @@ pcie { pcie_pwr: pcie-pwr { rockchip,pins = <4 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>; }; + pcie_perst: pcie-perst { + rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>; + }; + }; pmic {
Re: [RFC PATCH 2/2] PCI: rockchip-host: drop wait on PERST# toggle
Quoted 14 lines · Geraldo Nascimento
Quoted 3 lines · Bjorn Helgaas
Quoted 2 lines · Geraldo Nascimento
With this change PCIe will complete link-training with a known quirky device - Samsung OEM PM981a SSD. This is completely against the PCIe
Something in my intuition kept telling me this was PERST# related, and so I followed that rabbit-hole.
It seems the following change will allow the SSD to work with the Rockchip-IP PCIe core without any other changes. So it is purely a DT change and we are able to keep the mandatory 100ms delay after driving PERST# low, as well as the always-on/boot-on properties of the 3v3 power regulator.
Quoted 6 lines
This time everything is within the PCIe spec AFAICT, PERST# indeed is an Open Drain signal, and indeed it does requires pull-up resistor to maintain the drive after driving it high.
I'm still testing the overall stability of this, let's hope for the best!
I have a Samsung PM981 (without the 'a') and AFAICT it works fine. I had noticed that the PERST# (pinctrl) was missing, but 'ep-gpios' was/is new to me and I hadn't had time to research that properly yet. Good to see you already found it yourself :-)
Cheers,
Diederik
Quoted 33 lines
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +7
-2@@ -383,13 +383,14 @@ &pcie_phy { }; &pcie0 { - ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>; + ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; num-lanes = <4>; - pinctrl-0 = <&pcie_clkreqnb_cpm>; + pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>; pinctrl-names = "default"; vpcie0v9-supply = <&vcca_0v9>; /* VCC_0V9_S0 */ vpcie1v8-supply = <&vcca_1v8>; /* VCC_1V8_S0 */ vpcie3v3-supply = <&vcc3v3_pcie>; + max-link-speed = <2>; status = "okay"; };@@ -408,6 +409,10 @@ pcie { pcie_pwr: pcie-pwr { rockchip,pins = <4 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>; }; + pcie_perst: pcie-perst { + rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>; + }; + }; pmic {
Re: [RFC PATCH 2/2] PCI: rockchip-host: drop wait on PERST# toggle
Quoted 7 lines · Diederik de Haas
I have a Samsung PM981 (without the 'a') and AFAICT it works fine. I had noticed that the PERST# (pinctrl) was missing, but 'ep-gpios' was/is new to me and I hadn't had time to research that properly yet. Good to see you already found it yourself :-)
Cheers,
Diederik
Hello Diederik,
Thanks for the heads up!
Would you mind testing the following DT change to make sure your PM981 continues to work fine?
Thank you,
Geraldo Nascimento
---
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +1 -1
@@ -383,7 +383,7 @@ &pcie_phy { }; &pcie0 { - ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>; + ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_SINGLE_ENDED)>; num-lanes = <4>; pinctrl-0 = <&pcie_clkreqnb_cpm>; pinctrl-names = "default";
Re: [RFC PATCH 2/2] PCI: rockchip-host: drop wait on PERST# toggle
Hi Geraldo,
Quoted 8 lines · Geraldo Nascimento
Quoted 4 lines · Diederik de Haas
I have a Samsung PM981 (without the 'a') and AFAICT it works fine. I had noticed that the PERST# (pinctrl) was missing, but 'ep-gpios' was/is new to me and I hadn't had time to research that properly yet. Good to see you already found it yourself :-)
Would you mind testing the following DT change to make sure your PM981 continues to work fine?
I just learned the the PCIe system on rk3399 is indeed different from the systems where I use it with (rk3568 & rk3588). And 'ep-gpios' is only used with rk3399 based devices (in the Rockchip dts tree), which explains why I hadn't seen that before.
Which in turn means I can't test your proposed change.
I guess I was triggered by RFC patch 2 which said 'a known quirky device' while my Samsung PM981's are working fine ... but with DW based IP. You did specify in your cover letter the connection with Rockchip PCI IP, which apparently can make a (lot of?) difference.
Me finding the PERST# pinctrl in a few minutes and we finding improvements in RockPro64's PCI 'config' recently, indicated to me that a new look into the dts definition may be warranted, before changing ``drivers/pci/controller/pcie-rockchip-host.c`` for everyone.
Cheers,
Diederik
Quoted 18 lines
Thank you,
Geraldo Nascimento---
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +1
-1@@ -383,7 +383,7 @@ &pcie_phy { }; &pcie0 { - ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>; + ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_SINGLE_ENDED)>; num-lanes = <4>; pinctrl-0 = <&pcie_clkreqnb_cpm>; pinctrl-names = "default";
Re: [RFC PATCH 2/2] PCI: rockchip-host: drop wait on PERST# toggle
In reply to Geraldo NascimentoHello Geraldo,
Quoted 28 lines · Geraldo Nascimento
I did some more testing, intrigued by why would a delay of more than 5 ms after the enablement of the power rails trigger failure in initial link-training.
Something in my intuition kept telling me this was PERST# related, and so I followed that rabbit-hole.
It seems the following change will allow the SSD to work with the Rockchip-IP PCIe core without any other changes. So it is purely a DT change and we are able to keep the mandatory 100ms delay after driving PERST# low, as well as the always-on/boot-on properties of the 3v3 power regulator.
This time everything is within the PCIe spec AFAICT, PERST# indeed is an Open Drain signal, and indeed it does requires pull-up resistor to maintain the drive after driving it high.
I'm still testing the overall stability of this, let's hope for the best!
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +0
-0@@ -383,13 +383,14 @@ &pcie_phy { }; &pcie0 {
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi &pcie_phy { · line 393
Quoted 10 lines
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +3
-2- ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>; + ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; num-lanes = <4>; - pinctrl-0 = <&pcie_clkreqnb_cpm>; + pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>; pinctrl-names = "default"; vpcie0v9-supply = <&vcca_0v9>; /* VCC_0V9_S0 */ vpcie1v8-supply = <&vcca_1v8>; /* VCC_1V8_S0 */ vpcie3v3-supply = <&vcc3v3_pcie>; + max-link-speed = <2>;
FWIW, we shouldn't be enabling PCIe Gen2 here, because it's been already disabled for the RK3399 due to unknown errata in the commit 712fa1777207 ("arm64: dts: rockchip: add max-link-speed for rk3399", 2016-12-16). It's perfectly reasonable to assume the same for the RK3399Pro, which is basically RK3399 packaged together with RK1808, AFAIK with no on-package interconnects.
Quoted 14 lines
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +4
-0status = "okay"; };@@ -408,6 +409,10 @@ pcie { pcie_pwr: pcie-pwr { rockchip,pins = <4 RK_PD4 RK_FUNC_GPIO &pcfg_pull_up>; }; + pcie_perst: pcie-perst { + rockchip,pins = <0 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>; + }; + }; pmic {
Re: [RFC PATCH 2/2] PCI: rockchip-host: drop wait on PERST# toggle
Quoted 40 lines · Dragan Simic
Hello Geraldo,
Quoted 37 lines · Geraldo Nascimento
I did some more testing, intrigued by why would a delay of more than 5 ms after the enablement of the power rails trigger failure in initial link-training.
Something in my intuition kept telling me this was PERST# related, and so I followed that rabbit-hole.
It seems the following change will allow the SSD to work with the Rockchip-IP PCIe core without any other changes. So it is purely a DT change and we are able to keep the mandatory 100ms delay after driving PERST# low, as well as the always-on/boot-on properties of the 3v3 power regulator.
This time everything is within the PCIe spec AFAICT, PERST# indeed is an Open Drain signal, and indeed it does requires pull-up resistor to maintain the drive after driving it high.
I'm still testing the overall stability of this, let's hope for the best!
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +2
-2@@ -383,13 +383,14 @@ &pcie_phy { }; &pcie0 { - ep-gpios = <&gpio0 RK_PB4 GPIO_ACTIVE_HIGH>; + ep-gpios = <&gpio0 RK_PB4 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>; num-lanes = <4>; - pinctrl-0 = <&pcie_clkreqnb_cpm>; + pinctrl-0 = <&pcie_clkreqnb_cpm>, <&pcie_perst>; pinctrl-names = "default"; vpcie0v9-supply = <&vcca_0v9>; /* VCC_0V9_S0 */ vpcie1v8-supply = <&vcca_1v8>; /* VCC_1V8_S0 */ vpcie3v3-supply = <&vcc3v3_pcie>;
Quoted 8 lines
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi &pcie_phy { · line 393
Quoted 1 line
arch/arm64/boot/dts/rockchip/rk3399pro-vmarc-som.dtsi +1
-0+ max-link-speed = <2>;FWIW, we shouldn't be enabling PCIe Gen2 here, because it's been already disabled for the RK3399 due to unknown errata in the commit
712fa1777207("arm64: dts: rockchip: add max-link-speed for rk3399", 2016-12-16). It's perfectly reasonable to assume the same for the RK3399Pro, which is basically RK3399 packaged together with RK1808, AFAIK with no on-package interconnects.
Hi Dragan!
Thanks for the catch, you are correct. But in this case it was just for my tests and it crept in in the git diff. I wasn't really proposing to make that change.
Thanks,
Geraldo Nascimento