[PATCH v2] PCI: rockchip: Skip the Tpvperl wait when power is already valid
Enrique Hernández Bello
ehbello at gmail.com
Sat Sep 12 07:14:56 PDT 2026
On Sat, 12 Sept 2026 at 02:21, Shawn Lin <shawn.lin at 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 at vger.kernel.org
> > Signed-off-by: Enrique Hernández Bello <ehbello at 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");
> >
> >
>
More information about the linux-arm-kernel
mailing list