[PATCH v3 3/3] common: Move autoprobe out to board init

[Caleb Connolly]

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 at 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
>   *

-- 
// Caleb (they/them)