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

[Marek Vasut]

On 9/25/24 3:04 PM, Svyatoslav Ryhel wrote:

ср, 25 вер. 2024 р. о 15:48 Marek Vasut  пише:


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.

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

[Jonas Karlman]

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=40
> (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=40
> (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

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

[Svyatoslav Ryhel]

ср, 25 вер. 2024 р. о 15:48 Marek Vasut  пише:
>
> 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

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

[Marek Vasut]

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.

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

[Simon Glass]

Hi,

On Fri, 20 Sept 2024 at 18:49, Tom Rini  wrote:
>
> On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote:
> > пн, 16 вер. 2024 р. о 19:28 Tom Rini  пише:
> > >
> > > 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  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.

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

[Svyatoslav Ryhel]

ср, 25 вер. 2024 р. о 02:44 Tom Rini  пише:
>
> 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  пише:
> > > >
> > > > 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  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 

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

[Tom Rini]

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  пише:
> > >
> > > 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  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, e

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

[Tom Rini]

On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote:
> пн, 16 вер. 2024 р. о 19:28 Tom Rini  пише:
> >
> > 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  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
> > currentl

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

[Svyatoslav Ryhel]

пн, 16 вер. 2024 р. о 19:28 Tom Rini  пише:
>
> 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  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?

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

[Tom Rini]

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


signature.asc
Description: PGP signature

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

[Marek Vasut]

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.

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

[Marek Vasut]

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 ?

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

[Simon Glass]

Hi Marek,

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

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

[Simon Glass]

Hi Tom,

On Wed, 11 Sept 2024 at 10:25, Tom Rini  wrote:
>
> On Wed, Sep 11, 2024 at 02:12:12PM +0300, Svyatoslav Ryhel wrote:
> > ср, 11 вер. 2024 р. о 14:01 Marek Vasut  пише:
> > >
> > > 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

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

[Tom Rini]

On Wed, Sep 11, 2024 at 02:12:12PM +0300, Svyatoslav Ryhel wrote:
> ср, 11 вер. 2024 р. о 14:01 Marek Vasut  пише:
> >
> > 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.

-- 
Tom


signature.asc
Description: PGP signature

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

[Svyatoslav Ryhel]

ср, 11 вер. 2024 р. о 14:01 Marek Vasut  пише:
>
> 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.

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

[Marek Vasut]

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 ?

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

[Svyatoslav Ryhel]

вт, 10 вер. 2024 р. о 13:18 Marek Vasut  пише:
>
> On 9/10/24 11:05 AM, Svyatoslav Ryhel wrote:
> > пн, 9 вер. 2024 р. о 19:13 Marek Vasut  пише:
> >>
> >> On 8/20/24 9:08 AM, Svyatoslav Ryhel wrote:
> >>> пн, 19 серп. 2024 р. о 20:27 Marek Vasut  пише:
> 
>  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=40
> >>> 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.

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

[Marek Vasut]

On 9/10/24 11:05 AM, Svyatoslav Ryhel wrote:

пн, 9 вер. 2024 р. о 19:13 Marek Vasut  пише:


On 8/20/24 9:08 AM, Svyatoslav Ryhel wrote:

пн, 19 серп. 2024 р. о 20:27 Marek Vasut  пише:


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=40
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?

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

[Svyatoslav Ryhel]

пн, 9 вер. 2024 р. о 19:13 Marek Vasut  пише:
>
> On 8/20/24 9:08 AM, Svyatoslav Ryhel wrote:
> > пн, 19 серп. 2024 р. о 20:27 Marek Vasut  пише:
> >>
> >> 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=40
> > 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=40
> > 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 ?

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

[Marek Vasut]

On 8/20/24 9:08 AM, Svyatoslav Ryhel wrote:

пн, 19 серп. 2024 р. о 20:27 Marek Vasut  пише:


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=40
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=40
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 ?

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

[Svyatoslav Ryhel]

пн, 19 серп. 2024 р. о 20:27 Marek Vasut  пише:
>
> 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=40
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=40
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=0x

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

[Marek Vasut]

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

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

[Marek Vasut]

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 ?

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

[Svyatoslav Ryhel]

пн, 29 лип. 2024 р. о 13:42 Marek Vasut  пише:
>
> 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."

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

[Marek Vasut]

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.

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

[Svyatoslav Ryhel]

нд, 28 лип. 2024 р. о 23:08 Marek Vasut  пише:
>
> 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  
> > написав(-ла):
> >> On 7/28/24 7:55 PM, Svyatoslav Ryhel wrote:
> >>> нд, 28 лип. 2024 р. о 19:38 Marek Vasut  пише:
> 
>  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.
> >
> > 
> >
> > 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.

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

[Svyatoslav]

28 липня 2024 р. 21:35:27 GMT+03:00, Marek Vasut  написав(-ла):
>On 7/28/24 7:55 PM, Svyatoslav Ryhel wrote:
>> нд, 28 лип. 2024 р. о 19:38 Marek Vasut  пише:
>>> 
>>> 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.



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.

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

[Marek Vasut]

On 7/28/24 7:55 PM, Svyatoslav Ryhel wrote:

нд, 28 лип. 2024 р. о 19:38 Marek Vasut  пише:


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?

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

[Svyatoslav Ryhel]

нд, 28 лип. 2024 р. о 19:38 Marek Vasut  пише:
>
> 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.

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

[Svyatoslav Ryhel]

нд, 28 лип. 2024 р. о 19:38 Marek Vasut  пише:
>
> 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.

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

[Marek Vasut]

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 ?

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

[Marek Vasut]

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  написав(-ла):

On 7/28/24 7:55 PM, Svyatoslav Ryhel wrote:

нд, 28 лип. 2024 р. о 19:38 Marek Vasut  пише:


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.



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 ?

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

[Marek Vasut]

On 6/28/24 9:32 AM, Simon Glass wrote:

Hi Marek,


Hi,

[...]


@@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
  -ENODATA);
  uc_pdata->max_uA = dev_read_u32_default(dev, "regulator-max-microamp",
  -ENODATA);
-   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
-   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
  uc_pdata->ramp_delay = dev_read_u32_default(dev, 
"regulator-ramp-delay",
  0);
  uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
--
2.43.0



This is reading a lot of DT stuff very early, which may be slow. It is
also reading from the DT in the bind() step which we sometimes have to
do, but try to avoid.


Actually, it is reading only the bare minimum very early in bind, the
always-on and boot-on, the rest is in pre_probe, i.e. later.


Yes, I see that. Also it is inevitable that these properties need to
be read before probe(), since they control whether to probe().




Also this seems to happen in SPL and again pre-reloc and again in
U-Boot post-reloc?


What does, the uclass post_bind ?


I mean that this code will be called in SPL (if the regulators are in
the DT there), U-Boot pre-reloc and post-reloc, each time turning on
the regulators. We need a way to control that, don't we?


I would assume that if those regulators are turned on once in the 
earliest stage , turning them on again in the follow up stage would be a 
noop ? This is the point of regulator-*-on, to keep the regulators on.



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


Let's not do this , the entire point of this series is to get rid of
those functions and do the regulator configuration the same way LED
subsystem does it -- by probing always-on/boot-on regulators and
configuring them correctly on probe.

To me regulators_enable_boot_on() and the like was always an
inconsistently applied workaround for missing DM_FLAG_PROBE_AFTER_BIND ,
which is now implemented, so such workarounds can be removed.


That patch seemed to slip under the radar, sent and applied on the
same day! We really need to add a test for it, BTW.


Which patch ? My recollection of DM_FLAG_PROBE_AFTER_BIND was that it 
took a while to get in.



My concern is that this might cause us ordering problems. We perhaps
want the regulators to be done first. If drivers are probed which use
regulators, then presumably they will enable them. Consider this:

- LED driver auto-probes
- probes I2C bus 2
- probes LDO1 which is autoset so turns on
- LDO1 probes, but is already running
- LDO2 probes, which is autoset so turns on

So long as it is OK to enable the regulators in any order, then this
seems fine. But is it? How do we handle the case where are particular
sequence is needed?


Did we finally arrive at the point where we need -EPROBE_DEFER alike 
mechanism ?

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

[Caleb Connolly]

Hi Simon,



This is reading a lot of DT stuff very early, which may be slow. It is
also reading from the DT in the bind() step which we sometimes have to
do, but try to avoid.


Could we set up the livetree pre-bind? What about MMU? On armv8 at least
this would have a huge impact on performance. I've done some
measurements and there is at least 1 order of magnitude difference
between parsing FDT with no caches vs parsing livetree with, it's huge.


That seems like a great idea to me, in general. The fact that SPL sets
up the MMU on armv8 makes it more practical.


Well, on qcom we don't use SPL (yet?), we did have a cyclical dependency
since we rely on DTB for the memory layout, although I have some patches
to do all the memory parsing in board_fdt_blob_setup() since that's
needed for multi-dtb FIT. So I guess we could enable caches at the same
time.


Yes...it seems that enabling cache in SPL has become common on armv8.

As to the memory layout, I'm not sure what is happening there, but it
seems that the DT does not describe it in general (at least not until
U-Boot adds the nodes).


I suppose this depends on the platform. On Qualcomm we use DT as the 
source of truth as it lets us support many platforms (with totally 
different memory maps) with a single U-Boot binary, at least for 
development this is quite nice.




But for this series I believe we are going to have to define what
happens in what phase. We have power_init_board() which is the old way
of doing this...but perhaps we could use that as a way to start up
regulators which are needed.

As to my question about whether this happens in SPL / pre-reloc /
proper, I forgot that we have the bootph tags for that, so it should
be fine. The main issue is that in U-Boot proper we will re-init the
regulators even though that has already been done. Probably that can
be handled by Kconfig or a flag in SPL handoff.


Ensuring that it isn't done multiple times sounds like the right
approach to me.


OK...I wonder how we can solve this. Needs some though.







Also this seems to happen in SPL and again pre-reloc and again in
U-Boot post-reloc?

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


Regards,
Simon


--
// Caleb (they/them)

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

[Simon Glass]

Hi Marek,

On Thu, 27 Jun 2024 at 17:05, Marek Vasut  wrote:
>
> On 6/27/24 10:37 AM, Simon Glass wrote:
> > Hi Marek,
>
> Hi,
>
> [...]
>
> >> ---
> >>   drivers/power/regulator/regulator-uclass.c | 22 +++---
> >>   1 file changed, 15 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/power/regulator/regulator-uclass.c 
> >> b/drivers/power/regulator/regulator-uclass.c
> >> index 66fd531da04..ccc4ef33d83 100644
> >> --- a/drivers/power/regulator/regulator-uclass.c
> >> +++ b/drivers/power/regulator/regulator-uclass.c
> >> @@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
> >>  const char *property = "regulator-name";
> >>
> >>  uc_pdata = dev_get_uclass_plat(dev);
> >> +   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> >> +   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> >>
> >>  /* Regulator's mandatory constraint */
> >>  uc_pdata->name = dev_read_string(dev, property);
> >> @@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
> >>  return -EINVAL;
> >>  }
> >>
> >> -   if (regulator_name_is_unique(dev, uc_pdata->name))
> >> -   return 0;
> >> +   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
> >> +   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> >> + property, dev->name, uc_pdata->name);
> >> +   return -EINVAL;
> >> +   }
> >>
> >> -   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> >> - property, dev->name, uc_pdata->name);
> >> +   /*
> >> +* In case the regulator has regulator-always-on or
> >> +* regulator-boot-on DT property, trigger probe() to
> >> +* configure its default state during startup.
> >> +*/
> >> +   if (uc_pdata->always_on && uc_pdata->boot_on)
> >> +   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> >>
> >> -   return -EINVAL;
> >> +   return 0;
> >>   }
> >>
> >>   static int regulator_pre_probe(struct udevice *dev)
> >> @@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
> >>  -ENODATA);
> >>  uc_pdata->max_uA = dev_read_u32_default(dev, 
> >> "regulator-max-microamp",
> >>  -ENODATA);
> >> -   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> >> -   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> >>  uc_pdata->ramp_delay = dev_read_u32_default(dev, 
> >> "regulator-ramp-delay",
> >>  0);
> >>  uc_pdata->force_off = dev_read_bool(dev, 
> >> "regulator-force-boot-off");
> >> --
> >> 2.43.0
> >>
> >
> > This is reading a lot of DT stuff very early, which may be slow. It is
> > also reading from the DT in the bind() step which we sometimes have to
> > do, but try to avoid.
>
> Actually, it is reading only the bare minimum very early in bind, the
> always-on and boot-on, the rest is in pre_probe, i.e. later.

