[PATCH 0/2] regulator: Fix boot speed regression w/ off-on-delay-us

[PATCH 0/2] regulator: Fix boot speed regression w/ off-on-delay-us

[Douglas Anderson]

These two patches together fix a boot speed regression on
sc7180-trogdor boards which use off-on-delay-us together with
regulator-boot-on with the eDP power rail.

The first patch is _almost_ a revert of another patch. Depending on
when the regulator is probed and the actual delay, the end result
might be the same as a revert or might result in a delay that's
somewhere between "no delay" and a full delay. As talked about in the
patch, I believe this is the correct behavior and, if the problems
fixed in the original patch are still present, I'm hoping that they
can be fixed in a cleaner way.

The first patch doesn't fully eliminate the boot speed regression, it
only gets us back ~250 of the 500 ms delay. The second patch gets us
the rest of the way.

The two patches can be applied independently of each other.


Douglas Anderson (2):
  regulator: core: Shorten off-on-delay-us for always-on/boot-on by time
    since booted
  regulator: fixed: Set PROBE_PREFER_ASYNCHRONOUS

 drivers/regulator/core.c  | 7 +++----
 drivers/regulator/fixed.c | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH 1/2] regulator: core: Shorten off-on-delay-us for always-on/boot-on by time since booted

[Douglas Anderson]

This is very close to a straight revert of commit 218320fec294
("regulator: core: Fix off-on-delay-us for always-on/boot-on
regulators"). We've identified that patch as causing a boot speed
regression on sc7180-trogdor boards. While boot speed certainly isn't
more important than making sure that power sequencing is correct,
looking closely at the original change it doesn't seem to have been
fully justified. It mentions "cycling issues" without describing
exactly what the issues were. That means it's possible that the
cycling issues were really a problem that should be fixed in a
different way.

Let's take a careful look at how we should handle regulators that have
an off-on-delay and that are boot-on or always-on. Linux currently
doesn't have any way to identify whether a GPIO regulator was already
on when the kernel booted. That means that when the kernel boots we
probe a regulator, see that it wants boot-on / always-on we, and then
turn the regulator on. We could be in one of two cases when we do
this:

a) The regulator might have been left on by the bootloader and we're
   ensuring that it stays on.
b) The regulator might have been left off by the bootloader and we're
   just now turning it on.

For case a) we definitely don't need any sort of delay. For case b) we
_might_ need some delay in case the bootloader turned the regulator
off _right_ before booting the kernel. To get the proper delay for
case b) then we can just assume a `last_off` of 0, which is what it
gets initialized to by default.

As per above, we can't tell whether we're in case a) or case b) so
we'll assume the longer delay (case b). This basically puts the code
to how it was before commit 218320fec294 ("regulator: core: Fix
off-on-delay-us for always-on/boot-on regulators"). However, we add
one important change: we make sure that the delay is actually honored
if `last_off` is 0. Though the original "cycling issues" cited were
vague, I'm hopeful that this important extra change will be enough to
fix the issues that the initial commit mentioned.

With this fix, I've confined that on a sc7180-trogdor board the delay
at boot goes down from 500 ms to ~250 ms. That's not as good as the 0
ms that we had prior to commit 218320fec294 ("regulator: core: Fix
off-on-delay-us for always-on/boot-on regulators"), but it's probably
safer because we don't know if the bootloader turned the regulator off
right before booting.

One note is that it's possible that we could be in a state that's not
a) or b) if there are other issues in the kernel. The only one I can
think of is related to pinctrl. If the pinctrl driver being used on a
board isn't careful about avoiding glitches when setting up a pin then
it's possible that setting up a pin could cause the regulator to "turn
off" briefly immediately before the regulator probes. If this is
indeed causing problems then the pinctrl driver should be fixed,
perhaps in a similar way to what was done in commit d21f4b7ffc22
("pinctrl: qcom: Avoid glitching lines when we first mux to output")

Fixes: 218320fec294 ("regulator: core: Fix off-on-delay-us for always-on/boot-on regulators")
Cc: Christian Kohlschütter <christian@kohlschutter.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/core.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 4fcd36055b02..1490eb40c973 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1583,9 +1583,6 @@ static int set_machine_constraints(struct regulator_dev *rdev)
 			rdev->constraints->always_on = true;
 	}
 
-	if (rdev->desc->off_on_delay)
-		rdev->last_off = ktime_get_boottime();
-
 	/* If the constraints say the regulator should be on at this point
 	 * and we have control then make sure it is enabled.
 	 */
