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 ?
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
Hi Simon, 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. Yes...it seems that enabling cache in SPL has become common on armv8. As to the memory layout, I'm not sure what is happening there, but it seems that the DT does not describe it in general (at least not until U-Boot adds the nodes). I suppose this depends on the platform. On Qualcomm we use DT as the source of truth as it lets us support many platforms (with totally different memory maps) with a single U-Boot binary, at least for development this is quite nice. 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. OK...I wonder how we can solve this. Needs some though. 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)
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
Hi Marek, On Thu, 27 Jun 2024 at 17:05, Marek Vasut 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
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
Hi Caleb, On Fri, 28 Jun 2024 at 01:09, Caleb Connolly wrote: > > > > On 27/06/2024 11:26, Simon Glass wrote: > > Hi Caleb, > > > > On Thu, 27 Jun 2024 at 09:48, Caleb Connolly > > wrote: > >> > >> > >> > >> On 27/06/2024 10:37, Simon Glass wrote: > >>> Hi Marek, > >>> > >>> On Thu, 27 Jun 2024 at 00:57, Marek Vasut 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 > --- > Cc: Ben Wolsieffer > Cc: Caleb Connolly > Cc: Chris Morgan > Cc: Dragan Simic > Cc: Eugen Hristev > Cc: Francesco Dolcini > Cc: Heinrich Schuchardt > Cc: Jaehoon Chung > Cc: Jagan Teki > Cc: Jonas Karlman > Cc: Kever Yang > Cc: Kostya Porotchkin > Cc: Matteo Lisi > Cc: Mattijs Korpershoek > Cc: Max Krummenacher > Cc: Neil Armstrong > Cc: Patrice Chotard > Cc: Patrick Delaunay > Cc: Philipp Tomsich > Cc: Quentin Schulz > Cc: Sam Day > Cc: Simon Glass > Cc: Sumit Garg > Cc: Svyatoslav Ryhel > Cc: Thierry Reding > Cc: Tom Rini > Cc: Volodymyr Babchuk > Cc: u-boot-amlo...@groups.io > Cc: u-boot-q...@groups.io > Cc: u-b...@dh-electronics.com > Cc: u-boot@lists.denx.de > Cc: uboot-st...@st-md-mailman.stormreply.com > --- > 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
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 wrote: On 27/06/2024 10:37, Simon Glass wrote: Hi Marek, On Thu, 27 Jun 2024 at 00:57, Marek Vasut 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 --- Cc: Ben Wolsieffer Cc: Caleb Connolly Cc: Chris Morgan Cc: Dragan Simic Cc: Eugen Hristev Cc: Francesco Dolcini Cc: Heinrich Schuchardt Cc: Jaehoon Chung Cc: Jagan Teki Cc: Jonas Karlman Cc: Kever Yang Cc: Kostya Porotchkin Cc: Matteo Lisi Cc: Mattijs Korpershoek Cc: Max Krummenacher Cc: Neil Armstrong Cc: Patrice Chotard Cc: Patrick Delaunay Cc: Philipp Tomsich Cc: Quentin Schulz Cc: Sam Day Cc: Simon Glass Cc: Sumit Garg Cc: Svyatoslav Ryhel Cc: Thierry Reding Cc: Tom Rini Cc: Volodymyr Babchuk Cc: u-boot-amlo...@groups.io Cc: u-boot-q...@groups.io Cc: u-b...@dh-electronics.com Cc: u-boot@lists.denx.de Cc: uboot-st...@st-md-mailman.stormreply.com --- 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
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
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. Also this seems to happen in SPL and again pre-reloc and again in U-Boot post-reloc? What does, the uclass post_bind ? 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.
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
Hi Svyatoslav, On Thu, 27 Jun 2024 at 11:34, Svyatoslav Ryhel wrote: > > чт, 27 черв. 2024 р. о 12:26 Simon Glass пише: > > > > Hi Svyatoslav, > > > > On Thu, 27 Jun 2024 at 10:09, Svyatoslav wrote: > > > > > > > > > > > > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly > > > написав(-ла): > > > > > > > > > > > >On 27/06/2024 10:37, Simon Glass wrote: > > > >> Hi Marek, > > > >> > > > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut 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 > > > >>> --- > > > >>> Cc: Ben Wolsieffer > > > >>> Cc: Caleb Connolly > > > >>> Cc: Chris Morgan > > > >>> Cc: Dragan Simic > > > >>> Cc: Eugen Hristev > > > >>> Cc: Francesco Dolcini > > > >>> Cc: Heinrich Schuchardt > > > >>> Cc: Jaehoon Chung > > > >>> Cc: Jagan Teki > > > >>> Cc: Jonas Karlman > > > >>> Cc: Kever Yang > > > >>> Cc: Kostya Porotchkin > > > >>> Cc: Matteo Lisi > > > >>> Cc: Mattijs Korpershoek > > > >>> Cc: Max Krummenacher > > > >>> Cc: Neil Armstrong > > > >>> Cc: Patrice Chotard > > > >>> Cc: Patrick Delaunay > > > >>> Cc: Philipp Tomsich > > > >>> Cc: Quentin Schulz > > > >>> Cc: Sam Day > > > >>> Cc: Simon Glass > > > >>> Cc: Sumit Garg > > > >>> Cc: Svyatoslav Ryhel > > > >>> Cc: Thierry Reding > > > >>> Cc: Tom Rini > > > >>> Cc: Volodymyr Babchuk > > > >>> Cc: u-boot-amlo...@groups.io > > > >>> Cc: u-boot-q...@groups.io > > > >>> Cc: u-b...@dh-electronics.com > > > >>> Cc: u-boot@lists.denx.de > > > >>> Cc: uboot-st...@st-md-mailman.stormreply.com > > > >>> --- > > > >>> 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 =
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
чт, 27 черв. 2024 р. о 12:26 Simon Glass пише: > > Hi Svyatoslav, > > On Thu, 27 Jun 2024 at 10:09, Svyatoslav wrote: > > > > > > > > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly > > написав(-ла): > > > > > > > > >On 27/06/2024 10:37, Simon Glass wrote: > > >> Hi Marek, > > >> > > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut 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 > > >>> --- > > >>> Cc: Ben Wolsieffer > > >>> Cc: Caleb Connolly > > >>> Cc: Chris Morgan > > >>> Cc: Dragan Simic > > >>> Cc: Eugen Hristev > > >>> Cc: Francesco Dolcini > > >>> Cc: Heinrich Schuchardt > > >>> Cc: Jaehoon Chung > > >>> Cc: Jagan Teki > > >>> Cc: Jonas Karlman > > >>> Cc: Kever Yang > > >>> Cc: Kostya Porotchkin > > >>> Cc: Matteo Lisi > > >>> Cc: Mattijs Korpershoek > > >>> Cc: Max Krummenacher > > >>> Cc: Neil Armstrong > > >>> Cc: Patrice Chotard > > >>> Cc: Patrick Delaunay > > >>> Cc: Philipp Tomsich > > >>> Cc: Quentin Schulz > > >>> Cc: Sam Day > > >>> Cc: Simon Glass > > >>> Cc: Sumit Garg > > >>> Cc: Svyatoslav Ryhel > > >>> Cc: Thierry Reding > > >>> Cc: Tom Rini > > >>> Cc: Volodymyr Babchuk > > >>> Cc: u-boot-amlo...@groups.io > > >>> Cc: u-boot-q...@groups.io > > >>> Cc: u-b...@dh-electronics.com > > >>> Cc: u-boot@lists.denx.de > > >>> Cc: uboot-st...@st-md-mailman.stormreply.com > > >>> --- > > >>> 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
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
Hi Caleb, On Thu, 27 Jun 2024 at 09:48, Caleb Connolly wrote: > > > > On 27/06/2024 10:37, Simon Glass wrote: > > Hi Marek, > > > > On Thu, 27 Jun 2024 at 00:57, Marek Vasut 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 > >> --- > >> Cc: Ben Wolsieffer > >> Cc: Caleb Connolly > >> Cc: Chris Morgan > >> Cc: Dragan Simic > >> Cc: Eugen Hristev > >> Cc: Francesco Dolcini > >> Cc: Heinrich Schuchardt > >> Cc: Jaehoon Chung > >> Cc: Jagan Teki > >> Cc: Jonas Karlman > >> Cc: Kever Yang > >> Cc: Kostya Porotchkin > >> Cc: Matteo Lisi > >> Cc: Mattijs Korpershoek > >> Cc: Max Krummenacher > >> Cc: Neil Armstrong > >> Cc: Patrice Chotard > >> Cc: Patrick Delaunay > >> Cc: Philipp Tomsich > >> Cc: Quentin Schulz > >> Cc: Sam Day > >> Cc: Simon Glass > >> Cc: Sumit Garg > >> Cc: Svyatoslav Ryhel > >> Cc: Thierry Reding > >> Cc: Tom Rini > >> Cc: Volodymyr Babchuk > >> Cc: u-boot-amlo...@groups.io > >> Cc: u-boot-q...@groups.io > >> Cc: u-b...@dh-electronics.com > >> Cc: u-boot@lists.denx.de > >> Cc: uboot-st...@st-md-mailman.stormreply.com > >> --- > >> 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
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
Hi Svyatoslav, On Thu, 27 Jun 2024 at 10:09, Svyatoslav wrote: > > > > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly > написав(-ла): > > > > > >On 27/06/2024 10:37, Simon Glass wrote: > >> Hi Marek, > >> > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut 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 > >>> --- > >>> Cc: Ben Wolsieffer > >>> Cc: Caleb Connolly > >>> Cc: Chris Morgan > >>> Cc: Dragan Simic > >>> Cc: Eugen Hristev > >>> Cc: Francesco Dolcini > >>> Cc: Heinrich Schuchardt > >>> Cc: Jaehoon Chung > >>> Cc: Jagan Teki > >>> Cc: Jonas Karlman > >>> Cc: Kever Yang > >>> Cc: Kostya Porotchkin > >>> Cc: Matteo Lisi > >>> Cc: Mattijs Korpershoek > >>> Cc: Max Krummenacher > >>> Cc: Neil Armstrong > >>> Cc: Patrice Chotard > >>> Cc: Patrick Delaunay > >>> Cc: Philipp Tomsich > >>> Cc: Quentin Schulz > >>> Cc: Sam Day > >>> Cc: Simon Glass > >>> Cc: Sumit Garg > >>> Cc: Svyatoslav Ryhel > >>> Cc: Thierry Reding > >>> Cc: Tom Rini > >>> Cc: Volodymyr Babchuk > >>> Cc: u-boot-amlo...@groups.io > >>> Cc: u-boot-q...@groups.io > >>> Cc: u-b...@dh-electronics.com > >>> Cc: u-boot@lists.denx.de > >>> Cc: uboot-st...@st-md-mailman.stormreply.com > >>> --- > >>> 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
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly написав(-ла): > > >On 27/06/2024 10:37, Simon Glass wrote: >> Hi Marek, >> >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut 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 >>> --- >>> Cc: Ben Wolsieffer >>> Cc: Caleb Connolly >>> Cc: Chris Morgan >>> Cc: Dragan Simic >>> Cc: Eugen Hristev >>> Cc: Francesco Dolcini >>> Cc: Heinrich Schuchardt >>> Cc: Jaehoon Chung >>> Cc: Jagan Teki >>> Cc: Jonas Karlman >>> Cc: Kever Yang >>> Cc: Kostya Porotchkin >>> Cc: Matteo Lisi >>> Cc: Mattijs Korpershoek >>> Cc: Max Krummenacher >>> Cc: Neil Armstrong >>> Cc: Patrice Chotard >>> Cc: Patrick Delaunay >>> Cc: Philipp Tomsich >>> Cc: Quentin Schulz >>> Cc: Sam Day >>> Cc: Simon Glass >>> Cc: Sumit Garg >>> Cc: Svyatoslav Ryhel >>> Cc: Thierry Reding >>> Cc: Tom Rini >>> Cc: Volodymyr Babchuk >>> Cc: u-boot-amlo...@groups.io >>> Cc: u-boot-q...@groups.io >>> Cc: u-b...@dh-electronics.com >>> Cc: u-boot@lists.denx.de >>> Cc: uboot-st...@st-md-mailman.stormreply.com >>> --- >>> 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? Not so long ago I proposed a similar patchset with the same goal and I have discovered massive issues with
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 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 --- Cc: Ben Wolsieffer Cc: Caleb Connolly Cc: Chris Morgan Cc: Dragan Simic Cc: Eugen Hristev Cc: Francesco Dolcini Cc: Heinrich Schuchardt Cc: Jaehoon Chung Cc: Jagan Teki Cc: Jonas Karlman Cc: Kever Yang Cc: Kostya Porotchkin Cc: Matteo Lisi Cc: Mattijs Korpershoek Cc: Max Krummenacher Cc: Neil Armstrong Cc: Patrice Chotard Cc: Patrick Delaunay Cc: Philipp Tomsich Cc: Quentin Schulz Cc: Sam Day Cc: Simon Glass Cc: Sumit Garg Cc: Svyatoslav Ryhel Cc: Thierry Reding Cc: Tom Rini Cc: Volodymyr Babchuk Cc: u-boot-amlo...@groups.io Cc: u-boot-q...@groups.io Cc: u-b...@dh-electronics.com Cc: u-boot@lists.denx.de Cc: uboot-st...@st-md-mailman.stormreply.com --- 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)
Re: [PATCH 1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on
Hi Marek, On Thu, 27 Jun 2024 at 00:57, Marek Vasut 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 > --- > Cc: Ben Wolsieffer > Cc: Caleb Connolly > Cc: Chris Morgan > Cc: Dragan Simic > Cc: Eugen Hristev > Cc: Francesco Dolcini > Cc: Heinrich Schuchardt > Cc: Jaehoon Chung > Cc: Jagan Teki > Cc: Jonas Karlman > Cc: Kever Yang > Cc: Kostya Porotchkin > Cc: Matteo Lisi > Cc: Mattijs Korpershoek > Cc: Max Krummenacher > Cc: Neil Armstrong > Cc: Patrice Chotard > Cc: Patrick Delaunay > Cc: Philipp Tomsich > Cc: Quentin Schulz > Cc: Sam Day > Cc: Simon Glass > Cc: Sumit Garg > Cc: Svyatoslav Ryhel > Cc: Thierry Reding > Cc: Tom Rini > Cc: Volodymyr Babchuk > Cc: u-boot-amlo...@groups.io > Cc: u-boot-q...@groups.io > Cc: u-b...@dh-electronics.com > Cc: u-boot@lists.denx.de > Cc: uboot-st...@st-md-mailman.stormreply.com > --- > 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. 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