[U-Boot] [PATCH v2 0/8] add inital SF tests

[U-Boot] [PATCH v2 0/8] add inital SF tests

[Liam Beguin]

Hi all,

This is the inital step to adding tests for the SF subsystem plus very
minor fixes. It is based on work I found on the mailing list[1].
For now, it doesn't do much but I plan on adding code to reset the flash
to its initial state (based on an env flag) and more code to test the
`sf protect` subcommand (which is the main goal of this series).
I'm sending it now to make sure it's headed in the right direction.

Base on Stephen's comment[2], I haven't added the radomized features.
I'll see how this iteration goes and maybe add it later.

Changes since v1:
 - remove unnecessary skip flag from environment
 - move crc32() to u_boot_utils.py and add extra checks
 - rewrite sf_prepare to return a dict of parameter
 - use assert instead of pytest.fail
 - remove verbose from sf_prepare()
 - update documentation
 - improve readability
 - use ' consistently instead of "
 - use sf_read() in test_sf_read()
 - rename crc variables
 - add speed parameter with optional random range
 - allow `sf read` to write at 0x00

Thanks,
Liam Beguin

[ 1 ] https://patchwork.ozlabs.org/patch/623061/
[ 2 ] https://lists.denx.de/pipermail/u-boot/2018-March/321688.html

Liam Beguin (8):
  spi: spi_flash: do not fail silently on bad user input
  cmd: sf: fix map_physmem check
  test/py: README: fix typo
  test/py: README: add HOSTNAME to PYTHONPATH
  test/py: do not import pytest multiple times
  test/py: add generic CRC32 function
  test/py: add spi_flash tests

 cmd/sf.c                    |   2 +-
 drivers/mtd/spi/spi_flash.c |   2 +-
 test/py/README.md           |   6 +-
 test/py/tests/test_sf.py    | 223 ++++++++++++++++++++++++++++++++++++++++++++
 test/py/u_boot_utils.py     |  24 ++++-
 5 files changed, 251 insertions(+), 6 deletions(-)
 create mode 100644 test/py/tests/test_sf.py


base-commit: 77bba970e2372b01156c66585db3d6fc751c7178
Published-As: https://github.com/Liambeguin/u-boot/releases/tag/test_sf-v2

-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH v2 1/7] spi: spi_flash: do not fail silently on bad user input

[Liam Beguin]

Make sure the user is notified instead of silently returning an error.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 drivers/mtd/spi/spi_flash.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/spi/spi_flash.c b/drivers/mtd/spi/spi_flash.c
index 294d9f9d79c6..2e61685d3ea4 100644
--- a/drivers/mtd/spi/spi_flash.c
+++ b/drivers/mtd/spi/spi_flash.c
@@ -320,7 +320,7 @@ int spi_flash_cmd_erase_ops(struct spi_flash *flash, u32 offset, size_t len)
 
 	erase_size = flash->erase_size;
 	if (offset % erase_size || len % erase_size) {
-		debug("SF: Erase offset/length not multiple of erase size\n");
+		printf("SF: Erase offset/length not multiple of erase size\n");
 		return -1;
 	}
 
-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH v2 2/7] cmd: sf: fix map_physmem check

[Liam Beguin]

Make sure 0x00 is a valid address to read to. If `addr` is 0x00 then
map_physmem() will return 0 which should be a valid address.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 cmd/sf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/sf.c b/cmd/sf.c
index f971eec781cc..e7ff9a646208 100644
--- a/cmd/sf.c
+++ b/cmd/sf.c
@@ -287,7 +287,7 @@ static int do_spi_flash_read_write(int argc, char * const argv[])
 	}
 
 	buf = map_physmem(addr, len, MAP_WRBACK);
-	if (!buf) {
+	if (!buf && addr) {
 		puts("Failed to map physical memory\n");
 		return 1;
 	}
-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH v2 3/7] test/py: README: fix typo

[Liam Beguin]