Yes, I see that. Also it is inevitable that these properties need to
be read before probe(), since they control whether to probe().

>
> > Also this seems to happen in SPL and again pre-reloc and again in
> > U-Boot post-reloc?
>
> What does, the uclass post_bind ?

I mean that this code will be called in SPL (if the regulators are in
the DT there), U-Boot pre-reloc and post-reloc, each time turning on
the regulators. We need a way to control that, don't we?

>
> > Should we have a step in the init sequence where we set up the
> > regulators, by calling regulators_enable_boot_on() ?
>
> Let's not do this , the entire point of this series is to get rid of
> those functions and do the regulator configuration the same way LED
> subsystem does it -- by probing always-on/boot-on regulators and
> configuring them correctly on probe.
>
> To me regulators_enable_boot_on() and the like was always an
> inconsistently applied workaround for missing DM_FLAG_PROBE_AFTER_BIND ,
> which is now implemented, so such workarounds can be removed.

That patch seemed to slip under the radar, sent and applied on the
same day! We really need to add a test for it, BTW.

My concern is that this might cause us ordering problems. We perhaps
want the regulators to be done first. If drivers are probed which use
regulators, then presumably they will enable them. Consider this:

- LED driver auto-probes
   - probes I2C bus 2
   - probes LDO1 which is autoset so turns on
- LDO1 probes, but is already running
- LDO2 probes, which is autoset so turns on

So long as it is OK to enable the regulators in any order, then this
seems fine. But is it? How do we handle the case where are particular
sequence is needed?

Regards,
Simon

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

[Simon Glass]

Hi Caleb,

