add inital SF tests

fix a minor typo causing vim (and possibly other) to get confused with coloring.

Quoted 3 lines · Liam Beguin

fix a minor typo causing vim (and possibly other) to get confused with coloring.

Nit: s/fix/Fix/.

Reviewed-by Stephen Warren <[email protected]>

As opposed to PATH, HOSTNAME is not appended to PYTHONPATH automatically. Lets add it to the examples to make it more obvious to new users.

Add basic tests for the spi_flash subsystem.

Signed-off-by Liam Beguin <[email protected]>

---
 test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 233 insertions(+)

test/py/tests/test_sf.py new file +45 -0

new file mode 100644
index 000000000000..7017d8072ea9
@@ -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.
+

test/py/tests/test_sf.py new file +31 -0

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

test/py/tests/test_sf.py new file +34 -0

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

test/py/tests/test_sf.py new file +17 -0

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

test/py/tests/test_sf.py new file +21 -0

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

test/py/tests/test_sf.py new file +25 -0

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

Hi Liam,

Quoted 1 line · Liam Beguin

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.

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

Quoted 61 lines

Signed-off-by Liam Beguin <[email protected]>

---
 test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 233 insertions(+)

test/py/tests/test_sf.py new file +49 -0

new file mode 100644
index 000000000000..7017d8072ea9
@@ -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.
+    """
+

test/py/tests/test_sf.py line 55

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 37 lines

test/py/tests/test_sf.py new file +35 -0

+
+    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')

test/py/tests/test_sf.py line 96

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 41 lines

test/py/tests/test_sf.py new file +38 -0

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

test/py/tests/test_sf.py line 140

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 22 lines

test/py/tests/test_sf.py new file +21 -0

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

test/py/tests/test_sf.py line 167

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

+    """
+    from time import time
+
+    u_boot_console.run_command('mw %08x %08x %x' %
+                               (env__sf_config['ram_address'], time(),
+                                env__sf_config['len']))

ditto

Quoted 27 lines

test/py/tests/test_sf.py new file +26 -0

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

test/py/tests/test_sf.py line 198

Quoted 5 lines

test/py/tests/test_sf.py new file +5 -0

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

Also these sf read should be able to write bytes to non align addresses to make sure that driver is handling that properly.

Quoted 34 lines

test/py/tests/test_sf.py new file +32 -0

+
+    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')

test/py/tests/test_sf.py line 233

Quoted 4 lines

test/py/tests/test_sf.py new file +3 -0

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

Anyway good to see this patch.

Thanks,
Michal

Hi Michal,

Quoted 9 lines · Michal Simek

Hi Liam,

Quoted 1 line · Liam Beguin

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.

Quoted 9 lines

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.

Quoted 61 lines

Quoted 61 lines

Signed-off-by Liam Beguin <[email protected]>

---
 test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 233 insertions(+)

test/py/tests/test_sf.py new file +49 -0

new file mode 100644
index 000000000000..7017d8072ea9
@@ -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.
+    """
+

Quoted 8 lines

test/py/tests/test_sf.py line 55

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 37 lines

Quoted 37 lines

test/py/tests/test_sf.py new file +35 -0

+
+    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')

Quoted 8 lines

test/py/tests/test_sf.py line 96

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 41 lines

Quoted 41 lines

test/py/tests/test_sf.py new file +38 -0

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

Quoted 8 lines

test/py/tests/test_sf.py line 140

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 22 lines

Quoted 22 lines

test/py/tests/test_sf.py new file +21 -0

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

Quoted 8 lines

test/py/tests/test_sf.py line 167

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 27 lines

Quoted 27 lines

test/py/tests/test_sf.py new file +26 -0

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

Quoted 8 lines

test/py/tests/test_sf.py line 198

Quoted 5 lines

test/py/tests/test_sf.py new file +5 -0

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

Quoted 3 lines

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?

Quoted 35 lines

Quoted 35 lines

test/py/tests/test_sf.py new file +32 -0

+
+    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')

Quoted 8 lines

test/py/tests/test_sf.py line 233

Quoted 4 lines

test/py/tests/test_sf.py new file +3 -0

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

Quoted 276 lines · Liam Beguin

Hi Michal,

Quoted 9 lines · Michal Simek

Hi Liam,

Quoted 1 line · Liam Beguin

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.

Quoted 9 lines

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.

Quoted 61 lines

Quoted 61 lines

Signed-off-by Liam Beguin <[email protected]>

---
 test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 233 insertions(+)

test/py/tests/test_sf.py new file +49 -0

new file mode 100644
index 000000000000..7017d8072ea9
@@ -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.
+    """
+

