[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Johannes Schmitz]

Updated the help text for the size of the uboot environment. The size of
the uboot environment, which is given in bytes, needs to be exactly
correct in order to calculate the CRC checksum as expected and
re-calculated by uboot during the target boot process. Otherwise the
environment is ignored during boot due to CRC mismatch.

Additionally, add the missing option to specify the padding byte which
is used by the mkenvimage -p parameter.

Signed-off-by: Johannes Schmitz <johannes.schmitz1@gmail.com>
---
 boot/uboot/Config.in | 13 +++++++++++--
 boot/uboot/uboot.mk  | 10 ++++++----
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 2829d2c..32e283a 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -413,8 +413,17 @@ config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE
 config BR2_TARGET_UBOOT_ENVIMAGE_SIZE
 	string "Size of environment"
 	help
-	  Size of envronment, can be prefixed with 0x for hexadecimal
-	  values.
+	  Size of environment in bytes, can be prefixed with 0x for
+	  hexadecimal values. Needs to match exactly for correct CRC
+	  checksum calculation as re-calculated by uboot during each
+	  target boot. If a CRC mismatch occurs during boot the
+	  environment will be ignored.
+
+config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
+	hex "Padding byte"
+	default 0x00
+	help
+	  The byte used for padding at the end of the environment image.
 
 config BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT
 	bool "Environment has two copies"
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index 3917599..43fdde4 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -256,10 +256,12 @@ define UBOOT_INSTALL_IMAGES_CMDS
 	)
 	$(if $(BR2_TARGET_UBOOT_ENVIMAGE),
 		cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) | \
-			$(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
-			$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
-			$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
-			-o $(BINARIES_DIR)/uboot-env.bin -)
+			$(HOST_DIR)/bin/mkenvimage \
+				$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
+				$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
+				-p $(BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE) \
+				-s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
+				-o $(BINARIES_DIR)/uboot-env.bin -)
 	$(if $(BR2_TARGET_UBOOT_BOOT_SCRIPT),
 		$(HOST_DIR)/bin/mkimage -C none -A $(MKIMAGE_ARCH) -T script \
 			-d $(call qstrip,$(BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE)) \
-- 
2.7.4

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Thomas Petazzoni]

Hello,

On Mon, 11 Dec 2017 19:47:08 +0100, Johannes Schmitz wrote:

>  config BR2_TARGET_UBOOT_ENVIMAGE_SIZE
>  	string "Size of environment"
>  	help
> -	  Size of envronment, can be prefixed with 0x for hexadecimal
> -	  values.
> +	  Size of environment in bytes, can be prefixed with 0x for
> +	  hexadecimal values. Needs to match exactly for correct CRC

"Needs to match exactly" with what ?

It needs to match with the size of the environment declared in the
U-Boot configuration.

> +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> +	hex "Padding byte"
> +	default 0x00
> +	help
> +	  The byte used for padding at the end of the environment image.

I'm still not sure what is the point of customizing the padding byte.
Does someone has a use-case for that ? If not, I'm not sure it makes
sense to add a Buildroot option for it.

Best regards,

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

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Johannes Schmitz]

> +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> +     hex "Padding byte"
> +     default 0x00
> +     help
> +       The byte used for padding at the end of the environment image.

I'm still not sure what is the point of customizing the padding byte.
Does someone has a use-case for that ? If not, I'm not sure it makes
sense to add a Buildroot option for it.


I find it definitely useful for debugging. I wouldn't have been able to
isolate the issue with the size and CRC without this option because if you
want to compare with hexdump you want everything to exactly match to see
what's wrong. Without this, the next person might go down the same way like
myself. The default of uboot when saving the environment is 0x00 and the
default of mkenvimage is 0xFF. My idea is to make it the same to (0x00) to
make the developers life easier. My experience is that any of these details
takes some amount of your time to sort out so a less confusing default can
safe time for everyone. On the other hand people might want to deliberately
set it to 0xFF to exactly check the boarders of the env partition on the SD
card, especially during development of genimage.cfg. Furthermore it makes
you understand quicker what happens during env image generation. If you
hide the option from the user/platform developer again makes life harder. I
am speaking from the point of view of a buildroot newcomer. Now of course I
now what's going on but would that option have been there it would have
saved me one afternoon.

