[U-Boot] [PATCH 0/3] add inital SF tests

[U-Boot] [PATCH 0/3] add inital SF tests

[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

 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

-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH 1/3] test/py: README: fix typo

[Liam Beguin]

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

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 test/py/README.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test/py/README.md b/test/py/README.md
index eefac377567a..000afce93c4a 100644
--- a/test/py/README.md
+++ b/test/py/README.md
@@ -150,7 +150,7 @@ processing.
   option takes a single argument which is used to filter test names. Simple
   logical operators are supported. For example:
   - `'ums'` runs only tests with "ums" in their name.
-  - ``ut_dm'` runs only tests with "ut_dm" in their name. Note that in this
+  - `'ut_dm'` runs only tests with "ut_dm" in their name. Note that in this
     case, "ut_dm" is a parameter to a test rather than the test name. The full
     test name is e.g. "test_ut[ut_dm_leak]".
   - `'not reset'` runs everything except tests with "reset" in their name.
-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH 1/3] test/py: README: fix typo

[Stephen Warren]

On 02/26/2018 09:17 PM, Liam Beguin wrote:
> fix a minor typo causing vim (and possibly other) to get confused with
> coloring.

Nit: s/fix/Fix/.

Reviewed-by: Stephen Warren <swarren@nvidia.com>

[U-Boot] [PATCH 1/3] test/py: README: fix typo

[Liam Beguin]

On 1 March 2018 at 16:23, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/26/2018 09:17 PM, Liam Beguin wrote:
>>
>> fix a minor typo causing vim (and possibly other) to get confused with
>> coloring.
>
>
> Nit: s/fix/Fix/.

Will fix

>
> Reviewed-by: Stephen Warren <swarren@nvidia.com>

[U-Boot] [PATCH 2/3] test/py: README: add HOSTNAME to PYTHONPATH

[Liam Beguin]

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.

Signed-off-by: Liam Beguin <liambeguin@gmail.com>
---
 test/py/README.md | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/test/py/README.md b/test/py/README.md
index 000afce93c4a..aed2fd063a81 100644
--- a/test/py/README.md
+++ b/test/py/README.md
@@ -320,7 +320,7 @@ If U-Boot has already been built:
 
 ```bash
 PATH=$HOME/ubtest/bin:$PATH \
-    PYTHONPATH=${HOME}/ubtest/py:${PYTHONPATH} \
+    PYTHONPATH=${HOME}/ubtest/py/${HOSTNAME}:${PYTHONPATH} \
     ./test/py/test.py --bd seaboard
 ```
 
@@ -331,7 +331,7 @@ follow:
 ```bash
 CROSS_COMPILE=arm-none-eabi- \
     PATH=$HOME/ubtest/bin:$PATH \
-    PYTHONPATH=${HOME}/ubtest/py:${PYTHONPATH} \
+    PYTHONPATH=${HOME}/ubtest/py/${HOSTNAME}:${PYTHONPATH} \
     ./test/py/test.py --bd seaboard --build
 ```
 
-- 
2.16.1.72.g5be1f00a9a70

[U-Boot] [PATCH 2/3] test/py: README: add HOSTNAME to PYTHONPATH

[Stephen Warren]

On 02/26/2018 09:17 PM, Liam Beguin wrote:
> 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.

Reviewed-by: Stephen Warren <swarren@nvidia.com>

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

[Liam Beguin]

Add basic tests for the spi_flash subsystem.

Signed-off-by: Liam Beguin <liambeguin@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
+
+    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")
+
+    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))
+
+    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']))
+    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)
+
+    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
-- 
2.16.1.72.g5be1f00a9a70

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

[Michal Simek]

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

> 
> Signed-off-by: Liam Beguin <liambeguin@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.


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



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

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

> +    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.
Also these sf read should be able to write bytes to non align addresses
to make sure that driver is handling that properly.


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

Anyway good to see this patch.

Thanks,
Michal

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

[Liam Beguin]

Hi Michal,

