| Message ID | 20241120153642.861633-4-sjg@chromium.org |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Adjust how autoprobe is implemented | expand |
Hi Simon, Marex, On 20/11/2024 16:36, Simon Glass wrote: > Rather than doing autoprobe within the driver model code, move it out to > the board-init code. This makes it clear that it is a separate step from > binding devices. > > For now this is always done twice, before and after relocation, but we > should discuss whether it might be possible to drop the post-relocation > probe. > > For boards with SPL, the autoprobe is still done there as well. > > Note that with this change, autoprobe happens after the > EVT_DM_POST_INIT_R/F events are sent, rather than before. > > Link: https://lore.kernel.org/u-boot/20240626235717.272219-1-marex@denx.de/ > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- [...] > +For some platforms, certain devices must be probed to get the platform into > +a working state. To help with this, drivers marked with DM_FLAG_PROBE_AFTER_BIND I've found the documentation around this flag lacking, and the implementation confusing. Here you say /drivers/ with this flag (which would imply I can add it to the U_BOOT_DRIVER() definition, but further below the function doc says /device/ with this flag. In practise, it seems like the flag has to be added to the device in the bind() function, and adding it to the driver definition doesn't work (this left me scrataching my head for a while). The commit message adding it explains this and it seems like the idea was to have a way to trigger probing a device from the .bind() callback. I don't think I'm the only one confused by this, since grepping for it reveals two watchdog drivers which set this on their U_BOOT_DRIVER() definition. I can only see one driver that ever enables this flag conditionally, that's the ARM PSCI driver. Every other user sets it unconditionally in the .bind() callback. Would it make sense to rework this into a driver flag? Otherwise, could you document this behaviour more explicitly in this series? Kind regards, > +will be probed immediately after all devices are bound. For now, this happens in > +SPL, before relocation and after relocation. See the call to ``dm_autoprobe()`` > +for where this is done. > + > +The auto-probe feature is tricky because it bypasses the normal ordering of > +probing. General, if device A (e.g. video) needs device B (e.g. clock), then > +A's probe() method uses ``clk_get_by_index()`` and B is probed before A. But > +A is only probed when it is used. Therefore care should be taken when using > +auto-probe, limiting it to devices which truly are essential, such as power > +domains or critical clocks. > + > +See here for more discussion of this feature: > + > +:Link: https://patchwork.ozlabs.org/project/uboot/patch/20240626235717.272219-1-marex@denx.de/ > + > Running stage > ^^^^^^^^^^^^^ > > diff --git a/drivers/core/root.c b/drivers/core/root.c > index 2d4f078f97f..045f8fcda65 100644 > --- a/drivers/core/root.c > +++ b/drivers/core/root.c > @@ -281,6 +281,15 @@ void *dm_priv_to_rw(void *priv) > } > #endif > > +/** > + * dm_probe_devices() - Check whether to probe a device and all children > + * > + * Probes the device if DM_FLAG_PROBE_AFTER_BIND is enabled for it. Then scans > + * all its children recursively to do the same. > + * > + * @dev: Device to (maybe) probe > + * Return 0 if OK, -ve on error > + */ > static int dm_probe_devices(struct udevice *dev) > { > struct udevice *child; > @@ -299,6 +308,17 @@ static int dm_probe_devices(struct udevice *dev) > return 0; > } > > +int dm_autoprobe(void) > +{ > + int ret; > + > + ret = dm_probe_devices(gd->dm_root); > + if (ret) > + return log_msg_ret("pro", ret); > + > + return 0; > +} > + > /** > * dm_scan() - Scan tables to bind devices > * > @@ -331,7 +351,7 @@ static int dm_scan(bool pre_reloc_only) > if (ret) > return ret; > > - return dm_probe_devices(gd->dm_root); > + return 0; > } > > int dm_init_and_scan(bool pre_reloc_only) > diff --git a/include/dm/root.h b/include/dm/root.h > index b2f30a842f5..a71a2ebef85 100644 > --- a/include/dm/root.h > +++ b/include/dm/root.h > @@ -136,6 +136,21 @@ int dm_scan_other(bool pre_reloc_only); > */ > int dm_init_and_scan(bool pre_reloc_only); > > +/** > + * dm_autoprobe() - Probe devices which are marked for probe-after-bind > + * > + * This probes all devices with a DM_FLAG_PROBE_AFTER_BIND flag. It checks the > + * entire tree, so parent nodes need not have the flag set. > + * > + * It recursively probes parent nodes, so they do not need to have the flag > + * set themselves. Since parents are always probed before children, if a child > + * has the flag set, then its parent (and any devices up the chain to the root > + * device) will be probed too. > + * > + * Return: 0 if OK, -ve on error > + */ > +int dm_autoprobe(void); > + > /** > * dm_init() - Initialise Driver Model structures > *
Hi Caleb, On Wed, 20 Nov 2024 at 08:51, Caleb Connolly <caleb.connolly@linaro.org> wrote: > > Hi Simon, Marex, > > On 20/11/2024 16:36, Simon Glass wrote: > > Rather than doing autoprobe within the driver model code, move it out to > > the board-init code. This makes it clear that it is a separate step from > > binding devices. > > > > For now this is always done twice, before and after relocation, but we > > should discuss whether it might be possible to drop the post-relocation > > probe. > > > > For boards with SPL, the autoprobe is still done there as well. > > > > Note that with this change, autoprobe happens after the > > EVT_DM_POST_INIT_R/F events are sent, rather than before. > > > > Link: https://lore.kernel.org/u-boot/20240626235717.272219-1-marex@denx.de/ > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > [...] > > > +For some platforms, certain devices must be probed to get the platform into > > +a working state. To help with this, drivers marked with DM_FLAG_PROBE_AFTER_BIND > > I've found the documentation around this flag lacking, and the > implementation confusing. Here you say /drivers/ with this flag (which > would imply I can add it to the U_BOOT_DRIVER() definition, but further > below the function doc says /device/ with this flag. > > In practise, it seems like the flag has to be added to the device in the > bind() function, and adding it to the driver definition doesn't work > (this left me scrataching my head for a while). The commit message > adding it explains this and it seems like the idea was to have a way to > trigger probing a device from the .bind() callback. > > I don't think I'm the only one confused by this, since grepping for it > reveals two watchdog drivers which set this on their U_BOOT_DRIVER() > definition. > > I can only see one driver that ever enables this flag conditionally, > that's the ARM PSCI driver. Every other user sets it unconditionally in > the .bind() callback. > > Would it make sense to rework this into a driver flag? > > Otherwise, could you document this behaviour more explicitly in this series? I agree with you and I very-much want to document this better. The original patch[1] came with no documentation nor tests. It was applied within 5 days, after Sean raised exactly the same objections that I have with this feature. Those objections were overruled and the patch was applied anyway. So here I am trying to clean this up. As to the driver thing, my main concern is that it is too easy for people to add the flag, causing a large slowdown in U-Boot's pre-relocation startup speed. Perhaps the solution to that could be to enable bootstage everywhere and report the time in the banner? Regards, Simon [1] https://patchwork.ozlabs.org/project/uboot/patch/20220422131555.123598-1-marex@denx.de/
On 20/11/2024 17:03, Simon Glass wrote: > Hi Caleb, > > On Wed, 20 Nov 2024 at 08:51, Caleb Connolly <caleb.connolly@linaro.org> wrote: >> >> Hi Simon, Marex, >> >> On 20/11/2024 16:36, Simon Glass wrote: >>> Rather than doing autoprobe within the driver model code, move it out to >>> the board-init code. This makes it clear that it is a separate step from >>> binding devices. >>> >>> For now this is always done twice, before and after relocation, but we >>> should discuss whether it might be possible to drop the post-relocation >>> probe. >>> >>> For boards with SPL, the autoprobe is still done there as well. >>> >>> Note that with this change, autoprobe happens after the >>> EVT_DM_POST_INIT_R/F events are sent, rather than before. >>> >>> Link: https://lore.kernel.org/u-boot/20240626235717.272219-1-marex@denx.de/ >>> >>> Signed-off-by: Simon Glass <sjg@chromium.org> >>> --- >> >> [...] >> >>> +For some platforms, certain devices must be probed to get the platform into >>> +a working state. To help with this, drivers marked with DM_FLAG_PROBE_AFTER_BIND >> >> I've found the documentation around this flag lacking, and the >> implementation confusing. Here you say /drivers/ with this flag (which >> would imply I can add it to the U_BOOT_DRIVER() definition, but further >> below the function doc says /device/ with this flag. >> >> In practise, it seems like the flag has to be added to the device in the >> bind() function, and adding it to the driver definition doesn't work >> (this left me scrataching my head for a while). The commit message >> adding it explains this and it seems like the idea was to have a way to >> trigger probing a device from the .bind() callback. >> >> I don't think I'm the only one confused by this, since grepping for it >> reveals two watchdog drivers which set this on their U_BOOT_DRIVER() >> definition. >> >> I can only see one driver that ever enables this flag conditionally, >> that's the ARM PSCI driver. Every other user sets it unconditionally in >> the .bind() callback. >> >> Would it make sense to rework this into a driver flag? >> >> Otherwise, could you document this behaviour more explicitly in this series? > > I agree with you and I very-much want to document this better. > > The original patch[1] came with no documentation nor tests. It was > applied within 5 days, after Sean raised exactly the same objections > that I have with this feature. Those objections were overruled and the > patch was applied anyway. > > So here I am trying to clean this up. Ok, good to hear. Then it would be great if at least the comment above the flag definition could be made more explicit (that it must be applied to a device not a driver). > > As to the driver thing, my main concern is that it is too easy for > people to add the flag, causing a large slowdown in U-Boot's > pre-relocation startup speed. Perhaps the solution to that could be to > enable bootstage everywhere and report the time in the banner? I'm not sure what the problem is here. You're worried that people will set this flag when it isn't needed? This ought to be handled during review of the driver in question. Certainly I don't think making features more obscure will point people towards better solution, more likely they'll just get frustrated, particularly when this flag actually IS the correct solution as is the case for many drivers. Kind regards, > > Regards, > Simon > > [1] https://patchwork.ozlabs.org/project/uboot/patch/20220422131555.123598-1-marex@denx.de/
On 11/20/24 5:55 PM, Caleb Connolly wrote: > > > On 20/11/2024 17:03, Simon Glass wrote: >> Hi Caleb, >> >> On Wed, 20 Nov 2024 at 08:51, Caleb Connolly <caleb.connolly@linaro.org> wrote: >>> >>> Hi Simon, Marex, >>> >>> On 20/11/2024 16:36, Simon Glass wrote: >>>> Rather than doing autoprobe within the driver model code, move it out to >>>> the board-init code. This makes it clear that it is a separate step from >>>> binding devices. >>>> >>>> For now this is always done twice, before and after relocation, but we >>>> should discuss whether it might be possible to drop the post-relocation >>>> probe. >>>> >>>> For boards with SPL, the autoprobe is still done there as well. >>>> >>>> Note that with this change, autoprobe happens after the >>>> EVT_DM_POST_INIT_R/F events are sent, rather than before. >>>> >>>> Link: https://lore.kernel.org/u-boot/20240626235717.272219-1-marex@denx.de/ >>>> >>>> Signed-off-by: Simon Glass <sjg@chromium.org> >>>> --- >>> >>> [...] >>> >>>> +For some platforms, certain devices must be probed to get the platform into >>>> +a working state. To help with this, drivers marked with DM_FLAG_PROBE_AFTER_BIND >>> >>> I've found the documentation around this flag lacking, and the >>> implementation confusing. Here you say /drivers/ with this flag (which >>> would imply I can add it to the U_BOOT_DRIVER() definition, but further >>> below the function doc says /device/ with this flag. >>> >>> In practise, it seems like the flag has to be added to the device in the >>> bind() function, and adding it to the driver definition doesn't work >>> (this left me scrataching my head for a while). The commit message >>> adding it explains this and it seems like the idea was to have a way to >>> trigger probing a device from the .bind() callback. >>> >>> I don't think I'm the only one confused by this, since grepping for it >>> reveals two watchdog drivers which set this on their U_BOOT_DRIVER() >>> definition. >>> >>> I can only see one driver that ever enables this flag conditionally, >>> that's the ARM PSCI driver. Every other user sets it unconditionally in >>> the .bind() callback. >>> >>> Would it make sense to rework this into a driver flag? >>> >>> Otherwise, could you document this behaviour more explicitly in this series? >> >> I agree with you and I very-much want to document this better. >> >> The original patch[1] came with no documentation nor tests. It was >> applied within 5 days, after Sean raised exactly the same objections >> that I have with this feature. Those objections were overruled and the >> patch was applied anyway. >> >> So here I am trying to clean this up. > > Ok, good to hear. Then it would be great if at least the comment above > the flag definition could be made more explicit (that it must be applied > to a device not a driver). >> >> As to the driver thing, my main concern is that it is too easy for >> people to add the flag, causing a large slowdown in U-Boot's >> pre-relocation startup speed. Perhaps the solution to that could be to >> enable bootstage everywhere and report the time in the banner? > > I'm not sure what the problem is here. You're worried that people will > set this flag when it isn't needed? This ought to be handled during > review of the driver in question. > > Certainly I don't think making features more obscure will point people > towards better solution, more likely they'll just get frustrated, > particularly when this flag actually IS the correct solution as is the > case for many drivers. The flag is per-device , see e.g. c438866b1674 ("led: Mark device instance with DM_FLAG_PROBE_AFTER_BIND") . It was long needed for exactly that use case -- probe things like LEDs when they need to be preconfigured somehow.
diff --git a/common/board_f.c b/common/board_f.c index 1e1aa08530a..974eb4f39e9 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -822,6 +822,10 @@ static int initf_dm(void) if (ret) return ret; + ret = dm_autoprobe(); + if (ret) + return ret; + if (IS_ENABLED(CONFIG_TIMER_EARLY)) { ret = dm_timer_init(); if (ret) diff --git a/common/board_r.c b/common/board_r.c index 62228a723e1..b761833294f 100644 --- a/common/board_r.c +++ b/common/board_r.c @@ -241,7 +241,7 @@ static int initr_dm(void) if (ret) return ret; - return 0; + return dm_autoprobe(); } #endif diff --git a/common/spl/spl.c b/common/spl/spl.c index 1ceb63daf31..c4256f237ee 100644 --- a/common/spl/spl.c +++ b/common/spl/spl.c @@ -500,6 +500,10 @@ static int spl_common_init(bool setup_malloc) debug("dm_init_and_scan() returned error %d\n", ret); return ret; } + + ret = dm_autoprobe(); + if (ret) + return ret; } return 0; diff --git a/doc/develop/driver-model/design.rst b/doc/develop/driver-model/design.rst index 8c2c81d7ac9..92f638a0204 100644 --- a/doc/develop/driver-model/design.rst +++ b/doc/develop/driver-model/design.rst @@ -842,6 +842,23 @@ steps (see device_probe()): cause the uclass to do some housekeeping to record the device as activated and 'known' by the uclass. +For some platforms, certain devices must be probed to get the platform into +a working state. To help with this, drivers marked with DM_FLAG_PROBE_AFTER_BIND +will be probed immediately after all devices are bound. For now, this happens in +SPL, before relocation and after relocation. See the call to ``dm_autoprobe()`` +for where this is done. + +The auto-probe feature is tricky because it bypasses the normal ordering of +probing. General, if device A (e.g. video) needs device B (e.g. clock), then +A's probe() method uses ``clk_get_by_index()`` and B is probed before A. But +A is only probed when it is used. Therefore care should be taken when using +auto-probe, limiting it to devices which truly are essential, such as power +domains or critical clocks. + +See here for more discussion of this feature: + +:Link: https://patchwork.ozlabs.org/project/uboot/patch/20240626235717.272219-1-marex@denx.de/ + Running stage ^^^^^^^^^^^^^ diff --git a/drivers/core/root.c b/drivers/core/root.c index 2d4f078f97f..045f8fcda65 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -281,6 +281,15 @@ void *dm_priv_to_rw(void *priv) } #endif +/** + * dm_probe_devices() - Check whether to probe a device and all children + * + * Probes the device if DM_FLAG_PROBE_AFTER_BIND is enabled for it. Then scans + * all its children recursively to do the same. + * + * @dev: Device to (maybe) probe + * Return 0 if OK, -ve on error + */ static int dm_probe_devices(struct udevice *dev) { struct udevice *child; @@ -299,6 +308,17 @@ static int dm_probe_devices(struct udevice *dev) return 0; } +int dm_autoprobe(void) +{ + int ret; + + ret = dm_probe_devices(gd->dm_root); + if (ret) + return log_msg_ret("pro", ret); + + return 0; +} + /** * dm_scan() - Scan tables to bind devices * @@ -331,7 +351,7 @@ static int dm_scan(bool pre_reloc_only) if (ret) return ret; - return dm_probe_devices(gd->dm_root); + return 0; } int dm_init_and_scan(bool pre_reloc_only) diff --git a/include/dm/root.h b/include/dm/root.h index b2f30a842f5..a71a2ebef85 100644 --- a/include/dm/root.h +++ b/include/dm/root.h @@ -136,6 +136,21 @@ int dm_scan_other(bool pre_reloc_only); */ int dm_init_and_scan(bool pre_reloc_only); +/** + * dm_autoprobe() - Probe devices which are marked for probe-after-bind + * + * This probes all devices with a DM_FLAG_PROBE_AFTER_BIND flag. It checks the + * entire tree, so parent nodes need not have the flag set. + * + * It recursively probes parent nodes, so they do not need to have the flag + * set themselves. Since parents are always probed before children, if a child + * has the flag set, then its parent (and any devices up the chain to the root + * device) will be probed too. + * + * Return: 0 if OK, -ve on error + */ +int dm_autoprobe(void); + /** * dm_init() - Initialise Driver Model structures *
Rather than doing autoprobe within the driver model code, move it out to the board-init code. This makes it clear that it is a separate step from binding devices. For now this is always done twice, before and after relocation, but we should discuss whether it might be possible to drop the post-relocation probe. For boards with SPL, the autoprobe is still done there as well. Note that with this change, autoprobe happens after the EVT_DM_POST_INIT_R/F events are sent, rather than before. Link: https://lore.kernel.org/u-boot/20240626235717.272219-1-marex@denx.de/ Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v3: - Add a link to the previous discussion - Add a summary of the design challenge in the DM docs - Use a simple return in initr_dm() - Add a comment to dm_probe_devices() - Expand comment to dm_autoprobe() Changes in v2: - Add autoprobe to SPL also - Leave the function name the same - Fix 'Prove' typo - Update cover letter since SPL is now covered also - Update the documentation too, with a discussion link common/board_f.c | 4 ++++ common/board_r.c | 2 +- common/spl/spl.c | 4 ++++ doc/develop/driver-model/design.rst | 17 +++++++++++++++++ drivers/core/root.c | 22 +++++++++++++++++++++- include/dm/root.h | 15 +++++++++++++++ 6 files changed, 62 insertions(+), 2 deletions(-)