Regards
Johannes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20171212/22cc0c96/attachment.html>

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Thomas Petazzoni]

Hello,

On Tue, 12 Dec 2017 10:04:38 +0100, Johannes Schmitz wrote:

> I find it definitely useful for debugging. I wouldn't have been able to
> isolate the issue with the size and CRC without this option because if you
> want to compare with hexdump you want everything to exactly match to see
> what's wrong. Without this, the next person might go down the same way like
> myself. The default of uboot when saving the environment is 0x00 and the
> default of mkenvimage is 0xFF. My idea is to make it the same to (0x00) to
> make the developers life easier. My experience is that any of these details
> takes some amount of your time to sort out so a less confusing default can
> safe time for everyone. On the other hand people might want to deliberately
> set it to 0xFF to exactly check the boarders of the env partition on the SD
> card, especially during development of genimage.cfg. Furthermore it makes
> you understand quicker what happens during env image generation. If you
> hide the option from the user/platform developer again makes life harder. I
> am speaking from the point of view of a buildroot newcomer. Now of course I
> now what's going on but would that option have been there it would have
> saved me one afternoon.

Hm, ok. I'm still not super convinced, but the code/complexity isn't
big enough to really argue more than that :)

Thanks!

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

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Yann E. MORIN]

Thomas, Johannes, All,

On 2017-12-12 06:27 +0100, Thomas Petazzoni spake thusly:
> On Mon, 11 Dec 2017 19:47:08 +0100, Johannes Schmitz wrote:
[--SNIP--]
> > +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> > +	hex "Padding byte"
> > +	default 0x00
> > +	help
> > +	  The byte used for padding at the end of the environment image.
> 
> I'm still not sure what is the point of customizing the padding byte.
> Does someone has a use-case for that ? If not, I'm not sure it makes
> sense to add a Buildroot option for it.

As far as I understand, this is because the U-Boot on Johannes' board
would expect the padding byte to be 0x00 when the default for mkenvimage
is 0xFF.

I have no idea why U-Boot expects 0x00 in Johannes' case, though. The
only thing I can think of is that the 'default' byte value on flash
after a page-erase depends on the technology: for NOR, the value after
erase ix 0x00, while for NAND, it is 0xFF. So maybe that's the reason...

But any case, as you said in a further reply, adding that new option
does not add much complexity.

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Johannes Schmitz]

I slightly changed the help text in the last version of the patch. It seems
to actually work with other padding bytes if the size is correct. The
problem was a combination of those two things.
It's really hard to compare with hexdump if not all the bytes are exactly
identical. So setting the padding byte to the same value as uboot save
command uses helped me to finally debug and understand the problem.

There was another suggestion to slightly extend the help text regarding the
size a bit which I yet have to implement.

Regards
Johannes


Am 13.12.2017 7:23 nachm. schrieb "Yann E. MORIN" <yann.morin.1998@free.fr>:

Thomas, Johannes, All,

On 2017-12-12 06:27 +0100, Thomas Petazzoni spake thusly:
> On Mon, 11 Dec 2017 19:47:08 +0100, Johannes Schmitz wrote:
[--SNIP--]
> > +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> > +   hex "Padding byte"
> > +   default 0x00
> > +   help
> > +     The byte used for padding at the end of the environment image.
>
> I'm still not sure what is the point of customizing the padding byte.
> Does someone has a use-case for that ? If not, I'm not sure it makes
> sense to add a Buildroot option for it.

As far as I understand, this is because the U-Boot on Johannes' board
would expect the padding byte to be 0x00 when the default for mkenvimage
is 0xFF.

I have no idea why U-Boot expects 0x00 in Johannes' case, though. The
only thing I can think of is that the 'default' byte value on flash
after a page-erase depends on the technology: for NOR, the value after
erase ix 0x00, while for NAND, it is 0xFF. So maybe that's the reason...

But any case, as you said in a further reply, adding that new option
does not add much complexity.

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

--
.-----------------.--------------------.------------------.-
-------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics'
conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___
   |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is
