[PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[PATCH v2] PCI: rockchip: Skip the Tpvperl wait 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 almost never becomes usable: the link trains
normally, but the endpoint's configuration space never answers, so the
device is not enumerated. On this controller a configuration read that
gets no usable completion is reported as an external abort rather than
as an all-ones response, which on arm64 brings the machine down.

The change added an unconditional 100 ms sleep so that PERST# stays
asserted for at least Tpvperl after power becomes valid. The wait is
performed while PERST# is asserted, so it also extends the reset by
100 ms, and this endpoint does not tolerate the longer assertion.

Tpvperl is counted from the supplies becoming valid (PCIe CEM r5.1,
sec 2.9.2). On boards whose PCIe supplies are always-on -- vcc3v3_pcie
on ROCK Pi 4 is regulator-always-on and regulator-boot-on -- power has
been valid since boot, seconds before the driver probes, so the
requirement is already met and the sleep only lengthens the reset.

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.

Measured on a ROCK Pi 4C with a Radxa Penta SATA HAT (JMB585) by
booting repeatedly and counting how often the endpoint enumerated:

  unmodified ....................................  0 out of 84 boots
  with this patch ...............................  3 out of 3 boots
  other ways of dropping the same wait ..........  16 out of 16 boots

Fisher's exact test, pooling the last two rows against the first, gives
p = 4.1e-21. With the patch the endpoint enumerated on every boot and all
four disks behind it came up.

Each of the three PERST#-related changes that landed together in
v6.11-rc1 was also reverted individually; only removing this wait made
any difference. Moving the wait to before link training is enabled,
rather than removing it, did not help (0 out of 15 boots), which is
what identified the length of the PERST# assertion rather than any
interaction with link training as the cause.

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>
---
---
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/

--- a/drivers/pci/controller/pcie-rockchip.h
+++ b/drivers/pci/controller/pcie-rockchip.h
@@ -318,6 +318,7 @@
 	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;
--- a/drivers/pci/controller/pcie-rockchip-host.c
+++ b/drivers/pci/controller/pcie-rockchip-host.c
@@ -314,7 +314,9 @@
 	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,33 @@
 	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.
+ */
+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 +914,13 @@
 	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");

Re: [PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[Shawn Lin]

在 2026/09/12 星期六 5:15, 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 almost never becomes usable: the link trains
> normally, but the endpoint's configuration space never answers, so the
> device is not enumerated. On this controller a configuration read that
> gets no usable completion is reported as an external abort rather than
> as an all-ones response, which on arm64 brings the machine down.
> 
> The change added an unconditional 100 ms sleep so that PERST# stays
> asserted for at least Tpvperl after power becomes valid. The wait is
> performed while PERST# is asserted, so it also extends the reset by
> 100 ms, and this endpoint does not tolerate the longer assertion.
> 
> Tpvperl is counted from the supplies becoming valid (PCIe CEM r5.1,
> sec 2.9.2). On boards whose PCIe supplies are always-on -- vcc3v3_pcie
> on ROCK Pi 4 is regulator-always-on and regulator-boot-on -- power has
> been valid since boot, seconds before the driver probes, so the
> requirement is already met and the sleep only lengthens the reset.
> 
> 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.
> 
> Measured on a ROCK Pi 4C with a Radxa Penta SATA HAT (JMB585) by
> booting repeatedly and counting how often the endpoint enumerated:
> 
>    unmodified ....................................  0 out of 84 boots
>    with this patch ...............................  3 out of 3 boots
>    other ways of dropping the same wait ..........  16 out of 16 boots
> 
> Fisher's exact test, pooling the last two rows against the first, gives
> p = 4.1e-21. With the patch the endpoint enumerated on every boot and all
> four disks behind it came up.
> 
> Each of the three PERST#-related changes that landed together in
> v6.11-rc1 was also reverted individually; only removing this wait made
> any difference. Moving the wait to before link training is enabled,
> rather than removing it, did not help (0 out of 15 boots), which is
> what identified the length of the PERST# assertion rather than any
> interaction with link training as the cause.
> 
> 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>
> ---
> ---
> 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/
> 
> --- a/drivers/pci/controller/pcie-rockchip.h
> +++ b/drivers/pci/controller/pcie-rockchip.h
> @@ -318,6 +318,7 @@
>   	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;
> --- a/drivers/pci/controller/pcie-rockchip-host.c
> +++ b/drivers/pci/controller/pcie-rockchip-host.c
> @@ -314,7 +314,9 @@
>   	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,33 @@
>   	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.
> + */
> +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 it's pre-enabled, should we still need to enable this regulator just
for adding a refcount for it?

>   	if (!IS_ERR(rockchip->vpcie12v)) {
>   		err = regulator_enable(rockchip->vpcie12v);
>   		if (err) {
> @@ -890,6 +914,13 @@
>   	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");
> 
> 

Re: [PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[Enrique Hernández Bello]

On Sat, 12 Sept 2026 at 02:21, Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> 在 2026/09/12 星期六 5:15, 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 almost never becomes usable: the link trains
> > normally, but the endpoint's configuration space never answers, so the
> > device is not enumerated. On this controller a configuration read that
> > gets no usable completion is reported as an external abort rather than
> > as an all-ones response, which on arm64 brings the machine down.
> >
> > The change added an unconditional 100 ms sleep so that PERST# stays
> > asserted for at least Tpvperl after power becomes valid. The wait is
> > performed while PERST# is asserted, so it also extends the reset by
> > 100 ms, and this endpoint does not tolerate the longer assertion.
> >
> > Tpvperl is counted from the supplies becoming valid (PCIe CEM r5.1,
> > sec 2.9.2). On boards whose PCIe supplies are always-on -- vcc3v3_pcie
> > on ROCK Pi 4 is regulator-always-on and regulator-boot-on -- power has
> > been valid since boot, seconds before the driver probes, so the
> > requirement is already met and the sleep only lengthens the reset.
> >
> > 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.
> >
> > Measured on a ROCK Pi 4C with a Radxa Penta SATA HAT (JMB585) by
> > booting repeatedly and counting how often the endpoint enumerated:
> >
> >    unmodified ....................................  0 out of 84 boots
> >    with this patch ...............................  3 out of 3 boots
> >    other ways of dropping the same wait ..........  16 out of 16 boots
> >
> > Fisher's exact test, pooling the last two rows against the first, gives
> > p = 4.1e-21. With the patch the endpoint enumerated on every boot and all
> > four disks behind it came up.
> >
> > Each of the three PERST#-related changes that landed together in
> > v6.11-rc1 was also reverted individually; only removing this wait made
> > any difference. Moving the wait to before link training is enabled,
> > rather than removing it, did not help (0 out of 15 boots), which is
> > what identified the length of the PERST# assertion rather than any
> > interaction with link training as the cause.
> >
> > 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>
> > ---
> > ---
> > 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/
> >
> > --- a/drivers/pci/controller/pcie-rockchip.h
> > +++ b/drivers/pci/controller/pcie-rockchip.h
> > @@ -318,6 +318,7 @@
> >       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;
> > --- a/drivers/pci/controller/pcie-rockchip-host.c
> > +++ b/drivers/pci/controller/pcie-rockchip-host.c
> > @@ -314,7 +314,9 @@
> >       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,33 @@
> >       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.
> > + */
> > +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 it's pre-enabled, should we still need to enable this regulator just
> for adding a refcount for it?
>

Yes, I think the regulator_enable() calls have to stay, and the patch
deliberately leaves them untouched: the new flag only records what the
Tpvperl requirement already is, it does not take over the supply's
lifetime management.

Two reasons.

1) Without a reference the regulator core can switch the rail off
   underneath us. regulator_late_cleanup() disables, at late_initcall,
   every regulator that is not always_on, whose status it is allowed to
   change, and whose use_count is zero:

        if (c && c->always_on)
                return 0;
        if (!regulator_ops_is_valid(rdev, REGULATOR_CHANGE_STATUS))
                return 0;
        regulator_lock(rdev);
        if (rdev->use_count)
                goto unlock;
        /* If reading the status failed, assume that it's off. */
        if (_regulator_is_enabled(rdev) <= 0)
                goto unlock;
        if (have_full_constraints()) {
                rdev_info(rdev, "disabling\n");
                ret = _regulator_do_disable(rdev);

   have_full_constraints() is true for any populated device tree, so
   this is the normal path here. A supply that the bootloader merely
   left on -- as opposed to one marked regulator-always-on, which is
   what ROCK Pi 4 happens to use -- would be turned off shortly after
   probe if this driver had not taken a reference on it.

2) The disables would become unbalanced. rockchip_pcie_suspend_noirq()
   and the probe error paths call regulator_disable(), and with no
   matching enable _regulator_disable() trips

        WARN(regulator->enable_count == 0,
             "unbalanced disables for %s\n", rdev_get_name(rdev))

   and returns -EIO.

So the enables are about owning the supply, which is orthogonal to how
long PERST# is held. Keeping them is also what makes the change minimal:
the only behavioural difference is whether a 100 ms sleep happens inside
the PERST# assertion.

If you think it would help the next reader, I am happy to send a v3 that
notes in the comment that the enables are kept for the reference count
and are independent of the Tpvperl question.

Thanks for taking a look.

> >       if (!IS_ERR(rockchip->vpcie12v)) {
> >               err = regulator_enable(rockchip->vpcie12v);
> >               if (err) {
> > @@ -890,6 +914,13 @@
> >       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");
> >
> >
>

Re: [PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[Manivannan Sadhasivam]

On 2026-09-11 22:15 +0100, Enrique Hernández Bello wrote:
> Since commit c47f90be4c89 ("PCI: rockchip-host: Fix
> rockchip_pcie_host_init_port() PERST# handling"), a JMicron JMB585
> behind an rk3399 root port almost never becomes usable: the link trains
> normally, but the endpoint's configuration space never answers, so the
> device is not enumerated. On this controller a configuration read that
> gets no usable completion is reported as an external abort rather than
> as an all-ones response, which on arm64 brings the machine down.
> 
> The change added an unconditional 100 ms sleep so that PERST# stays
> asserted for at least Tpvperl after power becomes valid. The wait is
> performed while PERST# is asserted, so it also extends the reset by
> 100 ms, and this endpoint does not tolerate the longer assertion.
> 

This is hard to believe. First of all, Tpvperl is the min time specified by the
CEM spec to allow the power to be stable, not the max. Even if the bootloader
has already enabled the power supply during boot, it would've taken several
seconds for the kernel driver to probe and execute this delay of 100ms. But
claiming that the Endpoint doesn't tolerate this 100ms delay, but still several
seconds of the boot delay is quite hard to believe.

Maybe the issue is with the delay between PERST# assert and deassert? Is the
PERST# signal polarity on the board DTS correct? We used to see Endpoint
firmware crashes when the polarity was incorrect and the Endpoint got confused
with incorrect PERST# sequence.

> Tpvperl is counted from the supplies becoming valid (PCIe CEM r5.1,
> sec 2.9.2). On boards whose PCIe supplies are always-on -- vcc3v3_pcie
> on ROCK Pi 4 is regulator-always-on and regulator-boot-on -- power has
> been valid since boot, seconds before the driver probes, so the
> requirement is already met and the sleep only lengthens the reset.
> 

I do agree that the Tpvperl delay is *redundant* if the supplies are already
enabled by the bootloader. But I do not agree that it causes the Endpoint to not
respond.

- Mani

Re: [PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[Igor Velkov]

On Wed, 14 Oct 2026 12:57:47 +0530, Manivannan Sadhasivam wrote:
> I do agree that the Tpvperl delay is *redundant* if the supplies are already
> enabled by the bootloader. But I do not agree that it causes the Endpoint to not
> respond.

A data point from a Kobol Helios64 (RK3399, JMB585 on pcie0, ep-gpios as in
the mainline DTS):

- Enumeration at boot works with and without this patch.
- Resume from s2idle fails in rockchip_pcie_resume_noirq. On plain v7.3-rc7
  with the mainline DTB, 13 cycles pass and the 14th fails with "PCIe link
  training gen1 timeout"; resume_noirq returns -110 and the board resets.
  On an Armbian kernel with this patch (all supplies stay enabled across
  suspend, so the wait is skipped on resume too) the first resume ends in a
  synchronous external abort in rockchip_pcie_rd_conf from pci_power_up.
- Plain v7.3-rc7 also hit an asynchronous SError during the bus scan at probe
  in 1 of 4 boots.

So on this board the resume failure does not depend on the Tpvperl wait.

Igor

Re: [PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[Enrique Hernández Bello]

On Wed, 14 Oct 2026 12:57:47 +0530, Manivannan Sadhasivam wrote:
> This is hard to believe. First of all, Tpvperl is the min time specified by
> the CEM spec to allow the power to be stable, not the max.

Agreed, and the patch should not have implied otherwise. Waiting longer than
Tpvperl breaks no requirement, and I am not claiming the current code
violates the spec. What the patch rests on is only your next point: that the
wait is redundant when the supplies are already up.

> Maybe the issue is with the delay between PERST# assert and deassert?

That is exactly the variable this patch changes, and I buried it under the
power-validity argument. Let me restate the mechanism in those terms,
because I think we agree on it.

PERST# is asserted when the GPIO is acquired, not at the top of
rockchip_pcie_host_init_port():

  pcie-rockchip.c:
	rockchip->perst_gpio = devm_gpiod_get_optional(dev, "ep", GPIOD_OUT_LOW);

and it is released at the end of the init sequence:

  pcie-rockchip-host.c:
	gpiod_set_value_cansleep(rockchip->perst_gpio, 0);	/* already low */
	rockchip_pcie_init_port(rockchip);			/* PHY, clocks */
	... register writes, Gen1 training enable ...
	msleep(PCIE_T_PVPERL_MS);
	gpiod_set_value_cansleep(rockchip->perst_gpio, 1);	/* release */

So the low pulse spans the regulator enables, the PHY init and the register
writes, and commit c47f90be4c89 added 100 ms to the end of it. Removing that
sleep does not shorten the time power has been valid -- which, as you say,
is seconds by then -- it shortens the PERST# assertion by 100 ms. That is
the only behavioural difference the patch makes at probe, and it is what the
boot statistics in the commit message measure.

> But claiming that the Endpoint doesn't tolerate this 100ms delay, but still
> several seconds of the boot delay is quite hard to believe.

The endpoint is not held in reset for seconds. Before this driver probes,
the pin is in whatever state the bootloader left it; the reset the endpoint
sees is the pulse the driver applies, from the gpiod acquisition above to
the release, and c47f90be4c89 made that pulse 100 ms longer. Whether that
crosses a threshold in this particular endpoint's firmware is what the
measurements below address.

> Is the PERST# signal polarity on the board DTS correct?

As far as I can tell, yes. ROCK Pi 4 declares

	ep-gpios = <&gpio4 RK_PD3 GPIO_ACTIVE_HIGH>;

and the driver acquires the line with GPIOD_OUT_LOW and releases it with
gpiod_set_value(..., 1). PERST# is active low at the connector, so logical
low is physical low is reset asserted: the DTS polarity and the driver's use
of it agree. The Kobol Helios64 that Igor reports on uses the same polarity
(GPIO_ACTIVE_HIGH) with the same SoC and the same endpoint chip, and does
not show the boot failure, so polarity does not separate the two boards
either.

> I do agree that the Tpvperl delay is *redundant* if the supplies are already
> enabled by the bootloader. But I do not agree that it causes the Endpoint to
> not respond.

You were right, and the two claims needed separating. I have now run that
experiment, so here is the data rather than the argument.

I instrumented the driver with two module parameters, one sleeping before
the gpiod acquisition (so before PERST# is asserted) and one in place of the
existing msleep (so inside the assertion), plus a ktime delta across the two
gpiod_set_value() calls to measure the pulse the endpoint actually sees. One
kernel, both knobs writable at runtime, and the host driver rebound through
unbind/bind to re-run the whole probe -- I checked first that a rebind
reproduces the cold-boot failure, and it does, identically.

  before assert   after assert   measured 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

Compare each 200 ms row with its 0 ms counterpart: delaying the assertion
changes nothing either way -- 5 of 5 with a short pulse, 0 of 4 and 0 of 1
with a long one -- while widening the pulse decides the outcome every time.
So it is the delay between assert and deassert, exactly as you guessed.

And your objection holds in a way I had not appreciated. vcc3v3_pcie on this
board is regulator-always-on and regulator-boot-on, so the supplies have
been valid for seconds before the driver probes in *every* row above,
including the failing ones. Tpvperl is therefore met whether the sleep runs
or not, which means it cannot be what distinguishes a working configuration
from a failing one. The redundancy of the wait is only the licence to drop
it; it was never the explanation. The commit message had that backwards.

Two caveats I should state rather than smooth over. The threshold is noisy,
not a cliff: at 15 ms one trial measured a 47 ms pulse and still enumerated,
while 20 ms fails at ~28 ms. Across every configuration with the sleep at
15 ms or less it enumerated 17 of 17; around 28 ms it fails more often than
not; at ~108 ms it has never once worked, in 5 attempts (3 cold boots and
2 rebinds). And the failure is not a missing
device: the unanswered config read comes back as an external abort, so the
board dies with an asynchronous SError in rockchip_pcie_rd_conf() rather
than just enumerating nothing.

v3 therefore leads with the pulse width and demotes Tpvperl to the reason
the sleep can safely be skipped. The code is unchanged from v2: the two
cannot be had together in general, since on a board that powers the slot at
probe time the assertion does have to outlast Tpvperl, so skipping the wait
only where the supplies are already up is the conservative form of the fix.

Thanks for pushing back on this -- the causal claim deserved it, and the
alternative you proposed turned out to be the right one.

Re: [PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[Enrique Hernández Bello]

On Thu, 16 Oct 2026 01:21 +0300, Igor Velkov wrote:
> A data point from a Kobol Helios64 (RK3399, JMB585 on pcie0, ep-gpios as in
> the mainline DTS):
> - Enumeration at boot works with and without this patch.

Thank you -- this is the most useful thing anyone could have sent. It says
that on a second RK3399 board with the same endpoint chip the patch causes
no boot regression, and that your board does not have the failure I am
chasing. Both halves matter: the first is the thing I could not test, and
the second says the failure is not simply "RK3399 plus JMB585".

The boards differ in ways that could matter here: ROCK Pi 4 has
vpcie3v3-supply = <&vcc3v3_pcie>, which is regulator-always-on, and no 12V
supply; Helios64 drives pcie_power from a GPIO and feeds vpcie12v from
vcc12v_dcin. On your board the endpoint's rails may genuinely come up with
the probe rather than seconds earlier, which would make the sleep do what it
was written to do.

That distinction now has measurements behind it. I instrumented the driver
with two module parameters, one sleeping before the gpiod acquisition and
one in place of the existing msleep, plus a ktime delta across the two
gpiod_set_value() calls, so the PERST# pulse the endpoint sees can be set
and read on one kernel. On ROCK Pi 4C the pulse is 1 ms without the sleep
and 108 ms with it, and what decides enumeration is the pulse width alone:
delaying the assertion by 200 ms changes nothing, while 100 ms inside the
assertion fails every time (0 of 5). Since vcc3v3_pcie here is
regulator-always-on, Tpvperl is met in the failing configurations too, so it
cannot be the discriminator -- the wait being redundant is only why it is
safe to skip.

If your board genuinely powers the slot at probe, you would expect no
pulse-width sensitivity at all, which is consistent with it enumerating
either way. If you ever want to check that directly, the two-knob patch is
throwaway but small, and I am happy to send it off-list.

> - Resume from s2idle fails in rockchip_pcie_resume_noirq. On plain v7.3-rc7
>   with the mainline DTB, 13 cycles pass and the 14th fails [...]
>   On an Armbian kernel with this patch [...] the first resume ends in a
>   synchronous external abort in rockchip_pcie_rd_conf from pci_power_up.

Two things, one agreeing with you and one asking for more.

Agreeing: the failure happens on plain v7.3-rc7 without the patch, so it is
a pre-existing resume bug and not something this patch introduces. Your
conclusion that it does not depend on the Tpvperl wait is supported by your
own first measurement.

Asking: the with-patch run is on an Armbian kernel and the without-patch run
is on mainline v7.3-rc7, so the comparison is between two kernels, not
between two values of one condition. "First resume aborts" versus "13 cycles
pass" is a big enough difference that I do not want to attribute it to
either kernel without a same-kernel A/B. If you are willing, the cheapest
version is one kernel with the patch applied and the resume hunk neutered,
i.e. in rockchip_pcie_resume_noirq() replace

	rockchip->supplies_pre_enabled = rockchip_pcie_supplies_enabled(rockchip);

with

	rockchip->supplies_pre_enabled = false;

so the full wait is always performed on resume and only the probe path
differs. If your first resume then survives, the resume hunk is the problem
and I will drop it: the failure this patch is about is at probe, and the
resume re-evaluation came out of an automated review of v1 rather than out
of any measurement of mine. I would rather ship the narrow change that was
measured.

> - Plain v7.3-rc7 also hit an asynchronous SError during the bus scan at probe
>   in 1 of 4 boots.

That one I recognise. On ROCK Pi 4 a config read that gets no usable
completion comes back as an external abort rather than as all-ones, which is
why a non-responding endpoint takes the machine down instead of just failing
enumeration. Yours is during the bus scan on an unpatched kernel, so it
looks like the same root-port behaviour showing up independently of this
patch. If you have the log with the faulting address I would be interested,
but it belongs in its own thread rather than this one.

Re: [PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[Igor Velkov]

On Wed, 28 Oct 2026 00:55:48 +0000, Enrique Hernández Bello wrote:
> If your first resume then survives, the resume hunk is the problem
> and I will drop it

Since the next version keeps the full wait on resume, it no longer
changes the resume path, so I'll skip the A/B and test that version
on Helios64 instead: boots plus s2idle cycles, results in reply to it.

The probe SError is in the cover letter of my rockchip-host RFC:
https://lore.kernel.org/r/20261024-rockchip-pcie-resume-v1-0-58e54a5cf5f1@iav.lv

Igor

Re: [PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid

[Enrique Hernández Bello]

On Wed, 28 Oct 2026 05:10:00 +0000, Igor Velkov wrote:

> Since the next version keeps the full wait on resume, it no longer
> changes the resume path, so I'll skip the A/B and test that version
> on Helios64 instead: boots plus s2idle cycles, results in reply to it.

Thank you -- that coverage is worth a lot, but hold off a little, because the
shape is changing again.

Shawn Lin has asked for a DT property for the PERST# pulse rather than a
decision taken in the driver, matching rockchip,perst-inactive-ms and
rockchip,s2r-perst-inactive-ms in the vendor dwc driver, and I have agreed. So
the version to test is that one, not the probe-only patch I was about to send.

Whether the default changes or stays at PCIE_T_PVPERL_MS is still open, so I
will put the DT snippet to set on a Helios64 in the cover letter instead of
leaving it to guesswork.