Message-ID: <CANAwSgTj0JzFN2CHOSG=X_gx5KttP-tZeLaC5uYZYhcPheP_Vg@mail.gmail.com>
Date: Mon, 10 Nov 2025 14:10:56 +0530
From: Anand Moon <linux.amoon@...il.com>
To: Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: Manivannan Sadhasivam <mani@...nel.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kwilczynski@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, Heiko Stuebner <heiko@...ech.de>, 
	Philipp Zabel <p.zabel@...gutronix.de>, Jingoo Han <jingoohan1@...il.com>, 
	Shawn Lin <shawn.lin@...k-chips.com>, linux-pci@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [PATCH v4 0/9] PCI: dw-rockchip: add system suspend support

Hi All,

On Fri, 7 Nov 2025 at 00:31, Anand Moon <linux.amoon@...il.com> wrote:
>
> Hi Sebastian,
>
> On Tue, 4 Nov 2025 at 00:29, Sebastian Reichel
> <sebastian.reichel@...labora.com> wrote:
> >
> > Hi,
> >
> > On Sat, Nov 01, 2025 at 07:29:41PM +0530, Manivannan Sadhasivam wrote:
> > > On Wed, Oct 29, 2025 at 06:56:39PM +0100, Sebastian Reichel wrote:
> > > > I've recently been working on fixing up at least basic system suspend
> > > > support on the Rockchip RK3576 platform. Currently the biggest open
> > > > issue is missing support in the PCIe driver. This series is a follow-up
> > > > for Shawn Lin's series with feedback from Niklas Cassel and Manivannan
> > > > Sadhasivam being handled as well as some of my own changes fixing up
> > > > things I noticed.
> > > >
> > > > In opposite to Shawn Lin I did not test with different peripherals as my
> > > > main goal is getting basic suspend to ram working in the first place.
> > >
> > > Wouldn't it break users who have connected endpoint devices and suspend their
> > > platform? I don't want to have an untested feature that could potentially cause
> > > regressions, just for the sake of getting basic system PM.
> > >
> > > But if your goal is to just add basic system PM operations for CI
> > > testing, then I would suggest you to do something minimal in the
> > > suspend/resume path that don't disrupt the operation of a device.
> > >
> > > But this also should be tested with some devices for sanity.
> >
> > My goal is proper system PM support, but I would like to go step by
> > step. Right now system suspend on the Rockchip RK3576 EVB just hangs
> > the board and it has to be power cycled afterwards. In parallel to
> > this series I've send a bunch of fixes to get it working. It surely
> > isn't perfect, but I fear things regressing again in other areas while
> > the complex PCIe system sleep is being worked on - simply blocking system
> > suspend is not very helpful, since it effectively hides suspend problems.
> >
> As per my understanding, the current DTS configuration is missing definitions
> for critical PCIe power management GPIOs (clkreq-gpios, perst-gpios, wake-gpios)
>
> clkreq-gpios, such as PCIE30x1_0_CLKREQn_M1_L (not sure if it is used ?)
> perst-gpios such as PCIE30x1_0_PERSTn_M1_L
> wake-gpios, such as PCIE30x1_0_WAKEn_M1_L.
>
As per the TRM 11.5 Interface Description
Signal Name     Direction   IO Attribute             Description
------------    ---------   -----------------------
------------------------------------------------------------
BUTTON_RSTN     I             Pull-down                    External
reset button input; pulled low to initiate reset
WAKEN                   I/O        Open-drain, pull-up      Wake
signal; enables device or host to signal wake events
PERSTN                 I/O         Pull-down                     PCIe
reset signal; asserted low to reset endpoint or root complex
CLKREQN              I/O         Open-drain, pull-up      Clock
request signal; used for dynamic clock management

See the following commit 4294e3211178 ("arm64: dts: rockchip: Split up
RK3588's PCIe pinctrls")

    These pinctrls manage the low-speed PCIe signals:
    - CLKREQ#: An output on the RK3588 (both RC or EP modes), used to
      request that external clock-generation circuitry provide a clock.
    - PERST#: An input on the RK3588 in EP mode, used to detect a reset
      signal from the RC. In RC mode, the hardware does not use this signal:
      Linux itself generates it by putting the pin in GPIO mode.
    - WAKE#: In EP mode, this is an output; in RC mode, this is an input.

    Each of these signals serves a distinct purpose, and more importantly,
    PERST# should not be muxed when the RK3588 is in the RC role. Bundling
    them together in pinctrl groups prevents proper use: indeed, almost none
    of the current board-specific .dts files make any use of them.
    (Exception: Rock 5A recently had a patch land that misuses _pins; this
     patch corrects that.)

    However, on some RK3588 boards, the PCIe 3 controller will indefinitely
    stall the boot if CLKREQ# is not muxed (details in the next patch).
    This patch unbundles the signals to allow them to be used.

So we can use these pinctrl to perform different tasks
like PERST# to reset and WAKE# to wake the PCIE in suspend / resume state.

Is this a good way to split the rk368 PCIe pinctrl into separate components?

Here is the example user wake gpio.

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
index 19a08f7794e6..13a7aa3ec1fc 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5a.dts
@@ -359,9 +359,10 @@ rgmii_phy1: ethernet-phy@1 {
 };

 &pcie2x1l2 {
-       pinctrl-0 = <&pcie2_reset>, <&pcie20x1m0_clkreqn>, <&pcie20x1m0_waken>;
+       pinctrl-0 = <&pcie2_reset>, <&pcie20x1m0_clkreqn>, <&pcie2wakeup>;
        pinctrl-names = "default";
        reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>;
+       wakeup-gpio = <&gpio3 RK_PD0 GPIO_ACTIVE_HIGH>;
        vpcie3v3-supply = <&vcc3v3_wf>;
        status = "okay";
 };

I haven’t come across a working example for this in RC mode.
Is there any confirmation that this approach functions as expected?


> However, the RK3588 TRM indicates that these power management functions
> can be controlled programmatically using specific memory-mapped registers:
>
> The PCIE_CLIENT_POWER_CON register at 0x002C provides comprehensive
> power management controls, including link-state management, wake-up
> event handling,
> and clock management.
>
> In PHY GRF, we have the PCIe3PHY_GRF_PHY0_LN0_CON0 register at 0x1000 allows
> direct control over the PHY's power state (P-states like P1, P2),
> enabling transitions into
> deep suspend (L2/L3) via register writes
> clkreq_n Clock request for lane X. This is a side-band signal that a
> PIPE 4.2 controller needs
> to enter and exit P1.CPM, P1.1, and P1.2 power states.
>
> My thought is that using rockchip_pcie_phy_deinit() in the suspend
> routine and rockchip_pcie_phy_init() in the resume routine are
> incorrect; these functions
> likely perform full resets or power cuts rather than managed power
> state transitions,
> thus disrupting the desired suspended state of the PCIe link
>
> I tried a few things on my own, but I am not moving forward.
>
Thanks
-Anand

Powered by blists - more mailing lists