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

On 27/06/2024 11:26, Simon Glass wrote:
> Hi Caleb,
> 
> On Thu, 27 Jun 2024 at 09:48, Caleb Connolly <[email protected]> wrote:
>>
>>
>>
>> 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.
> 
> That seems like a great idea to me, in general. The fact that SPL sets
> up the MMU on armv8 makes it more practical.

Well, on qcom we don't use SPL (yet?), we did have a cyclical dependency 
since we rely on DTB for the memory layout, although I have some patches 
to do all the memory parsing in board_fdt_blob_setup() since that's 
needed for multi-dtb FIT. So I guess we could enable caches at the same 
time.
> 
> But for this series I believe we are going to have to define what
> happens in what phase. We have power_init_board() which is the old way
> of doing this...but perhaps we could use that as a way to start up
> regulators which are needed.
> 
> As to my question about whether this happens in SPL / pre-reloc /
> proper, I forgot that we have the bootph tags for that, so it should
> be fine. The main issue is that in U-Boot proper we will re-init the
> regulators even though that has already been done. Probably that can
> be handled by Kconfig or a flag in SPL handoff.

Ensuring that it isn't done multiple times sounds like the right 
approach to me.
> 
> 
>>>
>>> Also this seems to happen in SPL and again pre-reloc and again in
>>> U-Boot post-reloc?
>>>
>>> Should we have a step in the init sequence where we set up the
>>> regulators, by calling regulators_enable_boot_on() ?
> 
> Regards,
> Simon

-- 
// Caleb (they/them)