开云体育

In case a regulator DT node contains regulator-always-on or regulator-boo=
t-on
property, make sure the regulator gets correctly configured by U-Boot on =
start
up. Unconditionally probe such regulator drivers. This is a preparatory p=
atch
for introduction of .regulator_post_probe() which would trigger the regul=
ator
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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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/r=
egulator/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 =3D "regulator-name";
=20
uc_pdata =3D dev_get_uclass_plat(dev);
+ uc_pdata->always_on =3D dev_read_bool(dev, "regulator-always-on");
+ uc_pdata->boot_on =3D dev_read_bool(dev, "regulator-boot-on");
=20
/* Regulator's mandatory constraint */
uc_pdata->name =3D dev_read_string(dev, property);
@@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
return -EINVAL;
}
=20
- 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;
+ }
=20
- 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);
=20
- return -EINVAL;
+ return 0;
}
=20
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 =3D dev_read_u32_default(dev, "regulator-max-microamp"=
,
-ENODATA);
- uc_pdata->always_on =3D dev_read_bool(dev, "regulator-always-on");
- uc_pdata->boot_on =3D dev_read_bool(dev, "regulator-boot-on");
uc_pdata->ramp_delay =3D dev_read_u32_default(dev, "regulator-ramp-dela=
y",
0);
uc_pdata->force_off =3D dev_read_bool(dev, "regulator-force-boot-off");
--=20
2.43.0

On 27/06/2024 11:26, Simon Glass wrote:

Hi Caleb,
On Thu, 27 Jun 2024 at 09:48, Caleb Connolly <caleb.connolly@...> wrote:

On 27/06/2024 10:37, Simon Glass wrote:

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@...> 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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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)

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.

Hi Svyatoslav,

On Thu, 27 Jun 2024 at 11:34, Svyatoslav Ryhel <clamor95@...> wrote:

чт, 27 черв. 2024?р. о 12:26 Simon Glass <sjg@...> пише:

Hi Svyatoslav,

On Thu, 27 Jun 2024 at 10:09, Svyatoslav <clamor95@...> wrote:

27 червня 2024?р. 11:48:46 GMT+03:00, Caleb Connolly <caleb.connolly@...> написав(-ла):

On 27/06/2024 10:37, Simon Glass wrote:

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@...> 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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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 SPL and relocation
interfering with driver loading.

The issue which I have personally encountered was i2c driver failure
due to double probing. This behavior was triggered by
always-on/boot-on regulators provided by pmic which in most
cases is an i2c device.

At that time everyone just ignored me, so idk if tegra i2c is the only
driver which has this response or there are more, but be aware that
this patch set may cause cascade failure on many devices.

I'm not sure if I remember this, but I can certainly see some problems
with it. Did we have drivers that probed in the bind() function,
perhaps?

It is expected not to remember all patchsets sent, not an issue. As for
drivers probed after bind there are several, but they are quite essential.
Core GPIO and pinmux drivers are probed as early as possible but in most
cases they have no dependencies among complex subsystems (like i2c).

OK, well I suppose that you managed to find a solution.

From my side I just want to avoid doing too much such stuff
'automatically' in driver model. As you say, it can lead to difficult
race conditions / bugs.

Regards,
Simon

On 27/06/2024 10:37, Simon Glass wrote:

Hi Marek,
On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@...> 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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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)

Hi Caleb,

On Thu, 27 Jun 2024 at 09:48, Caleb Connolly <caleb.connolly@...> wrote:

On 27/06/2024 10:37, Simon Glass wrote:

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@...> 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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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.

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.

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

чт, 27 черв. 2024?р. о 12:26 Simon Glass <sjg@...> пише:

Hi Svyatoslav,

On Thu, 27 Jun 2024 at 10:09, Svyatoslav <clamor95@...> wrote:

27 червня 2024?р. 11:48:46 GMT+03:00, Caleb Connolly <caleb.connolly@...> написав(-ла):

On 27/06/2024 10:37, Simon Glass wrote:

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@...> 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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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 SPL and relocation
interfering with driver loading.

The issue which I have personally encountered was i2c driver failure
due to double probing. This behavior was triggered by
always-on/boot-on regulators provided by pmic which in most
cases is an i2c device.

At that time everyone just ignored me, so idk if tegra i2c is the only
driver which has this response or there are more, but be aware that
this patch set may cause cascade failure on many devices.

I'm not sure if I remember this, but I can certainly see some problems
with it. Did we have drivers that probed in the bind() function,
perhaps?

