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

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

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

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
V2: Update the commit messge to describe the changs.
    use pinctl group as its pre define in pinctl dtsi
---
 arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
 1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
 	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
 	vpcie3v3-supply = <&vcc3v3_pcie30>;
 	status = "okay";
@@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
 	};
 
 	pcie3 {
-		pcie3_rst: pcie3-rst {
-			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
-		};
-
 		pcie3_vcc3v3_en: pcie3-vcc3v3-en {
 			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
 		};

base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
-- 
2.44.0

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Jonas Karlman]

Hi Anand,

On 2024-07-11 08:09, Anand Moon wrote:
> 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.
> 
> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> ---
> V2: Update the commit messge to describe the changs.
>     use pinctl group as its pre define in pinctl dtsi
> ---
>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
>  1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;

Use of the existing pcie30x4m1_pins group may not be fully accurate for
the PERST pin. The use of reset-gpios indicate that the PERST pin is
used with GPIO function and the driver will implicitly change the
function from perstn_m1 to GPIO. So this may not be best representation
of the hw, hence my initial suggestion, something like:

	pcie30x4_pins: pcie30x4-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>;
	};

Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
not just pcie3x4.

Regards,
Jonas

>  	reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
>  	vpcie3v3-supply = <&vcc3v3_pcie30>;
>  	status = "okay";
> @@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
>  	};
>  
>  	pcie3 {
> -		pcie3_rst: pcie3-rst {
> -			rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
> -		};
> -
>  		pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>  			rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
>  		};
> 
> base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Anand Moon]

Hi Jonas,

Thanks for your review comments.

On Thu, 11 Jul 2024 at 14:13, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Anand,
>
> On 2024-07-11 08:09, Anand Moon wrote:
> > 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.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > V2: Update the commit messge to describe the changs.
> >     use pinctl group as its pre define in pinctl dtsi
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
> >  1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
>
> Use of the existing pcie30x4m1_pins group may not be fully accurate for
> the PERST pin. The use of reset-gpios indicate that the PERST pin is
> used with GPIO function and the driver will implicitly change the
> function from perstn_m1 to GPIO. So this may not be best representation
> of the hw, hence my initial suggestion, something like:
>
>         pcie30x4_pins: pcie30x4-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>;
>         };
>
> Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
> not just pcie3x4.
>

Ok but it is better to update this in rk3588s-pinctrl.dtsi otherwise
the pcie30x4m1_pins
will not be used at all on all the boards
I will update the PERST pin to RK_FUNC_GPIO on all the pcie2x1l0,
pcie2x1l2 and  pcie30x4
is this ok for you?

> Regards,
> Jonas

Thanks
-Anand
>
> >       reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
> >       vpcie3v3-supply = <&vcc3v3_pcie30>;
> >       status = "okay";
> > @@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
> >       };
> >
> >       pcie3 {
> > -             pcie3_rst: pcie3-rst {
> > -                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
> > -             };
> > -
> >               pcie3_vcc3v3_en: pcie3-vcc3v3-en {
> >                       rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
> >               };
> >
> > base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
>

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Jonas Karlman]

Hi Anand,

On 2024-07-11 11:09, Anand Moon wrote:
> Hi Jonas,
> 
> Thanks for your review comments.
> 
> On Thu, 11 Jul 2024 at 14:13, Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> Hi Anand,
>>
>> On 2024-07-11 08:09, Anand Moon wrote:
>>> 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.
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>> V2: Update the commit messge to describe the changs.
>>>     use pinctl group as its pre define in pinctl dtsi
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
>>>  1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
>>
>> Use of the existing pcie30x4m1_pins group may not be fully accurate for
>> the PERST pin. The use of reset-gpios indicate that the PERST pin is
>> used with GPIO function and the driver will implicitly change the
>> function from perstn_m1 to GPIO. So this may not be best representation
>> of the hw, hence my initial suggestion, something like:
>>
>>         pcie30x4_pins: pcie30x4-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>;
>>         };
>>
>> Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
>> not just pcie3x4.
>>
> 
> Ok but it is better to update this in rk3588s-pinctrl.dtsi otherwise
> the pcie30x4m1_pins
> will not be used at all on all the boards

