diff mbox

uboot: install multiple spl images

Message ID 1457648583-25071-1-git-send-email-jason.abele@gmail.com
State Changes Requested
Headers show

Commit Message

Jason Abele March 10, 2016, 10:23 p.m. UTC 
From: Jason Abele <jason@nextthing.co>

For some platforms, there are multiple generated spl images.  Extend
BR2_TARGET_UBOOT_SPL_NAME to allow these multiple images to be installed
after uboot build completes.

Signed-off-by: Jason Abele <jason@nextthing.co>
---
For example, the NextThingCo C.H.I.P. uses two binaries from uboot,
spl/sunxi-spl.bin and spl/sunxi-spl-with-ecc.bin.

This patch allows them to be listed explicitly.

 boot/uboot/Config.in |  2 +-
 boot/uboot/uboot.mk  | 12 ++++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

Comments

Thomas Petazzoni March 10, 2016, 10:34 p.m. UTC | #1 
Jason,

On Thu, 10 Mar 2016 14:23:03 -0800, Jason Abele wrote:
> From: Jason Abele <jason@nextthing.co>
> 
> For some platforms, there are multiple generated spl images.  Extend
> BR2_TARGET_UBOOT_SPL_NAME to allow these multiple images to be installed
> after uboot build completes.
> 
> Signed-off-by: Jason Abele <jason@nextthing.co>

For some reason, I was expecting this patch to arrive :-)

> ---
> For example, the NextThingCo C.H.I.P. uses two binaries from uboot,
> spl/sunxi-spl.bin and spl/sunxi-spl-with-ecc.bin.

This should be part of the commit log itself as it is a very useful
example of why this patch was needed.

> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index 4a6dc56..d4f9445 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -322,7 +322,7 @@ config BR2_TARGET_UBOOT_SPL_NAME
>  	default "spl/u-boot-spl.bin"
>  	depends on BR2_TARGET_UBOOT_SPL
>  	help
> -	  This is the name of the SPL binary, generated during
> +	  A space-separated list of SPL binaries, generated during
>  	  u-boot build. For most platform it is spl/u-boot-spl.bin
>  	  but not always. It is MLO on OMAP for example.
>  
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index d539b31..aae99f8 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -169,7 +169,9 @@ define UBOOT_INSTALL_IMAGES_CMDS
>  	$(if $(BR2_TARGET_UBOOT_FORMAT_NAND),
>  		cp -dpf $(@D)/$(UBOOT_MAKE_TARGET) $(BINARIES_DIR))
>  	$(if $(BR2_TARGET_UBOOT_SPL),
> -		cp -dpf $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)) $(BINARIES_DIR)/)
> +		for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
> +			cp -dpf $(@D)/$$p $(BINARIES_DIR)/; \
> +		done)

Ideally, using the make $(foreach ...) function here would be better.

>  ifeq ($(BR2_TARGET_UBOOT_ZYNQ_IMAGE),y)
>  define UBOOT_GENERATE_ZYNQ_IMAGE
> -	$(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
> -		-u $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME))     \
> -		-o $(BINARIES_DIR)/BOOT.BIN
> +	for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
> +		$(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
> +			-u $(@D)/$$p -o $(BINARIES_DIR)/BOOT.BIN; \
> +		fi; \
> +	done

Having a for loop here is really useless, since the output file is
BOOT.BIN at each iteration of the loop. Since for the specific Zynq
case it doesn't make sense to have multiple values in
BR2_TARGET_UBOOT_SPL_NAME, I would suggest to just do:

-	-u $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME))
+	-u $(@D)/$(firstword $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)))

Best regards,

Thomas
Jason Abele March 10, 2016, 11:04 p.m. UTC | #2 
Thomas,

