[PATCH v3] PCI: rockchip: Don't lengthen PERST# when power is already valid

[PATCH v3] PCI: rockchip: Don't lengthen PERST# when power is already valid

[Enrique Hernández Bello]

Since commit c47f90be4c89 ("PCI: rockchip-host: Fix
rockchip_pcie_host_init_port() PERST# handling"), a JMicron JMB585
behind an rk3399 root port is almost never enumerated: the link trains
normally, but the endpoint's configuration space never answers. On this
controller a configuration read that gets no usable completion is
reported as an external abort rather than as an all-ones response, so
the machine does not merely miss a device -- it dies with an
asynchronous SError inside rockchip_pcie_rd_conf().

That change added an unconditional msleep(PCIE_T_PVPERL_MS) to
rockchip_pcie_host_init_port(). PERST# is asserted much earlier, when
the line is acquired with GPIOD_OUT_LOW in rockchip_pcie_parse_dt(), so
the sleep lands inside the assertion and widens it. Measuring the pulse
in the driver with a ktime delta across the two gpiod_set_value() calls
gives 1 ms without the sleep and 108 ms with it; msleep(100) rounds up
to roughly 107 ms. This endpoint does not recover from a reset that
long.

To separate the two things that sleep changes -- how long PERST# is held
low, and how long the supplies have been valid when it is released --
both delays were made runtime-adjustable on a single kernel, one before
the assertion and one after it, and the host driver was rebound to
re-run the whole probe:

  before assert   after assert   pulse       trials   enumerated
  0 ms            0 ms            1-11 ms         5        5
  0 ms           10 ms           ~18 ms           3        3
  0 ms           15 ms           24-47 ms         4        4
  0 ms           20 ms           ~28 ms           3        1
  0 ms          100 ms          ~108 ms           4        0
  200 ms          0 ms            6-11 ms         5        5
  200 ms        100 ms          ~108 ms           1        0

Delaying the assertion by 200 ms changes nothing, whether the pulse is
short (5 of 5 either way) or long (0 of 4 and 0 of 1); widening the
pulse is what decides the outcome. vcc3v3_pcie on this board is
regulator-always-on and regulator-boot-on, so in every row above --
the failing ones included -- the supplies have been valid for seconds
by the time the driver probes, and Tpvperl is met whether the wait runs
or not. It therefore cannot be what distinguishes them. What this
endpoint reacts to is the width of the PERST# pulse.

The wait and a short pulse cannot both be had in general: Tpvperl is
counted from the supplies becoming valid to PERST# being de-asserted
(PCIe CEM r5.1, sec. 2.9.2), so on a board that powers the slot at
probe time the assertion does have to last that long. Where the
supplies are already up, though, the requirement is met before PERST#
is ever asserted, and the sleep only widens the pulse.

So record whether the supplies were already enabled before the driver
enabled them, and skip the wait in that case. A supply that is already
on at probe was brought up either by the bootloader or by the regulator
core at boot, both of which precede a PCIe probe by far more than
Tpvperl. When the driver really does bring the rails up the full wait
still happens, as it does if regulator_is_enabled() cannot tell.

The same check is repeated on resume rather than assuming that power
was cycled: suspend calls regulator_disable() on the 0.9V supply, which
only drops a reference, so on a board where that rail is always-on or
shared with another consumer the power stays valid across the cycle.

Skipping the wait only when every supply is already on is strictly more
conservative than what this driver did for years: until the change
cited above there was no Tpvperl wait at all, and PERST# stayed
asserted only for as long as the register writes in between took.

Boot-level counts on the same board agree with the table: unmodified,
the endpoint enumerated on 0 of 84 boots; with this patch, 3 of 3; with
other ways of dropping the same wait, 16 of 16. Each of the three
PERST#-related changes that landed together in v6.11-rc1 was also
reverted individually, and only removing this wait made a difference.
Moving the wait later but still inside the assertion did not help
(0 of 15 boots), which is consistent with the width being what matters.

The measurements were taken on 6.18, but the code in question is
unchanged between v6.11 and v7.2.

Fixes: c47f90be4c89 ("PCI: rockchip-host: Fix rockchip_pcie_host_init_port() PERST# handling")
Cc: stable@vger.kernel.org
Signed-off-by: Enrique Hernández Bello <ehbello@gmail.com>
---
v3:
 - Rewrite the commit message. It now leads with the width of the PERST#
   pulse, which is what the endpoint reacts to, and demotes the Tpvperl
   redundancy to the reason the wait can safely be skipped rather than
   the explanation of the failure. Manivannan was right that those are
   two different claims and that only the first one was evidenced.
 - Add the A/B measurement that was missing: one kernel with the delay
   before and after the PERST# assertion independently adjustable, so
   pulse width and power-valid time can be varied separately on the
   same binary. Also note that this board's PCIe supply is always-on,
   so Tpvperl is met in the failing configurations too.
 - Note in the comment that the regulator_enable() calls are kept for the
   reference count and are independent of the Tpvperl question, so the next
   reader does not have to ask it.
 - No functional change from v2.

v2:
 - Re-evaluate the supplies in rockchip_pcie_resume_noirq() instead of
   assuming that the 0.9V rail was really turned off. On a board where
   that rail is always-on or shared, regulator_disable() leaves it on,
   and forcing the wait there would reintroduce on resume exactly the
   failure this patch fixes. Spotted by an automated review of v1.
 - Factor the test into rockchip_pcie_supplies_enabled() now that it
   has two callers.

v1: https://lore.kernel.org/all/20260911104952.4190994-1-ehbello@gmail.com/
v2: https://lore.kernel.org/all/20260911211559.207990-1-ehbello@gmail.com/

diff --git i/drivers/pci/controller/pcie-rockchip-host.c w/drivers/pci/controller/pcie-rockchip-host.c
index d203c48..495428b 100644
--- i/drivers/pci/controller/pcie-rockchip-host.c
+++ w/drivers/pci/controller/pcie-rockchip-host.c
@@ -314,7 +314,9 @@ 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);
+	if (!rockchip->supplies_pre_enabled)
+		msleep(PCIE_T_PVPERL_MS);
+
 	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
 
 	msleep(PCIE_RESET_CONFIG_WAIT_MS);
