All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCHv2 0/4] Add flashing tools for tegra processors
@ 2016-03-17 15:03 Julian Scheel
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 1/4] Add package cryptopp Julian Scheel
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Julian Scheel @ 2016-03-17 15:03 UTC (permalink / raw)
  To: buildroot

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

-- 
2.7.3

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 1/4] Add package cryptopp
  2016-03-17 15:03 [Buildroot] [PATCHv2 0/4] Add flashing tools for tegra processors Julian Scheel
@ 2016-03-17 15:03 ` Julian Scheel
  2016-04-19 20:23   ` Thomas Petazzoni
  2016-04-21 20:40   ` Thomas Petazzoni
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 2/4] Add package tegrarcm Julian Scheel
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 14+ messages in thread
From: Julian Scheel @ 2016-03-17 15:03 UTC (permalink / raw)
  To: buildroot

Signed-off-by: Julian Scheel <julian@jusst.de>
---
 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

diff --git a/package/cryptopp/cryptopp.hash b/package/cryptopp/cryptopp.hash
new file mode 100644
index 0000000..3af7183
--- /dev/null
+++ b/package/cryptopp/cryptopp.hash
@@ -0,0 +1,2 @@
+# Locally computed
+sha256  9390670a14170dd0f48a6b6b06f74269ef4b056d4718a1a329f6f6069dc957c9  cryptopp563.zip
diff --git a/package/cryptopp/cryptopp.mk b/package/cryptopp/cryptopp.mk
new file mode 100644
index 0000000..cd7c9fa
--- /dev/null
+++ b/package/cryptopp/cryptopp.mk
@@ -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))
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 2/4] Add package tegrarcm
  2016-03-17 15:03 [Buildroot] [PATCHv2 0/4] Add flashing tools for tegra processors Julian Scheel
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 1/4] Add package cryptopp Julian Scheel
@ 2016-03-17 15:03 ` Julian Scheel
  2016-04-19 20:24   ` Thomas Petazzoni
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 3/4] Add package cbootimage Julian Scheel
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 4/4] Add package cbootimage-configs Julian Scheel
  3 siblings, 1 reply; 14+ messages in thread
From: Julian Scheel @ 2016-03-17 15:03 UTC (permalink / raw)
  To: buildroot

Add package for the tegrarcm host utility that allows loading data to tegra
processors in recovery mode.

Signed-off-by: Julian Scheel <julian@jusst.de>
---
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

diff --git a/package/Config.in.host b/package/Config.in.host
index ce1b6bc..05e0644 100644
--- a/package/Config.in.host
+++ b/package/Config.in.host
@@ -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"
 
diff --git a/package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch b/package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch
new file mode 100644
index 0000000..ed0d1fc
--- /dev/null
+++ b/package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch
@@ -0,0 +1,27 @@
+From 0e60af53fa76aa2f274ade98da7ba543147e82c7 Mon Sep 17 00:00:00 2001
+From: Julian Scheel <julian@jusst.de>
+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 <julian@jusst.de>
+---
+ 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
+
diff --git a/package/tegrarcm/Config.in.host b/package/tegrarcm/Config.in.host
new file mode 100644
index 0000000..47590fa
--- /dev/null
+++ b/package/tegrarcm/Config.in.host
@@ -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
diff --git a/package/tegrarcm/tegrarcm.hash b/package/tegrarcm/tegrarcm.hash
new file mode 100644
index 0000000..4194a4c
--- /dev/null
+++ b/package/tegrarcm/tegrarcm.hash
@@ -0,0 +1,2 @@
+# Locally computed
+sha256  538cb0af237ab33e070d3aeb6cc828cd7ef453753ba2ccc21b87ed43faac51bd  tegrarcm-v1.7.tar.gz
diff --git a/package/tegrarcm/tegrarcm.mk b/package/tegrarcm/tegrarcm.mk
new file mode 100644
index 0000000..5dbc483
--- /dev/null
+++ b/package/tegrarcm/tegrarcm.mk
@@ -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))
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 3/4] Add package cbootimage
  2016-03-17 15:03 [Buildroot] [PATCHv2 0/4] Add flashing tools for tegra processors Julian Scheel
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 1/4] Add package cryptopp Julian Scheel
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 2/4] Add package tegrarcm Julian Scheel
@ 2016-03-17 15:03 ` Julian Scheel
  2016-04-19 20:34   ` Thomas Petazzoni
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 4/4] Add package cbootimage-configs Julian Scheel
  3 siblings, 1 reply; 14+ messages in thread
From: Julian Scheel @ 2016-03-17 15:03 UTC (permalink / raw)
  To: buildroot

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 <julian@jusst.de>
---
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

diff --git a/package/Config.in.host b/package/Config.in.host
index 05e0644..4eb62d6 100644
--- a/package/Config.in.host
+++ b/package/Config.in.host
@@ -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"
diff --git a/package/cbootimage/Config.in.host b/package/cbootimage/Config.in.host
new file mode 100644
index 0000000..5f6a93d
--- /dev/null
+++ b/package/cbootimage/Config.in.host
@@ -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
diff --git a/package/cbootimage/cbootimage.hash b/package/cbootimage/cbootimage.hash
new file mode 100644
index 0000000..96cf172
--- /dev/null
+++ b/package/cbootimage/cbootimage.hash
@@ -0,0 +1,2 @@
+# Locally computed
+sha256 373c108d7b6778c62a33e59ad0cd5ea9ebb379319a0c8b4cf469eaa8bec5521b  cbootimage-v1.7.tar.gz
diff --git a/package/cbootimage/cbootimage.mk b/package/cbootimage/cbootimage.mk
new file mode 100644
index 0000000..20b17dd
--- /dev/null
+++ b/package/cbootimage/cbootimage.mk
@@ -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))
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 4/4] Add package cbootimage-configs
  2016-03-17 15:03 [Buildroot] [PATCHv2 0/4] Add flashing tools for tegra processors Julian Scheel
                   ` (2 preceding siblings ...)
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 3/4] Add package cbootimage Julian Scheel
@ 2016-03-17 15:03 ` Julian Scheel
  2016-04-19 21:08   ` Thomas Petazzoni
  3 siblings, 1 reply; 14+ messages in thread
