arm64: dts: rockchip: Add PCIe pinctrls to Turing RK1

PATCH RESEND Superseded Sam Edwards 2 participants 6 messages Dec 5, 2023 – Dec 7, 2023

Subsystems (2)

Device tree bindings Arm Rockchip

lore

Share & tools

lore

Sam Edwards

author

2023-12-05 20:29 UTC #0 lore raw

PATCH RESEND 1/1 arm64: dts: rockchip: Add PCIe pinctrls to Turing RK1

The RK3588 PCIe 3.0 controller seems to have unpredictable behavior when no CLKREQ/PERST/WAKE pins are configured in the pinmux. In particular, it will sometimes (varying between specific RK3588 chips, not over time) shut off the DBI block, and reads to this range will instead stall indefinitely.

When this happens, it will prevent Linux from booting altogether. The PCIe driver will stall the CPU core once it attempts to read the version information from the DBI range.

Fix this boot hang by adding the correct pinctrl configuration to the PCIe 3.0 device node, which is the proper thing to do anyway. While we're at it, also add the necessary configuration to the PCIe 2.0 node, which may or may not fix the equivalent problem over there -- but is the proper thing to do anyway. :)

Fixes 2806a69f3fef6 ("arm64: dts: rockchip: Add Turing RK1 SoM support")

Signed-off-by Sam Edwards <[email protected]>

---
 .../arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +2 ~~-12~~