On Thu, Mar 10, 2016 at 2:34 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Jason,
>
> On Thu, 10 Mar 2016 14:23:03 -0800, Jason Abele wrote:
>> From: Jason Abele <jason@nextthing.co>
>>
>> For some platforms, there are multiple generated spl images.  Extend
>> BR2_TARGET_UBOOT_SPL_NAME to allow these multiple images to be installed
>> after uboot build completes.
>>
>> Signed-off-by: Jason Abele <jason@nextthing.co>
>
> For some reason, I was expecting this patch to arrive :-)
>
>> ---
>> For example, the NextThingCo C.H.I.P. uses two binaries from uboot,
>> spl/sunxi-spl.bin and spl/sunxi-spl-with-ecc.bin.
>
> This should be part of the commit log itself as it is a very useful
> example of why this patch was needed.
>
>> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
>> index 4a6dc56..d4f9445 100644
>> --- a/boot/uboot/Config.in
>> +++ b/boot/uboot/Config.in
>> @@ -322,7 +322,7 @@ config BR2_TARGET_UBOOT_SPL_NAME
>>       default "spl/u-boot-spl.bin"
>>       depends on BR2_TARGET_UBOOT_SPL
>>       help
>> -       This is the name of the SPL binary, generated during
>> +       A space-separated list of SPL binaries, generated during
>>         u-boot build. For most platform it is spl/u-boot-spl.bin
>>         but not always. It is MLO on OMAP for example.
>>
>> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
>> index d539b31..aae99f8 100644
>> --- a/boot/uboot/uboot.mk
>> +++ b/boot/uboot/uboot.mk
>> @@ -169,7 +169,9 @@ define UBOOT_INSTALL_IMAGES_CMDS
>>       $(if $(BR2_TARGET_UBOOT_FORMAT_NAND),
>>               cp -dpf $(@D)/$(UBOOT_MAKE_TARGET) $(BINARIES_DIR))
>>       $(if $(BR2_TARGET_UBOOT_SPL),
>> -             cp -dpf $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)) $(BINARIES_DIR)/)
>> +             for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
>> +                     cp -dpf $(@D)/$$p $(BINARIES_DIR)/; \
>> +             done)
>
> Ideally, using the make $(foreach ...) function here would be better.

Ok, should I stack a patch ahead of this fixing the usage of the shell
'for' loop with UBOOT_PATCH variable above here?  Seems like
maintaining consistency within the file is probably a good idea

>
>>  ifeq ($(BR2_TARGET_UBOOT_ZYNQ_IMAGE),y)
>>  define UBOOT_GENERATE_ZYNQ_IMAGE
>> -     $(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
>> -             -u $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME))     \
>> -             -o $(BINARIES_DIR)/BOOT.BIN
>> +     for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
>> +             $(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
>> +                     -u $(@D)/$$p -o $(BINARIES_DIR)/BOOT.BIN; \
>> +             fi; \
>> +     done
>
> Having a for loop here is really useless, since the output file is
> BOOT.BIN at each iteration of the loop. Since for the specific Zynq
> case it doesn't make sense to have multiple values in
> BR2_TARGET_UBOOT_SPL_NAME, I would suggest to just do:
>
> -       -u $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME))
> +       -u $(@D)/$(firstword $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)))
>

So, I feel like the whole SPL_NAME can be MLO or BOOT.BIN or what not
is a horrible mess and that it would be bad form for
BR2_TARGET_UBOOT_SPL_NAME to take a list of files in some cases and
only a single file in other cases.  I am no uboot expert, but this
seems like it would be hard to document in a clear way.  If there is
better language for the documentation which will make the difference
in usage clear, then I am happy to amend the documentation and use
make's firstword function for the xynq platform.

It feels like the cleaner solution would just be to use
BR2_TARGET_UBOOT_SPL_NAME="spl/sunxi-spl*.bin" in the defconfig for my
platform than add yet another variant usage in uboot.mk, any thoughts
on this option?

Thanks for the review,
Jason
Thomas De Schampheleire May 4, 2016, 3:08 p.m. UTC | #3 
Hi,

(late reply, just noticed this patch)

