[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 at 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