I agree that it is unfortunate that the PERST pin is included in the
groups in pinctrl.dtsi, however, for pcie ep mode the pins should more
than likely be using the perstn_m1 function and not GPIO, so there are
uses for the existing pin groups for ep mode.

In my opinion using pcie30x4m1_pins as-is and having implicitly changing
to GPIO function when driver/usage in the software require it is an okay
description of the hw.

However, seeing how use of pcie ep is described using its own node in DT
I can understand that for rc mode the pin should be changed to GPIO to
properly describe the expected usage.

When I tried to add similar implicit change to use GPIO function in
U-Boot to fix system freeze when improper pinctrl was used, it resulted
in some discussions, see [1] ;-)

[1] https://lore.kernel.org/u-boot/20240511112821.1156519-1-jonas@kwiboo.se/t/#u

> I will update the PERST pin to RK_FUNC_GPIO on all the pcie2x1l0,
> pcie2x1l2 and  pcie30x4
> is this ok for you?

This should probably be reworked in pinctrl.dtsi together with all
affected boards and/or use a group override in the board dts.

Regards,
Jonas

> 
>> Regards,
>> Jonas
> 
> Thanks
> -Anand
>>
>>>       reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
>>>       vpcie3v3-supply = <&vcc3v3_pcie30>;
>>>       status = "okay";
>>> @@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
>>>       };
>>>
>>>       pcie3 {
>>> -             pcie3_rst: pcie3-rst {
>>> -                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
>>> -             };
>>> -
>>>               pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>>>                       rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
>>>               };
>>>
>>> base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
>>

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Anand Moon]

Hi Jonas

On Thu, 11 Jul 2024 at 15:15, Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Anand,
>
> On 2024-07-11 11:09, Anand Moon wrote:
> > Hi Jonas,
> >
> > Thanks for your review comments.
> >
> > On Thu, 11 Jul 2024 at 14:13, Jonas Karlman <jonas@kwiboo.se> wrote:
> >>
> >> Hi Anand,
> >>
> >> On 2024-07-11 08:09, Anand Moon wrote:
> >>> 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.
> >>>
> >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>> ---
> >>> V2: Update the commit messge to describe the changs.
> >>>     use pinctl group as its pre define in pinctl dtsi
> >>> ---
> >>>  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
> >>>  1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
> >>
> >> Use of the existing pcie30x4m1_pins group may not be fully accurate for
> >> the PERST pin. The use of reset-gpios indicate that the PERST pin is
> >> used with GPIO function and the driver will implicitly change the
> >> function from perstn_m1 to GPIO. So this may not be best representation
> >> of the hw, hence my initial suggestion, something like:
> >>
> >>         pcie30x4_pins: pcie30x4-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>;
> >>         };
> >>
> >> Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
> >> not just pcie3x4.
> >>
> >
> > Ok but it is better to update this in rk3588s-pinctrl.dtsi otherwise
> > the pcie30x4m1_pins
> > will not be used at all on all the boards
>
> I agree that it is unfortunate that the PERST pin is included in the
> groups in pinctrl.dtsi, however, for pcie ep mode the pins should more
> than likely be using the perstn_m1 function and not GPIO, so there are
> uses for the existing pin groups for ep mode.
>
> In my opinion using pcie30x4m1_pins as-is and having implicitly changing
> to GPIO function when driver/usage in the software require it is an okay
> description of the hw.
>
> However, seeing how use of pcie ep is described using its own node in DT
> I can understand that for rc mode the pin should be changed to GPIO to
> properly describe the expected usage.
>

Thanks for your explanation.

