[U-Boot] [PATCH 3/3] test/py: add spi_flash tests
Michal Simek
michal.simek at xilinx.com
Thu Mar 1 11:58:01 UTC 2018
On 1.3.2018 12:48, Liam Beguin wrote:
> 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.
crc test should be there as it is the part of cmd/mem.c.
On the other hand none is saying that the same DDR configuration is
shared for the whole DDR in the system. On our devices you can have one
DDR setup to hard code and the second for soft core that's why I would
suggest to never use the same offsets for repeated read.
Also in connection to address randomization will be also good to cover
cases above 4GB address space. It means for systems with multiple memory
banks test "read" to every bank. If this is continues then cross that
boundary with data. But this could be done as follow up patch.
Thanks,
Michal
More information about the U-Boot
mailing list