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

[Andreas Färber]

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! 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.

> 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.

> 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.

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.

>> 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?

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? 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.

Regards,
Andreas

-- 
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)