On 27 February 2018 at 03:51, Michal Simek <michal.simek@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@gmail.com>
>> ---
>>  test/py/tests/test_sf.py | 233 +++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 233 insertions(+)
>>
>> diff --git a/test/py/tests/test_sf.py b/test/py/tests/test_sf.py
>> new file mode 100644
>> index 000000000000..7017d8072ea9
>> --- /dev/null
>> +++ b/test/py/tests/test_sf.py
>> @@ -0,0 +1,233 @@
>> +# Copyright (c) 2017, Xiphos Systems Corp. All rights reserved.
>> +#
>> +# SPDX-License-Identifier: GPL-2.0
>> +
>> +import re
>> +import pytest
>> +import u_boot_utils
>> +
>> +
>> +"""
>> +Note: This test relies on boardenv_* containing configuration values to define
>> +which spi flash areas are available for testing.  Without this, this test will
>> +be automatically skipped.
>> +For example:
>> +
>> +# Boolean indicating whether the SF tests should be skipped.
>> +env__sf_skip = False
>> +
>> +# A list of sections of flash memory to be tested.
>> +env__sf_configs = (
>> +    {
>> +        # Optional, [[bus:]cs] argument used in `sf probe`
>> +        'id': "0",
>> +        # Where in SPI flash should the test operate.
>> +        'offset': 0x00000000,
>> +        # This value is optional.
>> +        #   If present, specifies the size to use for read/write operations.
>> +        #   If missing, the SPI Flash page size is used as a default (based on
>> +        #   the `sf probe` output).
>> +        'len': 0x10000,
>> +        # Specifies if the test can write to offset
>> +        'writeable': False,
>> +    },
>> +)
>> +"""
>> +
>> +
>> +def sf_prepare(u_boot_console, env__sf_config, verbose=False):
>> +    """Check global state of the SPI Flash before running any test.
>> +
>> +   Args:
>> +        u_boot_console: A U-Boot console connection.
>> +        env__sf_config: The single SPI flash device configuration on which to
>> +            run the tests.
>> +
>> +    Returns:
>> +        Nothing.
>> +    """
>> +
>> +    if u_boot_console.config.env.get('env__sf_skip', True):
>> +        pytest.skip('sf test disabled in environment')
>> +
>> +    # NOTE: sf read at address 0 fails because map_physmem 'converts' it
>> +    #       address 0 to a pointer.
>> +    ram_address = u_boot_utils.find_ram_base(u_boot_console) + 0x10
>
> Maybe you can add it to sf_configs too.

I'm not sure what it would add to the test but I could make the
configuration optional and default to find_ram_base().

>
>
>> +
>> +    probe_id = env__sf_config.get('id', '')
>> +    output = u_boot_console.run_command('sf probe ' + probe_id)
>> +    if 'SF: Detected' not in output:
>> +        pytest.fail('No flash device available')
>> +
>> +    m = re.search('page size (.+?) Bytes', output)
>> +    if m:
>> +        try:
>> +            page_size = int(m.group(1))
>> +        except ValueError:
>> +            pytest.fail('SPI Flash page size not recognized')
>> +
>> +    m = re.search('erase size (.+?) KiB', output)
>> +    if m:
>> +        try:
>> +            erase_size = int(m.group(1))
>> +        except ValueError:
>> +            pytest.fail('SPI Flash erase size not recognized')
>> +
>> +        erase_size *= 1024
>> +
>> +    m = re.search('total (.+?) MiB', output)
>> +    if m:
>> +        try:
>> +            total_size = int(m.group(1))
>> +        except ValueError:
>> +            pytest.fail('SPI Flash total size not recognized')
>> +
>> +        total_size *= 1024 * 1024
>> +
>> +    if verbose:
>> +        u_boot_console.log.info('Page size is: ' + str(page_size) + ' B')
>> +        u_boot_console.log.info('Erase size is: ' + str(erase_size) + ' B')
>> +        u_boot_console.log.info('Total size is: ' + str(total_size) + ' B')
>> +
>> +    env__sf_config['len'] = env__sf_config.get('len', erase_size)
>> +    if env__sf_config['offset'] % erase_size or \
>> +            env__sf_config['len'] % erase_size:
>> +        u_boot_console.log.warning("erase offset/length not multiple of "
>> +                                   "erase size")
>
> Is this only warning or you can destroy any data by that?

It's a warning because `sf erase` will fail if this condition is true.
It shouldn't destroy any data. I'm not really sure on how this should
be handled because if this warning appears, the erase and update
operations will fail.

