Hi Andreas,

On 15 June 2016 at 13:26, Andreas Dannenberg dannenberg@ti.com wrote:

From: Daniel Allred d-allred@ti.com

The next stage boot loader image and the selected FDT can be post-processed by board/platform/device-specific code, which can include modifying the size and altering the starting source address before copying these binary blobs to their final desitination. This might be desired to do things like strip headers or footers attached to the images before they were packeaged into the FIT, or to perform operations such as decryption or authentication.

Signed-off-by: Daniel Allred d-allred@ti.com Signed-off-by: Andreas Dannenberg dannenberg@ti.com

---

common/spl/spl_fit.c | 21 ++++++++++++++++----- include/image.h | 15 +++++++++++++++ 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/common/spl/spl_fit.c b/common/spl/spl_fit.c index 9874708..ecbcb97 100644 --- a/common/spl/spl_fit.c +++ b/common/spl/spl_fit.c @@ -121,6 +121,10 @@ static int get_aligned_image_size(struct spl_load_info *info, int data_size, return (data_size + info->bl_len - 1) / info->bl_len; }

+void __weak board_fit_image_post_process(void **p_src, size_t *p_size) +{ +}

Instead of making this weak, can you add a Kconfig option to enable this feature?

int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit) { int sectors; @@ -132,7 +136,7 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit) int data_offset, data_size; int base_offset, align_len = ARCH_DMA_MINALIGN - 1; int src_sector;

•   void *dst;
  

•   void *dst, *src;
  
    /*
     * Figure out where the external images start. This is the base for the
  

@@ -206,8 +210,11 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit) return -EIO; debug("image: dst=%p, data_offset=%x, size=%x\n", dst, data_offset, data_size);

•   memcpy(dst, dst + get_aligned_image_overhead(info, data_offset),
  

•          data_size);
  

•   src = dst + get_aligned_image_overhead(info, data_offset);
  

•   board_fit_image_post_process((void **)&src, (size_t *)&data_size);
  

•   memcpy(dst, src, data_size);
  
    /* Figure out which device tree the board wants to use */
    fdt_len = spl_fit_select_fdt(fit, images, &fdt_offset);
  

@@ -236,8 +243,12 @@ int spl_load_simple_fit(struct spl_load_info *info, ulong sector, void *fit) */ debug("fdt: dst=%p, data_offset=%x, size=%x\n", dst, fdt_offset, fdt_len);

•   memcpy(load_ptr + data_size,
  

•          dst + get_aligned_image_overhead(info, fdt_offset), fdt_len);
  

•   src = dst + get_aligned_image_overhead(info, fdt_offset);
  

•   dst = load_ptr + data_size;
  

•   board_fit_image_post_process((void **)&src, (size_t *)&fdt_len);
  

•   memcpy(dst, src, fdt_len);
  
    return 0;
  

} diff --git a/include/image.h b/include/image.h index a8f6bd1..9536874 100644 --- a/include/image.h +++ b/include/image.h @@ -1172,4 +1172,19 @@ ulong android_image_get_kload(const struct andr_img_hdr *hdr); */ int board_fit_config_name_match(const char *name);

+/**

• • board_fit_image_post_process() - Do any post-process on FIT binary data
• • This is used to do any sort of image manipulation, verification, decryption
• • etc. in a platform or board specific way. Obviously, anything done here would
• • need to be comprehended in how the images were prepared before being injected
• • into the FIT creation (i.e. the binary blobs would have been pre-processed
• • before being added to the FIT image).
• • @image: pointer to the image start pointer
• • @size: pointer to the image size
• • @return no return value (failure should be handled internally)
• */

+void board_fit_image_post_process(void **p_image, size_t *p_size);

#endif /* __IMAGE_H__ */

2.6.4

Regards, Simon

On Thursday 16 June 2016 12:56 AM, Andreas Dannenberg wrote:

From: Daniel Allred d-allred@ti.com

Adds a generic C-callable API for making secure ROM calls on OMAP and OMAP-compatible devices. This API provides the important function of flushing the ROM call arguments to memory from the cache, so that the secure world will have a coherent view of those arguments. Then is simply calls the omap_smc_sec routine.

Signed-off-by: Daniel Allred d-allred@ti.com Signed-off-by: Andreas Dannenberg dannenberg@ti.com

---

arch/arm/cpu/armv7/omap-common/Makefile | 4 +++ arch/arm/cpu/armv7/omap-common/sec_bridge.c | 47 +++++++++++++++++++++++++++++ arch/arm/include/asm/omap_common.h | 4 +++ 3 files changed, 55 insertions(+) create mode 100644 arch/arm/cpu/armv7/omap-common/sec_bridge.c

diff --git a/arch/arm/cpu/armv7/omap-common/Makefile b/arch/arm/cpu/armv7/omap-common/Makefile index 87a7ac0..4fc3926 100644 --- a/arch/arm/cpu/armv7/omap-common/Makefile +++ b/arch/arm/cpu/armv7/omap-common/Makefile @@ -28,6 +28,10 @@ obj-y += pipe3-phy.o obj-$(CONFIG_SCSI_AHCI_PLAT) += sata.o endif

+ifneq ($(CONFIG_TI_SECURE_DEVICE),) +obj-y += sec_bridge.o +endif

can we use: obj-$(CONFIG_TI_SECURE_DEVICE) += sec_bridge.o ?