It is expected not to remember all patchsets sent, not an issue. As for
drivers probed after bind there are several, but they are quite essential.
Core GPIO and pinmux drivers are probed as early as possible but in most
cases they have no dependencies among complex subsystems (like i2c).

Best regards,
Svyatoslav R.

Should we have a step in the init sequence where we set up the
regulators, by calling regulators_enable_boot_on() ?

Regards,
Simon

Hi Marek,

On Thu, 27 Jun 2024 at 17:05, Marek Vasut <marex@...> 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

27 червня 2024?р. 11:48:46 GMT+03:00, Caleb Connolly <caleb.connolly@...> написав(-ла):

On 27/06/2024 10:37, Simon Glass wrote:

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@...> 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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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 SPL and relocation
interfering with driver loading.

The issue which I have personally encountered was i2c driver failure
due to double probing. This behavior was triggered by
always-on/boot-on regulators provided by pmic which in most
cases is an i2c device.

At that time everyone just ignored me, so idk if tegra i2c is the only
driver which has this response or there are more, but be aware that
this patch set may cause cascade failure on many devices.

Best regards,
Svyatoslav R.

Should we have a step in the init sequence where we set up the
regulators, by calling regulators_enable_boot_on() ?

Regards,
Simon

Hi Svyatoslav,

On Thu, 27 Jun 2024 at 10:09, Svyatoslav <clamor95@...> wrote:

27 червня 2024?р. 11:48:46 GMT+03:00, Caleb Connolly <caleb.connolly@...> написав(-ла):

On 27/06/2024 10:37, Simon Glass wrote:

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@...> 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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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 SPL and relocation
interfering with driver loading.

The issue which I have personally encountered was i2c driver failure
due to double probing. This behavior was triggered by
always-on/boot-on regulators provided by pmic which in most
cases is an i2c device.

At that time everyone just ignored me, so idk if tegra i2c is the only
driver which has this response or there are more, but be aware that
this patch set may cause cascade failure on many devices.

I'm not sure if I remember this, but I can certainly see some problems
with it. Did we have drivers that probed in the bind() function,
perhaps?

Best regards,
Svyatoslav R.

Should we have a step in the init sequence where we set up the
regulators, by calling regulators_enable_boot_on() ?

Regards,
Simon

Hi Caleb,

On Fri, 28 Jun 2024 at 01:09, Caleb Connolly <caleb.connolly@...> wrote:

On 27/06/2024 11:26, Simon Glass wrote:

Hi Caleb,

On Thu, 27 Jun 2024 at 09:48, Caleb Connolly <caleb.connolly@...> wrote:

On 27/06/2024 10:37, Simon Glass wrote:

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@...> 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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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.

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

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

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@...> 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 <marex@...>
---
Cc: Ben Wolsieffer <benwolsieffer@...>
Cc: Caleb Connolly <caleb.connolly@...>
Cc: Chris Morgan <macromorgan@...>
Cc: Dragan Simic <dsimic@...>
Cc: Eugen Hristev <eugen.hristev@...>
Cc: Francesco Dolcini <francesco.dolcini@...>
Cc: Heinrich Schuchardt <xypron.glpk@...>
Cc: Jaehoon Chung <jh80.chung@...>
Cc: Jagan Teki <jagan@...>
Cc: Jonas Karlman <jonas@...>
Cc: Kever Yang <kever.yang@...>
Cc: Kostya Porotchkin <kostap@...>
Cc: Matteo Lisi <matteo.lisi@...>
Cc: Mattijs Korpershoek <mkorpershoek@...>
Cc: Max Krummenacher <max.krummenacher@...>
Cc: Neil Armstrong <neil.armstrong@...>
Cc: Patrice Chotard <patrice.chotard@...>
Cc: Patrick Delaunay <patrick.delaunay@...>
Cc: Philipp Tomsich <philipp.tomsich@...>
Cc: Quentin Schulz <quentin.schulz@...>
Cc: Sam Day <me@...>
Cc: Simon Glass <sjg@...>
Cc: Sumit Garg <sumit.garg@...>
Cc: Svyatoslav Ryhel <clamor95@...>
Cc: Thierry Reding <treding@...>
Cc: Tom Rini <trini@...>
Cc: Volodymyr Babchuk <Volodymyr_Babchuk@...>
Cc: [email protected]
Cc: [email protected]
Cc: u-boot@...
Cc: u-boot@...
Cc: uboot-stm32@...
---
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

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)

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 ?

Hi Marek,

On Fri, 28 Jun 2024 at 07:26, Marek Vasut <marex@...> 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.

Regards,
Simon

[1]

