Hi Moteen,
On Wed, 12 Feb 2025 at 02:18, Moteen Shah <m-s...@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-s...@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
