Message ID | 20181224131904.20891-1-kostap@marvell.com |
---|---|
State | Changes Requested |
Headers | show |
Series | boot/uboot: add support for custom DT name | expand |
Hello Kostya, +Clemens Gruber in Cc, who posted a related U-Boot patch. On Mon, 24 Dec 2018 15:19:04 +0200, kostap@marvell.com wrote: > From: Konstantin Porotchkin <kostap@marvell.com> > > Some u-boot default configuration files could be shared between > targets and used for building images for multiple board types. > The only difference between such builds is the DTB embedded in > in the boot image for each specific platform. > This approach is widely used by Marvell, having the same u-boot > configuration file for the entire SoC family, but allowing builds > of multiple target flavors by supplying the device tree name through > make command parameter DEVICE_TREE=xxx > This patch adds such capability to uboot module of the buildroot. > The custome DT name could be defined by > BR2_TARGET_UBOOT_CUSTOM_DTS_NAME entry. > > Change-Id: I69e193339b0369a736bdf98491b9914d24a54e17 > Signed-off-by: Konstantin Porotchkin <kostap@marvell.com> Reading this again, in fact it is very similar to patch http://patchwork.ozlabs.org/patch/881197/ we already have in the queue. With a very important distinction: your patch passes DEVICE_TREE=, while the existing pending patch passes EXT_DTB=. The obvious question that comes up is: which one is right ? Or are these two orthogonal things ? Thomas
Hi, Thomas, > -----Original Message----- > From: buildroot <buildroot-bounces@busybox.net> On Behalf Of Thomas > Petazzoni > Sent: Monday, December 24, 2018 15:48 > To: Kostya Porotchkin <kostap@marvell.com> > Cc: Clemens Gruber <clemens.gruber@pqgruber.com>; > buildroot@buildroot.org > Subject: [EXT] Re: [Buildroot] [PATCH] boot/uboot: add support for custom > DT name > > External Email > > ---------------------------------------------------------------------- > Hello Kostya, > > +Clemens Gruber in Cc, who posted a related U-Boot patch. > > On Mon, 24 Dec 2018 15:19:04 +0200, kostap@marvell.com wrote: > > From: Konstantin Porotchkin <kostap@marvell.com> > > > > Some u-boot default configuration files could be shared between > > targets and used for building images for multiple board types. > > The only difference between such builds is the DTB embedded in in the > > boot image for each specific platform. > > This approach is widely used by Marvell, having the same u-boot > > configuration file for the entire SoC family, but allowing builds of > > multiple target flavors by supplying the device tree name through make > > command parameter DEVICE_TREE=xxx This patch adds such capability to > > uboot module of the buildroot. > > The custome DT name could be defined by > > BR2_TARGET_UBOOT_CUSTOM_DTS_NAME entry. > > > > Change-Id: I69e193339b0369a736bdf98491b9914d24a54e17 > > Signed-off-by: Konstantin Porotchkin <kostap@marvell.com> > > Reading this again, in fact it is very similar to patch > http://patchwork.ozlabs.org/patch/881197/ we already have in the queue. > With a very important distinction: your patch passes DEVICE_TREE=, while > the existing pending patch passes EXT_DTB=. The obvious question that > comes up is: which one is right ? Or are these two orthogonal things ? [KP] As far as I understand, the EXT_DTP is used for pointing to out of the tree pre-compiled DTB file, while DEVICE_TREE allows selection between DTS files existing in u-boot tree. So the first one is for customers that do not want to share their DT details and the second one for re-using the same configuration with different DTs. Does it makes sense? Kosta > > Thomas > -- > Thomas Petazzoni, CTO, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot
Hi Thomas, Kostya, Matthew, On Mon, Dec 24, 2018 at 02:47:59PM +0100, Thomas Petazzoni wrote: > Hello Kostya, > > +Clemens Gruber in Cc, who posted a related U-Boot patch. > > On Mon, 24 Dec 2018 15:19:04 +0200, kostap@marvell.com wrote: > > From: Konstantin Porotchkin <kostap@marvell.com> > > > > Some u-boot default configuration files could be shared between > > targets and used for building images for multiple board types. > > The only difference between such builds is the DTB embedded in > > in the boot image for each specific platform. > > This approach is widely used by Marvell, having the same u-boot > > configuration file for the entire SoC family, but allowing builds > > of multiple target flavors by supplying the device tree name through > > make command parameter DEVICE_TREE=xxx > > This patch adds such capability to uboot module of the buildroot. > > The custome DT name could be defined by > > BR2_TARGET_UBOOT_CUSTOM_DTS_NAME entry. > > > > Change-Id: I69e193339b0369a736bdf98491b9914d24a54e17 > > Signed-off-by: Konstantin Porotchkin <kostap@marvell.com> > > Reading this again, in fact it is very similar to patch > http://patchwork.ozlabs.org/patch/881197/ we already have in the queue. > With a very important distinction: your patch passes DEVICE_TREE=, > while the existing pending patch passes EXT_DTB=. The obvious question > that comes up is: which one is right ? Or are these two orthogonal > things ? They solve different problems. The reason for the EXT_DTB patch was to allow passing pre-built dtbs with metadata used for verified boot in U-Boot. (It's a simple device tree blob which stores the public key for the FIT image with the kernel, initrd etc. and is generated by U-Boot mkimage) Matthew Weber suggested alternatives like appending this blob to uboot.bin or decompiling the dtb and including the resulting dts via BR2_TARGET_UBOOT_CUSTOM_DTS_PATH. Afaik, DEVICE_TREE is for user-specified "normal" DTs, e.g. for configuration sharing / to override CONFIG_DEFAULT_DEVICE_TREE. EXT_DTB is used for these extra DT blobs in verified boot scenarios. Happy holidays! Clemens
Hello Clemens, Kostya, On Mon, 24 Dec 2018 17:59:31 +0100 Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > They solve different problems. > > The reason for the EXT_DTB patch was to allow passing pre-built dtbs > with metadata used for verified boot in U-Boot. > (It's a simple device tree blob which stores the public key for the FIT > image with the kernel, initrd etc. and is generated by U-Boot mkimage) > > Matthew Weber suggested alternatives like appending this blob to > uboot.bin or decompiling the dtb and including the resulting dts via > BR2_TARGET_UBOOT_CUSTOM_DTS_PATH. > > Afaik, DEVICE_TREE is for user-specified "normal" DTs, e.g. for > configuration sharing / to override CONFIG_DEFAULT_DEVICE_TREE. > EXT_DTB is used for these extra DT blobs in verified boot scenarios. I thought a bit about this. U-Boot in fact has tons of environment variables to tweak stuff. Instead of adding explicit options for each of them, perhaps we should simply add an option that allows to pass arbitrary environment variables to the U-Boot build. Something long the lines of: config BR2_TARGET_UBOOT_CUSTOM_ENVVARS string "additional environment variables" help ... UBOOT_MAKE_OPTS += $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_ENVVARS)) Not only this would allow to support more than EXT_DTB and DEVICE_TREE, but other variables. In addition, what worries me about EXT_DTB and DEVICE_TREE as individual options is that the semantic is hard to explain. Both are device trees, but they don't serve the same purpose. Thoughts ? Thomas
Hi Thomas, On Sun, Feb 03, 2019 at 08:49:50PM +0100, Thomas Petazzoni wrote: > Hello Clemens, Kostya, > > On Mon, 24 Dec 2018 17:59:31 +0100 > Clemens Gruber <clemens.gruber@pqgruber.com> wrote: > > > They solve different problems. > > > > The reason for the EXT_DTB patch was to allow passing pre-built dtbs > > with metadata used for verified boot in U-Boot. > > (It's a simple device tree blob which stores the public key for the FIT > > image with the kernel, initrd etc. and is generated by U-Boot mkimage) > > > > Matthew Weber suggested alternatives like appending this blob to > > uboot.bin or decompiling the dtb and including the resulting dts via > > BR2_TARGET_UBOOT_CUSTOM_DTS_PATH. > > > > Afaik, DEVICE_TREE is for user-specified "normal" DTs, e.g. for > > configuration sharing / to override CONFIG_DEFAULT_DEVICE_TREE. > > EXT_DTB is used for these extra DT blobs in verified boot scenarios. > > I thought a bit about this. U-Boot in fact has tons of environment > variables to tweak stuff. Instead of adding explicit options for each > of them, perhaps we should simply add an option that allows to pass > arbitrary environment variables to the U-Boot build. Something long the > lines of: > > config BR2_TARGET_UBOOT_CUSTOM_ENVVARS > string "additional environment variables" > help > ... > > UBOOT_MAKE_OPTS += $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_ENVVARS)) > > Not only this would allow to support more than EXT_DTB and DEVICE_TREE, > but other variables. In addition, what worries me about EXT_DTB and > DEVICE_TREE as individual options is that the semantic is hard to > explain. Both are device trees, but they don't serve the same purpose. > > Thoughts ? Sounds good to me! And it's future-proof. Thanks, Clemens
diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in index ac6f8bc8c1..c67994229a 100644 --- a/boot/uboot/Config.in +++ b/boot/uboot/Config.in @@ -529,4 +529,12 @@ config BR2_TARGET_UBOOT_CUSTOM_DTS_PATH endif +config BR2_TARGET_UBOOT_CUSTOM_DTS_NAME + string "Device Tree Source name" + help + The value of u-boot DEVICE_TREE for usage with the selected board + configuration file. Allows sharing of the same u-boot configuration + for building images for different platforms/boards. + + endif # BR2_TARGET_UBOOT diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk index cbdfee6ac3..a0342ec6c8 100644 --- a/boot/uboot/uboot.mk +++ b/boot/uboot/uboot.mk @@ -135,6 +135,11 @@ UBOOT_MAKE_OPTS += \ HOSTCC="$(HOSTCC) $(subst -I/,-isystem /,$(subst -I /,-isystem /,$(HOST_CFLAGS)))" \ HOSTLDFLAGS="$(HOST_LDFLAGS)" +UBOOT_CUSTOM_DTS_NAME = $(call qstrip,$(BR2_TARGET_UBOOT_CUSTOM_DTS_NAME)) +ifneq ($(UBOOT_CUSTOM_DTS_NAME),) +UBOOT_MAKE_OPTS += DEVICE_TREE=$(UBOOT_CUSTOM_DTS_NAME) +endif + ifeq ($(BR2_TARGET_UBOOT_NEEDS_ATF_BL31),y) UBOOT_DEPENDENCIES += arm-trusted-firmware UBOOT_MAKE_OPTS += BL31=$(BINARIES_DIR)/bl31.bin