| Message ID | 1513018028-18256-1-git-send-email-johannes.schmitz1@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [1/1] boot/uboot: add config option for uboot environment padding byte | expand |
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
> +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 <div dir="auto"><div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="quoted-text">> +config BR2_TARGET_UBOOT_ENVIMAGE_<wbr>PADDING_BYTE<br> > + hex "Padding byte"<br> > + default 0x00<br> > + help<br> > + The byte used for padding at the end of the environment image.<br> <br> </div>I'm still not sure what is the point of customizing the padding byte.<br> Does someone has a use-case for that ? If not, I'm not sure it makes<br> sense to add a Buildroot option for it.</blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">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. </div><div dir="auto"><br></div><div dir="auto">Regards </div><div dir="auto">Johannes </div></div>
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, 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.
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. | '------------------------------^-------^------------------^- -------------------' <div dir="auto"><div>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. <div dir="auto">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. </div><div dir="auto"><br></div><div dir="auto">There was another suggestion to slightly extend the help text regarding the size a bit which I yet have to implement. </div><div dir="auto"><br></div><div dir="auto">Regards </div><div dir="auto">Johannes </div><br><div class="gmail_extra"><br><div class="gmail_quote">Am 13.12.2017 7:23 nachm. schrieb "Yann E. MORIN" <<a href="mailto:yann.morin.1998@free.fr">yann.morin.1998@free.fr</a>>:<br type="attribution"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thomas, Johannes, All,<br> <br> On 2017-12-12 06:27 +0100, Thomas Petazzoni spake thusly:<br> <div class="quoted-text">> On Mon, 11 Dec 2017 19:47:08 +0100, Johannes Schmitz wrote:<br> </div>[--SNIP--]<br> <div class="quoted-text">> > +config BR2_TARGET_UBOOT_ENVIMAGE_<wbr>PADDING_BYTE<br> > > + hex "Padding byte"<br> > > + default 0x00<br> > > + help<br> > > + The byte used for padding at the end of the environment image.<br> ><br> > I'm still not sure what is the point of customizing the padding byte.<br> > Does someone has a use-case for that ? If not, I'm not sure it makes<br> > sense to add a Buildroot option for it.<br> <br> </div>As far as I understand, this is because the U-Boot on Johannes' board<br> would expect the padding byte to be 0x00 when the default for mkenvimage<br> is 0xFF.<br> <br> I have no idea why U-Boot expects 0x00 in Johannes' case, though. The<br> only thing I can think of is that the 'default' byte value on flash<br> after a page-erase depends on the technology: for NOR, the value after<br> erase ix 0x00, while for NAND, it is 0xFF. So maybe that's the reason...<br> <br> But any case, as you said in a further reply, adding that new option<br> does not add much complexity.<br> <br> Reviewed-by: "Yann E. MORIN" <<a href="mailto:yann.morin.1998@free.fr">yann.morin.1998@free.fr</a>><br> <br> Regards,<br> Yann E. MORIN.<br> <font color="#888888"><br> --<br> .-----------------.-----------<wbr>---------.------------------.-<wbr>-------------------.<br> | Yann E. MORIN | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |<br> | <a href="tel:%2B33%20662%20376%20056" value="+33662376056">+33 662 376 056</a> | Software Designer | \ / CAMPAIGN | ___ |<br> | <a href="tel:%2B33%20223%20225%20172" value="+33223225172">+33 223 225 172</a> `------------.-------: X AGAINST | \e/ There is no |<br> | <a href="http://ymorin.is-a-geek.org/" rel="noreferrer" target="_blank">http://ymorin.is-a-geek.org/</a> | _/*\_ | / \ HTML MAIL | v conspiracy. |<br> '-----------------------------<wbr>-^-------^------------------^-<wbr>-------------------'<br> </font></blockquote></div><br></div></div></div>
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
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 <div dir="auto"><div><br><div class="gmail_extra"><br><div class="gmail_quote">Am 14.12.2017 7:13 vorm. schrieb "Thomas Petazzoni" <<a href="mailto:thomas.petazzoni@free-electrons.com">thomas.petazzoni@free-electrons.com</a>>:<br type="attribution"><blockquote class="quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello,<br> <div class="quoted-text"><br> On Wed, 13 Dec 2017 19:23:32 +0100, Yann E. MORIN wrote:<br> <br></div><div class="quoted-text"> ><br> > But any case, as you said in a further reply, adding that new option<br> > does not add much complexity.<br> <br> </div>But it serves no purpose whatsoever :-)</blockquote></div></div></div><div dir="auto"><br></div><div dir="auto">I disagree with this but maybe it's my electrical engineering background, I always want full control down to the physical layer. </div><div dir="auto">And mostly there is a use case even if I don't know all of them myself. </div><div dir="auto"><br></div><div dir="auto">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. </div><div dir="auto"><br></div><div dir="auto">Regards </div><div dir="auto">Johannes </div><div dir="auto"></div></div>
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" == 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.
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)) \
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(-)