[PATCH v5 0/2] PCI: rockchip-host: Make the PERST# inactive delay configurable

[PATCH v5 0/2] PCI: rockchip-host: Make the PERST# inactive delay configurable

[Enrique Hernández Bello]

rockchip_pcie_host_init_port() waits T_PVPERL, 100 ms, before releasing
PERST#. The line has been asserted since the GPIO was acquired early in
probe, so that wait is also the reset pulse width.

Endpoints do not agree on what they want there. Some need longer to settle;
a JMB585 behind a ROCK Pi 4C stops answering configuration reads if the
pulse is stretched that far. The measurements are in patch 2.

So, as Shawn suggested, take the delay from the device tree instead of
changing it for everyone. Patch 1 documents rockchip,perst-inactive-ms and
rockchip,s2r-perst-inactive-ms, named after the properties the Rockchip
vendor driver already carries for the dwc-based controllers; patch 2 reads
them. The defaults leave current behaviour untouched, so there is no Fixes:
tag and nothing to backport.

There is no DT patch for my board in this series. Its JMB585 sits on a Radxa
Penta SATA HAT, which is not described upstream, so the property belongs in
an overlay.

Tested on that board with the series backported to 6.18.58, the kernel
Talos Linux ships; only the context of the #include hunk differs. One
kernel build, two images that differ only in the overlay: with
rockchip,perst-inactive-ms = <0>, the JMB585 enumerated on 4 boots out of
4; without the property, all 3 boots hit the same SError in
rockchip_pcie_rd_conf() during probe that the board hits without the
series.

Igor, since the defaults are unchanged, a Helios64 with no DT change should
behave exactly like plain mainline. If you want to exercise the property
as well:

	&pcie0 {
		rockchip,perst-inactive-ms = <10>;
	};

A wait of 0 to 15 ms enumerated 17 times out of 17 on my board, but your
slot supply is a GPIO-switched regulator where mine is always on, so I
would not take my numbers as a guide for yours.

v4: https://lore.kernel.org/all/20261028014116.2432425-1-ehbello@gmail.com/
v3 (discussion with Shawn): https://lore.kernel.org/all/20261028005603.2399220-1-ehbello@gmail.com/

Changes in v5:
- Take the delay from the device tree, as suggested by Shawn Lin, instead of
  shortening the wait automatically when the slot supplies were found
  already enabled.
- Drop struct rockchip_pcie::supplies_pre_enabled and
  rockchip_pcie_supplies_enabled() altogether. That also retires the hole
  Shawn found in them: devm_regulator_get() returns a dummy regulator for an
  absent rail and the dummy reports itself enabled, so the old test was
  unconditionally true on a board describing none of its supplies, on resume
  included.
- Keep the 100 us floor from v4, now for any value a board asks for.
- Split into a bindings patch and a driver patch.

Changes in v4:
- Hold PERST# for T_PERST-CLK in the shortened path. Old power does not
  imply an old REFCLK: init_port() runs phy_init()/phy_power_on() on every
  probe and resume, so the PLL relocks microseconds before the release.
  Measured 72-75 us by ftrace, i.e. a real violation of the 100 us minimum.

Enrique Hernández Bello (2):
  dt-bindings: PCI: rockchip,rk3399-pcie: Add PERST# inactive delay
    properties
  PCI: rockchip-host: Make the PERST# inactive delay configurable

 .../bindings/pci/rockchip,rk3399-pcie.yaml    | 15 +++++++++++
 drivers/pci/controller/pcie-rockchip-host.c   | 26 ++++++++++++++++---
 drivers/pci/controller/pcie-rockchip.h        |  2 ++
 3 files changed, 39 insertions(+), 4 deletions(-)

-- 
2.56.0

[PATCH v5 1/2] dt-bindings: PCI: rockchip,rk3399-pcie: Add PERST# inactive delay properties

[Enrique Hernández Bello]

The driver holds PERST# asserted for T_PVPERL (100 ms) before releasing it.
Endpoints disagree about what they want there: some need longer than the
default to settle, while others stop answering configuration reads when the
reset pulse is stretched that far, because PERST# stays asserted for the
whole wait.

Describe two properties so a board can state what its endpoint needs, named
after the ones the Rockchip vendor driver already uses for the dwc-based
controllers:

  rockchip,perst-inactive-ms      - the delay used at probe
  rockchip,s2r-perst-inactive-ms  - the delay used when resuming

Both are optional and the defaults preserve the current behaviour.

Signed-off-by: Enrique Hernández Bello <ehbello@gmail.com>
---
 .../bindings/pci/rockchip,rk3399-pcie.yaml        | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
index 720a5f9..7ac915c 100644
--- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
+++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
@@ -40,6 +40,21 @@ properties:
     maxItems: 1
     description: pre-reset GPIO
 
