Hi, On Fri, 20 Sept 2024 at 18:49, Tom Rini <tr...@konsulko.com> wrote: > > On Fri, Sep 20, 2024 at 07:40:35PM +0300, Svyatoslav Ryhel wrote: > > пн, 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! > > > > 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?
We did have a pmic maintainer for a while, but perhaps has gone quiet? I am OK with a call to regulators_enable_boot_on(), but where should it go? Should it happen in SPL? Should it happen before relocation? How is that decided and controlled? I actually don't like DM_FLAG_PROBE_AFTER_BIND since it makes bind == probe, something which I believe U-Boot should really try to avoid. I would even rather have an explicit call in initf_dm() - or perhaps better initr_dm()? My favourite option would be a new line in the board_r init sequence instead of power_init_board(), which predates driver model. Regards, Simon
