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

[Tom Rini]

On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote:
> пн, 16 вер. 2024 р. о 19:28 Tom Rini <trini at konsulko.com> пише:
> >
> > On Wed, Sep 11, 2024 at 07:00:56PM -0600, Simon Glass wrote:
> > > Hi Marek,
> > >
> > > On Fri, 28 Jun 2024 at 07:26, Marek Vasut <marex at denx.de> wrote:
> > > >
> > > > On 6/28/24 9:32 AM, Simon Glass wrote:
> > > > > Hi Marek,
> > > >
> > > > Hi,
> > > >
> > > > [...]
> > > >
> > > > >>>> @@ -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?
> > > >
> > > > I would assume that if those regulators are turned on once in the
> > > > earliest stage , turning them on again in the follow up stage would be a
> > > > noop ? This is the point of regulator-*-on, to keep the regulators on.
> > >
> > > No, there is sometimes a particular sequence needed.
> > >
> > > >
> > > > >>> 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.
> > > >
> > > > Which patch ? My recollection of DM_FLAG_PROBE_AFTER_BIND was that it
> > > > took a while to get in.
> > >
> > > [1]
> > >
> > > >
> > > > > 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?
> > > >
> > > > Did we finally arrive at the point where we need -EPROBE_DEFER alike
> > > > mechanism ?
> > >
> > > Nope. But I am concerned that this patch is leading us there. bind()
> > > really needs to be as clean as possible. It is one of the design
> > > elements of driver model which Linux should adopt.
> > >
> > > There is always a race to be the first to init something, move the
> > > init earlier, etc... I don't see any general need to init the
> > > regulators right at the start. It should be done in a separate
> > > function and be optional. I am happy to send a patch if you like.
> >
> > Since we're currently stuck on the point where Marek has patches that
> > fix a real problem, and Svyatoslav has a problem with them, but isn't
> > currently able to debug it, yes, please put forward your patch that
> > might address both sets of problems so we can all figure out how to
> > resolve the problems, thanks!
> 
> With patches from Marek there is no i2c chip probe of PMIC, while
> without i2c chip probe is called (probe_chip function). How this is
> even possible?

Yes, it's very puzzling. Do you have the ability to get some debug
console type information out so you can sprinkle in some prints and
figure it out?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240920/a6500a8f/attachment.sig>