| Message ID | 20240626235717.272219-1-marex@denx.de |
|---|---|
| State | Superseded |
| Delegated to: | Jaehoon Chung |
| Headers | show |
| Series | [1/4] power: regulator: Trigger probe of regulators which are always-on or boot-on | expand |
Hi Marek, On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@denx.de> 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@denx.de> > --- > Cc: Ben Wolsieffer <benwolsieffer@gmail.com> > Cc: Caleb Connolly <caleb.connolly@linaro.org> > Cc: Chris Morgan <macromorgan@hotmail.com> > Cc: Dragan Simic <dsimic@manjaro.org> > Cc: Eugen Hristev <eugen.hristev@collabora.com> > Cc: Francesco Dolcini <francesco.dolcini@toradex.com> > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > Cc: Jaehoon Chung <jh80.chung@samsung.com> > Cc: Jagan Teki <jagan@amarulasolutions.com> > Cc: Jonas Karlman <jonas@kwiboo.se> > Cc: Kever Yang <kever.yang@rock-chips.com> > Cc: Kostya Porotchkin <kostap@marvell.com> > Cc: Matteo Lisi <matteo.lisi@engicam.com> > Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> > Cc: Max Krummenacher <max.krummenacher@toradex.com> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > Cc: Patrice Chotard <patrice.chotard@foss.st.com> > Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> > Cc: Quentin Schulz <quentin.schulz@cherry.de> > Cc: Sam Day <me@samcday.com> > Cc: Simon Glass <sjg@chromium.org> > Cc: Sumit Garg <sumit.garg@linaro.org> > Cc: Svyatoslav Ryhel <clamor95@gmail.com> > Cc: Thierry Reding <treding@nvidia.com> > Cc: Tom Rini <trini@konsulko.com> > Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > Cc: u-boot-amlogic@groups.io > Cc: u-boot-qcom@groups.io > Cc: u-boot@dh-electronics.com > Cc: u-boot@lists.denx.de > Cc: uboot-stm32@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
On 27/06/2024 10:37, Simon Glass wrote: > Hi Marek, > > On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@denx.de> 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@denx.de> >> --- >> Cc: Ben Wolsieffer <benwolsieffer@gmail.com> >> Cc: Caleb Connolly <caleb.connolly@linaro.org> >> Cc: Chris Morgan <macromorgan@hotmail.com> >> Cc: Dragan Simic <dsimic@manjaro.org> >> Cc: Eugen Hristev <eugen.hristev@collabora.com> >> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> >> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> >> Cc: Jaehoon Chung <jh80.chung@samsung.com> >> Cc: Jagan Teki <jagan@amarulasolutions.com> >> Cc: Jonas Karlman <jonas@kwiboo.se> >> Cc: Kever Yang <kever.yang@rock-chips.com> >> Cc: Kostya Porotchkin <kostap@marvell.com> >> Cc: Matteo Lisi <matteo.lisi@engicam.com> >> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> >> Cc: Max Krummenacher <max.krummenacher@toradex.com> >> Cc: Neil Armstrong <neil.armstrong@linaro.org> >> Cc: Patrice Chotard <patrice.chotard@foss.st.com> >> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> >> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> >> Cc: Quentin Schulz <quentin.schulz@cherry.de> >> Cc: Sam Day <me@samcday.com> >> Cc: Simon Glass <sjg@chromium.org> >> Cc: Sumit Garg <sumit.garg@linaro.org> >> Cc: Svyatoslav Ryhel <clamor95@gmail.com> >> Cc: Thierry Reding <treding@nvidia.com> >> Cc: Tom Rini <trini@konsulko.com> >> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> Cc: u-boot-amlogic@groups.io >> Cc: u-boot-qcom@groups.io >> Cc: u-boot@dh-electronics.com >> Cc: u-boot@lists.denx.de >> Cc: uboot-stm32@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
27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly <caleb.connolly@linaro.org> написав(-ла): > > >On 27/06/2024 10:37, Simon Glass wrote: >> Hi Marek, >> >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@denx.de> 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@denx.de> >>> --- >>> Cc: Ben Wolsieffer <benwolsieffer@gmail.com> >>> Cc: Caleb Connolly <caleb.connolly@linaro.org> >>> Cc: Chris Morgan <macromorgan@hotmail.com> >>> Cc: Dragan Simic <dsimic@manjaro.org> >>> Cc: Eugen Hristev <eugen.hristev@collabora.com> >>> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> >>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> Cc: Jaehoon Chung <jh80.chung@samsung.com> >>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>> Cc: Jonas Karlman <jonas@kwiboo.se> >>> Cc: Kever Yang <kever.yang@rock-chips.com> >>> Cc: Kostya Porotchkin <kostap@marvell.com> >>> Cc: Matteo Lisi <matteo.lisi@engicam.com> >>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> >>> Cc: Max Krummenacher <max.krummenacher@toradex.com> >>> Cc: Neil Armstrong <neil.armstrong@linaro.org> >>> Cc: Patrice Chotard <patrice.chotard@foss.st.com> >>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> >>> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> >>> Cc: Quentin Schulz <quentin.schulz@cherry.de> >>> Cc: Sam Day <me@samcday.com> >>> Cc: Simon Glass <sjg@chromium.org> >>> Cc: Sumit Garg <sumit.garg@linaro.org> >>> Cc: Svyatoslav Ryhel <clamor95@gmail.com> >>> Cc: Thierry Reding <treding@nvidia.com> >>> Cc: Tom Rini <trini@konsulko.com> >>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>> Cc: u-boot-amlogic@groups.io >>> Cc: u-boot-qcom@groups.io >>> Cc: u-boot@dh-electronics.com >>> Cc: u-boot@lists.denx.de >>> Cc: uboot-stm32@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 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@gmail.com> wrote: > > > > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly <caleb.connolly@linaro.org> написав(-ла): > > > > > >On 27/06/2024 10:37, Simon Glass wrote: > >> Hi Marek, > >> > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@denx.de> 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@denx.de> > >>> --- > >>> Cc: Ben Wolsieffer <benwolsieffer@gmail.com> > >>> Cc: Caleb Connolly <caleb.connolly@linaro.org> > >>> Cc: Chris Morgan <macromorgan@hotmail.com> > >>> Cc: Dragan Simic <dsimic@manjaro.org> > >>> Cc: Eugen Hristev <eugen.hristev@collabora.com> > >>> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> > >>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > >>> Cc: Jaehoon Chung <jh80.chung@samsung.com> > >>> Cc: Jagan Teki <jagan@amarulasolutions.com> > >>> Cc: Jonas Karlman <jonas@kwiboo.se> > >>> Cc: Kever Yang <kever.yang@rock-chips.com> > >>> Cc: Kostya Porotchkin <kostap@marvell.com> > >>> Cc: Matteo Lisi <matteo.lisi@engicam.com> > >>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> > >>> Cc: Max Krummenacher <max.krummenacher@toradex.com> > >>> Cc: Neil Armstrong <neil.armstrong@linaro.org> > >>> Cc: Patrice Chotard <patrice.chotard@foss.st.com> > >>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > >>> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> > >>> Cc: Quentin Schulz <quentin.schulz@cherry.de> > >>> Cc: Sam Day <me@samcday.com> > >>> Cc: Simon Glass <sjg@chromium.org> > >>> Cc: Sumit Garg <sumit.garg@linaro.org> > >>> Cc: Svyatoslav Ryhel <clamor95@gmail.com> > >>> Cc: Thierry Reding <treding@nvidia.com> > >>> Cc: Tom Rini <trini@konsulko.com> > >>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > >>> Cc: u-boot-amlogic@groups.io > >>> Cc: u-boot-qcom@groups.io > >>> Cc: u-boot@dh-electronics.com > >>> Cc: u-boot@lists.denx.de > >>> Cc: uboot-stm32@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 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 Thu, 27 Jun 2024 at 09:48, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > On 27/06/2024 10:37, Simon Glass wrote: > > Hi Marek, > > > > On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@denx.de> 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@denx.de> > >> --- > >> Cc: Ben Wolsieffer <benwolsieffer@gmail.com> > >> Cc: Caleb Connolly <caleb.connolly@linaro.org> > >> Cc: Chris Morgan <macromorgan@hotmail.com> > >> Cc: Dragan Simic <dsimic@manjaro.org> > >> Cc: Eugen Hristev <eugen.hristev@collabora.com> > >> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> > >> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > >> Cc: Jaehoon Chung <jh80.chung@samsung.com> > >> Cc: Jagan Teki <jagan@amarulasolutions.com> > >> Cc: Jonas Karlman <jonas@kwiboo.se> > >> Cc: Kever Yang <kever.yang@rock-chips.com> > >> Cc: Kostya Porotchkin <kostap@marvell.com> > >> Cc: Matteo Lisi <matteo.lisi@engicam.com> > >> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> > >> Cc: Max Krummenacher <max.krummenacher@toradex.com> > >> Cc: Neil Armstrong <neil.armstrong@linaro.org> > >> Cc: Patrice Chotard <patrice.chotard@foss.st.com> > >> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > >> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> > >> Cc: Quentin Schulz <quentin.schulz@cherry.de> > >> Cc: Sam Day <me@samcday.com> > >> Cc: Simon Glass <sjg@chromium.org> > >> Cc: Sumit Garg <sumit.garg@linaro.org> > >> Cc: Svyatoslav Ryhel <clamor95@gmail.com> > >> Cc: Thierry Reding <treding@nvidia.com> > >> Cc: Tom Rini <trini@konsulko.com> > >> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > >> Cc: u-boot-amlogic@groups.io > >> Cc: u-boot-qcom@groups.io > >> Cc: u-boot@dh-electronics.com > >> Cc: u-boot@lists.denx.de > >> Cc: uboot-stm32@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. 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@chromium.org> пише: > > Hi Svyatoslav, > > On Thu, 27 Jun 2024 at 10:09, Svyatoslav <clamor95@gmail.com> wrote: > > > > > > > > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly <caleb.connolly@linaro.org> написав(-ла): > > > > > > > > >On 27/06/2024 10:37, Simon Glass wrote: > > >> Hi Marek, > > >> > > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@denx.de> 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@denx.de> > > >>> --- > > >>> Cc: Ben Wolsieffer <benwolsieffer@gmail.com> > > >>> Cc: Caleb Connolly <caleb.connolly@linaro.org> > > >>> Cc: Chris Morgan <macromorgan@hotmail.com> > > >>> Cc: Dragan Simic <dsimic@manjaro.org> > > >>> Cc: Eugen Hristev <eugen.hristev@collabora.com> > > >>> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> > > >>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > > >>> Cc: Jaehoon Chung <jh80.chung@samsung.com> > > >>> Cc: Jagan Teki <jagan@amarulasolutions.com> > > >>> Cc: Jonas Karlman <jonas@kwiboo.se> > > >>> Cc: Kever Yang <kever.yang@rock-chips.com> > > >>> Cc: Kostya Porotchkin <kostap@marvell.com> > > >>> Cc: Matteo Lisi <matteo.lisi@engicam.com> > > >>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> > > >>> Cc: Max Krummenacher <max.krummenacher@toradex.com> > > >>> Cc: Neil Armstrong <neil.armstrong@linaro.org> > > >>> Cc: Patrice Chotard <patrice.chotard@foss.st.com> > > >>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > > >>> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> > > >>> Cc: Quentin Schulz <quentin.schulz@cherry.de> > > >>> Cc: Sam Day <me@samcday.com> > > >>> Cc: Simon Glass <sjg@chromium.org> > > >>> Cc: Sumit Garg <sumit.garg@linaro.org> > > >>> Cc: Svyatoslav Ryhel <clamor95@gmail.com> > > >>> Cc: Thierry Reding <treding@nvidia.com> > > >>> Cc: Tom Rini <trini@konsulko.com> > > >>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > >>> Cc: u-boot-amlogic@groups.io > > >>> Cc: u-boot-qcom@groups.io > > >>> Cc: u-boot@dh-electronics.com > > >>> Cc: u-boot@lists.denx.de > > >>> Cc: uboot-stm32@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 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 Svyatoslav, On Thu, 27 Jun 2024 at 11:34, Svyatoslav Ryhel <clamor95@gmail.com> wrote: > > чт, 27 черв. 2024 р. о 12:26 Simon Glass <sjg@chromium.org> пише: > > > > Hi Svyatoslav, > > > > On Thu, 27 Jun 2024 at 10:09, Svyatoslav <clamor95@gmail.com> wrote: > > > > > > > > > > > > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly <caleb.connolly@linaro.org> написав(-ла): > > > > > > > > > > > >On 27/06/2024 10:37, Simon Glass wrote: > > > >> Hi Marek, > > > >> > > > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@denx.de> 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@denx.de> > > > >>> --- > > > >>> Cc: Ben Wolsieffer <benwolsieffer@gmail.com> > > > >>> Cc: Caleb Connolly <caleb.connolly@linaro.org> > > > >>> Cc: Chris Morgan <macromorgan@hotmail.com> > > > >>> Cc: Dragan Simic <dsimic@manjaro.org> > > > >>> Cc: Eugen Hristev <eugen.hristev@collabora.com> > > > >>> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> > > > >>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > >>> Cc: Jaehoon Chung <jh80.chung@samsung.com> > > > >>> Cc: Jagan Teki <jagan@amarulasolutions.com> > > > >>> Cc: Jonas Karlman <jonas@kwiboo.se> > > > >>> Cc: Kever Yang <kever.yang@rock-chips.com> > > > >>> Cc: Kostya Porotchkin <kostap@marvell.com> > > > >>> Cc: Matteo Lisi <matteo.lisi@engicam.com> > > > >>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> > > > >>> Cc: Max Krummenacher <max.krummenacher@toradex.com> > > > >>> Cc: Neil Armstrong <neil.armstrong@linaro.org> > > > >>> Cc: Patrice Chotard <patrice.chotard@foss.st.com> > > > >>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > > > >>> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> > > > >>> Cc: Quentin Schulz <quentin.schulz@cherry.de> > > > >>> Cc: Sam Day <me@samcday.com> > > > >>> Cc: Simon Glass <sjg@chromium.org> > > > >>> Cc: Sumit Garg <sumit.garg@linaro.org> > > > >>> Cc: Svyatoslav Ryhel <clamor95@gmail.com> > > > >>> Cc: Thierry Reding <treding@nvidia.com> > > > >>> Cc: Tom Rini <trini@konsulko.com> > > > >>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > > >>> Cc: u-boot-amlogic@groups.io > > > >>> Cc: u-boot-qcom@groups.io > > > >>> Cc: u-boot@dh-electronics.com > > > >>> Cc: u-boot@lists.denx.de > > > >>> Cc: uboot-stm32@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 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 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.
On 27/06/2024 11:26, Simon Glass wrote: > Hi Caleb, > > On Thu, 27 Jun 2024 at 09:48, Caleb Connolly <caleb.connolly@linaro.org> wrote: >> >> >> >> On 27/06/2024 10:37, Simon Glass wrote: >>> Hi Marek, >>> >>> On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@denx.de> 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@denx.de> >>>> --- >>>> Cc: Ben Wolsieffer <benwolsieffer@gmail.com> >>>> Cc: Caleb Connolly <caleb.connolly@linaro.org> >>>> Cc: Chris Morgan <macromorgan@hotmail.com> >>>> Cc: Dragan Simic <dsimic@manjaro.org> >>>> Cc: Eugen Hristev <eugen.hristev@collabora.com> >>>> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> >>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>> Cc: Jaehoon Chung <jh80.chung@samsung.com> >>>> Cc: Jagan Teki <jagan@amarulasolutions.com> >>>> Cc: Jonas Karlman <jonas@kwiboo.se> >>>> Cc: Kever Yang <kever.yang@rock-chips.com> >>>> Cc: Kostya Porotchkin <kostap@marvell.com> >>>> Cc: Matteo Lisi <matteo.lisi@engicam.com> >>>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> >>>> Cc: Max Krummenacher <max.krummenacher@toradex.com> >>>> Cc: Neil Armstrong <neil.armstrong@linaro.org> >>>> Cc: Patrice Chotard <patrice.chotard@foss.st.com> >>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> >>>> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> >>>> Cc: Quentin Schulz <quentin.schulz@cherry.de> >>>> Cc: Sam Day <me@samcday.com> >>>> Cc: Simon Glass <sjg@chromium.org> >>>> Cc: Sumit Garg <sumit.garg@linaro.org> >>>> Cc: Svyatoslav Ryhel <clamor95@gmail.com> >>>> Cc: Thierry Reding <treding@nvidia.com> >>>> Cc: Tom Rini <trini@konsulko.com> >>>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >>>> Cc: u-boot-amlogic@groups.io >>>> Cc: u-boot-qcom@groups.io >>>> Cc: u-boot@dh-electronics.com >>>> Cc: u-boot@lists.denx.de >>>> Cc: uboot-stm32@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 > 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
Hi Marek, On Thu, 27 Jun 2024 at 17:05, Marek Vasut <marex@denx.de> 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
Hi Caleb, On Fri, 28 Jun 2024 at 01:09, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > > > On 27/06/2024 11:26, Simon Glass wrote: > > Hi Caleb, > > > > On Thu, 27 Jun 2024 at 09:48, Caleb Connolly <caleb.connolly@linaro.org> wrote: > >> > >> > >> > >> On 27/06/2024 10:37, Simon Glass wrote: > >>> Hi Marek, > >>> > >>> On Thu, 27 Jun 2024 at 00:57, Marek Vasut <marex@denx.de> 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@denx.de> > >>>> --- > >>>> Cc: Ben Wolsieffer <benwolsieffer@gmail.com> > >>>> Cc: Caleb Connolly <caleb.connolly@linaro.org> > >>>> Cc: Chris Morgan <macromorgan@hotmail.com> > >>>> Cc: Dragan Simic <dsimic@manjaro.org> > >>>> Cc: Eugen Hristev <eugen.hristev@collabora.com> > >>>> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> > >>>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > >>>> Cc: Jaehoon Chung <jh80.chung@samsung.com> > >>>> Cc: Jagan Teki <jagan@amarulasolutions.com> > >>>> Cc: Jonas Karlman <jonas@kwiboo.se> > >>>> Cc: Kever Yang <kever.yang@rock-chips.com> > >>>> Cc: Kostya Porotchkin <kostap@marvell.com> > >>>> Cc: Matteo Lisi <matteo.lisi@engicam.com> > >>>> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> > >>>> Cc: Max Krummenacher <max.krummenacher@toradex.com> > >>>> Cc: Neil Armstrong <neil.armstrong@linaro.org> > >>>> Cc: Patrice Chotard <patrice.chotard@foss.st.com> > >>>> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> > >>>> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> > >>>> Cc: Quentin Schulz <quentin.schulz@cherry.de> > >>>> Cc: Sam Day <me@samcday.com> > >>>> Cc: Simon Glass <sjg@chromium.org> > >>>> Cc: Sumit Garg <sumit.garg@linaro.org> > >>>> Cc: Svyatoslav Ryhel <clamor95@gmail.com> > >>>> Cc: Thierry Reding <treding@nvidia.com> > >>>> Cc: Tom Rini <trini@konsulko.com> > >>>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > >>>> Cc: u-boot-amlogic@groups.io > >>>> Cc: u-boot-qcom@groups.io > >>>> Cc: u-boot@dh-electronics.com > >>>> Cc: u-boot@lists.denx.de > >>>> Cc: uboot-stm32@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. 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 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
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 ?
On 6/27/24 1:55 AM, 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. Is there anything blocking this series from being applied ?
нд, 28 лип. 2024 р. о 19:38 Marek Vasut <marex@denx.de> пише: > > On 6/27/24 1:55 AM, 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. > > Is there anything blocking this series from being applied ? I would like to try it to be sure that it does not break my devices. I will respond within next 24 hours.
нд, 28 лип. 2024 р. о 19:38 Marek Vasut <marex@denx.de> пише: > > On 6/27/24 1:55 AM, 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. > > Is there anything blocking this series from being applied ? This patchset causes PMIC regulators probe too early which results in i2c line setup failure. These patches MUST NOT be applied in this form since they will break at least 15 Tegra 3 devices which use DM PMIC, maybe more.
On 7/28/24 7:55 PM, Svyatoslav Ryhel wrote: > нд, 28 лип. 2024 р. о 19:38 Marek Vasut <marex@denx.de> пише: >> >> On 6/27/24 1:55 AM, 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. >> >> Is there anything blocking this series from being applied ? > > This patchset causes PMIC regulators probe too early which results in > i2c line setup failure. These patches MUST NOT be applied in this form > since they will break at least 15 Tegra 3 devices which use DM PMIC, > maybe more. Thank you for testing. I do not have any tegra 3 devices, but this patchset does not do anything with pinmuxing. If a regulator is probed, all of its dependencies (i2c bus, pinmux configuration, etc.) should be probed as well. Can you have a look at what the problem with pinmuxing is on tegra 3? It seems it might be unrelated to this patchset and would eventually show up elsewhere?
28 липня 2024 р. 21:35:27 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >On 7/28/24 7:55 PM, Svyatoslav Ryhel wrote: >> нд, 28 лип. 2024 р. о 19:38 Marek Vasut <marex@denx.de> пише: >>> >>> On 6/27/24 1:55 AM, 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. >>> >>> Is there anything blocking this series from being applied ? >> >> This patchset causes PMIC regulators probe too early which results in >> i2c line setup failure. These patches MUST NOT be applied in this form >> since they will break at least 15 Tegra 3 devices which use DM PMIC, >> maybe more. > >Thank you for testing. I do not have any tegra 3 devices, but this patchset does not do anything with pinmuxing. If a regulator is probed, all of its dependencies (i2c bus, pinmux configuration, etc.) should be probed as well. Can you have a look at what the problem with pinmuxing is on tegra 3? It seems it might be unrelated to this patchset and would eventually show up elsewhere? Pinmux? Wdym, I wrote about a PMIC which is usually located on i2c line. <https://patchwork.ozlabs.org/project/uboot/patch/20231003062126.42026-4-clamor95@gmail.com/> This is a similar patch. You may be able to reproduce the issue I face if you have a device which uses SPL and has DM PMIC with regulators that need always-on/boot-on properties.
On 7/28/24 9:02 PM, Svyatoslav wrote: Hi, I'm trimming the CC because I keep getting ML blockage due to large CC list. If someone has been removed too hastily, sorry. > 28 липня 2024 р. 21:35:27 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): >> On 7/28/24 7:55 PM, Svyatoslav Ryhel wrote: >>> нд, 28 лип. 2024 р. о 19:38 Marek Vasut <marex@denx.de> пише: >>>> >>>> On 6/27/24 1:55 AM, 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. >>>> >>>> Is there anything blocking this series from being applied ? >>> >>> This patchset causes PMIC regulators probe too early which results in >>> i2c line setup failure. These patches MUST NOT be applied in this form >>> since they will break at least 15 Tegra 3 devices which use DM PMIC, >>> maybe more. >> >> Thank you for testing. I do not have any tegra 3 devices, but this patchset does not do anything with pinmuxing. If a regulator is probed, all of its dependencies (i2c bus, pinmux configuration, etc.) should be probed as well. Can you have a look at what the problem with pinmuxing is on tegra 3? It seems it might be unrelated to this patchset and would eventually show up elsewhere? > > Pinmux? Wdym, I wrote about a PMIC which is usually located on i2c line. > > <https://patchwork.ozlabs.org/project/uboot/patch/20231003062126.42026-4-clamor95@gmail.com/> > > This is a similar patch. > > You may be able to reproduce the issue I face if you have a device which uses SPL and has DM PMIC with regulators that need always-on/boot-on properties. I actually do use: configs/imx8mp_dhcom_pdk3_defconfig:CONFIG_DM_PMIC=y configs/imx8mp_dhcom_pdk3_defconfig:CONFIG_DM_PMIC_PCA9450=y configs/imx8mp_dhcom_pdk3_defconfig:CONFIG_SPL_DM_PMIC_PCA9450=y which is one of the devices I test this on. The PMIC is on I2C, DM_PMIC enabled in SPL, both buck4 and buck5 regulators are enabled in SPL, have regulator-always-on and regulator-boot-on and bootph-pre-ram properties. This seems similar enough, right ? What is the problem you observe on tegra 3 ?
нд, 28 лип. 2024 р. о 23:08 Marek Vasut <marex@denx.de> пише: > > On 7/28/24 9:02 PM, Svyatoslav wrote: > > Hi, > > I'm trimming the CC because I keep getting ML blockage due to large CC > list. If someone has been removed too hastily, sorry. > > > 28 липня 2024 р. 21:35:27 GMT+03:00, Marek Vasut <marex@denx.de> написав(-ла): > >> On 7/28/24 7:55 PM, Svyatoslav Ryhel wrote: > >>> нд, 28 лип. 2024 р. о 19:38 Marek Vasut <marex@denx.de> пише: > >>>> > >>>> On 6/27/24 1:55 AM, 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. > >>>> > >>>> Is there anything blocking this series from being applied ? > >>> > >>> This patchset causes PMIC regulators probe too early which results in > >>> i2c line setup failure. These patches MUST NOT be applied in this form > >>> since they will break at least 15 Tegra 3 devices which use DM PMIC, > >>> maybe more. > >> > >> Thank you for testing. I do not have any tegra 3 devices, but this patchset does not do anything with pinmuxing. If a regulator is probed, all of its dependencies (i2c bus, pinmux configuration, etc.) should be probed as well. Can you have a look at what the problem with pinmuxing is on tegra 3? It seems it might be unrelated to this patchset and would eventually show up elsewhere? > > > > Pinmux? Wdym, I wrote about a PMIC which is usually located on i2c line. > > > > <https://patchwork.ozlabs.org/project/uboot/patch/20231003062126.42026-4-clamor95@gmail.com/> > > > > This is a similar patch. > > > > You may be able to reproduce the issue I face if you have a device which uses SPL and has DM PMIC with regulators that need always-on/boot-on properties. > > I actually do use: > > configs/imx8mp_dhcom_pdk3_defconfig:CONFIG_DM_PMIC=y > configs/imx8mp_dhcom_pdk3_defconfig:CONFIG_DM_PMIC_PCA9450=y > configs/imx8mp_dhcom_pdk3_defconfig:CONFIG_SPL_DM_PMIC_PCA9450=y > > which is one of the devices I test this on. > > The PMIC is on I2C, DM_PMIC enabled in SPL, both buck4 and buck5 > regulators are enabled in SPL, have regulator-always-on and > regulator-boot-on and bootph-pre-ram properties. > > This seems similar enough, right ? > Yes, though SPL must remain as small as possible and you propose add there i2c driver, PMIC driver, PMIC regulator drivers, PMIC GPIO drivers along with relocation of all this stuff. It is not optimal at all. > What is the problem you observe on tegra 3 ? i2c line fails since it probes in spl with your patch, but it does not relocate and then probes once more after relocation. Probe fails along with all devices on same line. Even with CONFIG_SPL_I2C=y CONFIG_SPL_PMIC_PALMAS=y CONFIG_SPL_DM_REGULATOR_PALMAS=y CONFIG_SPL_PALMAS_GPIO=y and all bootph-pre-ram; set I still get i2c failure Here is log (bootloader) read error from device: bd26f8e0 register: 0x37! (bootloader) read error from device: bd26f8e0 register: 0x3b! (bootloader) read error from device: bd26f8e0 register: 0x61! (bootloader) read error from device: bd26f8e0 register: 0x65! (bootloader) Core: 177 devices, 27 uclasses, devicetree: separate (bootloader) MMC: sdhci@78000400: 1, sdhci@78000600: 0 (bootloader) Loading Environment from MMC... Reading from MMC(0)... *** (bootloader) Warning - bad CRC, using default environment (bootloader) (bootloader) read error from device: bd26f8e0 register: 0x53! (bootloader) read error from device: bd26f8e0 register: 0x2f! (bootloader) tegra_dsi_bridge_probe: Cannot get panel: error -5 (bootloader) tegra_dsi_bridge_probe: Cannot get panel: error -114 (bootloader) In: serial,usbkbd,button-kbd (bootloader) Out: serial,vidconsole (bootloader) Err: serial,vidconsole (bootloader) Net: No ethernet found.
On 7/29/24 7:38 AM, Svyatoslav Ryhel wrote: [...] >> The PMIC is on I2C, DM_PMIC enabled in SPL, both buck4 and buck5 >> regulators are enabled in SPL, have regulator-always-on and >> regulator-boot-on and bootph-pre-ram properties. >> >> This seems similar enough, right ? >> > Yes, though SPL must remain as small as possible and you propose add > there i2c driver, PMIC driver, PMIC regulator drivers, PMIC GPIO > drivers along with relocation of all this stuff. It is not optimal at > all. Sure, if you do use DM_PMIC for PMIC on I2C bus, then you also need DM_I2C . You can also do non-DM PMIC configuration in SPL, non-DM in SPL is allowed. >> What is the problem you observe on tegra 3 ? > i2c line fails since it probes in spl with your patch, but it does not > relocate and then probes once more after relocation. Probe fails along > with all devices on same line. Could it be that you either have to: - Add DM_I2C to tegra 3 SPL - Remove bootph-* from DT to remove the regulator node from SPL - /delete-property/ regulator-always-on; and /delete-property/ regulator-boot-on; in -u-boot.dtsi to prevent the regulator from being enabled in SPL ? regulator-always-on means the regulator has to be enabled unconditionally, and the system software has no other way to test whether the regulator is enabled but access the PMIC, so that is why the regulator is probed, even if it is early.
пн, 29 лип. 2024 р. о 13:42 Marek Vasut <marex@denx.de> пише: > > On 7/29/24 7:38 AM, Svyatoslav Ryhel wrote: > > [...] > > >> The PMIC is on I2C, DM_PMIC enabled in SPL, both buck4 and buck5 > >> regulators are enabled in SPL, have regulator-always-on and > >> regulator-boot-on and bootph-pre-ram properties. > >> > >> This seems similar enough, right ? > >> > > Yes, though SPL must remain as small as possible and you propose add > > there i2c driver, PMIC driver, PMIC regulator drivers, PMIC GPIO > > drivers along with relocation of all this stuff. It is not optimal at > > all. > > Sure, if you do use DM_PMIC for PMIC on I2C bus, then you also need > DM_I2C . You can also do non-DM PMIC configuration in SPL, non-DM in SPL > is allowed. > Thanks for explaining an obvious stuff, it seems that we are talking on different languages. > >> What is the problem you observe on tegra 3 ? > > i2c line fails since it probes in spl with your patch, but it does not > > relocate and then probes once more after relocation. Probe fails along > > with all devices on same line. > > Could it be that you either have to: > - Add DM_I2C to tegra 3 SPL > - Remove bootph-* from DT to remove the regulator node from SPL > - /delete-property/ regulator-always-on; and /delete-property/ > regulator-boot-on; in -u-boot.dtsi to prevent the regulator from being > enabled in SPL ? > Obviously NO, you propose nonsense. Same dts is used for both stages. And I have to add hack-ish stuff just because you wanna introduce code which causes known regressions. > regulator-always-on means the regulator has to be enabled > unconditionally, and the system software has no other way to test > whether the regulator is enabled but access the PMIC, so that is why the > regulator is probed, even if it is early. Thanks for explaining an obvious stuff, it seems that we are talking on different languages. Anyway, "We must not probe things as we go. There might be other dependencies not yet bound. It may also take some time. This is not following driver model design, sorry. So please think of a way to do this properly."
On 7/29/24 1:55 PM, Svyatoslav Ryhel wrote: [...] >>>> What is the problem you observe on tegra 3 ? >>> i2c line fails since it probes in spl with your patch, but it does not >>> relocate and then probes once more after relocation. Probe fails along >>> with all devices on same line. >> >> Could it be that you either have to: >> - Add DM_I2C to tegra 3 SPL >> - Remove bootph-* from DT to remove the regulator node from SPL >> - /delete-property/ regulator-always-on; and /delete-property/ >> regulator-boot-on; in -u-boot.dtsi to prevent the regulator from being >> enabled in SPL ? >> > Obviously NO, you propose nonsense. Same dts is used for both stages. DT source yes, DT blob likely no. > And I have to add hack-ish stuff just because you wanna introduce code > which causes known regressions. I am trying to understand what problem there is on tegra 3, but it is still not clear to me. Is the problem somehow related to PMICs (?) being probed in SPL (?) because they have regulators (?) which are marked as regulator-always-on ? If so, then this is correct behavior, and if this is not desired in SPL, then you can remove this property from SPL DT in -u-boot.dtsi using /delete-property/ . [...] > "We must not probe things as we go. There might be other > dependencies not yet bound. It may also take some time. This is not > following driver model design, sorry. > > So please think of a way to do this properly." What is this quote about ? Where is this from ?
On 8/1/24 2:28 AM, Marek Vasut wrote: > On 7/29/24 1:55 PM, Svyatoslav Ryhel wrote: > > [...] > >>>>> What is the problem you observe on tegra 3 ? >>>> i2c line fails since it probes in spl with your patch, but it does not >>>> relocate and then probes once more after relocation. Probe fails along >>>> with all devices on same line. >>> >>> Could it be that you either have to: >>> - Add DM_I2C to tegra 3 SPL >>> - Remove bootph-* from DT to remove the regulator node from SPL >>> - /delete-property/ regulator-always-on; and /delete-property/ >>> regulator-boot-on; in -u-boot.dtsi to prevent the regulator from being >>> enabled in SPL ? >>> >> Obviously NO, you propose nonsense. Same dts is used for both stages. > > DT source yes, DT blob likely no. > >> And I have to add hack-ish stuff just because you wanna introduce code >> which causes known regressions. > > I am trying to understand what problem there is on tegra 3, but it is > still not clear to me. > > Is the problem somehow related to PMICs (?) being probed in SPL (?) > because they have regulators (?) which are marked as regulator-always-on > ? If so, then this is correct behavior, and if this is not desired in > SPL, then you can remove this property from SPL DT in -u-boot.dtsi using > /delete-property/ . > > [...] > >> "We must not probe things as we go. There might be other >> dependencies not yet bound. It may also take some time. This is not >> following driver model design, sorry. >> >> So please think of a way to do this properly." > > What is this quote about ? Where is this from ? What is the problem with Tegra 3 and this patchset ? Can you please explain that so this patchset can move forward ? Thank you
пн, 19 серп. 2024 р. о 20:27 Marek Vasut <marex@denx.de> пише: > > On 8/1/24 2:28 AM, Marek Vasut wrote: > > On 7/29/24 1:55 PM, Svyatoslav Ryhel wrote: > > > > [...] > > > >>>>> What is the problem you observe on tegra 3 ? > >>>> i2c line fails since it probes in spl with your patch, but it does not > >>>> relocate and then probes once more after relocation. Probe fails along > >>>> with all devices on same line. > >>> > >>> Could it be that you either have to: > >>> - Add DM_I2C to tegra 3 SPL > >>> - Remove bootph-* from DT to remove the regulator node from SPL > >>> - /delete-property/ regulator-always-on; and /delete-property/ > >>> regulator-boot-on; in -u-boot.dtsi to prevent the regulator from being > >>> enabled in SPL ? > >>> > >> Obviously NO, you propose nonsense. Same dts is used for both stages. > > > > DT source yes, DT blob likely no. > > > >> And I have to add hack-ish stuff just because you wanna introduce code > >> which causes known regressions. > > > > I am trying to understand what problem there is on tegra 3, but it is > > still not clear to me. > > > > Is the problem somehow related to PMICs (?) being probed in SPL (?) > > because they have regulators (?) which are marked as regulator-always-on > > ? If so, then this is correct behavior, and if this is not desired in > > SPL, then you can remove this property from SPL DT in -u-boot.dtsi using > > /delete-property/ . > > > > [...] > > > >> "We must not probe things as we go. There might be other > >> dependencies not yet bound. It may also take some time. This is not > >> following driver model design, sorry. > >> > >> So please think of a way to do this properly." > > > > What is this quote about ? Where is this from ? > > What is the problem with Tegra 3 and this patchset ? > > Can you please explain that so this patchset can move forward ? > with your changes U-Boot 2024.07-00696-ge217e2769db9-dirty (Aug 20 2024 - 09:55:29 +0300) SoC: tegra114 Model: NVIDIA Tegra Note 7 Board: NVIDIA TegraTab DRAM: 1 GiB tegra_i2c_probe: called i2c: controller bus 0 at 7000d000, speed 0: tegra_i2c_probe: exit i2c_init_controller: speed=400000 i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25 i2c_xfer: 2 messages i2c_xfer: chip=0x58, len=0x1 i2c_write_data: chip=0x58, len=0x1 write_data: 0x37 pkt header 1 sent (0x10010) pkt header 2 sent (0x0) pkt header 3 sent (0x100b0) pkt data sent (0x37) tegra_i2c_write_data: Error (-1) !!! i2c_write_data(): rc=-1 i2c_write: error sending read error from device: bd26f8e0 register: 0x37! i2c_xfer: 2 messages i2c_xfer: chip=0x58, len=0x1 i2c_write_data: chip=0x58, len=0x1 write_data: 0x3b pkt header 1 sent (0x10010) pkt header 2 sent (0x0) pkt header 3 sent (0x100b0) pkt data sent (0x3b) tegra_i2c_write_data: Error (-1) !!! i2c_write_data(): rc=-1 i2c_write: error sending read error from device: bd26f8e0 register: 0x3b! i2c_xfer: 2 messages i2c_xfer: chip=0x58, len=0x1 i2c_write_data: chip=0x58, len=0x1 write_data: 0x61 pkt header 1 sent (0x10010) pkt header 2 sent (0x0) pkt header 3 sent (0x100b0) pkt data sent (0x61) tegra_i2c_write_data: Error (-1) !!! i2c_write_data(): rc=-1 i2c_write: error sending read error from device: bd26f8e0 register: 0x61! i2c_xfer: 2 messages i2c_xfer: chip=0x58, len=0x1 i2c_write_data: chip=0x58, len=0x1 i2c_xfer: chip=0x58, len=0x1 i2c_write_data: chip=0x58, len=0x1 write_data: 0x65 pkt header 1 sent (0x10010) pkt header 2 sent (0x0) pkt header 3 sent (0x100b0) pkt data sent (0x65) tegra_i2c_write_data: Error (-1) !!! i2c_write_data(): rc=-1 i2c_write: error sending read error from device: bd26f8e0 register: 0x65! pkt header 1 sent (0x10010) pkt header 2 sent (0x0) pkt header 3 sent (0xb0) pkt data sent (0xbd) tegra_i2c_write_data: Error (-1) !!! without your changes U-Boot 2024.07-00696-g45c25f82f356-dirty (Aug 20 2024 - 09:58:40 +0300) SoC: tegra114 Model: NVIDIA Tegra Note 7 Board: NVIDIA TegraTab DRAM: 1 GiB tegra_i2c_probe: called i2c: controller bus 0 at 7000d000, speed 0: tegra_i2c_probe: exit i2c_init_controller: speed=400000 i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25 pkt header 1 sent (0x10010) pkt header 2 sent (0x0) pkt header 3 sent (0xb0) pkt data sent (0x0) i2c_xfer: 2 messages i2c_xfer: chip=0x58, len=0x1 i2c_write_data: chip=0x58, len=0x1 write_data: 0xfb pkt header 1 sent (0x10010) pkt header 2 sent (0x0) pkt header 3 sent (0x100b0) pkt data sent (0xfb) i2c_xfer: chip=0x58, len=0x1 inside i2c_read_data(): pkt header 1 sent (0x10010) pkt header 2 sent (0x0) pkt header 3 sent (0x800b1) pkt data received (0x2) i2c_read_data: 0x02 i2c_xfer: 1 messages i2c_xfer: chip=0x58, len=0x2 i2c_write_data: chip=0x58, len=0x2 write_data: 0xfb 0x02 pkt header 1 sent (0x10010) pkt header 2 sent (0x1) pkt header 3 sent (0xb0) pkt data sent (0x2fb) i2c_xfer: 2 messages i2c_xfer: chip=0x58, len=0x1 i2c_write_data: chip=0x58, len=0x1 write_data: 0xfe pkt header 1 sent (0x10010) pkt header 2 sent (0x0) pkt header 3 sent (0x100b0) pkt data sent (0xfe) i2c_xfer: chip=0x58, len=0x1 inside i2c_read_data(): pkt header 1 sent (0x10010) i2c driver refuses to work properly > Thank you
On 8/20/24 9:08 AM, Svyatoslav Ryhel wrote: > пн, 19 серп. 2024 р. о 20:27 Marek Vasut <marex@denx.de> пише: >> >> On 8/1/24 2:28 AM, Marek Vasut wrote: >>> On 7/29/24 1:55 PM, Svyatoslav Ryhel wrote: >>> >>> [...] >>> >>>>>>> What is the problem you observe on tegra 3 ? >>>>>> i2c line fails since it probes in spl with your patch, but it does not >>>>>> relocate and then probes once more after relocation. Probe fails along >>>>>> with all devices on same line. >>>>> >>>>> Could it be that you either have to: >>>>> - Add DM_I2C to tegra 3 SPL >>>>> - Remove bootph-* from DT to remove the regulator node from SPL >>>>> - /delete-property/ regulator-always-on; and /delete-property/ >>>>> regulator-boot-on; in -u-boot.dtsi to prevent the regulator from being >>>>> enabled in SPL ? >>>>> >>>> Obviously NO, you propose nonsense. Same dts is used for both stages. >>> >>> DT source yes, DT blob likely no. >>> >>>> And I have to add hack-ish stuff just because you wanna introduce code >>>> which causes known regressions. >>> >>> I am trying to understand what problem there is on tegra 3, but it is >>> still not clear to me. >>> >>> Is the problem somehow related to PMICs (?) being probed in SPL (?) >>> because they have regulators (?) which are marked as regulator-always-on >>> ? If so, then this is correct behavior, and if this is not desired in >>> SPL, then you can remove this property from SPL DT in -u-boot.dtsi using >>> /delete-property/ . >>> >>> [...] >>> >>>> "We must not probe things as we go. There might be other >>>> dependencies not yet bound. It may also take some time. This is not >>>> following driver model design, sorry. >>>> >>>> So please think of a way to do this properly." >>> >>> What is this quote about ? Where is this from ? >> >> What is the problem with Tegra 3 and this patchset ? >> >> Can you please explain that so this patchset can move forward ? >> > > with your changes > > U-Boot 2024.07-00696-ge217e2769db9-dirty (Aug 20 2024 - 09:55:29 +0300) > > SoC: tegra114 > Model: NVIDIA Tegra Note 7 > Board: NVIDIA TegraTab > DRAM: 1 GiB > tegra_i2c_probe: called > i2c: controller bus 0 at 7000d000, speed 0: tegra_i2c_probe: exit > i2c_init_controller: speed=400000 > i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25 > i2c_xfer: 2 messages > i2c_xfer: chip=0x58, len=0x1 > i2c_write_data: chip=0x58, len=0x1 > write_data: 0x37 > pkt header 1 sent (0x10010) > pkt header 2 sent (0x0) > pkt header 3 sent (0x100b0) > pkt data sent (0x37) > tegra_i2c_write_data: Error (-1) !!! > i2c_write_data(): rc=-1 > i2c_write: error sending > read error from device: bd26f8e0 register: 0x37! This seems like the PMIC driver (palmas?) is trying to read register 0x37 PGOOD and the I2C transfer fails . Why does the I2C transfer fail ? You did mention something regarding I2C/PMIC driver probe timing, but it seems the I2C driver and PMIC drivers probe roughly around the same time in both pass and fail cases ? It seems the tegra3 DTs have most of the PMIC regulators marked as boot-on and always-on , so enabling the regulators early is the right thing to do. [...] > SoC: tegra114 > Model: NVIDIA Tegra Note 7 > Board: NVIDIA TegraTab > DRAM: 1 GiB > tegra_i2c_probe: called > i2c: controller bus 0 at 7000d000, speed 0: tegra_i2c_probe: exit > i2c_init_controller: speed=400000 > i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25 > pkt header 1 sent (0x10010) > pkt header 2 sent (0x0) > pkt header 3 sent (0xb0) > pkt data sent (0x0) > i2c_xfer: 2 messages > i2c_xfer: chip=0x58, len=0x1 > i2c_write_data: chip=0x58, len=0x1 > write_data: 0xfb This seems to be access to register 0xfb , i.e. something else ?
пн, 9 вер. 2024 р. о 19:13 Marek Vasut <marex@denx.de> пише: > > On 8/20/24 9:08 AM, Svyatoslav Ryhel wrote: > > пн, 19 серп. 2024 р. о 20:27 Marek Vasut <marex@denx.de> пише: > >> > >> On 8/1/24 2:28 AM, Marek Vasut wrote: > >>> On 7/29/24 1:55 PM, Svyatoslav Ryhel wrote: > >>> > >>> [...] > >>> > >>>>>>> What is the problem you observe on tegra 3 ? > >>>>>> i2c line fails since it probes in spl with your patch, but it does not > >>>>>> relocate and then probes once more after relocation. Probe fails along > >>>>>> with all devices on same line. > >>>>> > >>>>> Could it be that you either have to: > >>>>> - Add DM_I2C to tegra 3 SPL > >>>>> - Remove bootph-* from DT to remove the regulator node from SPL > >>>>> - /delete-property/ regulator-always-on; and /delete-property/ > >>>>> regulator-boot-on; in -u-boot.dtsi to prevent the regulator from being > >>>>> enabled in SPL ? > >>>>> > >>>> Obviously NO, you propose nonsense. Same dts is used for both stages. > >>> > >>> DT source yes, DT blob likely no. > >>> > >>>> And I have to add hack-ish stuff just because you wanna introduce code > >>>> which causes known regressions. > >>> > >>> I am trying to understand what problem there is on tegra 3, but it is > >>> still not clear to me. > >>> > >>> Is the problem somehow related to PMICs (?) being probed in SPL (?) > >>> because they have regulators (?) which are marked as regulator-always-on > >>> ? If so, then this is correct behavior, and if this is not desired in > >>> SPL, then you can remove this property from SPL DT in -u-boot.dtsi using > >>> /delete-property/ . > >>> > >>> [...] > >>> > >>>> "We must not probe things as we go. There might be other > >>>> dependencies not yet bound. It may also take some time. This is not > >>>> following driver model design, sorry. > >>>> > >>>> So please think of a way to do this properly." > >>> > >>> What is this quote about ? Where is this from ? > >> > >> What is the problem with Tegra 3 and this patchset ? > >> > >> Can you please explain that so this patchset can move forward ? > >> > > > > with your changes > > > > U-Boot 2024.07-00696-ge217e2769db9-dirty (Aug 20 2024 - 09:55:29 +0300) > > > > SoC: tegra114 > > Model: NVIDIA Tegra Note 7 > > Board: NVIDIA TegraTab > > DRAM: 1 GiB > > tegra_i2c_probe: called > > i2c: controller bus 0 at 7000d000, speed 0: tegra_i2c_probe: exit > > i2c_init_controller: speed=400000 > > i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25 > > i2c_xfer: 2 messages > > i2c_xfer: chip=0x58, len=0x1 > > i2c_write_data: chip=0x58, len=0x1 > > write_data: 0x37 > > pkt header 1 sent (0x10010) > > pkt header 2 sent (0x0) > > pkt header 3 sent (0x100b0) > > pkt data sent (0x37) > > tegra_i2c_write_data: Error (-1) !!! > > i2c_write_data(): rc=-1 > > i2c_write: error sending > > read error from device: bd26f8e0 register: 0x37! > > This seems like the PMIC driver (palmas?) is trying to read register > 0x37 PGOOD and the I2C transfer fails . Why does the I2C transfer fail ? You are asking me? Because your patches break some important setup sequence. PMIC model does not matter, all behave same way. > You did mention something regarding I2C/PMIC driver probe timing, but it > seems the I2C driver and PMIC drivers probe roughly around the same time > in both pass and fail cases ? Yes, here I agree that they both probe and probe passes, but I assume timing of i2c call is critical and there may be some dependency which is not ready. > It seems the tegra3 DTs have most of the PMIC regulators marked as > boot-on and always-on , so enabling the regulators early is the right > thing to do. Only essentials are added, thus they are marked this way. > > [...] > > > SoC: tegra114 > > Model: NVIDIA Tegra Note 7 > > Board: NVIDIA TegraTab > > DRAM: 1 GiB > > tegra_i2c_probe: called > > i2c: controller bus 0 at 7000d000, speed 0: tegra_i2c_probe: exit > > i2c_init_controller: speed=400000 > > i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25 > > pkt header 1 sent (0x10010) > > pkt header 2 sent (0x0) > > pkt header 3 sent (0xb0) > > pkt data sent (0x0) > > i2c_xfer: 2 messages > > i2c_xfer: chip=0x58, len=0x1 > > i2c_write_data: chip=0x58, len=0x1 > > write_data: 0xfb > This seems to be access to register 0xfb , i.e. something else ?
On 9/10/24 11:05 AM, Svyatoslav Ryhel wrote: > пн, 9 вер. 2024 р. о 19:13 Marek Vasut <marex@denx.de> пише: >> >> On 8/20/24 9:08 AM, Svyatoslav Ryhel wrote: >>> пн, 19 серп. 2024 р. о 20:27 Marek Vasut <marex@denx.de> пише: >>>> >>>> On 8/1/24 2:28 AM, Marek Vasut wrote: >>>>> On 7/29/24 1:55 PM, Svyatoslav Ryhel wrote: >>>>> >>>>> [...] >>>>> >>>>>>>>> What is the problem you observe on tegra 3 ? >>>>>>>> i2c line fails since it probes in spl with your patch, but it does not >>>>>>>> relocate and then probes once more after relocation. Probe fails along >>>>>>>> with all devices on same line. >>>>>>> >>>>>>> Could it be that you either have to: >>>>>>> - Add DM_I2C to tegra 3 SPL >>>>>>> - Remove bootph-* from DT to remove the regulator node from SPL >>>>>>> - /delete-property/ regulator-always-on; and /delete-property/ >>>>>>> regulator-boot-on; in -u-boot.dtsi to prevent the regulator from being >>>>>>> enabled in SPL ? >>>>>>> >>>>>> Obviously NO, you propose nonsense. Same dts is used for both stages. >>>>> >>>>> DT source yes, DT blob likely no. >>>>> >>>>>> And I have to add hack-ish stuff just because you wanna introduce code >>>>>> which causes known regressions. >>>>> >>>>> I am trying to understand what problem there is on tegra 3, but it is >>>>> still not clear to me. >>>>> >>>>> Is the problem somehow related to PMICs (?) being probed in SPL (?) >>>>> because they have regulators (?) which are marked as regulator-always-on >>>>> ? If so, then this is correct behavior, and if this is not desired in >>>>> SPL, then you can remove this property from SPL DT in -u-boot.dtsi using >>>>> /delete-property/ . >>>>> >>>>> [...] >>>>> >>>>>> "We must not probe things as we go. There might be other >>>>>> dependencies not yet bound. It may also take some time. This is not >>>>>> following driver model design, sorry. >>>>>> >>>>>> So please think of a way to do this properly." >>>>> >>>>> What is this quote about ? Where is this from ? >>>> >>>> What is the problem with Tegra 3 and this patchset ? >>>> >>>> Can you please explain that so this patchset can move forward ? >>>> >>> >>> with your changes >>> >>> U-Boot 2024.07-00696-ge217e2769db9-dirty (Aug 20 2024 - 09:55:29 +0300) >>> >>> SoC: tegra114 >>> Model: NVIDIA Tegra Note 7 >>> Board: NVIDIA TegraTab >>> DRAM: 1 GiB >>> tegra_i2c_probe: called >>> i2c: controller bus 0 at 7000d000, speed 0: tegra_i2c_probe: exit >>> i2c_init_controller: speed=400000 >>> i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25 >>> i2c_xfer: 2 messages >>> i2c_xfer: chip=0x58, len=0x1 >>> i2c_write_data: chip=0x58, len=0x1 >>> write_data: 0x37 >>> pkt header 1 sent (0x10010) >>> pkt header 2 sent (0x0) >>> pkt header 3 sent (0x100b0) >>> pkt data sent (0x37) >>> tegra_i2c_write_data: Error (-1) !!! >>> i2c_write_data(): rc=-1 >>> i2c_write: error sending >>> read error from device: bd26f8e0 register: 0x37! >> >> This seems like the PMIC driver (palmas?) is trying to read register >> 0x37 PGOOD and the I2C transfer fails . Why does the I2C transfer fail ? > > You are asking me? Because your patches break some important setup sequence. > PMIC model does not matter, all behave same way. These regulator patches do not modify anything related to I2C and I don't observe this kind of behavior on iMX8M or STM32 platforms, so I suspect this is something specific to tegra. >> You did mention something regarding I2C/PMIC driver probe timing, but it >> seems the I2C driver and PMIC drivers probe roughly around the same time >> in both pass and fail cases ? > > Yes, here I agree that they both probe and probe passes, but I assume > timing of i2c call is critical and there may be some dependency which > is not ready. My guess would be pinmux or clock, maybe the i2c controller is marked as bootph-* in DT and its pinmux/clock is not? Maybe the i2c on tegra works by sheer coincidence right now? Can you have a look?
вт, 10 вер. 2024 р. о 13:18 Marek Vasut <marex@denx.de> пише: > > On 9/10/24 11:05 AM, Svyatoslav Ryhel wrote: > > пн, 9 вер. 2024 р. о 19:13 Marek Vasut <marex@denx.de> пише: > >> > >> On 8/20/24 9:08 AM, Svyatoslav Ryhel wrote: > >>> пн, 19 серп. 2024 р. о 20:27 Marek Vasut <marex@denx.de> пише: > >>>> > >>>> On 8/1/24 2:28 AM, Marek Vasut wrote: > >>>>> On 7/29/24 1:55 PM, Svyatoslav Ryhel wrote: > >>>>> > >>>>> [...] > >>>>> > >>>>>>>>> What is the problem you observe on tegra 3 ? > >>>>>>>> i2c line fails since it probes in spl with your patch, but it does not > >>>>>>>> relocate and then probes once more after relocation. Probe fails along > >>>>>>>> with all devices on same line. > >>>>>>> > >>>>>>> Could it be that you either have to: > >>>>>>> - Add DM_I2C to tegra 3 SPL > >>>>>>> - Remove bootph-* from DT to remove the regulator node from SPL > >>>>>>> - /delete-property/ regulator-always-on; and /delete-property/ > >>>>>>> regulator-boot-on; in -u-boot.dtsi to prevent the regulator from being > >>>>>>> enabled in SPL ? > >>>>>>> > >>>>>> Obviously NO, you propose nonsense. Same dts is used for both stages. > >>>>> > >>>>> DT source yes, DT blob likely no. > >>>>> > >>>>>> And I have to add hack-ish stuff just because you wanna introduce code > >>>>>> which causes known regressions. > >>>>> > >>>>> I am trying to understand what problem there is on tegra 3, but it is > >>>>> still not clear to me. > >>>>> > >>>>> Is the problem somehow related to PMICs (?) being probed in SPL (?) > >>>>> because they have regulators (?) which are marked as regulator-always-on > >>>>> ? If so, then this is correct behavior, and if this is not desired in > >>>>> SPL, then you can remove this property from SPL DT in -u-boot.dtsi using > >>>>> /delete-property/ . > >>>>> > >>>>> [...] > >>>>> > >>>>>> "We must not probe things as we go. There might be other > >>>>>> dependencies not yet bound. It may also take some time. This is not > >>>>>> following driver model design, sorry. > >>>>>> > >>>>>> So please think of a way to do this properly." > >>>>> > >>>>> What is this quote about ? Where is this from ? > >>>> > >>>> What is the problem with Tegra 3 and this patchset ? > >>>> > >>>> Can you please explain that so this patchset can move forward ? > >>>> > >>> > >>> with your changes > >>> > >>> U-Boot 2024.07-00696-ge217e2769db9-dirty (Aug 20 2024 - 09:55:29 +0300) > >>> > >>> SoC: tegra114 > >>> Model: NVIDIA Tegra Note 7 > >>> Board: NVIDIA TegraTab > >>> DRAM: 1 GiB > >>> tegra_i2c_probe: called > >>> i2c: controller bus 0 at 7000d000, speed 0: tegra_i2c_probe: exit > >>> i2c_init_controller: speed=400000 > >>> i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25 > >>> i2c_xfer: 2 messages > >>> i2c_xfer: chip=0x58, len=0x1 > >>> i2c_write_data: chip=0x58, len=0x1 > >>> write_data: 0x37 > >>> pkt header 1 sent (0x10010) > >>> pkt header 2 sent (0x0) > >>> pkt header 3 sent (0x100b0) > >>> pkt data sent (0x37) > >>> tegra_i2c_write_data: Error (-1) !!! > >>> i2c_write_data(): rc=-1 > >>> i2c_write: error sending > >>> read error from device: bd26f8e0 register: 0x37! > >> > >> This seems like the PMIC driver (palmas?) is trying to read register > >> 0x37 PGOOD and the I2C transfer fails . Why does the I2C transfer fail ? > > > > You are asking me? Because your patches break some important setup sequence. > > PMIC model does not matter, all behave same way. > > These regulator patches do not modify anything related to I2C and I > don't observe this kind of behavior on iMX8M or STM32 platforms, so I > suspect this is something specific to tegra. > > >> You did mention something regarding I2C/PMIC driver probe timing, but it > >> seems the I2C driver and PMIC drivers probe roughly around the same time > >> in both pass and fail cases ? > > > > Yes, here I agree that they both probe and probe passes, but I assume > > timing of i2c call is critical and there may be some dependency which > > is not ready. > > My guess would be pinmux or clock, maybe the i2c controller is marked as > bootph-* in DT and its pinmux/clock is not? Maybe the i2c on tegra works > by sheer coincidence right now? Can you have a look? Power i2c line (one that hosts PMIC) is configured extremely early in SPL since it is needed for cpu and core voltage setup so even if, as you say, tegra works by sheer coincidence, specifically this i2c line should work non the less, since it has all its pre-requisites (clock and pinmux) configured on early stage. As I have told, I was not able to determine exact reason why this happens, it should not and yet it does. This is why I have abandoned my attempt to implement same changes you are currently proposing.
On 9/11/24 7:57 AM, Svyatoslav Ryhel wrote: [...] >>>> You did mention something regarding I2C/PMIC driver probe timing, but it >>>> seems the I2C driver and PMIC drivers probe roughly around the same time >>>> in both pass and fail cases ? >>> >>> Yes, here I agree that they both probe and probe passes, but I assume >>> timing of i2c call is critical and there may be some dependency which >>> is not ready. >> >> My guess would be pinmux or clock, maybe the i2c controller is marked as >> bootph-* in DT and its pinmux/clock is not? Maybe the i2c on tegra works >> by sheer coincidence right now? Can you have a look? > > Power i2c line (one that hosts PMIC) is configured extremely early in > SPL since it is needed for cpu and core voltage setup so even if, as > you say, tegra works by sheer coincidence, specifically this i2c line > should work non the less, since it has all its pre-requisites (clock > and pinmux) configured on early stage. Is it possible that this configuration is somehow reset or reconfigured from DT early on in U-Boot proper ? Do you have serial console output in board_f.c time in U-Boot proper , possibly using DEUBG_UART , to check if there might be some prior failing I2C transfer at that board_f.c time ? > As I have told, I was not able to determine exact reason why this > happens, it should not and yet it does. This is why I have abandoned > my attempt to implement same changes you are currently proposing. If tegra has a problem, it should be fixed on tegra side and not block core plumbing. I am not seeing the problem on stm32 or imx systems, so I am banking toward tegra-specific issue. Are you able to debug this ?
ср, 11 вер. 2024 р. о 14:01 Marek Vasut <marex@denx.de> пише: > > On 9/11/24 7:57 AM, Svyatoslav Ryhel wrote: > > [...] > > >>>> You did mention something regarding I2C/PMIC driver probe timing, but it > >>>> seems the I2C driver and PMIC drivers probe roughly around the same time > >>>> in both pass and fail cases ? > >>> > >>> Yes, here I agree that they both probe and probe passes, but I assume > >>> timing of i2c call is critical and there may be some dependency which > >>> is not ready. > >> > >> My guess would be pinmux or clock, maybe the i2c controller is marked as > >> bootph-* in DT and its pinmux/clock is not? Maybe the i2c on tegra works > >> by sheer coincidence right now? Can you have a look? > > > > Power i2c line (one that hosts PMIC) is configured extremely early in > > SPL since it is needed for cpu and core voltage setup so even if, as > > you say, tegra works by sheer coincidence, specifically this i2c line > > should work non the less, since it has all its pre-requisites (clock > > and pinmux) configured on early stage. > > Is it possible that this configuration is somehow reset or reconfigured > from DT early on in U-Boot proper ? No > Do you have serial console output in board_f.c time in U-Boot proper , > possibly using DEUBG_UART , to check if there might be some prior > failing I2C transfer at that board_f.c time ? Haven't spotted anything weird there. > > As I have told, I was not able to determine exact reason why this > > happens, it should not and yet it does. This is why I have abandoned > > my attempt to implement same changes you are currently proposing. > > If tegra has a problem, it should be fixed on tegra side and not block > core plumbing. I am not seeing the problem on stm32 or imx systems, so I > am banking toward tegra-specific issue. > And yet you are pushing tegra breaking stuff. I will insist on reverting this is it passes. > Are you able to debug this ? No, I am not able find exact cuse of this behavior.
On Wed, Sep 11, 2024 at 02:12:12PM +0300, Svyatoslav Ryhel wrote: > ср, 11 вер. 2024 р. о 14:01 Marek Vasut <marex@denx.de> пише: > > > > On 9/11/24 7:57 AM, Svyatoslav Ryhel wrote: > > > > [...] > > > > >>>> You did mention something regarding I2C/PMIC driver probe timing, but it > > >>>> seems the I2C driver and PMIC drivers probe roughly around the same time > > >>>> in both pass and fail cases ? > > >>> > > >>> Yes, here I agree that they both probe and probe passes, but I assume > > >>> timing of i2c call is critical and there may be some dependency which > > >>> is not ready. > > >> > > >> My guess would be pinmux or clock, maybe the i2c controller is marked as > > >> bootph-* in DT and its pinmux/clock is not? Maybe the i2c on tegra works > > >> by sheer coincidence right now? Can you have a look? > > > > > > Power i2c line (one that hosts PMIC) is configured extremely early in > > > SPL since it is needed for cpu and core voltage setup so even if, as > > > you say, tegra works by sheer coincidence, specifically this i2c line > > > should work non the less, since it has all its pre-requisites (clock > > > and pinmux) configured on early stage. > > > > Is it possible that this configuration is somehow reset or reconfigured > > from DT early on in U-Boot proper ? > > No > > > Do you have serial console output in board_f.c time in U-Boot proper , > > possibly using DEUBG_UART , to check if there might be some prior > > failing I2C transfer at that board_f.c time ? > > Haven't spotted anything weird there. > > > > As I have told, I was not able to determine exact reason why this > > > happens, it should not and yet it does. This is why I have abandoned > > > my attempt to implement same changes you are currently proposing. > > > > If tegra has a problem, it should be fixed on tegra side and not block > > core plumbing. I am not seeing the problem on stm32 or imx systems, so I > > am banking toward tegra-specific issue. > > > > And yet you are pushing tegra breaking stuff. I will insist on > reverting this is it passes. > > > Are you able to debug this ? > > No, I am not able find exact cuse of this behavior. How do you propose we resolve this then, Svyatoslav? I threw this patch at some TI platforms as well and they're all fine. Are you unable to get some early debuging information out like Marek was asking? Thanks.
Hi Tom, On Wed, 11 Sept 2024 at 10:25, Tom Rini <trini@konsulko.com> wrote: > > On Wed, Sep 11, 2024 at 02:12:12PM +0300, Svyatoslav Ryhel wrote: > > ср, 11 вер. 2024 р. о 14:01 Marek Vasut <marex@denx.de> пише: > > > > > > On 9/11/24 7:57 AM, Svyatoslav Ryhel wrote: > > > > > > [...] > > > > > > >>>> You did mention something regarding I2C/PMIC driver probe timing, but it > > > >>>> seems the I2C driver and PMIC drivers probe roughly around the same time > > > >>>> in both pass and fail cases ? > > > >>> > > > >>> Yes, here I agree that they both probe and probe passes, but I assume > > > >>> timing of i2c call is critical and there may be some dependency which > > > >>> is not ready. > > > >> > > > >> My guess would be pinmux or clock, maybe the i2c controller is marked as > > > >> bootph-* in DT and its pinmux/clock is not? Maybe the i2c on tegra works > > > >> by sheer coincidence right now? Can you have a look? > > > > > > > > Power i2c line (one that hosts PMIC) is configured extremely early in > > > > SPL since it is needed for cpu and core voltage setup so even if, as > > > > you say, tegra works by sheer coincidence, specifically this i2c line > > > > should work non the less, since it has all its pre-requisites (clock > > > > and pinmux) configured on early stage. > > > > > > Is it possible that this configuration is somehow reset or reconfigured > > > from DT early on in U-Boot proper ? > > > > No > > > > > Do you have serial console output in board_f.c time in U-Boot proper , > > > possibly using DEUBG_UART , to check if there might be some prior > > > failing I2C transfer at that board_f.c time ? > > > > Haven't spotted anything weird there. > > > > > > As I have told, I was not able to determine exact reason why this > > > > happens, it should not and yet it does. This is why I have abandoned > > > > my attempt to implement same changes you are currently proposing. > > > > > > If tegra has a problem, it should be fixed on tegra side and not block > > > core plumbing. I am not seeing the problem on stm32 or imx systems, so I > > > am banking toward tegra-specific issue. > > > > > > > And yet you are pushing tegra breaking stuff. I will insist on > > reverting this is it passes. > > > > > Are you able to debug this ? > > > > No, I am not able find exact cuse of this behavior. > > How do you propose we resolve this then, Svyatoslav? I threw this patch > at some TI platforms as well and they're all fine. Are you unable to get > some early debuging information out like Marek was asking? Thanks. At this point I would like to have an optional Kconfig to enable the always-on regulators in the init sequence, perhaps as part of initf_dm(). It should not be in DM core, sorry. > -- > Tom
Hi Marek, On Fri, 28 Jun 2024 at 07:26, Marek Vasut <marex@denx.de> 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] https://patchwork.ozlabs.org/project/uboot/patch/20220422131555.123598-1-marex@denx.de/
On 9/12/24 2:59 AM, Simon Glass wrote: > Hi Tom, Hello Simon, >> How do you propose we resolve this then, Svyatoslav? I threw this patch >> at some TI platforms as well and they're all fine. Are you unable to get >> some early debuging information out like Marek was asking? Thanks. > > At this point I would like to have an optional Kconfig to enable the > always-on regulators in the init sequence, perhaps as part of > initf_dm(). That would only move the current regulators_enable_boot_on() elsewhere without fixing the real issue which this series does address -- that the regulators which are marked as always-on/boot-on should always be enabled on boot, unconditionally. If there are regulators which should not be enabled on boot, they should not be marked always-on/boot-on in DT in the first place. The regulators_enable_boot_on() is missing or forgotten in multiple board files, which makes boards randomly misbehave due to disabled always-on regulators. Adding a Kconfig option would convert this problem to "new Kconfig option is missing in multiple configs" problem, which is effectively identical problem, only moved elsewhere. It should be the core code that handles this the same way for all boards and configs. The core has the tools for it too, the DM_FLAG_PROBE_AFTER_BIND flag which is already used for the same purpose for LEDs, pinctrl, PMICs, etc., which makes regulators_enable_boot_on() unnecessary. The DM_FLAG_PROBE_AFTER_BIND should be used more instead of ad-hoc callbacks in random places of the init sequence, those do not scale. > It should not be in DM core, sorry. This is in regulator uclass, not in DM core ? This discussion thread is about debugging tegra i2c, how is your comment related to the discussion here ?
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@denx.de> 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!
пн, 16 вер. 2024 р. о 19:28 Tom Rini <trini@konsulko.com> пише: > > 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@denx.de> 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?
On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote: > пн, 16 вер. 2024 р. о 19:28 Tom Rini <trini@konsulko.com> пише: > > > > 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@denx.de> 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?
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@konsulko.com> пише: > > > > > > 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@denx.de> 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 https://patchwork.ozlabs.org/project/uboot/patch/20240924220905.514122-1-marex@denx.de/ 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.
ср, 25 вер. 2024 р. о 02:44 Tom Rini <trini@konsulko.com> пише: > > 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@konsulko.com> пише: > > > > > > > > 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@denx.de> 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 > https://patchwork.ozlabs.org/project/uboot/patch/20240924220905.514122-1-marex@denx.de/ > 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
Hi, On Fri, 20 Sept 2024 at 18:49, Tom Rini <trini@konsulko.com> wrote: > > On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote: > > пн, 16 вер. 2024 р. о 19:28 Tom Rini <trini@konsulko.com> пише: > > > > > > 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@denx.de> 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? We did have a pmic maintainer for a while, but perhaps has gone quiet? I am OK with a call to regulators_enable_boot_on(), but where should it go? Should it happen in SPL? Should it happen before relocation? How is that decided and controlled? I actually don't like DM_FLAG_PROBE_AFTER_BIND since it makes bind == probe, something which I believe U-Boot should really try to avoid. I would even rather have an explicit call in initf_dm() - or perhaps better initr_dm()? My favourite option would be a new line in the board_r init sequence instead of power_init_board(), which predates driver model. Regards, Simon
On 9/25/24 12:18 PM, Svyatoslav Ryhel wrote: [...] > 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 Is it possible this is called earlier, before UART output is initialized, so it does not show up in the log below ? Could it be that it is called before your pinmux is properly initialized, hence no UART, and that is why the I2C communication fails? So far, I only found Toradex Tegra T20 board here, no T30.
ср, 25 вер. 2024 р. о 15:48 Marek Vasut <marex@denx.de> пише: > > On 9/25/24 12:18 PM, Svyatoslav Ryhel wrote: > > [...] > > > 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 > > Is it possible this is called earlier, before UART output is > initialized, so it does not show up in the log below ? > It is highly unlikely, this function is called when uart is already working and pinmux is set. > Could it be that it is called before your pinmux is properly > initialized, hence no UART, and that is why the I2C communication fails? > But if we assume that pinmux is not set, then how we resolve situation when pinmux and regulator enable are both set by probe after bind and regulators are probed before pinmux. Regulators will fail, they will not deffer till pinmux probes. Or you propose return to pre-DM version of pinmux? Inconsistency. > So far, I only found Toradex Tegra T20 board here, no T30. What do you mean? Specify please
On 2024-09-25 12:18, Svyatoslav Ryhel wrote: > 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. The i2c_get_chip_for_busnum call is typically protected by CONFIG_IS_ENABLED(DM_I2C), do you have DM_I2C and SPL_DM_I2C enabled? grep DM_I2C .config > > 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) What board target / defconfig is used? Would like to understand what bootph- props are used in the built control fdt. > > 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) !!! Is this at SPL, U-Boot pre-reloc or after relocation? A more complete boot log may help us understand when this happens. > > 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 Is the working scenario happening at same call site as not working? Try with #define LOG_DEBUG in lib/initcall.c and compare/include a longer part of the boot log. Regards, Jonas > > 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 9/25/24 3:04 PM, Svyatoslav Ryhel wrote: > ср, 25 вер. 2024 р. о 15:48 Marek Vasut <marex@denx.de> пише: >> >> On 9/25/24 12:18 PM, Svyatoslav Ryhel wrote: >> >> [...] >> >>> 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 >> >> Is it possible this is called earlier, before UART output is >> initialized, so it does not show up in the log below ? >> > It is highly unlikely, this function is called when uart is already > working and pinmux is set. It could be called earlier now, try to enable CONFIG_BOOTSTAGE and add BOOTSTAGE_MARKER() to points of interest (like the I2C functions). You should be able to see whether that function was called in 'bootstage' command output, even if UART wasn't active at that point yet. >> Could it be that it is called before your pinmux is properly >> initialized, hence no UART, and that is why the I2C communication fails? >> > But if we assume that pinmux is not set, then how we resolve situation > when pinmux and regulator enable are both set by probe after bind and > regulators are probed before pinmux. Regulators will fail, they will > not deffer till pinmux probes. Or you propose return to pre-DM version > of pinmux? Inconsistency. I am not going to guess what the problem is, currently it is unclear. Once we know what the problem is, we can determine the best solution. >> So far, I only found Toradex Tegra T20 board here, no T30. > What do you mean? Specify please I have Toradex Tegra T20 SoM and board here, probably not useful.
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");
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@denx.de> --- Cc: Ben Wolsieffer <benwolsieffer@gmail.com> Cc: Caleb Connolly <caleb.connolly@linaro.org> Cc: Chris Morgan <macromorgan@hotmail.com> Cc: Dragan Simic <dsimic@manjaro.org> Cc: Eugen Hristev <eugen.hristev@collabora.com> Cc: Francesco Dolcini <francesco.dolcini@toradex.com> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> Cc: Jaehoon Chung <jh80.chung@samsung.com> Cc: Jagan Teki <jagan@amarulasolutions.com> Cc: Jonas Karlman <jonas@kwiboo.se> Cc: Kever Yang <kever.yang@rock-chips.com> Cc: Kostya Porotchkin <kostap@marvell.com> Cc: Matteo Lisi <matteo.lisi@engicam.com> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com> Cc: Max Krummenacher <max.krummenacher@toradex.com> Cc: Neil Armstrong <neil.armstrong@linaro.org> Cc: Patrice Chotard <patrice.chotard@foss.st.com> Cc: Patrick Delaunay <patrick.delaunay@foss.st.com> Cc: Philipp Tomsich <philipp.tomsich@vrull.eu> Cc: Quentin Schulz <quentin.schulz@cherry.de> Cc: Sam Day <me@samcday.com> Cc: Simon Glass <sjg@chromium.org> Cc: Sumit Garg <sumit.garg@linaro.org> Cc: Svyatoslav Ryhel <clamor95@gmail.com> Cc: Thierry Reding <treding@nvidia.com> Cc: Tom Rini <trini@konsulko.com> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> Cc: u-boot-amlogic@groups.io Cc: u-boot-qcom@groups.io Cc: u-boot@dh-electronics.com Cc: u-boot@lists.denx.de Cc: uboot-stm32@st-md-mailman.stormreply.com --- drivers/power/regulator/regulator-uclass.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)