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

[Caleb Connolly]

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

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)