Hi Marek,
On Thu, 27 Jun 2024 at 17:05, Marek Vasut <ma...@denx.de> wrote:
>
> On 6/27/24 10:37 AM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> [...]
>
> >> ---
> >> 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.
>
> Actually, it is reading only the bare minimum very early in bind, the
> always-on and boot-on, the rest is in pre_probe, i.e. later.
Yes, I see that. Also it is inevitable that these properties need to
be read before probe(), since they control whether to probe().
>
> > Also this seems to happen in SPL and again pre-reloc and again in
> > U-Boot post-reloc?
>
> What does, the uclass post_bind ?
I mean that this code will be called in SPL (if the regulators are in
the DT there), U-Boot pre-reloc and post-reloc, each time turning on
the regulators. We need a way to control that, don't we?
>
> > Should we have a step in the init sequence where we set up the
> > regulators, by calling regulators_enable_boot_on() ?
>
> Let's not do this , the entire point of this series is to get rid of
> those functions and do the regulator configuration the same way LED
> subsystem does it -- by probing always-on/boot-on regulators and
> configuring them correctly on probe.
>
> To me regulators_enable_boot_on() and the like was always an
> inconsistently applied workaround for missing DM_FLAG_PROBE_AFTER_BIND ,
> which is now implemented, so such workarounds can be removed.
That patch seemed to slip under the radar, sent and applied on the
same day! We really need to add a test for it, BTW.
My concern is that this might cause us ordering problems. We perhaps
want the regulators to be done first. If drivers are probed which use
regulators, then presumably they will enable them. Consider this:
- LED driver auto-probes
- probes I2C bus 2
- probes LDO1 which is autoset so turns on
- LDO1 probes, but is already running
- LDO2 probes, which is autoset so turns on
So long as it is OK to enable the regulators in any order, then this
seems fine. But is it? How do we handle the case where are particular
sequence is needed?
Regards,
Simon
