[flasher PATCH V2 1/4] Add crc32 verification of the flash image

[flasher PATCH V2 1/4] Add crc32 verification of the flash image

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Verify the CRC32 of the flash image at two points in time:

1) Before starting the flashing process, to validate the download of the
   image into Tegra's RAM.

2) After writing the image to flash, read it back into RAM, in order to
   validate that it was correctly written to flash.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2: Use binascii.crc32() rather than shelling out to crc32 binary

(Note that I'm only resending 1/4 and 3/4 for V2; the others are unchanged)
---
 tegra-uboot-flasher | 34 ++++++++++++++++++++++++++++------
 1 file changed, 28 insertions(+), 6 deletions(-)

diff --git a/tegra-uboot-flasher b/tegra-uboot-flasher
index 41879c396b2f..3c696c23604f 100755
--- a/tegra-uboot-flasher
+++ b/tegra-uboot-flasher
@@ -21,6 +21,7 @@
 # DEALINGS IN THE SOFTWARE.
 
 import argparse
+import binascii
 import os
 import os.path
 import shutil
@@ -51,23 +52,26 @@ def run(dir, cmd):
         raise Exception('Command failed: %d' % ret)
     os.chdir(oldcwd)
 
-def gen_flashcmd_mmc(flash_image_addr, flash_img_size):
+def gen_flashcmd_mmc(flash_image_addr, readback_addr, flash_img_size):
     flash_id = config['flash-id-uboot']
     flash_img_size_sectors = flash_img_size / 512
     flashcmd = 'mmc dev %d 1 ; ' % flash_id
     flashcmd += 'mmc write 0x%08x 0 0x%x ; ' % (flash_image_addr, flash_img_size_sectors)
+    flashcmd += 'mmc read 0x%08x 0 0x%x ; ' % (readback_addr, flash_img_size_sectors)
     return flashcmd
 
-def gen_flashcmd_nand(flash_image_addr, flash_img_size):
+def gen_flashcmd_nand(flash_image_addr, readback_addr, flash_img_size):
     flashcmd = 'nand erase.chip ; '
     flashcmd += 'nand write 0x%08x 0 0x%08x ; ' % (flash_image_addr, flash_img_size)
+    flashcmd += 'nand read 0x%08x 0 0x%08x ; ' % (readback_addr, flash_img_size)
     return flashcmd
 
-def gen_flashcmd_spi(flash_image_addr, flash_img_size):
+def gen_flashcmd_spi(flash_image_addr, readback_addr, flash_img_size):
     flash_id = config.get('flash-id-uboot', '0')
     flashcmd = 'sf probe %s ; ' % flash_id
     flashcmd += 'sf erase 0 0x%08x ; ' % config['flash-erase-size']
     flashcmd += 'sf write 0x%08x 0 0x%08x ; ' % (flash_image_addr, flash_img_size)
+    flashcmd += 'sf read 0x%08x 0 0x%08x ; ' % (readback_addr, flash_img_size)
     return flashcmd
 
 gen_flashcmds = {
@@ -125,6 +129,19 @@ def func_flash():
     if args.debug:
         print 'flash_img_size %d 0x%x' % (flash_img_size, flash_img_size)
 
+    imgf = file(flash_img, 'rb')
+    imgd = imgf.read()
+    imgf.close()
+    flash_img_crc32 = binascii.crc32(imgd)
+    if args.debug:
+        print 'flash_img_crc32 %x' % flash_img_crc32
+    flash_img_crc32_bs = (
+        ((flash_img_crc32 & 0xff) << 24) |
+        ((flash_img_crc32 & 0xff00) << 8) |
+        ((flash_img_crc32 & 0xff0000) >> 8) |
+        ((flash_img_crc32 & 0xff000000) >> 24)
+    )
+
     u_boot_plus_dtb_size = u_boot_no_dtb_size + u_boot_dtb_size
     if args.debug:
         print 'u_boot_plus_dtb_size %d 0x%x' % (u_boot_plus_dtb_size, u_boot_plus_dtb_size)
@@ -144,6 +161,9 @@ def func_flash():
     flash_image_addr = loadaddr + padded_size
     if args.debug:
         print 'flash_image_addr %d 0x%x' % (flash_image_addr, flash_image_addr)
+    readback_addr = flash_image_addr + flash_img_size
+    if args.debug:
+        print 'readback_addr %d 0x%x' % (readback_addr, readback_addr)
 
     flash_type = config['flash-type']
     if not gen_flashcmds.has_key(flash_type):
@@ -165,9 +185,11 @@ def func_flash():
         run(workdir, cmd)
 
         bootcmd = ''
-        if args.debug:
-            bootcmd = 'crc32 0x%08x 0x%08x ; ' % (flash_image_addr, flash_img_size)
-        bootcmd += gen_flashcmd(flash_image_addr, flash_img_size)
+        bootcmd += 'crc32 0x%08x 0x%08x 0x%08x ; ' % (flash_image_addr, flash_img_size, soc['ram-base'])
+        bootcmd += 'if itest.l *0x%08x != 0x%x; then echo CRC MISMATCH of initial image; exit; fi ; ' % (soc['ram-base'], flash_img_crc32_bs)
+        bootcmd += gen_flashcmd(flash_image_addr, readback_addr, flash_img_size)
+        bootcmd += 'crc32 0x%08x 0x%08x 0x%08x ; ' % (readback_addr, flash_img_size, soc['ram-base'])
+        bootcmd += 'if itest.l *0x%08x != 0x%x; then echo CRC MISMATCH of readback image; exit; fi ; ' % (soc['ram-base'], flash_img_crc32_bs)
         bootcmd += 'env default -f -a ; '
         # Perhaps U-Boot should set $boardname based on the ID EEPROM; then we wouldn't need this
         if config['dtbfn-extra'] != '':
-- 
1.8.1.5

[flasher PATCH V2 3/4] Allow overriding environment variables during flashing

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

The flashing process resets the U-Boot environment to the built-in
default. Allow the user to specify some non-default values to be set
before saving the environment. This can be useful e.g. to set up the
default boot target:

./tegra-uboot-flasher flash --env boot_targets dhcp beaver

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
v2:
* Don't fail if no --env cmdline options are used.
* Enhance help text to:
  - Explicitly mention that the option can be used multiple times.
    (in --help output only, not the short help)
  - Say "--env VAR VALUE" not "--env ENV ENV" so the syntax is clear.
---
 tegra-uboot-flasher | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tegra-uboot-flasher b/tegra-uboot-flasher
index c9eb811db937..ea36ba1efd7a 100755
--- a/tegra-uboot-flasher
+++ b/tegra-uboot-flasher
@@ -195,6 +195,9 @@ def func_flash():
         # Perhaps U-Boot should set $boardname based on the ID EEPROM; then we wouldn't need this
         if config['dtbfn-extra'] != '':
             bootcmd += 'setenv board ' + boardname + config['dtbfn-extra'] + ' ; '
+        if args.env:
+            for (var, value) in args.env:
+                bootcmd += 'setenv %s %s ; ' % (var, value)
         bootcmd += 'saveenv ; '
         # To update the bootloader, reset.
         # If wanting to run installer, set installer_args.configname in environment, 'run bootcmd'
@@ -283,6 +286,10 @@ parser_flash.add_argument('--flash-image', type=str,
     help='The flash image to write, instead of U-Boot itself')
 parser_flash.add_argument('--gen-only', action='store_true',
     help='Just create the work-dir; don\'t actually flash the image')
+parser_flash.add_argument('--env', type=str, nargs=2, action='append',
+    metavar=("VAR", "VALUE"),
+    help='Set a U-Boot environment variable after flashing. This option ' +
+        'may be given multiple times')
 
 parser_exec = subparsers.add_parser('exec',
     help='Download and execute an unmodified bootloader binary, named ' +
-- 
1.8.1.5

Re: [flasher PATCH V2 1/4] Add crc32 verification of the flash image

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]

On Fri, Dec 06, 2013 at 10:35:33AM -0700, Stephen Warren wrote:
[...]
> diff --git a/tegra-uboot-flasher b/tegra-uboot-flasher
[...]
> @@ -125,6 +129,19 @@ def func_flash():
>      if args.debug:
>          print 'flash_img_size %d 0x%x' % (flash_img_size, flash_img_size)
>  
> +    imgf = file(flash_img, 'rb')
> +    imgd = imgf.read()
> +    imgf.close()
> +    flash_img_crc32 = binascii.crc32(imgd)
> +    if args.debug:
> +        print 'flash_img_crc32 %x' % flash_img_crc32
> +    flash_img_crc32_bs = (
> +        ((flash_img_crc32 & 0xff) << 24) |
> +        ((flash_img_crc32 & 0xff00) << 8) |
> +        ((flash_img_crc32 & 0xff0000) >> 8) |
> +        ((flash_img_crc32 & 0xff000000) >> 24)
> +    )

I would've thought that Python actually supported byteswapping with some
function, but it seems not (or at least not trivially). One could do
something like this:

	a = array.array('I', [crc])
	a.byteswap()
	crc = a[0]

But with the array module you apparently can't force 32-bit values. 'I'
will be at least 2, but 4 on 32-bit and 64-bit systems, 'L' will be 64
bits on 64-bit systems and 32 bits on 32-bit systems it seems. You could
check a.itemsize to determine the right type code. I suppose we won't be
running on anything but 32- or 64-bit systems for a while, but it still
said there's no support for sized types there.

It could possibly be done with the struct module as well, but that would
likely end up much more verbose too, so:

Reviewed-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [flasher PATCH V2 3/4] Allow overriding environment variables during flashing

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 998 bytes --]

On Fri, Dec 06, 2013 at 10:35:34AM -0700, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> The flashing process resets the U-Boot environment to the built-in
> default. Allow the user to specify some non-default values to be set
> before saving the environment. This can be useful e.g. to set up the
> default boot target:
> 
> ./tegra-uboot-flasher flash --env boot_targets dhcp beaver
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
> v2:
> * Don't fail if no --env cmdline options are used.
> * Enhance help text to:
>   - Explicitly mention that the option can be used multiple times.
>     (in --help output only, not the short help)
>   - Say "--env VAR VALUE" not "--env ENV ENV" so the syntax is clear.
> ---
>  tegra-uboot-flasher | 7 +++++++
>  1 file changed, 7 insertions(+)

Looks good:

Reviewed-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: [flasher PATCH V2 1/4] Add crc32 verification of the flash image

[Stephen Warren]

On 12/06/2013 12:56 PM, Thierry Reding wrote:
> On Fri, Dec 06, 2013 at 10:35:33AM -0700, Stephen Warren wrote:
> [...]
>> diff --git a/tegra-uboot-flasher b/tegra-uboot-flasher
> [...]
>> @@ -125,6 +129,19 @@ def func_flash():
>>      if args.debug:
>>          print 'flash_img_size %d 0x%x' % (flash_img_size, flash_img_size)
>>  
>> +    imgf = file(flash_img, 'rb')
>> +    imgd = imgf.read()
>> +    imgf.close()
>> +    flash_img_crc32 = binascii.crc32(imgd)
>> +    if args.debug:
>> +        print 'flash_img_crc32 %x' % flash_img_crc32
>> +    flash_img_crc32_bs = (
>> +        ((flash_img_crc32 & 0xff) << 24) |
>> +        ((flash_img_crc32 & 0xff00) << 8) |
>> +        ((flash_img_crc32 & 0xff0000) >> 8) |
>> +        ((flash_img_crc32 & 0xff000000) >> 24)
>> +    )
> 
> I would've thought that Python actually supported byteswapping with some
> function, but it seems not (or at least not trivially). One could do
> something like this:
> 
> 	a = array.array('I', [crc])
> 	a.byteswap()
> 	crc = a[0]
> 
> But with the array module you apparently can't force 32-bit values. 'I'
> will be at least 2, but 4 on 32-bit and 64-bit systems, 'L' will be 64
> bits on 64-bit systems and 32 bits on 32-bit systems it seems. You could
> check a.itemsize to determine the right type code. I suppose we won't be
> running on anything but 32- or 64-bit systems for a while, but it still
> said there's no support for sized types there.
> 
> It could possibly be done with the struct module as well, but that would
> likely end up much more verbose too, so:

Thanks. I did wonder about using the struct module, but I also figured
that since I'd already written the conversion, and struct would be
more complex, I wouldn't bother.

I'll push out the whole 4-patch series in a second.