From: Julian Scheel @ 2016-03-17 15:03 UTC (permalink / raw)
  To: buildroot

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 <julian@jusst.de>
---
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

diff --git a/package/Config.in b/package/Config.in
index bdc3063..9977190 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -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"
diff --git a/package/cbootimage-configs/Config.in b/package/cbootimage-configs/Config.in
new file mode 100644
index 0000000..6268740
--- /dev/null
+++ b/package/cbootimage-configs/Config.in
@@ -0,0 +1,122 @@
+menuconfig BR2_PACKAGE_CBOOTIMAGE_CONFIGS
+	bool "cbootimage-configs"
+	select BR2_PACKAGE_HOST_CBOOTIMAGE
+	depends on BR2_TARGET_UBOOT
+	help
+	  The cbootimage-configs project contains cbootimage configuration
+	  files for many Tegra boards, both those designed by NVIDIA, and
+	  various third-parties.
+
+	  Needs BR2_TARGET_UBOOT_FORMAT_CUSTOM_NAME to be u-boot-dtb-tegra.bin
+	  or {u-boot,u-boot-dtb-tegra.bin,u-boot-nodtb-tegra.bin,u-boot.dtb}"
+
+	  https://github.com/NVIDIA/cbootimage-configs
+
+
+if BR2_PACKAGE_CBOOTIMAGE_CONFIGS
+
+config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD
+	string "cbootimage board to build"
+	help
+	  The full path of a configuration inside cbootimage-configs that is
+	  to be built for the board you want to run. File directory structure
+	  usually is <soc>/<vendor>/<board>
+	  Example for Jetson TK1:
+	  tegra124/nvidia/jetson-tk1
+
+config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BCT
+	string "cbootimage bct filename"
+	help
+	  The name of the bct generated by cbootimage for the selected
+	  cbootimage configuration. If unsure look it up from the Makefile in
+	  the specified board directory.
+
+config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG
+	string "cbootimage img filename"
+	help
+	  The name of the image generated by cbootimage for the selected
+	  cbootimage configuration. If unsure look it up from the Makefile in
+	  the specified board directory.
+
+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
+	string "pkc file name"
+	depends on BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_SIGNED
+	help
+	  Key file to use for signing the loader image.
+
+config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_GENERATE_FLASHER
+	bool "generate self-contained u-boot flasher"
+	help
+	  Generate an image consisting of a u-boot to flash the actual target
+	  u-boot. For this a u-boot with a customized environment is generated
+	  that contains a bootcmd to copy a target image into mmc. The actual
+	  target u-boot, which is specified in BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG is
+	  appended to that image so that one self-contained image is generated
+	  that can be flashed with tegrarcm.
+
+	  Needs BR2_TARGET_UBOOT_FORMAT_CUSTOM_NAME to be
+	  {u-boot,u-boot-dtb-tegra.bin,u-boot-nodtb-tegra.bin,u-boot.dtb}
+
+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"
+	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\}
+
+config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU
+	bool "start dfu mode before reset"
+	help
+	  Put the device in DFU mode on usb 0 and mmc 0, so that filesystem
+	  images can be flashed to the device instantly.
+
+config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU_MAP
+	string "dfu map"
+	depends on BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_DFU
+	default "rootfs part 0 1"
+	help
+	  Mapping scheme for partitions to DFU unit names according to u-boot
+	  documentation README.dfutftp variable dfu_alt_info.
+
+	  Example:
+	  rootfs part 0 1;data part 0 2
+
+config BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_EXTRA_BOOTCMD
+	string "u-boot flasher bootcmd extras"
+	help
+	  Specify extra bootcmds you want the u-boot flasher instance to run
+	  before resetting the board. For example this could be writing a
+	  partition table and enter DFU flashing mode to write filesystem
+	  images.
+
+endif
+
+endif
diff --git a/package/cbootimage-configs/cbootimage-configs.hash b/package/cbootimage-configs/cbootimage-configs.hash
new file mode 100644
index 0000000..a4fd518
--- /dev/null
+++ b/package/cbootimage-configs/cbootimage-configs.hash
@@ -0,0 +1,2 @@
+# Locally computed
+sha256  f9eaa2e88fa24ea348d6358f18e4bab58ac21c586b5b6cb5300e21a6e1d9de83  cbootimage-configs-9d45319.tar.gz
diff --git a/package/cbootimage-configs/cbootimage-configs.mk b/package/cbootimage-configs/cbootimage-configs.mk
new file mode 100644
index 0000000..69d6d29
--- /dev/null
+++ b/package/cbootimage-configs/cbootimage-configs.mk
@@ -0,0 +1,128 @@
+################################################################################
+#
+# cbootimage-configs
+#
+################################################################################
+
+CBOOTIMAGE_CONFIGS_VERSION = 9d45319
+CBOOTIMAGE_CONFIGS_SITE = $(call github,avionic-design,cbootimage-configs,$(CBOOTIMAGE_CONFIGS_VERSION))
+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))
+
+ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS),y)
+ifeq ($(CONFIGS_BUILD),)
+	$(warning BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD=$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD))
+	$(warning CONFIGS_BUILD=$(CONFIGS_BUILD))
+	$(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),)
+	$(error No key file specified. Check your BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG_KEY setting)
+endif
+KEY_OPTS = skb=$(KEY_FILE)
+else
+KEY_OPTS =
+endif
+
+TARGET_SIZE = $(TARGET_CROSS)size
+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))
+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)
+
+	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)
+
+	# 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
+	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) )) )
+# 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) )
+
+ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT),y)
+PWD = $(shell pwd)
+GPT_SPEC = $(shell cat $(GPT_FILE))
+GPT_BOOTCMD = echo >>> Write partition table to MMC; gpt write mmc 0 '$(GPT_SPEC)'; mmc rescan
+else
+GPT_BOOTCMD =
+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"
+
+DTB_FILE = $(@D)/u-boot-flasher.dtb
+
+# Add the actual assembly hook
+CBOOTIMAGE_CONFIGS_POST_INSTALL_IMAGES_HOOKS += CBOOTIMAGE_CONFIGS_BUILD_FLASHER_CMD
+endif
+
+define CBOOTIMAGE_CONFIGS_INSTALL_IMAGES_CMDS
+	cp -dpf $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BCT) $(BINARIES_DIR)
+	cp -dpf $(@D)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD)/$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_IMG) $(BINARIES_DIR)
+endef
+
+$(eval $(generic-package))
diff --git a/package/cbootimage-configs/gpt-table b/package/cbootimage-configs/gpt-table
new file mode 100644
index 0000000..9a2dc18
--- /dev/null
+++ b/package/cbootimage-configs/gpt-table
@@ -0,0 +1 @@
+uuid_disk=\$\{disk_uuid\};name=rootfs,size=-,uuid=\$\{rootfs_uuid\}
-- 
2.7.3

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 1/4] Add package cryptopp
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 1/4] Add package cryptopp Julian Scheel
@ 2016-04-19 20:23   ` Thomas Petazzoni
  2016-04-21 20:40   ` Thomas Petazzoni
  1 sibling, 0 replies; 14+ messages in thread
