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

[Liam Beguin]

On 1 March 2018 at 16:56, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 02/26/2018 09:17 PM, Liam Beguin wrote:
>>
>> Add basic tests for the spi_flash subsystem.
>
>
>> diff --git a/test/py/tests/test_sf.py b/test/py/tests/test_sf.py
>
>
>> +import re
>> +import pytest
>> +import u_boot_utils
>> +
>> +
>
>
> Nit: Double blank line. The same issue exists in many other places too.

This is because I use flake8 and it was complaining if I didn't. Apparently
PEP8 suggests you surround top level functions with 2 blank lines.
I really have no preference.

>
>> +"""
>> +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:
>> +
>> +# Boolean indicating whether the SF tests should be skipped.
>> +env__sf_skip = False
>
>
> Why do we need this variable? Let's just have the test rely upon whether
> env__sf_configs is present or not. If your test function takes
> env__sf_config as an argument, it'll automatically be skipped by the core
> test code if env__sf_configs isn't defined, or is defined by empty.

Will do.

>
>> +# A list of sections of flash memory to be tested.
>> +env__sf_configs = (
>> +    {
>> +        # Optional, [[bus:]cs] argument used in `sf probe`
>> +        'id': "0",
>
>
> What is the default value if this isn't specified? I assume "0", but that
> would be good to document.

The default value is an empty string. I'll change it to 0 so I can test
different SPI frequencies.

>
>> +        # 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,
>
>
> Is len in u8s or u32s? I would assume bytes. len is used inconsistently
> below; as a parameter to "sf read" where it's interpreted as bytes, and as a
> parameter to "mw" where it's interpreted as words IIRC.

Thanks for pointing this out! I'll update call to `mw` to use bytes instead of
words.

>
>> +def sf_prepare(u_boot_console, env__sf_config, verbose=False):
>
>
>> +    # NOTE: sf read at address 0 fails because map_physmem 'converts' it
>> +    #       address 0 to a pointer.
>> +    ram_address = u_boot_utils.find_ram_base(u_boot_console) + 0x10
>
>
> Can we fix that bug instead? Other tests don't need that workaround, so I'm
> not sure why the SPI test does.

Good point. Let me see what I can do.

>
>> +    probe_id = env__sf_config.get('id', '')
>> +    output = u_boot_console.run_command('sf probe ' + probe_id)
>> +    if 'SF: Detected' not in output:
>> +        pytest.fail('No flash device available')
>
>
> assert 'SF: Detected' in output ?

Yes, I'll also replace all other `pytest.fail` with asserts.

>
>> +    m = re.search('page size (.+?) Bytes', output)
>> +    if m:
>> +        try:
>> +            page_size = int(m.group(1))
>> +        except ValueError:
>> +            pytest.fail('SPI Flash page size not recognized')
>
>
> What if not m? Currently the code leaves page_size uninitialized. Shouldn't
> the logic be:
>
> m = re.search ...
> assert m
> try:
>    ...
>

Will fix.

>> +    if verbose:
>> +        u_boot_console.log.info('Page size is: ' + str(page_size) + ' B')
>
>
> Shorter might be:
>
> u_boot_console.log.info('Page size is: %dB' % page_size)
>
>> +    env__sf_config['len'] = env__sf_config.get('len', erase_size)
>
>
> Over-writing len changes the value in the original structure which could
> affect tests that want to handle missing/inconsistent data in other ways.
> I'd rather store the data using a different key that the user will never
> set, or even better store derived/calculated data in a different dictionary
> altogether.

Sound like a better solution. I'll update `sf_prepare` to return a dictionary
of calculated data.

>
>> +    if env__sf_config['offset'] % erase_size or \
>> +            env__sf_config['len'] % erase_size:
>> +        u_boot_console.log.warning("erase offset/length not multiple of "
>> +                                   "erase size")
>
>
> I'd assert that here, rather than just warning.

Ok.

>
>> +    env__sf_config['ram_address'] = ram_address
>
>
> ram_address isn't user-supplied configuration, and doesn't vary from
> test-to-test. Let's either return it, or write it to a global, or just call
> find_ram_base() whenever the value is required.

I'll add it to the returned dictionary.

>
>> +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
>> +    """
>> +
>> +    output = u_boot_console.run_command('crc32 %08x %x' % (address,
>> count))
>> +
>> +    m = re.search('==> ([0-9a-fA-F]{8})$', output)
>> +    if not m:
>> +        pytest.fail('CRC32 failed')
>> +
>> +    return m.group(1)
>
>
> Should we put this into u_boot_utils.py? I was going to assert that other
> tests must already do this, but I guess other tests check for whether a
> known-ahead-of-time CRC value is on the crc32 command output, rather than
> parsing out the actual value. So, maybe this doesn't need to be a utility
> function.

