[RFC PATCH 2/2 v1] drivers: core: lists.c: Bind drivers using bootph* property in subnodes

[Simon Glass]

Hi Moteen,

On Wed, 12 Feb 2025 at 02:18, Moteen Shah <m-shah at ti.com> wrote:
>
> Add a function to scan through all the subnodes of a given node
> recusively for bootph* property. If found, propagate it to all
> of its parent node up the hierarchy so as to bind the respective
> drivers of the nodes in the pre-relocation stage.
>
> The current model skips the node if there is no bootph* found in
> it, even if the property is present in one of the subnodes. The
> issue is tracked in [0].
>
> Signed-off-by: Moteen Shah <m-shah at ti.com>
>
> References:
> [0] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/21
>
> ---
>  drivers/core/lists.c | 88 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/core/lists.c b/drivers/core/lists.c
> index c7be504b6fc..5be556ea951 100644
> --- a/drivers/core/lists.c
> +++ b/drivers/core/lists.c
> @@ -17,6 +17,7 @@
>  #include <dm/platdata.h>
>  #include <dm/uclass.h>
>  #include <dm/util.h>
> +#include <fdt_support.h>
>  #include <fdtdec.h>
>  #include <linux/compiler.h>
>
> @@ -196,6 +197,80 @@ static int driver_check_compatible(const struct udevice_id *of_match,
>         return -ENOENT;
>  }
>
> +/**
> + * scan_and_prop_bootph() - Check if a driver matches a compatible string
> + *
> + * @param of_node:     Parent node for child traversal
> + * @param fdt: Blob to use

We stopped using @param, so just @of_node now

Document return

> + */
> +static int scan_and_prop_bootph(ofnode node, void *fdt)
> +{
> +       static int bootph_index = -1;
> +       static const char * const bootph_props[] = {
> +               "bootph-all",
> +               "bootph-some-ram",
> +               "bootph-pre-ram",
> +               "bootph-pre-sram",

Since this is pre-relocation, you should only need the first two. But
there is ofnode_pre_reloc() which you can call.

BTW ofnode_pre_reloc() has the checks you have here. I'm not sure if
this code can be dropped?:

/*
* In regular builds individual spl and tpl handling both
* count as handled pre-relocation for later second init.
*/
if (ofnode_read_bool(node, "bootph-pre-ram") ||
    ofnode_read_bool(node, "bootph-pre-sram"))
return gd->flags & GD_FLG_RELOC;


> +       };
> +       int offset;
> +       int ret = -ENOENT;
> +       ofnode subnode;
> +
> +       /* Note the index where bootph-* was found and return success */
> +       for (int i = 0; i < ARRAY_SIZE(bootph_props); i++) {
> +               if (ofnode_read_bool(node, bootph_props[i])) {
> +                       bootph_index = i;
> +                       return 0;
> +               }
> +       }
> +
> +       ofnode_for_each_subnode(subnode, node) {
> +               if (!ofnode_valid(subnode))
> +                       continue;
> +
> +               ret = scan_and_prop_bootph(subnode, fdt);
> +               if (ret != -ENOENT && ret)
> +                       return ret;
> +
> +               /*
> +                * Break the search if bootph-* is found in any of the subnodes.
> +                * Breaking the loop early helps in avoiding unnecessary traversal
> +                * of the sibling node given that bootph-* has been found in previous
> +                * sibling node.
> +                */
> +               if (!ret)
> +                       break;
> +       }
> +
> +       /* If no bootph-* found then return no such entry */
> +       if (ret)
> +               return -ENOENT;
> +
> +       if (bootph_index != -1) {
> +               offset = ofnode_to_offset(node);
> +               if (offset < 0) {
> +                       log_err("Failed to get offset %d\n", offset);
> +                       return -EINVAL;
> +               }
> +
> +               ret = fdt_increase_size(fdt, 32);
> +               if (ret) {
> +                       log_err("Cannot increase FDT size!\n");
> +                       return ret;
> +               }
> +
> +               ret = fdt_setprop_empty(fdt, offset, bootph_props[bootph_index]);

I don't believe you need to actually add the property. It costs time
and I don't believe anything else will read it. You should just need
to return a value indicating whether the tag was found.

> +               if (ret) {
> +                       log_err("Failed to add bootph prop to node:%s ret=%d\n",
> +                               ofnode_get_name(node), ret);
> +                       return ret;
> +               }
> +               bootph_index = -1;
> +       }
> +
> +       return ret;
> +}
> +
>  int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>                    struct driver *drv, bool pre_reloc_only)
>  {
> @@ -205,6 +280,7 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>         struct driver *entry;
>         struct udevice *dev;
>         bool found = false;
> +       void *fdt = (void *)gd->fdt_blob;

Could you drop this var? I am hoping you don't need to change the devicetree.

>         const char *name, *compat_list, *compat;
>         int compat_length, i;
>         int result = 0;
> @@ -256,8 +332,16 @@ int lists_bind_fdt(struct udevice *parent, ofnode node, struct udevice **devp,
>                 if (pre_reloc_only) {
>                         if (!ofnode_pre_reloc(node) &&
>                             !(entry->flags & DM_FLAG_PRE_RELOC)) {
> -                               log_debug("Skipping device pre-relocation\n");
> -                               return 0;
> +                               if (IS_ENABLED(CONFIG_BIND_FROM_CHILD_BOOTPH)) {
> +                                       ret = scan_and_prop_bootph(node, fdt);
> +                                       if (ret) {
> +                                               log_debug("Skipping device pre-relocation\n");
> +                                               return 0;
> +                                       }
> +                               } else {
> +                                       log_debug("Skipping device pre-relocation\n");
> +                                       return 0;
> +                               }

Can you either use a bool or combine the statements so there is only
one log_debug() ?

if (!IS_ENABLED(CONFIG_BIND_FROM_CHILD_BOOTPH) ||
   scan_and_prop_bootph(node, fdt)) {
     log_debug...

>                         }
>                 }
>
> --
> 2.34.1
>

Please also add a test for this, probably to test/dm/test-fdt.c

Did you look at doing this in Binman?

Regards,
Simon