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.
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.
> 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 <sam...@sholland.org>
>
> One nit below, with that:
> Reviewed-by: Andre Przywara <andre.przyw...@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?
Cheers,
Samuel
> 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
>>
>