ifeq ($(CONFIG_SYS_DCACHE_OFF),) obj-y += omap-cache.o endif diff --git a/arch/arm/cpu/armv7/omap-common/sec_bridge.c b/arch/arm/cpu/armv7/omap-common/sec_bridge.c new file mode 100644 index 0000000..4eaba8e --- /dev/null +++ b/arch/arm/cpu/armv7/omap-common/sec_bridge.c @@ -0,0 +1,47 @@ +/*

• • Common bridge function to make OMAP secure ROM calls
• • (C) Copyright 2016
• • Texas Instruments, <www.ti.com>
• • Daniel Allred d-allred@ti.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <stdarg.h>

+#include <asm/arch/sys_proto.h> +#include <asm/omap_common.h>

+static uint32_t secure_rom_call_args[5] __aligned(ARCH_DMA_MINALIGN);

+u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...) +{

• int i;
• u32 num_args;
• va_list ap;
• va_start(ap, flag);
• num_args = va_arg(ap, u32);

Is there a cap on the num_args? can you add a check for that?

• /* Copy args to aligned args structure */
• for (i = 0; i < num_args; i++)

• secure_rom_call_args[i + 1] = va_arg(ap, u32);
  

• secure_rom_call_args[0] = num_args;
• va_end(ap);
• /* if data cache is enabled, flush the aligned args structure */

+#ifndef CONFIG_SYS_DCACHE_OFF

• flush_dcache_range(

• (unsigned int)&secure_rom_call_args[0],
  

• (unsigned int)&secure_rom_call_args[0] +
  

• roundup(sizeof(secure_rom_call_args), ARCH_DMA_MINALIGN));
  

+#endif

I guess you do not need #ifndef here. Patch 2 should take care of it.

Thanks and regards, Lokesh

• return omap_smc_sec(service, proc_id, flag, secure_rom_call_args);

+} diff --git a/arch/arm/include/asm/omap_common.h b/arch/arm/include/asm/omap_common.h index 5943e6f..cb02c88 100644 --- a/arch/arm/include/asm/omap_common.h +++ b/arch/arm/include/asm/omap_common.h @@ -629,6 +629,10 @@ void omap_smc1(u32 service, u32 val);

u32 omap_smc_sec(u32 service, u32 proc_id, u32 flag, u32 *params);

+#ifdef CONFIG_TI_SECURE_DEVICE +u32 secure_rom_call(u32 service, u32 proc_id, u32 flag, ...); +#endif

void enable_edma3_clocks(void); void disable_edma3_clocks(void);

On 15 June 2016 at 13:26, Andreas Dannenberg dannenberg@ti.com wrote:

From: Daniel Allred d-allred@ti.com

Adds an API that verifies a signature attached to an image (binary blob). This API is basically a entry to a secure ROM service provided by the device and accessed via an SMC call, using a particular calling convention.

Signed-off-by: Daniel Allred d-allred@ti.com Signed-off-by: Andreas Dannenberg dannenberg@ti.com

---

arch/arm/cpu/armv7/omap5/Makefile | 1 + arch/arm/cpu/armv7/omap5/sec_fxns.c | 70 +++++++++++++++++++++++++++++ arch/arm/include/asm/arch-omap5/sys_proto.h | 4 ++ 3 files changed, 75 insertions(+) create mode 100644 arch/arm/cpu/armv7/omap5/sec_fxns.c

Reviewed-by: Simon Glass sjg@chromium.org

Please see below.

diff --git a/arch/arm/cpu/armv7/omap5/Makefile b/arch/arm/cpu/armv7/omap5/Makefile index 3caba86..d373bf4 100644 --- a/arch/arm/cpu/armv7/omap5/Makefile +++ b/arch/arm/cpu/armv7/omap5/Makefile @@ -14,3 +14,4 @@ obj-y += hw_data.o obj-y += abb.o obj-y += fdt.o obj-$(CONFIG_IODELAY_RECALIBRATION) += dra7xx_iodelay.o +obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o diff --git a/arch/arm/cpu/armv7/omap5/sec_fxns.c b/arch/arm/cpu/armv7/omap5/sec_fxns.c new file mode 100644 index 0000000..766333a --- /dev/null +++ b/arch/arm/cpu/armv7/omap5/sec_fxns.c @@ -0,0 +1,70 @@ +/*

• • Common security functions that rely on secure ROM services
• • (C) Copyright 2016
• • Texas Instruments, <www.ti.com>
• • Daniel Allred d-allred@ti.com
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <asm/arch/sys_proto.h> +#include <asm/omap_common.h>

+#define SIGNATURE_LENGTH (0x118)

+/* API Index for OMAP5, DRA7xx */ +#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX (0x0000000E)

+int secure_boot_verify_image(void **image, size_t *size) +{

•   int result = 1;
  

•   u32 cert_addr, sig_addr;
  

•   size_t cert_size;
  

+#ifndef CONFIG_SYS_DCACHE_OFF

•   /* Perform cache writeback on input buffer */
  

•   flush_dcache_range(
  

•           (u32)*image,
  

•           (u32)*image + roundup(*size, ARCH_DMA_MINALIGN));
  

+#endif

•   cert_addr = (uint32_t)*image;
  

•   *size -= SIGNATURE_LENGTH;   /* Subtract out the signature size */
  

•   cert_size = *size;
  

•   sig_addr = cert_addr + cert_size;
  

•   /* Check if image load address is 32-bit aligned */
  

•   if (0 != (0x3 & cert_addr)) {
  

•           puts("Image is not 4-byte aligned.\n");
  

•           result = 1;
  

•           goto auth_exit;
  

•   }
  

•   /* Image size also should be multiple of 4 */
  

•   if (0 != (0x3 & cert_size)) {
  

•           puts("Image size is not 4-byte aligned.\n");
  

•           result = 1;
  

•           goto auth_exit;
  

•   }
  

•   /* Call ROM HAL API to verify certificate signature */
  

•   debug("%s: load_addr = %x, size = %x, sig_addr = %x\n", __func__,
  

•         cert_addr, cert_size, sig_addr);
  

•   result = secure_rom_call(
  

•           API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX, 0, 0,
  

•           4, cert_addr, cert_size, sig_addr, 0xFFFFFFFF);
  

+auth_exit:

•   if (result != 0) {
  

•           puts("Authentication failed!\n");
  

•           printf("Return Value = %08X\n", result);
  

•           hang();
  

•   }
  

•   printf("Authentication passed: %s\n", (char *)sig_addr);
  

•   return result;
  

+} diff --git a/arch/arm/include/asm/arch-omap5/sys_proto.h b/arch/arm/include/asm/arch-omap5/sys_proto.h index ab0e7fa..b175124 100644 --- a/arch/arm/include/asm/arch-omap5/sys_proto.h +++ b/arch/arm/include/asm/arch-omap5/sys_proto.h @@ -84,4 +84,8 @@ static inline u32 usec_to_32k(u32 usec) #define OMAP5_SERVICE_L2ACTLR_SET 0x104 #define OMAP5_SERVICE_ACR_SET 0x107