From: Thomas Petazzoni @ 2016-04-19 20:23 UTC (permalink / raw)
  To: buildroot

Hello,

On Thu, 17 Mar 2016 16:03:55 +0100, Julian Scheel wrote:

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

> +	$(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.

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

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

> +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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 2/4] Add package tegrarcm
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 2/4] Add package tegrarcm Julian Scheel
@ 2016-04-19 20:24   ` Thomas Petazzoni
  0 siblings, 0 replies; 14+ messages in thread
From: Thomas Petazzoni @ 2016-04-19 20:24 UTC (permalink / raw)
  To: buildroot

Hello,

The commit title should have been:

	tegrarcm: new package

On Thu, 17 Mar 2016 16:03:56 +0100, Julian Scheel wrote:

> diff --git a/package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch b/package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch
> new file mode 100644
> index 0000000..ed0d1fc
> --- /dev/null
> +++ b/package/tegrarcm/0001-Make-cryptopp-include-crosscompile-compatible.patch
> @@ -0,0 +1,27 @@
> +From 0e60af53fa76aa2f274ade98da7ba543147e82c7 Mon Sep 17 00:00:00 2001
> +From: Julian Scheel <julian@jusst.de>
> +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 <julian@jusst.de>
> +---
> + 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)

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:

  https://github.com/NVIDIA/tegrarcm/pull/2

> diff --git a/package/tegrarcm/tegrarcm.mk b/package/tegrarcm/tegrarcm.mk
> new file mode 100644
> index 0000000..5dbc483
> --- /dev/null
> +++ b/package/tegrarcm/tegrarcm.mk
> @@ -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)

Due to the patch being changed, I've removed this last line, and
applied.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 3/4] Add package cbootimage
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 3/4] Add package cbootimage Julian Scheel
@ 2016-04-19 20:34   ` Thomas Petazzoni
  0 siblings, 0 replies; 14+ messages in thread
