чт, 27 черв. 2024 р. о 12:26 Simon Glass <[email protected]> пише: > > 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? >
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). > > > > 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 > > >