On Fri, 28 Jun 2024 at 01:09, Caleb Connolly  wrote:
>
>
>
> On 27/06/2024 11:26, Simon Glass wrote:
> > Hi Caleb,
> >
> > On Thu, 27 Jun 2024 at 09:48, Caleb Connolly  
> > wrote:
> >>
> >>
> >>
> >> On 27/06/2024 10:37, Simon Glass wrote:
> >>> Hi Marek,
> >>>
> >>> On Thu, 27 Jun 2024 at 00:57, Marek Vasut  wrote:
> 
>  In case a regulator DT node contains regulator-always-on or 
>  regulator-boot-on
>  property, make sure the regulator gets correctly configured by U-Boot on 
>  start
>  up. Unconditionally probe such regulator drivers. This is a preparatory 
>  patch
>  for introduction of .regulator_post_probe() which would trigger the 
>  regulator
>  configuration.
> 
>  Parsing of regulator-always-on and regulator-boot-on DT property has been
>  moved to regulator_post_bind() as the information is required early, the
>  rest of the DT parsing has been kept in regulator_pre_probe() to avoid
>  slowing down the boot process.
> 
>  Signed-off-by: Marek Vasut 
>  ---
>  Cc: Ben Wolsieffer 
>  Cc: Caleb Connolly 
>  Cc: Chris Morgan 
>  Cc: Dragan Simic 
>  Cc: Eugen Hristev 
>  Cc: Francesco Dolcini 
>  Cc: Heinrich Schuchardt 
>  Cc: Jaehoon Chung 
>  Cc: Jagan Teki 
>  Cc: Jonas Karlman 
>  Cc: Kever Yang 
>  Cc: Kostya Porotchkin 
>  Cc: Matteo Lisi 
>  Cc: Mattijs Korpershoek 
>  Cc: Max Krummenacher 
>  Cc: Neil Armstrong 
>  Cc: Patrice Chotard 
>  Cc: Patrick Delaunay 
>  Cc: Philipp Tomsich 
>  Cc: Quentin Schulz 
>  Cc: Sam Day 
>  Cc: Simon Glass 
>  Cc: Sumit Garg 
>  Cc: Svyatoslav Ryhel 
>  Cc: Thierry Reding 
>  Cc: Tom Rini 
>  Cc: Volodymyr Babchuk 
>  Cc: u-boot-amlo...@groups.io
>  Cc: u-boot-q...@groups.io
>  Cc: u-b...@dh-electronics.com
>  Cc: u-boot@lists.denx.de
>  Cc: uboot-st...@st-md-mailman.stormreply.com
>  ---
> drivers/power/regulator/regulator-uclass.c | 22 +++---
> 1 file changed, 15 insertions(+), 7 deletions(-)
> 
>  diff --git a/drivers/power/regulator/regulator-uclass.c 
>  b/drivers/power/regulator/regulator-uclass.c
>  index 66fd531da04..ccc4ef33d83 100644
>  --- a/drivers/power/regulator/regulator-uclass.c
>  +++ b/drivers/power/regulator/regulator-uclass.c
>  @@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
>    const char *property = "regulator-name";
> 
>    uc_pdata = dev_get_uclass_plat(dev);
>  +   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
>  +   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> 
>    /* Regulator's mandatory constraint */
>    uc_pdata->name = dev_read_string(dev, property);
>  @@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
>    return -EINVAL;
>    }
> 
>  -   if (regulator_name_is_unique(dev, uc_pdata->name))
>  -   return 0;
>  +   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
>  +   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
>  + property, dev->name, uc_pdata->name);
>  +   return -EINVAL;
>  +   }
> 
>  -   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
>  - property, dev->name, uc_pdata->name);
>  +   /*
>  +* In case the regulator has regulator-always-on or
>  +* regulator-boot-on DT property, trigger probe() to
>  +* configure its default state during startup.
>  +*/
>  +   if (uc_pdata->always_on && uc_pdata->boot_on)
>  +   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> 
>  -   return -EINVAL;
>  +   return 0;
> }
> 
> static int regulator_pre_probe(struct udevice *dev)
>  @@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
>    -ENODATA);
>    uc_pdata->max_uA = dev_read_u32_default(dev, 
>  "regulator-max-microamp",
>    -ENODATA);
>  -   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
>  -   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
>    uc_pdata->ramp_delay = dev_read_u32_default(dev, 
>  "regulator-ramp-delay",
>    0);
>    uc_pdata->force_off = dev_read_bool(dev, 
>  "regulator-force-boot-off");
>  --
>  2.43.0
> 
> >>>
> >>> This is reading a lot of DT stuff very early, which may be slow. It is
> >>> also reading from the DT in the bind() step which we sometimes have to
> >>> do, but try to avo

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

[Caleb Connolly]

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

Hi Caleb,

On Thu, 27 Jun 2024 at 09:48, Caleb Connolly  wrote:




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

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut  wrote:


In case a regulator DT node contains regulator-always-on or regulator-boot-on
property, make sure the regulator gets correctly configured by U-Boot on start
up. Unconditionally probe such regulator drivers. This is a preparatory patch
for introduction of .regulator_post_probe() which would trigger the regulator
configuration.

Parsing of regulator-always-on and regulator-boot-on DT property has been
moved to regulator_post_bind() as the information is required early, the
rest of the DT parsing has been kept in regulator_pre_probe() to avoid
slowing down the boot process.

Signed-off-by: Marek Vasut 
---
Cc: Ben Wolsieffer 
Cc: Caleb Connolly 
Cc: Chris Morgan 
Cc: Dragan Simic 
Cc: Eugen Hristev 
Cc: Francesco Dolcini 
Cc: Heinrich Schuchardt 
Cc: Jaehoon Chung 
Cc: Jagan Teki 
Cc: Jonas Karlman 
Cc: Kever Yang 
Cc: Kostya Porotchkin 
Cc: Matteo Lisi 
Cc: Mattijs Korpershoek 
Cc: Max Krummenacher 
Cc: Neil Armstrong 
Cc: Patrice Chotard 
Cc: Patrick Delaunay 
Cc: Philipp Tomsich 
Cc: Quentin Schulz 
Cc: Sam Day 
Cc: Simon Glass 
Cc: Sumit Garg 
Cc: Svyatoslav Ryhel 
Cc: Thierry Reding 
Cc: Tom Rini 
Cc: Volodymyr Babchuk 
Cc: u-boot-amlo...@groups.io
Cc: u-boot-q...@groups.io
Cc: u-b...@dh-electronics.com
Cc: u-boot@lists.denx.de
Cc: uboot-st...@st-md-mailman.stormreply.com
---
   drivers/power/regulator/regulator-uclass.c | 22 +++---
   1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/power/regulator/regulator-uclass.c 
b/drivers/power/regulator/regulator-uclass.c
index 66fd531da04..ccc4ef33d83 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
  const char *property = "regulator-name";

  uc_pdata = dev_get_uclass_plat(dev);
+   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
+   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");

  /* Regulator's mandatory constraint */
  uc_pdata->name = dev_read_string(dev, property);
@@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
  return -EINVAL;
  }

-   if (regulator_name_is_unique(dev, uc_pdata->name))
-   return 0;
+   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
+   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
+ property, dev->name, uc_pdata->name);
+   return -EINVAL;
+   }

-   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
- property, dev->name, uc_pdata->name);
+   /*
+* In case the regulator has regulator-always-on or
+* regulator-boot-on DT property, trigger probe() to
+* configure its default state during startup.
+*/
+   if (uc_pdata->always_on && uc_pdata->boot_on)
+   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);

-   return -EINVAL;
+   return 0;
   }

   static int regulator_pre_probe(struct udevice *dev)
@@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
  -ENODATA);
  uc_pdata->max_uA = dev_read_u32_default(dev, "regulator-max-microamp",
  -ENODATA);
-   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
-   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
  uc_pdata->ramp_delay = dev_read_u32_default(dev, 
"regulator-ramp-delay",
  0);
  uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
--
2.43.0



This is reading a lot of DT stuff very early, which may be slow. It is
also reading from the DT in the bind() step which we sometimes have to
do, but try to avoid.


Could we set up the livetree pre-bind? What about MMU? On armv8 at least
this would have a huge impact on performance. I've done some
measurements and there is at least 1 order of magnitude difference
between parsing FDT with no caches vs parsing livetree with, it's huge.


That seems like a great idea to me, in general. The fact that SPL sets
up the MMU on armv8 makes it more practical.


Well, on qcom we don't use SPL (yet?), we did have a cyclical dependency 
since we rely on DTB for the memory layout, although I have some patches 
to do all the memory parsing in board_fdt_blob_setup() since that's 
needed for multi-dtb FIT. So I guess we could enable caches at the same 
time.


But for this series I believe we are going to have to define what
happens in what phase. We have power_init_board() which is the old way
of doing this...but perhaps we could use that as a way to start up

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

[Marek Vasut]

On 6/27/24 10:37 AM, Simon Glass wrote:

Hi Marek,


Hi,

[...]


---
  drivers/power/regulator/regulator-uclass.c | 22 +++---
  1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/power/regulator/regulator-uclass.c 
b/drivers/power/regulator/regulator-uclass.c
index 66fd531da04..ccc4ef33d83 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
 const char *property = "regulator-name";

 uc_pdata = dev_get_uclass_plat(dev);
+   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
+   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");

 /* Regulator's mandatory constraint */
 uc_pdata->name = dev_read_string(dev, property);
@@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
 return -EINVAL;
 }