>
>
>
>> +
>> +    env__sf_config['ram_address'] = ram_address
>> +
>> +
>> +def crc32(u_boot_console, address, count):
>> +    """Helper function used to compute the CRC32 value of a section of RAM.
>> +
>> +    Args:
>> +        u_boot_console: A U-Boot console connection.
>> +        address: Address where data starts.
>> +        count: Amount of data to use for calculation.
>> +
>> +    Returns:
>> +        CRC32 value
>> +    """
>> +
>> +    output = u_boot_console.run_command('crc32 %08x %x' % (address, count))
>> +
>> +    m = re.search('==> ([0-9a-fA-F]{8})$', output)
>> +    if not m:
>> +        pytest.fail('CRC32 failed')
>> +
>> +    return m.group(1)
>> +
>> +
>> +def sf_read(u_boot_console, env__sf_config, size=None):
>> +    """Helper function used to read and compute the CRC32 value of a section of
>> +    SPI Flash memory.
>> +
>> +    Args:
>> +        u_boot_console: A U-Boot console connection.
>> +        env__sf_config: The single SPI flash device configuration on which to
>> +            run the tests.
>> +        size: Optional, used to override env__sf_config value.
>> +
>> +    Returns:
>> +        CRC32 value of SPI Flash section
>> +    """
>> +
>> +    if size is None:
>> +        size = env__sf_config['len']
>> +
>> +    u_boot_console.run_command('mw %08x 0 %x' % (env__sf_config['ram_address'],
>> +                                                 size))
>
> Any reason for this write? It should be rewritten by sf read below anyway.

Yes, it is rewritten by `sf read`. I added it as an extra precaution
to make sure
that the data changes if we do two consecutive reads of the same address.

>
>> +
>> +    response = u_boot_console.run_command('sf read %08x %08x %x' %
>> +                                          (env__sf_config['ram_address'],
>> +                                           env__sf_config['offset'],
>> +                                           size))
>> +    assert 'Read: OK' in response, "Read operation failed"
>> +
>> +    return crc32(u_boot_console, env__sf_config['ram_address'],
>> +                 env__sf_config['len'])
>> +
>> +
>> +def sf_update(u_boot_console, env__sf_config):
>> +    """Helper function used to update a section of SPI Flash memory.
>> +
>> +   Args:
>> +        u_boot_console: A U-Boot console connection.
>> +        env__sf_config: The single SPI flash device configuration on which to
>> +           run the tests.
>> +
>> +    Returns:
>> +        CRC32 value of SPI Flash section
>> +    """
>> +    from time import time
>> +
>> +    u_boot_console.run_command('mw %08x %08x %x' %
>> +                               (env__sf_config['ram_address'], time(),
>> +                                env__sf_config['len']))
>
> ditto

same here.

>
>> +    crc_ram = crc32(u_boot_console, env__sf_config['ram_address'],
>> +                    env__sf_config['len'])
>> +    u_boot_console.run_command('sf update %08x %08x %x' %
>> +                               (env__sf_config['ram_address'],
>> +                                env__sf_config['offset'],
>> +                                env__sf_config['len']))
>> +
>> +    crc2 = sf_read(u_boot_console, env__sf_config)
>> +
>> +    return (crc2 == crc_ram)
>> +
>> +
>> + at pytest.mark.buildconfigspec("cmd_sf")
>> +def test_sf_read(u_boot_console, env__sf_config):
>> +    sf_prepare(u_boot_console, env__sf_config)
>> +
>> +    output = u_boot_console.run_command('sf read %08x %08x %x' %
>> +                                        (env__sf_config['ram_address'],
>> +                                         env__sf_config['offset'],
>> +                                         env__sf_config['len']))
>> +    assert 'Read: OK' in output, "Read operation failed"
>> +
>> +
>> + at pytest.mark.buildconfigspec("cmd_sf")
>> + at pytest.mark.buildconfigspec("cmd_crc32")
>> + at pytest.mark.buildconfigspec("cmd_memory")
>> +def test_sf_read_twice(u_boot_console, env__sf_config):
>> +    sf_prepare(u_boot_console, env__sf_config)
>> +
>> +    crc1 = sf_read(u_boot_console, env__sf_config)
>> +    crc2 = sf_read(u_boot_console, env__sf_config)
>
> The test is using the same memory. I tried to avoid that to make sure
> that there is no issue with memory itself. Small offset should be enough.

This is a reason why I added the extra `mw` above.
Do you think it's not enough?

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

s/write/read/ ?

I guess I could add an extra test to read unaligned addresses. would that
work? should the extra offset also be randomized?