@@ -1619,6 +1616,8 @@ static int set_machine_constraints(struct regulator_dev *rdev)
 
 		if (rdev->constraints->always_on)
 			rdev->use_count++;
+	} else if (rdev->desc->off_on_delay) {
+		rdev->last_off = ktime_get();
 	}
 
 	print_constraints(rdev);
@@ -2668,7 +2667,7 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 
 	trace_regulator_enable(rdev_get_name(rdev));
 
-	if (rdev->desc->off_on_delay && rdev->last_off) {
+	if (rdev->desc->off_on_delay) {
 		/* if needed, keep a distance of off_on_delay from last time
 		 * this regulator was disabled.
 		 */
-- 
2.40.0.rc1.284.g88254d51c5-goog

[PATCH 2/2] regulator: fixed: Set PROBE_PREFER_ASYNCHRONOUS

[Douglas Anderson]

As of commit 218320fec294 ("regulator: core: Fix off-on-delay-us for
always-on/boot-on regulators"), we now might have a big delay during
probe of fixed regulators. That can have a significant boot speed
impact. Let's mitigate this by preferring async probe for fixed
regulators. The regulator framework itself has no issues with
regulators probing in an asynchronous way. The fixed regulator driver
is fairly straightforward and also has no issues.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/regulator/fixed.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 2a9867abba20..1927dc2d4cf8 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -334,6 +334,7 @@ static struct platform_driver regulator_fixed_voltage_driver = {
 	.probe		= reg_fixed_voltage_probe,
 	.driver		= {
 		.name		= "reg-fixed-voltage",
+		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
 		.of_match_table = of_match_ptr(fixed_of_match),
 	},
 };
-- 
2.40.0.rc1.284.g88254d51c5-goog

Re: [PATCH 2/2] regulator: fixed: Set PROBE_PREFER_ASYNCHRONOUS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 578 bytes --]

On Mon, Mar 13, 2023 at 11:18:20AM -0700, Douglas Anderson wrote:
> As of commit 218320fec294 ("regulator: core: Fix off-on-delay-us for
> always-on/boot-on regulators"), we now might have a big delay during
> probe of fixed regulators. That can have a significant boot speed
> impact. Let's mitigate this by preferring async probe for fixed
> regulators. The regulator framework itself has no issues with
> regulators probing in an asynchronous way. The fixed regulator driver
> is fairly straightforward and also has no issues.

This is going to be true for all regulators...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] regulator: fixed: Set PROBE_PREFER_ASYNCHRONOUS

[Doug Anderson]

Hi,

On Mon, Mar 13, 2023 at 12:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Mar 13, 2023 at 11:18:20AM -0700, Douglas Anderson wrote:
> > As of commit 218320fec294 ("regulator: core: Fix off-on-delay-us for
> > always-on/boot-on regulators"), we now might have a big delay during
> > probe of fixed regulators. That can have a significant boot speed
> > impact. Let's mitigate this by preferring async probe for fixed
> > regulators. The regulator framework itself has no issues with
> > regulators probing in an asynchronous way. The fixed regulator driver
> > is fairly straightforward and also has no issues.
>
> This is going to be true for all regulators...

Sure, that's true. So what are you suggesting?

There has always been a hope that someday we could just turn on async
probe everywhere. Folks at ChromeOS / Android at Google have looked at
this on and off over the years. Most recently +briannorris looked at
and described it as "no small task". There were a lot of kinks across
the various subsystems. I think +Saravana also has it on his wishlist
to tackle, too.

For the most part, the best we've been able to do is to identify
devices that happened to be involved in long delays at bootup and add
the flag to those devices after confirming that they worked OK. It's
not super elegant but it's pragmatic. That's what I've done here. If
there are other regulator drivers that are known to be involved in big
delays during probe time then we should add the flag there, too. Right
now, "off-on-delay" is only for fixed regulators, though there's no
real reason it shouldn't be applied to all regulators. I suppose some
of the other regulator related delays could affect pretty much any
regulator, though I guess normally those aren't associated with
always-on / boot-on regulators?

If you want, I can submit a pile of patches adding this flag to more
regulators. I did that in the past for mmc:

31ae403513be mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that are
newer than 5.4
d86472ae8b20 mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that existed in v5.4
a1a489197a07 mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that
existed in v4.19
7320915c8861 mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that
existed in v4.14
2a99f3fa85ea mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that existed in v4.9
21b2cec61c04 mmc: Set PROBE_PREFER_ASYNCHRONOUS for drivers that existed in v4.4


-Doug

Re: [PATCH 2/2] regulator: fixed: Set PROBE_PREFER_ASYNCHRONOUS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 582 bytes --]

On Mon, Mar 13, 2023 at 12:49:37PM -0700, Doug Anderson wrote:
> On Mon, Mar 13, 2023 at 12:05 PM Mark Brown <broonie@kernel.org> wrote:

> > This is going to be true for all regulators...

> Sure, that's true. So what are you suggesting?

That we shouldn't be making this change for just one driver, if you can
write an identical commit log for most if not all drivers but are just
making the change for one random driver then that suggests something is
being missed somewhere.  Either there's something special about this
driver or we should do things consistently.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (subset) [PATCH 0/2] regulator: Fix boot speed regression w/ off-on-delay-us

[Mark Brown]

On Mon, 13 Mar 2023 11:18:18 -0700, Douglas Anderson wrote:
> These two patches together fix a boot speed regression on
> sc7180-trogdor boards which use off-on-delay-us together with
> regulator-boot-on with the eDP power rail.
> 
> The first patch is _almost_ a revert of another patch. Depending on
> when the regulator is probed and the actual delay, the end result
> might be the same as a revert or might result in a delay that's
> somewhere between "no delay" and a full delay. As talked about in the
> patch, I believe this is the correct behavior and, if the problems
> fixed in the original patch are still present, I'm hoping that they
> can be fixed in a cleaner way.
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/2] regulator: core: Shorten off-on-delay-us for always-on/boot-on by time since booted
      commit: 691c1fcda5351ed98a44610b7dccc0e3ee920020

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH 2/2] regulator: fixed: Set PROBE_PREFER_ASYNCHRONOUS

