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

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.

>>> 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.

> 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 ?