-   if (regulator_name_is_unique(dev, uc_pdata->name))
-   return 0;
+   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
+   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
+ property, dev->name, uc_pdata->name);
+   return -EINVAL;
+   }

-   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
- property, dev->name, uc_pdata->name);
+   /*
+* In case the regulator has regulator-always-on or
+* regulator-boot-on DT property, trigger probe() to
+* configure its default state during startup.
+*/
+   if (uc_pdata->always_on && uc_pdata->boot_on)
+   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);

-   return -EINVAL;
+   return 0;
  }

  static int regulator_pre_probe(struct udevice *dev)
@@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
 -ENODATA);
 uc_pdata->max_uA = dev_read_u32_default(dev, "regulator-max-microamp",
 -ENODATA);
-   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
-   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
 uc_pdata->ramp_delay = dev_read_u32_default(dev, 
"regulator-ramp-delay",
 0);
 uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
--
2.43.0



This is reading a lot of DT stuff very early, which may be slow. It is
also reading from the DT in the bind() step which we sometimes have to
do, but try to avoid.


Actually, it is reading only the bare minimum very early in bind, the 
always-on and boot-on, the rest is in pre_probe, i.e. later.



Also this seems to happen in SPL and again pre-reloc and again in
U-Boot post-reloc?


What does, the uclass post_bind ?


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


Let's not do this , the entire point of this series is to get rid of 
those functions and do the regulator configuration the same way LED 
subsystem does it -- by probing always-on/boot-on regulators and 
configuring them correctly on probe.


To me regulators_enable_boot_on() and the like was always an 
inconsistently applied workaround for missing DM_FLAG_PROBE_AFTER_BIND , 
which is now implemented, so such workarounds can be removed.

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

[Simon Glass]

Hi Svyatoslav,

On Thu, 27 Jun 2024 at 11:34, Svyatoslav Ryhel  wrote:
>
> чт, 27 черв. 2024 р. о 12:26 Simon Glass  пише:
> >
> > Hi Svyatoslav,
> >
> > On Thu, 27 Jun 2024 at 10:09, Svyatoslav  wrote:
> > >
> > >
> > >
> > > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly 
> > >  написав(-ла):
> > > >
> > > >
> > > >On 27/06/2024 10:37, Simon Glass wrote:
> > > >> Hi Marek,
> > > >>
> > > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut  wrote:
> > > >>>
> > > >>> In case a regulator DT node contains regulator-always-on or 
> > > >>> regulator-boot-on
> > > >>> property, make sure the regulator gets correctly configured by U-Boot 
> > > >>> on start
> > > >>> up. Unconditionally probe such regulator drivers. This is a 
> > > >>> preparatory patch
> > > >>> for introduction of .regulator_post_probe() which would trigger the 
> > > >>> regulator
> > > >>> configuration.
> > > >>>
> > > >>> Parsing of regulator-always-on and regulator-boot-on DT property has 
> > > >>> been
> > > >>> moved to regulator_post_bind() as the information is required early, 
> > > >>> the
> > > >>> rest of the DT parsing has been kept in regulator_pre_probe() to avoid
> > > >>> slowing down the boot process.
> > > >>>
> > > >>> Signed-off-by: Marek Vasut 
> > > >>> ---
> > > >>> Cc: Ben Wolsieffer 
> > > >>> Cc: Caleb Connolly 
> > > >>> Cc: Chris Morgan 
> > > >>> Cc: Dragan Simic 
> > > >>> Cc: Eugen Hristev 
> > > >>> Cc: Francesco Dolcini 
> > > >>> Cc: Heinrich Schuchardt 
> > > >>> Cc: Jaehoon Chung 
> > > >>> Cc: Jagan Teki 
> > > >>> Cc: Jonas Karlman 
> > > >>> Cc: Kever Yang 
> > > >>> Cc: Kostya Porotchkin 
> > > >>> Cc: Matteo Lisi 
> > > >>> Cc: Mattijs Korpershoek 
> > > >>> Cc: Max Krummenacher 
> > > >>> Cc: Neil Armstrong 
> > > >>> Cc: Patrice Chotard 
> > > >>> Cc: Patrick Delaunay 
> > > >>> Cc: Philipp Tomsich 
> > > >>> Cc: Quentin Schulz 
> > > >>> Cc: Sam Day 
> > > >>> Cc: Simon Glass 
> > > >>> Cc: Sumit Garg 
> > > >>> Cc: Svyatoslav Ryhel 
> > > >>> Cc: Thierry Reding 
> > > >>> Cc: Tom Rini 
> > > >>> Cc: Volodymyr Babchuk 
> > > >>> Cc: u-boot-amlo...@groups.io
> > > >>> Cc: u-boot-q...@groups.io
> > > >>> Cc: u-b...@dh-electronics.com
> > > >>> Cc: u-boot@lists.denx.de
> > > >>> Cc: uboot-st...@st-md-mailman.stormreply.com
> > > >>> ---
> > > >>>   drivers/power/regulator/regulator-uclass.c | 22 
> > > >>> +++---
> > > >>>   1 file changed, 15 insertions(+), 7 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/power/regulator/regulator-uclass.c 
> > > >>> b/drivers/power/regulator/regulator-uclass.c
> > > >>> index 66fd531da04..ccc4ef33d83 100644
> > > >>> --- a/drivers/power/regulator/regulator-uclass.c
> > > >>> +++ b/drivers/power/regulator/regulator-uclass.c
> > > >>> @@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice 
> > > >>> *dev)
> > > >>>  const char *property = "regulator-name";
> > > >>>
> > > >>>  uc_pdata = dev_get_uclass_plat(dev);
> > > >>> +   uc_pdata->always_on = dev_read_bool(dev, 
> > > >>> "regulator-always-on");
> > > >>> +   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> > > >>>
> > > >>>  /* Regulator's mandatory constraint */
> > > >>>  uc_pdata->name = dev_read_string(dev, property);
> > > >>> @@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice 
> > > >>> *dev)
> > > >>>  return -EINVAL;
> > > >>>  }
> > > >>>
> > > >>> -   if (regulator_name_is_unique(dev, uc_pdata->name))
> > > >>> -   return 0;
> > > >>> +   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
> > > >>> +   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> > > >>> + property, dev->name, uc_pdata->name);
> > > >>> +   return -EINVAL;
> > > >>> +   }
> > > >>>
> > > >>> -   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> > > >>> - property, dev->name, uc_pdata->name);
> > > >>> +   /*
> > > >>> +* In case the regulator has regulator-always-on or
> > > >>> +* regulator-boot-on DT property, trigger probe() to
> > > >>> +* configure its default state during startup.
> > > >>> +*/
> > > >>> +   if (uc_pdata->always_on && uc_pdata->boot_on)
> > > >>> +   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> > > >>>
> > > >>> -   return -EINVAL;
> > > >>> +   return 0;
> > > >>>   }
> > > >>>
> > > >>>   static int regulator_pre_probe(struct udevice *dev)
> > > >>> @@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice 
> > > >>> *dev)
> > > >>>  -ENODATA);
> > > >>>  uc_pdata->max_uA = dev_read_u32_default(dev, 
> > > >>> "regulator-max-microamp",
> > > >>>  -ENODATA);
> > > >>> -   uc_pdata->always_on = dev_read_bool(dev, 
> > > >>> "regulator-always-on");
> > > >>> -   uc_pdata->boot_on = dev_read_bo

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

[Svyatoslav Ryhel]

