diff mbox series

[v1] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

Message ID 20240710142001.7234-1-linux.amoon@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v1] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node | expand

Commit Message

Anand Moon July 10, 2024, 2:19 p.m. UTC 
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 <linux.amoon@gmail.com>
---
 .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)


base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905

Comments

Jonas Karlman July 10, 2024, 2:41 p.m. UTC | #1 
Hi Anand,

On 2024-07-10 16:19, Anand Moon wrote:
> 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 <linux.amoon@gmail.com>
> ---
>  .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index 2e7512676b7e..a9b55b7996cf 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -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>;
> +		};

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

> +
> +		pcie30x4_perstn_m1: pcie30x4-perstn-m1 {
> +			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_down>;
> +		};
>  	};
>  
>  	usb {
> 
> base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
Heiko Stübner July 10, 2024, 2:54 p.m. UTC | #2 
Hi,

Am Mittwoch, 10. Juli 2024, 16:19:56 CEST schrieb 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 <linux.amoon@gmail.com>
> ---
>  .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> index 2e7512676b7e..a9b55b7996cf 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> @@ -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>;
> +		};

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
Anand Moon July 10, 2024, 4:30 p.m. UTC | #3 
Hi Heiko,

Thanks for your review comments.

On Wed, 10 Jul 2024 at 20:24, Heiko Stübner <heiko@sntech.de> wrote:
>
> Hi,
>
> Am Mittwoch, 10. Juli 2024, 16:19:56 CEST schrieb 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 <linux.amoon@gmail.com>
> > ---
> >  .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > index 2e7512676b7e..a9b55b7996cf 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -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>;
> > +             };
>
> 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.

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

> 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.
>
I will work on and update in the next version.
>
> Heiko

Thanks
-Anand
Anand Moon July 10, 2024, 4:34 p.m. UTC | #4 
Hi Jonas,

On Wed, 10 Jul 2024 at 20:11, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Anand,
>
> On 2024-07-10 16:19, Anand Moon wrote:
> > 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 <linux.amoon@gmail.com>
> > ---
> >  .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
> >  1 file changed, 14 insertions(+), 6 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > index 2e7512676b7e..a9b55b7996cf 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > @@ -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>;
> > +             };
>
> 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.
>

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

> Regards,
> Jonas
>
Thanks
-Anand
Anand Moon July 11, 2024, 4:39 a.m. UTC | #5 
Hi Jonas,

On Wed, 10 Jul 2024 at 22:04, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Jonas,
>
> On Wed, 10 Jul 2024 at 20:11, Jonas Karlman <jonas@kwiboo.se> wrote:
> >
> > Hi Anand,
> >
> > On 2024-07-10 16:19, Anand Moon wrote:
> > > 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 <linux.amoon@gmail.com>
> > > ---
> > >  .../boot/dts/rockchip/rk3588-rock-5b.dts      | 20 +++++++++++++------
> > >  1 file changed, 14 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > index 2e7512676b7e..a9b55b7996cf 100644
> > > --- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > +++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
> > > @@ -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>;
> > > +             };
> >
> > 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.
> >
>
> 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
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
index 2e7512676b7e..a9b55b7996cf 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts
@@ -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 {