On 03/09/2024 20:29, Tom Rini wrote:
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 <[email protected]> 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
<[email protected]> 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 <[email protected]>
---
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?
My original justification was so we could add a boot menu option on
phones that just did "fdt print /chosen bootargs" (to dump the cmdline
args set by Qualcomm's bootloader). I originally had "fdt addr
$fdtcontroladdr" in preboot but it felt a bit hacky, but maybe that was
the right approach
--
// Caleb (they/them)