@@ -609,11 +611,39 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
 	return 0;
 }
 
+/*
+ * Tpvperl is counted from the supplies becoming valid, and the driver waits
+ * for it with PERST# asserted, so the wait also lengthens the reset pulse.
+ * Supplies that are already enabled before this driver enables them were
+ * brought up by the bootloader or by the regulator core at boot, both of
+ * which precede this point by far more than Tpvperl, so the requirement is
+ * already met. Treat an error from regulator_is_enabled() as "not known to
+ * be on" so that the caller waits.
+ *
+ * This does not change who owns the supplies: the regulator_enable() calls
+ * below stay, both so that regulator_late_cleanup() cannot switch off a rail
+ * that the bootloader merely left on and so that the regulator_disable() calls
+ * on suspend and in the probe error paths stay balanced. Whether a supply was
+ * already on says only whether Tpvperl has elapsed.
+ */
+static bool rockchip_pcie_supplies_enabled(struct rockchip_pcie *rockchip)
+{
+	return (IS_ERR(rockchip->vpcie12v) ||
+		regulator_is_enabled(rockchip->vpcie12v) > 0) &&
+	       (IS_ERR(rockchip->vpcie3v3) ||
+		regulator_is_enabled(rockchip->vpcie3v3) > 0) &&
+	       regulator_is_enabled(rockchip->vpcie1v8) > 0 &&
+	       regulator_is_enabled(rockchip->vpcie0v9) > 0;
+}
+
 static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
 {
 	struct device *dev = rockchip->dev;
 	int err;
 
+	rockchip->supplies_pre_enabled =
+		rockchip_pcie_supplies_enabled(rockchip);
+
 	if (!IS_ERR(rockchip->vpcie12v)) {
 		err = regulator_enable(rockchip->vpcie12v);
 		if (err) {
@@ -890,6 +920,13 @@ static int rockchip_pcie_resume_noirq(struct device *dev)
 	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
 	int err;
 
+	/*
+	 * Suspend calls regulator_disable() on the 0.9V supply, but on boards
+	 * where it is always-on or shared the rail does not actually drop, so
+	 * re-evaluate instead of assuming that power was cycled.
+	 */
+	rockchip->supplies_pre_enabled = rockchip_pcie_supplies_enabled(rockchip);
+
 	err = regulator_enable(rockchip->vpcie0v9);
 	if (err) {
 		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
diff --git i/drivers/pci/controller/pcie-rockchip.h w/drivers/pci/controller/pcie-rockchip.h
index 3e82a69..e63259a 100644
--- i/drivers/pci/controller/pcie-rockchip.h
+++ w/drivers/pci/controller/pcie-rockchip.h
@@ -318,6 +318,7 @@ struct rockchip_pcie {
 	struct	regulator *vpcie1v8; /* 1.8V power supply */
 	struct	regulator *vpcie0v9; /* 0.9V power supply */
 	struct	gpio_desc *perst_gpio;
+	bool	supplies_pre_enabled;
 	u32	lanes;
 	u8      lanes_map;
 	int	link_gen;

Re: [PATCH v3] PCI: rockchip: Don't lengthen PERST# when power is already valid

[Shawn Lin]

在 2026/10/28 星期三 8:56, Enrique Hernández Bello 写道:
> Since commit c47f90be4c89 ("PCI: rockchip-host: Fix
> rockchip_pcie_host_init_port() PERST# handling"), a JMicron JMB585
> behind an rk3399 root port is almost never enumerated: the link trains
> normally, but the endpoint's configuration space never answers. On this
> controller a configuration read that gets no usable completion is
> reported as an external abort rather than as an all-ones response, so
> the machine does not merely miss a device -- it dies with an
> asynchronous SError inside rockchip_pcie_rd_conf().
> 
> That change added an unconditional msleep(PCIE_T_PVPERL_MS) to
> rockchip_pcie_host_init_port(). PERST# is asserted much earlier, when
> the line is acquired with GPIOD_OUT_LOW in rockchip_pcie_parse_dt(), so
> the sleep lands inside the assertion and widens it. Measuring the pulse
> in the driver with a ktime delta across the two gpiod_set_value() calls
> gives 1 ms without the sleep and 108 ms with it; msleep(100) rounds up
> to roughly 107 ms. This endpoint does not recover from a reset that
> long.

Intuitively, one would expect that a short reset period prevents the
endpoint from recovering fully, that seems more plausible than the
opposite. A reset pulse that is too short may cause the device to be
enumerated before it has completely reset. As long as the reset is
clean, a longer reset wait should make the device work more reliably.

Therefore, the actual observation that a longer reset causes failure
is highly counterintuitive. Droping this wait for

> 
> To separate the two things that sleep changes -- how long PERST# is held
> low, and how long the supplies have been valid when it is released --
> both delays were made runtime-adjustable on a single kernel, one before
> the assertion and one after it, and the host driver was rebound to
> re-run the whole probe:
> 
>    before assert   after assert   pulse       trials   enumerated
>    0 ms            0 ms            1-11 ms         5        5
>    0 ms           10 ms           ~18 ms           3        3
>    0 ms           15 ms           24-47 ms         4        4
>    0 ms           20 ms           ~28 ms           3        1
>    0 ms          100 ms          ~108 ms           4        0
>    200 ms          0 ms            6-11 ms         5        5
>    200 ms        100 ms          ~108 ms           1        0
> 
> Delaying the assertion by 200 ms changes nothing, whether the pulse is
> short (5 of 5 either way) or long (0 of 4 and 0 of 1); widening the
> pulse is what decides the outcome. vcc3v3_pcie on this board is
> regulator-always-on and regulator-boot-on, so in every row above --
> the failing ones included -- the supplies have been valid for seconds
> by the time the driver probes, and Tpvperl is met whether the wait runs
> or not. It therefore cannot be what distinguishes them. What this
> endpoint reacts to is the width of the PERST# pulse.
> 
> The wait and a short pulse cannot both be had in general: Tpvperl is
> counted from the supplies becoming valid to PERST# being de-asserted
> (PCIe CEM r5.1, sec. 2.9.2), so on a board that powers the slot at
> probe time the assertion does have to last that long. Where the
> supplies are already up, though, the requirement is met before PERST#
> is ever asserted, and the sleep only widens the pulse.
> 
> So record whether the supplies were already enabled before the driver
> enabled them, and skip the wait in that case. A supply that is already
> on at probe was brought up either by the bootloader or by the regulator
> core at boot, both of which precede a PCIe probe by far more than
> Tpvperl. When the driver really does bring the rails up the full wait
> still happens, as it does if regulator_is_enabled() cannot tell.
> 
> The same check is repeated on resume rather than assuming that power
> was cycled: suspend calls regulator_disable() on the 0.9V supply, which
> only drops a reference, so on a board where that rail is always-on or
> shared with another consumer the power stays valid across the cycle.
> 
> Skipping the wait only when every supply is already on is strictly more
> conservative than what this driver did for years: until the change
> cited above there was no Tpvperl wait at all, and PERST# stayed
> asserted only for as long as the register writes in between took.
> 
> Boot-level counts on the same board agree with the table: unmodified,
> the endpoint enumerated on 0 of 84 boots; with this patch, 3 of 3; with
> other ways of dropping the same wait, 16 of 16. Each of the three
> PERST#-related changes that landed together in v6.11-rc1 was also
> reverted individually, and only removing this wait made a difference.
> Moving the wait later but still inside the assertion did not help
> (0 of 15 boots), which is consistent with the width being what matters.
> 
> The measurements were taken on 6.18, but the code in question is
> unchanged between v6.11 and v7.2.
> 
> Fixes: c47f90be4c89 ("PCI: rockchip-host: Fix rockchip_pcie_host_init_port() PERST# handling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Enrique Hernández Bello <ehbello@gmail.com>
> ---
> v3:
>   - Rewrite the commit message. It now leads with the width of the PERST#
>     pulse, which is what the endpoint reacts to, and demotes the Tpvperl
>     redundancy to the reason the wait can safely be skipped rather than
>     the explanation of the failure. Manivannan was right that those are
>     two different claims and that only the first one was evidenced.
>   - Add the A/B measurement that was missing: one kernel with the delay
>     before and after the PERST# assertion independently adjustable, so
>     pulse width and power-valid time can be varied separately on the
>     same binary. Also note that this board's PCIe supply is always-on,
>     so Tpvperl is met in the failing configurations too.
>   - Note in the comment that the regulator_enable() calls are kept for the
>     reference count and are independent of the Tpvperl question, so the next
>     reader does not have to ask it.
>   - No functional change from v2.
> 
> v2:
>   - Re-evaluate the supplies in rockchip_pcie_resume_noirq() instead of
>     assuming that the 0.9V rail was really turned off. On a board where
>     that rail is always-on or shared, regulator_disable() leaves it on,
>     and forcing the wait there would reintroduce on resume exactly the
>     failure this patch fixes. Spotted by an automated review of v1.
>   - Factor the test into rockchip_pcie_supplies_enabled() now that it
>     has two callers.
> 
> v1: https://lore.kernel.org/all/20260911104952.4190994-1-ehbello@gmail.com/
> v2: https://lore.kernel.org/all/20260911211559.207990-1-ehbello@gmail.com/
> 
> diff --git i/drivers/pci/controller/pcie-rockchip-host.c w/drivers/pci/controller/pcie-rockchip-host.c
> index d203c48..495428b 100644
> --- i/drivers/pci/controller/pcie-rockchip-host.c
> +++ w/drivers/pci/controller/pcie-rockchip-host.c
> @@ -314,7 +314,9 @@ 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);
> +	if (!rockchip->supplies_pre_enabled)
> +		msleep(PCIE_T_PVPERL_MS);
> +
>   	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);
>   
>   	msleep(PCIE_RESET_CONFIG_WAIT_MS);
> @@ -609,11 +611,39 @@ static int rockchip_pcie_parse_host_dt(struct rockchip_pcie *rockchip)
>   	return 0;
>   }
>   
> +/*
> + * Tpvperl is counted from the supplies becoming valid, and the driver waits
> + * for it with PERST# asserted, so the wait also lengthens the reset pulse.
> + * Supplies that are already enabled before this driver enables them were
> + * brought up by the bootloader or by the regulator core at boot, both of
> + * which precede this point by far more than Tpvperl, so the requirement is
> + * already met. Treat an error from regulator_is_enabled() as "not known to
> + * be on" so that the caller waits.
> + *
> + * This does not change who owns the supplies: the regulator_enable() calls
> + * below stay, both so that regulator_late_cleanup() cannot switch off a rail
> + * that the bootloader merely left on and so that the regulator_disable() calls
> + * on suspend and in the probe error paths stay balanced. Whether a supply was
> + * already on says only whether Tpvperl has elapsed.
> + */
> +static bool rockchip_pcie_supplies_enabled(struct rockchip_pcie *rockchip)
> +{
> +	return (IS_ERR(rockchip->vpcie12v) ||
> +		regulator_is_enabled(rockchip->vpcie12v) > 0) &&
> +	       (IS_ERR(rockchip->vpcie3v3) ||
> +		regulator_is_enabled(rockchip->vpcie3v3) > 0) &&
> +	       regulator_is_enabled(rockchip->vpcie1v8) > 0 &&
> +	       regulator_is_enabled(rockchip->vpcie0v9) > 0;
> +}
> +


The assumptions here rely on a very weak constraint. If some designs
don't define the 12V and 3.3V regulators in the device tree, but those
rails actually do power down along with the rest of the system during
suspend, then upon resume the reset wait will be skipped entirely. I'm
worried this could cause regressions.

Furthermore, even with the same power supply design, different endpoints
may behave differently. What if a particular device requires a longer
reset pulse to work reliably? How would we handle that?

>   static int rockchip_pcie_set_vpcie(struct rockchip_pcie *rockchip)
>   {
>   	struct device *dev = rockchip->dev;
>   	int err;
>   
> +	rockchip->supplies_pre_enabled =
> +		rockchip_pcie_supplies_enabled(rockchip);
> +
>   	if (!IS_ERR(rockchip->vpcie12v)) {
>   		err = regulator_enable(rockchip->vpcie12v);
>   		if (err) {
> @@ -890,6 +920,13 @@ static int rockchip_pcie_resume_noirq(struct device *dev)
>   	struct rockchip_pcie *rockchip = dev_get_drvdata(dev);
>   	int err;
>   
> +	/*
> +	 * Suspend calls regulator_disable() on the 0.9V supply, but on boards
> +	 * where it is always-on or shared the rail does not actually drop, so
> +	 * re-evaluate instead of assuming that power was cycled.
> +	 */
> +	rockchip->supplies_pre_enabled = rockchip_pcie_supplies_enabled(rockchip);
> +
>   	err = regulator_enable(rockchip->vpcie0v9);
>   	if (err) {
>   		dev_err(dev, "fail to enable vpcie0v9 regulator\n");
> diff --git i/drivers/pci/controller/pcie-rockchip.h w/drivers/pci/controller/pcie-rockchip.h
> index 3e82a69..e63259a 100644
> --- i/drivers/pci/controller/pcie-rockchip.h
> +++ w/drivers/pci/controller/pcie-rockchip.h
> @@ -318,6 +318,7 @@ struct rockchip_pcie {
>   	struct	regulator *vpcie1v8; /* 1.8V power supply */
>   	struct	regulator *vpcie0v9; /* 0.9V power supply */
>   	struct	gpio_desc *perst_gpio;
> +	bool	supplies_pre_enabled;
>   	u32	lanes;
>   	u8      lanes_map;
>   	int	link_gen;
> 
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip

Re: [PATCH v3] PCI: rockchip: Don't lengthen PERST# when power is already valid

[Enrique Hernández Bello]

On Wed, 28 Oct 2026 09:34:04 +0800, Shawn Lin wrote:

First, an apology for the noise: a v4 went out seven minutes after your mail
and crossed it. It answers an unrelated review comment about T_PERST-CLK and
does nothing about the points you raise, so please do not spend time on it:

  https://lore.kernel.org/all/20261028014116.2432425-1-ehbello@gmail.com/

I was respinning too quickly. The next version will wait for this thread to
settle rather than landing on top of it.

> The assumptions here rely on a very weak constraint. If some designs
> don't define the 12V and 3.3V regulators in the device tree, but those
> rails actually do power down along with the rest of the system during
> suspend, then upon resume the reset wait will be skipped entirely. I'm
> worried this could cause regressions.

You are right, and it is worse than that. vpcie1v8 and vpcie0v9 come from
devm_regulator_get(), not the _optional() variant, so when they are absent
from DT the core substitutes a dummy rather than failing, and
_regulator_is_enabled() returns 1 for a dummy:

	/* If we don't know then assume that the regulator is always on */
	if (!rdev->desc->ops->is_enabled)
		return 1;

Combined with my IS_ERR() checks treating an absent optional supply as
satisfied, rockchip_pcie_supplies_enabled() is unconditionally true on a
board that describes none of the four rails. The wait would then be skipped
on every such board, on resume included. That is a regression and the patch
should not go in as it stands.

The underlying mistake is that regulator_is_enabled() reports the present
state, not for how long it has held. Probe can bridge that gap: the system
has been running for seconds by then and this driver has not touched the
rails yet, so a rail that reads enabled has been enabled since boot -- and
that holds for a dummy too, since a dummy means nobody is switching it.
Resume cannot bridge it at all: the rail may have dropped and come back
microseconds ago, possibly below the regulator core's visibility.

So the next version drops the resume hunk and sets supplies_pre_enabled to
false there explicitly, leaving the full wait in place on resume. The change
then only touches probe, where the inference is sound. That also removes the
one hunk I had no measurement for: it came out of an automated review of v1,
not from hardware.

> Intuitively, one would expect that a short reset period prevents the
> endpoint from recovering fully, that seems more plausible than the
> opposite. [...] Therefore, the actual observation that a longer reset
> causes failure is highly counterintuitive.

Agreed, it is counterintuitive, and I cannot explain the endpoint's
internals -- only what it does. What I can rule out is the reading your "as
long as the reset is clean" suggests, that something else happening during
the wait is the real cause. Link training is enabled just before the sleep,
so the root port spends those 100 ms training against a device held in
reset, which looked like the obvious suspect. Moving the wait to before the
link-training enable, keeping the pulse just as long, still failed 0 of 15
boots. It is the duration, not what the controller does during it.

The v3 table is one kernel with two module parameters, one sleeping before
the PERST# assertion and one in place of the msleep, and the host driver
rebound to re-run probe, so nothing varies between rows except where the
delay sits. A rebind reproduces the cold-boot failure identically, which I
checked before trusting it.

> Furthermore, even with the same power supply design, different endpoints
> may behave differently. What if a particular device requires a longer
> reset pulse to work reliably? How would we handle that?

The only floor the spec gives is T_PERST, 100 us of active PERST#, and v4
started guaranteeing that explicitly with an fsleep() rather than relying on
the 100 ms. Beyond that there is no required minimum to preserve.

And the short pulse is not new behaviour: from the driver's introduction
until c47f90be4c89 in v6.11-rc1 there was no wait before the de-assert at
all, so the pulse was only as long as the register writes in between took.
That is what every endpoint on this driver saw for years. The 100 ms pulse
is the recent change, and it is what broke this one. If some device turns
out to need longer, that seems like a quirk for that device rather than a
reason to keep a default that regressed another.

Thanks for the review -- the regulator point in particular is one I would
not have found.

Re: [PATCH v3] PCI: rockchip: Don't lengthen PERST# when power is already valid

[Shawn Lin]

在 2026/10/28 星期三 9:59, Enrique Hernández Bello 写道:
> On Wed, 28 Oct 2026 09:34:04 +0800, Shawn Lin wrote:
> 

...

> 
>> Furthermore, even with the same power supply design, different endpoints
>> may behave differently. What if a particular device requires a longer
>> reset pulse to work reliably? How would we handle that?
> 
> The only floor the spec gives is T_PERST, 100 us of active PERST#, and v4
> started guaranteeing that explicitly with an fsleep() rather than relying on
> the 100 ms. Beyond that there is no required minimum to preserve.
> 
> And the short pulse is not new behaviour: from the driver's introduction
> until c47f90be4c89 in v6.11-rc1 there was no wait before the de-assert at
> all, so the pulse was only as long as the register writes in between took.
> That is what every endpoint on this driver saw for years. The 100 ms pulse
> is the recent change, and it is what broke this one. If some device turns
> out to need longer, that seems like a quirk for that device rather than a
> reason to keep a default that regressed another.
> 

Was it possible to add a DT property for configurable delay? I came
across several EPs need quite a longer wait time, so add it in the
vendor tree for dwc-based IP[1]. That could be applied here to slove
your cases and others', and might avoid regression.

[1] 
https://github.com/rockchip-linux/kernel/blob/develop-6.12/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L865

Re: [PATCH v3] PCI: rockchip: Don't lengthen PERST# when power is already valid

[Enrique Hernández Bello]

On Wed, 28 Oct 2026 02:21:14 +0000, Shawn Lin wrote:

> Was it possible to add a DT property for configurable delay? I came
> across several EPs need quite a longer wait time, so add it in the
> vendor tree for dwc-based IP[1]. That could be applied here to slove
> your cases and others', and might avoid regression.
>
> [1]
> https://github.com/rockchip-linux/kernel/blob/develop-6.12/drivers/pci/controller/dwc/pcie-dw-rockchip.c#L865

Thanks -- that is a better shape than mine, and I will do it.

I will mirror the vendor driver: rockchip,perst-inactive-ms for the pulse, and
rockchip,s2r-perst-inactive-ms for the resume path, since the two paths want
different values here as well.

One thing I would rather ask than guess: should the default stay at
PCIE_T_PVPERL_MS, with affected boards setting the property, or should the
default be the spec minimum, with the property there for endpoints that need
longer?

What makes me ask is where T_PVPERL is measured from. It runs from power
stable to PERST# inactive, so on a board whose slot rails are regulator-
always-on it is already satisfied seconds before the driver touches the
GPIO, and the 100 ms then only widens the pulse. The one pulse-width floor
the spec gives is T_PERST, 100 us.

On my board that difference decides whether the JMB585 answers config reads at
all: 0 of 5 boots with a 100 ms pulse, 5 of 5 with a short one, with the
power-settling time held constant across both.

Keeping the current default is the safer change and I am happy to do that. I
would just rather we did it knowingly, instead of carrying a number that was
only ever right for boards which power the slot at the moment they assert
PERST#.

Secondary question: do you want the vendor-prefixed names, or would the DT
binding maintainers prefer something generic, given the timing comes from the
CEM spec rather than from Rockchip?