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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANAwSgQdr-SADuKMxZqa=j6mHAC0K1N7nje29RTPm9N5j-++qg@mail.gmail.com>
Date: Thu, 11 Jul 2024 15:41:06 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Jonas Karlman <jonas@...boo.se>
Cc: Heiko Stuebner <heiko@...ech.de>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64: dts: rockchip: Add missing pinctrl for PCIe30x4 node

Hi Jonas

On Thu, 11 Jul 2024 at 15:15, Jonas Karlman <jonas@...boo.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@...boo.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@...il.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