[PATCH v3 0/3] Adjust how autoprobe is implemented
This little series makes a minor change to how autoprobe is implemeneted, as discussed on the list:
https://patchwork.ozlabs.org/project/uboot/patch/ 20240626235717.272219-1-marex@denx.de/
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
Simon Glass (3): common: Drop check for DM in initf_dm() dm: core: Simplify dm_probe_devices() common: Move autoprobe out to board init
common/board_f.c | 9 ++++++-- common/board_r.c | 2 +- common/spl/spl.c | 4 ++++ doc/develop/driver-model/design.rst | 17 ++++++++++++++ drivers/core/root.c | 36 ++++++++++++++++++++--------- include/dm/root.h | 15 ++++++++++++ 6 files changed, 69 insertions(+), 14 deletions(-)
This is enabled by all boards, so drop the condition.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Tom Rini trini@konsulko.com ---
(no changes since v1)
common/board_f.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/common/board_f.c b/common/board_f.c index 98dc2591e1d..1e1aa08530a 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -811,9 +811,11 @@ static int initf_bootstage(void)
static int initf_dm(void) { -#if defined(CONFIG_DM) && CONFIG_IS_ENABLED(SYS_MALLOC_F) int ret;
+ if (!CONFIG_IS_ENABLED(SYS_MALLOC_F)) + return 0; + bootstage_start(BOOTSTAGE_ID_ACCUM_DM_F, "dm_f"); ret = dm_init_and_scan(true); bootstage_accum(BOOTSTAGE_ID_ACCUM_DM_F); @@ -825,7 +827,6 @@ static int initf_dm(void) if (ret) return ret; } -#endif
return 0; }
There is no point in checking the pre_reloc flag, since devices not marked as pre-reloc will not have been bound, so won't exist yet.
There doesn't seem to be any point in checking if the device has a valid devicetree node either, so drop that too.
Signed-off-by: Simon Glass sjg@chromium.org ---
(no changes since v1)
drivers/core/root.c | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/core/root.c b/drivers/core/root.c index 7a714f5478a..2d4f078f97f 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -281,26 +281,20 @@ void *dm_priv_to_rw(void *priv) } #endif
-static int dm_probe_devices(struct udevice *dev, bool pre_reloc_only) +static int dm_probe_devices(struct udevice *dev) { - ofnode node = dev_ofnode(dev); struct udevice *child; - int ret; - - if (pre_reloc_only && - (!ofnode_valid(node) || !ofnode_pre_reloc(node)) && - !(dev->driver->flags & DM_FLAG_PRE_RELOC)) - goto probe_children;
if (dev_get_flags(dev) & DM_FLAG_PROBE_AFTER_BIND) { + int ret; + ret = device_probe(dev); if (ret) return ret; }
-probe_children: list_for_each_entry(child, &dev->child_head, sibling_node) - dm_probe_devices(child, pre_reloc_only); + dm_probe_devices(child);
return 0; } @@ -337,7 +331,7 @@ static int dm_scan(bool pre_reloc_only) if (ret) return ret;
- return dm_probe_devices(gd->dm_root, pre_reloc_only); + return dm_probe_devices(gd->dm_root); }
int dm_init_and_scan(bool pre_reloc_only)
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(-)
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-mar... + 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 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-mar...
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-mar...
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-mar...
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.
On Wed, 20 Nov 2024 08:36:38 -0700, Simon Glass wrote:
This little series makes a minor change to how autoprobe is implemeneted, as discussed on the list:
https://patchwork.ozlabs.org/project/uboot/patch/ 20240626235717.272219-1-marex@denx.de/
[...]
Applied to u-boot/master, thanks!
participants (4)
-
Caleb Connolly -
Marek Vasut -
Simon Glass -
Tom Rini