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