From: Thomas Petazzoni @ 2016-04-19 20:34 UTC (permalink / raw)
  To: buildroot

Hello,

On Thu, 17 Mar 2016 16:03:57 +0100, Julian Scheel wrote:
> 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 <julian@jusst.de>

The commit title should have been:

	cbootimage: new package


> +	  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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 4/4] Add package cbootimage-configs
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 4/4] Add package cbootimage-configs Julian Scheel
@ 2016-04-19 21:08   ` Thomas Petazzoni
  2016-04-19 22:36     ` Arnout Vandecappelle
  2016-04-20  6:18     ` Julian Scheel
  0 siblings, 2 replies; 14+ messages in thread
From: Thomas Petazzoni @ 2016-04-19 21:08 UTC (permalink / raw)
  To: buildroot

Hello,

On Thu, 17 Mar 2016 16:03:58 +0100, Julian Scheel wrote:
> 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 <julian@jusst.de>

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.



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


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

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

> diff --git a/package/cbootimage-configs/cbootimage-configs.hash b/package/cbootimage-configs/cbootimage-configs.hash
> new file mode 100644
> index 0000000..a4fd518
> --- /dev/null
> +++ b/package/cbootimage-configs/cbootimage-configs.hash
> @@ -0,0 +1,2 @@
> +# Locally computed
> +sha256  f9eaa2e88fa24ea348d6358f18e4bab58ac21c586b5b6cb5300e21a6e1d9de83  cbootimage-configs-9d45319.tar.gz
> diff --git a/package/cbootimage-configs/cbootimage-configs.mk b/package/cbootimage-configs/cbootimage-configs.mk
> new file mode 100644
> index 0000000..69d6d29
> --- /dev/null
> +++ b/package/cbootimage-configs/cbootimage-configs.mk
> @@ -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?

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

> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS),y)
> +ifeq ($(CONFIGS_BUILD),)
> +	$(warning BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD=$(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_BUILD))

That's debug stuff.

> +	$(warning CONFIGS_BUILD=$(CONFIGS_BUILD))

Ditto.

> +	$(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),)
> +	$(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.

> +endif
> +
> +TARGET_SIZE = $(TARGET_CROSS)size

To be moved to package/Makefile.in I believe, in a separate patch.

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

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

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

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

> +	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) )) )
> +# 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.

> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT),y)
> +PWD = $(shell pwd)

Debugging stuff?

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

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

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

> +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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 4/4] Add package cbootimage-configs
  2016-04-19 21:08   ` Thomas Petazzoni