Fix a minor typo causing vim (and possibly other) to get confused with
coloring.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 test/py/README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/py/README.md b/test/py/README.md
index eefac377567a..000afce93c4a 100644
--- a/test/py/README.md
+++ b/test/py/README.md
@@ -150,7 +150,7 @@ processing.
   option takes a single argument which is used to filter test names. Simple
   logical operators are supported. For example:
   - `'ums'` runs only tests with "ums" in their name.
-  - ``ut_dm'` runs only tests with "ut_dm" in their name. Note that in this
+  - `'ut_dm'` runs only tests with "ut_dm" in their name. Note that in this
     case, "ut_dm" is a parameter to a test rather than the test name. The full
     test name is e.g. "test_ut[ut_dm_leak]".
   - `'not reset'` runs everything except tests with "reset" in their name.
-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH v2 4/7] test/py: README: add HOSTNAME to PYTHONPATH

[Liam Beguin]

As opposed to PATH, HOSTNAME is not appended to PYTHONPATH
automatically. Lets add it to the examples to make it more
obvious to new users.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 test/py/README.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/py/README.md b/test/py/README.md
index 000afce93c4a..aed2fd063a81 100644
--- a/test/py/README.md
+++ b/test/py/README.md
@@ -320,7 +320,7 @@ If U-Boot has already been built:
 
 ```bash
 PATH=$HOME/ubtest/bin:$PATH \
-    PYTHONPATH=${HOME}/ubtest/py:${PYTHONPATH} \
+    PYTHONPATH=${HOME}/ubtest/py/${HOSTNAME}:${PYTHONPATH} \
     ./test/py/test.py --bd seaboard
 ```
 
@@ -331,7 +331,7 @@ follow:
 ```bash
 CROSS_COMPILE=arm-none-eabi- \
     PATH=$HOME/ubtest/bin:$PATH \
-    PYTHONPATH=${HOME}/ubtest/py:${PYTHONPATH} \
+    PYTHONPATH=${HOME}/ubtest/py/${HOSTNAME}:${PYTHONPATH} \
     ./test/py/test.py --bd seaboard --build
 ```
 
-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH v2 5/7] test/py: do not import pytest multiple times

[Liam Beguin]

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 test/py/u_boot_utils.py | 1 -
 1 file changed, 1 deletion(-)

diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py
index 9acb92ddc448..64584494e463 100644
--- a/test/py/u_boot_utils.py
+++ b/test/py/u_boot_utils.py
@@ -11,7 +11,6 @@ import os.path
 import pytest
 import sys
 import time
-import pytest
 
 def md5sum_data(data):
     """Calculate the MD5 hash of some data.
-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH v2 6/7] test/py: add generic CRC32 function

[Liam Beguin]

Add a generic function which can be used to compute the CRC32 value of
a region of RAM.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 test/py/u_boot_utils.py | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/test/py/u_boot_utils.py b/test/py/u_boot_utils.py
index 64584494e463..de9ee2643f51 100644
--- a/test/py/u_boot_utils.py
+++ b/test/py/u_boot_utils.py
@@ -11,6 +11,7 @@ import os.path
 import pytest
 import sys
 import time
+import re
 
 def md5sum_data(data):
     """Calculate the MD5 hash of some data.