чт, 27 черв. 2024 р. о 12:26 Simon Glass  пише:
>
> Hi Svyatoslav,
>
> On Thu, 27 Jun 2024 at 10:09, Svyatoslav  wrote:
> >
> >
> >
> > 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly 
> >  написав(-ла):
> > >
> > >
> > >On 27/06/2024 10:37, Simon Glass wrote:
> > >> Hi Marek,
> > >>
> > >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut  wrote:
> > >>>
> > >>> In case a regulator DT node contains regulator-always-on or 
> > >>> regulator-boot-on
> > >>> property, make sure the regulator gets correctly configured by U-Boot 
> > >>> on start
> > >>> up. Unconditionally probe such regulator drivers. This is a preparatory 
> > >>> patch
> > >>> for introduction of .regulator_post_probe() which would trigger the 
> > >>> regulator
> > >>> configuration.
> > >>>
> > >>> Parsing of regulator-always-on and regulator-boot-on DT property has 
> > >>> been
> > >>> moved to regulator_post_bind() as the information is required early, the
> > >>> rest of the DT parsing has been kept in regulator_pre_probe() to avoid
> > >>> slowing down the boot process.
> > >>>
> > >>> Signed-off-by: Marek Vasut 
> > >>> ---
> > >>> Cc: Ben Wolsieffer 
> > >>> Cc: Caleb Connolly 
> > >>> Cc: Chris Morgan 
> > >>> Cc: Dragan Simic 
> > >>> Cc: Eugen Hristev 
> > >>> Cc: Francesco Dolcini 
> > >>> Cc: Heinrich Schuchardt 
> > >>> Cc: Jaehoon Chung 
> > >>> Cc: Jagan Teki 
> > >>> Cc: Jonas Karlman 
> > >>> Cc: Kever Yang 
> > >>> Cc: Kostya Porotchkin 
> > >>> Cc: Matteo Lisi 
> > >>> Cc: Mattijs Korpershoek 
> > >>> Cc: Max Krummenacher 
> > >>> Cc: Neil Armstrong 
> > >>> Cc: Patrice Chotard 
> > >>> Cc: Patrick Delaunay 
> > >>> Cc: Philipp Tomsich 
> > >>> Cc: Quentin Schulz 
> > >>> Cc: Sam Day 
> > >>> Cc: Simon Glass 
> > >>> Cc: Sumit Garg 
> > >>> Cc: Svyatoslav Ryhel 
> > >>> Cc: Thierry Reding 
> > >>> Cc: Tom Rini 
> > >>> Cc: Volodymyr Babchuk 
> > >>> Cc: u-boot-amlo...@groups.io
> > >>> Cc: u-boot-q...@groups.io
> > >>> Cc: u-b...@dh-electronics.com
> > >>> Cc: u-boot@lists.denx.de
> > >>> Cc: uboot-st...@st-md-mailman.stormreply.com
> > >>> ---
> > >>>   drivers/power/regulator/regulator-uclass.c | 22 +++---
> > >>>   1 file changed, 15 insertions(+), 7 deletions(-)
> > >>>
> > >>> diff --git a/drivers/power/regulator/regulator-uclass.c 
> > >>> b/drivers/power/regulator/regulator-uclass.c
> > >>> index 66fd531da04..ccc4ef33d83 100644
> > >>> --- a/drivers/power/regulator/regulator-uclass.c
> > >>> +++ b/drivers/power/regulator/regulator-uclass.c
> > >>> @@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
> > >>>  const char *property = "regulator-name";
> > >>>
> > >>>  uc_pdata = dev_get_uclass_plat(dev);
> > >>> +   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> > >>> +   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> > >>>
> > >>>  /* Regulator's mandatory constraint */
> > >>>  uc_pdata->name = dev_read_string(dev, property);
> > >>> @@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice 
> > >>> *dev)
> > >>>  return -EINVAL;
> > >>>  }
> > >>>
> > >>> -   if (regulator_name_is_unique(dev, uc_pdata->name))
> > >>> -   return 0;
> > >>> +   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
> > >>> +   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> > >>> + property, dev->name, uc_pdata->name);
> > >>> +   return -EINVAL;
> > >>> +   }
> > >>>
> > >>> -   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> > >>> - property, dev->name, uc_pdata->name);
> > >>> +   /*
> > >>> +* In case the regulator has regulator-always-on or
> > >>> +* regulator-boot-on DT property, trigger probe() to
> > >>> +* configure its default state during startup.
> > >>> +*/
> > >>> +   if (uc_pdata->always_on && uc_pdata->boot_on)
> > >>> +   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> > >>>
> > >>> -   return -EINVAL;
> > >>> +   return 0;
> > >>>   }
> > >>>
> > >>>   static int regulator_pre_probe(struct udevice *dev)
> > >>> @@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
> > >>>  -ENODATA);
> > >>>  uc_pdata->max_uA = dev_read_u32_default(dev, 
> > >>> "regulator-max-microamp",
> > >>>  -ENODATA);
> > >>> -   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> > >>> -   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> > >>>  uc_pdata->ramp_delay = dev_read_u32_default(dev, 
> > >>> "regulator-ramp-delay",
> > >>>  0);
> > >>>  uc_pdata->force_off = dev_read_bool(dev, 
> > >>> "regulator-force-boot-off");
> > >>> --
> > >>> 2.43.0
> > >>>
> > >>
> > >> This is reading a lot of DT stuff very ea

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

[Simon Glass]

Hi Caleb,

On Thu, 27 Jun 2024 at 09:48, Caleb Connolly  wrote:
>
>
>
> On 27/06/2024 10:37, Simon Glass wrote:
> > Hi Marek,
> >
> > On Thu, 27 Jun 2024 at 00:57, Marek Vasut  wrote:
> >>
> >> In case a regulator DT node contains regulator-always-on or 
> >> regulator-boot-on
> >> property, make sure the regulator gets correctly configured by U-Boot on 
> >> start
> >> up. Unconditionally probe such regulator drivers. This is a preparatory 
> >> patch
> >> for introduction of .regulator_post_probe() which would trigger the 
> >> regulator
> >> configuration.
> >>
> >> Parsing of regulator-always-on and regulator-boot-on DT property has been
> >> moved to regulator_post_bind() as the information is required early, the
> >> rest of the DT parsing has been kept in regulator_pre_probe() to avoid
> >> slowing down the boot process.
> >>
> >> Signed-off-by: Marek Vasut 
> >> ---
> >> Cc: Ben Wolsieffer 
> >> Cc: Caleb Connolly 
> >> Cc: Chris Morgan 
> >> Cc: Dragan Simic 
> >> Cc: Eugen Hristev 
> >> Cc: Francesco Dolcini 
> >> Cc: Heinrich Schuchardt 
> >> Cc: Jaehoon Chung 
> >> Cc: Jagan Teki 
> >> Cc: Jonas Karlman 
> >> Cc: Kever Yang 
> >> Cc: Kostya Porotchkin 
> >> Cc: Matteo Lisi 
> >> Cc: Mattijs Korpershoek 
> >> Cc: Max Krummenacher 
> >> Cc: Neil Armstrong 
> >> Cc: Patrice Chotard 
> >> Cc: Patrick Delaunay 
> >> Cc: Philipp Tomsich 
> >> Cc: Quentin Schulz 
> >> Cc: Sam Day 
> >> Cc: Simon Glass 
> >> Cc: Sumit Garg 
> >> Cc: Svyatoslav Ryhel 
> >> Cc: Thierry Reding 
> >> Cc: Tom Rini 
> >> Cc: Volodymyr Babchuk 
> >> Cc: u-boot-amlo...@groups.io
> >> Cc: u-boot-q...@groups.io
> >> Cc: u-b...@dh-electronics.com
> >> Cc: u-boot@lists.denx.de
> >> Cc: uboot-st...@st-md-mailman.stormreply.com
> >> ---
> >>   drivers/power/regulator/regulator-uclass.c | 22 +++---
> >>   1 file changed, 15 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/power/regulator/regulator-uclass.c 
> >> b/drivers/power/regulator/regulator-uclass.c
> >> index 66fd531da04..ccc4ef33d83 100644
> >> --- a/drivers/power/regulator/regulator-uclass.c
> >> +++ b/drivers/power/regulator/regulator-uclass.c
> >> @@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
> >>  const char *property = "regulator-name";
> >>
> >>  uc_pdata = dev_get_uclass_plat(dev);
> >> +   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> >> +   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> >>
> >>  /* Regulator's mandatory constraint */
> >>  uc_pdata->name = dev_read_string(dev, property);
> >> @@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
> >>  return -EINVAL;
> >>  }
> >>
> >> -   if (regulator_name_is_unique(dev, uc_pdata->name))
> >> -   return 0;
> >> +   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
> >> +   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> >> + property, dev->name, uc_pdata->name);
> >> +   return -EINVAL;
> >> +   }
> >>
> >> -   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> >> - property, dev->name, uc_pdata->name);
> >> +   /*
> >> +* In case the regulator has regulator-always-on or
> >> +* regulator-boot-on DT property, trigger probe() to
> >> +* configure its default state during startup.
> >> +*/
> >> +   if (uc_pdata->always_on && uc_pdata->boot_on)
> >> +   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> >>
> >> -   return -EINVAL;
> >> +   return 0;
> >>   }
> >>
> >>   static int regulator_pre_probe(struct udevice *dev)
> >> @@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
> >>  -ENODATA);
> >>  uc_pdata->max_uA = dev_read_u32_default(dev, 
> >> "regulator-max-microamp",
> >>  -ENODATA);
> >> -   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> >> -   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> >>  uc_pdata->ramp_delay = dev_read_u32_default(dev, 
> >> "regulator-ramp-delay",
> >>  0);
> >>  uc_pdata->force_off = dev_read_bool(dev, 
> >> "regulator-force-boot-off");
> >> --
> >> 2.43.0
> >>
> >
> > This is reading a lot of DT stuff very early, which may be slow. It is
> > also reading from the DT in the bind() step which we sometimes have to
> > do, but try to avoid.
>
> Could we set up the livetree pre-bind? What about MMU? On armv8 at least
> this would have a huge impact on performance. I've done some
> measurements and there is at least 1 order of magnitude difference
> between parsing FDT with no caches vs parsing livetree with, it's huge.