@ 2016-04-19 22:36     ` Arnout Vandecappelle
  2016-04-20  6:18     ` Julian Scheel
  1 sibling, 0 replies; 14+ messages in thread
From: Arnout Vandecappelle @ 2016-04-19 22:36 UTC (permalink / raw)
  To: buildroot

On 04/19/16 23:08, Thomas Petazzoni wrote:
> Hello,
>
> On Thu, 17 Mar 2016 16:03:58 +0100, Julian Scheel wrote:
[snip]
>> >+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
-- 
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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 4/4] Add package cbootimage-configs
  2016-04-19 21:08   ` Thomas Petazzoni
  2016-04-19 22:36     ` Arnout Vandecappelle
@ 2016-04-20  6:18     ` Julian Scheel
  2016-04-20  7:11       ` Thomas Petazzoni
  1 sibling, 1 reply; 14+ messages in thread
From: Julian Scheel @ 2016-04-20  6:18 UTC (permalink / raw)
  To: buildroot

Hi Thomas,

thanks for the review, see comments below.

On 19.04.2016 23:08, Thomas Petazzoni wrote:
> Hello,
>
> On Thu, 17 Mar 2016 16:03:58 +0100, Julian Scheel wrote:
>> 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 <julian@jusst.de>
>
> 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.

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

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

> I've added some more comments/questions below.
>
>
>
>> +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.

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

>> +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"
>> +	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.
>
>> diff --git a/package/cbootimage-configs/cbootimage-configs.hash b/package/cbootimage-configs/cbootimage-configs.hash
>> new file mode 100644
>> index 0000000..a4fd518
>> --- /dev/null
>> +++ b/package/cbootimage-configs/cbootimage-configs.hash
>> @@ -0,0 +1,2 @@
>> +# Locally computed
>> +sha256  f9eaa2e88fa24ea348d6358f18e4bab58ac21c586b5b6cb5300e21a6e1d9de83  cbootimage-configs-9d45319.tar.gz
>> diff --git a/package/cbootimage-configs/cbootimage-configs.mk b/package/cbootimage-configs/cbootimage-configs.mk
>> new file mode 100644
>> index 0000000..69d6d29
>> --- /dev/null
>> +++ b/package/cbootimage-configs/cbootimage-configs.mk
>> @@ -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?

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.

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