+#ifdef CONFIG_TI_SECURE_DEVICE +int secure_boot_verify_image(void **p_image, size_t *p_size);

Function comment please.

+#endif

#endif

2.6.4

On Thursday 16 June 2016 12:56 AM, Andreas Dannenberg wrote:

From: Daniel Allred d-allred@ti.com

Adds a board specific FIT image post processing function for when CONFIG_SECURE_BOOT is defined. Also update the omap common config header to enable CONFIG_SECURE_BOOT always for secure TI devices (CONFIG_TI_SECURE_DEVICE is defined).

Signed-off-by: Daniel Allred d-allred@ti.com Signed-off-by: Andreas Dannenberg dannenberg@ti.com

---

board/ti/am57xx/board.c | 7 +++++++ board/ti/dra7xx/evm.c | 7 +++++++ include/configs/ti_omap5_common.h | 4 ++++ 3 files changed, 18 insertions(+)

diff --git a/board/ti/am57xx/board.c b/board/ti/am57xx/board.c index 08cf14d..a9635c2 100644 --- a/board/ti/am57xx/board.c +++ b/board/ti/am57xx/board.c @@ -750,3 +750,10 @@ int board_fit_config_name_match(const char *name) return -1; } #endif

+#ifdef CONFIG_SECURE_BOOT +void board_fit_image_post_process(void **p_image, size_t *p_size) +{

• secure_boot_verify_image(p_image, p_size);

+} +#endif diff --git a/board/ti/dra7xx/evm.c b/board/ti/dra7xx/evm.c index 3fbbc9b..03eefb6 100644 --- a/board/ti/dra7xx/evm.c +++ b/board/ti/dra7xx/evm.c @@ -739,3 +739,10 @@ int board_fit_config_name_match(const char *name) return -1; } #endif

+#ifdef CONFIG_SECURE_BOOT +void board_fit_image_post_process(void **p_image, size_t *p_size) +{

• secure_boot_verify_image(p_image, p_size);

+} +#endif diff --git a/include/configs/ti_omap5_common.h b/include/configs/ti_omap5_common.h index 2e4c8e9..9db6da2 100644 --- a/include/configs/ti_omap5_common.h +++ b/include/configs/ti_omap5_common.h @@ -138,6 +138,10 @@

• print some information.

*/ #ifdef CONFIG_TI_SECURE_DEVICE

+/* Always enforce for secure devices */ +#define CONFIG_SECURE_BOOT

Can you make this a Kconfig option?

You are enabling it for GP devices as well. What happens in GP devices?

Thanks and regards, Lokesh

/*

• For memory booting on HS parts, the first 4KB of the internal RAM is
• reserved for secure world use and the flash loader image is

Hi Andreas,

On 15 June 2016 at 13:26, Andreas Dannenberg dannenberg@ti.com wrote:

From: Madan Srinivas madans@ti.com

Adds an API that verifies a signature attached to an image (binary blob). This API is basically a entry to a secure ROM service provided by the device and accessed via an SMC call, using a particular calling convention. This API is common across AM3x HS and AM4x HS devices.

Signed-off-by: Madan Srinivas madans@ti.com Signed-off-by: Andreas Dannenberg dannenberg@ti.com

---

arch/arm/cpu/armv7/am33xx/Makefile | 2 + arch/arm/cpu/armv7/am33xx/sec_fxns.c | 90 ++++++++++++++++++++++++++++ arch/arm/include/asm/arch-am33xx/sys_proto.h | 6 +- 3 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 arch/arm/cpu/armv7/am33xx/sec_fxns.c

diff --git a/arch/arm/cpu/armv7/am33xx/Makefile b/arch/arm/cpu/armv7/am33xx/Makefile index 6fda482..d2b3e37 100644 --- a/arch/arm/cpu/armv7/am33xx/Makefile +++ b/arch/arm/cpu/armv7/am33xx/Makefile @@ -20,3 +20,5 @@ obj-y += board.o obj-y += mux.o

obj-$(CONFIG_CLOCK_SYNTHESIZER) += clk_synthesizer.o

+obj-$(CONFIG_TI_SECURE_DEVICE) += sec_fxns.o diff --git a/arch/arm/cpu/armv7/am33xx/sec_fxns.c b/arch/arm/cpu/armv7/am33xx/sec_fxns.c new file mode 100644 index 0000000..560297c --- /dev/null +++ b/arch/arm/cpu/armv7/am33xx/sec_fxns.c @@ -0,0 +1,90 @@ +/*

• • sec_fxns.c
• • Common security functions for AMxx devices that rely
• • on secure ROM services.
• • Copyright (C) 2013, Texas Instruments, Incorporated - http://www.ti.com/
• • SPDX-License-Identifier: GPL-2.0+
• */

+#include <common.h> +#include <asm/arch/sys_proto.h> +#include <asm/omap_common.h>

+/* Index for signature verify ROM API*/ +#define API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX (0x0000000E)

