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

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

Thanks.

-E