>
>
>> +
>> +    assert crc1 == crc2, "CRC32 of two successive read operation do not match"
>> +
>> +
>> + at pytest.mark.buildconfigspec("cmd_sf")
>> + at pytest.mark.buildconfigspec("cmd_crc32")
>> + at pytest.mark.buildconfigspec("cmd_memory")
>> +def test_sf_erase(u_boot_console, env__sf_config):
>> +    if not env__sf_config['writeable']:
>> +        pytest.skip('flash config is tagged as not writeable')
>> +
>> +    sf_prepare(u_boot_console, env__sf_config)
>> +    output = u_boot_console.run_command('sf erase %08x %x' %
>> +                                        (env__sf_config['offset'],
>> +                                         env__sf_config['len']))
>> +    assert 'Erased: OK' in output, "Erase operation failed"
>> +
>> +    u_boot_console.run_command('mw %08x ffffffff %x' %
>> +                               (env__sf_config['ram_address'],
>> +                                env__sf_config['len']))
>> +    crc1 = crc32(u_boot_console, env__sf_config['ram_address'],
>> +                 env__sf_config['len'])
>> +
>> +    crc2 = sf_read(u_boot_console, env__sf_config)
>> +    assert crc1 == crc2, "CRC32 of erase section does not match expected value"
>> +
>> +
>> + at pytest.mark.buildconfigspec("cmd_sf")
>> + at pytest.mark.buildconfigspec("cmd_memory")
>> +def test_sf_update(u_boot_console, env__sf_config):
>> +    if not env__sf_config['writeable']:
>> +        pytest.skip('flash config is tagged as not writeable')
>> +
>> +    sf_prepare(u_boot_console, env__sf_config)
>> +    assert sf_update(u_boot_console, env__sf_config) is True
>>
>
> Also I wanted to design these tests that you will check bytes in front
> of and behind region in DDR to make sure that your driver is writing
> just amount of data you requested not aligned with words for example.

It's a good point! In a follow up series, I was planning on doing this on
the flash after write/erase operations to make sure we don't go
beyond `offset + size`.

>
> Anyway good to see this patch.
>
> Thanks,
> Michal
>
>

Thanks for the feedback,
Liam

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

[Michal Simek]

On 1.3.2018 05:01, Liam Beguin wrote:
> Hi Michal,
> 
> On 27 February 2018 at 03:51, Michal Simek <michal.simek@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@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.

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

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.

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

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

[Liam Beguin]

On 1 March 2018 at 01:59, Michal Simek <michal.simek@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@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@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

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

[Michal Simek]

On 1.3.2018 12:48, Liam Beguin wrote:
> On 1 March 2018 at 01:59, Michal Simek <michal.simek@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@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@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

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

[Stephen Warren]

On 03/01/2018 04:48 AM, Liam Beguin wrote:
> On 1 March 2018 at 01:59, Michal Simek <michal.simek@xilinx.com> wrote:
...
>> 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.

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

[Michal Simek]

On 1.3.2018 22:18, Stephen Warren wrote:
> On 03/01/2018 04:48 AM, Liam Beguin wrote:
>> On 1 March 2018 at 01:59, Michal Simek <michal.simek@xilinx.com> wrote:
> ...
>>> 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.

The thing is that I have real experience with random tests which were
able to find out issues in the driver compare to fixed tests.

But I have not a problem with two set of tests.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180305/43056d00/attachment.sig>

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

[Stephen Warren]

On 02/26/2018 09:17 PM, Liam Beguin wrote:
> Add basic tests for the spi_flash subsystem.

> diff --git a/test/py/tests/test_sf.py b/test/py/tests/test_sf.py

> +import re
> +import pytest
> +import u_boot_utils
> +
> +

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

