[PATCH 6/8] sunxi: board: Set fdtfile to match the DT chosen by SPL

[Andre Przywara <andre.przywara@arm.com>]

On 22/09/2020 02:33, Samuel Holland wrote:
> On 9/21/20 7:43 PM, Andr? Przywara wrote:
>> On 03/09/2020 06:07, Samuel Holland wrote:
>>
>> Hi,
>>
>>> Previously, fdtfile was always the value in CONFIG_DEFAULT_DEVICE_TREE.
>>> This meant that, regardless of the DT chosen by SPL (either by changing
>>> the header in the image or by the selection code at runtime), Linux
>>> always used the default DT.
>>>
>>> By using the name from the SPL header (which, because of the previous
>>> commit, always matches the DT used by U-Boot proper), Linux also sees
>>> the same board as U-Boot/SPL, even if the boot script later loads a DT
>>> from disk.
>>
>> I strongly frown upon proliferating the broken way of explicitly loading
>> a DT from somewhere, when the DT embedded in U-Boot should be all we
>> will ever need.
>
> The embedded DT is only "all you ever need" if 1) your DT is 100% complete and
> accurate (ha, ha, ha) or 2) you re-flash U-Boot every time the device tree
> changes, which is risky and an unnecessary waste of flash memory write cycles.

Ideally it doesn't change that often, and I don't see how a DT update on
a filesystem would be different when it comes to flash tear and wear.
Either U-Boot and the boot FS live on the very same (NAND flash) based
media (eMMC), or U-Boot lives on NOR flash, which has a much greater
number of write cycles.

> Having a built-in DT that's always available and mostly works is quite useful,
> but complaining that distros and users want to update their DTs without
> patching, recompiling, and re-flashing U-Boot is frankly ridiculous. Especially
> considering that the U-Boot DTs are usually out of date, and users might not
> want to update the U-Boot code for various reasons.

You have a point here: U-Boot updates are typically more tedious and
more risky. If it just comes to a DT update, this could actually be done
without compiling U-Boot, though, by re-packing the FIT image.

The more general problem is that you rely on your distributions to ship
your board's DT. In the embedded world, where distribution means "some
image specifically compiled for this particular board", this isn't a
problem. But for more generic distributions, or OSes outside of Linux,
it can become quite tedious to collect all possible DTs.
Also for instance Linux 5.7 or 5.8 don't carry the Pinephone-1.2 DTS,
even though the code is perfectly able to handle this revision.

>> But making the selected DT available to U-Boot scripts doesn't really
>> hurt or prevent us from doing it properly, so:
>>
>>> Signed-off-by: Samuel Holland <samuel@sholland.org>
>>
>> One nit below, with that:
>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>>
>> Cheers,
>> Andre
>>
>>> ---
>>>  board/sunxi/board.c | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/board/sunxi/board.c b/board/sunxi/board.c
>>> index eaa40a1ea96..5457b28e135 100644
>>> --- a/board/sunxi/board.c
>>> +++ b/board/sunxi/board.c
>>> @@ -870,6 +870,7 @@ static void setup_environment(const void *fdt)
>>>
>>>  int misc_init_r(void)
>>>  {
>>> +   const char *spl_dt_name;
>>>     uint boot;
>>>
>>>     env_set("fel_booted", NULL);
>>> @@ -888,6 +889,16 @@ int misc_init_r(void)
>>>             env_set("mmc_bootdev", "1");
>>>     }
>>>
>>> +   /* Set fdtfile to match the FIT configuration chosen in SPL. */
>>> +   spl_dt_name = get_spl_dt_name();
>>> +   if (spl_dt_name) {
>>> +           char *prefix = IS_ENABLED(CONFIG_ARM64) ? "allwinner/" : "";
>>> +           char str[64];
>>
>> The longest name (including the directory name) is 49 characters so far,
>> so let's hope that people don't go crazy with their DT names. Shall we
>> check the return value of snprintf() and at least complain? In contrast
>> to strncpy(), snprintf() is safe, but might still truncate the name.
>
> What kind of complaint were you thinking of? Trying to actually use a truncated
> $fdtfile would produce a "file not found" error. Maybe that would be obvious enough?

True, that should be good enough.

Cheers,
Andre

>>> +
>>> +           snprintf(str, sizeof(str), "%s%s.dtb", prefix, spl_dt_name);
>>> +           env_set("fdtfile", str);
>>> +   }
>>> +
>>>     setup_environment(gd->fdt_blob);
>>>
>>>  #ifdef CONFIG_USB_ETHER
>>>
>>
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.