> When I tried to add similar implicit change to use GPIO function in
> U-Boot to fix system freeze when improper pinctrl was used, it resulted
> in some discussions, see [1] ;-)
>
> [1] https://lore.kernel.org/u-boot/20240511112821.1156519-1-jonas@kwiboo.se/t/#u
>
Ok, I get this.

> > I will update the PERST pin to RK_FUNC_GPIO on all the pcie2x1l0,
> > pcie2x1l2 and  pcie30x4
> > is this ok for you?
>
> This should probably be reworked in pinctrl.dtsi together with all
> affected boards and/or use a group override in the board dts.

I will keep these board-specific pinctl changes.

>
> Regards,
> Jonas
>

Thanks
-Anand

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Kever Yang]

Hi Anand, Jonas,

On 2024/7/11 17:44, Jonas Karlman wrote:
> Hi Anand,
>
> On 2024-07-11 11:09, Anand Moon wrote:
>> Hi Jonas,
>>
>> Thanks for your review comments.
>>
>> On Thu, 11 Jul 2024 at 14:13, Jonas Karlman<jonas@kwiboo.se>  wrote:
>>> Hi Anand,
>>>
>>> On 2024-07-11 08:09, Anand Moon wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Anand Moon<linux.amoon@gmail.com>
>>>> ---
>>>> V2: Update the commit messge to describe the changs.
>>>>      use pinctl group as its pre define in pinctl dtsi
>>>> ---
>>>>   arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
>>>>   1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
>>> Use of the existing pcie30x4m1_pins group may not be fully accurate for
>>> the PERST pin. The use of reset-gpios indicate that the PERST pin is
>>> used with GPIO function and the driver will implicitly change the
>>> function from perstn_m1 to GPIO. So this may not be best representation
>>> of the hw, hence my initial suggestion, something like:
>>>
>>>          pcie30x4_pins: pcie30x4-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>;
>>>          };
>>>
>>> Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
>>> not just pcie3x4.
>>>
>> Ok but it is better to update this in rk3588s-pinctrl.dtsi otherwise
>> the pcie30x4m1_pins
>> will not be used at all on all the boards
> I agree that it is unfortunate that the PERST pin is included in the
> groups in pinctrl.dtsi, however, for pcie ep mode the pins should more
> than likely be using the perstn_m1 function and not GPIO, so there are
> uses for the existing pin groups for ep mode.
Please note that the pcie30x4m1_pins define in pinctrl.dtsi is somehow 
not correct due to the TRM.
- The WAKE should always work in GPIO mode, this is totally support by 
SoC, not PCIe controller;
- The PERST should always work in GPIO mode in PCIe RC mode; and for all 
the rk3588 EP mode
hardware I have seem, the PERST from RC connect in to board POR instead 
of SoC PERST;
The PCIe in most of boards works in RC mode, so we should make PERST 
default as GPIO;
- CLKREQ, it depends on boards, if the board need to support L1SS or 
not, most of the boards
does not support L1SS, which means CLKREQ works in GPIO mode instead of 
PCIe function.

Thanks,
- Kever
> In my opinion using pcie30x4m1_pins as-is and having implicitly changing
> to GPIO function when driver/usage in the software require it is an okay
> description of the hw.
>
> However, seeing how use of pcie ep is described using its own node in DT
> I can understand that for rc mode the pin should be changed to GPIO to
> properly describe the expected usage.
>
> When I tried to add similar implicit change to use GPIO function in
> U-Boot to fix system freeze when improper pinctrl was used, it resulted
> in some discussions, see [1] ;-)
>
> [1]https://lore.kernel.org/u-boot/20240511112821.1156519-1-jonas@kwiboo.se/t/#u
>
>> I will update the PERST pin to RK_FUNC_GPIO on all the pcie2x1l0,
>> pcie2x1l2 and  pcie30x4
>> is this ok for you?
> This should probably be reworked in pinctrl.dtsi together with all
> affected boards and/or use a group override in the board dts.
>
> Regards,
> Jonas
>
>>> Regards,
>>> Jonas
>> Thanks
>> -Anand
>>>>        reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
>>>>        vpcie3v3-supply = <&vcc3v3_pcie30>;
>>>>        status = "okay";
>>>> @@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
>>>>        };
>>>>
>>>>        pcie3 {
>>>> -             pcie3_rst: pcie3-rst {
>>>> -                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
>>>> -             };
>>>> -
>>>>                pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>>>>                        rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
>>>>                };
>>>>
>>>> base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Anand Moon]

