[PATCH 09/10] fdtgrep: Allow propagating properties up to supernodes

[Manorit Chawdhry]

Hi Simon,

On 10:39-20240711, Simon Glass wrote:
> Hi Manorit,
> 
> On Wed, 10 Jul 2024 at 07:15, Manorit Chawdhry <m-chawdhry at ti.com> wrote:
> >
> > Hi Simon,
> >
> > On 09:36-20231217, Simon Glass wrote:
> > > The existing bootph binding is defined such that properties in a
> > > subnode are also implied in the supernode also, as in this example:
> > >
> > >    buttons {
> > >       /* bootph,pre-ram is implied by btn1 */
> > >       compatible = "gpio-keys";
> > >
> > >       btn1 {
> > >          bootph,pre-ram;
> > >          gpios = <&gpio_a 3 0>;
> > >          label = "button1";
> > >          linux,code = <BTN_1>;
> > >       };
> > >
> > > Provide an option to implement this in fdtgrep.
> > >
> > > Signed-off-by: Simon Glass <sjg at chromium.org>
> >
> > Coming back to this patch as am facing some issues with the bootph-all
> > propagation to the parent nodes [0]. We have a dmsc/sms node in our
> > devices and it is a parent node of k3_pds, k3_clks, k3_reset.
> >
> > During U-boot proper we are initializing the serial console and during
> > that time we have to make some calls to our Device manager for clocks
> > and power domains. With the DT modelling, the clocks and power domains
> > come under the device manager node ( dmsc/sms ) but interestingly that
> > driver is not getting probed while putting bootph-all in the child
> > nodes. Do you know anything about this? I thought putting it in child
> > node will propagate it to the parent nodes as well so it'll be probed as
> > well. We are having to put bootph-all explicitely in the dmsc/sms node
> > for it to work.
> 
> Yes, that is because fdtgrep is only used with the SPL DT. The U-Boot
> one is used unmodified by U-Boot and it does not check parents for
> such properties (neither does SPL, but fdtgrep has logic to imply
> these properties internally).
> 
> The easiest way to fix this would probably be to process the DT in
> Binman for insertion into the final image, since Binman already does
> lots of other DT processing.
> 
> If you like, you could create an issue for it [1]. If you want to have
> a crack at fixing it, I could provide some pointers.

I am not sure if I'll be able to focus on this now but feel free to give
your pointers, might help anyone else as well if they want to take a
look at it.

Meanwhile I will open a issue as you mentioned for tracking this, trying
to register on the URL that you provided. Thanks!

Regards,
Manorit

> 
> Regards,
> Simon
> 
> [1] https://source.denx.de/u-boot/custodians/u-boot-dm/-/issues
> 
> 
> >
> > [0]: https://lore.kernel.org/linux-arm-kernel/20240705-b4-upstream-bootph-all-v2-0-9007681ed7d8@ti.com/T/#t
> >
> > Regards,
> > Manorit
> > > ---
> > >
> > >  tools/fdtgrep.c | 18 +++++++++++++++++-
> > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c
> > > index ca639a2d9f4f..f1ff1946bd4a 100644
> > > --- a/tools/fdtgrep.c
> > > +++ b/tools/fdtgrep.c
> > > @@ -63,6 +63,7 @@ struct display_info {
> > >       int types_inc;          /* Mask of types that we include (FDT_IS...) */
> > >       int types_exc;          /* Mask of types that we exclude (FDT_IS...) */
> > >       int invert;             /* Invert polarity of match */
> > > +     int props_up;           /* Imply properties up to supernodes */
> > >       struct value_node *value_head;  /* List of values to match */
> > >       const char *output_fname;       /* Output filename */
> > >       FILE *fout;             /* File to write dts/dtb output */
> > > @@ -606,6 +607,16 @@ static int check_props(struct display_info *disp, const void *fdt, int node,
> > >                                        strlen(str));
> > >       }
> > >
> > > +     /* if requested, check all subnodes for this property too */
> > > +     if (inc != 1 && disp->props_up) {
> > > +             int subnode;
> > > +
> > > +             for (subnode = fdt_first_subnode(fdt, node);
> > > +                  subnode > 0 && inc != 1;
> > > +                  subnode = fdt_next_subnode(fdt, subnode))
> > > +                     inc = check_props(disp, fdt, subnode, inc);
> > > +     }
> > > +
> > >       return inc;
> > >  }
> > >
> > > @@ -955,7 +966,7 @@ static const char usage_synopsis[] =
> > >       case '?': usage("unknown option");
> > >
> > >  static const char usage_short_opts[] =
> > > -             "haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTv"
> > > +             "haAc:b:C:defg:G:HIlLmn:N:o:O:p:P:rRsStTuv"
> > >               USAGE_COMMON_SHORT_OPTS;
> > >  static const struct option usage_long_opts[] = {
> > >       {"show-address",        no_argument, NULL, 'a'},
> > > @@ -985,6 +996,7 @@ static const struct option usage_long_opts[] = {
> > >       {"skip-supernodes",     no_argument, NULL, 'S'},
> > >       {"show-stringtab",      no_argument, NULL, 't'},
> > >       {"show-aliases",        no_argument, NULL, 'T'},
> > > +     {"props-up-to-supernode", no_argument, NULL, 'u'},
> > >       {"invert-match",        no_argument, NULL, 'v'},
> > >       USAGE_COMMON_LONG_OPTS,
> > >  };
> > > @@ -1016,6 +1028,7 @@ static const char * const usage_opts_help[] = {
> > >       "Don't include supernodes of matching nodes",
> > >       "Include string table in binary output",
> > >       "Include matching aliases in output",
> > > +     "Add -p properties to supernodes too",
> > >       "Invert the sense of matching (select non-matching lines)",
> > >       USAGE_COMMON_OPTS_HELP
> > >  };
> > > @@ -1202,6 +1215,9 @@ static void scan_args(struct display_info *disp, int argc, char *argv[])
> > >               case 'T':
> > >                       disp->add_aliases = 1;
> > >                       break;
> > > +             case 'u':
> > > +                     disp->props_up = 1;
> > > +                     break;
> > >               case 'v':
> > >                       disp->invert = 1;
> > >                       break;
> > > --
> > > 2.43.0.472.g3155946c3a-goog
> > >