Quoted 8 lines

test/py/tests/test_sf.py line 55

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 37 lines

Quoted 37 lines

test/py/tests/test_sf.py new file +35 -0

+
+    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')

Quoted 8 lines

test/py/tests/test_sf.py line 96

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 41 lines

Quoted 41 lines

test/py/tests/test_sf.py new file +38 -0

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

Quoted 8 lines

test/py/tests/test_sf.py line 140

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 22 lines

Quoted 22 lines

test/py/tests/test_sf.py new file +21 -0

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

Quoted 8 lines

test/py/tests/test_sf.py line 167

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 35 lines

Quoted 22 lines

test/py/tests/test_sf.py new file +21 -0

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

test/py/tests/test_sf.py line 198

Quoted 10 lines

test/py/tests/test_sf.py new file +10 -0

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

Quoted 2 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 1 line

Quoted 2 lines

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.

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.

Quoted 44 lines

Quoted 43 lines

Quoted 29 lines

test/py/tests/test_sf.py new file +25 -0

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

test/py/tests/test_sf.py line 233

Quoted 10 lines

test/py/tests/test_sf.py new file +10 -0

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

Quoted 3 lines

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

Quoted 280 lines · Michal Simek

Quoted 278 lines · Liam Beguin

Hi Michal,

Quoted 9 lines · Michal Simek

Hi Liam,

Quoted 1 line · Liam Beguin

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.

Quoted 9 lines

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.

Quoted 61 lines

Quoted 61 lines

Signed-off-by Liam Beguin <[email protected]>

---
 test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 233 insertions(+)

test/py/tests/test_sf.py new file +49 -0

new file mode 100644
index 000000000000..7017d8072ea9
@@ -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.
+    """
+

Quoted 8 lines

test/py/tests/test_sf.py line 55

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 37 lines

Quoted 37 lines

test/py/tests/test_sf.py new file +35 -0

+
+    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')

Quoted 8 lines

test/py/tests/test_sf.py line 96

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 41 lines

Quoted 41 lines

test/py/tests/test_sf.py new file +38 -0

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

Quoted 8 lines

test/py/tests/test_sf.py line 140

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 22 lines

Quoted 22 lines

test/py/tests/test_sf.py new file +21 -0

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

Quoted 8 lines

test/py/tests/test_sf.py line 167

Quoted 6 lines

test/py/tests/test_sf.py new file +6 -0

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

Quoted 27 lines

Quoted 27 lines

test/py/tests/test_sf.py new file +26 -0

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

Quoted 8 lines

test/py/tests/test_sf.py line 198

Quoted 5 lines

test/py/tests/test_sf.py new file +5 -0

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

Quoted 4 lines

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.

On 1 March 2018 at 01:59, Michal Simek <[email protected]> wrote:

Quoted 4 lines

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.

Quoted 1 line · Liam Beguin

On 1 March 2018 at 01:59, Michal Simek <[email protected]> wrote:

...

Quoted 6 lines

Quoted 4 lines

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.

I'd strongly prefer the tests to be deterministic and not do random different actions each time they're run. That way, any failures are much easier to track down. Writing randomly generated data is fine, but not random access patterns or sets of operations.

Although I suppose you could implement two sets of tests; one that does a static set of operations driven by a table in the board config file, and one that does a more randomized set of operations controlled by limits set by a table in the board config file.

+ env__sf_config['len'] = env__sf_config.get('len', erase_size)

+ env__sf_config['ram_address'] = ram_address

test/py/tests/test_sf.py +9 -0

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

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

+def test_sf_erase(u_boot_console, env__sf_config):

Quoted 5 lines · Liam Beguin

Add basic tests for the spi_flash subsystem.

test/py/tests/test_sf.py +0 -0

test/py/tests/test_sf.py line 9

Quoted 5 lines

test/py/tests/test_sf.py +5 -0

+import re
+import pytest
+import u_boot_utils
+
+

Nit: Double blank line. The same issue exists in many other places too.

test/py/tests/test_sf.py line 17

Quoted 11 lines

+"""
+Note: This test relies on boardenv_* containing configuration values to define
+which spi flash areas are available for testing. Without this, this test will

test/py/tests/test_sf.py +5 -0

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

test/py/tests/test_sf.py line 23

Quoted 6 lines

test/py/tests/test_sf.py +5 -0

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

test/py/tests/test_sf.py line 30

Quoted 8 lines

test/py/tests/test_sf.py +2 -0

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

test/py/tests/test_sf.py +2 -0

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

test/py/tests/test_sf.py line 55

Quoted 7 lines