Hi Kever, Jonas.

On Thu, 1 Aug 2024 at 07:53, Kever Yang <kever.yang@rock-chips.com> wrote:
>
> Hi Anand, Jonas,
>
> On 2024/7/11 17:44, Jonas Karlman wrote:
> > Hi Anand,
> >
> > On 2024-07-11 11:09, Anand Moon wrote:
> >> Hi Jonas,
> >>
> >> Thanks for your review comments.
> >>
> >> On Thu, 11 Jul 2024 at 14:13, Jonas Karlman<jonas@kwiboo.se>  wrote:
> >>> Hi Anand,
> >>>
> >>> On 2024-07-11 08:09, Anand Moon wrote:
> >>>> 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.
> >>>>
> >>>> Signed-off-by: Anand Moon<linux.amoon@gmail.com>
> >>>> ---
> >>>> V2: Update the commit messge to describe the changs.
> >>>>      use pinctl group as its pre define in pinctl dtsi
> >>>> ---
> >>>>   arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
> >>>>   1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
> >>> Use of the existing pcie30x4m1_pins group may not be fully accurate for
> >>> the PERST pin. The use of reset-gpios indicate that the PERST pin is
> >>> used with GPIO function and the driver will implicitly change the
> >>> function from perstn_m1 to GPIO. So this may not be best representation
> >>> of the hw, hence my initial suggestion, something like:
> >>>
> >>>          pcie30x4_pins: pcie30x4-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>;
> >>>          };
> >>>
> >>> Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
> >>> not just pcie3x4.
> >>>
> >> Ok but it is better to update this in rk3588s-pinctrl.dtsi otherwise
> >> the pcie30x4m1_pins
> >> will not be used at all on all the boards
> > I agree that it is unfortunate that the PERST pin is included in the
> > groups in pinctrl.dtsi, however, for pcie ep mode the pins should more
> > than likely be using the perstn_m1 function and not GPIO, so there are
> > uses for the existing pin groups for ep mode.
> Please note that the pcie30x4m1_pins define in pinctrl.dtsi is somehow
> not correct due to the TRM.
> - The WAKE should always work in GPIO mode, this is totally support by
> SoC, not PCIe controller;
> - The PERST should always work in GPIO mode in PCIe RC mode; and for all
> the rk3588 EP mode
> hardware I have seem, the PERST from RC connect in to board POR instead
> of SoC PERST;
> The PCIe in most of boards works in RC mode, so we should make PERST
> default as GPIO;
> - CLKREQ, it depends on boards, if the board need to support L1SS or
> not, most of the boards
> does not support L1SS, which means CLKREQ works in GPIO mode instead of
> PCIe function.
>

As per the PCIe hardware pin description below,
it seems that PCIE_CLKREQN is NC (Not connected).

[1] https://docs.radxa.com/en/rock5/rock5b/hardware-design/hardware-interface#pcie

Thanks
-Anand

