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

[Moteen Shah <m-shah@ti.com>]

Hey Simon

On 12/02/25 19:07, Simon Glass wrote:
> Hi Moteen, On Wed, 12 Feb 2025 at 02: 18, Moteen Shah 
> <m-shah@ 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
> ZjQcmQRYFpfptBannerStart
> This message was sent from outside of Texas Instruments.
> Do not click links or open attachments unless you recognize the source 
> of this email and know the content is safe.
> Report Suspicious
> <https://us-phishalarm-ewt.proofpoint.com/EWT/v1/G3vK!tDdkczlA_gRXoS0MsZKL_DT2tZpXfsvUu_exS1E1jUk7Sm4iKSS-jq4dD2ZmFBvi3F-y_3fESFLg5xMd4B_mX8lyj-MIXHAbpyrcoh9f55Q5L2iNtE7kn3s$> 
>
> ZjQcmQRYFpfptBannerEnd
> Hi Moteen,
>
> On Wed, 12 Feb 2025 at 02:18, Moteen Shah<m-shah@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@ti.com>
> >
> > References:
> > [0] https://urldefense.com/v3/__https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues/21__;!!G3vK!W_iIPAjX_8KQ0KYkaLmMpffBZbsxQLF5MvKHbZeivm2bxn7gih_UYCufMU0pfun_xo1lK17Y$
> >
> > ---
> >  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
Noted, will fix this up in v2.
> > + */
> > +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?:
I started with using ofnode_pre_reloc(), but the function won't return
which bootph-* property was found hence the manual checks here.
> /*
> * 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.
Considering a case that we are deep inside the recursion and bootph-*
is found, with the return value approach we will be able to bind the
super parent node but for nodes under this super parent, we will again
have to traverse the DT right?

One more approach I thought of was to bind the nodes in this function
itself if bootph-* is found in subnode, but for that we will require the
driver/device info of that node which we don't have at this point in the
code flow. (Again considering a node deep in the DT).

> > +               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.
Sure, if the return value approach works.
> >         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() ?
Noted, will fix this in v2.
> 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
Noted.
> Did you look at doing this in Binman?
Haven't explored much on the binman implementation, if my understanding
is correct we might have to decompile the blob and do the fixup in
blob_dtb.py likely?
If you have some more pointers on this do let me know.
> Regards,
> Simon


Thank you for the review.

Regards,
Moteen