+static u32 find_sig_start(char *image, size_t size) +{

•   char *image_end = image + size;
  

•   char *sig_start_magic = "CERT_";
  

•   int magic_str_len = strlen(sig_start_magic);
  

•   char *ch;
  

•   while (--image_end > image) {
  

•           if (*image_end == '_') {
  

•                   ch = image_end - magic_str_len + 1;
  

•                   if (!strncmp(ch, sig_start_magic, magic_str_len))
  

•                           return (u32)ch;
  

•           }
  

•   }
  

•   return 0;
  

+}

+int secure_boot_verify_image(void **image, size_t *size) +{

•   int result = 1;
  

•   u32 cert_addr, sig_addr;
  

•   size_t cert_size;
  

+#ifndef CONFIG_SYS_DCACHE_OFF

•   /* Perform cache writeback on input buffer */
  

•   flush_dcache_range(
  

•           (u32)*image,
  

•           (u32)*image + roundup(*size, ARCH_DMA_MINALIGN));
  

+#endif

•   cert_addr = (uint32_t)*image;
  

•   sig_addr = find_sig_start((char *)*image, *size);
  

This seems similar to the code you added to arch/arm/cpu/armv7/omap5/sec_fxns.c.

Can you put the common code somewhere?

•   if (sig_addr == 0) {
  

•           puts("No signature found in image.\n");
  

•           result = 1;
  

•           goto auth_exit;
  

•   }
  

•   *size = sig_addr - cert_addr;   /* Subtract out the signature size */
  

•   cert_size = *size;
  

•   /* Check if image load address is 32-bit aligned */
  

•   if (0 != (0x3 & cert_addr)) {
  

•           puts("Image is not 4-byte aligned.\n");
  

•           result = 1;
  

•           goto auth_exit;
  

•   }
  

•   /* Image size also should be multiple of 4 */
  

•   if (0 != (0x3 & cert_size)) {
  

•           puts("Image size is not 4-byte aligned.\n");
  

•           result = 1;
  

•           goto auth_exit;
  

•   }
  

•   /* Call ROM HAL API to verify certificate signature */
  

•   debug("%s: load_addr = %x, size = %x, sig_addr = %x\n", __func__,
  

•         cert_addr, cert_size, sig_addr);
  

•   result = secure_rom_call(
  

•           API_HAL_KM_VERIFYCERTIFICATESIGNATURE_INDEX, 0, 0,
  

•           4, cert_addr, cert_size, sig_addr, 0xFFFFFFFF);
  

+auth_exit:

•   if (result != 0) {
  

•           puts("Authentication failed!\n");
  

Please use printf() instead of puts() unless you have a good reason.

•           printf("Return Value = %08X\n", result);
  

•           hang();
  

•   }
  

•   printf("Authentication passed: %s\n", (char *)sig_addr);
  

•   return result;
  

+} diff --git a/arch/arm/include/asm/arch-am33xx/sys_proto.h b/arch/arm/include/asm/arch-am33xx/sys_proto.h index 8f573d2..f5fc916 100644 --- a/arch/arm/include/asm/arch-am33xx/sys_proto.h +++ b/arch/arm/include/asm/arch-am33xx/sys_proto.h @@ -41,7 +41,11 @@ void enable_norboot_pin_mux(void); void am33xx_spl_board_init(void); int am335x_get_efuse_mpu_max_freq(struct ctrl_dev *cdev); int am335x_get_tps65910_mpu_vdd(int sil_rev, int frequency); -#endif

void enable_usb_clocks(int index); void disable_usb_clocks(int index);

+#ifdef CONFIG_TI_SECURE_DEVICE +int secure_boot_verify_image(void **p_image, size_t *p_size);

Please add function comment.

+#endif

+#endif

2.6.4

Regards, Simon

Hi Simon, many thanks for your feedback on the series... I know it takes a lot of effort to digest all that stuff. We'll see how we can tackle the feedback one by one and send out an updated series.

Regarding this patch, please see below...

On Thu, Jun 16, 2016 at 09:52:52PM -0600, Simon Glass wrote:

Hi Andreas,

On 15 June 2016 at 13:26, Andreas Dannenberg dannenberg@ti.com wrote:

...

From: Daniel Allred d-allred@ti.com

Adds commands so that when a secure device is in use and the SPL is built to load a FIT image (with combined u-boot binary and various DTBs), these components that get fed into the FIT are all processed to be signed/encrypted/etc. as per the operations performed by the secure-binary-image script of the TI SECDEV package.

Signed-off-by: Daniel Allred d-allred@ti.com Signed-off-by: Andreas Dannenberg dannenberg@ti.com

---

arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++- arch/arm/cpu/armv7/omap5/config.mk | 3 ++ 2 files changed, 58 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

But please can you add a README for this somewhere?

Also, can this tool be added to U-Boot instead of being external?

Yes we will definitely enhance the readme in the PATCH version, this wasn't quite ready yet by the time we submitted the RFC.

Also regarding the external singing/encryption tool this is only available from TI under NDA after customers engage with TI just like the high-secure (HS) devices themselves (you can't just buy them on Digi-Key). Personally I hope that we eventually lower this barrier of entry (and this has been brought up more than once) but as many things in the corporate world there is a complex decision process behind it. So until this strategy changes (if ever) our poor developer's hands are tied. All we can do for now is trying to allow this external tool to get hooked into the U-Boot build process in the easiest way possible which is already done today by way of the $TI_SECURE_DEV_PKG environmental variable during SPL build.

...

diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk index c7bb101..c4514ad 100644 --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk @@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \ $(if $(KBUILD_VERBOSE:1=), >/dev/null) else cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \

• $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
• $(if $(KBUILD_VERBOSE:1=), >/dev/null)

•   $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
  

•   $(if $(KBUILD_VERBOSE:1=), >/dev/null)
  

endif else cmd_mkomapsecimg = echo "WARNING:" \ @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \ "variable must be defined for TI secure devices. $@ was NOT created!" endif

+ifdef CONFIG_SPL_LOAD_FIT +quiet_cmd_omapsecureimg = SECURE $@ +ifneq ($(TI_SECURE_DEV_PKG),) +ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),) +cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \

•   $< $@ \
  

•   $(if $(KBUILD_VERBOSE:1=), >/dev/null)
  

+else +cmd_omapsecureimg = echo "WARNING:" \

•   "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
  

•   "$@ was NOT created!"; cp $< $@
  

+endif +else +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \

•   "variable must be defined for TI secure devices." \
  

•   "$@ was NOT created!"; cp $< $@
  

+endif +endif

# Standard X-LOADER target (QPSI, NOR flash) u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin $(call if_changed,mkomapsecimg) @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin # the mkomapsecimg command looks for a u-boot-HS_* prefix u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin $(call if_changed,mkomapsecimg)

+# For supporting the SPL loading and interpreting +# of FIT images whose components are pre-processed +# before being integrated into the FIT image in order +# to secure them in some way +ifdef CONFIG_SPL_LOAD_FIT

+MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \

•   -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
  

•   -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
  

•   $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
  

+OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST))) +$(OF_LIST_TARGETS): dtbs

+%_HS.dtb: %.dtb

•   $(call if_changed,omapsecureimg)
  

•   $(Q)if [ -f $@ ]; then \
  

•           cp -f $@ $<; \
  

•   fi
  

+u-boot-nodtb_HS.bin: u-boot-nodtb.bin

•   $(call if_changed,omapsecureimg)
  

+u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))

•   $(call if_changed,mkimage)
  

•   $(Q)if [ -f $@ ]; then \
  

•           cp -f $@ u-boot.img; \
  

•   fi
  

+.NOTPARALLEL: dtbs

Why is that needed?

Good catch! This issue actually caused me a fair amount of grief when running into it. During development we found the build process works with make -j1 but would fail for parallel builds (like make -j8) with the following error....

----------------------->8--------------------------- $ make mrproper $ make dra7xx_hs_evm_defconfig $ make -j8 .... LD u-boot OBJCOPY u-boot-nodtb.bin OBJCOPY u-boot.srec SYM u-boot.sym DTC arch/arm/dts/dra72-evm.dtb DTC arch/arm/dts/dra7-evm.dtb DTC arch/arm/dts/dra72-evm.dtb DTC arch/arm/dts/dra7-evm.dtb Error: arch/arm/dts/dra7-evm.dts:492.15-16 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [arch/arm/dts/dra7-evm.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... Error: arch/arm/dts/dra72-evm.dts:149.26-150.2 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [arch/arm/dts/dra72-evm.dtb] Error 1 make[1]: *** [arch-dtbs] Error 2 make: *** [dtbs] Error 2 make: *** Waiting for unfinished jobs.... SHIPPED dts/dt.dtb ----------------------->8---------------------------

However with the .NOTPARALLEL: dtbs line included it works. Also when doing make a second time it works as well (clearly that's not a solution:)

On my quest trying to digest/root cause this I concluded that this may be caused by the DTB files somehow getting compiled twice (see snippet). This in combination with the fact that we post-process files like dta7-evm.dtb (signing them) and creating new DTB files with the same name (like dra7-evm.dtb) replacing the old ones causes issues during the DTB compile step.

Now I'm not a make guru but I tried to clean up the dependency chain as good as I can but could not get it to the point where each DTB file is only build once which most likely would allow us getting rid of NOPARALLEL.

On a related note we wanted to keep the same names for the DTB files (rather than turning dra7-evm.dtb into dra7-evm-sec.dtb for example, as I had it initially) since it's important as they get bundled into the u-boot.img FIT blob so that they can be later matched by the SPL to a board. But it appears like that trying to keep the same names despite the additional signing step seems to complicate things from a make perspective.

If you or anybody else has any smart suggestion how to address this in a better way I'd love to hear about it.

Thanks, Andreas

...

+endif diff --git a/arch/arm/cpu/armv7/omap5/config.mk b/arch/arm/cpu/armv7/omap5/config.mk index a7e55a5..503f31c 100644 --- a/arch/arm/cpu/armv7/omap5/config.mk +++ b/arch/arm/cpu/armv7/omap5/config.mk @@ -15,5 +15,8 @@ else ALL-y += MLO endif else +ifeq ($(CONFIG_TI_SECURE_DEVICE)$(CONFIG_SECURE_BOOT),yy) +ALL-y += u-boot_HS.img +endif ALL-y += u-boot.img endif -- 2.6.4

Regards, Simon

Hi Simon, thanks for the continued feedback. Comments below...

On Mon, Jun 20, 2016 at 04:40:10PM -0600, Simon Glass wrote:

+Masahiro for the Makefile question

Hi Andreas,

On 17 June 2016 at 10:13, Andreas Dannenberg dannenberg@ti.com wrote:

...

Hi Simon, many thanks for your feedback on the series... I know it takes a lot of effort to digest all that stuff. We'll see how we can tackle the feedback one by one and send out an updated series.

Regarding this patch, please see below...

On Thu, Jun 16, 2016 at 09:52:52PM -0600, Simon Glass wrote:

...

Hi Andreas,

On 15 June 2016 at 13:26, Andreas Dannenberg dannenberg@ti.com wrote:

...

From: Daniel Allred d-allred@ti.com

Adds commands so that when a secure device is in use and the SPL is built to load a FIT image (with combined u-boot binary and various DTBs), these components that get fed into the FIT are all processed to be signed/encrypted/etc. as per the operations performed by the secure-binary-image script of the TI SECDEV package.

Signed-off-by: Daniel Allred d-allred@ti.com Signed-off-by: Andreas Dannenberg dannenberg@ti.com

---

arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++- arch/arm/cpu/armv7/omap5/config.mk | 3 ++ 2 files changed, 58 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

But please can you add a README for this somewhere?

Also, can this tool be added to U-Boot instead of being external?

Yes we will definitely enhance the readme in the PATCH version, this wasn't quite ready yet by the time we submitted the RFC.

Also regarding the external singing/encryption tool this is only available from TI under NDA after customers engage with TI just like the high-secure (HS) devices themselves (you can't just buy them on Digi-Key). Personally I hope that we eventually lower this barrier of entry (and this has been brought up more than once) but as many things in the corporate world there is a complex decision process behind it. So until this strategy changes (if ever) our poor developer's hands are tied. All we can do for now is trying to allow this external tool to get hooked into the U-Boot build process in the easiest way possible which is already done today by way of the $TI_SECURE_DEV_PKG environmental variable during SPL build.

OK. That's odd because it should be the keys that are secure, not the tool! If anything, the tool should have as many eyes on it as possible.

Yes that's the ideal-world case I was also arguing we should follow... But as indicated decision processes in the corporate world sometimes are complex :)

...

...

...

diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk index c7bb101..c4514ad 100644 --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk @@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \ $(if $(KBUILD_VERBOSE:1=), >/dev/null) else cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \

• $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
• $(if $(KBUILD_VERBOSE:1=), >/dev/null)

•   $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
  

•   $(if $(KBUILD_VERBOSE:1=), >/dev/null)
  

endif else cmd_mkomapsecimg = echo "WARNING:" \ @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \ "variable must be defined for TI secure devices. $@ was NOT created!" endif

+ifdef CONFIG_SPL_LOAD_FIT +quiet_cmd_omapsecureimg = SECURE $@ +ifneq ($(TI_SECURE_DEV_PKG),) +ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),) +cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \

•   $< $@ \
  

•   $(if $(KBUILD_VERBOSE:1=), >/dev/null)
  

+else +cmd_omapsecureimg = echo "WARNING:" \

•   "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
  

•   "$@ was NOT created!"; cp $< $@
  

+endif +else +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \

•   "variable must be defined for TI secure devices." \
  

•   "$@ was NOT created!"; cp $< $@
  

+endif +endif

# Standard X-LOADER target (QPSI, NOR flash) u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin $(call if_changed,mkomapsecimg) @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin # the mkomapsecimg command looks for a u-boot-HS_* prefix u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin $(call if_changed,mkomapsecimg)

+# For supporting the SPL loading and interpreting +# of FIT images whose components are pre-processed +# before being integrated into the FIT image in order +# to secure them in some way +ifdef CONFIG_SPL_LOAD_FIT

+MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \

•   -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
  

•   -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
  

•   $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
  

+OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST))) +$(OF_LIST_TARGETS): dtbs

