[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').