Hi Jonas
On Thu, 11 Jul 2024 at 15:15, Jonas Karlman <jonas@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxx>
> >>> ---
> >>> 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@xxxxxxxxx/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
