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

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

[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@nvidia.com>
Cc: Alexander Graf <agraf@suse.de>
Signed-off-by: Andreas F?rber <afaerber@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" \
+	""
+
 #include "tegra-common-usb-gadget.h"
 #include "tegra-common-post.h"
 
-- 
2.6.6

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

[Andreas Färber]

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@nvidia.com>
> Cc: Alexander Graf <agraf@suse.de>
> Signed-off-by: Andreas F?rber <afaerber@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?

Andreas

> +
>  #include "tegra-common-usb-gadget.h"
>  #include "tegra-common-post.h"
>  
> 


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

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

[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@nvidia.com>
>> Cc: Alexander Graf <agraf@suse.de>
>> Signed-off-by: Andreas F?rber <afaerber@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.

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

[Alexander Graf]

On 04/13/2016 05:31 PM, Stephen Warren wrote:
> 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@nvidia.com>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Andreas F?rber <afaerber@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.

The point of all of the efi magic is that we can completely get rid of 
boot scripts. Boards use the distro scripts, everything else gets 
implicitly detected and executed. The way other boards deal with common 
code mapping into separate boards is to either implement a "findfdt" 
scriptlet or directly write the fdtfile variable (e.g. beaglebone) in 
board init (e.g. rpi).

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

s/un// :)

Just imagine a world where people don't have to worry about bootloaders 
anymore. Things would "just work". You plug in a usb stick, it comes up, 
boots Linux, everthing goes without anyone touching boot scripts, 
downloading board specific files, etc. You could get a random 
distribution from a common download page from somewhere and just run it.

Well, you can also just look at any random x86 system. They get at least 
that part pretty right these days.


Alex

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

[Stephen Warren]

On 04/13/2016 09:51 AM, Alexander Graf wrote:
> On 04/13/2016 05:31 PM, Stephen Warren wrote:
>> 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@nvidia.com>
>>>> Cc: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Andreas F?rber <afaerber@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.
>
> The point of all of the efi magic is that we can completely get rid of
> boot scripts. Boards use the distro scripts, everything else gets
> implicitly detected and executed. The way other boards deal with common
> code mapping into separate boards is to either implement a "findfdt"
> scriptlet or directly write the fdtfile variable (e.g. beaglebone) in
> board init (e.g. rpi).
>
>> 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.
>
> s/un// :)
>
> Just imagine a world where people don't have to worry about bootloaders
> anymore. Things would "just work". You plug in a usb stick, it comes up,
> boots Linux, everthing goes without anyone touching boot scripts,
> downloading board specific files, etc. You could get a random
> distribution from a common download page from somewhere and just run it.
>
> Well, you can also just look at any random x86 system. They get at least
> that part pretty right these days.

Well, you can also get the same benefit using extlinux.conf, and without 
relying on EFI:-P

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.

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

[Alexander Graf]

> Am 13.04.2016 um 19:00 schrieb Stephen Warren <swarren@wwwdotorg.org>:
> 
>> On 04/13/2016 09:51 AM, Alexander Graf wrote:
>>> On 04/13/2016 05:31 PM, Stephen Warren wrote:
>>>> 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@nvidia.com>
>>>>> Cc: Alexander Graf <agraf@suse.de>
>>>>> Signed-off-by: Andreas F?rber <afaerber@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.
>> 
>> The point of all of the efi magic is that we can completely get rid of
>> boot scripts. Boards use the distro scripts, everything else gets
>> implicitly detected and executed. The way other boards deal with common
>> code mapping into separate boards is to either implement a "findfdt"
>> scriptlet or directly write the fdtfile variable (e.g. beaglebone) in
>> board init (e.g. rpi).
>> 
>>> 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.
>> 
>> s/un// :)
>> 
>> Just imagine a world where people don't have to worry about bootloaders
>> anymore. Things would "just work". You plug in a usb stick, it comes up,
>> boots Linux, everthing goes without anyone touching boot scripts,
>> downloading board specific files, etc. You could get a random
>> distribution from a common download page from somewhere and just run it.
>> 
>> Well, you can also just look at any random x86 system. They get at least
>> that part pretty right these days.
> 
> Well, you can also get the same benefit using extlinux.conf, and without relying on EFI:-P
> 
> 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.

On boards that fon't set fdtfile we just don't load it, because we can't find the file. So you're getting a working grub2 payload, but Linux gets an empty device tree unless you pass it in using the grub2 "devicetree" command.

It's really just a convenience helper. And a nice piece to the puzzle that by convention allows users to think less about u-boot internals. The efi code works fine without.


Alex

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

[Andreas Färber]

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@nvidia.com>
>>> Cc: Alexander Graf <agraf@suse.de>
>>> Signed-off-by: Andreas F?rber <afaerber@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.

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

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.

Thanks,
Andreas

P.S. Without a standardized $fdtfile you can't have a standard boot.scr
either, so the generic mechanism becomes moot.

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

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

[Stephen Warren]

On 04/13/2016 11:21 AM, Alexander Graf wrote:
>
>
>> Am 13.04.2016 um 19:00 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>
>>> On 04/13/2016 09:51 AM, Alexander Graf wrote:
>>>> On 04/13/2016 05:31 PM, Stephen Warren wrote:
>>>>> 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@nvidia.com>
>>>>>> Cc: Alexander Graf <agraf@suse.de>
>>>>>> Signed-off-by: Andreas F?rber <afaerber@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.
>>>
>>> The point of all of the efi magic is that we can completely get rid of
>>> boot scripts. Boards use the distro scripts, everything else gets
>>> implicitly detected and executed. The way other boards deal with common
>>> code mapping into separate boards is to either implement a "findfdt"
>>> scriptlet or directly write the fdtfile variable (e.g. beaglebone) in
>>> board init (e.g. rpi).
>>>
>>>> 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.
>>>
>>> s/un// :)
>>>
>>> Just imagine a world where people don't have to worry about bootloaders
>>> anymore. Things would "just work". You plug in a usb stick, it comes up,
>>> boots Linux, everthing goes without anyone touching boot scripts,
>>> downloading board specific files, etc. You could get a random
>>> distribution from a common download page from somewhere and just run it.
>>>
>>> Well, you can also just look at any random x86 system. They get at least
>>> that part pretty right these days.
>>
>> Well, you can also get the same benefit using extlinux.conf, and without relying on EFI:-P
>>
>> 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.
>
> On boards that fon't set fdtfile we just don't load it, because we can't find the file. So you're getting a working grub2 payload, but Linux gets an empty device tree unless you pass it in using the grub2 "devicetree" command.
>
> It's really just a convenience helper. And a nice piece to the puzzle that by convention allows users to think less about u-boot internals. The efi code works fine without.

Except for the fact that it doesn't just work, since if it did there 
wouldn't be a need for the patch in this email thread.

I think the correct fix is to abandon this patch, and have the EFI code 
(i.e. the scripts in the distro boot commands most likely) calculate a 
default value for $fdtfile if it isn't already set. Something like:

if test -n "${fdtfile}"; then
     set _fdt ${fdtfile};
else
     set _fdt ${soc}-${board}${boardver}.dtb;

... and use ${_fdt} instead of relying on ${fdtfile}.

If you still want to pursue this patch, it should be enhanced to cover 
all boards where the EFI code is enabled and $fdtfile isn't already set 
somehow, which I expect is almost all U-Boot boards since the Kconfig 
option is "default y".

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

[Stephen Warren]

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@nvidia.com>
>>>> Cc: Alexander Graf <agraf@suse.de>
>>>> Signed-off-by: Andreas F?rber <afaerber@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

:-(

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

[Alexander Graf]

> Am 13.04.2016 um 19:31 schrieb Stephen Warren <swarren@wwwdotorg.org>:
> 
>> On 04/13/2016 11:21 AM, Alexander Graf wrote:
>> 
>> 
>>>> Am 13.04.2016 um 19:00 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>>> 
>>>>> On 04/13/2016 09:51 AM, Alexander Graf wrote:
>>>>>> On 04/13/2016 05:31 PM, Stephen Warren wrote:
>>>>>>> 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@nvidia.com>
>>>>>>> Cc: Alexander Graf <agraf@suse.de>
>>>>>>> Signed-off-by: Andreas F?rber <afaerber@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.
>>>> 
>>>> The point of all of the efi magic is that we can completely get rid of
>>>> boot scripts. Boards use the distro scripts, everything else gets
>>>> implicitly detected and executed. The way other boards deal with common
>>>> code mapping into separate boards is to either implement a "findfdt"
>>>> scriptlet or directly write the fdtfile variable (e.g. beaglebone) in
>>>> board init (e.g. rpi).
>>>> 
>>>>> 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.
>>>> 
>>>> s/un// :)
>>>> 
>>>> Just imagine a world where people don't have to worry about bootloaders
>>>> anymore. Things would "just work". You plug in a usb stick, it comes up,
>>>> boots Linux, everthing goes without anyone touching boot scripts,
>>>> downloading board specific files, etc. You could get a random
>>>> distribution from a common download page from somewhere and just run it.
>>>> 
>>>> Well, you can also just look at any random x86 system. They get at least
>>>> that part pretty right these days.
>>> 
>>> Well, you can also get the same benefit using extlinux.conf, and without relying on EFI:-P
>>> 
>>> 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.
>> 
>> On boards that fon't set fdtfile we just don't load it, because we can't find the file. So you're getting a working grub2 payload, but Linux gets an empty device tree unless you pass it in using the grub2 "devicetree" command.
>> 
>> It's really just a convenience helper. And a nice piece to the puzzle that by convention allows users to think less about u-boot internals. The efi code works fine without.
> 
> Except for the fact that it doesn't just work, since if it did there wouldn't be a need for the patch in this email thread.
> 
> I think the correct fix is to abandon this patch, and have the EFI code (i.e. the scripts in the distro boot commands most likely) calculate a default value for $fdtfile if it isn't already set. Something like:
> 
> if test -n "${fdtfile}"; then
>    set _fdt ${fdtfile};
> else
>    set _fdt ${soc}-${board}${boardver}.dtb;
> 
> ... and use ${_fdt} instead of relying on ${fdtfile}.
> 
> If you still want to pursue this patch, it should be enhanced to cover all boards where the EFI code is enabled and $fdtfile isn't already set somehow, which I expect is almost all U-Boot boards since the Kconfig option is "default y".

Only the ones that do distro bootcmd. I'll send a patch later. I really like the idea :).

Thanks!

Alex

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

[Andreas Färber]

Am 13.04.2016 um 19:00 schrieb Stephen Warren:
> On 04/13/2016 09:51 AM, Alexander Graf wrote:
>> On 04/13/2016 05:31 PM, Stephen Warren wrote:
>>> 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@nvidia.com>
>>>>> Cc: Alexander Graf <agraf@suse.de>
>>>>> Signed-off-by: Andreas F?rber <afaerber@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.
>>
>> The point of all of the efi magic is that we can completely get rid of
>> boot scripts. Boards use the distro scripts, everything else gets
>> implicitly detected and executed. The way other boards deal with common
>> code mapping into separate boards is to either implement a "findfdt"
>> scriptlet or directly write the fdtfile variable (e.g. beaglebone) in
>> board init (e.g. rpi).
>>
>>> 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.
>>
>> s/un// :)
>>
>> Just imagine a world where people don't have to worry about bootloaders
>> anymore. Things would "just work". You plug in a usb stick, it comes up,
>> boots Linux, everthing goes without anyone touching boot scripts,
>> downloading board specific files, etc. You could get a random
>> distribution from a common download page from somewhere and just run it.
>>
>> Well, you can also just look at any random x86 system. They get at least
>> that part pretty right these days.
> 
> Well, you can also get the same benefit using extlinux.conf, and without
> relying on EFI:-P

You're late for that discussion, we had that months ago on this mailing
list. We already concluded that SUSE does not and will not generate
extlinux.conf; EFI is a boot mechanism that we already support from x86
and aarch64 and that there are tools for (e.g., grub-mkconfig), unlike
extlinux.conf. There was also a FOSDEM talk on extlinux.conf that can be
summarized as some people like it and there's nothing wrong with it but
it's not a one-size-fits-all solution for everyone, including non-Linux
OSes such as Haiku.

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

It's part of the generic mechanism, so not just select boards. Yet I was
told that all boards are expected to set their cacheline size (although
that is not a board but CPU property), so similarly we can (yes, newly)
desire all boards to provide DT related settings as well.

If you would supply a feature-complete DT in the first place, we
wouldn't need $fdtfile here, but it seemed that that was not realistic
to expect for the upcoming U-Boot release.

Regards,
Andreas

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

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

[Alexander Graf]

> Am 13.04.2016 um 19:40 schrieb Stephen Warren <swarren@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@nvidia.com>
>>>>> Cc: Alexander Graf <agraf@suse.de>
>>>>> Signed-off-by: Andreas F?rber <afaerber@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.

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.


Alex

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

[Stephen Warren]

On 04/13/2016 11:42 AM, Andreas F?rber wrote:
> Am 13.04.2016 um 19:00 schrieb Stephen Warren:
>> On 04/13/2016 09:51 AM, Alexander Graf wrote:
>>> On 04/13/2016 05:31 PM, Stephen Warren wrote:
>>>> 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@nvidia.com>
>>>>>> Cc: Alexander Graf <agraf@suse.de>
>>>>>> Signed-off-by: Andreas F?rber <afaerber@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.
>>>
>>> The point of all of the efi magic is that we can completely get rid of
>>> boot scripts. Boards use the distro scripts, everything else gets
>>> implicitly detected and executed. The way other boards deal with common
>>> code mapping into separate boards is to either implement a "findfdt"
>>> scriptlet or directly write the fdtfile variable (e.g. beaglebone) in
>>> board init (e.g. rpi).
>>>
>>>> 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.
>>>
>>> s/un// :)
>>>
>>> Just imagine a world where people don't have to worry about bootloaders
>>> anymore. Things would "just work". You plug in a usb stick, it comes up,
>>> boots Linux, everthing goes without anyone touching boot scripts,
>>> downloading board specific files, etc. You could get a random
>>> distribution from a common download page from somewhere and just run it.
>>>
>>> Well, you can also just look at any random x86 system. They get at least
>>> that part pretty right these days.
>>
>> Well, you can also get the same benefit using extlinux.conf, and without
>> relying on EFI:-P
>
> You're late for that discussion, we had that months ago on this mailing
> list. We already concluded that SUSE does not and will not generate
> extlinux.conf; EFI is a boot mechanism that we already support from x86
> and aarch64 and that there are tools for (e.g., grub-mkconfig), unlike
> extlinux.conf. There was also a FOSDEM talk on extlinux.conf that can be
> summarized as some people like it and there's nothing wrong with it but
> it's not a one-size-fits-all solution for everyone, including non-Linux
> OSes such as Haiku.
>
>> 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.

> It's part of the generic mechanism, so not just select boards. Yet I was
> told that all boards are expected to set their cacheline size (although
> that is not a board but CPU property), so similarly we can (yes, newly)
> desire all boards to provide DT related settings as well.

OK, but enabling the feature on boards where we know the requirements 
aren't met doesn't seem useful, since it won't work well, as evidenced 
by this patch.

> If you would supply a feature-complete DT in the first place, we
> wouldn't need $fdtfile here, but it seemed that that was not realistic
> to expect for the upcoming U-Boot release.

Given the current primary DT source location, I don't think the issue is 
complete-vs-incomplete DTs at all. However, that's straying quite off-topic.

[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@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@nvidia.com>
>>>>>> Cc: Alexander Graf <agraf@suse.de>
>>>>>> Signed-off-by: Andreas F?rber <afaerber@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.

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

[Andreas Färber]

Am 13.04.2016 um 19:40 schrieb Stephen Warren:
> 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@nvidia.com>
>>>>> Cc: Alexander Graf <agraf@suse.de>
>>>>> Signed-off-by: Andreas F?rber <afaerber@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.

Alex was arguing about benefits, not me. I am specifically objecting to
you telling me that the solution for a generic boot mechanism were for
the user to supply custom board information. That has nothing to do with
EFI as I underlined by referring to boot.scr as well.

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

That does not help me as developer since the environment is getting
updated with several of the EFI patches I'm testing. How would I know
when to do that? And as soon as I do reset the environment to default
it'll be gone again.

Not all boards actually have saveenv anyway.

>> 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
> 
> 
> :-(

See my other reply with another suggestion to be used at distro header
level.

But my point was that U-Boot != Linux filename in some cases. For
example, the dragonboard410c is using dragonboard410c.dts but in Linux
it's qcom/apq8016-sbc.dts. To make things worse, for arm64 they're in
vendor subdirectories, for arm they're flat. It's not as easy as you
make it sound.

Regards,
Andreas

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

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

[Andreas Färber]

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

I believe there is no argument that this patch will not be applied.
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.

Thanks,
Andreas

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

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

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

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

[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)

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

[Tom Rini]

On Wed, Apr 13, 2016 at 09:31:43AM -0600, Stephen Warren wrote:
> 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@nvidia.com>
> >>Cc: Alexander Graf <agraf@suse.de>
> >>Signed-off-by: Andreas F?rber <afaerber@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.

Bah.  But the good news is we haven't done a release with EFI stuff yet,
so we can fix it.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160413/4f303447/attachment-0001.sig>

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

[Tom Rini]

On Wed, Apr 13, 2016 at 08:02:24PM +0200, Andreas F?rber wrote:
[snip]
> But my point was that U-Boot != Linux filename in some cases. For
> example, the dragonboard410c is using dragonboard410c.dts but in Linux
> it's qcom/apq8016-sbc.dts. To make things worse, for arm64 they're in
> vendor subdirectories, for arm they're flat. It's not as easy as you
> make it sound.

Ug, really?  Did someone mention that on the devicetree list or did it
just sneak in?  I know you're just the messenger here but having arm64
be different from ARM and PowerPC (and MIPS?  Or is MIPS doing the
vendor subdirectory too) seems like a bad idea...

That said, in U-Boot it should be apq8016-sbc.dts at least.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160413/dfb75bc1/attachment.sig>

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

[Tom Rini]

On Wed, Apr 13, 2016 at 07:21:27PM +0200, Alexander Graf wrote:
> 
> 
> > Am 13.04.2016 um 19:00 schrieb Stephen Warren <swarren@wwwdotorg.org>:
> > 
> >> On 04/13/2016 09:51 AM, Alexander Graf wrote:
> >>> On 04/13/2016 05:31 PM, Stephen Warren wrote:
> >>>> 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@nvidia.com>
> >>>>> Cc: Alexander Graf <agraf@suse.de>
> >>>>> Signed-off-by: Andreas F?rber <afaerber@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.
> >> 
> >> The point of all of the efi magic is that we can completely get rid of
> >> boot scripts. Boards use the distro scripts, everything else gets
> >> implicitly detected and executed. The way other boards deal with common
> >> code mapping into separate boards is to either implement a "findfdt"
> >> scriptlet or directly write the fdtfile variable (e.g. beaglebone) in
> >> board init (e.g. rpi).
> >> 
> >>> 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.
> >> 
> >> s/un// :)
> >> 
> >> Just imagine a world where people don't have to worry about bootloaders
> >> anymore. Things would "just work". You plug in a usb stick, it comes up,
> >> boots Linux, everthing goes without anyone touching boot scripts,
> >> downloading board specific files, etc. You could get a random
> >> distribution from a common download page from somewhere and just run it.
> >> 
> >> Well, you can also just look at any random x86 system. They get at least
> >> that part pretty right these days.
> > 
> > Well, you can also get the same benefit using extlinux.conf, and without relying on EFI:-P
> > 
> > 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.
> 
> On boards that fon't set fdtfile we just don't load it, because we can't find the file. So you're getting a working grub2 payload, but Linux gets an empty device tree unless you pass it in using the grub2 "devicetree" command.

Well, hold up.  We are _not_ at the point right now where the device
tree we use is the kernel device tree.  We're trying to make sure there
are no cases of incompatible bindings, and we're trying to make sure we
play nicely and get the canonical device tree to have everything we
need.  But it seems risky at best to assume the loaded tree is the right
tree for the kernel.

> It's really just a convenience helper. And a nice piece to the puzzle
> that by convention allows users to think less about u-boot internals.
> The efi code works fine without.

Yes, but it's also something that requires some external logic.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160413/583ba864/attachment.sig>

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

[Tom Rini]

On Wed, Apr 13, 2016 at 07:42:11PM +0200, Andreas F?rber wrote:
[snip]
> $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.
> 
> It's part of the generic mechanism, so not just select boards. Yet I was
> told that all boards are expected to set their cacheline size (although
> that is not a board but CPU property), so similarly we can (yes, newly)
> desire all boards to provide DT related settings as well.
> 
> If you would supply a feature-complete DT in the first place, we
> wouldn't need $fdtfile here, but it seemed that that was not realistic
> to expect for the upcoming U-Boot release.

So here's the thing.  Figuring out what the device tree to load is, and
where it's going to reside is a sucky problem.  For most of the complex
cases we do this today with "run findfdt".  Why?  Well, check out the
implementations in "git grep -l findfdt=" right now.  It sounds like we
need to figure out how to get EFI in line with everything else that
U-Boot does/supports rather than to re-invent the wheel here.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160413/c17f3972/attachment.sig>

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

[Alexander Graf]

On 14.04.16 00:17, Tom Rini wrote:
> On Wed, Apr 13, 2016 at 07:21:27PM +0200, Alexander Graf wrote:
>>
>>
>>> Am 13.04.2016 um 19:00 schrieb Stephen Warren <swarren@wwwdotorg.org>:
>>>
>>>> On 04/13/2016 09:51 AM, Alexander Graf wrote:
>>>>> On 04/13/2016 05:31 PM, Stephen Warren wrote:
>>>>>> 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@nvidia.com>
>>>>>>> Cc: Alexander Graf <agraf@suse.de>
>>>>>>> Signed-off-by: Andreas F?rber <afaerber@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.
>>>>
>>>> The point of all of the efi magic is that we can completely get rid of
>>>> boot scripts. Boards use the distro scripts, everything else gets
>>>> implicitly detected and executed. The way other boards deal with common
>>>> code mapping into separate boards is to either implement a "findfdt"
>>>> scriptlet or directly write the fdtfile variable (e.g. beaglebone) in
>>>> board init (e.g. rpi).
>>>>
>>>>> 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.
>>>>
>>>> s/un// :)
>>>>
>>>> Just imagine a world where people don't have to worry about bootloaders
>>>> anymore. Things would "just work". You plug in a usb stick, it comes up,
>>>> boots Linux, everthing goes without anyone touching boot scripts,
>>>> downloading board specific files, etc. You could get a random
>>>> distribution from a common download page from somewhere and just run it.
>>>>
>>>> Well, you can also just look at any random x86 system. They get at least
>>>> that part pretty right these days.
>>>
>>> Well, you can also get the same benefit using extlinux.conf, and without relying on EFI:-P
>>>
>>> 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.
>>
>> On boards that fon't set fdtfile we just don't load it, because we can't find the file. So you're getting a working grub2 payload, but Linux gets an empty device tree unless you pass it in using the grub2 "devicetree" command.
> 
> Well, hold up.  We are _not_ at the point right now where the device
> tree we use is the kernel device tree.  We're trying to make sure there

If I understand correctly that depends on the board. On some systems
like the pine64 the device tree is just a copy of the Linux device tree.
I don't see why we couldn't aim for that model going forward.

And with that I'm not saying "this is how it works today". If you want a
reasonable working system today you probably need to provide your own
device tree at a well-known location. But thinking mid-term I don't see
why a board author couldn't just keep the U-Boot device tree and the
Linux device tree in sync.

Again, none of this is mandatory, required, or even recommended today. I
just want to make sure that people see the potential and make the (in my
opinion) best path be the easiest one to take :).

> are no cases of incompatible bindings, and we're trying to make sure we
> play nicely and get the canonical device tree to have everything we
> need.  But it seems risky at best to assume the loaded tree is the right
> tree for the kernel.

So would you prefer if the board manually specifies it as the right tree
for the kernel? I'm not sure that buys us anything really. If it's the
wrong one, booting will fail.


Alex

> 
>> It's really just a convenience helper. And a nice piece to the puzzle
>> that by convention allows users to think less about u-boot internals.
>> The efi code works fine without.
> 
> Yes, but it's also something that requires some external logic.
> 

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

[Tom Rini]

On Thu, Apr 14, 2016 at 12:38:22AM +0200, Alexander Graf wrote:
> 
> 
> On 14.04.16 00:17, Tom Rini wrote:
> > On Wed, Apr 13, 2016 at 07:21:27PM +0200, Alexander Graf wrote:
> >>
> >>
> >>> Am 13.04.2016 um 19:00 schrieb Stephen Warren <swarren@wwwdotorg.org>:
> >>>
> >>>> On 04/13/2016 09:51 AM, Alexander Graf wrote:
> >>>>> On 04/13/2016 05:31 PM, Stephen Warren wrote:
> >>>>>> 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@nvidia.com>
> >>>>>>> Cc: Alexander Graf <agraf@suse.de>
> >>>>>>> Signed-off-by: Andreas F?rber <afaerber@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.
> >>>>
> >>>> The point of all of the efi magic is that we can completely get rid of
> >>>> boot scripts. Boards use the distro scripts, everything else gets
> >>>> implicitly detected and executed. The way other boards deal with common
> >>>> code mapping into separate boards is to either implement a "findfdt"
> >>>> scriptlet or directly write the fdtfile variable (e.g. beaglebone) in
> >>>> board init (e.g. rpi).
> >>>>
> >>>>> 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.
> >>>>
> >>>> s/un// :)
> >>>>
> >>>> Just imagine a world where people don't have to worry about bootloaders
> >>>> anymore. Things would "just work". You plug in a usb stick, it comes up,
> >>>> boots Linux, everthing goes without anyone touching boot scripts,
> >>>> downloading board specific files, etc. You could get a random
> >>>> distribution from a common download page from somewhere and just run it.
> >>>>
> >>>> Well, you can also just look at any random x86 system. They get at least
> >>>> that part pretty right these days.
> >>>
> >>> Well, you can also get the same benefit using extlinux.conf, and without relying on EFI:-P
> >>>
> >>> 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.
> >>
> >> On boards that fon't set fdtfile we just don't load it, because we can't find the file. So you're getting a working grub2 payload, but Linux gets an empty device tree unless you pass it in using the grub2 "devicetree" command.
> > 
> > Well, hold up.  We are _not_ at the point right now where the device
> > tree we use is the kernel device tree.  We're trying to make sure there
> 
> If I understand correctly that depends on the board. On some systems
> like the pine64 the device tree is just a copy of the Linux device tree.
> I don't see why we couldn't aim for that model going forward.
> 
> And with that I'm not saying "this is how it works today". If you want a
> reasonable working system today you probably need to provide your own
> device tree at a well-known location. But thinking mid-term I don't see
> why a board author couldn't just keep the U-Boot device tree and the
> Linux device tree in sync.
> 
> Again, none of this is mandatory, required, or even recommended today. I
> just want to make sure that people see the potential and make the (in my
> opinion) best path be the easiest one to take :).

Yes, the long term goal is that everyone be able to share the same
canonical device tree, bootloader and OS.  And that's making progress
all around and I think devicetree.org will help there too.  But the
problem I want to make sure you see is that you can't just use the
kernel device tree on U-Boot and have it work on anything that does
DM+SPL.  The u-boot,dm-pre-reloc flag is required, isn't going to be in
the kernel tree and is still something that needs sorting out.

> > are no cases of incompatible bindings, and we're trying to make sure we
> > play nicely and get the canonical device tree to have everything we
> > need.  But it seems risky at best to assume the loaded tree is the right
> > tree for the kernel.
> 
> So would you prefer if the board manually specifies it as the right tree
> for the kernel? I'm not sure that buys us anything really. If it's the
> wrong one, booting will fail.

I'm saying I think today unless you know you've loaded a known correct
for the kernel device tree into $fdtaddr you shouldn't use that for the
kernel.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160413/1d32263f/attachment.sig>

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

[Andreas Färber]

Am 14.04.2016 um 00:05 schrieb Tom Rini:
> On Wed, Apr 13, 2016 at 08:02:24PM +0200, Andreas F?rber wrote:
>> my point was that U-Boot != Linux filename in some cases. For
>> example, the dragonboard410c is using dragonboard410c.dts but in Linux
>> it's qcom/apq8016-sbc.dts. To make things worse, for arm64 they're in
>> vendor subdirectories, for arm they're flat. It's not as easy as you
>> make it sound.
> 
> Ug, really?  Did someone mention that on the devicetree list or did it
> just sneak in?

Don't have the discussions handy, but I recall it was a conscious
decision to refactor arm64:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=ca5b34100c571658e605c5554aac374649593327

AMD Seattle was the first new one:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=419043609689d0aba9f727a6faf1ff406e269ecf

>  I know you're just the messenger here but having arm64
> be different from ARM and PowerPC (and MIPS?  Or is MIPS doing the
> vendor subdirectory too) seems like a bad idea...
[snip]

Quick arch/ survey:

Vendor subdirs:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mips/boot/dts

Flat:

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/powerpc/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arc/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/c6x/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/cris/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/h8300/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/metag/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/microblaze/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/nios2/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/openrisc/boot/dts
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/xtensa/boot/dts

Regards,
Andreas

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160414/9aa458eb/attachment.sig>

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

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

[Alexander Graf]

On 14.04.16 00:29, Tom Rini wrote:
> On Wed, Apr 13, 2016 at 07:42:11PM +0200, Andreas F?rber wrote:
> [snip]
>> $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.
>>
>> It's part of the generic mechanism, so not just select boards. Yet I was
>> told that all boards are expected to set their cacheline size (although
>> that is not a board but CPU property), so similarly we can (yes, newly)
>> desire all boards to provide DT related settings as well.
>>
>> If you would supply a feature-complete DT in the first place, we
>> wouldn't need $fdtfile here, but it seemed that that was not realistic
>> to expect for the upcoming U-Boot release.
> 
> So here's the thing.  Figuring out what the device tree to load is, and
> where it's going to reside is a sucky problem.  For most of the complex
> cases we do this today with "run findfdt".  Why?  Well, check out the
> implementations in "git grep -l findfdt=" right now.  It sounds like we
> need to figure out how to get EFI in line with everything else that
> U-Boot does/supports rather than to re-invent the wheel here.

Sure, I fully agree. Where exactly do you see the EFI bits
reimplementing the wheel here? We use the same logic as the rest of
U-Boot for this. Findfdt gets called before the distro boot command, so
we use that. If a board sets fdtfile, we use that. If it doesn't, with
v2 we fall back to the same logic as pxe boot for fdtfile naming fallbacks.

So in the end, I'd say it's pretty much in line :).


Alex