> Thanks,
> - Kever
> > In my opinion using pcie30x4m1_pins as-is and having implicitly changing
> > to GPIO function when driver/usage in the software require it is an okay
> > description of the hw.
> >
> > However, seeing how use of pcie ep is described using its own node in DT
> > I can understand that for rc mode the pin should be changed to GPIO to
> > properly describe the expected usage.
> >
> > When I tried to add similar implicit change to use GPIO function in
> > U-Boot to fix system freeze when improper pinctrl was used, it resulted
> > in some discussions, see [1] ;-)
> >
> > [1]https://lore.kernel.org/u-boot/20240511112821.1156519-1-jonas@kwiboo.se/t/#u
> >
> >> I will update the PERST pin to RK_FUNC_GPIO on all the pcie2x1l0,
> >> pcie2x1l2 and  pcie30x4
> >> is this ok for you?
> > This should probably be reworked in pinctrl.dtsi together with all
> > affected boards and/or use a group override in the board dts.
> >
> > Regards,
> > Jonas
> >
> >>> Regards,
> >>> Jonas
> >> Thanks
> >> -Anand
> >>>>        reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
> >>>>        vpcie3v3-supply = <&vcc3v3_pcie30>;
> >>>>        status = "okay";
> >>>> @@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
> >>>>        };
> >>>>
> >>>>        pcie3 {
> >>>> -             pcie3_rst: pcie3-rst {
> >>>> -                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
> >>>> -             };
> >>>> -
> >>>>                pcie3_vcc3v3_en: pcie3-vcc3v3-en {
> >>>>                        rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
> >>>>                };
> >>>>
> >>>> base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 255 bytes --]

On Thursday, 11 July 2024 11:09:48 CEST Anand Moon wrote:
> Ok but it is better to update this in rk3588s-pinctrl.dtsi

It's probably better to base your patch(es) on Heiko's 'for-next' (or 'v6.11-
armsoc/dts64') branch as that file no longer exists there

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Anand Moon]

Hi Diederik,

On Thu, 11 Jul 2024 at 15:35, Diederik de Haas <didi.debian@cknow.org> wrote:
>
> On Thursday, 11 July 2024 11:09:48 CEST Anand Moon wrote:
> > Ok but it is better to update this in rk3588s-pinctrl.dtsi
>
> It's probably better to base your patch(es) on Heiko's 'for-next' (or 'v6.11-
> armsoc/dts64') branch as that file no longer exists there

Ok.

Thanks
-Anand

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Chen-Yu Tsai]

On Thu, Jul 11, 2024 at 4:44 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>
> Hi Anand,
>
> On 2024-07-11 08:09, Anand Moon wrote:
> > 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.
> >
> > Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> > ---
> > V2: Update the commit messge to describe the changs.
> >     use pinctl group as its pre define in pinctl dtsi
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
> >  1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
>
> Use of the existing pcie30x4m1_pins group may not be fully accurate for
> the PERST pin. The use of reset-gpios indicate that the PERST pin is
> used with GPIO function and the driver will implicitly change the
> function from perstn_m1 to GPIO. So this may not be best representation
> of the hw, hence my initial suggestion, something like:
>
>         pcie30x4_pins: pcie30x4-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>;
>         };
>
> Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
> not just pcie3x4.

Can we consider implementing strict mode in the pinctrl driver so we don't
have to keep doing this GPIO + pinmux dance?

ChenYu


> Regards,
> Jonas
>
> >       reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
> >       vpcie3v3-supply = <&vcc3v3_pcie30>;
> >       status = "okay";
> > @@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
> >       };
> >
> >       pcie3 {
> > -             pcie3_rst: pcie3-rst {
> > -                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
> > -             };
> > -
> >               pcie3_vcc3v3_en: pcie3-vcc3v3-en {
> >                       rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
> >               };
> >
> > base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
>
>

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Alex Bee]

