Re: [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  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, 0x)
>
>
> 0xFF not 0x 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() * 0x)
>
>
> 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  %x' % (addr, count)
>
>
> Just ff not ?
>
>> +@pytest.mark.buildconfigspec('cmd_sf')
>> +@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
___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [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, 0x)


0xFF not 0x 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() * 0x)


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  %x' % (addr, count)


Just ff not ?


+@pytest.mark.buildconfigspec('cmd_sf')
+@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 mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot