Re: [PATCH 2/5] common: deep-probe: support specifying support in DT

[Marco Felsch <m.felsch@pengutronix.de>]

Hi Ahmad,

On 25-05-28, Ahmad Fatoum wrote:
> New boards should be deep probe enabled by default, which is currently
> done via board code. We want to support second stage boot on new boards
> without board code however, which means we need the possibility to make
> deep probe opt-out instead of opt-in.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
> ---
>  .../bindings/barebox/barebox,deep-probe.rst   | 39 +++++++++++++++++++
>  common/deep-probe.c                           | 26 +++++++++++--
>  drivers/base/Kconfig                          | 22 +++++++++++
>  3 files changed, 84 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/barebox/barebox,deep-probe.rst
> 
> diff --git a/Documentation/devicetree/bindings/barebox/barebox,deep-probe.rst b/Documentation/devicetree/bindings/barebox/barebox,deep-probe.rst
> new file mode 100644
> index 000000000000..bfc28ac9d1ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/barebox/barebox,deep-probe.rst
> @@ -0,0 +1,39 @@
> +Deep probe properties
> +=====================
> +
> +A ``barebox,deep-probe`` property in the top-level node indicates to barebox
> +that the barebox board and device drivers support recursively probing devices
> +on demand (deep probe).
> +
> +For drivers to support deep probe, they must not rely on initcall ordering.
> +Resources needed by drivers should be referenced via device tree, e.g.,
> +instead of direct use of hardcoded GPIO numbers, GPIOs must either be:
> +
> +* described in the device's device tree node and requested using API that
> +  that takes the device or the device tree node as argument
> +
> +* probe of the GPIO controller be ensured via ``of_device_ensure_probed``,
> +  e.g., ``of_devices_ensure_probed_by_property("gpio-controller");``
> +
> +The inverted flag is ``barebox,disable-deep-probe``, which means that the
> +board is not deep probe capable (or tested) yet.
> +
> +The ``barebox,disable-deep-probe`` property takes precedence over
> +``barebox,deep-probe``, but not over ``BAREBOX_DEEP_PROBE_ENABLE``
> +in the board code.
> +
> +If neither property exists, the default deep probe behavior depends on
> +the ``CONFIG_DEEP_PROBE_DEFAULT`` variable.
> +
> +.. code-block:: none
> +
> +   / { /* SoM Device Tree */
> +   	barebox,deep-probe;
> +   };
> +
> +   / { /* Board Device Tree */
> +        /* FIXME: While the SoM supports deep probe, our board code is broken
> +	 * currently, so override until it's fixed
> +	 */
> +   	barebox,disable-deep-probe;
> +   };
> diff --git a/common/deep-probe.c b/common/deep-probe.c
> index ab1da87b626b..0a7f2f3769ae 100644
> --- a/common/deep-probe.c
> +++ b/common/deep-probe.c
> @@ -20,11 +20,18 @@ static enum deep_probe_state boardstate = DEEP_PROBE_UNKNOWN;
>  
>  bool deep_probe_is_supported(void)
>  {
> +	bool deep_probe_default = IS_ENABLED(CONFIG_DEEP_PROBE_DEFAULT);
>  	struct deep_probe_entry *board;
> +	struct device_node *root;
>  
>  	if (boardstate > DEEP_PROBE_UNKNOWN)
>  		return boardstate;
>  
> +	/* deep probe requires resources to be described in DT */
> +	root = of_get_root_node();
> +	if (!root)
> +		return false;

	if (!root) {
		boardstate = DEEP_PROBE_NOT_SUPPORTED;
		return false;
	}

> +
>  	/* determine boardstate */
>  	for (board = __barebox_deep_probe_start;
>  	     board != __barebox_deep_probe_end; board++) {
> @@ -39,8 +46,21 @@ bool deep_probe_is_supported(void)
>  		}
>  	}
>  
> -	boardstate = DEEP_PROBE_NOT_SUPPORTED;
> -	pr_info("not activated\n");
> -	return false;

Can we please add a comment about the below if-else, e.g. referencing
the barebox,deep-probe.rst as well?

> +	if (of_property_read_bool(root, "barebox,disable-deep-probe")) {
> +		boardstate = DEEP_PROBE_NOT_SUPPORTED;
> +		pr_info("disabled in device tree\n");
> +	} else if (of_property_read_bool(root, "barebox,deep-probe")) {
> +		boardstate = DEEP_PROBE_SUPPORTED;
> +		pr_debug("enabled in device tree\n");
> +	} else if (deep_probe_default) {
> +		boardstate = DEEP_PROBE_SUPPORTED;
> +		pr_debug("activated by default\n");
> +	} else {
> +		boardstate = DEEP_PROBE_NOT_SUPPORTED;
> +		pr_warn("DT missing barebox,deep-probe or barebox,disable-deep-probe property\n");
> +		pr_info("not activated by default\n");
> +	}
> +
> +	return boardstate;
>  }
>  EXPORT_SYMBOL_GPL(deep_probe_is_supported);
> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> index 21a4793cfa47..5766e2afda9a 100644
> --- a/drivers/base/Kconfig
> +++ b/drivers/base/Kconfig
> @@ -1,5 +1,27 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
> +config DEEP_PROBE_DEFAULT
> +	bool "Probe devices recursively on-demand"
> +	help
> +          The barebox 'deep probe' or 'probe on demand' mechanism gets
> +          rid of the EPROBE_DEFER error code from probes by reordering
> +          the device population and the driver registration.
> +          All drivers are registered first and afterwards devices are
> +          populated. When a device probing requires a resource that's
> +          not yet available, the device providing that resource is probed
> +          recursively, hence the probe callstack gets deeper until all
> +          resources are available.
> +
> +          This changes the order in which drivers are initialized, which
> +          can unearth bugs in drivers. Board code that makes use of
> +          different initcall levels to interleave with driver probes
> +          will behave differently.
> +
> +          If unsure and you want to fix implicit assumptions that may
> +          break your startup, say 'y'.
> +          If unsure and you want deep probe to only be enabled
> +          explicitly per top-level machine compatible, say 'n'.
> +
>  config PM_GENERIC_DOMAINS
>  	bool
>  
> -- 
> 2.39.5
> 
> 
>