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

[Stephen Warren]

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.

> +"""
> +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.

> +# 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.

> +        # 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.

> +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.

> +    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 ?

> +    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:
    ...

> +    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.

> +    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.

> +    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.

> +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.

> +def sf_read(u_boot_console, env__sf_config, size=None):

Is size ever used? If not, let's delete it.

> +    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.

> +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.

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.

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.

> +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.