> +"""
> +Note: This test relies on boardenv_* containing configuration values to define
> +which spi flash areas are available for testing.  Without this, this test will
> +be automatically skipped.
> +For example:
> +
> +# Boolean indicating whether the SF tests should be skipped.
> +env__sf_skip = False

Why do we need this variable? Let's just have the test rely upon whether 
env__sf_configs is present or not. If your test function takes 
env__sf_config as an argument, it'll automatically be skipped by the 
core test code if env__sf_configs isn't defined, or is defined by empty.

> +# A list of sections of flash memory to be tested.
> +env__sf_configs = (
> +    {
> +        # Optional, [[bus:]cs] argument used in `sf probe`
> +        'id': "0",

What is the default value if this isn't specified? I assume "0", but 
that would be good to document.

> +        # This value is optional.
> +        #   If present, specifies the size to use for read/write operations.
> +        #   If missing, the SPI Flash page size is used as a default (based on
> +        #   the `sf probe` output).
> +        'len': 0x10000,

Is len in u8s or u32s? I would assume bytes. len is used inconsistently 
below; as a parameter to "sf read" where it's interpreted as bytes, and 
as a parameter to "mw" where it's interpreted as words IIRC.

> +def sf_prepare(u_boot_console, env__sf_config, verbose=False):

> +    # NOTE: sf read at address 0 fails because map_physmem 'converts' it
> +    #       address 0 to a pointer.
> +    ram_address = u_boot_utils.find_ram_base(u_boot_console) + 0x10

Can we fix that bug instead? Other tests don't need that workaround, so 
I'm not sure why the SPI test does.

> +    probe_id = env__sf_config.get('id', '')
> +    output = u_boot_console.run_command('sf probe ' + probe_id)
> +    if 'SF: Detected' not in output:
> +        pytest.fail('No flash device available')

assert 'SF: Detected' in output ?

> +    m = re.search('page size (.+?) Bytes', output)
> +    if m:
> +        try:
> +            page_size = int(m.group(1))
> +        except ValueError:
> +            pytest.fail('SPI Flash page size not recognized')

What if not m? Currently the code leaves page_size uninitialized. 
Shouldn't the logic be:

m = re.search ...
assert m
try:
    ...

> +    if verbose:
> +        u_boot_console.log.info('Page size is: ' + str(page_size) + ' B')

Shorter might be:

u_boot_console.log.info('Page size is: %dB' % page_size)

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

Over-writing len changes the value in the original structure which could 
affect tests that want to handle missing/inconsistent data in other 
ways. I'd rather store the data using a different key that the user will 
never set, or even better store derived/calculated data in a different 
dictionary altogether.

> +    if env__sf_config['offset'] % erase_size or \
> +            env__sf_config['len'] % erase_size:
> +        u_boot_console.log.warning("erase offset/length not multiple of "
> +                                   "erase size")

I'd assert that here, rather than just warning.

> +    env__sf_config['ram_address'] = ram_address

ram_address isn't user-supplied configuration, and doesn't vary from 
test-to-test. Let's either return it, or write it to a global, or just 
call find_ram_base() whenever the value is required.

> +def crc32(u_boot_console, address, count):
> +    """Helper function used to compute the CRC32 value of a section of RAM.
> +
> +    Args:
> +        u_boot_console: A U-Boot console connection.
> +        address: Address where data starts.
> +        count: Amount of data to use for calculation.
> +
> +    Returns:
> +        CRC32 value
> +    """
> +
> +    output = u_boot_console.run_command('crc32 %08x %x' % (address, count))
> +
> +    m = re.search('==> ([0-9a-fA-F]{8})$', output)
> +    if not m:
> +        pytest.fail('CRC32 failed')
> +
> +    return m.group(1)

Should we put this into u_boot_utils.py? I was going to assert that 
other tests must already do this, but I guess other tests check for 
whether a known-ahead-of-time CRC value is on the crc32 command output, 
rather than parsing out the actual value. So, maybe this doesn't need to 
be a utility function.

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

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

> +    u_boot_console.run_command('mw %08x 0 %x' % (env__sf_config['ram_address'],
> +                                                 size))

mw.b instead? Same in sf_update().

This function could benefit from doing:

a) Fill target memory with known pattern.
b) sf read.
c) Check that target memory doesn't contain the known pattern.

> +def sf_update(u_boot_console, env__sf_config):

> +    from time import time

That's not needed. It should be at the top of the file if it was.

> +    u_boot_console.run_command('mw %08x %08x %x' %
> +                               (env__sf_config['ram_address'], time(),
> +                                env__sf_config['len']))
> +    crc_ram = crc32(u_boot_console, env__sf_config['ram_address'],
> +                    env__sf_config['len'])
> +    u_boot_console.run_command('sf update %08x %08x %x' %
> +                               (env__sf_config['ram_address'],
> +                                env__sf_config['offset'],
> +                                env__sf_config['len']))

Here the RAM should be cleared, although I guess that can happen inside 
sf_read().

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

> + at pytest.mark.buildconfigspec("cmd_sf")
> +def test_sf_read(u_boot_console, env__sf_config):
> +    sf_prepare(u_boot_console, env__sf_config)
> +
> +    output = u_boot_console.run_command('sf read %08x %08x %x' %
> +                                        (env__sf_config['ram_address'],
> +                                         env__sf_config['offset'],
> +                                         env__sf_config['len']))
> +    assert 'Read: OK' in output, "Read operation failed"

This doesn't verify that the command actually read anything, since it 
doesn't look for changes in the RAM. Let's "mw.b ram_address offset len" 
first and check that the CRC changes before/after the read to ensure 
that some bytes were actually transferred. If you take a look at the 
test_mmc_rd patch I sent recently, you can see how to do that. It'd be 
nice to structure the two tests as similarly as possible since they're 
doing roughly the same thing.

Also, it'd be good if env__sf_configs specified a known CRC for data 
that should be in the SPI flash, so we can verify not only that /some/ 
data was read, but that the /correct/ data was read.

Why doesn't this function use the sf_read helper function like 
test_sf_read_twice() does?

> + at pytest.mark.buildconfigspec("cmd_sf")
> + at pytest.mark.buildconfigspec("cmd_crc32")
> + at pytest.mark.buildconfigspec("cmd_memory")
> +def test_sf_read_twice(u_boot_console, env__sf_config):
> +    sf_prepare(u_boot_console, env__sf_config)
> +
> +    crc1 = sf_read(u_boot_console, env__sf_config)
> +    crc2 = sf_read(u_boot_console, env__sf_config)
> +
> +    assert crc1 == crc2, "CRC32 of two successive read operation do not match"

Similarly here, the test will pass if neither commands reads anything, 
or if only the first does, or if they both read the wrong thing.

> +def test_sf_erase(u_boot_console, env__sf_config):
...
> +    u_boot_console.run_command('mw %08x ffffffff %x' %
> +                               (env__sf_config['ram_address'],
> +                                env__sf_config['len']))
> +    crc1 = crc32(u_boot_console, env__sf_config['ram_address'],
> +                 env__sf_config['len'])

It'd be nice to name that crc_ffs or something like that ...

> +
> +    crc2 = sf_read(u_boot_console, env__sf_config)

... and name that crc_read or crc_readback, just to give a quick/easy 
idea of what each variable holds.

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

[Liam Beguin]

On 1 March 2018 at 16:56, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 02/26/2018 09:17 PM, Liam Beguin wrote:
>>
>> Add basic tests for the spi_flash subsystem.
>
>
>> diff --git a/test/py/tests/test_sf.py b/test/py/tests/test_sf.py
>
>
>> +import re
>> +import pytest
>> +import u_boot_utils
>> +
>> +
>
>
> Nit: Double blank line. The same issue exists in many other places too.

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.

>
>> +"""
>> +Note: This test relies on boardenv_* containing configuration values to
>> define
>> +which spi flash areas are available for testing.  Without this, this test
>> will
>> +be automatically skipped.
>> +For example:
>> +
>> +# Boolean indicating whether the SF tests should be skipped.
>> +env__sf_skip = False
>
>
> Why do we need this variable? Let's just have the test rely upon whether
> env__sf_configs is present or not. If your test function takes
> env__sf_config as an argument, it'll automatically be skipped by the core
> test code if env__sf_configs isn't defined, or is defined by empty.

