Hi Svyatoslav, On Thu, 27 Jun 2024 at 10:09, Svyatoslav <[email protected]> wrote: > > > > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly > <[email protected]> написав(-ла): > > > > > >On 27/06/2024 10:37, Simon Glass wrote: > >> Hi Marek, > >> > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut <[email protected]> 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 <[email protected]> > >>> --- > >>> Cc: Ben Wolsieffer <[email protected]> > >>> Cc: Caleb Connolly <[email protected]> > >>> Cc: Chris Morgan <[email protected]> > >>> Cc: Dragan Simic <[email protected]> > >>> Cc: Eugen Hristev <[email protected]> > >>> Cc: Francesco Dolcini <[email protected]> > >>> Cc: Heinrich Schuchardt <[email protected]> > >>> Cc: Jaehoon Chung <[email protected]> > >>> Cc: Jagan Teki <[email protected]> > >>> Cc: Jonas Karlman <[email protected]> > >>> Cc: Kever Yang <[email protected]> > >>> Cc: Kostya Porotchkin <[email protected]> > >>> Cc: Matteo Lisi <[email protected]> > >>> Cc: Mattijs Korpershoek <[email protected]> > >>> Cc: Max Krummenacher <[email protected]> > >>> Cc: Neil Armstrong <[email protected]> > >>> Cc: Patrice Chotard <[email protected]> > >>> Cc: Patrick Delaunay <[email protected]> > >>> Cc: Philipp Tomsich <[email protected]> > >>> Cc: Quentin Schulz <[email protected]> > >>> Cc: Sam Day <[email protected]> > >>> Cc: Simon Glass <[email protected]> > >>> Cc: Sumit Garg <[email protected]> > >>> Cc: Svyatoslav Ryhel <[email protected]> > >>> Cc: Thierry Reding <[email protected]> > >>> Cc: Tom Rini <[email protected]> > >>> Cc: Volodymyr Babchuk <[email protected]> > >>> Cc: [email protected] > >>> Cc: [email protected] > >>> Cc: [email protected] > >>> Cc: [email protected] > >>> Cc: [email protected] > >>> --- > >>> 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? > > Best regards, > Svyatoslav R. > > >> > >> Should we have a step in the init sequence where we set up the > >> regulators, by calling regulators_enable_boot_on() ? > >> > >> Regards, > >> Simon > >