no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v
 conspiracy.  |
'------------------------------^-------^------------------^-
-------------------'
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20171213/7e8dd683/attachment.html>

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Thomas Petazzoni]

Hello,

On Wed, 13 Dec 2017 19:23:32 +0100, Yann E. MORIN wrote:

> > I'm still not sure what is the point of customizing the padding byte.
> > Does someone has a use-case for that ? If not, I'm not sure it makes
> > sense to add a Buildroot option for it.  
> 
> As far as I understand, this is because the U-Boot on Johannes' board
> would expect the padding byte to be 0x00 when the default for mkenvimage
> is 0xFF.

No, U-Boot does not care at all what the padding byte is. Johannes
board works just fine with the default padding byte of 0xff. All what
matters for U-Boot is that the CRC is correct.

Obviously if your environment image has a size of X bytes, its CRC is
calculated over X bytes. Then if the U-Boot configuration says the
environment has Y bytes, it calculates the CRC on Y bytes. So if X !=
Y, you're screwed, as the CRCs won't match.

Johannes problem was *not* a padding byte problem, but *only* a wrong
environment size.

> I have no idea why U-Boot expects 0x00 in Johannes' case, though. 

It does not. Johannes' U-Boot works fine with 0xff as the padding byte.

> The only thing I can think of is that the 'default' byte value on flash
> after a page-erase depends on the technology: for NOR, the value after
> erase ix 0x00, while for NAND, it is 0xFF. So maybe that's the reason...
> 
> But any case, as you said in a further reply, adding that new option
> does not add much complexity.

But it serves no purpose whatsoever :-)

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

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Johannes Schmitz]

Am 14.12.2017 7:13 vorm. schrieb "Thomas Petazzoni" <
thomas.petazzoni@free-electrons.com>:

Hello,

On Wed, 13 Dec 2017 19:23:32 +0100, Yann E. MORIN wrote:

>
> But any case, as you said in a further reply, adding that new option
> does not add much complexity.

But it serves no purpose whatsoever :-)


I disagree with this but maybe it's my electrical engineering background, I
always want full control down to the physical layer.
And mostly there is a use case even if I don't know all of them myself.

I have a question though, is 32k the default uboot environment size? Then I
could also set this as a default. Otherwise again it is a good argument for
keeping the padding option in for debugging  purposes.

Regards
Johannes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20171214/a3be6725/attachment.html>

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Thomas Petazzoni]

Hello,

On Thu, 14 Dec 2017 08:06:13 +0100, Johannes Schmitz wrote:

> I disagree with this but maybe it's my electrical engineering background, I
> always want full control down to the physical layer.
> And mostly there is a use case even if I don't know all of them myself.

If Buildroot starts to support use cases that we don't even understand
when they are needed, then we are going on a very, very slippery road.

> I have a question though, is 32k the default uboot environment size? Then I
> could also set this as a default. Otherwise again it is a good argument for
> keeping the padding option in for debugging  purposes.

No, there is nothing like a default environment size. Each board has
its own value in its U-boot configuration file. 

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

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Peter Korsgaard]

>>>>> "Thomas" == Thomas Petazzoni <thomas.petazzoni@free-electrons.com> writes:

 > Hello,
 > On Thu, 14 Dec 2017 08:06:13 +0100, Johannes Schmitz wrote:

 >> I disagree with this but maybe it's my electrical engineering background, I
 >> always want full control down to the physical layer.
 >> And mostly there is a use case even if I don't know all of them myself.

 > If Buildroot starts to support use cases that we don't even understand
 > when they are needed, then we are going on a very, very slippery road.

I agree. I was marked this patch as rejected in patchwork.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Johannes Schmitz]

The size of the uboot environment (which is provided in bytes -> help
text updated) as well as the padding bytes have to be exactly correct in
order to generate an image with the correct CRC checksum that can be
read by the target during boot. Hence we add the option to specify the
padding byte which is used by mkenvimage and updated the help texts to
inform the user about the importance of these parameters.

Also cleaned up and reindented all mkenvimage parameters.

