* [PATCH v5] arm64: dts: rockchip: Add missing pinctrl wake and clkreq for PCIe node
@ 2024-07-29 12:37 Anand Moon
2024-08-09 9:59 ` Jonas Karlman
0 siblings, 1 reply; 4+ messages in thread
From: Anand Moon @ 2024-07-29 12:37 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Heiko Stuebner
Cc: Anand Moon, Jonas Karlman, Dragan Simic, devicetree,
linux-arm-kernel, linux-rockchip, linux-kernel
Add missing pinctrl settings WAKE and CLKREQ for PCIe 3.0 x4, PCIe 3.0 x1
and PCIe 2.1 x1 nodes. 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. L1 sub-states is providing a digital signal
(CLKREQ#) for PHYs to use to wake up and resume normal operation.
Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
v5: Merged all 3 patch into single patch, reabse on master
Fix the $subject and commit message.
Drop the RK_FUNC_GPIO for WAKE and CLKREQ as these seignal are
ment for was introduced to allow PCI Express devices to enter
even deeper power savings states (“L1.1” and “L1.2”) while still
appearing to legacy software to be in the “L1” state
---
.../boot/dts/rockchip/rk3588-rock-5b.dts | 46 +++++++++++++------
1 file changed, 33 insertions(+), 13 deletions(-)
diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index 966bbc582d89..a1e83546f1be 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -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";
@@ -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";
@@ -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";
@@ -363,28 +363,48 @@ hp_detect: hp-detect {
};
};
- pcie2 {
- pcie2_0_rst: pcie2-0-rst {
- rockchip,pins = <4 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
+ pcie20x1 {
+ pcie20x12_pins: pcie20x12-pins {
+ rockchip,pins =
+ /* PCIE20_1_2_CLKREQn_M1_L */
+ <3 RK_PC7 4 &pcfg_pull_up>,
+ /* PCIE_PERST_L */
+ <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_up>,
+ /* PCIE20_1_2_WAKEn_M1_L */
+ <3 RK_PD0 4 &pcfg_pull_up>;
};
+ };
+ 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 4 &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 4 &pcfg_pull_down>;
};
};
- 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 4 &pcfg_pull_up>,
+ /* PCIE30X4_PERSTn_M1_L */
+ <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>,
+ /* PCIE30X4_WAKEn_M1_L */
+ <4 RK_PB5 4 &pcfg_pull_down>;
+ };
};
usb {
base-commit: dc1c8034e31b14a2e5e212104ec508aec44ce1b9
--
2.44.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v5] arm64: dts: rockchip: Add missing pinctrl wake and clkreq for PCIe node
2024-07-29 12:37 [PATCH v5] arm64: dts: rockchip: Add missing pinctrl wake and clkreq for PCIe node Anand Moon
@ 2024-08-09 9:59 ` Jonas Karlman
2024-08-09 10:41 ` Anand Moon
0 siblings, 1 reply; 4+ messages in thread
From: Jonas Karlman @ 2024-08-09 9:59 UTC (permalink / raw)
To: Anand Moon, Heiko Stuebner
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Dragan Simic,
devicetree, linux-arm-kernel, linux-rockchip, linux-kernel
Hi Anand,
On 2024-07-29 14:37, Anand Moon wrote:
> Add missing pinctrl settings WAKE and CLKREQ for PCIe 3.0 x4, PCIe 3.0 x1
> and PCIe 2.1 x1 nodes. 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. L1 sub-states is providing a digital signal
> (CLKREQ#) for PHYs to use to wake up and resume normal operation.
>
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> v5: Merged all 3 patch into single patch, reabse on master
> Fix the $subject and commit message.
> Drop the RK_FUNC_GPIO for WAKE and CLKREQ as these seignal are
> ment for was introduced to allow PCI Express devices to enter
> even deeper power savings states (“L1.1” and “L1.2”) while still
> appearing to legacy software to be in the “L1” state
> ---
> .../boot/dts/rockchip/rk3588-rock-5b.dts | 46 +++++++++++++------
> 1 file changed, 33 insertions(+), 13 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index 966bbc582d89..a1e83546f1be 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -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";
> @@ -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";
> @@ -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";
> @@ -363,28 +363,48 @@ hp_detect: hp-detect {
> };
> };
>
> - pcie2 {
> - pcie2_0_rst: pcie2-0-rst {
> - rockchip,pins = <4 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
> + pcie20x1 {
> + pcie20x12_pins: pcie20x12-pins {
> + rockchip,pins =
> + /* PCIE20_1_2_CLKREQn_M1_L */
> + <3 RK_PC7 4 &pcfg_pull_up>,
> + /* PCIE_PERST_L */
> + <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_up>,
> + /* PCIE20_1_2_WAKEn_M1_L */
> + <3 RK_PD0 4 &pcfg_pull_up>;
Some unanswered questions from v4:
How come you use internal pull-up/down on these pins?
And why do they differ for each pcie node in this series?
Regards,
Jonas
> };
> + };
>
> + 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 4 &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 4 &pcfg_pull_down>;
> };
> };
>
> - 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 4 &pcfg_pull_up>,
> + /* PCIE30X4_PERSTn_M1_L */
> + <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>,
> + /* PCIE30X4_WAKEn_M1_L */
> + <4 RK_PB5 4 &pcfg_pull_down>;
> + };
> };
>
> usb {
>
> base-commit: dc1c8034e31b14a2e5e212104ec508aec44ce1b9
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] arm64: dts: rockchip: Add missing pinctrl wake and clkreq for PCIe node
2024-08-09 9:59 ` Jonas Karlman
@ 2024-08-09 10:41 ` Anand Moon
2024-08-09 11:35 ` Jonas Karlman
0 siblings, 1 reply; 4+ messages in thread
From: Anand Moon @ 2024-08-09 10:41 UTC (permalink / raw)
To: Jonas Karlman
Cc: Heiko Stuebner, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Dragan Simic, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
Hi Jonas
On Fri, 9 Aug 2024 at 15:29, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Anand,
>
> On 2024-07-29 14:37, Anand Moon wrote:
> > Add missing pinctrl settings WAKE and CLKREQ for PCIe 3.0 x4, PCIe 3.0 x1
> > and PCIe 2.1 x1 nodes. 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. L1 sub-states is providing a digital signal
> > (CLKREQ#) for PHYs to use to wake up and resume normal operation.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > v5: Merged all 3 patch into single patch, reabse on master
> > Fix the $subject and commit message.
> > Drop the RK_FUNC_GPIO for WAKE and CLKREQ as these seignal are
> > ment for was introduced to allow PCI Express devices to enter
> > even deeper power savings states (“L1.1” and “L1.2”) while still
> > appearing to legacy software to be in the “L1” state
> > ---
> > .../boot/dts/rockchip/rk3588-rock-5b.dts | 46 +++++++++++++------
> > 1 file changed, 33 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > index 966bbc582d89..a1e83546f1be 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -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";
> > @@ -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";
> > @@ -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";
> > @@ -363,28 +363,48 @@ hp_detect: hp-detect {
> > };
> > };
> >
> > - pcie2 {
> > - pcie2_0_rst: pcie2-0-rst {
> > - rockchip,pins = <4 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
> > + pcie20x1 {
> > + pcie20x12_pins: pcie20x12-pins {
> > + rockchip,pins =
> > + /* PCIE20_1_2_CLKREQn_M1_L */
> > + <3 RK_PC7 4 &pcfg_pull_up>,
> > + /* PCIE_PERST_L */
> > + <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_up>,
> > + /* PCIE20_1_2_WAKEn_M1_L */
> > + <3 RK_PD0 4 &pcfg_pull_up>;
>
> Some unanswered questions from v4:
>
> How come you use internal pull-up/down on these pins?
As per the schematic radxa_rock5b_v13_sch.pdf [1] pin description
GPIO3_B0_u pin for PCIE_PERST_L (pcfg_pull_up)
GPIO3_D0_u pin for PCIE20_1_2_WAKEn_M1_L (pcfg_pull_up)
GPIO4_A4_d pin for PCIE30x1_0_WAKEn_M1_L (pcfg_pull_down)
[1] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock5b_v13_sch.pdf
> And why do they differ for each pcie node in this series?
It also depends on how the pins are defined in the schematics.
I have not made many changes in this series combined in a single patch.
>
> Regards,
> Jonas
Thanks
-Anand
>
> > };
> > + };
> >
> > + 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 4 &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 4 &pcfg_pull_down>;
> > };
> > };
> >
> > - 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 4 &pcfg_pull_up>,
> > + /* PCIE30X4_PERSTn_M1_L */
> > + <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>,
> > + /* PCIE30X4_WAKEn_M1_L */
> > + <4 RK_PB5 4 &pcfg_pull_down>;
> > + };
> > };
> >
> > usb {
> >
> > base-commit: dc1c8034e31b14a2e5e212104ec508aec44ce1b9
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] arm64: dts: rockchip: Add missing pinctrl wake and clkreq for PCIe node
2024-08-09 10:41 ` Anand Moon
@ 2024-08-09 11:35 ` Jonas Karlman
0 siblings, 0 replies; 4+ messages in thread
From: Jonas Karlman @ 2024-08-09 11:35 UTC (permalink / raw)
To: Anand Moon
Cc: Heiko Stuebner, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Dragan Simic, devicetree, linux-arm-kernel, linux-rockchip,
linux-kernel
Hi Anand,
On 2024-08-09 12:41, Anand Moon wrote:
> Hi Jonas
>
> On Fri, 9 Aug 2024 at 15:29, Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> Hi Anand,
>>
>> On 2024-07-29 14:37, Anand Moon wrote:
>>> Add missing pinctrl settings WAKE and CLKREQ for PCIe 3.0 x4, PCIe 3.0 x1
>>> and PCIe 2.1 x1 nodes. 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. L1 sub-states is providing a digital signal
>>> (CLKREQ#) for PHYs to use to wake up and resume normal operation.
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>> v5: Merged all 3 patch into single patch, reabse on master
>>> Fix the $subject and commit message.
>>> Drop the RK_FUNC_GPIO for WAKE and CLKREQ as these seignal are
>>> ment for was introduced to allow PCI Express devices to enter
>>> even deeper power savings states (“L1.1” and “L1.2”) while still
>>> appearing to legacy software to be in the “L1” state
>>> ---
>>> .../boot/dts/rockchip/rk3588-rock-5b.dts | 46 +++++++++++++------
>>> 1 file changed, 33 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> index 966bbc582d89..a1e83546f1be 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
>>> @@ -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";
>>> @@ -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";
>>> @@ -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";
>>> @@ -363,28 +363,48 @@ hp_detect: hp-detect {
>>> };
>>> };
>>>
>>> - pcie2 {
>>> - pcie2_0_rst: pcie2-0-rst {
>>> - rockchip,pins = <4 RK_PA5 RK_FUNC_GPIO &pcfg_pull_none>;
>>> + pcie20x1 {
>>> + pcie20x12_pins: pcie20x12-pins {
>>> + rockchip,pins =
>>> + /* PCIE20_1_2_CLKREQn_M1_L */
>>> + <3 RK_PC7 4 &pcfg_pull_up>,
>>> + /* PCIE_PERST_L */
>>> + <3 RK_PB0 RK_FUNC_GPIO &pcfg_pull_up>,
>>> + /* PCIE20_1_2_WAKEn_M1_L */
>>> + <3 RK_PD0 4 &pcfg_pull_up>;
>>
>> Some unanswered questions from v4:
>>
>> How come you use internal pull-up/down on these pins?
>
> As per the schematic radxa_rock5b_v13_sch.pdf [1] pin description
>
> GPIO3_B0_u pin for PCIE_PERST_L (pcfg_pull_up)
> GPIO3_D0_u pin for PCIE20_1_2_WAKEn_M1_L (pcfg_pull_up)
> GPIO4_A4_d pin for PCIE30x1_0_WAKEn_M1_L (pcfg_pull_down)
The highlighting of GPIO3_B0_u etc typically only refers to the default
value at SoC reset, and the pinctrl should describe how the pin should
be configured, not the default value (unless it happens to match).
The schematic tells us that the GPIO3_B0 pin is used for PERST# signal,
your description of this signal tells us that the PERST# should be held
low and a transition from low to high in this signal usually indicates
the beginning of link initialization, so stand to reason that an
internal or external pull-down may be used on a PERST# signal.
Similarly the WAKE# and CLKREQ# signal is an active-low signal, so
use of internal or external pull-up may be correct.
However the mixed use of bias-pull-up/down as pinconf for a pin used
for same signal on different pcie controllers does not make that much
sense to me.
pcie2x1l0 WAKE# use pcfg_pull_down (bias-pull-down)
pcie2x1l2 WAKE# use pcfg_pull_up (bias-pull-up)
pcie3x4 WAKE# use pcfg_pull_down (bias-pull-down)
And similar it is mixed for CLKREQ# and PERST# across the different pcie
controllers.
I am no expert in this area but this mixed bias pinconf of these signals
look strange to me.
>
> [1] https://dl.radxa.com/rock5/5b/docs/hw/radxa_rock5b_v13_sch.pdf
>
>> And why do they differ for each pcie node in this series?
>
> It also depends on how the pins are defined in the schematics.
> I have not made many changes in this series combined in a single patch.
The internal pull is different compared to how the default pcie pins is
configured from rk3588 pinctrl.dtsi, where they are all bias-disable.
And my wondering is why this deviate from the use of pcfg_pull_none from
those existing pcie pins pin groups?
Regards,
Jonas
>
>>
>> Regards,
>> Jonas
>
> Thanks
> -Anand
>>
>>> };
>>> + };
>>>
>>> + 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 4 &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 4 &pcfg_pull_down>;
>>> };
>>> };
>>>
>>> - 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 4 &pcfg_pull_up>,
>>> + /* PCIE30X4_PERSTn_M1_L */
>>> + <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>,
>>> + /* PCIE30X4_WAKEn_M1_L */
>>> + <4 RK_PB5 4 &pcfg_pull_down>;
>>> + };
>>> };
>>>
>>> usb {
>>>
>>> base-commit: dc1c8034e31b14a2e5e212104ec508aec44ce1b9
>>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-08-09 11:36 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-07-29 12:37 [PATCH v5] arm64: dts: rockchip: Add missing pinctrl wake and clkreq for PCIe node Anand Moon
2024-08-09 9:59 ` Jonas Karlman
2024-08-09 10:41 ` Anand Moon
2024-08-09 11:35 ` Jonas Karlman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).