That seems like a great idea to me, in general. The fact that SPL sets
up the MMU on armv8 makes it mo

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

[Simon Glass]

Hi Svyatoslav,

On Thu, 27 Jun 2024 at 10:09, Svyatoslav  wrote:
>
>
>
> 27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly 
>  написав(-ла):
> >
> >
> >On 27/06/2024 10:37, Simon Glass wrote:
> >> Hi Marek,
> >>
> >> On Thu, 27 Jun 2024 at 00:57, Marek Vasut  wrote:
> >>>
> >>> In case a regulator DT node contains regulator-always-on or 
> >>> regulator-boot-on
> >>> property, make sure the regulator gets correctly configured by U-Boot on 
> >>> start
> >>> up. Unconditionally probe such regulator drivers. This is a preparatory 
> >>> patch
> >>> for introduction of .regulator_post_probe() which would trigger the 
> >>> regulator
> >>> configuration.
> >>>
> >>> Parsing of regulator-always-on and regulator-boot-on DT property has been
> >>> moved to regulator_post_bind() as the information is required early, the
> >>> rest of the DT parsing has been kept in regulator_pre_probe() to avoid
> >>> slowing down the boot process.
> >>>
> >>> Signed-off-by: Marek Vasut 
> >>> ---
> >>> Cc: Ben Wolsieffer 
> >>> Cc: Caleb Connolly 
> >>> Cc: Chris Morgan 
> >>> Cc: Dragan Simic 
> >>> Cc: Eugen Hristev 
> >>> Cc: Francesco Dolcini 
> >>> Cc: Heinrich Schuchardt 
> >>> Cc: Jaehoon Chung 
> >>> Cc: Jagan Teki 
> >>> Cc: Jonas Karlman 
> >>> Cc: Kever Yang 
> >>> Cc: Kostya Porotchkin 
> >>> Cc: Matteo Lisi 
> >>> Cc: Mattijs Korpershoek 
> >>> Cc: Max Krummenacher 
> >>> Cc: Neil Armstrong 
> >>> Cc: Patrice Chotard 
> >>> Cc: Patrick Delaunay 
> >>> Cc: Philipp Tomsich 
> >>> Cc: Quentin Schulz 
> >>> Cc: Sam Day 
> >>> Cc: Simon Glass 
> >>> Cc: Sumit Garg 
> >>> Cc: Svyatoslav Ryhel 
> >>> Cc: Thierry Reding 
> >>> Cc: Tom Rini 
> >>> Cc: Volodymyr Babchuk 
> >>> Cc: u-boot-amlo...@groups.io
> >>> Cc: u-boot-q...@groups.io
> >>> Cc: u-b...@dh-electronics.com
> >>> Cc: u-boot@lists.denx.de
> >>> Cc: uboot-st...@st-md-mailman.stormreply.com
> >>> ---
> >>>   drivers/power/regulator/regulator-uclass.c | 22 +++---
> >>>   1 file changed, 15 insertions(+), 7 deletions(-)
> >>>
> >>> diff --git a/drivers/power/regulator/regulator-uclass.c 
> >>> b/drivers/power/regulator/regulator-uclass.c
> >>> index 66fd531da04..ccc4ef33d83 100644
> >>> --- a/drivers/power/regulator/regulator-uclass.c
> >>> +++ b/drivers/power/regulator/regulator-uclass.c
> >>> @@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
> >>>  const char *property = "regulator-name";
> >>>
> >>>  uc_pdata = dev_get_uclass_plat(dev);
> >>> +   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> >>> +   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> >>>
> >>>  /* Regulator's mandatory constraint */
> >>>  uc_pdata->name = dev_read_string(dev, property);
> >>> @@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
> >>>  return -EINVAL;
> >>>  }
> >>>
> >>> -   if (regulator_name_is_unique(dev, uc_pdata->name))
> >>> -   return 0;
> >>> +   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
> >>> +   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> >>> + property, dev->name, uc_pdata->name);
> >>> +   return -EINVAL;
> >>> +   }
> >>>
> >>> -   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> >>> - property, dev->name, uc_pdata->name);
> >>> +   /*
> >>> +* In case the regulator has regulator-always-on or
> >>> +* regulator-boot-on DT property, trigger probe() to
> >>> +* configure its default state during startup.
> >>> +*/
> >>> +   if (uc_pdata->always_on && uc_pdata->boot_on)
> >>> +   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
> >>>
> >>> -   return -EINVAL;
> >>> +   return 0;
> >>>   }
> >>>
> >>>   static int regulator_pre_probe(struct udevice *dev)
> >>> @@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
> >>>  -ENODATA);
> >>>  uc_pdata->max_uA = dev_read_u32_default(dev, 
> >>> "regulator-max-microamp",
> >>>  -ENODATA);
> >>> -   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> >>> -   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> >>>  uc_pdata->ramp_delay = dev_read_u32_default(dev, 
> >>> "regulator-ramp-delay",
> >>>  0);
> >>>  uc_pdata->force_off = dev_read_bool(dev, 
> >>> "regulator-force-boot-off");
> >>> --
> >>> 2.43.0
> >>>
> >>
> >> This is reading a lot of DT stuff very early, which may be slow. It is
> >> also reading from the DT in the bind() step which we sometimes have to
> >> do, but try to avoid.
> >
> >Could we set up the livetree pre-bind? What about MMU? On armv8 at least 
> >this would have a huge impact on performance. I've done some measurements 
> >and there is at least 1 

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

[Svyatoslav]

27 червня 2024 р. 11:48:46 GMT+03:00, Caleb Connolly 
 написав(-ла):