+  rockchip,perst-inactive-ms:
+    default: 100
+    description:
+      Time PERST# is held asserted before it is released, which is also the
+      width of the reset pulse. The default meets T_PVPERL for a slot that
+      is powered at the moment PERST# is asserted. Some endpoints need more
+      settling time than that, while others stop responding when the pulse
+      is that long. A value of 0 selects the 100 us minimum the PCI Express
+      CEM specification sets for the pulse.
+
+  rockchip,s2r-perst-inactive-ms:
+    description:
+      The same time, applied when resuming from system suspend. Defaults to
+      the value of rockchip,perst-inactive-ms.
+
   vpcie12v-supply:
     description: The 12v regulator to use for PCIe.
 
-- 
2.56.0

[PATCH v5 2/2] PCI: rockchip-host: Make the PERST# inactive delay configurable

[Enrique Hernández Bello]

rockchip_pcie_host_init_port() waits T_PVPERL, 100 ms, before releasing
PERST#. The GPIO is acquired with GPIOD_OUT_LOW early in probe, so PERST#
is already asserted when that wait starts, and the wait therefore sets the
reset pulse width as well.

That suits a board which powers the slot at the moment it asserts PERST#,
but endpoints do not agree on what they want. On a ROCK Pi 4C carrying a
JMB585 SATA controller, a pulse that long stops the endpoint answering
configuration reads at all. Measured on that board with a single kernel and
two module parameters, one sleeping before the GPIO is acquired and one in
place of the wait, timing the pulse with a ktime delta across the two
gpiod_set_value() calls:

  pre (ms)  post (ms)  pulse      trials  enumerated
  0         0          1-11 ms    5       5
  0         10         ~18 ms     3       3
  0         15         24-47 ms   4       4
  0         20         ~28 ms     3       1
  0         100        ~108 ms    4       0
  200       0          6-11 ms    5       5
  200       100        ~108 ms    1       0

Delaying the assertion changes nothing, while widening the pulse decides
everything. The slot supply on this board is regulator-always-on, so power
had been valid for seconds in every row above, the failing ones included.

Rather than change the delay for every board, read it from the device tree,
reusing the property names the Rockchip vendor driver already carries for
the dwc-based controllers in pcie-dw-rockchip.c. The default is unchanged,
so a board that describes nothing behaves exactly as before. Whatever the
board asks for, PERST# stays asserted for at least the 100 us minimum the
specification sets, which covers T_PERST and T_PERST-CLK.

Suggested-by: Shawn Lin <shawn.lin@rock-chips.com>
Link: https://lore.kernel.org/all/95aeecc0-2794-4cc6-a635-5c042afc1405@rock-chips.com/
Signed-off-by: Enrique Hernández Bello <ehbello@gmail.com>
---
 drivers/pci/controller/pcie-rockchip-host.c | 26 +++++++++++++++++----
 drivers/pci/controller/pcie-rockchip.h      |  2 ++
 2 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pcie-rockchip-host.c b/drivers/pci/controller/pcie-rockchip-host.c
index 5b071bd..5f78cd6 100644
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -24,6 +24,7 @@
 #include <linux/of_pci.h>
 #include <linux/phy/phy.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 
 #include "../pci.h"
@@ -280,8 +281,10 @@ static void rockchip_pcie_set_power_limit(struct rockchip_pcie *rockchip)
 /**
  * rockchip_pcie_host_init_port - Initialize hardware
  * @rockchip: PCIe port information
+ * @perst_inactive_ms: how long to hold PERST# asserted before releasing it
  */
-static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
+static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip,
+					u32 perst_inactive_ms)
 {
 	struct device *dev = rockchip->dev;
 	int err, i = MAX_LANE_NUM;
@@ -315,7 +318,12 @@ static int rockchip_pcie_host_init_port(struct rockchip_pcie *rockchip)
 	rockchip_pcie_write(rockchip, PCIE_CLIENT_LINK_TRAIN_ENABLE,
 			    PCIE_CLIENT_CONFIG);
 
-	msleep(PCIE_T_PVPERL_MS);
+	/*
+	 * PERST# has been asserted since the GPIO was acquired, so this wait
+	 * is also the reset pulse width. Never go below the specification
+	 * minimum, which covers both T_PERST and T_PERST-CLK, each 100 us.
+	 */
+	fsleep(max(perst_inactive_ms * USEC_PER_MSEC, PCIE_T_PERST_CLK_US));
 	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
 
 	msleep(PCIE_RESET_CONFIG_WAIT_MS);
@@ -587,6 +595,14 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 	if (IS_ERR(rockchip->vpcie0v9))
 		return PTR_ERR(rockchip->vpcie0v9);
 
+	if (device_property_read_u32(dev, "rockchip,perst-inactive-ms",
+				     &rockchip->perst_inactive_ms))
+		rockchip->perst_inactive_ms = PCIE_T_PVPERL_MS;
+
+	if (device_property_read_u32(dev, "rockchip,s2r-perst-inactive-ms",
+				     &rockchip->s2r_perst_inactive_ms))
+		rockchip->s2r_perst_inactive_ms = rockchip->perst_inactive_ms;
+
 	return 0;
 }
 
