Hi Simon,
On 01/09/2024 21:10, Simon Glass wrote:
> Hi Caleb,
>
> On Sun, 1 Sept 2024 at 08:21, Caleb Connolly <caleb.conno...@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.conno...@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.conno...@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
--
// Caleb (they/them)