On 9/12/24 3:00 AM, Simon Glass wrote:

Hello Simon,

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.

If the regulators are already enabled, enabling them again will be a noop, do you agree ?

[...]

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.

I strongly disagree that regulators which are marked in DT as always-on/boot-on should somehow be treated as optional-on in U-Boot , no , they should not. They should be enabled by the regulator uclass core code, for every regulator which is marked that way. If they are not to be enabled, they should not be marked that way in DT.

While the board code may exercise some control over enabling regulators earlier, it should still be the default in the core code to enable such regulators unconditionally.

If the concern is ordering problems between regulators, then regulators have to define vin-supply to describe their upstream supply in DT, which should address the ordering problem. DTTO for clock and pinmux etc.

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

--
Tom

ср, 25 вер. 2024?р. о 02:44 Tom Rini <trini@...> пише:

On Fri, Sep 20, 2024 at 10:48:56AM -0600, Tom Rini wrote:

On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote:

пн, 16 вер. 2024?р. о 19:28 Tom Rini <trini@...> пише:

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

So, here's my plan. Marek posted

which is a work-around, so that v2024.10 can work. I'm going to take
that for master tomorrow. I'm also going to take _this_ series to -next
tomorrow, as this is the best approach we currently have to solving the
overall problem. The Tegra platforms that are now very oddly broken need
to get debugged to see where their problem is, and if there's an
entirely alternate approach, Simon can post patches for that instead on
top of next.

--
Tom

Hello there!
I was digging this when I had some free time and found that with
patches from Marek the only difference is that function
i2c_get_chip_for_busnum is not called for PMIC's main i2c address
which results in issues with i2c you have seen in logs before. I
assume this is not a tegra specific issue and not even related to
these regulator patches itself.

To verify my suspicions, Tom and Marek my you please dump u-boot log
with and without patches and with enabled debug logging from
i2c-uclass and i2c driver of your SoC.

Here are logs from my device (Tegra SoC)

Not working
(bootloader) i2c: controller bus 0 at 7000d000, speed 0:
i2c_init_controller: speed=400000
(bootloader) i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25
(bootloader) i2c_get_chip: Searching bus 'i2c@7000d000' for address 59:
(bootloader) not found
(bootloader) pmic_reg_read: reg=37 priv->trans_len:1i2c_xfer: 2 messages
(bootloader)
(bootloader) i2c_xfer: chip=0x58, len=0x1
(bootloader) i2c_write_data: chip=0x58, len=0x1
(bootloader) write_data: 0x37
(bootloader) pkt header 1 sent (0x10010)
(bootloader) pkt header 2 sent (0x0)
(bootloader) pkt header 3 sent (0x100b0)
(bootloader) pkt data sent (0x37)
(bootloader) tegra_i2c_write_data: Error (-1) !!!

Working
(bootloader) i2c: controller bus 0 at 7000d000, speed 0:
i2c_init_controller: speed=400000
(bootloader) i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25
(bootloader) pkt header 1 sent (0x10010)
(bootloader) pkt header 2 sent (0x0)
(bootloader) pkt header 3 sent (0xb0)
(bootloader) pkt data sent (0x0)
(bootloader) i2c_get_chip: Searching bus 'i2c@7000d000' for address 58:
(bootloader) i2c_get_chip: Searching bus 'i2c@7000d000' for address 59:
(bootloader) not found
(bootloader) found, ret=0
(bootloader) i2c_xfer: 2 messages
(bootloader) i2c_xfer: chip=0x58, len=0x1
(bootloader) i2c_write_data: chip=0x58, len=0x1
(bootloader) write_data: 0xfb
(bootloader) pkt header 1 sent (0x10010)
(bootloader) pkt header 2 sent (0x0)
(bootloader) pkt header 3 sent (0x100b0)
(bootloader) pkt data sent (0xfb)
(bootloader) i2c_xfer: chip=0x58, len=0x1

As you can see this part

(bootloader) pkt header 1 sent (0x10010)
(bootloader) pkt header 2 sent (0x0)
(bootloader) pkt header 3 sent (0xb0)
(bootloader) pkt data sent (0x0)
(bootloader) i2c_get_chip: Searching bus 'i2c@7000d000' for address 58:

is missing in log from u-boot where Marek's patches are applied and
this log fragment co-responds to i2c_get_chip_for_busnum call

On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote:

пн, 16 вер. 2024?р. о 19:28 Tom Rini <trini@...> пише:

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

пн, 16 вер. 2024?р. о 19:28 Tom Rini <trini@...> пише:

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

--
Tom

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?