@@ -881,7 +897,8 @@ static int rockchip_pcie_resume_noirq(struct device *dev)
 	if (err)
 		goto err_disable_0v9;
 
-	err = rockchip_pcie_host_init_port(rockchip);
+	err = rockchip_pcie_host_init_port(rockchip,
+					   rockchip->s2r_perst_inactive_ms);
 	if (err)
 		goto err_pcie_resume;
 
@@ -938,7 +955,8 @@ static int rockchip_pcie_probe(struct platform_device *pdev)
 		goto err_set_vpcie;
 	}
 
-	err = rockchip_pcie_host_init_port(rockchip);
+	err = rockchip_pcie_host_init_port(rockchip,
+					   rockchip->perst_inactive_ms);
 	if (err)
 		goto err_vpcie;
 
diff --git a/drivers/pci/controller/pcie-rockchip.h b/drivers/pci/controller/pcie-rockchip.h
index b5da156..c773f74 100644
--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -315,6 +315,8 @@ struct rockchip_pcie {
 	struct	regulator *vpcie1v8; /* 1.8V power supply */
 	struct	regulator *vpcie0v9; /* 0.9V power supply */
 	struct	gpio_desc *perst_gpio;
+	u32	perst_inactive_ms;
+	u32	s2r_perst_inactive_ms;
 	u32	lanes;
 	u8      lanes_map;
 	int	link_gen;
-- 
2.56.0

Re: [PATCH v5 1/2] dt-bindings: PCI: rockchip,rk3399-pcie: Add PERST# inactive delay properties

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] dt-bindings: PCI: rockchip,rk3399-pcie: OS policy and magic 0 in PERST# props
--

commit 9f3338157ea2855233568eb255bd2687fadd36fe
Author: Enrique Hernández Bello <ehbello@gmail.com>

dt-bindings: PCI: rockchip,rk3399-pcie: Add PERST# inactive delay properties

This commit adds the optional rockchip,perst-inactive-ms and
rockchip,s2r-perst-inactive-ms properties to the RK3399 PCIe host controller
binding to configure the reset assertion delay at probe and system resume.

> diff --git a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
> index 720a5f945a4e6..7ac915c267910 100644
> --- a/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/rockchip,rk3399-pcie.yaml
> @@ -40,6 +40,21 @@ properties:
>      maxItems: 1
>      description: pre-reset GPIO
>  
> +  rockchip,perst-inactive-ms:
> +    default: 100
> +    description:
> +      Time PERST# is held asserted before it is released, which is also the
> +      width of the reset pulse. The default meets T_PVPERL for a slot that
> +      is powered at the moment PERST# is asserted. Some endpoints need more
> +      settling time than that, while others stop responding when the pulse
> +      is that long. A value of 0 selects the 100 us minimum the PCI Express
> +      CEM specification sets for the pulse.
> +
> +  rockchip,s2r-perst-inactive-ms:
> +    description:
> +      The same time, applied when resuming from system suspend. Defaults to
> +      the value of rockchip,perst-inactive-ms.

[Severity: Medium]
Are vendor-specific host controller properties split by OS lifecycle state
the right way to model this in Device Tree?

Splitting the reset delay into rockchip,perst-inactive-ms (probe) and
rockchip,s2r-perst-inactive-ms (system suspend-to-ram resume) encodes OS
driver power-management transitions directly into the binding rather than
describing hardware characteristics.

In drivers/pci/controller/pcie-rockchip-host.c, rockchip_pcie_suspend_noirq()
only disables vpcie0v9 while leaving vpcie12v, vpcie3v3, and vpcie1v8
enabled, and does not assert perst_gpio on suspend. Then on both probe and
resume, rockchip_pcie_host_init_port() asserts perst_gpio, enables link
training (PCIE_CLIENT_LINK_TRAIN_ENABLE) while reset is still asserted, and
sleeps for PCIE_T_PVPERL_MS (100 ms) before deasserting perst_gpio.

Holding reset asserted for 100 ms after enabling link training or when slot
power rails are already stable looks like a sequencing issue in
pcie-rockchip-host.c rather than a property of the RK3399 controller.

Also, if an endpoint requires a non-standard reset delay, shouldn't that be
described using generic reset or PCI device properties (such as
reset-assert-ms or reset-delay-us) rather than SoC-specific host properties?
Neither rockchip,perst-inactive-ms nor rockchip,s2r-perst-inactive-ms exists
in mainline Documentation/devicetree/bindings/pci/rockchip-dw-pcie.yaml.

Finally, rockchip,perst-inactive-ms uses the standard -ms unit suffix, but
defines 0 as a magic value that selects a 100 us pulse width instead of 0 ms,
and names the active-low reset assertion duration "perst-inactive-ms" even
though the signal is held active (asserted) during this window. Could this be
handled by fixing the driver reset sequencing or using standard unit-accurate
properties instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104215848.2087944-1-ehbello@gmail.com?part=1