As it is quite generic, I think it's a good idea to move it to u_boot_utils.py.

>
>> +def sf_read(u_boot_console, env__sf_config, size=None):
>
>
> Is size ever used? If not, let's delete it.

I forgot to remove it from a previous iteration. Will remove.

>
>> +    u_boot_console.run_command('mw %08x 0 %x' %
>> (env__sf_config['ram_address'],
>> +                                                 size))
>
>
> mw.b instead? Same in sf_update().
>
> This function could benefit from doing:
>
> a) Fill target memory with known pattern.
> b) sf read.
> c) Check that target memory doesn't contain the known pattern.
>

Will do.

>> +def sf_update(u_boot_console, env__sf_config):
>
>
>> +    from time import time
>
>
> That's not needed. It should be at the top of the file if it was.
>
>> +    u_boot_console.run_command('mw %08x %08x %x' %
>> +                               (env__sf_config['ram_address'], time(),
>> +                                env__sf_config['len']))
>> +    crc_ram = crc32(u_boot_console, env__sf_config['ram_address'],
>> +                    env__sf_config['len'])
>> +    u_boot_console.run_command('sf update %08x %08x %x' %
>> +                               (env__sf_config['ram_address'],
>> +                                env__sf_config['offset'],
>> +                                env__sf_config['len']))
>
>
> Here the RAM should be cleared, although I guess that can happen inside
> sf_read().
>
>> +    crc2 = sf_read(u_boot_console, env__sf_config)
>> +
>> +    return (crc2 == crc_ram)
>
>
>> + at pytest.mark.buildconfigspec("cmd_sf")
>> +def test_sf_read(u_boot_console, env__sf_config):
>> +    sf_prepare(u_boot_console, env__sf_config)
>> +
>> +    output = u_boot_console.run_command('sf read %08x %08x %x' %
>> +                                        (env__sf_config['ram_address'],
>> +                                         env__sf_config['offset'],
>> +                                         env__sf_config['len']))
>> +    assert 'Read: OK' in output, "Read operation failed"
>
>
> This doesn't verify that the command actually read anything, since it
> doesn't look for changes in the RAM. Let's "mw.b ram_address offset len"
> first and check that the CRC changes before/after the read to ensure that
> some bytes were actually transferred. If you take a look at the test_mmc_rd
> patch I sent recently, you can see how to do that. It'd be nice to structure
> the two tests as similarly as possible since they're doing roughly the same
> thing.

Ok, I'll have a look at that. If it's similar, I can use the updated
`sf_read()` you
propose above.

>
> Also, it'd be good if env__sf_configs specified a known CRC for data that
> should be in the SPI flash, so we can verify not only that /some/ data was
> read, but that the /correct/ data was read.

Ok, I'll add something like `expected_crc32` to env__sf_configs.

>
> Why doesn't this function use the sf_read helper function like
> test_sf_read_twice() does?
>
>> + 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_prepare(u_boot_console, env__sf_config)
>> +
>> +    crc1 = sf_read(u_boot_console, env__sf_config)
>> +    crc2 = sf_read(u_boot_console, env__sf_config)
>> +
>> +    assert crc1 == crc2, "CRC32 of two successive read operation do not
>> match"
>
>
> Similarly here, the test will pass if neither commands reads anything, or if
> only the first does, or if they both read the wrong thing.

It's true that this will pass if both commands don't read anything. I think what
you suggested for `sf_read` will fix this.

>
>> +def test_sf_erase(u_boot_console, env__sf_config):
>
> ...
>>
>> +    u_boot_console.run_command('mw %08x ffffffff %x' %
>> +                               (env__sf_config['ram_address'],
>> +                                env__sf_config['len']))
>> +    crc1 = crc32(u_boot_console, env__sf_config['ram_address'],
>> +                 env__sf_config['len'])
>
>
> It'd be nice to name that crc_ffs or something like that ...
>
>> +
>> +    crc2 = sf_read(u_boot_console, env__sf_config)
>
>
> ... and name that crc_read or crc_readback, just to give a quick/easy idea
> of what each variable holds.

Will do.

Thanks for your time,
Liam