On Thu, Mar 10, 2016 at 11:23 PM, Jason Abele <jason.abele@gmail.com> wrote:
> From: Jason Abele <jason@nextthing.co>
>
> For some platforms, there are multiple generated spl images.  Extend
> BR2_TARGET_UBOOT_SPL_NAME to allow these multiple images to be installed
> after uboot build completes.
>
> Signed-off-by: Jason Abele <jason@nextthing.co>
> ---
> For example, the NextThingCo C.H.I.P. uses two binaries from uboot,
> spl/sunxi-spl.bin and spl/sunxi-spl-with-ecc.bin.
>
> This patch allows them to be listed explicitly.
>
>  boot/uboot/Config.in |  2 +-
>  boot/uboot/uboot.mk  | 12 ++++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index 4a6dc56..d4f9445 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -322,7 +322,7 @@ config BR2_TARGET_UBOOT_SPL_NAME
>         default "spl/u-boot-spl.bin"
>         depends on BR2_TARGET_UBOOT_SPL
>         help
> -         This is the name of the SPL binary, generated during
> +         A space-separated list of SPL binaries, generated during
>           u-boot build. For most platform it is spl/u-boot-spl.bin
>           but not always. It is MLO on OMAP for example.
>
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index d539b31..aae99f8 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -169,7 +169,9 @@ define UBOOT_INSTALL_IMAGES_CMDS
>         $(if $(BR2_TARGET_UBOOT_FORMAT_NAND),
>                 cp -dpf $(@D)/$(UBOOT_MAKE_TARGET) $(BINARIES_DIR))
>         $(if $(BR2_TARGET_UBOOT_SPL),
> -               cp -dpf $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)) $(BINARIES_DIR)/)
> +               for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
> +                       cp -dpf $(@D)/$$p $(BINARIES_DIR)/; \
> +               done)
>         $(if $(BR2_TARGET_UBOOT_ENVIMAGE),
>                 $(HOST_DIR)/usr/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
>                 $(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
> @@ -196,9 +198,11 @@ endif
>
>  ifeq ($(BR2_TARGET_UBOOT_ZYNQ_IMAGE),y)
>  define UBOOT_GENERATE_ZYNQ_IMAGE
> -       $(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
> -               -u $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME))     \
> -               -o $(BINARIES_DIR)/BOOT.BIN
> +       for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
> +               $(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
> +                       -u $(@D)/$$p -o $(BINARIES_DIR)/BOOT.BIN; \
> +               fi; \
> +       done
>  endef
>  UBOOT_DEPENDENCIES += host-zynq-boot-bin
>  UBOOT_POST_INSTALL_IMAGES_HOOKS += UBOOT_GENERATE_ZYNQ_IMAGE

We have a similar modification in our tree but for the real u-boot
image and not the SPL image. In this case, the UBOOT_BOARD_NAME can
take a list of board names, and we use a similar for loop to iterate
over the configure, build, install step for each board.

Use case is to use a single defconfig for different hardware boards
that are very much alike but need some different u-boot (different
device tree, among other subtleties).

I thought I once shared this patch with the list but I cannot find it back.

If the path is now more open for inclusion of such change, I would be
more than happy to send it.

Let me know,
Thomas
Maxime Hadjinlian July 5, 2016, 11:19 a.m. UTC | #4 
Hi Jason, all,

I have respinned your patch, you can find it here:
https://patchwork.ozlabs.org/patch/644708/

Your patch has been marked as "Changes Requested" in our patchwork.

Thomas: I haven't taken into account your demand since you have a
patch, it's better that you send yours it will match your needs better
that what I could propose :).

Thanks for your patch

On Wed, May 4, 2016 at 5:08 PM, Thomas De Schampheleire
<patrickdepinguin@gmail.com> wrote:
> Hi,
>
> (late reply, just noticed this patch)
>
> On Thu, Mar 10, 2016 at 11:23 PM, Jason Abele <jason.abele@gmail.com> wrote:
>> From: Jason Abele <jason@nextthing.co>
>>
>> For some platforms, there are multiple generated spl images.  Extend
>> BR2_TARGET_UBOOT_SPL_NAME to allow these multiple images to be installed
>> after uboot build completes.
>>
>> Signed-off-by: Jason Abele <jason@nextthing.co>
>> ---
>> For example, the NextThingCo C.H.I.P. uses two binaries from uboot,
>> spl/sunxi-spl.bin and spl/sunxi-spl-with-ecc.bin.
>>
>> This patch allows them to be listed explicitly.
>>
>>  boot/uboot/Config.in |  2 +-
>>  boot/uboot/uboot.mk  | 12 ++++++++----
>>  2 files changed, 9 insertions(+), 5 deletions(-)
>>
>> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
>> index 4a6dc56..d4f9445 100644
>> --- a/boot/uboot/Config.in
>> +++ b/boot/uboot/Config.in
>> @@ -322,7 +322,7 @@ config BR2_TARGET_UBOOT_SPL_NAME
>>         default "spl/u-boot-spl.bin"
>>         depends on BR2_TARGET_UBOOT_SPL
>>         help
>> -         This is the name of the SPL binary, generated during
>> +         A space-separated list of SPL binaries, generated during
>>           u-boot build. For most platform it is spl/u-boot-spl.bin
>>           but not always. It is MLO on OMAP for example.
>>
>> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
>> index d539b31..aae99f8 100644
>> --- a/boot/uboot/uboot.mk
>> +++ b/boot/uboot/uboot.mk
>> @@ -169,7 +169,9 @@ define UBOOT_INSTALL_IMAGES_CMDS
>>         $(if $(BR2_TARGET_UBOOT_FORMAT_NAND),
>>                 cp -dpf $(@D)/$(UBOOT_MAKE_TARGET) $(BINARIES_DIR))
>>         $(if $(BR2_TARGET_UBOOT_SPL),
>> -               cp -dpf $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)) $(BINARIES_DIR)/)
>> +               for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
>> +                       cp -dpf $(@D)/$$p $(BINARIES_DIR)/; \
>> +               done)
>>         $(if $(BR2_TARGET_UBOOT_ENVIMAGE),
>>                 $(HOST_DIR)/usr/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
>>                 $(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
>> @@ -196,9 +198,11 @@ endif
>>
>>  ifeq ($(BR2_TARGET_UBOOT_ZYNQ_IMAGE),y)
>>  define UBOOT_GENERATE_ZYNQ_IMAGE
>> -       $(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
>> -               -u $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME))     \
>> -               -o $(BINARIES_DIR)/BOOT.BIN
>> +       for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
>> +               $(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
>> +                       -u $(@D)/$$p -o $(BINARIES_DIR)/BOOT.BIN; \
>> +               fi; \
>> +       done
>>  endef
>>  UBOOT_DEPENDENCIES += host-zynq-boot-bin
>>  UBOOT_POST_INSTALL_IMAGES_HOOKS += UBOOT_GENERATE_ZYNQ_IMAGE
>
> We have a similar modification in our tree but for the real u-boot
> image and not the SPL image. In this case, the UBOOT_BOARD_NAME can
> take a list of board names, and we use a similar for loop to iterate
> over the configure, build, install step for each board.
>
> Use case is to use a single defconfig for different hardware boards
> that are very much alike but need some different u-boot (different
> device tree, among other subtleties).
>
> I thought I once shared this patch with the list but I cannot find it back.
>
> If the path is now more open for inclusion of such change, I would be
> more than happy to send it.
>
> Let me know,
> Thomas
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
diff mbox

Patch

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 4a6dc56..d4f9445 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -322,7 +322,7 @@  config BR2_TARGET_UBOOT_SPL_NAME
 	default "spl/u-boot-spl.bin"
 	depends on BR2_TARGET_UBOOT_SPL
 	help
-	  This is the name of the SPL binary, generated during
+	  A space-separated list of SPL binaries, generated during
 	  u-boot build. For most platform it is spl/u-boot-spl.bin
 	  but not always. It is MLO on OMAP for example.
 
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index d539b31..aae99f8 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -169,7 +169,9 @@  define UBOOT_INSTALL_IMAGES_CMDS
 	$(if $(BR2_TARGET_UBOOT_FORMAT_NAND),
 		cp -dpf $(@D)/$(UBOOT_MAKE_TARGET) $(BINARIES_DIR))
 	$(if $(BR2_TARGET_UBOOT_SPL),
-		cp -dpf $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)) $(BINARIES_DIR)/)
+		for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
+			cp -dpf $(@D)/$$p $(BINARIES_DIR)/; \
+		done)
 	$(if $(BR2_TARGET_UBOOT_ENVIMAGE),
 		$(HOST_DIR)/usr/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
 		$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
@@ -196,9 +198,11 @@  endif
 
 ifeq ($(BR2_TARGET_UBOOT_ZYNQ_IMAGE),y)
 define UBOOT_GENERATE_ZYNQ_IMAGE
-	$(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
-		-u $(@D)/$(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME))     \
-		-o $(BINARIES_DIR)/BOOT.BIN
+	for p in $(call qstrip,$(BR2_TARGET_UBOOT_SPL_NAME)); do \
+		$(HOST_DIR)/usr/bin/python2 $(HOST_DIR)/usr/bin/zynq-boot-bin.py \
+			-u $(@D)/$$p -o $(BINARIES_DIR)/BOOT.BIN; \
+		fi; \
+	done
 endef
 UBOOT_DEPENDENCIES += host-zynq-boot-bin
 UBOOT_POST_INSTALL_IMAGES_HOOKS += UBOOT_GENERATE_ZYNQ_IMAGE