Am 11.07.24 um 11:17 schrieb Chen-Yu Tsai:
> On Thu, Jul 11, 2024 at 4:44 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>>
>> Hi Anand,
>>
>> On 2024-07-11 08:09, Anand Moon wrote:
>>> 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.
>>>
>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>> ---
>>> V2: Update the commit messge to describe the changs.
>>>      use pinctl group as its pre define in pinctl dtsi
>>> ---
>>>   arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
>>>   1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
>>
>> Use of the existing pcie30x4m1_pins group may not be fully accurate for
>> the PERST pin. The use of reset-gpios indicate that the PERST pin is
>> used with GPIO function and the driver will implicitly change the
>> function from perstn_m1 to GPIO. So this may not be best representation
>> of the hw, hence my initial suggestion, something like:
>>
>>          pcie30x4_pins: pcie30x4-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>;
>>          };
>>
>> Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
>> not just pcie3x4.
> 
> Can we consider implementing strict mode in the pinctrl driver so we don't
> have to keep doing this GPIO + pinmux dance?
> 

This is not about drivers, this is DT which is independent from drivers.
Jonas and I had discussion recently on u-boot mailing list with somebody
using Open/FreeBSD about exact that topic. Pinctrl mux settings should be
very explicit, even if it is not required by the linux-driver.

Alex

> ChenYu
> 
> 
>> Regards,
>> Jonas
>>
>>>        reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
>>>        vpcie3v3-supply = <&vcc3v3_pcie30>;
>>>        status = "okay";
>>> @@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
>>>        };
>>>
>>>        pcie3 {
>>> -             pcie3_rst: pcie3-rst {
>>> -                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
>>> -             };
>>> -
>>>                pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>>>                        rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
>>>                };
>>>
>>> base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
>>
>>
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Chen-Yu Tsai]

On Thu, Jul 11, 2024 at 7:13 PM Alex Bee <knaerzche@gmail.com> wrote:
>
> Am 11.07.24 um 11:17 schrieb Chen-Yu Tsai:
> > On Thu, Jul 11, 2024 at 4:44 PM Jonas Karlman <jonas@kwiboo.se> wrote:
> >>
> >> Hi Anand,
> >>
> >> On 2024-07-11 08:09, Anand Moon wrote:
> >>> 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.
> >>>
> >>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
> >>> ---
> >>> V2: Update the commit messge to describe the changs.
> >>>      use pinctl group as its pre define in pinctl dtsi
> >>> ---
> >>>   arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
> >>>   1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
> >>
> >> Use of the existing pcie30x4m1_pins group may not be fully accurate for
> >> the PERST pin. The use of reset-gpios indicate that the PERST pin is
> >> used with GPIO function and the driver will implicitly change the
> >> function from perstn_m1 to GPIO. So this may not be best representation
> >> of the hw, hence my initial suggestion, something like:
> >>
> >>          pcie30x4_pins: pcie30x4-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>;
> >>          };
> >>
> >> Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
> >> not just pcie3x4.
> >
> > Can we consider implementing strict mode in the pinctrl driver so we don't
> > have to keep doing this GPIO + pinmux dance?
> >
>
> This is not about drivers, this is DT which is independent from drivers.
> Jonas and I had discussion recently on u-boot mailing list with somebody
> using Open/FreeBSD about exact that topic. Pinctrl mux settings should be
> very explicit, even if it is not required by the linux-driver.

The hardware also prevents you from using GPIOs with any other mux setting.
That is already implied by the compatible string and doesn't need to be
spelled out. The strict driver mode merely enforces this hardware limitation
without the very explicit setting. One could also say that the driver is
wrong to support conflicting settings.


ChenYu