Signed-off-by: Johannes Schmitz <johannes.schmitz1@gmail.com>
---
 boot/uboot/Config.in | 11 +++++++++--
 boot/uboot/uboot.mk  | 10 ++++++----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 8215912..b52fb81 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -400,8 +400,15 @@ config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE
 config BR2_TARGET_UBOOT_ENVIMAGE_SIZE
 	string "Size of environment"
 	help
-	  Size of envronment, can be prefixed with 0x for hexadecimal
-	  values.
+	  Size of environment in bytes, can be prefixed with 0x for hexadecimal
+	  values. Needs to match exactly for correct CRC checksum calculation.
+
+config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
+	hex "Padding byte"
+	default 0x00
+	help
+	  The byte used for padding at the end of the environment image.
+	  Needs to be correct for correct CRC checksum calculation.
 
 config BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT
 	bool "Environment has two copies"
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index a1fac7d..8796e8d 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -251,10 +251,12 @@ define UBOOT_INSTALL_IMAGES_CMDS
 	)
 	$(if $(BR2_TARGET_UBOOT_ENVIMAGE),
 		cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) | \
-			$(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
-			$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
-			$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
-			-o $(BINARIES_DIR)/uboot-env.bin -)
+			$(HOST_DIR)/bin/mkenvimage \
+				$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
+				$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
+				-p $(BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE) \
+				-s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
+				-o $(BINARIES_DIR)/uboot-env.bin -)
 	$(if $(BR2_TARGET_UBOOT_BOOT_SCRIPT),
 		$(HOST_DIR)/bin/mkimage -C none -A $(MKIMAGE_ARCH) -T script \
 			-d $(call qstrip,$(BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE)) \
-- 
2.7.4

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Johannes Schmitz]

The size of the uboot environment (which is in bytes -> help text
updated) as well as the padding bytes have to be exactly correct in
order to generate an image with the correct CRC checksum that can be
read by the target during boot. Hence we add the option to specify the
padding byte which is used by mkenvimage and updated the help texts to
inform the user about the importance of these parameters.

Also cleaned up and reindented all mkenvimage parameters.

Signed-off-by: Johannes Schmitz <johannes.schmitz1@gmail.com>
---
 boot/uboot/Config.in | 11 +++++++++--
 boot/uboot/uboot.mk  | 10 ++++++----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 8215912..b52fb81 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -400,8 +400,15 @@ config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE
 config BR2_TARGET_UBOOT_ENVIMAGE_SIZE
 	string "Size of environment"
 	help
-	  Size of envronment, can be prefixed with 0x for hexadecimal
-	  values.
+	  Size of environment in bytes, can be prefixed with 0x for hexadecimal
+	  values. Needs to match exactly for correct CRC checksum calculation.
+
+config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
+	hex "Padding byte"
+	default 0x00
+	help
+	  The byte used for padding at the end of the environment image.
+	  Needs to be correct for correct CRC checksum calculation.
 
 config BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT
 	bool "Environment has two copies"
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index a1fac7d..8796e8d 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -251,10 +251,12 @@ define UBOOT_INSTALL_IMAGES_CMDS
 	)
 	$(if $(BR2_TARGET_UBOOT_ENVIMAGE),
 		cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) | \
-			$(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
-			$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
-			$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
-			-o $(BINARIES_DIR)/uboot-env.bin -)
+			$(HOST_DIR)/bin/mkenvimage \
+				$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
+				$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
+				-p $(BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE) \
+				-s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
+				-o $(BINARIES_DIR)/uboot-env.bin -)
 	$(if $(BR2_TARGET_UBOOT_BOOT_SCRIPT),
 		$(HOST_DIR)/bin/mkimage -C none -A $(MKIMAGE_ARCH) -T script \
 			-d $(call qstrip,$(BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE)) \
-- 
2.7.4

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Yann E. MORIN]

Johannes, All,

On 2017-12-10 17:43 +0100, Johannes Schmitz spake thusly:
> The size of the uboot environment as well as the padding bytes have to
> be exactly correct in order to generate an image with the correct CRC
> checksum that can be read by the target during boot. Hence add the
> option to specify the padding byte which is used by mkenvimage and
> updated the help texts to inform the user about the importance of these
> parameters.

