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?
_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot
