diff mbox series

u-boot: add option to generate env image from default env

Message ID 20181002053526.11374-1-Denis.Osterland@diehl.com
State Superseded
Headers show
Series u-boot: add option to generate env image from default env | expand

Commit Message

Denis Osterland-Heim Oct. 2, 2018, 5:38 a.m. UTC 
From: Denis Osterland <Denis.Osterland@diehl.com>

This patch adds support to extract compiled in default env
via u-boots get_default_envs script and generate env image from it.

Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
---
 boot/uboot/Config.in | 15 +++++++++++++++
 boot/uboot/uboot.mk  |  6 +++++-
 2 files changed, 20 insertions(+), 1 deletion(-)

Comments

Arnout Vandecappelle Oct. 8, 2018, 5:33 p.m. UTC | #1 
Hi Denis,

 I have a few very small comments about this patch.

On 2/10/18 07:38, Denis OSTERLAND wrote:
> From: Denis Osterland <Denis.Osterland@diehl.com>
> 
> This patch adds support to extract compiled in default env
> via u-boots get_default_envs script and generate env image from it.
> 
> Signed-off-by: Denis Osterland <Denis.Osterland@diehl.com>
> ---
>  boot/uboot/Config.in | 15 +++++++++++++++
>  boot/uboot/uboot.mk  |  6 +++++-
>  2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index 9e40c11fa1..a2e9c3866a 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -468,8 +468,23 @@ menuconfig BR2_TARGET_UBOOT_ENVIMAGE
>  
>  if BR2_TARGET_UBOOT_ENVIMAGE
>  
> +choice
> +	prompt "source"

 It's not immediately obvious that this is the source of the envimage, so better
"Source for environment".

> +
> +config BR2_TARGET_UBOOT_ENVIMAGE_TEXTFILE

 I would call this BR2_TARGET_UBOOT_ENVIMAGE_CUSTOM to be consistent with the
other uses of custom config files. However, see below.

> +	bool "text file"

 Here as well: "custom".

> +
> +config BR2_TARGET_UBOOT_ENVIMAGE_BUIILTIN

 and here _DEFAULT

> +	bool "compiled in"

 and "default".

> +	help
> +	  Use the default env from u-boot image.
> +	  requires >= v2018.03
> +
> +endchoice # source
> +
>  config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE
>  	string "Source files for environment"
> +	depends on BR2_TARGET_UBOOT_ENVIMAGE_TEXTFILE

 However, I think it would be simpler to just allow this option to be empty. In
other words, remove the choice, and add something like the following at the end
of the help text:

	  For U-Boot >= v2018.03, it is possible to leave this empty. In that
	  case, the default environment for the target configuration will be
	  used.

 This is just an idea, if you don't like it, feel free to keep the current solution.


>  	help
>  	  Text files describing the environment. Files should have
>  	  lines of the form var=value, one per line. Blank lines and
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index c5abc125f3..ae9e38c8c2 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -263,7 +263,9 @@ endef
>  
>  ifneq ($(BR2_TARGET_UBOOT_ENVIMAGE),)
>  define UBOOT_GENERATE_ENV_IMAGE
> -	cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) \
> +	$(if $(BR2_TARGET_UBOOT_ENVIMAGE_BUIILTIN), \

 With the choice removed, this test could be:

	$(if $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)),

but then it is better to factor it out in a separate variable, e.g.
UBOOT_GENERATE_ENV_FILE, that is protected by an ifdef.

> +	CROSS_COMPILE="$(TARGET_CROSS)" $(@D)/scripts/get_default_envs.sh $(@D), \
> +	cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE))) \
>  		>$(@D)/buildroot-env.txt
>  	$(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
>  		$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
> @@ -376,9 +378,11 @@ endef
>  
>  ifeq ($(BR2_TARGET_UBOOT_ENVIMAGE),y)
>  ifeq ($(BR_BUILDING),y)
> +ifeq ($(BR2_TARGET_UBOOT_ENVIMAGE_TEXTFILE),y)
>  ifeq ($(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)),)
>  $(error Please define a source file for Uboot environment (BR2_TARGET_UBOOT_ENVIMAGE_SOURCE setting))
>  endif
> +endif

 With the choice removed, this entire condition+error would just be removed.

 Regards,
 Arnout

>  ifeq ($(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SIZE)),)
>  $(error Please provide Uboot environment size (BR2_TARGET_UBOOT_ENVIMAGE_SIZE setting))
>  endif
>
Denis Osterland-Heim Oct. 9, 2018, 7:07 a.m. UTC | #2 
Hi Arnout,

thanks for the review.
Am Montag, den 08.10.2018, 19:33 +0200 schrieb Arnout Vandecappelle:
> >  
> > +choice
> > +	prompt "source"
>  It's not immediately obvious that this is the source of the envimage, so better
> "Source for environment".
clear
> 
> > 
> > +
> > +config BR2_TARGET_UBOOT_ENVIMAGE_TEXTFILE
>  I would call this BR2_TARGET_UBOOT_ENVIMAGE_CUSTOM to be consistent with the
> other uses of custom config files. However, see below.
> 
> > 
> > +	bool "text file"
>  Here as well: "custom".
okay
> 
> > 
> > +
> > +config BR2_TARGET_UBOOT_ENVIMAGE_BUIILTIN
>  and here _DEFAULT
> 
> > 
> > +	bool "compiled in"
>  and "default".
okay
> 
> > 
> > +	help
> > +	  Use the default env from u-boot image.
> > +	  requires >= v2018.03
> > +
> > +endchoice # source
> > +
> >  config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE
> >  	string "Source files for environment"
> > +	depends on BR2_TARGET_UBOOT_ENVIMAGE_TEXTFILE
>  However, I think it would be simpler to just allow this option to be empty. In
> other words, remove the choice, and add something like the following at the end
> of the help text:
> 
> 	  For U-Boot >= v2018.03, it is possible to leave this empty. In that
> 	  case, the default environment for the target configuration will be
> 	  used.
> 
>  This is just an idea, if you don't like it, feel free to keep the current solution.
Well, I thought about it.
contra:
 - adds additional configuration switches
pro:
 - clear on first look (not required to read help first)
 - easier to extend (add other sources in future)
 - keeps current behavior (failed to build, if only BR2_TARGET_UBOOT_ENVIMAGE is selected)

I think it is not very likely that other sources were implemented.
Maybe it would be nice to just tick BR2_TARGET_UBOOT_ENVIMAGE and
empty default of SOURCE will lead to default env.

What do you think?
> 
> 
> > 
> >  	help
> >  	  Text files describing the environment. Files should have
> >  	  lines of the form var=value, one per line. Blank lines and
> > diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> > index c5abc125f3..ae9e38c8c2 100644
> > --- a/boot/uboot/uboot.mk
> > +++ b/boot/uboot/uboot.mk
> > @@ -263,7 +263,9 @@ endef
> >  
> >  ifneq ($(BR2_TARGET_UBOOT_ENVIMAGE),)
> >  define UBOOT_GENERATE_ENV_IMAGE
> > -	cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) \
> > +	$(if $(BR2_TARGET_UBOOT_ENVIMAGE_BUIILTIN), \
>  With the choice removed, this test could be:
> 
> 	$(if $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)),
> 
> but then it is better to factor it out in a separate variable, e.g.
> UBOOT_GENERATE_ENV_FILE, that is protected by an ifdef.
indeed
> 
> > 
> > +	CROSS_COMPILE="$(TARGET_CROSS)" $(@D)/scripts/get_default_envs.sh $(@D), \
> > +	cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE))) \
> >  		>$(@D)/buildroot-env.txt
> >  	$(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
> >  		$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
> > @@ -376,9 +378,11 @@ endef
> >  
> >  ifeq ($(BR2_TARGET_UBOOT_ENVIMAGE),y)
> >  ifeq ($(BR_BUILDING),y)
> > +ifeq ($(BR2_TARGET_UBOOT_ENVIMAGE_TEXTFILE),y)
> >  ifeq ($(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)),)
> >  $(error Please define a source file for Uboot environment (BR2_TARGET_UBOOT_ENVIMAGE_SOURCE setting))
> >  endif
> > +endif
>  With the choice removed, this entire condition+error would just be removed.
remove code is always nice ;-)
> 
>  Regards,
>  Arnout
Regards Denis

Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315
Arnout Vandecappelle Oct. 9, 2018, 8:01 a.m. UTC | #3 
On 9/10/18 09:07, Denis OSTERLAND wrote:
> Hi Arnout,
> 
> thanks for the review.
> Am Montag, den 08.10.2018, 19:33 +0200 schrieb Arnout Vandecappelle:
[snip]
>>  However, I think it would be simpler to just allow this option to be empty. In
>> other words, remove the choice, and add something like the following at the end
>> of the help text:
>>
>> 	  For U-Boot >= v2018.03, it is possible to leave this empty. In that
>> 	  case, the default environment for the target configuration will be
>> 	  used.
>>
>>  This is just an idea, if you don't like it, feel free to keep the current solution.
> Well, I thought about it.
> contra:
>  - adds additional configuration switches
> pro:
>  - clear on first look (not required to read help first)

 This is a valid point.

>  - easier to extend (add other sources in future)

 Hm, there is something to be said for this point as well. It might be possible
that U-Boot will start shipping internal environment fragments.

 However, in that case, I think we would really want to add an additional list
of internal sources, again without an option to control enabling it.

>  - keeps current behavior (failed to build, if only BR2_TARGET_UBOOT_ENVIMAGE is selected)

 This is really not important.


> I think it is not very likely that other sources were implemented.
> Maybe it would be nice to just tick BR2_TARGET_UBOOT_ENVIMAGE and
> empty default of SOURCE will lead to default env.
> 
> What do you think?

 That is indeed what I meant.

 Regards,
 Arnout

[snip]
Denis Osterland-Heim Oct. 9, 2018, 9:20 a.m. UTC | #4 
Hi Arnout,

Am Dienstag, den 09.10.2018, 10:01 +0200 schrieb Arnout Vandecappelle:
> 
> On 9/10/18 09:07, Denis OSTERLAND wrote:
> > 
> > pro:
> >  - clear on first look (not required to read help first)
>  This is a valid point.
> 
> > 
> >  - easier to extend (add other sources in future)
>  Hm, there is something to be said for this point as well. It might be possible
> that U-Boot will start shipping internal environment fragments.
> 
>  However, in that case, I think we would really want to add an additional list
> of internal sources, again without an option to control enabling it.
> 
> > 
> >  - keeps current behavior (failed to build, if only BR2_TARGET_UBOOT_ENVIMAGE is selected)
>  This is really not important.
> 
> 
> > 
> > I think it is not very likely that other sources were implemented.
> > Maybe it would be nice to just tick BR2_TARGET_UBOOT_ENVIMAGE and
> > empty default of SOURCE will lead to default env.
> > 
> > What do you think?
>  That is indeed what I meant.
so... you vote for empty list?

What about this?
        $(if $(BR2_TARGET_UBOOT_ENVIMAGE_DEFAULT), \
        CROSS_COMPILE="$(TARGET_CROSS)" $(@D)/scripts/get_default_envs.sh $(@D), \
        echo "") | cat - $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) \
                >$(@D)/buildroot-env.txt

Introduce a bool DEFAULT to enable read of compiled in env
and concatenate it with the files from SOURCES list.
> 
>  Regards,
>  Arnout
> 
Regards Denis

Diehl Connectivity Solutions GmbH
Geschäftsführung: Horst Leonberger
Sitz der Gesellschaft: Nürnberg - Registergericht: Amtsgericht
Nürnberg: HRB 32315
diff mbox series

Patch

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 9e40c11fa1..a2e9c3866a 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -468,8 +468,23 @@  menuconfig BR2_TARGET_UBOOT_ENVIMAGE
 
 if BR2_TARGET_UBOOT_ENVIMAGE
 
+choice
+	prompt "source"
+
+config BR2_TARGET_UBOOT_ENVIMAGE_TEXTFILE
+	bool "text file"
+
+config BR2_TARGET_UBOOT_ENVIMAGE_BUIILTIN
+	bool "compiled in"
+	help
+	  Use the default env from u-boot image.
+	  requires >= v2018.03
+
+endchoice # source
+
 config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE
 	string "Source files for environment"
+	depends on BR2_TARGET_UBOOT_ENVIMAGE_TEXTFILE
 	help
 	  Text files describing the environment. Files should have
 	  lines of the form var=value, one per line. Blank lines and
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index c5abc125f3..ae9e38c8c2 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -263,7 +263,9 @@  endef
 
 ifneq ($(BR2_TARGET_UBOOT_ENVIMAGE),)
 define UBOOT_GENERATE_ENV_IMAGE
-	cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) \
+	$(if $(BR2_TARGET_UBOOT_ENVIMAGE_BUIILTIN), \
+	CROSS_COMPILE="$(TARGET_CROSS)" $(@D)/scripts/get_default_envs.sh $(@D), \
+	cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE))) \
 		>$(@D)/buildroot-env.txt
 	$(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
 		$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
@@ -376,9 +378,11 @@  endef
 
 ifeq ($(BR2_TARGET_UBOOT_ENVIMAGE),y)
 ifeq ($(BR_BUILDING),y)
+ifeq ($(BR2_TARGET_UBOOT_ENVIMAGE_TEXTFILE),y)
 ifeq ($(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)),)
 $(error Please define a source file for Uboot environment (BR2_TARGET_UBOOT_ENVIMAGE_SOURCE setting))
 endif
+endif
 ifeq ($(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SIZE)),)
 $(error Please provide Uboot environment size (BR2_TARGET_UBOOT_ENVIMAGE_SIZE setting))
 endif