[Doug Anderson]

Hi,

On Tue, Mar 14, 2023 at 6:29 AM Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Mar 13, 2023 at 12:49:37PM -0700, Doug Anderson wrote:
> > On Mon, Mar 13, 2023 at 12:05 PM Mark Brown <broonie@kernel.org> wrote:
>
> > > This is going to be true for all regulators...
>
> > Sure, that's true. So what are you suggesting?
>
> That we shouldn't be making this change for just one driver, if you can
> write an identical commit log for most if not all drivers but are just
> making the change for one random driver then that suggests something is
> being missed somewhere.  Either there's something special about this
> driver or we should do things consistently.

The "special" things are:

* It's been confirmed that this one random driver is involved in
slowing down boot a significant amount.

* The fixed regulator driver is among the simplest of the regulators
and doesn't have the complex interactions that are typically
associated with async probe problems.

For reference, some of the problems I'm aware of that have been seen
in the past when trying to enable async probe more broadly:

a) Apparently, on ACPI child devices aren't guaranteed to be probed
before parent devices once you turn on async probe. This is not
typically the case with device-tree probed devices where children show
up due to of_platform_populate() which is called by a parent device.
This can be handled properly but often isn't.

b) Some drivers try to poke directly at other devices and can get
confused if the other device is probing at the same time. One example
was dual-MIPI on Rockchip [1]. Again, this can be handled properly but
often isn't because nobody tested it.

c) Dynamically allocated ID numbers can change unpredictably from boot
to boot with async probe. This showed up on MMC where eMMC and SD
would change each boot between "mmcblk0" and "mmcblk1".

d) Async probe (obviously) changes timing and that can expose latent
bugs. Almost always those bugs should have been fixed anyway.

e) Async probe tends to stress out other driver's (consumers of the
device that's now probing async) error handling (since they are more
likely to see -EPROBE_DEFER) and can expose latent bugs there.


Let's take an example that I have a tiny bit of familiarity with:
max77686. This is not too strange of a device. It's implemented as a
MFD and has sub-drivers for PMIC (regulator), RTC, and clock.

I think we can be pretty confident that a) above isn't a problem. The
MFD driver (the parent) populates the regulator (the child) with
devm_mfd_add_devices() and it won't do that until the (MFD) parent is
ready. So far so good.

I think we can also be fairly certain that c) isn't a problem, and
probably it's not a problem anywhere for regulators. Nobody (that I'm
aware of) relies on the stability of a dynamically allocated number
for a regulator.

Issues d) and e) could be an issue for max77686, but in almost all
cases where they are we'd want to fix those latent bugs anyway since
they could have hit even without async probe.

