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