Message-ID: <CANAwSgT+-UcjrH=hhD6fF1qoRMDHuudeKOi8fTpArJfhxy9Unw@mail.gmail.com>
Date: Thu, 1 Aug 2024 10:21:24 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Kever Yang <kever.yang@...k-chips.com>
Cc: Jonas Karlman <jonas@...boo.se>, 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 Kever, Jonas.

On Thu, 1 Aug 2024 at 07:53, Kever Yang <kever.yang@...k-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@...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.
> 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@...ts.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-rockchip

Powered by blists - more mailing lists