[U-Boot] [PATCH] jetson-tk1: Set fdtfile environment variable

[Stephen Warren]

On 04/13/2016 11:50 AM, Alexander Graf wrote:
>
>
>> Am 13.04.2016 um 19:40 schrieb Stephen Warren <swarren at wwwdotorg.org>:
>>
>>> On 04/13/2016 11:22 AM, Andreas Färber wrote:
>>>> Am 13.04.2016 um 17:31 schrieb Stephen Warren:
>>>>> On 04/13/2016 06:55 AM, Andreas Färber wrote:
>>>>>> Am 13.04.2016 um 14:48 schrieb Andreas Färber:
>>>>>> The 4.5.0 kernel cannot cope with U-Boot's internal device tree, and the
>>>>>> distro boot commands are looking for $fdtfile, so provide it to avoid
>>>>>> having users supply a dumb boot.scr doing a setenv fdtfile ...; boot,
>>>>>> defeating the purpose of generic EFI boot.
>>>>>>
>>>>>> Cc: Stephen Warren <swarren at nvidia.com>
>>>>>> Cc: Alexander Graf <agraf at suse.de>
>>>>>> Signed-off-by: Andreas Färber <afaerber at suse.de>
>>>>>> ---
>>>>>>    include/configs/jetson-tk1.h | 4 ++++
>>>>>>    1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/include/configs/jetson-tk1.h b/include/configs/jetson-tk1.h
>>>>>> index 59dbb20..82a4be4 100644
>>>>>> --- a/include/configs/jetson-tk1.h
>>>>>> +++ b/include/configs/jetson-tk1.h
>>>>>> @@ -63,6 +63,10 @@
>>>>>>    /* General networking support */
>>>>>>    #define CONFIG_CMD_DHCP
>>>>>>
>>>>>> +#define BOARD_EXTRA_ENV_SETTINGS \
>>>>>> +    "fdtfile=tegra124-jetson-tk1.dtb\0" \
>>>>>> +    ""
>>>>>
>>>>> Is there any more intelligent solution than doing this for each board?
>>>>
>>>> Yes, the distro boot scripts shouldn't be using $fdtfile unconditionally
>>>> since it's not guaranteed to be set. The model is that boot scripts
>>>> determine the FDT filename, and $fdtfile is an optional override.
>>>>
>>>> It looks like the hard-coded use of $fdtfile was added into the EFI
>>>> path, which I didn't get to review, and which shouldn't be enabled by
>>>> default but unfortunately is.
>>>
>>> As Alex described, you're entirely missing the point here.
>>
>> Well, that's because the point you're making is re: the benefits of EFI, but that's not the point the patch is addressing nor the point I'm objecting to. The patch addresses the need for all boards to set $fdtfile. That is what I'm arguing about. The benefits of EFI aren't relevant to this discussion.
>>
>>> The EFI bootloader is an alternative to a board-specific script, not an
>>> addition. The loading logic is all in the U-Boot environment and it
>>> needs to know what device tree to use without the user telling it:
>>>
>>> a) master branch searches for $fdtfile in various prefixes on the
>>> current boot device partition.
>>>
>>> a') We're testing a variation where we load $fdtfile from a different
>>> partition on the same device (i.e., from ext4/btrfs rather that fat).
>>>
>>> b) A pending patch exposes the internal U-Boot device tree.
>>>
>>> The former is what we need to boot today. For openSUSE we get the .dtb
>>> files from rpm packages built from the kernel.
>>>
>>> The latter would match the Tianocore/Aptio model where all board info
>>> comes from the firmware exclusively. As reported elsewhere it does not
>>> yet work on this board with your DT though; you yourselves discussed
>>> about differing cell sizes and node names.
>>>
>>> Now during my EFI testing I had to repeatedly remember to interrupt
>>> U-Boot and type:
>>>
>>> setenv fdtfile tegra124-jetson-tk1.dtb
>>
>> You can always run "saveenv" here. Admittedly it's not a nice user-experience to have to do that, but it's a workaround that would work today.
>>
>>> boot
>>>
>>> until I got so annoyed that I figured out this patch to make it permanent.
>>>
>>> The hikey and some other boards got their variable renamed to fit
>>> standardized $fdtfile, for dragonboard410c I sent a similar patch.
>>>
>>> My above question was more precisely: Can we automate supplying the
>>> $fdtfile at tegra124-common.h or tegra-common.h level instead of as in
>>> this patch manually at jetson-tk1.h level where I happened to notice?
>>
>> As I mentioned in my other reply, it would be better if the EFI logic handled this, rather than requiring every board to solve the problem over and over.
>>
>>> The Raspberry Pi has been supplying $fdtfile just fine (modulo the rev
>>> B), so I don't understand why you'd be against it now.
>>
>> I have no objection to boards setting $fdtfile where they need to. Some U-Boot boards support multiple HW, so it's a base requirement that the code supply $fdtfile since there's no other way to know what the correct value is. Other boards only operate on a single piece of HW, and we shouldn't burden every config header (or other board-specific code/...) with defining this value since there's a reasonable default that core code could use. Rather, let's deal with it in some core code (not per-SoC, but U-Boot-wide), for example using the code snippet I posted in my other response.
>>
>>> Thanks,
>>> Andreas
>>>
>>> P.S. Without a standardized $fdtfile you can't have a standard boot.scr
>>> either, so the generic mechanism becomes moot.
>>
>> That's not true. the model there is to use ${soc} ${board} and ${boardver} to construct it. I thought that was documented in doc/README.distro, but perhaps I only mentioned it in commit descriptions and scripts that build boot.scr, e.g.:
>>
>> https://github.com/NVIDIA/tegra-uboot-scripts/blob/master/gen-uboot-script.py#L133
>
> So do all systems follow this scheme? We need to make sure that fdtfile ends up with the same value as what arch/arm{,64}/boot/dtb build and install.

Well, it's up to whoever generates boot.scr. It certainly should be 
possible to use that generic scheme. However, it's also possible that 
people hard-code DTB filenames in their boot.scr, use some other 
variable, etc.

> If not we can probably add a define for the distro uboot variables to set fdtfile according to your scheme for boards where it gets used, so tegra for example would only need to define that define :). Or we could leave it as fallback for distro boot as you suggested.

I think placing the code snippet I gave into config_distro_bootcmd.h 
makes sense. It would allow any board to set $fdtfile in the default 
environment or in code at boot time, or allow the user to override the 
value, yet still allow fallback to the default scheme.