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

Thu Jun 27 12:52:44 CEST 2024

Hi Svyatoslav,

On Thu, 27 Jun 2024 at 11:34, Svyatoslav Ryhel <clamor95 at gmail.com> wrote:
>
> чт, 27 черв. 2024 р. о 12:26 Simon Glass <sjg at chromium.org> пише:
> >
> > Hi Svyatoslav,
> >
> > On Thu, 27 Jun 2024 at 10:09, Svyatoslav <clamor95 at gmail.com> wrote:
> > >
> > >
> > >
> > > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly <caleb.connolly at linaro.org> написав(-ла):
> > > >
> > > >
> > > >On 27/06/2024 10:37, Simon Glass wrote:
> > > >> Hi Marek,
> > > >>
> > > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex at 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 <marex at denx.de>
> > > >>> ---
> > > >>> Cc: Ben Wolsieffer <benwolsieffer at gmail.com>
> > > >>> Cc: Caleb Connolly <caleb.connolly at linaro.org>
> > > >>> Cc: Chris Morgan <macromorgan at hotmail.com>
> > > >>> Cc: Dragan Simic <dsimic at manjaro.org>
> > > >>> Cc: Eugen Hristev <eugen.hristev at collabora.com>
> > > >>> Cc: Francesco Dolcini <francesco.dolcini at toradex.com>
> > > >>> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> > > >>> Cc: Jaehoon Chung <jh80.chung at samsung.com>
> > > >>> Cc: Jagan Teki <jagan at amarulasolutions.com>
> > > >>> Cc: Jonas Karlman <jonas at kwiboo.se>
> > > >>> Cc: Kever Yang <kever.yang at rock-chips.com>
> > > >>> Cc: Kostya Porotchkin <kostap at marvell.com>
> > > >>> Cc: Matteo Lisi <matteo.lisi at engicam.com>
> > > >>> Cc: Mattijs Korpershoek <mkorpershoek at baylibre.com>
> > > >>> Cc: Max Krummenacher <max.krummenacher at toradex.com>
> > > >>> Cc: Neil Armstrong <neil.armstrong at linaro.org>
> > > >>> Cc: Patrice Chotard <patrice.chotard at foss.st.com>
> > > >>> Cc: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > > >>> Cc: Philipp Tomsich <philipp.tomsich at vrull.eu>
> > > >>> Cc: Quentin Schulz <quentin.schulz at cherry.de>
> > > >>> Cc: Sam Day <me at samcday.com>
> > > >>> Cc: Simon Glass <sjg at chromium.org>
> > > >>> Cc: Sumit Garg <sumit.garg at linaro.org>
> > > >>> Cc: Svyatoslav Ryhel <clamor95 at gmail.com>
> > > >>> Cc: Thierry Reding <treding at nvidia.com>
> > > >>> Cc: Tom Rini <trini at konsulko.com>
> > > >>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk at epam.com>
> > > >>> Cc: u-boot-amlogic at groups.io
> > > >>> Cc: u-boot-qcom at groups.io
> > > >>> Cc: u-boot at dh-electronics.com
> > > >>> Cc: u-boot at lists.denx.de
> > > >>> Cc: uboot-stm32 at 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?
> > >
> > > Not so long ago I proposed a similar patchset with the same goal
> > > and I have discovered massive issues with SPL and relocation
> > > interfering with driver loading.
> > >
> > > The issue which I have personally encountered was i2c driver failure
> > > due to double probing. This behavior was triggered by
> > > always-on/boot-on regulators provided by pmic which in most
> > > cases is an i2c device.
> > >
> > > At that time everyone just ignored me, so idk if tegra i2c is the only
> > > driver which has this response or there are more, but be aware that
> > > this patch set may cause cascade failure on many devices.
> >
> > I'm not sure if I remember this, but I can certainly see some problems
> > with it. Did we have drivers that probed in the bind() function,
> > perhaps?
> >
>
> It is expected not to remember all patchsets sent, not an issue. As for
> drivers probed after bind there are several, but they are quite essential.
> Core GPIO and pinmux drivers are probed as early as possible but in most
> cases they have no dependencies among complex subsystems (like i2c).

OK, well I suppose that you managed to find a solution.

>From my side I just want to avoid doing too much such stuff
'automatically' in driver model. As you say, it can lead to difficult
race conditions / bugs.

Regards,
Simon