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

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