@@ -310,3 +311,25 @@ def persistent_file_helper(u_boot_log, filename):
     """
 
     return PersistentFileHelperCtxMgr(u_boot_log, filename)
+
+def crc32(u_boot_console, address, count):
+    """Helper function used to compute the CRC32 value of a section of RAM.
+
+    Args:
+        u_boot_console: A U-Boot console connection.
+        address: Address where data starts.
+        count: Amount of data to use for calculation.
+
+    Returns:
+        CRC32 value
+    """
+
+    bcfg = u_boot_console.config.buildconfig
+    has_cmd_crc32 = bcfg.get('config_cmd_crc32', 'n') == 'y'
+    assert has_cmd_crc32, 'Cannot compute crc32 without CONFIG_CMD_CRC32.'
+    output = u_boot_console.run_command('crc32 %08x %x' % (address, count))
+
+    m = re.search('==> ([0-9a-fA-F]{8})$', output)
+    assert m, 'CRC32 operation failed.'
+
+    return m.group(1)
-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH v2 7/7] test/py: add spi_flash tests

[Liam Beguin]

Add basic tests for the spi_flash subsystem.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 test/py/tests/test_sf.py | 223 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 223 insertions(+)
 create mode 100644 test/py/tests/test_sf.py

diff --git a/test/py/tests/test_sf.py b/test/py/tests/test_sf.py
new file mode 100644
index 000000000000..0cc2a60e68d4
--- /dev/null
+++ b/test/py/tests/test_sf.py
@@ -0,0 +1,223 @@
+# Copyright (c) 2017, Xiphos Systems Corp. All rights reserved.
+#
+# SPDX-License-Identifier: GPL-2.0
+
+import re
+import pytest
+import random
+import u_boot_utils
+
+
+"""
+Note: This test relies on boardenv_* containing configuration values to define
+which SPI Flash areas are available for testing.  Without this, this test will
+be automatically skipped.
+For example:
+
+# A list of sections of Flash memory to be tested.
+env__sf_configs = (
+    {
+        # Where in SPI Flash should the test operate.
+        'offset': 0x00000000,
+        # This value is optional.
+        #   If present, specifies the [[bus:]cs] argument used in `sf probe`
+        #   If missing, defaults to 0.
+        'id': '0:1',
+        # This value is optional.
+        #   If set as a number, specifies the speed of the SPI Flash.
+        #   If set as an array of 2, specifies a range for a random speed.
+        #   If missing, defaults to 0.
+        'speed': 1000000,
+        # This value is optional.
+        #   If present, specifies the size to use for read/write operations.
+        #   If missing, the SPI Flash page size is used as a default (based on
+        #   the `sf probe` output).
+        'len': 0x10000,
+        # This value is optional.
+        #   If present, specifies if the test can write to Flash offset
+        #   If missing, defaults to False.
+        'writeable': False,
+        # This value is optional.
+        #   If present, specifies the expected CRC32 value of the flash area.
+        #   If missing, extra check is ignored.
+        'crc32': 0xCAFECAFE,
+    },
+)
+"""
+
+
+def sf_prepare(u_boot_console, env__sf_config):
+    """Check global state of the SPI Flash before running any test.
+
+   Args:
+        u_boot_console: A U-Boot console connection.
+        env__sf_config: The single SPI Flash device configuration on which to
+            run the tests.
+
+    Returns:
+        sf_params: a dictionnary of SPI Flash parameters.
+    """
+
+    sf_params = {}
+    sf_params['ram_base'] = u_boot_utils.find_ram_base(u_boot_console)
+
+    probe_id = env__sf_config.get('id', 0)
+    speed = env__sf_config.get('speed', 0)
+    if isinstance(speed, list) and len(speed) == 2:
+        sf_params['speed'] = random.randint(speed[0], speed[1])
+    else:
+        sf_params['speed'] = speed
+
+    cmd = 'sf probe %d %d' % (probe_id, sf_params['speed'])
+
+    output = u_boot_console.run_command(cmd)
+    assert 'SF: Detected' in output, 'No Flash device available'
+
+    m = re.search('page size (.+?) Bytes', output)
+    assert m, 'SPI Flash page size not recognized'
+    sf_params['page_size'] = int(m.group(1))
+
+    m = re.search('erase size (.+?) KiB', output)
+    assert m, 'SPI Flash erase size not recognized'
+    sf_params['erase_size'] = int(m.group(1))
+    sf_params['erase_size'] *= 1024
+
+    m = re.search('total (.+?) MiB', output)
+    assert m, 'SPI Flash total size not recognized'
+    sf_params['total_size'] = int(m.group(1))
+    sf_params['total_size'] *= 1024 * 1024
+
+    assert env__sf_config['offset'] is not None, \
+        '\'offset\' is required for this test.'
+    sf_params['len'] = env__sf_config.get('len', sf_params['erase_size'])
+
+    assert not env__sf_config['offset'] % sf_params['erase_size'], \
+        'offset not multiple of erase size.'
+    assert not sf_params['len'] % sf_params['erase_size'], \
+        'erase length not multiple of erase size.'
+
+    assert not (env__sf_config.get('writeable', False) and
+                env__sf_config.get('crc32', False)), \
+        'Cannot check crc32 on  writeable sections'
+
+    return sf_params
+
+
+def sf_read(u_boot_console, env__sf_config, sf_params):
+    """Helper function used to read and compute the CRC32 value of a section of
+    SPI Flash memory.
+
+    Args:
+        u_boot_console: A U-Boot console connection.
+        env__sf_config: The single SPI Flash device configuration on which to
+            run the tests.
+        sf_params: SPI Flash parameters.
+
+    Returns:
+        CRC32 value of SPI Flash section
+    """
+
+    addr = sf_params['ram_base']
+    offset = env__sf_config['offset']
+    count = sf_params['len']
+    pattern = random.randint(0, 0xFFFFFFFF)
+    crc_expected = env__sf_config.get('crc32', None)
+
+    cmd = 'mw.b %08x %08x %x' % (addr, pattern, count)
+    u_boot_console.run_command(cmd)
+    crc_read = u_boot_utils.crc32(u_boot_console, addr, count)
+    if crc_expected:
+        assert crc_read != crc_expected
+
+    cmd = 'sf read %08x %08x %x' % (addr, offset, count)
+    response = u_boot_console.run_command(cmd)
+    assert 'Read: OK' in response, 'Read operation failed'
+    crc_readback = u_boot_utils.crc32(u_boot_console, addr, count)
+    assert crc_read != crc_readback, 'sf read did not update RAM content.'
+    if crc_expected:
+        assert crc_readback == crc_expected
+
+    return crc_readback
+
+
+def sf_update(u_boot_console, env__sf_config, sf_params):
+    """Helper function used to update a section of SPI Flash memory.
+
+   Args:
+        u_boot_console: A U-Boot console connection.
+        env__sf_config: The single SPI Flash device configuration on which to
+           run the tests.
+
+    Returns:
+        CRC32 value of SPI Flash section
+    """
+
+    addr = sf_params['ram_base']
+    offset = env__sf_config['offset']
+    count = sf_params['len']
+    pattern = int(random.random() * 0xFFFFFFFF)
+
+    cmd = 'mw.b %08x %08x %x' % (addr, pattern, count)
+    u_boot_console.run_command(cmd)
+    crc_read = u_boot_utils.crc32(u_boot_console, addr, count)
+
+    cmd = 'sf update %08x %08x %x' % (addr, offset, count)
+    u_boot_console.run_command(cmd)
+    crc_readback = sf_read(u_boot_console, env__sf_config, sf_params)
+
+    assert crc_readback == crc_read
+
+
+ at pytest.mark.buildconfigspec('cmd_sf')
+ at pytest.mark.buildconfigspec('cmd_crc32')
+ at pytest.mark.buildconfigspec('cmd_memory')
+def test_sf_read(u_boot_console, env__sf_config):
+    sf_params = sf_prepare(u_boot_console, env__sf_config)
+    sf_read(u_boot_console, env__sf_config, sf_params)
+
+
+ at pytest.mark.buildconfigspec('cmd_sf')
+ at pytest.mark.buildconfigspec('cmd_crc32')
+ at pytest.mark.buildconfigspec('cmd_memory')
+def test_sf_read_twice(u_boot_console, env__sf_config):
+    sf_params = sf_prepare(u_boot_console, env__sf_config)
+
+    crc1 = sf_read(u_boot_console, env__sf_config, sf_params)
+    sf_params['ram_base'] += 0x100
+    crc2 = sf_read(u_boot_console, env__sf_config, sf_params)
+
+    assert crc1 == crc2, 'CRC32 of two successive read operation do not match'
+
+
+ at pytest.mark.buildconfigspec('cmd_sf')
+ at pytest.mark.buildconfigspec('cmd_crc32')
+ at pytest.mark.buildconfigspec('cmd_memory')
+def test_sf_erase(u_boot_console, env__sf_config):
+    if not env__sf_config.get('writeable', False):
+        pytest.skip('Flash config is tagged as not writeable')
+
+    sf_params = sf_prepare(u_boot_console, env__sf_config)
+    addr = sf_params['ram_base']
+    offset = env__sf_config['offset']
+    count = sf_params['len']
+
+    cmd = 'sf erase %08x %x' % (offset, count)
+    output = u_boot_console.run_command(cmd)
+    assert 'Erased: OK' in output, 'Erase operation failed'
+
+    cmd = 'mw.b %08x ffffffff %x' % (addr, count)
+    u_boot_console.run_command(cmd)
+    crc_ffs = u_boot_utils.crc32(u_boot_console, addr, count)
+
+    crc_read = sf_read(u_boot_console, env__sf_config, sf_params)
+    assert crc_ffs == crc_read, 'Unexpected CRC32 after erase operation.'
+
+
+ at pytest.mark.buildconfigspec('cmd_sf')
+ at pytest.mark.buildconfigspec('cmd_memory')
+def test_sf_update(u_boot_console, env__sf_config):
+    if not env__sf_config.get('writeable', False):
+        pytest.skip('Flash config is tagged as not writeable')
+
+    sf_params = sf_prepare(u_boot_console, env__sf_config)
+    sf_update(u_boot_console, env__sf_config, sf_params)
-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH v2 0/8] add inital SF tests

[Liam Beguin]

Hi everyone,

Any new comments on this iteration?

Thanks,
Liam

On Sun, 4 Mar 2018 at 23:22 Liam Beguin <liambeguin@gmail.com> wrote:

> Hi all,
>
> This is the inital step to adding tests for the SF subsystem plus very
> minor fixes. It is based on work I found on the mailing list[1].
> For now, it doesn't do much but I plan on adding code to reset the flash
> to its initial state (based on an env flag) and more code to test the
> `sf protect` subcommand (which is the main goal of this series).
> I'm sending it now to make sure it's headed in the right direction.
>
> Base on Stephen's comment[2], I haven't added the radomized features.
> I'll see how this iteration goes and maybe add it later.
>
> Changes since v1:
>  - remove unnecessary skip flag from environment
>  - move crc32() to u_boot_utils.py and add extra checks
>  - rewrite sf_prepare to return a dict of parameter
>  - use assert instead of pytest.fail
>  - remove verbose from sf_prepare()
>  - update documentation
>  - improve readability
>  - use ' consistently instead of "
>  - use sf_read() in test_sf_read()
>  - rename crc variables
>  - add speed parameter with optional random range
>  - allow `sf read` to write at 0x00
>
> Thanks,
> Liam Beguin
>
> [ 1 ] https://patchwork.ozlabs.org/patch/623061/
> [ 2 ] https://lists.denx.de/pipermail/u-boot/2018-March/321688.html
>
> Liam Beguin (8):
>   spi: spi_flash: do not fail silently on bad user input
>   cmd: sf: fix map_physmem check
>   test/py: README: fix typo
>   test/py: README: add HOSTNAME to PYTHONPATH
>   test/py: do not import pytest multiple times
>   test/py: add generic CRC32 function
>   test/py: add spi_flash tests
>
>  cmd/sf.c                    |   2 +-
>  drivers/mtd/spi/spi_flash.c |   2 +-
>  test/py/README.md           |   6 +-
>  test/py/tests/test_sf.py    | 223
> ++++++++++++++++++++++++++++++++++++++++++++
>  test/py/u_boot_utils.py     |  24 ++++-
>  5 files changed, 251 insertions(+), 6 deletions(-)
>  create mode 100644 test/py/tests/test_sf.py
>
>
> base-commit: 77bba970e2372b01156c66585db3d6fc751c7178
> Published-As: https://github.com/Liambeguin/u-boot/releases/tag/test_sf-v2
>
> --
> 2.16.1.72.g5be1f00a9a70
>
>

[U-Boot] [PATCH v2 6/7] test/py: add generic CRC32 function

[Stephen Warren]

On 03/04/2018 09:22 PM, Liam Beguin wrote:
> Add a generic function which can be used to compute the CRC32 value of
> a region of RAM.

Patches 1-6,
Reviewed-by: Stephen Warren <swarren@nvidia.com>

[U-Boot] [PATCH v2 7/7] test/py: add spi_flash tests

[Stephen Warren]

On 03/04/2018 09:22 PM, Liam Beguin wrote:
> Add basic tests for the spi_flash subsystem.

Looks good. A few small issues:

> +def sf_prepare(u_boot_console, env__sf_config):
...
> +    speed = env__sf_config.get('speed', 0)
> +    if isinstance(speed, list) and len(speed) == 2:
> +        sf_params['speed'] = random.randint(speed[0], speed[1])
> +    else:
> +        sf_params['speed'] = speed

What if speed is a tuple or other indexable type not a list? Perhaps 
invert the test. Also, perhaps assume any list has two entries, or 
assert that.

if isintance(speed, int):
   int case
else:
   assert len(speed) == 2, "Speed must have two entries"
   list/tuple case

> +    assert env__sf_config['offset'] is not None, \
> +        '\'offset\' is required for this test.'

Is this meant to test for:
a) A key that's present, with value set to None.
b) A missing key.

It currently tests (a), but testing for (b) seems more likely to catch 
issues. Perhaps:

assert env__sf_config.get('offset', None) is not None

or:

assert 'offset' in env__sf_config.get

> +    assert not (env__sf_config.get('writeable', False) and
> +                env__sf_config.get('crc32', False)), \
> +        'Cannot check crc32 on  writeable sections'

What if the crc32 value is 0, which IIRC is False? Unlikely admittedly, 
but you never know. Perhaps:

assert not (env__sf_config.get('writeable', False) and
             'crc32' in env__sf_config.get), \
         'Cannot check crc32 on  writeable sections'

> +def sf_read(u_boot_console, env__sf_config, sf_params):
> +    addr = sf_params['ram_base']
> +    offset = env__sf_config['offset']
> +    count = sf_params['len']
> +    pattern = random.randint(0, 0xFFFFFFFF)

0xFF not 0xFFFFFFFF since it's bytes.

> +    crc_expected = env__sf_config.get('crc32', None)
> +
> +    cmd = 'mw.b %08x %08x %x' % (addr, pattern, count)

%02x for pattern.

> +    u_boot_console.run_command(cmd)
> +    crc_read = u_boot_utils.crc32(u_boot_console, addr, count)

crc_read sounds like a CRC for data that's been read. Perhaps crc_pattern?

> +def sf_update(u_boot_console, env__sf_config, sf_params):
> +    addr = sf_params['ram_base']
> +    offset = env__sf_config['offset']
> +    count = sf_params['len']
> +    pattern = int(random.random() * 0xFFFFFFFF)

0xFF.

> +    cmd = 'mw.b %08x %08x %x' % (addr, pattern, count)

%02x for pattern.

> +    u_boot_console.run_command(cmd)
> +    crc_read = u_boot_utils.crc32(u_boot_console, addr, count)

crc_pattern?

> +def test_sf_erase(u_boot_console, env__sf_config):
...
> +    cmd = 'mw.b %08x ffffffff %x' % (addr, count)

Just ff not ffffffff?

> + at pytest.mark.buildconfigspec('cmd_sf')
> + at pytest.mark.buildconfigspec('cmd_memory')
> +def test_sf_update(u_boot_console, env__sf_config):

sf_update() unconditionally calls u_boot_utils.crc32 which asserts if 
the crc32 command isn't available, so this function needs to be 
@pytest.mark.buildconfigspec('cmd_crc32').

[U-Boot] [PATCH v2 7/7] test/py: add spi_flash tests

[Liam Beguin]

Hi Stephen,

On 13 March 2018 at 17:41, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 03/04/2018 09:22 PM, Liam Beguin wrote:
>>
>> Add basic tests for the spi_flash subsystem.
>
>
> Looks good. A few small issues:
>
>> +def sf_prepare(u_boot_console, env__sf_config):
>
> ...
>>
>> +    speed = env__sf_config.get('speed', 0)
>> +    if isinstance(speed, list) and len(speed) == 2:
>> +        sf_params['speed'] = random.randint(speed[0], speed[1])
>> +    else:
>> +        sf_params['speed'] = speed
>
>
> What if speed is a tuple or other indexable type not a list? Perhaps invert
> the test. Also, perhaps assume any list has two entries, or assert that.
>
> if isintance(speed, int):
>   int case
> else:
>   assert len(speed) == 2, "Speed must have two entries"
>   list/tuple case
>

Right, that's better!

>> +    assert env__sf_config['offset'] is not None, \
>> +        '\'offset\' is required for this test.'
>
>
> Is this meant to test for:
> a) A key that's present, with value set to None.
> b) A missing key.
>
> It currently tests (a), but testing for (b) seems more likely to catch
> issues. Perhaps:
>
> assert env__sf_config.get('offset', None) is not None
>
> or:
>
> assert 'offset' in env__sf_config.get
>

Thanks for seeing this, will fix.

>> +    assert not (env__sf_config.get('writeable', False) and
>> +                env__sf_config.get('crc32', False)), \
>> +        'Cannot check crc32 on  writeable sections'
>
>
> What if the crc32 value is 0, which IIRC is False? Unlikely admittedly, but
> you never know. Perhaps:
>
> assert not (env__sf_config.get('writeable', False) and
>             'crc32' in env__sf_config.get), \
>         'Cannot check crc32 on  writeable sections'
>

Ok

>> +def sf_read(u_boot_console, env__sf_config, sf_params):
>> +    addr = sf_params['ram_base']
>> +    offset = env__sf_config['offset']
>> +    count = sf_params['len']
>> +    pattern = random.randint(0, 0xFFFFFFFF)
>
>
> 0xFF not 0xFFFFFFFF since it's bytes.
>

Will fix across file...

>> +    crc_expected = env__sf_config.get('crc32', None)
>> +
>> +    cmd = 'mw.b %08x %08x %x' % (addr, pattern, count)
>
>
> %02x for pattern.
>
>> +    u_boot_console.run_command(cmd)
>> +    crc_read = u_boot_utils.crc32(u_boot_console, addr, count)
>
>
> crc_read sounds like a CRC for data that's been read. Perhaps crc_pattern?

crc_pattern sounds good.

>
>> +def sf_update(u_boot_console, env__sf_config, sf_params):
>> +    addr = sf_params['ram_base']
>> +    offset = env__sf_config['offset']
>> +    count = sf_params['len']
>> +    pattern = int(random.random() * 0xFFFFFFFF)
>
>
> 0xFF.
>
>> +    cmd = 'mw.b %08x %08x %x' % (addr, pattern, count)
>
>
> %02x for pattern.
>
>> +    u_boot_console.run_command(cmd)
>> +    crc_read = u_boot_utils.crc32(u_boot_console, addr, count)
>
>
> crc_pattern?
>
>> +def test_sf_erase(u_boot_console, env__sf_config):
>
> ...
>>
>> +    cmd = 'mw.b %08x ffffffff %x' % (addr, count)
>
>
> Just ff not ffffffff?
>
>> + at pytest.mark.buildconfigspec('cmd_sf')
>> + at pytest.mark.buildconfigspec('cmd_memory')
>> +def test_sf_update(u_boot_console, env__sf_config):
>
>
> sf_update() unconditionally calls u_boot_utils.crc32 which asserts if the
> crc32 command isn't available, so this function needs to be
> @pytest.mark.buildconfigspec('cmd_crc32').

You're right, I missed this one...

Thanks for taking the time to review this series,
Liam Beguin