index 9570b34aca2e..129f14dbd42f 100644
@@ -214,7 +214,7 @@ rgmii_phy: ethernet-phy@1 {
 &pcie2x1l1 {
 	linux,pci-domain = <1>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie2_reset>;
+	pinctrl-0 = <&pcie30x1m1_pins>;
 	reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_HIGH>;
 	status = "okay";
 };

@@ -226,7 +226,7 @@ &pcie30phy {
 &pcie3x4 {
 	linux,pci-domain = <0>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_reset>;
+	pinctrl-0 = <&pcie30x4m1_pins>;
 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";

@@ -245,17 +245,7 @@ hym8563_int: hym8563-int {
 		};
 	};
 
-	pcie2 {
-		pcie2_reset: pcie2-reset {
-			rockchip,pins = <4 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-	};
-
 	pcie3 {
-		pcie3_reset: pcie3-reset {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
 		vcc3v3_pcie30_en: pcie3-reg {
 			rockchip,pins = <2 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
 		};

Signature · 1 line

2.41.0

Heiko Stübner

maintainer

2023-12-06 09:35 UTC #1 lore raw

Am Dienstag, 5. Dezember 2023, 21:28:59 CET schrieb Sam Edwards:

Quoted 27 lines

The RK3588 PCIe 3.0 controller seems to have unpredictable behavior when no CLKREQ/PERST/WAKE pins are configured in the pinmux. In particular, it will sometimes (varying between specific RK3588 chips, not over time) shut off the DBI block, and reads to this range will instead stall indefinitely.
When this happens, it will prevent Linux from booting altogether. The PCIe driver will stall the CPU core once it attempts to read the version information from the DBI range.
Fix this boot hang by adding the correct pinctrl configuration to the PCIe 3.0 device node, which is the proper thing to do anyway. While we're at it, also add the necessary configuration to the PCIe 2.0 node, which may or may not fix the equivalent problem over there -- but is the proper thing to do anyway. :)
Fixes 2806a69f3fef6 ("arm64: dts: rockchip: Add Turing RK1 SoM support")
Signed-off-by Sam Edwards <[email protected]>
---
 .../arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +0 -0
index 9570b34aca2e..129f14dbd42f 100644

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi rgmii_phy: ethernet-phy@1 { · line 217

Quoted 5 lines

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 -1

@@ -214,7 +214,7 @@ rgmii_phy: ethernet-phy@1 {
 &pcie2x1l1 {
 	linux,pci-domain = <1>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie2_reset>;
+	pinctrl-0 = <&pcie30x1m1_pins>;

This really throws me for a loop here - in the original submission too already. Because somehow those pins are named pcie30x1... for the pcie2 controller ;-) .

Quoted 31 lines

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 ~~-11~~

 	reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_HIGH>;
 	status = "okay";
 };

@@ -226,7 +226,7 @@ &pcie30phy {
 &pcie3x4 {
 	linux,pci-domain = <0>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_reset>;
+	pinctrl-0 = <&pcie30x4m1_pins>;
 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";

@@ -245,17 +245,7 @@ hym8563_int: hym8563-int {
 		};
 	};
 
-	pcie2 {
-		pcie2_reset: pcie2-reset {
-			rockchip,pins = <4 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-	};
-
 	pcie3 {
-		pcie3_reset: pcie3-reset {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
 		vcc3v3_pcie30_en: pcie3-reg {
 			rockchip,pins = <2 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
 		};

Sam Edwards

author

2023-12-06 19:42 UTC #2 lore raw

Quoted 23 lines · Heiko Stübner

Am Dienstag, 5. Dezember 2023, 21:28:59 CET schrieb Sam Edwards:
Quoted 22 lines
The RK3588 PCIe 3.0 controller seems to have unpredictable behavior when no CLKREQ/PERST/WAKE pins are configured in the pinmux. In particular, it will sometimes (varying between specific RK3588 chips, not over time) shut off the DBI block, and reads to this range will instead stall indefinitely.
When this happens, it will prevent Linux from booting altogether. The PCIe driver will stall the CPU core once it attempts to read the version information from the DBI range.
Fix this boot hang by adding the correct pinctrl configuration to the PCIe 3.0 device node, which is the proper thing to do anyway. While we're at it, also add the necessary configuration to the PCIe 2.0 node, which may or may not fix the equivalent problem over there -- but is the proper thing to do anyway. :)
Fixes 2806a69f3fef6 ("arm64: dts: rockchip: Add Turing RK1 SoM support")
Signed-off-by Sam Edwards <[email protected]>
---
  .../arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi | 14 ++------------
  1 file changed, 2 insertions(+), 12 deletions(-)

Quoted 14 lines

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi rgmii_phy: ethernet-phy@1 { · line 217
Quoted 10 lines
arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 -1
index 9570b34aca2e..129f14dbd42f 100644
@@ -214,7 +214,7 @@ rgmii_phy: ethernet-phy@1 {
  &pcie2x1l1 {
  	linux,pci-domain = <1>;
  	pinctrl-names = "default";
-	pinctrl-0 = <&pcie2_reset>;
+	pinctrl-0 = <&pcie30x1m1_pins>;
This really throws me for a loop here - in the original submission too already. Because somehow those pins are named pcie30x1... for the pcie2 controller ;-) .

Hi Heiko,

I just double-checked. The RK1's PCIe 2.0 x1 has the following pin assignments:
PCIE1_CLKREQ_N -> AK30 (4 RK_PA0)
PCIE_WAKE (shared with PCIE0 wake signal) -> AL30 (4 RK_PA1) PCIE1_RST_N -> AM29 (4 RK_PA2)

...so the patch's pinmux setting is indeed correct. (But we may still want to drop the reset-gpios property; see my reply to your other email.)

The confusion seems to be that the PCIe 2.0 path used here is:
PCIe30X1_1(1L1) -> Combo PIPE PHY2
(So, a PCIe3 controller, but a PCIe2 PHY.)

The WAKE/CLKREQ/PERST signals are very low-speed, and thus bypass the PHY: the RK3588's IOMUX subsystem connects them directly to the PCIe3 controller. So they are "pcie30" pins in that sense.

The (potential) misnomer here is pcie2x1l1. The controller at 0xFE180000 is unequivocally a PCIe 3.0 core, and it *could* be muxed to a (bifurcated) PCIe 3.0 x2 PHY for true PCIe 3.0 operation. But since it appears that mainline doesn't support this bifurcation (yet), this PCIe 3.0 core can only be used for PCIe 2.0 through combphy2, which is probably why the DT node is labeled pcie2x1l1 (for now).

In any case, thank you for calling attention to this! I enjoyed researching the "why" and hope that it clarifies things for you as well. :)

Cheers,
Sam

Quoted 37 lines

Quoted 33 lines

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 ~~-11~~

  	reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_HIGH>;
  	status = "okay";
  };

@@ -226,7 +226,7 @@ &pcie30phy {
  &pcie3x4 {
  	linux,pci-domain = <0>;
  	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_reset>;
+	pinctrl-0 = <&pcie30x4m1_pins>;
  	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
  	vpcie3v3-supply = <&vcc3v3_pcie30>;
  	status = "okay";

@@ -245,17 +245,7 @@ hym8563_int: hym8563-int {
  		};
  	};
 
-	pcie2 {
-		pcie2_reset: pcie2-reset {
-			rockchip,pins = <4 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-	};
-
  	pcie3 {
-		pcie3_reset: pcie3-reset {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
  		vcc3v3_pcie30_en: pcie3-reg {
  			rockchip,pins = <2 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
  		};

Heiko Stübner

maintainer

2023-12-06 14:55 UTC #3 lore raw

In reply to Sam Edwards

Am Dienstag, 5. Dezember 2023, 21:28:59 CET schrieb Sam Edwards:

Quoted 36 lines

The RK3588 PCIe 3.0 controller seems to have unpredictable behavior when no CLKREQ/PERST/WAKE pins are configured in the pinmux. In particular, it will sometimes (varying between specific RK3588 chips, not over time) shut off the DBI block, and reads to this range will instead stall indefinitely.
When this happens, it will prevent Linux from booting altogether. The PCIe driver will stall the CPU core once it attempts to read the version information from the DBI range.
Fix this boot hang by adding the correct pinctrl configuration to the PCIe 3.0 device node, which is the proper thing to do anyway. While we're at it, also add the necessary configuration to the PCIe 2.0 node, which may or may not fix the equivalent problem over there -- but is the proper thing to do anyway. :)
Fixes 2806a69f3fef6 ("arm64: dts: rockchip: Add Turing RK1 SoM support")
Signed-off-by Sam Edwards <[email protected]>
---
 .../arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 -1
index 9570b34aca2e..129f14dbd42f 100644
@@ -214,7 +214,7 @@ rgmii_phy: ethernet-phy@1 {
 &pcie2x1l1 {
 	linux,pci-domain = <1>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie2_reset>;
+	pinctrl-0 = <&pcie30x1m1_pins>;
 	reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_HIGH>;
 	status = "okay";
 };

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi &pcie30phy { · line 229

Quoted 5 lines

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 -1

@@ -226,7 +226,7 @@ &pcie30phy {
 &pcie3x4 {
 	linux,pci-domain = <0>;
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_reset>;
+	pinctrl-0 = <&pcie30x4m1_pins>;

also, why are you throwing out the pinctrl for the reset pin. That seems not related to the regular pins and you could instead just do

+ pinctrl-0 = <&pcie30x4m1_pins>, <&pcie3_reset>;

Thanks
Heiko

Quoted 22 lines

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +0 ~~-10~~

 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";

@@ -245,17 +245,7 @@ hym8563_int: hym8563-int {
 		};
 	};
 
-	pcie2 {
-		pcie2_reset: pcie2-reset {
-			rockchip,pins = <4 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-	};
-
 	pcie3 {
-		pcie3_reset: pcie3-reset {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
 		vcc3v3_pcie30_en: pcie3-reg {
 			rockchip,pins = <2 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
 		};

Sam Edwards

author

2023-12-06 18:26 UTC #4 lore raw

Quoted 34 lines · Heiko Stübner

Am Dienstag, 5. Dezember 2023, 21:28:59 CET schrieb Sam Edwards:
Quoted 33 lines
The RK3588 PCIe 3.0 controller seems to have unpredictable behavior when no CLKREQ/PERST/WAKE pins are configured in the pinmux. In particular, it will sometimes (varying between specific RK3588 chips, not over time) shut off the DBI block, and reads to this range will instead stall indefinitely.
When this happens, it will prevent Linux from booting altogether. The PCIe driver will stall the CPU core once it attempts to read the version information from the DBI range.
Fix this boot hang by adding the correct pinctrl configuration to the PCIe 3.0 device node, which is the proper thing to do anyway. While we're at it, also add the necessary configuration to the PCIe 2.0 node, which may or may not fix the equivalent problem over there -- but is the proper thing to do anyway. :)
Fixes 2806a69f3fef6 ("arm64: dts: rockchip: Add Turing RK1 SoM support")
Signed-off-by Sam Edwards <[email protected]>
---
  .../arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi | 14 ++------------
  1 file changed, 2 insertions(+), 12 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 -1
index 9570b34aca2e..129f14dbd42f 100644
@@ -214,7 +214,7 @@ rgmii_phy: ethernet-phy@1 {
  &pcie2x1l1 {
  	linux,pci-domain = <1>;
  	pinctrl-names = "default";
-	pinctrl-0 = <&pcie2_reset>;
+	pinctrl-0 = <&pcie30x1m1_pins>;
  	reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_HIGH>;

Quoted 8 lines

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi &pcie30phy { · line 229
Quoted 8 lines
arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 -1
  	status = "okay";
  };
@@ -226,7 +226,7 @@ &pcie30phy {
  &pcie3x4 {
  	linux,pci-domain = <0>;
  	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_reset>;
+	pinctrl-0 = <&pcie30x4m1_pins>;

Hi Heiko,

Review

Quoted 5 lines

also, why are you throwing out the pinctrl for the reset pin. That seems not related to the regular pins and you could instead just do
(quoted diff) +1 -0
+	pinctrl-0 = <&pcie30x4m1_pins>, <&pcie3_reset>;

The pcie30x4m1_pins def does include pinmuxing 4 RK_PB6 to the DW controller already. The v2 patch should probably instead remove the reset-gpios property, since an out-of-band GPIO reset is not needed when the controller can do it.

I'm still looking into the story with the PCIe 2.0 pins, since 2.0x1's PERST# is definitely 4A2. I'll ask around and try to find out where the corresponding CLKREQ# is going.

Quoted 3 lines

Thanks
Heiko

Likewise,
Sam

Quoted 27 lines

Quoted 23 lines

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +0 ~~-10~~

  	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
  	vpcie3v3-supply = <&vcc3v3_pcie30>;
  	status = "okay";

@@ -245,17 +245,7 @@ hym8563_int: hym8563-int {
  		};
  	};
 
-	pcie2 {
-		pcie2_reset: pcie2-reset {
-			rockchip,pins = <4 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-	};
-
  	pcie3 {
-		pcie3_reset: pcie3-reset {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
  		vcc3v3_pcie30_en: pcie3-reg {
  			rockchip,pins = <2 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
  		};

Heiko Stübner

maintainer

2023-12-07 11:41 UTC #5 lore raw

Am Mittwoch, 6. Dezember 2023, 19:26:40 CET schrieb Sam Edwards:

Quoted 51 lines · Heiko Stübner

Quoted 34 lines
Am Dienstag, 5. Dezember 2023, 21:28:59 CET schrieb Sam Edwards:
Quoted 33 lines
The RK3588 PCIe 3.0 controller seems to have unpredictable behavior when no CLKREQ/PERST/WAKE pins are configured in the pinmux. In particular, it will sometimes (varying between specific RK3588 chips, not over time) shut off the DBI block, and reads to this range will instead stall indefinitely.
When this happens, it will prevent Linux from booting altogether. The PCIe driver will stall the CPU core once it attempts to read the version information from the DBI range.
Fix this boot hang by adding the correct pinctrl configuration to the PCIe 3.0 device node, which is the proper thing to do anyway. While we're at it, also add the necessary configuration to the PCIe 2.0 node, which may or may not fix the equivalent problem over there -- but is the proper thing to do anyway. :)
Fixes 2806a69f3fef6 ("arm64: dts: rockchip: Add Turing RK1 SoM support")
Signed-off-by Sam Edwards <[email protected]>
---
  .../arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi | 14 ++------------
  1 file changed, 2 insertions(+), 12 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 -1
index 9570b34aca2e..129f14dbd42f 100644
@@ -214,7 +214,7 @@ rgmii_phy: ethernet-phy@1 {
  &pcie2x1l1 {
  	linux,pci-domain = <1>;
  	pinctrl-names = "default";
-	pinctrl-0 = <&pcie2_reset>;
+	pinctrl-0 = <&pcie30x1m1_pins>;
  	reset-gpios = <&gpio4 RK_PA2 GPIO_ACTIVE_HIGH>;
Quoted 8 lines
arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi &pcie30phy { · line 229
Quoted 8 lines
arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +1 -1
  	status = "okay";
  };
@@ -226,7 +226,7 @@ &pcie30phy {
  &pcie3x4 {
  	linux,pci-domain = <0>;
  	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_reset>;
+	pinctrl-0 = <&pcie30x4m1_pins>;
Hi Heiko,
Quoted 4 lines
also, why are you throwing out the pinctrl for the reset pin. That seems not related to the regular pins and you could instead just do

Quoted 6 lines

Review
Quoted 1 line
(quoted diff) +1 -0
+	pinctrl-0 = <&pcie30x4m1_pins>, <&pcie3_reset>;
The pcie30x4m1_pins def does include pinmuxing 4 RK_PB6 to the DW controller already. The v2 patch should probably instead remove the reset-gpios property, since an out-of-band GPIO reset is not needed when the controller can do it.

yep, also because of the reset-gpios the pinctrl/gpio driver will mux the pin to gpio function even though the pinctrl would've set if the pcie- function before that.

So I'm really in favor of not mixing the two concepts :-) When setting the pins, the reset-gpio should be gone and vice-versa.

Quoted 3 lines

I'm still looking into the story with the PCIe 2.0 pins, since 2.0x1's PERST# is definitely 4A2. I'll ask around and try to find out where the corresponding CLKREQ# is going.

Yeah, I tried reading up in the TRM but it was really hard following which pin-group actually goes to which controller and the naming definitly does not help :-) .

Heiko

Quoted 27 lines

Quoted 26 lines

Quoted 22 lines

arch/arm64/boot/dts/rockchip/rk3588-turing-rk1.dtsi +0 ~~-10~~

  	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
  	vpcie3v3-supply = <&vcc3v3_pcie30>;
  	status = "okay";

@@ -245,17 +245,7 @@ hym8563_int: hym8563-int {
  		};
  	};
 
-	pcie2 {
-		pcie2_reset: pcie2-reset {
-			rockchip,pins = <4 RK_PA2 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-	};
-
  	pcie3 {
-		pcie3_reset: pcie3-reset {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
  		vcc3v3_pcie30_en: pcie3-reg {
  			rockchip,pins = <2 RK_PC5 RK_FUNC_GPIO &pcfg_pull_none>;
  		};