>
>
>On 27/06/2024 10:37, Simon Glass wrote:
>> Hi Marek,
>> 
>> On Thu, 27 Jun 2024 at 00:57, Marek Vasut  wrote:
>>> 
>>> In case a regulator DT node contains regulator-always-on or 
>>> regulator-boot-on
>>> property, make sure the regulator gets correctly configured by U-Boot on 
>>> start
>>> up. Unconditionally probe such regulator drivers. This is a preparatory 
>>> patch
>>> for introduction of .regulator_post_probe() which would trigger the 
>>> regulator
>>> configuration.
>>> 
>>> Parsing of regulator-always-on and regulator-boot-on DT property has been
>>> moved to regulator_post_bind() as the information is required early, the
>>> rest of the DT parsing has been kept in regulator_pre_probe() to avoid
>>> slowing down the boot process.
>>> 
>>> Signed-off-by: Marek Vasut 
>>> ---
>>> Cc: Ben Wolsieffer 
>>> Cc: Caleb Connolly 
>>> Cc: Chris Morgan 
>>> Cc: Dragan Simic 
>>> Cc: Eugen Hristev 
>>> Cc: Francesco Dolcini 
>>> Cc: Heinrich Schuchardt 
>>> Cc: Jaehoon Chung 
>>> Cc: Jagan Teki 
>>> Cc: Jonas Karlman 
>>> Cc: Kever Yang 
>>> Cc: Kostya Porotchkin 
>>> Cc: Matteo Lisi 
>>> Cc: Mattijs Korpershoek 
>>> Cc: Max Krummenacher 
>>> Cc: Neil Armstrong 
>>> Cc: Patrice Chotard 
>>> Cc: Patrick Delaunay 
>>> Cc: Philipp Tomsich 
>>> Cc: Quentin Schulz 
>>> Cc: Sam Day 
>>> Cc: Simon Glass 
>>> Cc: Sumit Garg 
>>> Cc: Svyatoslav Ryhel 
>>> Cc: Thierry Reding 
>>> Cc: Tom Rini 
>>> Cc: Volodymyr Babchuk 
>>> Cc: u-boot-amlo...@groups.io
>>> Cc: u-boot-q...@groups.io
>>> Cc: u-b...@dh-electronics.com
>>> Cc: u-boot@lists.denx.de
>>> Cc: uboot-st...@st-md-mailman.stormreply.com
>>> ---
>>>   drivers/power/regulator/regulator-uclass.c | 22 +++---
>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/drivers/power/regulator/regulator-uclass.c 
>>> b/drivers/power/regulator/regulator-uclass.c
>>> index 66fd531da04..ccc4ef33d83 100644
>>> --- a/drivers/power/regulator/regulator-uclass.c
>>> +++ b/drivers/power/regulator/regulator-uclass.c
>>> @@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
>>>  const char *property = "regulator-name";
>>> 
>>>  uc_pdata = dev_get_uclass_plat(dev);
>>> +   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
>>> +   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
>>> 
>>>  /* Regulator's mandatory constraint */
>>>  uc_pdata->name = dev_read_string(dev, property);
>>> @@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
>>>  return -EINVAL;
>>>  }
>>> 
>>> -   if (regulator_name_is_unique(dev, uc_pdata->name))
>>> -   return 0;
>>> +   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
>>> +   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
>>> + property, dev->name, uc_pdata->name);
>>> +   return -EINVAL;
>>> +   }
>>> 
>>> -   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
>>> - property, dev->name, uc_pdata->name);
>>> +   /*
>>> +* In case the regulator has regulator-always-on or
>>> +* regulator-boot-on DT property, trigger probe() to
>>> +* configure its default state during startup.
>>> +*/
>>> +   if (uc_pdata->always_on && uc_pdata->boot_on)
>>> +   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
>>> 
>>> -   return -EINVAL;
>>> +   return 0;
>>>   }
>>> 
>>>   static int regulator_pre_probe(struct udevice *dev)
>>> @@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
>>>  -ENODATA);
>>>  uc_pdata->max_uA = dev_read_u32_default(dev, 
>>> "regulator-max-microamp",
>>>  -ENODATA);
>>> -   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
>>> -   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
>>>  uc_pdata->ramp_delay = dev_read_u32_default(dev, 
>>> "regulator-ramp-delay",
>>>  0);
>>>  uc_pdata->force_off = dev_read_bool(dev, 
>>> "regulator-force-boot-off");
>>> --
>>> 2.43.0
>>> 
>> 
>> This is reading a lot of DT stuff very early, which may be slow. It is
>> also reading from the DT in the bind() step which we sometimes have to
>> do, but try to avoid.
>
>Could we set up the livetree pre-bind? What about MMU? On armv8 at least this 
>would have a huge impact on performance. I've done some measurements and there 
>is at least 1 order of magnitude difference between parsing FDT with no caches 
>vs parsing livetree with, it's huge.
>> 
>> Also this seems to happen in SPL and again pre-reloc and again in
>> U-Boot post-reloc?

Not so long ago I proposed a similar patchset with the same goal
and I have discovered massive issues with 

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

[Caleb Connolly]

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

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut  wrote:


In case a regulator DT node contains regulator-always-on or regulator-boot-on
property, make sure the regulator gets correctly configured by U-Boot on start
up. Unconditionally probe such regulator drivers. This is a preparatory patch
for introduction of .regulator_post_probe() which would trigger the regulator
configuration.

Parsing of regulator-always-on and regulator-boot-on DT property has been
moved to regulator_post_bind() as the information is required early, the
rest of the DT parsing has been kept in regulator_pre_probe() to avoid
slowing down the boot process.

Signed-off-by: Marek Vasut 
---
Cc: Ben Wolsieffer 
Cc: Caleb Connolly 
Cc: Chris Morgan 
Cc: Dragan Simic 
Cc: Eugen Hristev 
Cc: Francesco Dolcini 
Cc: Heinrich Schuchardt 
Cc: Jaehoon Chung 
Cc: Jagan Teki 
Cc: Jonas Karlman 
Cc: Kever Yang 
Cc: Kostya Porotchkin 
Cc: Matteo Lisi 
Cc: Mattijs Korpershoek 
Cc: Max Krummenacher 
Cc: Neil Armstrong 
Cc: Patrice Chotard 
Cc: Patrick Delaunay 
Cc: Philipp Tomsich 
Cc: Quentin Schulz 
Cc: Sam Day 
Cc: Simon Glass 
Cc: Sumit Garg 
Cc: Svyatoslav Ryhel 
Cc: Thierry Reding 
Cc: Tom Rini 
Cc: Volodymyr Babchuk 
Cc: u-boot-amlo...@groups.io
Cc: u-boot-q...@groups.io
Cc: u-b...@dh-electronics.com
Cc: u-boot@lists.denx.de
Cc: uboot-st...@st-md-mailman.stormreply.com
---
  drivers/power/regulator/regulator-uclass.c | 22 +++---
  1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/power/regulator/regulator-uclass.c 
b/drivers/power/regulator/regulator-uclass.c
index 66fd531da04..ccc4ef33d83 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
 const char *property = "regulator-name";

 uc_pdata = dev_get_uclass_plat(dev);
+   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
+   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");

 /* Regulator's mandatory constraint */
 uc_pdata->name = dev_read_string(dev, property);
@@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
 return -EINVAL;
 }

-   if (regulator_name_is_unique(dev, uc_pdata->name))
-   return 0;
+   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
+   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
+ property, dev->name, uc_pdata->name);
+   return -EINVAL;
+   }

-   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
- property, dev->name, uc_pdata->name);
+   /*
+* In case the regulator has regulator-always-on or
+* regulator-boot-on DT property, trigger probe() to
+* configure its default state during startup.
+*/
+   if (uc_pdata->always_on && uc_pdata->boot_on)
+   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);

-   return -EINVAL;
+   return 0;
  }

  static int regulator_pre_probe(struct udevice *dev)
@@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
 -ENODATA);
 uc_pdata->max_uA = dev_read_u32_default(dev, "regulator-max-microamp",
 -ENODATA);
-   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
-   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
 uc_pdata->ramp_delay = dev_read_u32_default(dev, 
"regulator-ramp-delay",
 0);
 uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
--
2.43.0



This is reading a lot of DT stuff very early, which may be slow. It is
also reading from the DT in the bind() step which we sometimes have to
do, but try to avoid.


