Hi Caleb, On Thu, 27 Jun 2024 at 09:48, Caleb Connolly <caleb.conno...@linaro.org> wrote: > > > > 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.
That seems like a great idea to me, in general. The fact that SPL sets up the MMU on armv8 makes it more practical. But for this series I believe we are going to have to define what happens in what phase. We have power_init_board() which is the old way of doing this...but perhaps we could use that as a way to start up regulators which are needed. As to my question about whether this happens in SPL / pre-reloc / proper, I forgot that we have the bootph tags for that, so it should be fine. The main issue is that in U-Boot proper we will re-init the regulators even though that has already been done. Probably that can be handled by Kconfig or a flag in SPL handoff. > > > > 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