[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");
> >
> >
>