Add flashing tools for tegra processors
v2 cover Add flashing tools for tegra processors
This patch series adds the tools needed to get a bootloader and optionally a rootfilesystem flashed onto tegra processors. Packages tegrarcm and cbootimage are fairly simple (cbootimage should hopefully see a new release soonish, so it won't have to point to git).
The third package which is cbootimage-configs is where I'd be happy to get some more feedback. This package basically pulls the repository which holds build scripts and configurations for various tegra boards then utilizes cbootimage to actually generate flashable images out of what u-boot generated before.
On top of what cbootimage-configs does I added a feature into the makefile which in a similar fashion is provided by tegra-uboot-flasher-scripts (found on https://github.com/NVIDIA/tegra-uboot-flasher-scripts). As tegra-uboot-flasher-scripts contains a whole set of build scripting to generate uboot, tegrarcm and cbootimage it didn't seem feasible to merge it as a package into buildroot. Instead I decided to make up the relevant feature of it directly within buildroot which is generating a special u-boot image. This image consists of:
- (1) a u-boot instance that is actually running when loaded via tegrarcm
- (2) a dtb that amends the u-boot environment of (1) to run commands to write the u-boot target image (3) onto the systems mmc. Optionally it can also write a partition table and enter dfu mode for root filesystem flashing.
- (3) the u-boot target image which was generated by cbootimage-configs before
Unfortunately to generate this merged image several of the intermediate output files of u-boot are needed, namedly: u-boot.bin, u-boot-nodtb-tegra.bin, u-boot-dtb-tegra.bin, u-boot.dtb.
In v2 of this patch series these are taken from $(UBOOT_BUILDDIR). This avoids cluttering $(BINARIES_DIR) with intermediate files and simplifies the configuration for the user, as he won't have to provide them as glob to UBOOT_IMAGE_NAME.
Regards,
Julian
Julian Scheel (4):
Add package cryptopp
Add package tegrarcm
Add package cbootimage
Add package cbootimage-configs
package/Config.in | 1 + package/Config.in.host | 2 + package/cbootimage-configs/Config.in | 122 ++++++++++++++++++++ package/cbootimage-configs/cbootimage-configs.hash | 2 + package/cbootimage-configs/cbootimage-configs.mk | 128 +++++++++++++++++++++ package/cbootimage-configs/gpt-table | 1 + package/cbootimage/Config.in.host | 8 ++ package/cbootimage/cbootimage.hash | 2 + package/cbootimage/cbootimage.mk | 13 +++ package/cryptopp/cryptopp.hash | 2 + package/cryptopp/cryptopp.mk | 33 ++++++ ...-cryptopp-include-crosscompile-compatible.patch | 27 +++++ package/tegrarcm/Config.in.host | 8 ++ package/tegrarcm/tegrarcm.hash | 2 + package/tegrarcm/tegrarcm.mk | 15 +++ 15 files changed, 366 insertions(+) create mode 100644 package/cbootimage-configs/Config.in create mode 100644 package/cbootimage-configs/cbootimage-configs.hash create mode 100644 package/cbootimage-configs/cbootimage-configs.mk create mode 100644 package/cbootimage-configs/gpt-table create mode 100644 package/cbootimage/Config.in.host create mode 100644 package/cbootimage/cbootimage.hash create mode 100644 package/cbootimage/cbootimage.mk create mode 100644 package/cryptopp/cryptopp.hash create mode 100644 package/cryptopp/cryptopp.mk create mode 100644 package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch create mode 100644 package/tegrarcm/Config.in.host create mode 100644 package/tegrarcm/tegrarcm.hash create mode 100644 package/tegrarcm/tegrarcm.mk
Signature · 1 line
2.7.3
PATCH v2 1/4 Add package cryptopp
--- package/cryptopp/cryptopp.hash | 2 ++ package/cryptopp/cryptopp.mk | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 package/cryptopp/cryptopp.hash create mode 100644 package/cryptopp/cryptopp.mk
package/cryptopp/cryptopp.hash new file +2 -0
@@ -0,0 +1,2 @@ +# Locally computed +sha256 9390670a14170dd0f48a6b6b06f74269ef4b056d4718a1a329f6f6069dc957c9 cryptopp563.zip
package/cryptopp/cryptopp.mk new file +33 -0
@@ -0,0 +1,33 @@ +################################################################################ +# +# cryptopp +# +################################################################################ + +CRYPTOPP_VERSION = 5.6.3 +CRYPTOPP_SOURCE = cryptopp$(subst .,,$(CRYPTOPP_VERSION)).zip +CRYPTOPP_SITE = http://cryptopp.com/ +CRYPTOPP_LICENSE = Boost-v1.0 +CRYPTOPP_LICENSE_FILES = License.txt +CRYPTOPP_INSTALL_STAGING = YES + +HOST_CRRYPTOPP_MAKE_OPTS = \ + $(HOST_CONFIGURE_OPTS) \ + LDFLAGS="$(HOST_LDFLAGS)" + +define HOST_CRYPTOPP_BUILD_CMDS + $(MAKE) -C $(@D) $(HOST_CRYPTOPP_MAKE_OPTS) -f GNUmakefile + $(MAKE) -C $(@D) $(HOST_CRYPTOPP_MAKE_OPTS) libcryptopp.so +endef + +define HOST_CRYPTOPP_INSTALL_CMDS + $(INSTALL) -d ${HOST_DIR}/usr/include/cryptopp + $(INSTALL) -D -m 644 $(@D)/*.h ${HOST_DIR}/usr/include/cryptopp/ + $(INSTALL) -D -m 0755 $(@D)/libcryptopp.so ${HOST_DIR}/usr/lib/libcryptopp.so +endef + +define HOST_CRYPTOPP_EXTRACT_CMDS + $(UNZIP) $(DL_DIR)/$(CRYPTOPP_SOURCE) -d $(@D) +endef + +$(eval $(host-generic-package))
Signature · 1 line
2.7.3
PATCH v2 1/4 Add package cryptopp
Hello,
package/cryptopp/cryptopp.mk line 14
Quoted 8 lines · Julian Scheel
package/cryptopp/cryptopp.mk new file +8
-0+CRYPTOPP_VERSION = 5.6.3 +CRYPTOPP_SOURCE = cryptopp$(subst .,,$(CRYPTOPP_VERSION)).zip +CRYPTOPP_SITE = http://cryptopp.com/ +CRYPTOPP_LICENSE = Boost-v1.0 +CRYPTOPP_LICENSE_FILES = License.txt +CRYPTOPP_INSTALL_STAGING = YES + +HOST_CRRYPTOPP_MAKE_OPTS = \
Typo in the variable name, which means it wasn't used at all. If it had been used, you would have spotted some build issues.
package/cryptopp/cryptopp.mk line 16
Quoted 2 lines
package/cryptopp/cryptopp.mk new file +2
-0+ $(HOST_CONFIGURE_OPTS) \ + LDFLAGS="$(HOST_LDFLAGS)"
This line is not needed, since HOST_CONFIGURE_OPTS already contains LDFLAGS="$(HOST_LDFLAGS)". However, I had to pass CXXFLAGS="$(HOST_CXXFLAGS) -fPIC" to make the thing build.
package/cryptopp/cryptopp.mk line 20
Quoted 4 lines
package/cryptopp/cryptopp.mk new file +4
-0+ +define HOST_CRYPTOPP_BUILD_CMDS + $(MAKE) -C $(@D) $(HOST_CRYPTOPP_MAKE_OPTS) -f GNUmakefile + $(MAKE) -C $(@D) $(HOST_CRYPTOPP_MAKE_OPTS) libcryptopp.so
Having two lines is not needed, building the "shared" target is sufficient.
package/cryptopp/cryptopp.mk line 26
Quoted 6 lines
package/cryptopp/cryptopp.mk new file +6
-0+endef + +define HOST_CRYPTOPP_INSTALL_CMDS + $(INSTALL) -d ${HOST_DIR}/usr/include/cryptopp + $(INSTALL) -D -m 644 $(@D)/*.h ${HOST_DIR}/usr/include/cryptopp/ + $(INSTALL) -D -m 0755 $(@D)/libcryptopp.so ${HOST_DIR}/usr/lib/libcryptopp.so
Using the "install" target is sufficient, so I've used that instead.
package/cryptopp/cryptopp.mk line 31
Quoted 5 lines
package/cryptopp/cryptopp.mk new file +5
-0+endef + +define HOST_CRYPTOPP_EXTRACT_CMDS + $(UNZIP) $(DL_DIR)/$(CRYPTOPP_SOURCE) -d $(@D) +endef
Since extract is the first step, we usually put this before the build/install commands.
I've fixed up those issues and applied. Thanks!
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v2 1/4 Add package cryptopp
In reply to Julian ScheelHello,
Quoted 7 lines · Julian Scheel
Signed-off-by Julian Scheel <[email protected]>--- package/cryptopp/cryptopp.hash | 2 ++ package/cryptopp/cryptopp.mk | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+) create mode 100644 package/cryptopp/cryptopp.hash create mode 100644 package/cryptopp/cryptopp.mk
This package causes some build failures:
http://autobuild.buildroot.org/?reason=host-cryptopp-5.6.3
Could you have a look at them?
Thanks!
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v2 1/4 Add package cryptopp
Hi Thomas,
Quoted 9 lines · Thomas Petazzoni
Quoted 7 lines · Julian Scheel
Signed-off-by Julian Scheel <[email protected]>--- package/cryptopp/cryptopp.hash | 2 ++ package/cryptopp/cryptopp.mk | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 35 insertions(+)create mode 100644 package/cryptopp/cryptopp.hash create mode 100644
package/cryptopp/cryptopp.mk
Quoted 5 lines
This package causes some build failures:
http://autobuild.buildroot.org/?reason=host-cryptopp-5.6.3Could you have a look at them?
Should be fixed by:
[PATCH] package/cryptopp: Backport gcc compatibility patch
-Julian
PATCH v2 2/4 Add package tegrarcm
In reply to Julian ScheelAdd package for the tegrarcm host utility that allows loading data to tegra processors in recovery mode.
---
Changes in v2:
--------------
- Use github helper
- Fix license
- Add cryptopp dependency
- Add patch to fix cryptopp inclusion for cross build
--- package/Config.in.host | 1 + ...-cryptopp-include-crosscompile-compatible.patch | 27 ++++++++++++++++++++++ package/tegrarcm/Config.in.host | 8 +++++++ package/tegrarcm/tegrarcm.hash | 2 ++ package/tegrarcm/tegrarcm.mk | 15 ++++++++++++ 5 files changed, 53 insertions(+) create mode 100644 package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch create mode 100644 package/tegrarcm/Config.in.host create mode 100644 package/tegrarcm/tegrarcm.hash create mode 100644 package/tegrarcm/tegrarcm.mk
package/Config.in.host +1 -0
@@ -27,6 +27,7 @@ menu "Host utilities" source "package/sam-ba/Config.in.host" source "package/squashfs/Config.in.host" source "package/sunxi-tools/Config.in.host" + source "package/tegrarcm/Config.in.host" source "package/uboot-tools/Config.in.host" source "package/util-linux/Config.in.host"
package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch new file +27 -0
@@ -0,0 +1,27 @@ +From 0e60af53fa76aa2f274ade98da7ba543147e82c7 Mon Sep 17 00:00:00 2001 +From: Julian Scheel <[email protected]> +Date: Thu, 17 Mar 2016 12:37:04 +0100 +Subject: [PATCH] Make cryptopp include crosscompile compatible + +Allows user to set a SYSROOT variable for building against a specific root +filesystem. + +Signed-off-by: Julian Scheel <[email protected]> +--- + src/Makefile.am | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/src/Makefile.am b/src/Makefile.am +index 3dad0e6..7678dd4 100644 +--- a/src/Makefile.am ++++ b/src/Makefile.am +@@ -1,5 +1,5 @@ + AM_CFLAGS = -Wall -std=c99 +-AM_CPPFLAGS = -isystem /usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS) ++AM_CPPFLAGS = -isystem $(SYSROOT)/usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS) + + bin_PROGRAMS = tegrarcm + tegrarcm_SOURCES = \ +-- +2.7.3 +
package/tegrarcm/Config.in.host new file +8 -0
@@ -0,0 +1,8 @@ +config BR2_PACKAGE_HOST_TEGRARCM + bool "host tegrarcm" + depends on BR2_arm || BR2_armeb + help + This program is used to send code to a Tegra device in recovery + mode. + + https://github.com/NVIDIA/tegrarcm
package/tegrarcm/tegrarcm.hash new file +2 -0
@@ -0,0 +1,2 @@ +# Locally computed +sha256 538cb0af237ab33e070d3aeb6cc828cd7ef453753ba2ccc21b87ed43faac51bd tegrarcm-v1.7.tar.gz
package/tegrarcm/tegrarcm.mk new file +15 -0
@@ -0,0 +1,15 @@ +################################################################################ +# +# tegrarcm +# +################################################################################ + +TEGRARCM_VERSION = v1.7 +TEGRARCM_SITE = $(call github,NVIDIA,tegrarcm,$(TEGRARCM_VERSION)) +TEGRARCM_LICENSE = BSD-3c / NVIDIA Software License (src/miniloader) +TEGRARCM_LICENSE_FILE = LICENSE +TEGRARCM_AUTORECONF = YES +HOST_TEGRARCM_DEPENDENCIES = host-libusb host-pkgconf host-cryptopp +HOST_TEGRARCM_MAKE_OPTS = SYSROOT=$(HOST_DIR) + +$(eval $(host-autotools-package))
Signature · 1 line
2.7.3
PATCH v2 2/4 Add package tegrarcm
Hello,
The commit title should have been:
tegrarcm: new package
Quoted 17 lines · Julian Scheel
package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch new file +11
-0@@ -0,0 +1,27 @@ +From 0e60af53fa76aa2f274ade98da7ba543147e82c7 Mon Sep 17 00:00:00 2001 +From: Julian Scheel <[email protected]> +Date: Thu, 17 Mar 2016 12:37:04 +0100 +Subject: [PATCH] Make cryptopp include crosscompile compatible + +Allows user to set a SYSROOT variable for building against a specific root +filesystem. + +Signed-off-by: Julian Scheel <[email protected]> +--- + src/Makefile.am | 2 +-
package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch line 21
Quoted 10 lines
package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch new file +10
-0+ 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/src/Makefile.am b/src/Makefile.am +index 3dad0e6..7678dd4 100644 +--- a/src/Makefile.am ++++ b/src/Makefile.am +@@ -1,5 +1,5 @@ + AM_CFLAGS = -Wall -std=c99 +-AM_CPPFLAGS = -isystem /usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS) ++AM_CPPFLAGS = -isystem $(SYSROOT)/usr/include/$(CRYPTOLIB) $(LIBUSB_CFLAGS)
This is not really the recommended autoconf/automake solution. I've done a larger patch, but that I believe is more correct, as it relies on the normal compiler include path logic, and simply makes the source code include <cryptopp/foobar.h>, where of course cryptopp can also be crypto++ depending on the library being used.
I've also submitted my patch upstream:
Quoted 9 lines
package/tegrarcm/tegrarcm.mk new file +3
-0@@ -0,0 +1,15 @@ +################################################################################ +# +# tegrarcm
package/tegrarcm/tegrarcm.mk line 13
Quoted 10 lines
package/tegrarcm/tegrarcm.mk new file +10
-0+# +################################################################################ + +TEGRARCM_VERSION = v1.7 +TEGRARCM_SITE = $(call github,NVIDIA,tegrarcm,$(TEGRARCM_VERSION)) +TEGRARCM_LICENSE = BSD-3c / NVIDIA Software License (src/miniloader) +TEGRARCM_LICENSE_FILE = LICENSE +TEGRARCM_AUTORECONF = YES +HOST_TEGRARCM_DEPENDENCIES = host-libusb host-pkgconf host-cryptopp +HOST_TEGRARCM_MAKE_OPTS = SYSROOT=$(HOST_DIR)
Due to the patch being changed, I've removed this last line, and applied.
Thanks!
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v2 3/4 Add package cbootimage
In reply to Julian ScheelAdd package for cbootimage host utility that is able to compile bct files and generate flashable images out of a bct and an image for tegra processors.
---
Changes in v2:
--------------
- Depend on ARM platform
- Use github helper
- Add license file reference
--- package/Config.in.host | 1 + package/cbootimage/Config.in.host | 8 ++++++++ package/cbootimage/cbootimage.hash | 2 ++ package/cbootimage/cbootimage.mk | 13 +++++++++++++ 4 files changed, 24 insertions(+) create mode 100644 package/cbootimage/Config.in.host create mode 100644 package/cbootimage/cbootimage.hash create mode 100644 package/cbootimage/cbootimage.mk
package/Config.in.host +1 -0
@@ -1,5 +1,6 @@ menu "Host utilities" + source "package/cbootimage/Config.in.host" source "package/checkpolicy/Config.in.host" source "package/cramfs/Config.in.host" source "package/dfu-util/Config.in.host"
package/cbootimage/Config.in.host new file +8 -0
@@ -0,0 +1,8 @@ +config BR2_PACKAGE_HOST_CBOOTIMAGE + bool "host cbootimage" + depends on BR2_arm || BR2_armeb + help + This project provides a tool which compiles BCT (Boot Configuration + Table) images to place into the boot flash of a Tegra-based device. + + https://github.com/NVIDIA/cbootimage
package/cbootimage/cbootimage.hash new file +2 -0
@@ -0,0 +1,2 @@ +# Locally computed +sha256 373c108d7b6778c62a33e59ad0cd5ea9ebb379319a0c8b4cf469eaa8bec5521b cbootimage-v1.7.tar.gz
package/cbootimage/cbootimage.mk new file +13 -0
@@ -0,0 +1,13 @@ +################################################################################ +# +# cbootimage +# +################################################################################ + +CBOOTIMAGE_VERSION = v1.7 +CBOOTIMAGE_SITE = $(call github,NVIDIA,cbootimage,$(CBOOTIMAGE_VERSION)) +CBOOTIMAGE_LICENSE = GPLv2 +CBOOTIMAGE_LICENSE_FILES = COPYING +CBOOTIMAGE_AUTORECONF = YES + +$(eval $(host-autotools-package))
Signature · 1 line
2.7.3
PATCH v2 3/4 Add package cbootimage
Hello,
Quoted 4 lines · Julian Scheel
Add package for cbootimage host utility that is able to compile bct files and generate flashable images out of a bct and an image for tegra processors.
Signed-off-by Julian Scheel <[email protected]>
The commit title should have been:
cbootimage: new package
package/cbootimage/Config.in.host line 6
Quoted 2 lines
package/cbootimage/Config.in.host new file +2
-0+ This project provides a tool which compiles BCT (Boot Configuration + Table) images to place into the boot flash of a Tegra-based device.
Those lines were slightly too long. You shouldn't exceed 72 chars.
I've fixed up those minor issues and applied. Thanks!
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v2 4/4 Add package cbootimage-configs
In reply to Julian ScheelAdd package for cbootimage configurations that contains build scripts to generate bct and image files for various tegra based boards using cbootimage file.
Additionally the package makefile includes extra hooks to generate self-contained flasher binaries based on u-boot, that are actually able to flash a u-boot instance into mmc memory when loaded to tegra via tegrarcm.
---
Changes in v2:
--------------
- Add proper BR2_PACKAGE prefix to variable names
- Use menuconfig
- Read gpt spec from file, to avoid ugly double quotes
- Read intermediate files from UBOOT_BUILDDIR to avoid glob hack on UBOOT_IMAGE_NAME and keep BINARIES_DIR uncluttered
- Use github helper function
- Add hash
--- package/Config.in | 1 + package/cbootimage-configs/Config.in | 122 ++++++++++++++++++++ package/cbootimage-configs/cbootimage-configs.hash | 2 + package/cbootimage-configs/cbootimage-configs.mk | 128 +++++++++++++++++++++ package/cbootimage-configs/gpt-table | 1 + 5 files changed, 254 insertions(+) create mode 100644 package/cbootimage-configs/Config.in create mode 100644 package/cbootimage-configs/cbootimage-configs.hash create mode 100644 package/cbootimage-configs/cbootimage-configs.mk create mode 100644 package/cbootimage-configs/gpt-table
package/Config.in +1 -0
@@ -150,6 +150,7 @@ endmenu menu "Filesystem and flash utilities" source "package/btrfs-progs/Config.in" + source "package/cbootimage-configs/Config.in" source "package/cifs-utils/Config.in" source "package/cpio/Config.in" source "package/cramfs/Config.in"
package/cbootimage-configs/Config.in new file +122 -0
122 lines not on this page · raw message
package/cbootimage-configs/cbootimage-configs.hash new file +2 -0
@@ -0,0 +1,2 @@ +# Locally computed +sha256 f9eaa2e88fa24ea348d6358f18e4bab58ac21c586b5b6cb5300e21a6e1d9de83 cbootimage-configs-9d45319.tar.gz
package/cbootimage-configs/cbootimage-configs.mk new file +128 -0
128 lines not on this page · raw message
package/cbootimage-configs/gpt-table new file +1 -0
@@ -0,0 +1 @@ +uuid_disk=\$\{disk_uuid\};name=rootfs,size=-,uuid=\$\{rootfs_uuid\}
Signature · 1 line
2.7.3
PATCH v2 4/4 Add package cbootimage-configs
Hello,
Quoted 8 lines · Julian Scheel
Add package for cbootimage configurations that contains build scripts to generate bct and image files for various tegra based boards using cbootimage file.
Additionally the package makefile includes extra hooks to generate self-contained flasher binaries based on u-boot, that are actually able to flash a u-boot instance into mmc memory when loaded to tegra via tegrarcm.Signed-off-by Julian Scheel <[email protected]>
To be honest, this is too complicated. This package is doing too much stuff in its .mk file. At some point, you mention that your .mk code is heavily inspired by
https://github.com/NVIDIA/tegra-uboot-flasher-scripts. Why don't you use/package those scripts instead?
Or maybe this thing should do less stuff, and simply provide the necessary tools for a post-image script to do the job.
It would be useful if you could submit the defconfig for a Tegra based board as part of this series so that we can see how it all fits together.
I've added some more comments/questions below.
package/cbootimage-configs/Config.in line 19
Quoted 4 lines
package/cbootimage-configs/Config.in new file +4
-0+if BR2_PACKAGE_CBOOTIMAGE_CONFIGS + +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD + string "cbootimage board to build"
I think the "cbootimage" prefix in the string is useless here, and in the other options. In menuconfig, we clearly see it's a suboption.
Quoted 7 lines
package/cbootimage-configs/Config.in new file +7
-0+config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED + bool "sign generated image for secureboot" + help + Sign the image using a provided ssl key. This makes it possible to + run the image in secureboot environments with the matching key being + burned to the tegra fuses. + Be aware that the generated image will have the extension simg
package/cbootimage-configs/Config.in line 57
Quoted 10 lines
package/cbootimage-configs/Config.in new file +10
-0+ instead of img if signing is enabled. + + Compatible key files can be generated with openssl: + openssl genrsa -out rsa_priv.pem 2048 + +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY + string "pkc file name" + depends on BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED + help + Key file to use for signing the loader image.
When there is such a bool to enable/disable + one string giving the argument, I believe we generally prefer to just have the string. When the string is empty -> feature is disabled.
Quoted 12 lines
package/cbootimage-configs/Config.in new file +12
-0+if BR2_PACKAGE_CBOOTIMAGE_CONFIGS_GENERATE_FLASHER + +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT + bool "write partition table to mmc 0" + help + Add a flashing step which is writing a custom gpt partition table + onto the target devices first mmc storage + +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_GPT_FILE + string "partition table file" + depends on BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT + default "package/cbootimage-configs/gpt-table"
package/cbootimage-configs/Config.in line 93
Quoted 10 lines
package/cbootimage-configs/Config.in new file +10
-0+ help + Provide a file that contains the paritition table to write to mmc. + Use format as specified in u-boot's README.gpt documentation file. + + Make sure to escape variable properly, as u-boot shall not evaluate + the variables when setting the environment variable, but only when + running the gpt command. + + Example file content: + uuid_disk=\$\{disk_uuid\};name=rootfs,size=2000MiB,uuid=\$\{rootfs_uuid\};name=data,size=-,uuid=\$\{data_uuid\}
Same here.
Quoted 14 lines
package/cbootimage-configs/cbootimage-configs.hash new file +2
-0@@ -0,0 +1,2 @@ +# Locally computed +sha256 f9eaa2e88fa24ea348d6358f18e4bab58ac21c586b5b6cb5300e21a6e1d9de83 cbootimage-configs-9d45319.tar.gzpackage/cbootimage-configs/cbootimage-configs.mk new file +0
-0
package/cbootimage-configs/cbootimage-configs.mk line 8
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk new file +8
-0@@ -0,0 +1,128 @@ +################################################################################ +# +# cbootimage-configs +# +################################################################################ + +CBOOTIMAGE_CONFIGS_VERSION = 9d45319 +CBOOTIMAGE_CONFIGS_SITE = $(call github,avionic-design,cbootimage-configs,$(CBOOTIMAGE_CONFIGS_VERSION))
Why do you use the Avionic Design fork and not the original NVIDIA repository, which you reference as the upstream in your Config.in help text?
package/cbootimage-configs/cbootimage-configs.mk line 19
Quoted 11 lines
package/cbootimage-configs/cbootimage-configs.mk new file +11
-0+CBOOTIMAGE_CONFIGS_DEPENDENCIES = host-cbootimage uboot +CBOOTIMAGE_CONFIGS_INSTALL_TARGET = NO +CBOOTIMAGE_CONFIGS_INSTALL_IMAGES = YES + +CONFIGS_BUILD = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)) +BCT_NAME = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BCT)) +IMG_NAME = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG)) +EXTRA_BOOTCMD = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_EXTRA_BOOTCMD)) +GPT_FILE = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_GPT_FILE)) +DFU_MAP = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU_MAP)) +KEY_FILE = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY))
The variable namespace in Buildroot is global, so they should always be prefixed by the name of the package.
package/cbootimage-configs/cbootimage-configs.mk line 23
Quoted 3 lines
package/cbootimage-configs/cbootimage-configs.mk new file +3
-0+ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS),y) +ifeq ($(CONFIGS_BUILD),) + $(warning BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD=$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD))
That's debug stuff.
package/cbootimage-configs/cbootimage-configs.mk line 24
Quoted 1 line
package/cbootimage-configs/cbootimage-configs.mk new file +1
-0+ $(warning CONFIGS_BUILD=$(CONFIGS_BUILD))
Ditto.
Quoted 15 lines
package/cbootimage-configs/cbootimage-configs.mk new file +15
-0+ $(error No cbootimage-configuration specififed to build. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD setting) +endif +ifeq ($(BCT_NAME),) + $(error No bct filename specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BCT setting) +endif +ifeq ($(IMG_NAME),) + $(error No img filename specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG setting) +endif +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT),y) +ifeq ($(GPT_FILE),) + $(error No gpt partitions specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_GPT_FILE setting) +endif +endif +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU),y) +ifeq ($(DFU_MAP),)
package/cbootimage-configs/cbootimage-configs.mk line 49
Quoted 10 lines
package/cbootimage-configs/cbootimage-configs.mk new file +10
-0+ $(error No dfu map specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU_MAP setting) +endif +endif +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED),y) +ifeq ($(KEY_FILE),) + $(error No key file specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY setting) +endif +KEY_OPTS = skb=$(KEY_FILE) +else +KEY_OPTS =
Incorrect variable name. Also, I believe this should be separated from the big list of checks.
package/cbootimage-configs/cbootimage-configs.mk line 52
Quoted 3 lines
package/cbootimage-configs/cbootimage-configs.mk new file +3
-0+endif + +TARGET_SIZE = $(TARGET_CROSS)size
To be moved to package/Makefile.in I believe, in a separate patch.
package/cbootimage-configs/cbootimage-configs.mk line 57
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk new file +5
-0+endif + +define CBOOTIMAGE_CONFIGS_BUILD_CMDS + cp $(UBOOT_BUILDDIR)/u-boot-dtb-tegra.bin $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)/u-boot.bin + (cd $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD) && $(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(KEY_OPTS))
use $(MAKE) -C $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)
package/cbootimage-configs/cbootimage-configs.mk line 69
Quoted 12 lines
package/cbootimage-configs/cbootimage-configs.mk new file +12
-0+endef + +# Generate a self-contained u-boot image, flashing u-boot to mmc +define CBOOTIMAGE_CONFIGS_BUILD_FLASHER_CMD + # u-boot bss size: ${BSS_SIZE} + # u-boot-nodtb-tegra.bin size: ${UBOOT_NODTB_TEGRA_SIZE} + # u-boot.dtb size: ${UBOOT_DTB_SIZE} + # ${IMG_NAME} size: ${IMG_SIZE} + # u-boot plus dtb size: ${UBOOT_PLUS_DTB_SIZE} + # padded size: ${PADDED_SIZE} + # bootcmd: $(UBOOT_FLASH_ENV) + # gpt-file: $(PWD)/$(GPT_FILE)
I don't see the usefulness of this comment.
package/cbootimage-configs/cbootimage-configs.mk line 74
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk new file +5
-0+ + cp $(UBOOT_BUILDDIR)/u-boot.dtb ${DTB_FILE} + # Patch environment into u-boot dtb + $(HOST_MAKE_ENV) fdtput -p -t x ${DTB_FILE} '/config' 'bootdelay' '0xfffffffe' + $(HOST_MAKE_ENV) fdtput -p -t s ${DTB_FILE} '/config' 'bootcmd' $(UBOOT_FLASH_ENV)
What guarantees you that fdtput is available? You need to depend on host-dtc to be sure that it's available.
package/cbootimage-configs/cbootimage-configs.mk line 78
Quoted 4 lines
package/cbootimage-configs/cbootimage-configs.mk new file +4
-0+ + # Assemble u-boot-flasher.bin + cp $(UBOOT_BUILDDIR)/u-boot-nodtb-tegra.bin $(BINARIES_DIR)/u-boot-flasher.bin + cat ${DTB_FILE} >> $(BINARIES_DIR)/u-boot-flasher.bin
Use $() to reference variables. Why don't you do a single cat here ?
Quoted 11 lines
package/cbootimage-configs/cbootimage-configs.mk new file +11
-0+ truncate -s $(PADDED_SIZE) $(BINARIES_DIR)/u-boot-flasher.bin + cat $(@D)/$(CONFIGS_BUILD)/$(IMG_NAME) >> $(BINARIES_DIR)/u-boot-flasher.bin +endef + +# Computations for assembling u-boot with custom dtb mostly stolen from +# https://github.com/NVIDIA/tegra-uboot-flasher-scripts +# All credits go to Stephen Warren +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_GENERATE_FLASHER),y) +LOADADDR = 0x80108000 # FIXME: this is Tegra124 only +BSS_SIZE = $(shell $(TARGET_SIZE) -A $(UBOOT_BUILDDIR)/u-boot | grep ".bss\b" | tr -s ' ' | cut -d ' ' -f 2) +UBOOT_NODTB_TEGRA_SIZE = $(shell stat -c %s $(UBOOT_BUILDDIR)/u-boot-nodtb-tegra.bin)
package/cbootimage-configs/cbootimage-configs.mk line 99
Quoted 10 lines
package/cbootimage-configs/cbootimage-configs.mk new file +10
-0+UBOOT_DTB_SIZE = $(shell stat -c %s $(UBOOT_BUILDDIR)/u-boot.dtb) +IMG_SIZE = $(shell stat -c %s $(@D)/$(CONFIGS_BUILD)/$(IMG_NAME)) +IMG_SIZE_SECTORS = $(shell echo $$(( $(IMG_SIZE) / 512 )) ) +IMG_SIZE_SECTORS_HEX = $(shell printf '0x%x' $(IMG_SIZE_SECTORS) ) +UBOOT_PLUS_DTB_SIZE = $(shell echo $$(( $(UBOOT_NODTB_TEGRA_SIZE) + $(UBOOT_DTB_SIZE) )) ) +# Keep BSS area clear, acquire an extra 4kb for incereased dtb, align to 4k +# boundary +PADDED_SIZE = $(shell echo $$(( $(UBOOT_PLUS_DTB_SIZE) + $(BSS_SIZE) + (2 * 4 * 1024) - 1 & ~((4 * 1024) - 1) )) ) +IMAGEADDR = $(shell echo $$(( $(LOADADDR) + $(PADDED_SIZE) )) ) +IMAGEADDR_HEX = $(shell printf '0x%x' $(IMAGEADDR) )
This is really the messy part. That's clearly way too much stuff encoded into a Buildroot package.
package/cbootimage-configs/cbootimage-configs.mk line 102
Quoted 2 lines
package/cbootimage-configs/cbootimage-configs.mk new file +2
-0+ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT),y) +PWD = $(shell pwd)
Debugging stuff?
package/cbootimage-configs/cbootimage-configs.mk line 103
Quoted 1 line
package/cbootimage-configs/cbootimage-configs.mk new file +1
-0+GPT_SPEC = $(shell cat $(GPT_FILE))
If it's just a string, why is it the path to a file rather than having direcly the value in the Config.in option?
package/cbootimage-configs/cbootimage-configs.mk line 106
Quoted 3 lines
package/cbootimage-configs/cbootimage-configs.mk new file +3
-0+GPT_BOOTCMD = echo >>> Write partition table to MMC; gpt write mmc 0 '$(GPT_SPEC)'; mmc rescan +else +GPT_BOOTCMD =
Not needed, the default for a variable is to be empty.
Also, remember: all variables should be prefixed by the name of the package.
package/cbootimage-configs/cbootimage-configs.mk line 115
Quoted 9 lines
package/cbootimage-configs/cbootimage-configs.mk new file +9
-0+endif + +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU),y) +DFU_BOOTCMD = echo >>> Enter DFU mode; setenv dfu_alt_info '$(DFU_MAP)'; dfu 0 mmc 0; +else +DFU_BOOTCMD = +endif + +UBOOT_FLASH_ENV = "echo >>> Write image to MMC; mmc dev 0 1; mmc write ${IMAGEADDR_HEX} 0 ${IMG_SIZE_SECTORS_HEX}; echo >>> Configure environment; env default -f -a; saveenv; $(GPT_BOOTCMD) $(EXTRA_BOOTCMD); $(DFU_BOOTCMD) echo >>> Resetting system; reset"
All this U-Boot command construction seem really project-specific, and I don't see why we should hardcode all that stuff in a Buildroot package. Buildroot should give the tools to generate such a U-Boot image, but should not hardcode so much logic IMO.
package/cbootimage-configs/cbootimage-configs.mk line 121
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk new file +5
-0+DTB_FILE = $(@D)/u-boot-flasher.dtb + +# Add the actual assembly hook +CBOOTIMAGE_CONFIGS_POST_INSTALL_IMAGES_HOOKS += CBOOTIMAGE_CONFIGS_BUILD_FLASHER_CMD +endif
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
PATCH v2 4/4 Add package cbootimage-configs
Quoted 3 lines · Thomas Petazzoni
Hello,
On Thu, 17 Mar 2016 16:03:58 +0100, Julian Scheel wrote:
[snip]
Quoted 2 lines
Quoted 1 line
Quoted 1 line
+TARGET_SIZE = $(TARGET_CROSS)size
To be moved to package/Makefile.in I believe, in a separate patch.
Why do you need a cross-size anyway? size, like readelf, should be universal, no?
Regards,
Arnout
Signature · 6 lines
Arnout Vandecappelle arnout at mind be Senior Embedded Software Architect +32-16-286500 Essensium/Mind http://www.mind.be G.Geenslaan 9, 3001 Leuven, Belgium BE 872 984 063 RPR Leuven LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle GPG fingerprint: 7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF
PATCH v2 4/4 Add package cbootimage-configs
In reply to Thomas PetazzoniHi Thomas,
thanks for the review, see comments below.
Quoted 10 lines · Thomas Petazzoni
Hello,
Quoted 7 lines · Julian Scheel
Add package for cbootimage configurations that contains build scripts to generate bct and image files for various tegra based boards using cbootimage file.
Additionally the package makefile includes extra hooks to generate self-contained flasher binaries based on u-boot, that are actually able to flash a u-boot instance into mmc memory when loaded to tegra via tegrarcm.
Quoted 7 lines
Quoted 1 line
Signed-off-by Julian Scheel <[email protected]>To be honest, this is too complicated. This package is doing too much stuff in its .mk file. At some point, you mention that your .mk code is heavily inspired by
https://github.com/NVIDIA/tegra-uboot-flasher-scripts. Why don't you use/package those scripts instead?
The problem with tegra-uboot-flasher-scripts is, that it does a lot more than what this package does. In fact it includes build logic for all the required tools (like dtc, tegrarcm, cbootimage, u-boot), which wouldn't work out well in the context of buildroot. Using externally provided builds isn't easily possible without major abuse of the scripts, which I don't think would be merged upstream anyway.
Quoted 2 lines
Or maybe this thing should do less stuff, and simply provide the necessary tools for a post-image script to do the job.
If in the end it's decided that this is too complicated for the package makefile I'd rather create a dedicated package that handles this. So, that the magic would be in done in some independently maintained script and we only need a buildroot package that heads the options to that package. It just felt a bit like overkill for what it does in the end.
Quoted 3 lines
It would be useful if you could submit the defconfig for a Tegra based board as part of this series so that we can see how it all fits together.
Sure, I'll prepare a defconfig.
Quoted 11 lines
I've added some more comments/questions below.
package/cbootimage-configs/Config.in line 19
Quoted 4 lines
package/cbootimage-configs/Config.in new file +4
-0+if BR2_PACKAGE_CBOOTIMAGE_CONFIGS + +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD + string "cbootimage board to build"I think the "cbootimage" prefix in the string is useless here, and in the other options. In menuconfig, we clearly see it's a suboption.
Ack.
Quoted 13 lines
Quoted 13 lines
package/cbootimage-configs/Config.in new file +13
-0+config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED + bool "sign generated image for secureboot" + help + Sign the image using a provided ssl key. This makes it possible to + run the image in secureboot environments with the matching key being + burned to the tegra fuses. + Be aware that the generated image will have the extension simg + instead of img if signing is enabled. + + Compatible key files can be generated with openssl: + openssl genrsa -out rsa_priv.pem 2048 + +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY
Quoted 8 lines
package/cbootimage-configs/Config.in line 57
Quoted 4 lines
package/cbootimage-configs/Config.in new file +4
-0+ string "pkc file name" + depends on BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED + help + Key file to use for signing the loader image.When there is such a bool to enable/disable + one string giving the argument, I believe we generally prefer to just have the string. When the string is empty -> feature is disabled.
Makes sense, thanks.
Quoted 43 lines
Quoted 12 lines
package/cbootimage-configs/Config.in new file +12
-0+if BR2_PACKAGE_CBOOTIMAGE_CONFIGS_GENERATE_FLASHER + +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT + bool "write partition table to mmc 0" + help + Add a flashing step which is writing a custom gpt partition table + onto the target devices first mmc storage + +config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_GPT_FILE + string "partition table file" + depends on BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT + default "package/cbootimage-configs/gpt-table"package/cbootimage-configs/Config.in line 93
Quoted 10 lines
package/cbootimage-configs/Config.in new file +10
-0+ help + Provide a file that contains the paritition table to write to mmc. + Use format as specified in u-boot's README.gpt documentation file. + + Make sure to escape variable properly, as u-boot shall not evaluate + the variables when setting the environment variable, but only when + running the gpt command. + + Example file content: + uuid_disk=\$\{disk_uuid\};name=rootfs,size=2000MiB,uuid=\$\{rootfs_uuid\};name=data,size=-,uuid=\$\{data_uuid\}Same here.
Quoted 18 lines
package/cbootimage-configs/cbootimage-configs.hash new file +2
-0@@ -0,0 +1,2 @@ +# Locally computed +sha256 f9eaa2e88fa24ea348d6358f18e4bab58ac21c586b5b6cb5300e21a6e1d9de83 cbootimage-configs-9d45319.tar.gzpackage/cbootimage-configs/cbootimage-configs.mk new file +4
-0@@ -0,0 +1,128 @@ +################################################################################ +# +# cbootimage-configs +#
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk line 8
Quoted 4 lines
package/cbootimage-configs/cbootimage-configs.mk new file +4
-0+################################################################################ + +CBOOTIMAGE_CONFIGS_VERSION = 9d45319 +CBOOTIMAGE_CONFIGS_SITE = $(call github,avionic-design,cbootimage-configs,$(CBOOTIMAGE_CONFIGS_VERSION))Why do you use the Avionic Design fork and not the original NVIDIA repository, which you reference as the upstream in your Config.in help text?
Just because our configurations are not yet merged to the upstream repository. I really need to push on that, as this package really shall use upstream. Although it might be wise to have the usual custom-repository option like it's done for kernel and u-boot. It's not that unlikely that people would have configs in custom repositories which are not merged upstream.
Quoted 6 lines
Quoted 6 lines
package/cbootimage-configs/cbootimage-configs.mk new file +6
-0+CBOOTIMAGE_CONFIGS_DEPENDENCIES = host-cbootimage uboot +CBOOTIMAGE_CONFIGS_INSTALL_TARGET = NO +CBOOTIMAGE_CONFIGS_INSTALL_IMAGES = YES + +CONFIGS_BUILD = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)) +BCT_NAME = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BCT))
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk line 19
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk new file +5
-0+IMG_NAME = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG)) +EXTRA_BOOTCMD = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_EXTRA_BOOTCMD)) +GPT_FILE = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_GPT_FILE)) +DFU_MAP = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU_MAP)) +KEY_FILE = $(call qstrip,$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY))The variable namespace in Buildroot is global, so they should always be prefixed by the name of the package.
Ok.
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk line 23
Quoted 3 lines
package/cbootimage-configs/cbootimage-configs.mk new file +3
-0+ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS),y) +ifeq ($(CONFIGS_BUILD),) + $(warning BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD=$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD))That's debug stuff.
Yep, already removed in my tree :)
Quoted 24 lines
package/cbootimage-configs/cbootimage-configs.mk line 24
Quoted 1 line
package/cbootimage-configs/cbootimage-configs.mk new file +1
-0+ $(warning CONFIGS_BUILD=$(CONFIGS_BUILD))Ditto.
Quoted 20 lines
package/cbootimage-configs/cbootimage-configs.mk new file +20
-0+ $(error No cbootimage-configuration specififed to build. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD setting) +endif +ifeq ($(BCT_NAME),) + $(error No bct filename specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BCT setting) +endif +ifeq ($(IMG_NAME),) + $(error No img filename specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG setting) +endif +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT),y) +ifeq ($(GPT_FILE),) + $(error No gpt partitions specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_GPT_FILE setting) +endif +endif +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU),y) +ifeq ($(DFU_MAP),) + $(error No dfu map specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU_MAP setting) +endif +endif +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED),y) +ifeq ($(KEY_FILE),)
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk line 49
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk new file +5
-0+ $(error No key file specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY setting) +endif +KEY_OPTS = skb=$(KEY_FILE) +else +KEY_OPTS =Incorrect variable name. Also, I believe this should be separated from the big list of checks.
Yeah, you're probably right, that should be separated.
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk line 52
Quoted 3 lines
package/cbootimage-configs/cbootimage-configs.mk new file +3
-0+endif + +TARGET_SIZE = $(TARGET_CROSS)sizeTo be moved to package/Makefile.in I believe, in a separate patch.
Ok.
Quoted 7 lines
package/cbootimage-configs/cbootimage-configs.mk line 57
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk new file +5
-0+endif + +define CBOOTIMAGE_CONFIGS_BUILD_CMDS + cp $(UBOOT_BUILDDIR)/u-boot-dtb-tegra.bin $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)/u-boot.bin + (cd $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD) && $(HOST_MAKE_ENV) $(HOST_CONFIGURE_OPTS) $(MAKE) $(KEY_OPTS))use $(MAKE) -C $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)
Ack.
Quoted 6 lines
Quoted 6 lines
package/cbootimage-configs/cbootimage-configs.mk new file +6
-0+endef + +# Generate a self-contained u-boot image, flashing u-boot to mmc +define CBOOTIMAGE_CONFIGS_BUILD_FLASHER_CMD + # u-boot bss size: ${BSS_SIZE} + # u-boot-nodtb-tegra.bin size: ${UBOOT_NODTB_TEGRA_SIZE}
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk line 69
Quoted 6 lines
package/cbootimage-configs/cbootimage-configs.mk new file +6
-0+ # u-boot.dtb size: ${UBOOT_DTB_SIZE} + # ${IMG_NAME} size: ${IMG_SIZE} + # u-boot plus dtb size: ${UBOOT_PLUS_DTB_SIZE} + # padded size: ${PADDED_SIZE} + # bootcmd: $(UBOOT_FLASH_ENV) + # gpt-file: $(PWD)/$(GPT_FILE)I don't see the usefulness of this comment.
Debugging. I'll remove it.
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk line 74
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk new file +5
-0+ + cp $(UBOOT_BUILDDIR)/u-boot.dtb ${DTB_FILE} + # Patch environment into u-boot dtb + $(HOST_MAKE_ENV) fdtput -p -t x ${DTB_FILE} '/config' 'bootdelay' '0xfffffffe' + $(HOST_MAKE_ENV) fdtput -p -t s ${DTB_FILE} '/config' 'bootcmd' $(UBOOT_FLASH_ENV)What guarantees you that fdtput is available? You need to depend on host-dtc to be sure that it's available.
Ack, dependency is needed.
Quoted 6 lines
package/cbootimage-configs/cbootimage-configs.mk line 78
Quoted 4 lines
package/cbootimage-configs/cbootimage-configs.mk new file +4
-0+ + # Assemble u-boot-flasher.bin + cp $(UBOOT_BUILDDIR)/u-boot-nodtb-tegra.bin $(BINARIES_DIR)/u-boot-flasher.bin + cat ${DTB_FILE} >> $(BINARIES_DIR)/u-boot-flasher.binUse $() to reference variables. Why don't you do a single cat here ?
Good question. I'll change it to single cat.
Quoted 16 lines
Quoted 16 lines
package/cbootimage-configs/cbootimage-configs.mk new file +16
-0+ truncate -s $(PADDED_SIZE) $(BINARIES_DIR)/u-boot-flasher.bin + cat $(@D)/$(CONFIGS_BUILD)/$(IMG_NAME) >> $(BINARIES_DIR)/u-boot-flasher.bin +endef + +# Computations for assembling u-boot with custom dtb mostly stolen from +# https://github.com/NVIDIA/tegra-uboot-flasher-scripts +# All credits go to Stephen Warren +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_GENERATE_FLASHER),y) +LOADADDR = 0x80108000 # FIXME: this is Tegra124 only +BSS_SIZE = $(shell $(TARGET_SIZE) -A $(UBOOT_BUILDDIR)/u-boot | grep ".bss\b" | tr -s ' ' | cut -d ' ' -f 2) +UBOOT_NODTB_TEGRA_SIZE = $(shell stat -c %s $(UBOOT_BUILDDIR)/u-boot-nodtb-tegra.bin) +UBOOT_DTB_SIZE = $(shell stat -c %s $(UBOOT_BUILDDIR)/u-boot.dtb) +IMG_SIZE = $(shell stat -c %s $(@D)/$(CONFIGS_BUILD)/$(IMG_NAME)) +IMG_SIZE_SECTORS = $(shell echo $$(( $(IMG_SIZE) / 512 )) ) +IMG_SIZE_SECTORS_HEX = $(shell printf '0x%x' $(IMG_SIZE_SECTORS) ) +UBOOT_PLUS_DTB_SIZE = $(shell echo $$(( $(UBOOT_NODTB_TEGRA_SIZE) + $(UBOOT_DTB_SIZE) )) )
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk line 99
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk new file +5
-0+# Keep BSS area clear, acquire an extra 4kb for incereased dtb, align to 4k +# boundary +PADDED_SIZE = $(shell echo $$(( $(UBOOT_PLUS_DTB_SIZE) + $(BSS_SIZE) + (2 * 4 * 1024) - 1 & ~((4 * 1024) - 1) )) ) +IMAGEADDR = $(shell echo $$(( $(LOADADDR) + $(PADDED_SIZE) )) ) +IMAGEADDR_HEX = $(shell printf '0x%x' $(IMAGEADDR) )This is really the messy part. That's clearly way too much stuff encoded into a Buildroot package.
Well, yes, it's the messy part...
Just for the record, this is what it does:
- Take compiled u-boot
- Add a customized dtb providing environment for the actual flashing of cbootimage processed u-boot into mmc
- Append the cbootimage processed u-boot to the binary, so it's self contained
Quoted 4 lines
package/cbootimage-configs/cbootimage-configs.mk line 102
Quoted 2 lines
package/cbootimage-configs/cbootimage-configs.mk new file +2
-0+ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT),y) +PWD = $(shell pwd)Debugging stuff?
Yes, sorry, leftover.
Quoted 4 lines
package/cbootimage-configs/cbootimage-configs.mk line 103
Quoted 1 line
package/cbootimage-configs/cbootimage-configs.mk new file +1
-0+GPT_SPEC = $(shell cat $(GPT_FILE))If it's just a string, why is it the path to a file rather than having direcly the value in the Config.in option?
Yann asked me to move it to a file in his review of the initial patch, as quoting gets rather ugly if provided as Config.in option.
This was the previous default:
default "uuid_disk=\\$$\\{disk_uuid\\};name=rootfs,size=-,uuid=\\$$\\{rootfs_uuid\\}"
And that's just for a single partition, so it becomes really messy if you add some more.
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk line 106
Quoted 3 lines
package/cbootimage-configs/cbootimage-configs.mk new file +3
-0+GPT_BOOTCMD = echo >>> Write partition table to MMC; gpt write mmc 0 '$(GPT_SPEC)'; mmc rescan +else +GPT_BOOTCMD =Not needed, the default for a variable is to be empty.
Also, remember: all variables should be prefixed by the name of the package.
Ack.
Quoted 6 lines
Quoted 6 lines
package/cbootimage-configs/cbootimage-configs.mk new file +6
-0+endif + +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU),y) +DFU_BOOTCMD = echo >>> Enter DFU mode; setenv dfu_alt_info '$(DFU_MAP)'; dfu 0 mmc 0; +else +DFU_BOOTCMD =
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk line 115
Quoted 3 lines
package/cbootimage-configs/cbootimage-configs.mk new file +3
-0+endif + +UBOOT_FLASH_ENV = "echo >>> Write image to MMC; mmc dev 0 1; mmc write ${IMAGEADDR_HEX} 0 ${IMG_SIZE_SECTORS_HEX}; echo >>> Configure environment; env default -f -a; saveenv; $(GPT_BOOTCMD) $(EXTRA_BOOTCMD); $(DFU_BOOTCMD) echo >>> Resetting system; reset"All this U-Boot command construction seem really project-specific, and I don't see why we should hardcode all that stuff in a Buildroot package. Buildroot should give the tools to generate such a U-Boot image, but should not hardcode so much logic IMO.
The hardcoded part of this is not really board specific. All it does, is writing the bootloader to the emmc. While the tegra supports booting from SPI as well, I've never seen a board doing it. So writing to the emmc is what's needed on virtually any tegra based board.
In the end this uboot environment is just the tooling to generate an automated flash writer, as the core tegra flashing tools do only support loading images for execution, but not writing anything directly to memory.
Board specific are only the options to write a partition table and automatically expose it via dfu for flashing root filesystems. I added those as they appear to be rather useful to generate customized, automated flashing solution for the tegra.
Thanks,
Julian
Quoted 8 lines
package/cbootimage-configs/cbootimage-configs.mk line 121
Quoted 5 lines
package/cbootimage-configs/cbootimage-configs.mk new file +5
-0+DTB_FILE = $(@D)/u-boot-flasher.dtb + +# Add the actual assembly hook +CBOOTIMAGE_CONFIGS_POST_INSTALL_IMAGES_HOOKS += CBOOTIMAGE_CONFIGS_BUILD_FLASHER_CMD +endifThomas
PATCH v2 4/4 Add package cbootimage-configs
Hello,
Quoted 6 lines · Julian Scheel
The problem with tegra-uboot-flasher-scripts is, that it does a lot more than what this package does. In fact it includes build logic for all the required tools (like dtc, tegrarcm, cbootimage, u-boot), which wouldn't work out well in the context of buildroot. Using externally provided builds isn't easily possible without major abuse of the scripts, which I don't think would be merged upstream anyway.
Is this something that you can discuss with Stephen Warren? I believe he is generally quite open to suggestions, so maybe he'll be inclined to changing a bit the tegra-uboot-flasher-scripts to make it usable by build systems?
Quoted 8 lines
Quoted 2 lines
Or maybe this thing should do less stuff, and simply provide the necessary tools for a post-image script to do the job.
If in the end it's decided that this is too complicated for the package makefile I'd rather create a dedicated package that handles this. So, that the magic would be in done in some independently maintained script and we only need a buildroot package that heads the options to that package. It just felt a bit like overkill for what it does in the end.
Alternatively, you could have a shell script the package directory itself.
Quoted 10 lines
Quoted 3 lines
Why do you use the Avionic Design fork and not the original NVIDIA repository, which you reference as the upstream in your Config.in help text?
Just because our configurations are not yet merged to the upstream repository. I really need to push on that, as this package really shall use upstream. Although it might be wise to have the usual custom-repository option like it's done for kernel and u-boot. It's not that unlikely that people would have configs in custom repositories which are not merged upstream.
We generally try to not add such options for too many packages, but that remain to be discussed specifically for this package because it indeed contains board-specific stuff.
Quoted 5 lines
Quoted 4 lines
Quoted 1 line
+GPT_SPEC = $(shell cat $(GPT_FILE))
If it's just a string, why is it the path to a file rather than having direcly the value in the Config.in option?
Quoted 8 lines
Yann asked me to move it to a file in his review of the initial patch, as quoting gets rather ugly if provided as Config.in option.
This was the previous default:
default "uuid_disk=\\$$\\{disk_uuid\\};name=rootfs,size=-,uuid=\\$$\\{rootfs_uuid\\}"And that's just for a single partition, so it becomes really messy if you add some more.
OK, makes sense indeed. Please keep the file then. Sorry for not looking at the previous review.
Quoted 10 lines
Quoted 4 lines
All this U-Boot command construction seem really project-specific, and I don't see why we should hardcode all that stuff in a Buildroot package. Buildroot should give the tools to generate such a U-Boot image, but should not hardcode so much logic IMO.
The hardcoded part of this is not really board specific. All it does, is writing the bootloader to the emmc. While the tegra supports booting from SPI as well, I've never seen a board doing it. So writing to the emmc is what's needed on virtually any tegra based board.
Quoted 8 lines
In the end this uboot environment is just the tooling to generate an automated flash writer, as the core tegra flashing tools do only support loading images for execution, but not writing anything directly to memory.
Board specific are only the options to write a partition table and automatically expose it via dfu for flashing root filesystems. I added those as they appear to be rather useful to generate customized, automated flashing solution for the tegra.
Hum, ok. I guess a couple of comments above this logic would be quite useful.
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com