[U-Boot] [PATCH] jetson-tk1: Set fdtfile environment variable
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"
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"
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.
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
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.
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
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".
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
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.
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.
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.
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
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.
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
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?)
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
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?
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.
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
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.
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.p...
:-(
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.p...
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
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.p...
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.
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.p...
:-(
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
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.
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=ca5...
AMD Seattle was the first new one:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit?id=419...
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/arm... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mip...
Flat:
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arm... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/pow... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arc... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/c6x... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/cri... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/h83... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/met... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/mic... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/nio... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/ope... http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/xte...
Regards, Andreas
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.
participants (4)
-
Alexander Graf -
Andreas Färber -
Stephen Warren -
Tom Rini