Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on

[Caleb Connolly]

On 27/06/2024 10:37, Simon Glass wrote:
Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut <ma...@denx.de> wrote:

In case a regulator DT node contains regulator-always-on or regulator-boot-on
property, make sure the regulator gets correctly configured by U-Boot on start
up. Unconditionally probe such regulator drivers. This is a preparatory patch
for introduction of .regulator_post_probe() which would trigger the regulator
configuration.

Parsing of regulator-always-on and regulator-boot-on DT property has been
moved to regulator_post_bind() as the information is required early, the
rest of the DT parsing has been kept in regulator_pre_probe() to avoid
slowing down the boot process.

Signed-off-by: Marek Vasut <ma...@denx.de>
---
Cc: Ben Wolsieffer <benwolsief...@gmail.com>
Cc: Caleb Connolly <caleb.conno...@linaro.org>
Cc: Chris Morgan <macromor...@hotmail.com>
Cc: Dragan Simic <dsi...@manjaro.org>
Cc: Eugen Hristev <eugen.hris...@collabora.com>
Cc: Francesco Dolcini <francesco.dolc...@toradex.com>
Cc: Heinrich Schuchardt <xypron.g...@gmx.de>
Cc: Jaehoon Chung <jh80.ch...@samsung.com>
Cc: Jagan Teki <ja...@amarulasolutions.com>
Cc: Jonas Karlman <jo...@kwiboo.se>
Cc: Kever Yang <kever.y...@rock-chips.com>
Cc: Kostya Porotchkin <kos...@marvell.com>
Cc: Matteo Lisi <matteo.l...@engicam.com>
Cc: Mattijs Korpershoek <mkorpersh...@baylibre.com>
Cc: Max Krummenacher <max.krummenac...@toradex.com>
Cc: Neil Armstrong <neil.armstr...@linaro.org>
Cc: Patrice Chotard <patrice.chot...@foss.st.com>
Cc: Patrick Delaunay <patrick.delau...@foss.st.com>
Cc: Philipp Tomsich <philipp.toms...@vrull.eu>
Cc: Quentin Schulz <quentin.sch...@cherry.de>
Cc: Sam Day <m...@samcday.com>
Cc: Simon Glass <s...@chromium.org>
Cc: Sumit Garg <sumit.g...@linaro.org>
Cc: Svyatoslav Ryhel <clamo...@gmail.com>
Cc: Thierry Reding <tred...@nvidia.com>
Cc: Tom Rini <tr...@konsulko.com>
Cc: Volodymyr Babchuk <volodymyr_babc...@epam.com>
Cc: u-boot-amlo...@groups.io
Cc: u-boot-q...@groups.io
Cc: u-b...@dh-electronics.com
Cc: u-boot@lists.denx.de
Cc: uboot-st...@st-md-mailman.stormreply.com
---
  drivers/power/regulator/regulator-uclass.c | 22 +++++++++++++++-------
  1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/power/regulator/regulator-uclass.c 
b/drivers/power/regulator/regulator-uclass.c
index 66fd531da04..ccc4ef33d83 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
         const char *property = "regulator-name";

         uc_pdata = dev_get_uclass_plat(dev);
+       uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
+       uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");

         /* Regulator's mandatory constraint */
         uc_pdata->name = dev_read_string(dev, property);
@@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
                         return -EINVAL;
         }

-       if (regulator_name_is_unique(dev, uc_pdata->name))
-               return 0;
+       if (!regulator_name_is_unique(dev, uc_pdata->name)) {
+               debug("'%s' of dev: '%s', has nonunique value: '%s\n",
+                     property, dev->name, uc_pdata->name);
+               return -EINVAL;
+       }

-       debug("'%s' of dev: '%s', has nonunique value: '%s\n",
-             property, dev->name, uc_pdata->name);
+       /*
+        * In case the regulator has regulator-always-on or
+        * regulator-boot-on DT property, trigger probe() to
+        * configure its default state during startup.
+        */
+       if (uc_pdata->always_on && uc_pdata->boot_on)
+               dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);

-       return -EINVAL;
+       return 0;
  }

  static int regulator_pre_probe(struct udevice *dev)
@@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
                                                 -ENODATA);
         uc_pdata->max_uA = dev_read_u32_default(dev, "regulator-max-microamp",
                                                 -ENODATA);
-       uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
-       uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
         uc_pdata->ramp_delay = dev_read_u32_default(dev, 
"regulator-ramp-delay",
                                                     0);
         uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
--
2.43.0


This is reading a lot of DT stuff very early, which may be slow. It is
also reading from the DT in the bind() step which we sometimes have to
do, but try to avoid.

Could we set up the livetree pre-bind? What about MMU? On armv8 at least 
this would have a huge impact on performance. I've done some 
measurements and there is at least 1 order of magnitude difference 
between parsing FDT with no caches vs parsing livetree with, it's huge.

Also this seems to happen in SPL and again pre-reloc and again in
U-Boot post-reloc?

Should we have a step in the init sequence where we set up the
regulators, by calling regulators_enable_boot_on() ?

Regards,
Simon

--
// Caleb (they/them)