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

[Stephen Warren]

On 04/13/2016 02:27 PM, Andreas Färber wrote:
> Am 13.04.2016 um 20:51 schrieb Stephen Warren:
>> 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.
>
> You're twisting my words around!

That wasn't intentional. You explicitly mentioned those two files, so I 
responded to that aspect of your statement.

> I'm saying when I build U-Boot with
> jetson-tk1_defconfig then I'm intentionally building it for the Jetson
> TK1 board and not some random other board. In this case that uniquely
> identifies the .dtb file via an entry in .config. It may or may not
> choose to read EEPROMs or whatever heuristics it knows, but it will know
> that it's not a BeagleBone Black and it doesn't need a user i.e.
> boot.scr to be told that it's a Jetson TK1.
>
>>> 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.
>
> There's nothing to misinterpret here: this patch will not be applied.

(Note: This part of the response is intended to explain the 
communication issue; I'm not trying to be a nit-picking asshole, even 
though it might look like it)

Unfortunately, "I believe there is no argument that" can mean either 
"the following is true" or "the following is false". "No argument" 
simply means there's no argument; it doesn't imply anything much about 
the Boolean value of the rest of the sentence.

>> I believe this patch should not be applied.
>
> Agreed.
>
>> 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.
>
> Partially agree. I doubt we can solve everything at once, as indicated.

If we did solve this problem by editing config.h files to just set 
fdtfile in the default environment, it should be pretty easy to find all 
the config.h files that enable the distro boot commands, work out the 
value fdtfile should have, and add those all in one patch or a series of 
patches. Or, disable EFI+distro_bootcmd on those patches until someone 
more familiar with those platforms can send a patch to set fdtfile 
correctly there.

>> b) Boards should not need to define this value given that it can be
>> calculated automatically.
>
> Agree on the board part. However it seemed that you were generally
> against defining fdtfile at whatever level. Misunderstanding?
>
> I feel it is much easier if U-Boot does your
>
> fdtfile=${soc}-${board}${boardrev}.dtb
>
> than expecting a) boot.scr scripts from users and b) bootefi internal
> implementation for some $_fdt to both recreate the same thing.

I'd prefer any new code to follow the same algorithms as existing code 
for obtaining $fdtfile values. pxe.c already has code that generates a 
default if fdtfile is missing. That logic could be re-used elsewhere.

> What I am unclear about (not being too familiar with U-Boot's codebase)
> is how the generic level would know that it should not define fdtfile
> according to the generic scheme because it's already getting defined in
> some other way. Because in the other cases fdtfile= was just a line
> inmidst some EXTRA_ENV_VARS or so define, not a define that I could
> #ifndef on.

For code at run-time, we could simply assume that if fdtfile is set, 
that value is used. Otherwise, there is no choice but to attempt some 
default.

At compile time, some SoC-/board-specific header can set a #define for a 
custom value, and some core header can #define some default if the 
define is not already defined.

>>> 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.
>
> So the counter-example I already mentioned is dragonboard410c. It sets
> soc=snapdragon, board=dragonboard410c and no boardrev=, in your scheme
> that gives snapdragon-dragonboard410c which neither matches the internal
> .dts nor that in Linux that users might supply. I have no clue whether I
> can just change soc somewhere in the code to apc8016 and board to sbc
> (which I find idiotic in light of Inforce6309, but again that's
> off-topic) or whether U-Boot will then explode. CC'ing Mateusz.
>
> I assume it will not be the only exception to the rule if I find it so
> easily. :(
>
>> 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?
>
> No, I do not know how any fdtfile grep across the codebase would relate
> back to the defconfigs (and thereby relevant architectures).
>
>> 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?)
>
> I actually did mean it literally like you have it above:
>
> #ifndef FDTFILE
> #define FDTFILE "fdtfile=${soc}-${board}${boardrev}.dtb\0"
> #endif
>
> So I would expect this to at runtime fill in the dynamic ${boardrev} if
> set and any dynamic-or-static ${board} and ${soc}. Am I wrong?

I don't /believe/ that U-Boot expands variable references in the default 
environment values. However, I also don't think I've ever tried it, so 
could well be wrong.

> The problem with that is, like I said, that boards that do set fdtfile=
> statically don't set such a singular FDTFILE we could detect. Is it
> valid to define a variable twice in the default env?

I don't know. I've seen this handled for other variables by the 
board-specific config.h setting a pre-processor define, some core header 
possibly defining a default value for that define if it's not already 
set, and then using that define to set the default environment. This 
avoids putting two values for a single variable into the default 
environment.

> We could then try
> to order the generic definition first (or vice versa) so that the more
> specific one gets picked up. Otherwise we would need to refactor all
> fdtfile= setting boards, and a quick "git grep fdtfile=" finds 91
> occurrences before my patch. Six of which use CONFIG_FDTFILE, one
> "name/of/device-tree.dtb", five "your.fdt.dtb", one CONFIG_BOARDNAME,
> one CONFIG_HOSTNAME, six "undefined", five "/home/user/board.dtb" and
> other creative ideas... sunxi uses CONFIG_DEFAULT_DEVICE_TREE ".dtb".
>
> Alex seems to have a new idea where to put it that I'll test later.

Anyway, I think Alex's patch to config_distro_bootcmd.h should have 
solved this issue for now at least?