test/py/tests/test_sf.py +4 -0

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

test/py/tests/test_sf.py line 60

Quoted 5 lines

test/py/tests/test_sf.py +4 -0

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

test/py/tests/test_sf.py line 67

Quoted 7 lines

test/py/tests/test_sf.py +6 -0

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

   ...

test/py/tests/test_sf.py line 88

Quoted 2 lines

test/py/tests/test_sf.py +2 -0

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

Quoted 1 line

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

test/py/tests/test_sf.py line 96

Quoted 5 lines

test/py/tests/test_sf.py +4 -0

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

Quoted 2 lines

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

Quoted 21 lines

test/py/tests/test_sf.py +2 -0

+def crc32(u_boot_console, address, count):
+    """Helper function used to compute the CRC32 value of a section of

RAM.

test/py/tests/test_sf.py +11 -0

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

test/py/tests/test_sf.py +5 -0

+
+    m = re.search('==> ([0-9a-fA-F]{8})$', output)
+    if not m:
+        pytest.fail('CRC32 failed')
+

Quoted 1 line

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

Quoted 2 lines

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

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

Quoted 4 lines

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

test/py/tests/test_sf.py line 163

Quoted 4 lines

test/py/tests/test_sf.py +2 -0

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

test/py/tests/test_sf.py line 173

Quoted 9 lines

test/py/tests/test_sf.py +9 -0

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

Quoted 4 lines

test/py/tests/test_sf.py +3 -0

+    crc2 = sf_read(u_boot_console, env__sf_config)
+
+    return (crc2 == crc_ram)
 

test/py/tests/test_sf.py line 188

Quoted 10 lines

test/py/tests/test_sf.py +9 -0

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

Quoted 11 lines

+ at pytest.mark.buildconfigspec("cmd_sf")
+ at pytest.mark.buildconfigspec("cmd_crc32")
+ at pytest.mark.buildconfigspec("cmd_memory")

test/py/tests/test_sf.py +7 -0

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

Quoted 2 lines

+def test_sf_erase(u_boot_console, env__sf_config):

...

test/py/tests/test_sf.py line 220

Quoted 6 lines

test/py/tests/test_sf.py +5 -0

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

test/py/tests/test_sf.py

Quoted 2 lines

test/py/tests/test_sf.py +2 -0

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

Quoted 8 lines · Stephen Warren

Quoted 5 lines · Liam Beguin

Add basic tests for the spi_flash subsystem.

test/py/tests/test_sf.py +0 -0

Quoted 8 lines

test/py/tests/test_sf.py line 9

Quoted 5 lines

test/py/tests/test_sf.py +5 -0

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

Quoted 18 lines · Liam Beguin

Quoted 16 lines · Stephen Warren

test/py/tests/test_sf.py line 9

Quoted 12 lines · Liam Beguin

Add basic tests for the spi_flash subsystem.

test/py/tests/test_sf.py +5 -0

 
 
+import re
+import pytest
+import u_boot_utils
+
+

Nit: Double blank line. The same issue exists in many other places too.

Quoted 1 line

This is because I use flake8 and it was complaining if I didn't.

Apparently

Quoted 2 lines

PEP8 suggests you surround top level functions with 2 blank lines. I really have no preference.

Hmm. There are some things about PEP8 I'm not so found of. Either way though, I don't believe any current code in test/py uses double blank lines, and I /think/ the same is true for other Python code in U-Boot.

Looks like I don't have any other comments on your replies.

Hi all,

This is the inital step to adding tests for the SF subsystem plus very minor fixes. It is based on work I found on the mailing list[1]. For now, it doesn't do much but I plan on adding code to reset the flash to its initial state (based on an env flag) and more code to test the sf protect subcommand (which is the main goal of this series). I'm sending it now to make sure it's headed in the right direction.

Thanks,
Liam Beguin

[ 1 ] https://patchwork.ozlabs.org/patch/623061/

Liam Beguin (3):
test/py: README: fix typo
test/py: README: add HOSTNAME to PYTHONPATH
test/py: add spi_flash tests

 test/py/README.md        |   6 +-
 test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 236 insertions(+), 3 deletions(-)
 create mode 100644 test/py/tests/test_sf.py

base-commit: 4bafceff0e9e5a36908031e41c69a6b37e82da58
Published-As: https://github.com/Liambeguin/u-boot/releases/tag/test_sf_v1

Quoted 19 lines · Liam Beguin

Hi all,

This is the inital step to adding tests for the SF subsystem plus very minor fixes. It is based on work I found on the mailing list[1]. For now, it doesn't do much but I plan on adding code to reset the flash to its initial state (based on an env flag) and more code to test the sf protect subcommand (which is the main goal of this series). I'm sending it now to make sure it's headed in the right direction.

Thanks,
Liam Beguin

[ 1 ] https://patchwork.ozlabs.org/patch/623061/

Liam Beguin (3):
test/py: README: fix typo
test/py: README: add HOSTNAME to PYTHONPATH
test/py: add spi_flash tests

Quoted 8 lines

  test/py/README.md        |   6 +-
  test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++
  2 files changed, 236 insertions(+), 3 deletions(-)

create mode 100644 test/py/tests/test_sf.py

base-commit: 4bafceff0e9e5a36908031e41c69a6b37e82da58
Published-As: https://github.com/Liambeguin/u-boot/releases/tag/test_sf_v1

one more question about these tests in general.
I have looked at randomization of these tests and all tests should be written in a way that you can run just single one. That's why every test should be self contained without dependencies on each other.

I just found that running order in hush matters.
test_hush_if_test_setup
test_hush_if_test
test_hush_if_test_teardown

Stephen: Is this expected or this simply just a bug?

Quoted 40 lines · Michal Simek

Quoted 19 lines · Liam Beguin

Hi all,

This is the inital step to adding tests for the SF subsystem plus very minor fixes. It is based on work I found on the mailing list[1]. For now, it doesn't do much but I plan on adding code to reset the flash to its initial state (based on an env flag) and more code to test the sf protect subcommand (which is the main goal of this series). I'm sending it now to make sure it's headed in the right direction.

Thanks,
Liam Beguin

[ 1 ] https://patchwork.ozlabs.org/patch/623061/

Liam Beguin (3):
   test/py: README: fix typo
   test/py: README: add HOSTNAME to PYTHONPATH
   test/py: add spi_flash tests

Quoted 9 lines

  test/py/README.md        |   6 +-
  test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++

  2 files changed, 236 insertions(+), 3 deletions(-)

  create mode 100644 test/py/tests/test_sf.py

base-commit: 4bafceff0e9e5a36908031e41c69a6b37e82da58 Published-As:
https://github.com/Liambeguin/u-boot/releases/tag/test_sf_v1

one more question about these tests in general.
I have looked at randomization of these tests and all tests should be written in a way that you can run just single one. That's why every test should be self contained without dependencies on each other.

I just found that running order in hush matters.
test_hush_if_test_setup
test_hush_if_test
test_hush_if_test_teardown

Quoted 1 line

Stephen: Is this expected or this simply just a bug?

This is expected given the way the test is currently written. You can easily run all 3 tests together by specifying "-k test_hush_if_test"; that string is a substring of all the 3 tests above. I guess we could rework test_hush_if_test_setup/test_hush_if_test_teardown to be a pytest module-level fixture in order to squash those 3 functions into a single test though.

Hi all,

This is the inital step to adding tests for the SF subsystem plus very minor fixes. It is based on work I found on the mailing list[1]. For now, it doesn't do much but I plan on adding code to reset the flash to its initial state (based on an env flag) and more code to test the sf protect subcommand (which is the main goal of this series). I'm sending it now to make sure it's headed in the right direction.

Liam Beguin wrote at Monday, February 26, 2018 9:18 PM:

Quoted 2 lines

Hi all,

Quoted 6 lines

This is the inital step to adding tests for the SF subsystem plus very minor fixes. It is based on work I found on the mailing list[1]. For now, it doesn't do much but I plan on adding code to reset the flash to its initial state (based on an env flag) and more code to test the sf protect subcommand (which is the main goal of this series). I'm sending it now to make sure it's headed in the right direction.

These patches didn't make it to the U-Boot mailing list, although a couple of replies did. You probably want to resend them.

--
nvpublic

Hi,

Quoted 11 lines · On Tue, 27 Feb 2018 at 11:29 Stephen Warren <[email protected]> wrote:

Liam Beguin wrote at Monday, February 26, 2018 9:18 PM:

Quoted 8 lines

Hi all,

This is the inital step to adding tests for the SF subsystem plus very minor fixes. It is based on work I found on the mailing list[1]. For now, it doesn't do much but I plan on adding code to reset the flash to its initial state (based on an env flag) and more code to test the sf protect subcommand (which is the main goal of this series). I'm sending it now to make sure it's headed in the right direction.

Quoted 1 line

These patches didn't make it to the U-Boot mailing list, although a couple of replies did. You probably want to resend them.

The patches were delayed because I was not registered on the mailing list. They now seem to have reached the list but as you said, replies are not linked to the thread.

What would be the best way to fix this? Should I resend the series?