[U-Boot] [PATCH 3/3] test/py: add spi_flash tests
Liam Beguin
liambeguin at gmail.com
Thu Mar 1 04:01:08 UTC 2018
Hi Michal,
On 27 February 2018 at 03:51, Michal Simek <michal.simek at xilinx.com> wrote:
> Hi Liam,
>
> On 27.2.2018 05:17, Liam Beguin wrote:
>> Add basic tests for the spi_flash subsystem.
>
> Good to see this patch.
> FYI: We have created qspi tests too but didn't push it out because of
> missing sf_configs options as you have below. We are testing the whole
> flash all the time.
I tried to write this based on your test plus the feedback from when you
sent it on the mailing list.
> Test was designed to use more randomization to make sure that every run
> is working with different data. We found several issues thanks to this.
> (Also keep in your mind that some tests depends on order which is wrong
> but we didn't have time to clean them yet)
> IIRC I have sent that patch to mainline in past and using sf_configs was
> suggested too.
>
> https://github.com/Xilinx/u-boot-xlnx/blob/master/test/py/tests/test_qspi.py
>
Looking at the test, I see you randomize the spi frequency and the R/W size.
I could probably add these as env configs with default values.
>>
>> Signed-off-by: Liam Beguin <liambeguin at gmail.com>
>> ---
>> test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 233 insertions(+)
>>
>> diff --git a/test/py/tests/test_sf.py b/test/py/tests/test_sf.py
>> new file mode 100644
>> index 000000000000..7017d8072ea9
>> --- /dev/null
>> +++ b/test/py/tests/test_sf.py
>> @@ -0,0 +1,233 @@
>> +# Copyright (c) 2017, Xiphos Systems Corp. All rights reserved.
>> +#
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +import re
>> +import pytest
>> +import u_boot_utils
>> +
>> +
>> +"""
>> +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
>> +
>> +# A list of sections of flash memory to be tested.
>> +env__sf_configs = (
>> + {
>> + # Optional, [[bus:]cs] argument used in `sf probe`
>> + 'id': "0",
>> + # Where in SPI flash should the test operate.
>> + 'offset': 0x00000000,
>> + # 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,
>> + # Specifies if the test can write to offset
>> + 'writeable': False,
>> + },
>> +)
>> +"""
>> +
>> +
>> +def sf_prepare(u_boot_console, env__sf_config, verbose=False):
>> + """Check global state of the SPI Flash before running any test.
>> +
>> + Args:
>> + u_boot_console: A U-Boot console connection.
>> + env__sf_config: The single SPI flash device configuration on which to
>> + run the tests.
>> +
>> + Returns:
>> + Nothing.
>> + """
>> +
>> + if u_boot_console.config.env.get('env__sf_skip', True):
>> + pytest.skip('sf test disabled in environment')
>> +
>> + # 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
>
> Maybe you can add it to sf_configs too.
I'm not sure what it would add to the test but I could make the
configuration optional and default to find_ram_base().
>
>
>> +
>> + 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')
>> +
>> + 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')
>> +
>> + m = re.search('erase size (.+?) KiB', output)
>> + if m:
>> + try:
>> + erase_size = int(m.group(1))
>> + except ValueError:
>> + pytest.fail('SPI Flash erase size not recognized')
>> +
>> + erase_size *= 1024
>> +
>> + m = re.search('total (.+?) MiB', output)
>> + if m:
>> + try:
>> + total_size = int(m.group(1))
>> + except ValueError:
>> + pytest.fail('SPI Flash total size not recognized')
>> +
>> + total_size *= 1024 * 1024
>> +
>> + if verbose:
>> + u_boot_console.log.info('Page size is: ' + str(page_size) + ' B')
>> + u_boot_console.log.info('Erase size is: ' + str(erase_size) + ' B')
>> + u_boot_console.log.info('Total size is: ' + str(total_size) + ' B')
>> +
>> + env__sf_config['len'] = env__sf_config.get('len', erase_size)
>> + 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")
>
> Is this only warning or you can destroy any data by that?
It's a warning because `sf erase` will fail if this condition is true.
It shouldn't destroy any data. I'm not really sure on how this should
be handled because if this warning appears, the erase and update
operations will fail.
>
>
>
>> +
>> + env__sf_config['ram_address'] = ram_address
>> +
>> +
>> +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)
>> +
>> +
>> +def sf_read(u_boot_console, env__sf_config, size=None):
>> + """Helper function used to read and compute the CRC32 value of a section of
>> + SPI Flash memory.
>> +
>> + Args:
>> + u_boot_console: A U-Boot console connection.
>> + env__sf_config: The single SPI flash device configuration on which to
>> + run the tests.
>> + size: Optional, used to override env__sf_config value.
>> +
>> + Returns:
>> + CRC32 value of SPI Flash section
>> + """
>> +
>> + if size is None:
>> + size = env__sf_config['len']
>> +
>> + u_boot_console.run_command('mw %08x 0 %x' % (env__sf_config['ram_address'],
>> + size))
>
> Any reason for this write? It should be rewritten by sf read below anyway.
Yes, it is rewritten by `sf read`. I added it as an extra precaution
to make sure
that the data changes if we do two consecutive reads of the same address.
>
>> +
>> + response = u_boot_console.run_command('sf read %08x %08x %x' %
>> + (env__sf_config['ram_address'],
>> + env__sf_config['offset'],
>> + size))
>> + assert 'Read: OK' in response, "Read operation failed"
>> +
>> + return crc32(u_boot_console, env__sf_config['ram_address'],
>> + env__sf_config['len'])
>> +
>> +
>> +def sf_update(u_boot_console, env__sf_config):
>> + """Helper function used to update a section of SPI Flash memory.
>> +
>> + Args:
>> + u_boot_console: A U-Boot console connection.
>> + env__sf_config: The single SPI flash device configuration on which to
>> + run the tests.
>> +
>> + Returns:
>> + CRC32 value of SPI Flash section
>> + """
>> + from time import time
>> +
>> + u_boot_console.run_command('mw %08x %08x %x' %
>> + (env__sf_config['ram_address'], time(),
>> + env__sf_config['len']))
>
> ditto
same here.
>
>> + 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']))
>> +
>> + 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"
>> +
>> +
>> + 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)
>
> The test is using the same memory. I tried to avoid that to make sure
> that there is no issue with memory itself. Small offset should be enough.
This is a reason why I added the extra `mw` above.
Do you think it's not enough?
> Also these sf read should be able to write bytes to non align addresses
> to make sure that driver is handling that properly.
s/write/read/ ?
I guess I could add an extra test to read unaligned addresses. would that
work? should the extra offset also be randomized?
>
>
>> +
>> + assert crc1 == crc2, "CRC32 of two successive read operation do not match"
>> +
>> +
>> + at pytest.mark.buildconfigspec("cmd_sf")
>> + at pytest.mark.buildconfigspec("cmd_crc32")
>> + at pytest.mark.buildconfigspec("cmd_memory")
>> +def test_sf_erase(u_boot_console, env__sf_config):
>> + if not env__sf_config['writeable']:
>> + pytest.skip('flash config is tagged as not writeable')
>> +
>> + sf_prepare(u_boot_console, env__sf_config)
>> + output = u_boot_console.run_command('sf erase %08x %x' %
>> + (env__sf_config['offset'],
>> + env__sf_config['len']))
>> + assert 'Erased: OK' in output, "Erase operation failed"
>> +
>> + 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'])
>> +
>> + crc2 = sf_read(u_boot_console, env__sf_config)
>> + assert crc1 == crc2, "CRC32 of erase section does not match expected value"
>> +
>> +
>> + at pytest.mark.buildconfigspec("cmd_sf")
>> + at pytest.mark.buildconfigspec("cmd_memory")
>> +def test_sf_update(u_boot_console, env__sf_config):
>> + if not env__sf_config['writeable']:
>> + pytest.skip('flash config is tagged as not writeable')
>> +
>> + sf_prepare(u_boot_console, env__sf_config)
>> + assert sf_update(u_boot_console, env__sf_config) is True
>>
>
> Also I wanted to design these tests that you will check bytes in front
> of and behind region in DDR to make sure that your driver is writing
> just amount of data you requested not aligned with words for example.
It's a good point! In a follow up series, I was planning on doing this on
the flash after write/erase operations to make sure we don't go
beyond `offset + size`.
>
> Anyway good to see this patch.
>
> Thanks,
> Michal
>
>
Thanks for the feedback,
Liam
More information about the U-Boot
mailing list