Will do.

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

The default value is an empty string. I'll change it to 0 so I can test
different SPI frequencies.

>
>> +        # This value is optional.
>> +        #   If present, specifies the size to use for read/write
>> operations.
>> +        #   If missing, the SPI Flash page size is used as a default
>> (based on
>> +        #   the `sf probe` output).
>> +        'len': 0x10000,
>
>
> Is len in u8s or u32s? I would assume bytes. len is used inconsistently
> below; as a parameter to "sf read" where it's interpreted as bytes, and as a
> parameter to "mw" where it's interpreted as words IIRC.

Thanks for pointing this out! I'll update call to `mw` to use bytes instead of
words.

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

Good point. Let me see what I can do.

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

Yes, I'll also replace all other `pytest.fail` with asserts.

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

Will fix.

>> +    if verbose:
>> +        u_boot_console.log.info('Page size is: ' + str(page_size) + ' B')
>
>
> Shorter might be:
>
> u_boot_console.log.info('Page size is: %dB' % page_size)
>
>> +    env__sf_config['len'] = env__sf_config.get('len', erase_size)
>
>
> Over-writing len changes the value in the original structure which could
> affect tests that want to handle missing/inconsistent data in other ways.
> I'd rather store the data using a different key that the user will never
> set, or even better store derived/calculated data in a different dictionary
> altogether.

Sound like a better solution. I'll update `sf_prepare` to return a dictionary
of calculated data.

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

Ok.

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

I'll add it to the returned dictionary.

>
>> +def crc32(u_boot_console, address, count):
>> +    """Helper function used to compute the CRC32 value of a section of
>> RAM.
>> +
>> +    Args:
>> +        u_boot_console: A U-Boot console connection.
>> +        address: Address where data starts.
>> +        count: Amount of data to use for calculation.
>> +
>> +    Returns:
>> +        CRC32 value
>> +    """
>> +
>> +    output = u_boot_console.run_command('crc32 %08x %x' % (address,
>> count))
>> +
>> +    m = re.search('==> ([0-9a-fA-F]{8})$', output)
>> +    if not m:
>> +        pytest.fail('CRC32 failed')
>> +
>> +    return m.group(1)
>
>
> Should we put this into u_boot_utils.py? I was going to assert that other
> tests must already do this, but I guess other tests check for whether a
> known-ahead-of-time CRC value is on the crc32 command output, rather than
> parsing out the actual value. So, maybe this doesn't need to be a utility
> function.

As it is quite generic, I think it's a good idea to move it to u_boot_utils.py.

>
>> +def sf_read(u_boot_console, env__sf_config, size=None):
>
>
> Is size ever used? If not, let's delete it.

I forgot to remove it from a previous iteration. Will remove.

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

Will do.

>> +def sf_update(u_boot_console, env__sf_config):
>
>
>> +    from time import time
>
>
> That's not needed. It should be at the top of the file if it was.
>
>> +    u_boot_console.run_command('mw %08x %08x %x' %
>> +                               (env__sf_config['ram_address'], time(),
>> +                                env__sf_config['len']))
>> +    crc_ram = crc32(u_boot_console, env__sf_config['ram_address'],
>> +                    env__sf_config['len'])
>> +    u_boot_console.run_command('sf update %08x %08x %x' %
>> +                               (env__sf_config['ram_address'],
>> +                                env__sf_config['offset'],
>> +                                env__sf_config['len']))
>
>
> Here the RAM should be cleared, although I guess that can happen inside
> sf_read().
>
>> +    crc2 = sf_read(u_boot_console, env__sf_config)
>> +
>> +    return (crc2 == crc_ram)
>
>
>> + at pytest.mark.buildconfigspec("cmd_sf")
>> +def test_sf_read(u_boot_console, env__sf_config):
>> +    sf_prepare(u_boot_console, env__sf_config)
>> +
>> +    output = u_boot_console.run_command('sf read %08x %08x %x' %
>> +                                        (env__sf_config['ram_address'],
>> +                                         env__sf_config['offset'],
>> +                                         env__sf_config['len']))
>> +    assert 'Read: OK' in output, "Read operation failed"
>
>
> This doesn't verify that the command actually read anything, since it
> doesn't look for changes in the RAM. Let's "mw.b ram_address offset len"
> first and check that the CRC changes before/after the read to ensure that
> some bytes were actually transferred. If you take a look at the test_mmc_rd
> patch I sent recently, you can see how to do that. It'd be nice to structure
> the two tests as similarly as possible since they're doing roughly the same
> thing.

Ok, I'll have a look at that. If it's similar, I can use the updated
`sf_read()` you
propose above.

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

Ok, I'll add something like `expected_crc32` to env__sf_configs.

>
> Why doesn't this function use the sf_read helper function like
> test_sf_read_twice() does?
>
>> + at pytest.mark.buildconfigspec("cmd_sf")
>> + at pytest.mark.buildconfigspec("cmd_crc32")
>> + at pytest.mark.buildconfigspec("cmd_memory")
>> +def test_sf_read_twice(u_boot_console, env__sf_config):
>> +    sf_prepare(u_boot_console, env__sf_config)
>> +
>> +    crc1 = sf_read(u_boot_console, env__sf_config)
>> +    crc2 = sf_read(u_boot_console, env__sf_config)
>> +
>> +    assert crc1 == crc2, "CRC32 of two successive read operation do not
>> match"
>
>
> Similarly here, the test will pass if neither commands reads anything, or if
> only the first does, or if they both read the wrong thing.

It's true that this will pass if both commands don't read anything. I think what
you suggested for `sf_read` will fix this.

>
>> +def test_sf_erase(u_boot_console, env__sf_config):
>
> ...
>>
>> +    u_boot_console.run_command('mw %08x ffffffff %x' %
>> +                               (env__sf_config['ram_address'],
>> +                                env__sf_config['len']))
>> +    crc1 = crc32(u_boot_console, env__sf_config['ram_address'],
>> +                 env__sf_config['len'])
>
>
> It'd be nice to name that crc_ffs or something like that ...
>
>> +
>> +    crc2 = sf_read(u_boot_console, env__sf_config)
>
>
> ... and name that crc_read or crc_readback, just to give a quick/easy idea
> of what each variable holds.

Will do.

Thanks for your time,
Liam

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

[Stephen Warren]

On 03/03/2018 09:32 AM, Liam Beguin wrote:
> On 1 March 2018 at 16:56, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 02/26/2018 09:17 PM, Liam Beguin wrote:
>>>
>>> Add basic tests for the spi_flash subsystem.
>>
>>
>>> diff --git a/test/py/tests/test_sf.py b/test/py/tests/test_sf.py
>>
>>
>>> +import re
>>> +import pytest
>>> +import u_boot_utils
>>> +
>>> +
>>
>>
>> Nit: Double blank line. The same issue exists in many other places too.
> 
> 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.

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.

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

[Liam Beguin]

Hi Stephen,

On Tue, 13 Mar 2018 at 17:22 Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 03/03/2018 09:32 AM, Liam Beguin wrote:
> > On 1 March 2018 at 16:56, Stephen Warren <swarren@wwwdotorg.org> wrote:
> >> On 02/26/2018 09:17 PM, Liam Beguin wrote:
> >>>
> >>> Add basic tests for the spi_flash subsystem.
> >>
> >>
> >>> diff --git a/test/py/tests/test_sf.py b/test/py/tests/test_sf.py
> >>
> >>
> >>> +import re
> >>> +import pytest
> >>> +import u_boot_utils
> >>> +
> >>> +
> >>
> >>
> >> Nit: Double blank line. The same issue exists in many other places too.
> >
> > 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.
>
> 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.
>

As I said, I don't have any preference but you're right, consistency is
best.
I'll remove them in v3.


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


Thanks,
Liam Beguin

[U-Boot] [PATCH 0/3] add inital SF tests

[Michal Simek]

On 27.2.2018 05:17, Liam Beguin wrote:
> 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
> 

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?

Thanks,
Michal

[U-Boot] [PATCH 0/3] add inital SF tests

[Stephen Warren]

On 02/27/2018 04:52 AM, Michal Simek wrote:
> On 27.2.2018 05:17, Liam Beguin wrote:
>> 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
>>
> 
> 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?

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.

[U-Boot] [PATCH 0/3] add inital SF tests

[Michal Simek]

On 1.3.2018 22:22, Stephen Warren wrote:
> On 02/27/2018 04:52 AM, Michal Simek wrote:
>> On 27.2.2018 05:17, Liam Beguin wrote:
>>> 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
>>>
>>
>> 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?
> 
> 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.

It will be good to see implementation of that.

Thanks,
Michal





-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180305/9e6b09b6/attachment.sig>

[U-Boot] [PATCH 0/3] add inital SF tests

[Stephen Warren]

Liam Beguin wrote at Monday, February 26, 2018 9:18 PM:
> 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.

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

[U-Boot] [PATCH 0/3] add inital SF tests

[Liam Beguin]

Hi,

On Tue, 27 Feb 2018 at 11:29 Stephen Warren <swarren@nvidia.com> wrote:
>
> Liam Beguin wrote at Monday, February 26, 2018 9:18 PM:
> > 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.
>
> 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?

>
> --
> nvpublic
>

Thanks,
Liam Beguin

[U-Boot] [PATCH 0/3] add inital SF tests

[Tom Rini]

On Tue, Feb 27, 2018 at 01:52:38PM -0500, Liam Beguin wrote:
> Hi,
> 
> On Tue, 27 Feb 2018 at 11:29 Stephen Warren <swarren@nvidia.com> wrote:
> >
> > Liam Beguin wrote at Monday, February 26, 2018 9:18 PM:
> > > 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.
> >
> > 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?

The patches have been cleared and posted, no need, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20180228/3f426f8a/attachment.sig>