[PATCH] cmd: fdt: use U-Boot's FDT by default

[Simon Glass]

Hi Caleb,

On Sun, 1 Sept 2024 at 08:21, Caleb Connolly <caleb.connolly at linaro.org> wrote:
>
> Hi,
>
> On 31/08/2024 23:22, E Shattow wrote:
> > Hi Caleb, the problem here is hidden conditional behavior.
> >
> > On Sat, Aug 31, 2024 at 9:56 AM Caleb Connolly
> > <caleb.connolly at linaro.org> wrote:
> >>
> >> When using the FDT command to inspect an arbitrary FDT in memory, it
> >> will always be necessary to explicitly set the FDT address. However it
> >> is also quite likely that the command is being used to inspect U-Boot's
> >> own FDT. Simplify that common workflow of running "fdt addr -c" to get
> >> the control address and set it by just making $fdtcontroladdr the
> >> default FDT if there isn't one.
> >>
> >> Signed-off-by: Caleb Connolly <caleb.connolly at linaro.org>
> >> ---
> >>   cmd/fdt.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/cmd/fdt.c b/cmd/fdt.c
> >> index d16b141ce32d..8909706e2483 100644
> >> --- a/cmd/fdt.c
> >> +++ b/cmd/fdt.c
> >> @@ -276,8 +276,17 @@ static int do_fdt(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> >>
> >>                  return CMD_RET_SUCCESS;
> >>          }
> >>
> >> +       /* Try using U-Boot's FDT by default */
> >> +       if (!working_fdt) {
> >> +               struct fdt_header *addr;
> >> +
> >> +               addr = (void *)env_get_hex("fdtcontroladdr", 0);
> >> +               if (addr && fdt_check_header(&addr))
> >> +                       set_working_fdt_addr((phys_addr_t)addr);
> >> +       }
> >> +
> >>          if (!working_fdt) {
> >>                  puts("No FDT memory address configured. Please configure\n"
> >>                       "the FDT address via \"fdt addr <address>\" command.\n"
> >>                       "Aborting!\n");
> >> --
> >> 2.46.0
> >>
> >
> > The use of `fdt` command in the default action might be depended on by
> > some userspace script as a success/fail result. I don't imagine what
> > that might possibly be, just that the logic of scripts in u-boot
> > depend on that pattern of use.
>
> I'm not sure what the rule is about breaking changes in the CLI, I do
> think this is not a realistic concern here though. Maybe Tom or Simon
> can weigh in?
> >
> > Secondly there would need to be a warning to the user that some hidden
> > conditional action is being applied? i.e. "No valid FDT address is
> > configured to $fdt_addr_r or $fdt_addr so now configuring to use
> > $fdtcontroladdr instead." or however you would phrase that.
>
> Since I call set_working_fdt_addr() it prints a message telling you that
> the fdt address was set on the first call.
> >
> > Otherwise I agree improvement to the fdt is welcome and my memory of
> > first using this command is that it has not-sensible defaults but I
> > then assumed it was designed this way because of possible use in
> > U-Boot scripts.

Definitely a useful feature, but how about adding a flag which sets
the working fdt to the control one? That would make it less painful
than using -c, and will keep existing cases running.

Also note that test/cmd/fdt.c and doc/usage/cmd/fdt.rst need an update for this.

Regards,
Simon