§ã§â, 25 §Ó§Ö§â. 2024?§â. §à 02:44 Tom Rini <trini@...> §á§Ú§ê§Ö:
On Fri, Sep 20, 2024 at 10:48:56AM -0600, Tom Rini wrote:
On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote:
§á§ß, 16 §Ó§Ö§â. 2024?§â. §à 19:28 Tom Rini <trini@...> §á§Ú§ê§Ö:
On Wed, Sep 11, 2024 at 07:00:56PM -0600, Simon Glass wrote:
Hi Marek,
On Fri, 28 Jun 2024 at 07:26, Marek Vasut <marex@...> wrote:
On 6/28/24 9:32 AM, Simon Glass wrote:
Hi Marek,
Hi,
[...]
@@ -473,8 +483,6 @@ static int regulator_pre_probe(struct udevice *dev)
-ENODATA);
uc_pdata->max_uA = dev_read_u32_default(dev, "regulator-max-microamp",
-ENODATA);
- uc_pdata->always_on = dev_read_bool(dev, "regulator-always-on");
- uc_pdata->boot_on = dev_read_bool(dev, "regulator-boot-on");
uc_pdata->ramp_delay = dev_read_u32_default(dev, "regulator-ramp-delay",
0);
uc_pdata->force_off = dev_read_bool(dev, "regulator-force-boot-off");
--
2.43.0
This is reading a lot of DT stuff very early, which may be slow. It is
also reading from the DT in the bind() step which we sometimes have to
do, but try to avoid.
Actually, it is reading only the bare minimum very early in bind, the
always-on and boot-on, the rest is in pre_probe, i.e. later.
Yes, I see that. Also it is inevitable that these properties need to
be read before probe(), since they control whether to probe().
Also this seems to happen in SPL and again pre-reloc and again in
U-Boot post-reloc?
What does, the uclass post_bind ?
I mean that this code will be called in SPL (if the regulators are in
the DT there), U-Boot pre-reloc and post-reloc, each time turning on
the regulators. We need a way to control that, don't we?
I would assume that if those regulators are turned on once in the
earliest stage , turning them on again in the follow up stage would be a
noop ? This is the point of regulator-*-on, to keep the regulators on.
No, there is sometimes a particular sequence needed.
Should we have a step in the init sequence where we set up the
regulators, by calling regulators_enable_boot_on() ?
Let's not do this , the entire point of this series is to get rid of
those functions and do the regulator configuration the same way LED
subsystem does it -- by probing always-on/boot-on regulators and
configuring them correctly on probe.
To me regulators_enable_boot_on() and the like was always an
inconsistently applied workaround for missing DM_FLAG_PROBE_AFTER_BIND ,
which is now implemented, so such workarounds can be removed.
That patch seemed to slip under the radar, sent and applied on the
same day! We really need to add a test for it, BTW.
Which patch ? My recollection of DM_FLAG_PROBE_AFTER_BIND was that it
took a while to get in.
[1]
My concern is that this might cause us ordering problems. We perhaps
want the regulators to be done first. If drivers are probed which use
regulators, then presumably they will enable them. Consider this:
- LED driver auto-probes
- probes I2C bus 2
- probes LDO1 which is autoset so turns on
- LDO1 probes, but is already running
- LDO2 probes, which is autoset so turns on
So long as it is OK to enable the regulators in any order, then this
seems fine. But is it? How do we handle the case where are particular
sequence is needed?
Did we finally arrive at the point where we need -EPROBE_DEFER alike
mechanism ?
Nope. But I am concerned that this patch is leading us there. bind()
really needs to be as clean as possible. It is one of the design
elements of driver model which Linux should adopt.
There is always a race to be the first to init something, move the
init earlier, etc... I don't see any general need to init the
regulators right at the start. It should be done in a separate
function and be optional. I am happy to send a patch if you like.
Since we're currently stuck on the point where Marek has patches that
fix a real problem, and Svyatoslav has a problem with them, but isn't
currently able to debug it, yes, please put forward your patch that
might address both sets of problems so we can all figure out how to
resolve the problems, thanks!
With patches from Marek there is no i2c chip probe of PMIC, while
without i2c chip probe is called (probe_chip function). How this is
even possible?
Yes, it's very puzzling. Do you have the ability to get some debug
console type information out so you can sprinkle in some prints and
figure it out?
So, here's my plan. Marek posted
which is a work-around, so that v2024.10 can work. I'm going to take
that for master tomorrow. I'm also going to take _this_ series to -next
tomorrow, as this is the best approach we currently have to solving the
overall problem. The Tegra platforms that are now very oddly broken need
to get debugged to see where their problem is, and if there's an
entirely alternate approach, Simon can post patches for that instead on
top of next.
--
Tom
Hello there!
I was digging this when I had some free time and found that with
patches from Marek the only difference is that function
i2c_get_chip_for_busnum is not called for PMIC's main i2c address
which results in issues with i2c you have seen in logs before. I
assume this is not a tegra specific issue and not even related to
these regulator patches itself.
To verify my suspicions, Tom and Marek my you please dump u-boot log
with and without patches and with enabled debug logging from
i2c-uclass and i2c driver of your SoC.
Here are logs from my device (Tegra SoC)
Not working
(bootloader) i2c: controller bus 0 at 7000d000, speed 0:
i2c_init_controller: speed=400000
(bootloader) i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25
(bootloader) i2c_get_chip: Searching bus 'i2c@7000d000' for address 59:
(bootloader) not found
(bootloader) pmic_reg_read: reg=37 priv->trans_len:1i2c_xfer: 2 messages
(bootloader)
(bootloader) i2c_xfer: chip=0x58, len=0x1
(bootloader) i2c_write_data: chip=0x58, len=0x1
(bootloader) write_data: 0x37
(bootloader) pkt header 1 sent (0x10010)
(bootloader) pkt header 2 sent (0x0)
(bootloader) pkt header 3 sent (0x100b0)
(bootloader) pkt data sent (0x37)
(bootloader) tegra_i2c_write_data: Error (-1) !!!
Working
(bootloader) i2c: controller bus 0 at 7000d000, speed 0:
i2c_init_controller: speed=400000
(bootloader) i2c_init_controller: CLK_DIV_STD_FAST_MODE setting = 25
(bootloader) pkt header 1 sent (0x10010)
(bootloader) pkt header 2 sent (0x0)
(bootloader) pkt header 3 sent (0xb0)
(bootloader) pkt data sent (0x0)
(bootloader) i2c_get_chip: Searching bus 'i2c@7000d000' for address 58:
(bootloader) i2c_get_chip: Searching bus 'i2c@7000d000' for address 59:
(bootloader) not found
(bootloader) found, ret=0
(bootloader) i2c_xfer: 2 messages
(bootloader) i2c_xfer: chip=0x58, len=0x1
(bootloader) i2c_write_data: chip=0x58, len=0x1
(bootloader) write_data: 0xfb
(bootloader) pkt header 1 sent (0x10010)
(bootloader) pkt header 2 sent (0x0)
(bootloader) pkt header 3 sent (0x100b0)
(bootloader) pkt data sent (0xfb)
(bootloader) i2c_xfer: chip=0x58, len=0x1
As you can see this part
(bootloader) pkt header 1 sent (0x10010)
(bootloader) pkt header 2 sent (0x0)
(bootloader) pkt header 3 sent (0xb0)
(bootloader) pkt data sent (0x0)
(bootloader) i2c_get_chip: Searching bus 'i2c@7000d000' for address 58:
is missing in log from u-boot where Marek's patches are applied and
this log fragment co-responds to i2c_get_chip_for_busnum call