...but b) _could_ be a problem. Specifically, today I think that
(unless some of its supplies aren't available at probe time) the PMIC
driver will _always_ finish probing before the RTC/clock driver
because of the order specified in the source code:

  { .name = "max77686-pmic", },
  { .name = "max77686-rtc", },
  { .name = "max77686-clk", },

One would need to do deeper code inspection (and, ideally, testing) to
find out if indeed the RTC driver and clock driver will have problems
if the regulator is not finished probing before their probes start.
Hopefully everything here is fine, but it's the kind of place where
maybe somebody was sloppy because, today, the order is guaranteed. In
general drivers for multi-function devices are more likely to interact
directly with sibling drivers outside of the normal driver framework
and (in my experience) are more likely to be relying on probe order.

If someone wanted to carefully inspect the max77686 code (especially
the interactions between all the MFD sub-drivers) and test enabling
async probe there then it would be a nice improvement. ...switching
over and crossing our fingers might work OK but it also might not.


If we look at the fixed regulator driver and compare, we're in a
simpler state. Sure, we could still expose latent timing bugs or
latent wrong error handling in regulator consumers, but those should
be fixed anyway. The fixed regulator driver doesn't reach directly
into other devices through private APIs and pretty much just relies on
the regulator core for most of its work. The regulator core should be
async-robust because of needing to deal with regulators that show up
as modules.


I guess the last thing I'll say is that PROBE_PREFER_ASYNCHRONOUS was
added specifically so that we could enable this on drivers that were
found to be slow to boot and that were tested to work with async
probe. Without being able to add PROBE_PREFER_ASYNCHRONOUS people were
open-coding solutions per driver to speed probe.


[1] https://lore.kernel.org/r/20221019170255.2.I6b985b0ca372b7e35c6d9ea970b24bcb262d4fc1@changeid

Re: [PATCH 2/2] regulator: fixed: Set PROBE_PREFER_ASYNCHRONOUS

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3503 bytes --]

On Tue, Mar 14, 2023 at 09:35:28AM -0700, Doug Anderson wrote:

> The "special" things are:

> * It's been confirmed that this one random driver is involved in
> slowing down boot a significant amount.

> * The fixed regulator driver is among the simplest of the regulators
> and doesn't have the complex interactions that are typically
> associated with async probe problems.

I would be surprised to see regulators being in that category.

> For reference, some of the problems I'm aware of that have been seen
> in the past when trying to enable async probe more broadly:

> a) Apparently, on ACPI child devices aren't guaranteed to be probed
> before parent devices once you turn on async probe. This is not
> typically the case with device-tree probed devices where children show
> up due to of_platform_populate() which is called by a parent device.
> This can be handled properly but often isn't.

That seems unlikely to be an issue here.

> b) Some drivers try to poke directly at other devices and can get
> confused if the other device is probing at the same time. One example
> was dual-MIPI on Rockchip [1]. Again, this can be handled properly but
> often isn't because nobody tested it.

Again, unlikely to be an issue for regulators - where multiple
regulators interact with each other that's already coordinated through
the core.

> c) Dynamically allocated ID numbers can change unpredictably from boot
> to boot with async probe. This showed up on MMC where eMMC and SD
> would change each boot between "mmcblk0" and "mmcblk1".

No numbers exposed to userspace here so that'll be fine.

> d) Async probe (obviously) changes timing and that can expose latent
> bugs. Almost always those bugs should have been fixed anyway.

> e) Async probe tends to stress out other driver's (consumers of the
> device that's now probing async) error handling (since they are more
> likely to see -EPROBE_DEFER) and can expose latent bugs there.

Right, not sure I'd worry about either of those though and given how
widley used fixed regualtors are I'd expect it to be one of the riskiest
here.

> ...but b) _could_ be a problem. Specifically, today I think that
> (unless some of its supplies aren't available at probe time) the PMIC
> driver will _always_ finish probing before the RTC/clock driver
> because of the order specified in the source code:

>   { .name = "max77686-pmic", },
>   { .name = "max77686-rtc", },
>   { .name = "max77686-clk", },

> One would need to do deeper code inspection (and, ideally, testing) to
> find out if indeed the RTC driver and clock driver will have problems
> if the regulator is not finished probing before their probes start.

Behind the scenes interactions would be quite unusual, the power demands
of the on chip devices are typically minimal so there's not much to do -
it's typically all always on functions.  Could happen of course but it'd
be surprising.

> I guess the last thing I'll say is that PROBE_PREFER_ASYNCHRONOUS was
> added specifically so that we could enable this on drivers that were
> found to be slow to boot and that were tested to work with async
> probe. Without being able to add PROBE_PREFER_ASYNCHRONOUS people were
> open-coding solutions per driver to speed probe.

Sure, but my point is that at least every regulator with a ramp time
(which will be most of them, even those that don't have one they tell
the kernel about one probably should) is going to have a similar issue
so could probably benefit from a similar solution.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]