+%_HS.dtb: %.dtb

•   $(call if_changed,omapsecureimg)
  

•   $(Q)if [ -f $@ ]; then \
  

•           cp -f $@ $<; \
  

•   fi
  

+u-boot-nodtb_HS.bin: u-boot-nodtb.bin

•   $(call if_changed,omapsecureimg)
  

+u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))

•   $(call if_changed,mkimage)
  

•   $(Q)if [ -f $@ ]; then \
  

•           cp -f $@ u-boot.img; \
  

•   fi
  

+.NOTPARALLEL: dtbs

Why is that needed?

Good catch! This issue actually caused me a fair amount of grief when running into it. During development we found the build process works with make -j1 but would fail for parallel builds (like make -j8) with the following error....

----------------------->8--------------------------- $ make mrproper $ make dra7xx_hs_evm_defconfig $ make -j8 .... LD u-boot OBJCOPY u-boot-nodtb.bin OBJCOPY u-boot.srec SYM u-boot.sym DTC arch/arm/dts/dra72-evm.dtb DTC arch/arm/dts/dra7-evm.dtb DTC arch/arm/dts/dra72-evm.dtb DTC arch/arm/dts/dra7-evm.dtb Error: arch/arm/dts/dra7-evm.dts:492.15-16 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [arch/arm/dts/dra7-evm.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... Error: arch/arm/dts/dra72-evm.dts:149.26-150.2 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [arch/arm/dts/dra72-evm.dtb] Error 1 make[1]: *** [arch-dtbs] Error 2 make: *** [dtbs] Error 2 make: *** Waiting for unfinished jobs.... SHIPPED dts/dt.dtb ----------------------->8---------------------------

Probably you are missing a dependency somewhere - but the failure to parse is odd. What is it missing?

This failure in parsing is not the real issue. What I found (at least this is how interpreted it) two concurrent DTC builds on the same file are stomping over each other in terms of temp files. Besides this I'd also have a very hard time explaining this error in any other way since the dts file itself is perfectly fine.

...

However with the .NOTPARALLEL: dtbs line included it works. Also when doing make a second time it works as well (clearly that's not a solution:)

On my quest trying to digest/root cause this I concluded that this may be caused by the DTB files somehow getting compiled twice (see snippet). This in combination with the fact that we post-process files like dta7-evm.dtb (signing them) and creating new DTB files with the same name (like dra7-evm.dtb) replacing the old ones causes issues during the DTB compile step.

Yes I think it is better to create new files, since otherwise the build system can't determine which version of the file it should target...it assumes a single version as I understand it.

Yes this is how I had it initially. But as indicated the issue was that the mkimage step that packages this newly-named DTB files would put them into the u-boot.img FIT blob also with this odd new name. And then the board match would not work anymore... And that was when I decided to stop hacking stuff to get it to work.

...

Now I'm not a make guru but I tried to clean up the dependency chain as good as I can but could not get it to the point where each DTB file is only build once which most likely would allow us getting rid of NOPARALLEL.

On a related note we wanted to keep the same names for the DTB files (rather than turning dra7-evm.dtb into dra7-evm-sec.dtb for example, as I had it initially) since it's important as they get bundled into the u-boot.img FIT blob so that they can be later matched by the SPL to a board. But it appears like that trying to keep the same names despite the additional signing step seems to complicate things from a make perspective.

Yes - best to create a new u-boot-sec.img I think.

U-Boot itself is not really the issue here - in fact we are already creating an u-boot_HS.img output artifact. The issue is with the DTBs...

...

If you or anybody else has any smart suggestion how to address this in a better way I'd love to hear about it.

Masahiro is the expert, but my ideas are above.

Awesome. Now that the kids are asleep I'm actually in the process preparing/testing an optimized patch series based on your, Lokesh's, and Tom's feedback. I should be able to send it out soon hopefully tonight. It won't have 100% of feedback implemented (Make stuff is the same) but it'll be a better base for continued discussion.

Best Regards,

-- Andreas Dannenberg Texas Instruments Inc

On Thu, Jun 23, 2016 at 01:59:40PM +0900, Masahiro Yamada wrote:

2016-06-21 11:35 GMT+09:00 Andreas Dannenberg dannenberg@ti.com:

...

Hi Simon, thanks for the continued feedback. Comments below...

On Mon, Jun 20, 2016 at 04:40:10PM -0600, Simon Glass wrote:

...

+Masahiro for the Makefile question

Hi Andreas,

On 17 June 2016 at 10:13, Andreas Dannenberg dannenberg@ti.com wrote:

...

Hi Simon, many thanks for your feedback on the series... I know it takes a lot of effort to digest all that stuff. We'll see how we can tackle the feedback one by one and send out an updated series.

Regarding this patch, please see below...

On Thu, Jun 16, 2016 at 09:52:52PM -0600, Simon Glass wrote:

...

Hi Andreas,

On 15 June 2016 at 13:26, Andreas Dannenberg dannenberg@ti.com wrote:

...

From: Daniel Allred d-allred@ti.com

Adds commands so that when a secure device is in use and the SPL is built to load a FIT image (with combined u-boot binary and various DTBs), these components that get fed into the FIT are all processed to be signed/encrypted/etc. as per the operations performed by the secure-binary-image script of the TI SECDEV package.

Signed-off-by: Daniel Allred d-allred@ti.com Signed-off-by: Andreas Dannenberg dannenberg@ti.com

---

arch/arm/cpu/armv7/omap-common/config_secure.mk | 57 ++++++++++++++++++++++++- arch/arm/cpu/armv7/omap5/config.mk | 3 ++ 2 files changed, 58 insertions(+), 2 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

But please can you add a README for this somewhere?

Also, can this tool be added to U-Boot instead of being external?

Yes we will definitely enhance the readme in the PATCH version, this wasn't quite ready yet by the time we submitted the RFC.

Also regarding the external singing/encryption tool this is only available from TI under NDA after customers engage with TI just like the high-secure (HS) devices themselves (you can't just buy them on Digi-Key). Personally I hope that we eventually lower this barrier of entry (and this has been brought up more than once) but as many things in the corporate world there is a complex decision process behind it. So until this strategy changes (if ever) our poor developer's hands are tied. All we can do for now is trying to allow this external tool to get hooked into the U-Boot build process in the easiest way possible which is already done today by way of the $TI_SECURE_DEV_PKG environmental variable during SPL build.

OK. That's odd because it should be the keys that are secure, not the tool! If anything, the tool should have as many eyes on it as possible.

Yes that's the ideal-world case I was also arguing we should follow... But as indicated decision processes in the corporate world sometimes are complex :)

...

...

...

...

diff --git a/arch/arm/cpu/armv7/omap-common/config_secure.mk b/arch/arm/cpu/armv7/omap-common/config_secure.mk index c7bb101..c4514ad 100644 --- a/arch/arm/cpu/armv7/omap-common/config_secure.mk +++ b/arch/arm/cpu/armv7/omap-common/config_secure.mk @@ -12,8 +12,8 @@ cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \ $(if $(KBUILD_VERBOSE:1=), >/dev/null) else cmd_mkomapsecimg = $(TI_SECURE_DEV_PKG)/scripts/create-boot-image.sh \

• $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
• $(if $(KBUILD_VERBOSE:1=), >/dev/null)

•   $(patsubst u-boot_HS_%,%,$(@F)) $< $@ $(CONFIG_ISW_ENTRY_ADDR) \
  

•   $(if $(KBUILD_VERBOSE:1=), >/dev/null)
  

endif else cmd_mkomapsecimg = echo "WARNING:" \ @@ -25,6 +25,26 @@ cmd_mkomapsecimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \ "variable must be defined for TI secure devices. $@ was NOT created!" endif

+ifdef CONFIG_SPL_LOAD_FIT +quiet_cmd_omapsecureimg = SECURE $@ +ifneq ($(TI_SECURE_DEV_PKG),) +ifneq ($(wildcard $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh),) +cmd_omapsecureimg = $(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh \

•   $< $@ \
  

•   $(if $(KBUILD_VERBOSE:1=), >/dev/null)
  

+else +cmd_omapsecureimg = echo "WARNING:" \

•   "$(TI_SECURE_DEV_PKG)/scripts/secure-binary-image.sh not found." \
  

•   "$@ was NOT created!"; cp $< $@
  

+endif +else +cmd_omapsecureimg = echo "WARNING: TI_SECURE_DEV_PKG environment" \

•   "variable must be defined for TI secure devices." \
  

•   "$@ was NOT created!"; cp $< $@
  

+endif +endif

# Standard X-LOADER target (QPSI, NOR flash) u-boot-spl_HS_X-LOADER: $(obj)/u-boot-spl.bin $(call if_changed,mkomapsecimg) @@ -64,3 +84,36 @@ u-boot-spl_HS_SPI_X-LOADER: $(obj)/u-boot-spl.bin # the mkomapsecimg command looks for a u-boot-HS_* prefix u-boot_HS_XIP_X-LOADER: $(obj)/u-boot.bin $(call if_changed,mkomapsecimg)

+# For supporting the SPL loading and interpreting +# of FIT images whose components are pre-processed +# before being integrated into the FIT image in order +# to secure them in some way +ifdef CONFIG_SPL_LOAD_FIT

+MKIMAGEFLAGS_u-boot_HS.img = -f auto -A $(ARCH) -T firmware -C none -O u-boot \

•   -a $(CONFIG_SYS_TEXT_BASE) -e $(CONFIG_SYS_UBOOT_START) \
  

•   -n "U-Boot $(UBOOTRELEASE) for $(BOARD) board" -E \
  

•   $(patsubst %,-b arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST)))
  

+OF_LIST_TARGETS = $(patsubst %,arch/$(ARCH)/dts/%.dtb,$(subst ",,$(CONFIG_OF_LIST))) +$(OF_LIST_TARGETS): dtbs

+%_HS.dtb: %.dtb

•   $(call if_changed,omapsecureimg)
  

•   $(Q)if [ -f $@ ]; then \
  

•           cp -f $@ $<; \
  

•   fi
  

+u-boot-nodtb_HS.bin: u-boot-nodtb.bin

•   $(call if_changed,omapsecureimg)
  

+u-boot_HS.img: u-boot-nodtb_HS.bin u-boot.img $(patsubst %.dtb,%_HS.dtb,$(OF_LIST_TARGETS))

•   $(call if_changed,mkimage)
  

•   $(Q)if [ -f $@ ]; then \
  

•           cp -f $@ u-boot.img; \
  

•   fi
  

+.NOTPARALLEL: dtbs

Why is that needed?

Good catch! This issue actually caused me a fair amount of grief when running into it. During development we found the build process works with make -j1 but would fail for parallel builds (like make -j8) with the following error....

----------------------->8--------------------------- $ make mrproper $ make dra7xx_hs_evm_defconfig $ make -j8 .... LD u-boot OBJCOPY u-boot-nodtb.bin OBJCOPY u-boot.srec SYM u-boot.sym DTC arch/arm/dts/dra72-evm.dtb DTC arch/arm/dts/dra7-evm.dtb DTC arch/arm/dts/dra72-evm.dtb DTC arch/arm/dts/dra7-evm.dtb Error: arch/arm/dts/dra7-evm.dts:492.15-16 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [arch/arm/dts/dra7-evm.dtb] Error 1 make[2]: *** Waiting for unfinished jobs.... Error: arch/arm/dts/dra72-evm.dts:149.26-150.2 syntax error FATAL ERROR: Unable to parse input tree make[2]: *** [arch/arm/dts/dra72-evm.dtb] Error 1 make[1]: *** [arch-dtbs] Error 2 make: *** [dtbs] Error 2 make: *** Waiting for unfinished jobs.... SHIPPED dts/dt.dtb ----------------------->8---------------------------

Probably you are missing a dependency somewhere - but the failure to parse is odd. What is it missing?

This failure in parsing is not the real issue. What I found (at least this is how interpreted it) two concurrent DTC builds on the same file are stomping over each other in terms of temp files. Besides this I'd also have a very hard time explaining this error in any other way since the dts file itself is perfectly fine.

...

...

However with the .NOTPARALLEL: dtbs line included it works. Also when doing make a second time it works as well (clearly that's not a solution:)

On my quest trying to digest/root cause this I concluded that this may be caused by the DTB files somehow getting compiled twice (see snippet). This in combination with the fact that we post-process files like dta7-evm.dtb (signing them) and creating new DTB files with the same name (like dra7-evm.dtb) replacing the old ones causes issues during the DTB compile step.

Yes I think it is better to create new files, since otherwise the build system can't determine which version of the file it should target...it assumes a single version as I understand it.

Yes this is how I had it initially. But as indicated the issue was that the mkimage step that packages this newly-named DTB files would put them into the u-boot.img FIT blob also with this odd new name. And then the board match would not work anymore... And that was when I decided to stop hacking stuff to get it to work.

...

...

Now I'm not a make guru but I tried to clean up the dependency chain as good as I can but could not get it to the point where each DTB file is only build once which most likely would allow us getting rid of NOPARALLEL.

On a related note we wanted to keep the same names for the DTB files (rather than turning dra7-evm.dtb into dra7-evm-sec.dtb for example, as I had it initially) since it's important as they get bundled into the u-boot.img FIT blob so that they can be later matched by the SPL to a board. But it appears like that trying to keep the same names despite the additional signing step seems to complicate things from a make perspective.

Yes - best to create a new u-boot-sec.img I think.

U-Boot itself is not really the issue here - in fact we are already creating an u-boot_HS.img output artifact. The issue is with the DTBs...

...

...

If you or anybody else has any smart suggestion how to address this in a better way I'd love to hear about it.

Masahiro is the expert, but my ideas are above.

Awesome. Now that the kids are asleep I'm actually in the process preparing/testing an optimized patch series based on your, Lokesh's, and Tom's feedback. I should be able to send it out soon hopefully tonight. It won't have 100% of feedback implemented (Make stuff is the same) but it'll be a better base for continued discussion.

Best Regards,

Hi.

Could you try this? http://patchwork.ozlabs.org/patch/639478/

I think you can remove the ".NOTPARALLEL" line.

Yamada-san, yes your patch fixes this. Thanks for spending time thinking about this issue. Will remove the .NOTPARALLEL hack in the next revision of this patch series.

Regards,

-- Andreas Dannenberg Texas Instruments Inc