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

[Svyatoslav Ryhel]

ср, 25 вер. 2024 р. о 02:44 Tom Rini <trini at konsulko.com> пише:
>
> On Fri, Sep 20, 2024 at 10:48:56AM -0600, Tom Rini wrote:
> > On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote:
> > > пн, 16 вер. 2024 р. о 19:28 Tom Rini <trini at konsulko.com> пише:
> > > >
> > > > On Wed, Sep 11, 2024 at 07:00:56PM -0600, Simon Glass wrote:
> > > > > Hi Marek,
> > > > >
> > > > > On Fri, 28 Jun 2024 at 07:26, Marek Vasut <marex at denx.de> wrote:
> > > > > >
> > > > > > On 6/28/24 9:32 AM, Simon Glass wrote:
> > > > > > > Hi Marek,
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > >>>> @@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
> > > > > > >>>>                                                   -ENODATA);
> > > > > > >>>>           uc_pdata->max_uA = dev_read_u32_default(dev, "regulator-max-microamp",
> > > > > > >>>>                                                   -ENODATA);
> > > > > > >>>> -       uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
> > > > > > >>>> -       uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
> > > > > > >>>>           uc_pdata->ramp_delay = dev_read_u32_default(dev, "regulator-ramp-delay",
> > > > > > >>>>                                                       0);
> > > > > > >>>>           uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
> > > > > > >>>> --
> > > > > > >>>> 2.43.0
> > > > > > >>>>
> > > > > > >>>
> > > > > > >>> This is reading a lot of DT stuff very early, which may be slow. It is
> > > > > > >>> also reading from the DT in the bind() step which we sometimes have to
> > > > > > >>> do, but try to avoid.
> > > > > > >>
> > > > > > >> Actually, it is reading only the bare minimum very early in bind, the
> > > > > > >> always-on and boot-on, the rest is in pre_probe, i.e. later.
> > > > > > >
> > > > > > > Yes, I see that. Also it is inevitable that these properties need to
> > > > > > > be read before probe(), since they control whether to probe().
> > > > > > >
> > > > > > >>
> > > > > > >>> Also this seems to happen in SPL and again pre-reloc and again in
> > > > > > >>> U-Boot post-reloc?
> > > > > > >>
> > > > > > >> What does, the uclass post_bind ?
> > > > > > >
> > > > > > > I mean that this code will be called in SPL (if the regulators are in
> > > > > > > the DT there), U-Boot pre-reloc and post-reloc, each time turning on
> > > > > > > the regulators. We need a way to control that, don't we?
> > > > > >
> > > > > > I would assume that if those regulators are turned on once in the
> > > > > > earliest stage , turning them on again in the follow up stage would be a
> > > > > > noop ? This is the point of regulator-*-on, to keep the regulators on.
> > > > >
> > > > > No, there is sometimes a particular sequence needed.
> > > > >
> > > > > >
> > > > > > >>> Should we have a step in the init sequence where we set up the
> > > > > > >>> regulators, by calling regulators_enable_boot_on() ?
> > > > > > >>
> > > > > > >> Let's not do this , the entire point of this series is to get rid of
> > > > > > >> those functions and do the regulator configuration the same way LED
> > > > > > >> subsystem does it -- by probing always-on/boot-on regulators and
> > > > > > >> configuring them correctly on probe.
> > > > > > >>
> > > > > > >> To me regulators_enable_boot_on() and the like was always an
> > > > > > >> inconsistently applied workaround for missing DM_FLAG_PROBE_AFTER_BIND ,
> > > > > > >> which is now implemented, so such workarounds can be removed.
> > > > > > >
> > > > > > > That patch seemed to slip under the radar, sent and applied on the
> > > > > > > same day! We really need to add a test for it, BTW.
> > > > > >
> > > > > > Which patch ? My recollection of DM_FLAG_PROBE_AFTER_BIND was that it
> > > > > > took a while to get in.
> > > > >
> > > > > [1]
> > > > >
> > > > > >
> > > > > > > My concern is that this might cause us ordering problems. We perhaps
> > > > > > > want the regulators to be done first. If drivers are probed which use
> > > > > > > regulators, then presumably they will enable them. Consider this:
> > > > > > >
> > > > > > > - LED driver auto-probes
> > > > > > >     - probes I2C bus 2
> > > > > > >     - probes LDO1 which is autoset so turns on
> > > > > > > - LDO1 probes, but is already running
> > > > > > > - LDO2 probes, which is autoset so turns on
> > > > > > >
> > > > > > > So long as it is OK to enable the regulators in any order, then this
> > > > > > > seems fine. But is it? How do we handle the case where are particular
> > > > > > > sequence is needed?
> > > > > >
> > > > > > Did we finally arrive at the point where we need -EPROBE_DEFER alike
> > > > > > mechanism ?
> > > > >
> > > > > Nope. But I am concerned that this patch is leading us there. bind()
> > > > > really needs to be as clean as possible. It is one of the design
> > > > > elements of driver model which Linux should adopt.
> > > > >
> > > > > There is always a race to be the first to init something, move the
> > > > > init earlier, etc... I don't see any general need to init the
> > > > > regulators right at the start. It should be done in a separate
> > > > > function and be optional. I am happy to send a patch if you like.
> > > >
> > > > Since we're currently stuck on the point where Marek has patches that
> > > > fix a real problem, and Svyatoslav has a problem with them, but isn't
> > > > currently able to debug it, yes, please put forward your patch that
> > > > might address both sets of problems so we can all figure out how to
> > > > resolve the problems, thanks!
> > >
> > > With patches from Marek there is no i2c chip probe of PMIC, while
> > > without i2c chip probe is called (probe_chip function). How this is
> > > even possible?
> >
> > Yes, it's very puzzling. Do you have the ability to get some debug
> > console type information out so you can sprinkle in some prints and
> > figure it out?
>
> So, here's my plan. Marek posted
> https://patchwork.ozlabs.org/project/uboot/patch/20240924220905.514122-1-marex@denx.de/
> which is a work-around, so that v2024.10 can work. I'm going to take
> that for master tomorrow. I'm also going to take _this_ series to -next
> tomorrow, as this is the best approach we currently have to solving the
> overall problem. The Tegra platforms that are now very oddly broken need
> to get debugged to see where their problem is, and if there's an
> entirely alternate approach, Simon can post patches for that instead on
> top of next.
>
> --
> Tom

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

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

Here are logs from my device (Tegra SoC)

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

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

As you can see this part

(bootloader) pkt header 1 sent (0x10010)
(bootloader) pkt header 2 sent (0x0)
(bootloader) pkt header 3 sent (0xb0)
(bootloader) pkt data sent (0x0)
(bootloader) i2c_get_chip: Searching bus 'i2c at 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