> Alex
>
> > ChenYu
> >
> >
> >> Regards,
> >> Jonas
> >>
> >>>        reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
> >>>        vpcie3v3-supply = <&vcc3v3_pcie30>;
> >>>        status = "okay";
> >>> @@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
> >>>        };
> >>>
> >>>        pcie3 {
> >>> -             pcie3_rst: pcie3-rst {
> >>> -                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
> >>> -             };
> >>> -
> >>>                pcie3_vcc3v3_en: pcie3-vcc3v3-en {
> >>>                        rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
> >>>                };
> >>>
> >>> base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
> >>
> >>
> >
> > _______________________________________________
> > Linux-rockchip mailing list
> > Linux-rockchip@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip
>

Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

[Alex Bee]

Am 11.07.24 um 13:38 schrieb Chen-Yu Tsai:
> On Thu, Jul 11, 2024 at 7:13 PM Alex Bee <knaerzche@gmail.com> wrote:
>> Am 11.07.24 um 11:17 schrieb Chen-Yu Tsai:
>>> On Thu, Jul 11, 2024 at 4:44 PM Jonas Karlman <jonas@kwiboo.se> wrote:
>>>> Hi Anand,
>>>>
>>>> On 2024-07-11 08:09, Anand Moon wrote:
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Anand Moon <linux.amoon@gmail.com>
>>>>> ---
>>>>> V2: Update the commit messge to describe the changs.
>>>>>       use pinctl group as its pre define in pinctl dtsi
>>>>> ---
>>>>>    arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dts | 6 +-----
>>>>>    1 file changed, 1 insertion(+), 5 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..ab3a20986c6a 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 = <&pcie30x4m1_pins>;
>>>> Use of the existing pcie30x4m1_pins group may not be fully accurate for
>>>> the PERST pin. The use of reset-gpios indicate that the PERST pin is
>>>> used with GPIO function and the driver will implicitly change the
>>>> function from perstn_m1 to GPIO. So this may not be best representation
>>>> of the hw, hence my initial suggestion, something like:
>>>>
>>>>           pcie30x4_pins: pcie30x4-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>;
>>>>           };
>>>>
>>>> Similar change should probably also be done for pcie2x1l0 and pcie2x1l2,
>>>> not just pcie3x4.
>>> Can we consider implementing strict mode in the pinctrl driver so we don't
>>> have to keep doing this GPIO + pinmux dance?
>>>
>> This is not about drivers, this is DT which is independent from drivers.
>> Jonas and I had discussion recently on u-boot mailing list with somebody
>> using Open/FreeBSD about exact that topic. Pinctrl mux settings should be
>> very explicit, even if it is not required by the linux-driver.
> The hardware also prevents you from using GPIOs with any other mux setting.
> That is already implied by the compatible string and doesn't need to be
> spelled out. The strict driver mode merely enforces this hardware limitation
> without the very explicit setting. One could also say that the driver is
> wrong to support conflicting settings.
>
>
> ChenYu
"The" driver is in fact not correct here, since it's two actually two
drivers: pinctrl and gpio, two subsystems and separate HW blocks (not very
clear, but it's at least configured in different mmio regions). So with
what you are suggesting you want one driver to implicitly configure the
other and expect every OS to have such methods? Broad ...

Alex

>
>> Alex
>>
>>> ChenYu
>>>
>>>
>>>> Regards,
>>>> Jonas
>>>>
>>>>>         reset-gpios = <&gpio4 RK_PB6 GPIO_ACTIVE_HIGH>;
>>>>>         vpcie3v3-supply = <&vcc3v3_pcie30>;
>>>>>         status = "okay";
>>>>> @@ -341,10 +341,6 @@ pcie2_2_rst: pcie2-2-rst {
>>>>>         };
>>>>>
>>>>>         pcie3 {
>>>>> -             pcie3_rst: pcie3-rst {
>>>>> -                     rockchip,pins = <4 RK_PB6 RK_FUNC_GPIO &pcfg_pull_none>;
>>>>> -             };
>>>>> -
>>>>>                 pcie3_vcc3v3_en: pcie3-vcc3v3-en {
>>>>>                         rockchip,pins = <1 RK_PA4 RK_FUNC_GPIO &pcfg_pull_none>;
>>>>>                 };
>>>>>
>>>>> base-commit: 34afb82a3c67f869267a26f593b6f8fc6bf35905
>>>>
>>> _______________________________________________
>>> Linux-rockchip mailing list
>>> Linux-rockchip@lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/linux-rockchip