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

[Tom Rini]

On Mon, Sep 02, 2024 at 03:07:44PM +0200, Caleb Connolly wrote:
> Hi Simon,
> 
> On 01/09/2024 21:10, Simon Glass wrote:
> > 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 don't think anything depends on "fdt addr" returning 1 if nothing is
set.

> >> 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.

Yes, but it's not obvious as-is where that address came from / why,
since today that happens because you're passing that to the command.

> >>> 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.
> 
> Surely we have to /have/ some usecases to justify this??

Well, "fdt" the command was introduced back when Linux started taking
device trees on PowerPC platforms, so there wasn't some default to look
at. U-Boot isn't standardized on "fdtaddr" or "fdt_addr_r" or "fdt_addr"
as where the device tree for the OS is, only that "fdtcontroladdr" is
ours. So yes, I would also prefer a new flag to automatically set the
working address to fdtcontroladdr. But I assume there's a good reason to
not just do like other platforms have historically and fdt addr ... as
needed before other commands? Or is this really just for interactive
development?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20240903/d96f57ce/attachment.sig>