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

[Stephen Warren <swarren@wwwdotorg.org>]

On 04/13/2016 12:17 PM, Andreas F?rber wrote:
> Am 13.04.2016 um 19:58 schrieb Stephen Warren:
>> On 04/13/2016 11:42 AM, Andreas F?rber wrote:
>>> Am 13.04.2016 um 19:00 schrieb Stephen Warren:
>>>> Anyway, nothing in your benefits-of-EFI statement implies that relying
>>>> on $fdtfile being set is correct. That's a new requirement that didn't
>>>> exist before. Either the requirement needs to be removed (e.g. using a
>>>> default FDT filename such as "${soc}-${board}${boardver}.dts") or only
>>>> enabling this functionality on boards that do set $fdtfile, since it
>>>> relies on that.
>>>
>>> $fdtfile needs to be the Linux filename. It does not always follow the
>>> same pattern as the U-Boot variables you suggest here.
>>> CONFIG_DEFAULT_DEVICE_TREE ".dtb" might work better, and that was my
>>> question to you.
>>
>> That pattern is a good default that at least historically applied to all
>> the systems where the distro bootcmds were enabled. Perhaps the set of
>> systems using the distro bootcmds has increased now so the default isn't
>> always applicable. Boards can set $fdtfile /if/ needed because of that,
>> but I don't think should be forced to in all cases where the default
>> makes sense.
>
> I really don't care whether we set fdtfile and use $fdtfile or whether
> we insert the filename string directly into the appropriate command
> variable... My point is U-Boot via its jetson-tk1_defconfig / .config
> knows this (or should know) better than any user.

Yes, U-Boot is a good place to put this information. I disagree that it 
/has/ to come from the defconfig/.config file.

 > And it seemed to me
> that variables were not exactly used sparingly in the distro mechanism
> so far, so I don't see why not to populate that variable _if_ we know
> what its value needs to be. Do you have any real reason for being
> against populating fdtfile at whatever level turns out to be suitable?

Yes, $fdtfile} can be auto-populated /at whatver level turns out to be 
suitable/. I think the only question is what "level" /is/ suitable.

> I believe there is no argument that this patch will not be applied.

There's a double negative there so I may not be interpreting you correctly.

I believe this patch should not be applied.

My objections are:

a) This only solves the problem for a single board, yet the issue it 
solves occurs on many/all boards. We need a solution that solves them all.

b) Boards should not need to define this value given that it can be 
calculated automatically.

> However I am strongly rejecting your attitude that everything is there
> already with variables and that nothing new is needed. Something needs
> to be done somewhere - and we need to figure out what exactly and where
> for minimum impact to the release.

Everything is there.

$soc/$board are set by include/env_default.h, with the option for 
per-board configuration files to override those values if the default 
CONFIG_SYS_* are incorrect for some reason. Differences between 
CONFIG_SYS_* and DTB filenames should not be an argument against the 
default I proposed. So, those values are always available (well, the 
board must enable CONFIG_ENV_VARS_UBOOT_CONFIG to get them; part of 
using distro_bootcmd is either enabling that config option or setting 
$fdtfile some other way).

I put effort into ensuring that "${soc}-${board}${boardver}.dtb" was the 
correct filename for Tegra boards, and I believe this holds true for a 
variety of non-Tegra boards as well.

The proposed default $fdtfile value I mentioned above should be the 
default, since everything required to construct it is already there. 
Boards that use distro_bootcmd should ensure that they set those 
variables correctly to allow that default to work; that's the whole 
point of those variables. Anyone enabling distro_bootcmd on other 
platforms hopefully ensured those variables were set correctly. If not, 
we can surely go back and fix those platforms.

Do you have a handle on exactly how many boards don't set those 
variables in a way that default won't work for them? Can't we fix their 
$soc/$board values so that the default does work?

I believe the calculation of the default/fallback value for $fdtfile 
does need to happen at runtime. ${boardver} is by definition a runtime 
variable since it reflects the HW version. As such, we can't simply put 
the following into e.g. tegra-common.h:

#define fdtfile "${soc}-${board}${boardver}.dtb"

(or any equivalent of that is actually valid syntax, i.e. using C 
pre-processor macros)

(Right now, I believe we don't actually set $boardver at runtime since 
for the board where it matters we only support one HW revision at all. 
However, I don't want to break that runtime capability. If we do, we 
should just override $board for that board, and delete all references to 
$boardver. I vaguely recall that some non-Tegra boards might use 
$boardver though?)