Could we set up the livetree pre-bind? What about MMU? On armv8 at least 
this would have a huge impact on performance. I've done some 
measurements and there is at least 1 order of magnitude difference 
between parsing FDT with no caches vs parsing livetree with, it's huge.


Also this seems to happen in SPL and again pre-reloc and again in
U-Boot post-reloc?

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

Regards,
Simon


--
// Caleb (they/them)

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

[Simon Glass]

Hi Marek,

On Thu, 27 Jun 2024 at 00:57, Marek Vasut  wrote:
>
> In case a regulator DT node contains regulator-always-on or regulator-boot-on
> property, make sure the regulator gets correctly configured by U-Boot on start
> up. Unconditionally probe such regulator drivers. This is a preparatory patch
> for introduction of .regulator_post_probe() which would trigger the regulator
> configuration.
>
> Parsing of regulator-always-on and regulator-boot-on DT property has been
> moved to regulator_post_bind() as the information is required early, the
> rest of the DT parsing has been kept in regulator_pre_probe() to avoid
> slowing down the boot process.
>
> Signed-off-by: Marek Vasut 
> ---
> Cc: Ben Wolsieffer 
> Cc: Caleb Connolly 
> Cc: Chris Morgan 
> Cc: Dragan Simic 
> Cc: Eugen Hristev 
> Cc: Francesco Dolcini 
> Cc: Heinrich Schuchardt 
> Cc: Jaehoon Chung 
> Cc: Jagan Teki 
> Cc: Jonas Karlman 
> Cc: Kever Yang 
> Cc: Kostya Porotchkin 
> Cc: Matteo Lisi 
> Cc: Mattijs Korpershoek 
> Cc: Max Krummenacher 
> Cc: Neil Armstrong 
> Cc: Patrice Chotard 
> Cc: Patrick Delaunay 
> Cc: Philipp Tomsich 
> Cc: Quentin Schulz 
> Cc: Sam Day 
> Cc: Simon Glass 
> Cc: Sumit Garg 
> Cc: Svyatoslav Ryhel 
> Cc: Thierry Reding 
> Cc: Tom Rini 
> Cc: Volodymyr Babchuk 
> Cc: u-boot-amlo...@groups.io
> Cc: u-boot-q...@groups.io
> Cc: u-b...@dh-electronics.com
> Cc: u-boot@lists.denx.de
> Cc: uboot-st...@st-md-mailman.stormreply.com
> ---
>  drivers/power/regulator/regulator-uclass.c | 22 +++---
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/power/regulator/regulator-uclass.c 
> b/drivers/power/regulator/regulator-uclass.c
> index 66fd531da04..ccc4ef33d83 100644
> --- a/drivers/power/regulator/regulator-uclass.c
> +++ b/drivers/power/regulator/regulator-uclass.c
> @@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
> const char *property = "regulator-name";
>
> uc_pdata = dev_get_uclass_plat(dev);
> +   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> +   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
>
> /* Regulator's mandatory constraint */
> uc_pdata->name = dev_read_string(dev, property);
> @@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
> return -EINVAL;
> }
>
> -   if (regulator_name_is_unique(dev, uc_pdata->name))
> -   return 0;
> +   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
> +   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> + property, dev->name, uc_pdata->name);
> +   return -EINVAL;
> +   }
>
> -   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
> - property, dev->name, uc_pdata->name);
> +   /*
> +* In case the regulator has regulator-always-on or
> +* regulator-boot-on DT property, trigger probe() to
> +* configure its default state during startup.
> +*/
> +   if (uc_pdata->always_on && uc_pdata->boot_on)
> +   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
>
> -   return -EINVAL;
> +   return 0;
>  }
>
>  static int regulator_pre_probe(struct udevice *dev)
> @@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
> -ENODATA);
> uc_pdata->max_uA = dev_read_u32_default(dev, "regulator-max-microamp",
> -ENODATA);
> -   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> -   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> uc_pdata->ramp_delay = dev_read_u32_default(dev, 
> "regulator-ramp-delay",
> 0);
> uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
> --
> 2.43.0
>

This is reading a lot of DT stuff very early, which may be slow. It is
also reading from the DT in the bind() step which we sometimes have to
do, but try to avoid.

Also this seems to happen in SPL and again pre-reloc and again in
U-Boot post-reloc?

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

Regards,
Simon

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

[Marek Vasut]

In case a regulator DT node contains regulator-always-on or regulator-boot-on
property, make sure the regulator gets correctly configured by U-Boot on start
up. Unconditionally probe such regulator drivers. This is a preparatory patch
for introduction of .regulator_post_probe() which would trigger the regulator
configuration.

Parsing of regulator-always-on and regulator-boot-on DT property has been
moved to regulator_post_bind() as the information is required early, the
rest of the DT parsing has been kept in regulator_pre_probe() to avoid
slowing down the boot process.

Signed-off-by: Marek Vasut 
---
Cc: Ben Wolsieffer 
Cc: Caleb Connolly 
Cc: Chris Morgan 
Cc: Dragan Simic 
Cc: Eugen Hristev 
Cc: Francesco Dolcini 
Cc: Heinrich Schuchardt 
Cc: Jaehoon Chung 
Cc: Jagan Teki 
Cc: Jonas Karlman 
Cc: Kever Yang 
Cc: Kostya Porotchkin 
Cc: Matteo Lisi 
Cc: Mattijs Korpershoek 
Cc: Max Krummenacher 
Cc: Neil Armstrong 
Cc: Patrice Chotard 
Cc: Patrick Delaunay 
Cc: Philipp Tomsich 
Cc: Quentin Schulz 
Cc: Sam Day 
Cc: Simon Glass 
Cc: Sumit Garg 
Cc: Svyatoslav Ryhel 
Cc: Thierry Reding 
Cc: Tom Rini 
Cc: Volodymyr Babchuk 
Cc: u-boot-amlo...@groups.io
Cc: u-boot-q...@groups.io
Cc: u-b...@dh-electronics.com
Cc: u-boot@lists.denx.de
Cc: uboot-st...@st-md-mailman.stormreply.com
---
 drivers/power/regulator/regulator-uclass.c | 22 +++---
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/power/regulator/regulator-uclass.c 
b/drivers/power/regulator/regulator-uclass.c
index 66fd531da04..ccc4ef33d83 100644
--- a/drivers/power/regulator/regulator-uclass.c
+++ b/drivers/power/regulator/regulator-uclass.c
@@ -433,6 +433,8 @@ static int regulator_post_bind(struct udevice *dev)
const char *property = "regulator-name";
 
uc_pdata = dev_get_uclass_plat(dev);
+   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
+   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
 
/* Regulator's mandatory constraint */
uc_pdata->name = dev_read_string(dev, property);
@@ -444,13 +446,21 @@ static int regulator_post_bind(struct udevice *dev)
return -EINVAL;
}
 
-   if (regulator_name_is_unique(dev, uc_pdata->name))
-   return 0;
+   if (!regulator_name_is_unique(dev, uc_pdata->name)) {
+   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
+ property, dev->name, uc_pdata->name);
+   return -EINVAL;
+   }
 
-   debug("'%s' of dev: '%s', has nonunique value: '%s\n",
- property, dev->name, uc_pdata->name);
+   /*
+* In case the regulator has regulator-always-on or
+* regulator-boot-on DT property, trigger probe() to
+* configure its default state during startup.
+*/
+   if (uc_pdata->always_on && uc_pdata->boot_on)
+   dev_or_flags(dev, DM_FLAG_PROBE_AFTER_BIND);
 
-   return -EINVAL;
+   return 0;
 }
 
 static int regulator_pre_probe(struct udevice *dev)
@@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
-ENODATA);
uc_pdata->max_uA = dev_read_u32_default(dev, "regulator-max-microamp",
-ENODATA);
-   uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
-   uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
uc_pdata->ramp_delay = dev_read_u32_default(dev, "regulator-ramp-delay",
0);
uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
-- 
2.43.0