As said on IRC, I would love to see more commit loges like this one: it
explains the problem, and how the patch solves it.

Yet, a few comments below...

> Signed-off-by: Johannes Schmitz <johannes.schmitz1@gmail.com>
> ---
>  boot/uboot/Config.in | 11 +++++++++--
>  boot/uboot/uboot.mk  | 10 ++++++----
>  2 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
> index 8215912..b52fb81 100644
> --- a/boot/uboot/Config.in
> +++ b/boot/uboot/Config.in
> @@ -400,8 +400,15 @@ config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE
>  config BR2_TARGET_UBOOT_ENVIMAGE_SIZE
>  	string "Size of environment"
>  	help
> -	  Size of envronment, can be prefixed with 0x for hexadecimal
> -	  values.
> +	  Size of environment in bytes, can be prefixed with 0x for hexadecimal
> +	  values. Needs to match exactly for correct CRC checksum calculation.

Now you also state the size must be expressed in bytes, say so in the
commit log as well.

> +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> +	hex "Padding byte"
> +	default 0x00
> +	help
> +	  The byte used for padding at the end of the environment image.
> +	  Needs to be correct for correct CRC checksum calculation.
>  
>  config BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT
>  	bool "Environment has two copies"
> diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
> index a1fac7d..8796e8d 100644
> --- a/boot/uboot/uboot.mk
> +++ b/boot/uboot/uboot.mk
> @@ -251,10 +251,12 @@ define UBOOT_INSTALL_IMAGES_CMDS
>  	)
>  	$(if $(BR2_TARGET_UBOOT_ENVIMAGE),
>  		cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) | \
> -			$(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
> -			$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
> -			$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
> -			-o $(BINARIES_DIR)/uboot-env.bin -)
> +			$(HOST_DIR)/bin/mkenvimage \
> +				$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
> +				$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
> +				-p $(BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE) \
> +				-s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
> +				-o $(BINARIES_DIR)/uboot-env.bin -)

You re-indent here. State so in the commit log.

Otherwise, that's fine.

Of course, this conflicts with my own patch touching the same code, but
it's pretty obvious how to fix it then:
    https://patchwork.ozlabs.org/patch/846557/

Regards,
Yann E. MORIN.

>  	$(if $(BR2_TARGET_UBOOT_BOOT_SCRIPT),
>  		$(HOST_DIR)/bin/mkimage -C none -A $(MKIMAGE_ARCH) -T script \
>  			-d $(call qstrip,$(BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE)) \
> -- 
> 2.7.4
> 

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Johannes Schmitz]

The size of the uboot environment as well as the padding bytes have to
be exactly correct in order to generate an image with the correct CRC
checksum that can be read by the target during boot. Hence add the
option to specify the padding byte which is used by mkenvimage and
updated the help texts to inform the user about the importance of these
parameters.

Signed-off-by: Johannes Schmitz <johannes.schmitz1@gmail.com>
---
 boot/uboot/Config.in | 11 +++++++++--
 boot/uboot/uboot.mk  | 10 ++++++----
 2 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/boot/uboot/Config.in b/boot/uboot/Config.in
index 8215912..b52fb81 100644
--- a/boot/uboot/Config.in
+++ b/boot/uboot/Config.in
@@ -400,8 +400,15 @@ config BR2_TARGET_UBOOT_ENVIMAGE_SOURCE
 config BR2_TARGET_UBOOT_ENVIMAGE_SIZE
 	string "Size of environment"
 	help
-	  Size of envronment, can be prefixed with 0x for hexadecimal
-	  values.
+	  Size of environment in bytes, can be prefixed with 0x for hexadecimal
+	  values. Needs to match exactly for correct CRC checksum calculation.
+
+config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
+	hex "Padding byte"
+	default 0x00
+	help
+	  The byte used for padding at the end of the environment image.
+	  Needs to be correct for correct CRC checksum calculation.
 
 config BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT
 	bool "Environment has two copies"
diff --git a/boot/uboot/uboot.mk b/boot/uboot/uboot.mk
index a1fac7d..8796e8d 100644
--- a/boot/uboot/uboot.mk
+++ b/boot/uboot/uboot.mk
@@ -251,10 +251,12 @@ define UBOOT_INSTALL_IMAGES_CMDS
 	)
 	$(if $(BR2_TARGET_UBOOT_ENVIMAGE),
 		cat $(call qstrip,$(BR2_TARGET_UBOOT_ENVIMAGE_SOURCE)) | \
-			$(HOST_DIR)/bin/mkenvimage -s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
-			$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
-			$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
-			-o $(BINARIES_DIR)/uboot-env.bin -)
+			$(HOST_DIR)/bin/mkenvimage \
+				$(if $(BR2_TARGET_UBOOT_ENVIMAGE_REDUNDANT),-r) \
+				$(if $(filter BIG,$(BR2_ENDIAN)),-b) \
+				-p $(BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE) \
+				-s $(BR2_TARGET_UBOOT_ENVIMAGE_SIZE) \
+				-o $(BINARIES_DIR)/uboot-env.bin -)
 	$(if $(BR2_TARGET_UBOOT_BOOT_SCRIPT),
 		$(HOST_DIR)/bin/mkimage -C none -A $(MKIMAGE_ARCH) -T script \
 			-d $(call qstrip,$(BR2_TARGET_UBOOT_BOOT_SCRIPT_SOURCE)) \
-- 
2.7.4

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Thomas Petazzoni]

Hello,

On Sun, 10 Dec 2017 17:59:19 +0100, Johannes Schmitz wrote:

> +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> +	hex "Padding byte"
> +	default 0x00
> +	help
> +	  The byte used for padding at the end of the environment image.
> +	  Needs to be correct for correct CRC checksum calculation.

I've followed somewhat remotely the discussion on IRC, and I don't
understand why changing the padding byte makes any difference for you.

Indeed, the CRC is calculated including the padding bytes. So the
padding bytes can be whatever value, as long as the CRC is matching. So
I don't understand why on your platform you are forced to use a
specific padding byte value.

Best regards,

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

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Johannes Schmitz]

Hello Thomas,
this is a good question. So far I just know that reading the environment
only worked for me with a 0x00 padding. Just to be sure it was not a
combined problem of size and padding I can try again with the mkenvimage
default of 0xFF and the correct size.
In any case the padding option should be added to expose the full
mkenvimage capabilities, however if you're right I might have to edit the
help text again and remove the statement about the CRC calculation.

Regards
Johannes

Am 11.12.2017 7:18 vorm. schrieb "Thomas Petazzoni" <
thomas.petazzoni@free-electrons.com>:

Hello,

On Sun, 10 Dec 2017 17:59:19 +0100, Johannes Schmitz wrote:

> +config BR2_TARGET_UBOOT_ENVIMAGE_PADDING_BYTE
> +     hex "Padding byte"
> +     default 0x00
> +     help
> +       The byte used for padding at the end of the environment image.
> +       Needs to be correct for correct CRC checksum calculation.

I've followed somewhat remotely the discussion on IRC, and I don't
understand why changing the padding byte makes any difference for you.

Indeed, the CRC is calculated including the padding bytes. So the
padding bytes can be whatever value, as long as the CRC is matching. So
I don't understand why on your platform you are forced to use a
specific padding byte value.

Best regards,

Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.busybox.net/pipermail/buildroot/attachments/20171211/527a4e9b/attachment.html>

[Buildroot] [PATCH 1/1] boot/uboot: add config option for uboot environment padding byte

[Thomas Petazzoni]

Hello,

On Mon, 11 Dec 2017 07:48:06 +0100, Johannes Schmitz wrote:

> this is a good question. So far I just know that reading the environment
> only worked for me with a 0x00 padding. Just to be sure it was not a
> combined problem of size and padding I can try again with the mkenvimage
> default of 0xFF and the correct size.

Yes, it would be good to investigate some more. The fact that the CRC
was not matching the original environment CRC is fine, because the
padding was different. So the padding being different cannot on its own
explain why it was not working for you.

> In any case the padding option should be added to expose the full
> mkenvimage capabilities, however if you're right I might have to edit the
> help text again and remove the statement about the CRC calculation.

Yes, that's my thought: the help text you're proposing is potentially
wrong.

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