[U-Boot] [PATCH 3/3] test/py: add spi_flash tests
Liam Beguin
liambeguin at gmail.com
Thu Mar 1 11:48:08 UTC 2018
On 1 March 2018 at 01:59, Michal Simek <michal.simek at xilinx.com> wrote:
> On 1.3.2018 05:01, Liam Beguin wrote:
>> 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?
>
> I am not working with broken HW now but on incorrectly setup DDR
> controller you can get systematic error for certain bits that's why if
> you simply shift the second read you should be to find these issues.
>
I understand what you mean here but I'm wondering if this shouldn't be
added to test_md.py instead since it's more about testing the DDR.
>>
>>> 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/ ?
>
> right.
>
>>
>> I guess I could add an extra test to read unaligned addresses. would that
>> work? should the extra offset also be randomized?
>
> two tests are fine. We were talking internally about have more
> randomization in connection to data and test flow that's why we have
> changed that test to work with random values in that certain range.
> Also the part of that is randomized test order but I found that
> pytest-random-order does work properly.
I'll make sure test_sf.py runs with pytest-random-order enabled.
>
> Also these randomized data/lengths were able to find out more issues
> than fixed one. I was playing with that randomized test order and I see
> that not all current tests are self contained and this will be good to
> run too.
>
I'll add options to randomize in v2.
>>
>>>
>>>
>>>> +
>>>> + 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`.
>
> Ok good.
>
> Thanks,
> Michal
Thanks,
Liam
More information about the U-Boot
mailing list