Ok.

>> +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 :)

>> +	$(warning CONFIGS_BUILD=$(CONFIGS_BUILD))
>
> Ditto.
>
>> +	$(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),)
>> +	$(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.

>> +endif
>> +
>> +TARGET_SIZE = $(TARGET_CROSS)size
>
> To be moved to package/Makefile.in I believe, in a separate patch.

Ok.

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

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

Debugging. I'll remove it.

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

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

Good question. I'll change it to single cat.

>> +	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) )) )
>> +# 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

>> +ifeq ($(BR2_PACKAGE_CBOOTIMAGE_CONFIGS_FLASHER_CREATE_GPT),y)
>> +PWD = $(shell pwd)
>
> Debugging stuff?

Yes, sorry, leftover.

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

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

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

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

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 4/4] Add package cbootimage-configs
  2016-04-20  6:18     ` Julian Scheel
@ 2016-04-20  7:11       ` Thomas Petazzoni
  0 siblings, 0 replies; 14+ messages in thread
From: Thomas Petazzoni @ 2016-04-20  7:11 UTC (permalink / raw)
  To: buildroot

Hello,

On Wed, 20 Apr 2016 08:18:00 +0200, Julian Scheel wrote:

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

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

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


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

OK, makes sense indeed. Please keep the file then. Sorry for not
looking at the previous review.

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

Hum, ok. I guess a couple of comments above this logic would be quite
useful.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 1/4] Add package cryptopp
  2016-03-17 15:03 ` [Buildroot] [PATCHv2 1/4] Add package cryptopp Julian Scheel
  2016-04-19 20:23   ` Thomas Petazzoni
@ 2016-04-21 20:40   ` Thomas Petazzoni
  2016-04-22  7:29     ` Julian Scheel
  1 sibling, 1 reply; 14+ messages in thread
From: Thomas Petazzoni @ 2016-04-21 20:40 UTC (permalink / raw)
  To: buildroot

Hello,

On Thu, 17 Mar 2016 16:03:55 +0100, Julian Scheel wrote:
> Signed-off-by: Julian Scheel <julian@jusst.de>
> ---
>  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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [Buildroot] [PATCHv2 1/4] Add package cryptopp
  2016-04-21 20:40   ` Thomas Petazzoni
@ 2016-04-22  7:29     ` Julian Scheel
  0 siblings, 0 replies; 14+ messages in thread
From: Julian Scheel @ 2016-04-22  7:29 UTC (permalink / raw)
  To: buildroot

Hi Thomas,

On 21.04.2016 22:40, Thomas Petazzoni wrote:
> On Thu, 17 Mar 2016 16:03:55 +0100, Julian Scheel wrote:
>> Signed-off-by: Julian Scheel <julian@jusst.de>
>> ---
>>   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?

Should be fixed by:
[PATCH] package/cryptopp: Backport gcc compatibility patch

-Julian

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-04-22  7:29 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-17 15:03 [Buildroot] [PATCHv2 0/4] Add flashing tools for tegra processors Julian Scheel
2016-03-17 15:03 ` [Buildroot] [PATCHv2 1/4] Add package cryptopp Julian Scheel
2016-04-19 20:23   ` Thomas Petazzoni
2016-04-21 20:40   ` Thomas Petazzoni
2016-04-22  7:29     ` Julian Scheel
2016-03-17 15:03 ` [Buildroot] [PATCHv2 2/4] Add package tegrarcm Julian Scheel
2016-04-19 20:24   ` Thomas Petazzoni
2016-03-17 15:03 ` [Buildroot] [PATCHv2 3/4] Add package cbootimage Julian Scheel
2016-04-19 20:34   ` Thomas Petazzoni
2016-03-17 15:03 ` [Buildroot] [PATCHv2 4/4] Add package cbootimage-configs Julian Scheel
2016-04-19 21:08   ` Thomas Petazzoni
2016-04-19 22:36     ` Arnout Vandecappelle
2016-04-20  6:18     ` Julian Scheel
2016-04-20  7:11       ` Thomas Petazzoni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.