Skip to contentSkip to conversation

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

PATCH Superseded Anand Moon 3 participants 7 messages Jul 26, 2024 – Jul 28, 2024

Subsystems (2)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Anand Moon

author
#0 lore raw

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

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Each component of PCIe communication have the following control
signals: PERST, WAKE, CLKREQ, and REFCLK. These signals work to generate high-speed signals and communicate with other PCIe devices. Used by root complex to endpoint depending on the power state.

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.

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.

Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

---
v4: rebase on master, used RK_FUNC_GPIO GPIO function instead of PIN number.
V3: use pinctrl local to board
V2: Update the commit messge to describe the changs.
use pinctl group as its pre define in pinctrl dtsi

---
 .../arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +12 -6
index 966bbc582d89..1c7080cca11f 100644
@@ -338,7 +338,7 @@ &pcie30phy {
 
 &pcie3x4 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_rst>;
+	pinctrl-0 = <&pcie30x4_pins>;
 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";
@@ -377,14 +377,20 @@ 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_pins: pcie30x4-pins {
+			rockchip,pins =
+				/* PCIE30X4_CLKREQn_M1_L */
+				<4 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>,
+				/* PCIE30X4_PERSTn_M1_L */
+				<4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>,
+				/* PCIE30X4_WAKEn_M1_L */
+				<4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
 	};
 
 	usb {

base-commit: 1722389b0d863056d78287a120a1d6cadb8d4f7b

Signature · 1 line
2.44.0
author
#1 lore raw

PATCH v4 2/3 arm64: dts: rockchip: Add missing pinctrl for PCIe30x1 node

Add missing pinctrl settings for PCIe 3.0 x1 clock request and wake signals. Each component of PCIe communication have the following control
signals: PERST, WAKE, CLKREQ, and REFCLK. These signals work to generate high-speed signals and communicate with other PCIe devices. Used by root complex to endpoint depending on the power state.

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.

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.

Rename node from 'pcie2' to 'pcie30x1' to align with schematic nomenclature.

---
v4: rebase on master, used RK_FUNC_GPIO GPIO function instead of PIN number.
V3: New patch.

---
 .../arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +13 -5
index 1c7080cca11f..eeae14842b71 100644
@@ -318,7 +318,7 @@ map2 {
 
 &pcie2x1l0 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie2_0_rst>;
+	pinctrl-0 = <&pcie30x1_pins>;
 	reset-gpios = <&gpio4 RK_PA5 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie2x1l0>;
 	status = "okay";
@@ -364,16 +364,24 @@ hp_detect: hp-detect {
 	};
 
 	pcie2 {
-		pcie2_0_rst: pcie2-0-rst {
-			rockchip,pins = <4 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
+		pcie2_2_rst: pcie2-2-rst {
+			rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
+	};
 
+	pcie30x1 {
 		pcie2_0_vcc3v3_en: pcie2-0-vcc-en {
 			rockchip,pins = <1 RK_PD2 RK_FUNC_GPIO &pcfg_pull_none>;
 		};
 
-		pcie2_2_rst: pcie2-2-rst {
-			rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
+		pcie30x1_pins: pcie30x1-pins {
+			rockchip,pins =
+				/* PCIE30x1_0_CLKREQn_M1_L */
+				<4 RK_PA3 RK_FUNC_GPIO &pcfg_pull_down>,
+				/* PCIE30x1_0_PERSTn_M1_L */
+				<4 RK_PA5 RK_FUNC_GPIO &pcfg_pull_down>,
+				/* PCIE30x1_0_WAKEn_M1_L */
+				<4 RK_PA4 RK_FUNC_GPIO &pcfg_pull_down>;
 		};
 	};
 
Signature · 1 line
2.44.0
author
#2 lore raw

PATCH v4 3/3 arm64: dts: rockchip: Add missing pinctrl for PCIe20x1 node

In reply to Anand Moon

Add missing pinctrl settings for PCIe 2.0 x1 clock request and wake signals. Each component of PCIe communication have the following control
signals: PERST, WAKE, CLKREQ, and REFCLK. These signals work to generate high-speed signals and communicate with other PCIe devices. Used by root complex to endpoint depending on the power state.

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.

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.

Rename node from 'pcie2' to 'pcie20x1' to align with schematic nomenclature.

---
v4: rebase on master, used RK_FUNC_GPIO GPIO function instead of PIN number.
V3: New patch.

---
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +10 -4
index eeae14842b71..813a28b8916e 100644
@@ -326,7 +326,7 @@ &pcie2x1l0 {
 
 &pcie2x1l2 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie2_2_rst>;
+	pinctrl-0 = <&pcie20x12_pins>;
 	reset-gpios = <&gpio3 RK_PB0 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie2x1l2>;
 	status = "okay";
@@ -363,9 +363,15 @@ hp_detect: hp-detect {
 		};
 	};
 
-	pcie2 {
-		pcie2_2_rst: pcie2-2-rst {
-			rockchip,pins = <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_none>;
+	pcie20x1 {
+		pcie20x12_pins: pcie20x12-pins {
+			rockchip,pins =
+				/* PCIE20_1_2_CLKREQn_M1_L */
+				<3 RK_PC7 RK_FUNC_GPIO &pcfg_pull_up>,
+				/* PCIE_PERST_L */
+				<3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_up>,
+				/* PCIE20_1_2_WAKEn_M1_L */
+				<3 RK_PD0 RK_FUNC_GPIO &pcfg_pull_up>;
 		};
 	};
 
Signature · 1 line
2.44.0
#3 lore raw
In reply to Anand Moon

Hi Anand,

Sorry for no reply to your v3.

Quoted 17 lines · Anand Moon

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Each component of PCIe communication have the following control
signals: PERST, WAKE, CLKREQ, and REFCLK. These signals work to generate high-speed signals and communicate with other PCIe devices. Used by root complex to endpoint depending on the power state.

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.

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 7 lines

Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

---
v4: rebase on master, used RK_FUNC_GPIO GPIO function instead of PIN number.

Why this change? Only reset should use gpio function, if I am not mistaken. Also how come you change the internal pull-up/down on these pins?, and why do they differ for each pcie node in this series?

Please see [1] for some discussion related to these pins.

"""
The PERST is for sure should work as GPIO, and the same as WAKE;

for CLKREQ, only those board want to support L1SS need to work as function IO,
"""

As stated earlier only the reset pin need to be muxed to GPIO function, and that should also matches the only pin controlled with gpio in the driver, if I am not mistaken.

[1] https://lore.kernel.org/u-boot/[email protected]/

Regards,
Jonas

Quoted 48 lines

V3: use pinctrl local to board
V2: Update the commit messge to describe the changs.
use pinctl group as its pre define in pinctrl dtsi

---
 .../arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +12 -6
index 966bbc582d89..1c7080cca11f 100644
@@ -338,7 +338,7 @@ &pcie30phy {
 
 &pcie3x4 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_rst>;
+	pinctrl-0 = <&pcie30x4_pins>;
 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";
@@ -377,14 +377,20 @@ 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_pins: pcie30x4-pins {
+			rockchip,pins =
+				/* PCIE30X4_CLKREQn_M1_L */
+				<4 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>,
+				/* PCIE30X4_PERSTn_M1_L */
+				<4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>,
+				/* PCIE30X4_WAKEn_M1_L */
+				<4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
 	};
 
 	usb {

base-commit: 1722389b0d863056d78287a120a1d6cadb8d4f7b

author
#4 lore raw

Hi Jonas,

Quoted 31 lines · Jonas Karlman

Hi Anand,

Sorry for no reply to your v3.

Quoted 17 lines · Anand Moon

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Each component of PCIe communication have the following control
signals: PERST, WAKE, CLKREQ, and REFCLK. These signals work to generate high-speed signals and communicate with other PCIe devices. Used by root complex to endpoint depending on the power state.

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.

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 7 lines

Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

---
v4: rebase on master, used RK_FUNC_GPIO GPIO function instead of PIN number.

Quoted 5 lines

Why this change? Only reset should use gpio function, if I am not mistaken. Also how come you change the internal pull-up/down on these pins?, and why do they differ for each pcie node in this series?

Please see [1] for some discussion related to these pins.

I thought every board-specific dts supported GPIO function,

Quoted 7 lines

"""
The PERST is for sure should work as GPIO, and the same as WAKE;

for CLKREQ, only those board want to support L1SS need to work as function IO,
"""

Ok understood.

Quoted 4 lines

As stated earlier only the reset pin need to be muxed to GPIO function, and that should also matches the only pin controlled with gpio in the driver, if I am not mistaken.

I will drop this in the next version.

Quoted 2 lines

[1] https://lore.kernel.org/u-boot/[email protected]/

I'm sorry, I did not read the complete email thread.

Quoted 4 lines

Regards,
Jonas

Thanks
-Anand

#5 lore raw
In reply to Anand Moon

Hello Anand,

Quoted 11 lines · Anand Moon

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Each component of PCIe communication have the following control
signals: PERST, WAKE, CLKREQ, and REFCLK. These signals work to generate high-speed signals and communicate with other PCIe devices. Used by root complex to endpoint depending on the power state.

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 8 lines

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.

Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

I wonder why the three patches in this series cannot be squashed into a single patch, because they target the same thing for the same board dts file? I don't think that having these three separate patches may help with possible regression tracking in the future, for example.

Quoted 53 lines

---
v4: rebase on master, used RK_FUNC_GPIO GPIO function instead of PIN number.
V3: use pinctrl local to board
V2: Update the commit messge to describe the changs.
use pinctl group as its pre define in pinctrl dtsi

---
 .../arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)
arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts +12 -6
index 966bbc582d89..1c7080cca11f 100644
@@ -338,7 +338,7 @@ &pcie30phy {
 
 &pcie3x4 {
 	pinctrl-names = "default";
-	pinctrl-0 = <&pcie3_rst>;
+	pinctrl-0 = <&pcie30x4_pins>;
 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";
@@ -377,14 +377,20 @@ 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_pins: pcie30x4-pins {
+			rockchip,pins =
+				/* PCIE30X4_CLKREQn_M1_L */
+				<4 RK_PB4 RK_FUNC_GPIO &pcfg_pull_up>,
+				/* PCIE30X4_PERSTn_M1_L */
+				<4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>,
+				/* PCIE30X4_WAKEn_M1_L */
+				<4 RK_PB5 RK_FUNC_GPIO &pcfg_pull_down>;
+		};
 	};
 
 	usb {

base-commit: 1722389b0d863056d78287a120a1d6cadb8d4f7b

author
#6 lore raw

Hi Dragan,

Quoted 24 lines · Dragan Simic

Hello Anand,

Quoted 20 lines · Anand Moon

Add missing pinctrl settings for PCIe 3.0 x4 clock request and wake signals. Each component of PCIe communication have the following control
signals: PERST, WAKE, CLKREQ, and REFCLK. These signals work to generate
high-speed signals and communicate with other PCIe devices. Used by root complex to endpoint depending on the power state.

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.

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 8 lines
Quoted 2 lines

Rename node from 'pcie3' to 'pcie30x4' to align with schematic nomenclature.

I wonder why the three patches in this series cannot be squashed into a single patch, because they target the same thing for the same board dts file? I don't think that having these three separate patches may help with possible regression tracking in the future, for example.

Ok, I will merge this in a single patch.

Thanks
-Anand