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@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@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.
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??
Also note that test/cmd/fdt.c and doc/usage/cmd/fdt.rst need an update for this.
For sure
Regards, Simon