Skip to contentSkip to conversation

arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

PATCH Superseded Anand Moon 3 participants 6 messages Jul 10, 2024 – Jul 11, 2024

Subsystems (2)
Device tree bindings Arm Rockchip

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Anand Moon

author
#0 lore raw

PATCH 1/1 arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

Signed-off-by Anand Moon <[email protected]>
---
 .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +14 -6
index 2e7512676b7e..a9b55b7996cf 100644
@@ -301,7 +301,7 @@ &pcie30phy {
 
 &pcie3x4 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_rst>;
+	pinctrl-0 = <&pcie30x4_perstn_m1 &pcie30x4_clkreqn_m1 &pcie30x4_waken_m1>;
 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";

@@ -340,14 +340,22 @@ pcie2_2_rst: pcie2-2-rst {
 		};
 	};
 
-	pcie3 {
-		pcie3_rst: pcie3-rst {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
+	pcie30x4 {
 		pcie3_vcc3v3_en: pcie3-vcc3v3-en {
 			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
+
+		pcie30x4_clkreqn_m1: pcie30x4-clkreqn-m1 {
+			rockchip,pins = <4 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+
+		pcie30x4_waken_m1: pcie30x4-waken-m1 {
+			rockchip,pins = <4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		pcie30x4_perstn_m1: pcie30x4-perstn-m1 {
+			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
 	};
 
 	usb {

base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905

Signature · 1 line
2.44.0
#1 lore raw

Hi Anand,

Quoted 33 lines · Anand Moon

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

Signed-off-by Anand Moon <[email protected]>
---
 .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +2 -6
index 2e7512676b7e..a9b55b7996cf 100644
@@ -301,7 +301,7 @@ &pcie30phy {
 
 &pcie3x4 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_rst>;
+	pinctrl-0 = <&pcie30x4_perstn_m1 &pcie30x4_clkreqn_m1 &pcie30x4_waken_m1>;
 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";

@@ -340,14 +340,22 @@ pcie2_2_rst: pcie2-2-rst {
 		};
 	};
 
-	pcie3 {
-		pcie3_rst: pcie3-rst {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
+	pcie30x4 {
 		pcie3_vcc3v3_en: pcie3-vcc3v3-en {

arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts pcie2_2_rst: pcie2-2-rst { · line 354

Quoted 10 lines
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +8 -0
 			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
+
+		pcie30x4_clkreqn_m1: pcie30x4-clkreqn-m1 {
+			rockchip,pins = <4 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+
+		pcie30x4_waken_m1: pcie30x4-waken-m1 {
+			rockchip,pins = <4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};

Should these not be routed to the clkreqn_m1 and waken_m1 function instead of gpio function?

E.g. something like:

		pcie30x4m1_pins: pcie30x4m1-pins {
			rockchip,pins =
				<4 RK_PB4 4 &pcfg_pull_none>,
				<4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>,
				<4 RK_PB5 4 &pcfg_pull_none>;
		};

There are other rk35xx boards where only the perstn pin is configured and could use a similar fix.

Regards,
Jonas

Quoted 9 lines
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +4 -0
+
+		pcie30x4_perstn_m1: pcie30x4-perstn-m1 {
+			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
 	};
 
 	usb {

base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905

_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

author
#2 lore raw

Hi Jonas,

Quoted 60 lines · Jonas Karlman

Hi Anand,

Quoted 33 lines · Anand Moon

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

Signed-off-by Anand Moon <[email protected]>
---
 .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +2 -6
index 2e7512676b7e..a9b55b7996cf 100644
@@ -301,7 +301,7 @@ &pcie30phy {
 
 &pcie3x4 {
      pinctrl-names = "default";
-     pinctrl-0 = <&pcie3_rst>;
+     pinctrl-0 = <&pcie30x4_perstn_m1 &pcie30x4_clkreqn_m1 &pcie30x4_waken_m1>;
      reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
      vpcie3v3-supply = <&vcc3v3_pcie30>;
      status = "okay";

@@ -340,14 +340,22 @@ pcie2_2_rst: pcie2-2-rst {
              };
      };
 
-     pcie3 {
-             pcie3_rst: pcie3-rst {
-                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-             };
-
+     pcie30x4 {
              pcie3_vcc3v3_en: pcie3-vcc3v3-en {

arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts pcie2_2_rst: pcie2-2-rst { · line 354

Quoted 10 lines
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +8 -0
                      rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
              };
+
+             pcie30x4_clkreqn_m1: pcie30x4-clkreqn-m1 {
+                     rockchip,pins = <4 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
+             };
+
+             pcie30x4_waken_m1: pcie30x4-waken-m1 {
+                     rockchip,pins = <4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
+             };

Should these not be routed to the clkreqn_m1 and waken_m1 function instead of gpio function?

E.g. something like:

                pcie30x4m1_pins: pcie30x4m1-pins {
                        rockchip,pins =
                                <4 RK_PB4 4 &pcfg_pull_none>,
                                <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>,
                                <4 RK_PB5 4 &pcfg_pull_none>;
                };
Quoted 3 lines

There are other rk35xx boards where only the perstn pin is configured and could use a similar fix.

I understand this grouping for Gpio, but I am not very familiar with this feature.

Quoted 3 lines

Regards,
Jonas

Thanks
-Anand

author
#3 lore raw

Hi Jonas,

Quoted 64 lines · Anand Moon

Hi Jonas,

Quoted 60 lines · Jonas Karlman

Hi Anand,

Quoted 33 lines · Anand Moon

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

Signed-off-by Anand Moon <[email protected]>
---
 .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +2 -6
index 2e7512676b7e..a9b55b7996cf 100644
@@ -301,7 +301,7 @@ &pcie30phy {
 
 &pcie3x4 {
      pinctrl-names = "default";
-     pinctrl-0 = <&pcie3_rst>;
+     pinctrl-0 = <&pcie30x4_perstn_m1 &pcie30x4_clkreqn_m1 &pcie30x4_waken_m1>;
      reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
      vpcie3v3-supply = <&vcc3v3_pcie30>;
      status = "okay";

@@ -340,14 +340,22 @@ pcie2_2_rst: pcie2-2-rst {
              };
      };
 
-     pcie3 {
-             pcie3_rst: pcie3-rst {
-                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-             };
-
+     pcie30x4 {
              pcie3_vcc3v3_en: pcie3-vcc3v3-en {

arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts pcie2_2_rst: pcie2-2-rst { · line 354

Quoted 10 lines
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +8 -0
                      rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
              };
+
+             pcie30x4_clkreqn_m1: pcie30x4-clkreqn-m1 {
+                     rockchip,pins = <4 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
+             };
+
+             pcie30x4_waken_m1: pcie30x4-waken-m1 {
+                     rockchip,pins = <4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
+             };

Should these not be routed to the clkreqn_m1 and waken_m1 function instead of gpio function?

E.g. something like:

                pcie30x4m1_pins: pcie30x4m1-pins {
                        rockchip,pins =
                                <4 RK_PB4 4 &pcfg_pull_none>,
                                <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>,
                                <4 RK_PB5 4 &pcfg_pull_none>;
                };
Quoted 6 lines
Quoted 3 lines

There are other rk35xx boards where only the perstn pin is configured and could use a similar fix.

I understand this grouping for Gpio, but I am not very familiar with this feature.

Already we have a group of this pincrtl in the pinctrl.dtsi, I will use this in my patch.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/rk3588s-pinctrl.dtsi?h=v6.10-rc7#n1775

Thanks for the tip.

Thanks
-Anand

maintainer
#4 lore raw
In reply to Anand Moon

Hi,

Am Mittwoch, 10. Juli 2024, 16:19:56 CEST schrieb Anand Moon:

Quoted 37 lines

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

Signed-off-by Anand Moon <[email protected]>
---
 .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +4 -6
index 2e7512676b7e..a9b55b7996cf 100644
@@ -301,7 +301,7 @@ &pcie30phy {
 
 &pcie3x4 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_rst>;
+	pinctrl-0 = <&pcie30x4_perstn_m1 &pcie30x4_clkreqn_m1 &pcie30x4_waken_m1>;
 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";

@@ -340,14 +340,22 @@ pcie2_2_rst: pcie2-2-rst {
 		};
 	};
 
-	pcie3 {
-		pcie3_rst: pcie3-rst {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
+	pcie30x4 {
 		pcie3_vcc3v3_en: pcie3-vcc3v3-en {
 			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
+
+		pcie30x4_clkreqn_m1: pcie30x4-clkreqn-m1 {

arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts pcie2_2_rst: pcie2-2-rst { · line 358

Quoted 10 lines
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +10 -0
+			rockchip,pins = <4 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
+		};
+
+		pcie30x4_waken_m1: pcie30x4-waken-m1 {
+			rockchip,pins = <4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
+
+		pcie30x4_perstn_m1: pcie30x4-perstn-m1 {
+			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>;
+		};

I'm not sure what you're going for here, but defining them as gpio makes them essentially unused? Shouldn't they go to the pcie controller?

From what I found for example
The PERST# signal is used to indicate when the power supply is within its specified voltage tolerance and is stable.

At least you'll need to explain more in the commit message.

Heiko

author
#5 lore raw

Hi Heiko,

Thanks for your review comments.

Quoted 46 lines · Heiko Stübner

Hi,

Am Mittwoch, 10. Juli 2024, 16:19:56 CEST schrieb Anand Moon:

Quoted 42 lines

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

Signed-off-by Anand Moon <[email protected]>
---
 .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +9 -6
index 2e7512676b7e..a9b55b7996cf 100644
@@ -301,7 +301,7 @@ &pcie30phy {
 
 &pcie3x4 {
      pinctrl-names = "default";
-     pinctrl-0 = <&pcie3_rst>;
+     pinctrl-0 = <&pcie30x4_perstn_m1 &pcie30x4_clkreqn_m1 &pcie30x4_waken_m1>;
      reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
      vpcie3v3-supply = <&vcc3v3_pcie30>;
      status = "okay";

@@ -340,14 +340,22 @@ pcie2_2_rst: pcie2-2-rst {
              };
      };
 
-     pcie3 {
-             pcie3_rst: pcie3-rst {
-                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-             };
-
+     pcie30x4 {
              pcie3_vcc3v3_en: pcie3-vcc3v3-en {
                      rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
              };
+
+             pcie30x4_clkreqn_m1: pcie30x4-clkreqn-m1 {
+                     rockchip,pins = <4 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>;
+             };
+
+             pcie30x4_waken_m1: pcie30x4-waken-m1 {
+                     rockchip,pins = <4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
Quoted 9 lines

arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts pcie2_2_rst: pcie2-2-rst { · line 358

Quoted 5 lines
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +5 -0
+             };
+
+             pcie30x4_perstn_m1: pcie30x4-perstn-m1 {
+                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>;
+             };

I'm not sure what you're going for here, but defining them as gpio makes them essentially unused? Shouldn't they go to the pcie controller?

Ok, Each component of PCIe communication to have the following control signals #PERST # WAKE and CLKREQ from root complex to endpoint.

Quoted 3 lines

From what I found for example
The PERST# signal is used to indicate when the power supply is within its specified voltage tolerance and is stable.

PERST is referred to as a fundamental reset. PERST should be held low until all the power rails
in the system and the reference clock are stable. A transition from low to high in this signal usually
indicates the beginning of link initialization.

Quoted 2 lines

At least you'll need to explain more in the commit message.

Ok,
WAKE and CLKREQ signals are used to transition to and from low-power states. WAKE signal is an active-low signal that is used to return the PCIe interface to an active state
when in a low-power state.
CLKREQ signal is also an active-low signal and is used to request the reference clock.

Quoted 1 line

I will work on and update in the next version.

Quoted 2 lines

Heiko

Thanks
-Anand