[U-Boot] [PATCH] test/py: Add basic QSPI testing

[Stephen Warren]

On 05/17/2016 08:05 AM, Michal Simek wrote:
> This is the first attempt how to test qspi.
> Detect SPI size. Read it all, random size, erase every block,
> write random data with random size and read it back,
> erase the whole qspi and at the end load kernel image via tftp
> and save it to memory.

I don't see anything that downloads a kernel image via tftp. That's good 
because I'd rather not tie SPI testing to working networking; we should 
keep tests independent as much as possible.

I think this (test filename, all symbols/strings in the patch, etc.) 
should be named "sf" not "qspi" since (a) it tests serial flash 
specifically, not SPI in general (other device types besides flash might 
exist on SPI), and (b) the test looks like it will work for any SPI 
flash, not just QSPI; IIUC the U-Boot commands work identically for 
single-, dual- and quad-wide buses.

I would rather this test were explicitly parametrized via the boardenv:

* The boardenv should specify whether sf testing is expected to occur. 
The current test attempts to detect sf, and if U-Boot can't, it just 
skips the test. I'd rather the boardenv say whether sf detection should 
work, and if it fails when boardenv says it should work, the test should 
fail rather than be skipped.

* Some devices might store useful data in their SPI flash (e.g. some 
Tegra devices boot from SPI) and we should not destroy that data in this 
test. As such, the boardenv should indicate whether the test should 
erase anything at all, and specify what part(s) of the flash can be 
erased if so (e.g. offset/size of a writable region, or a flash to 
indicate whether the entire flash erase can be tested).

* A board might have multiple sf chips attached. The boardenv could list 
which to test, whereas this test currently only tests the default 
device, I think.

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

> +# Find out qspi memory parameters
> +def qspi_pre_commands(u_boot_console):
> +    output = u_boot_console.run_command('sf probe')
> +    if not "SF: Detected" in output:
> +        pytest.skip('No QSPI device available')

I think that should be an error.

> +    m = re.search('page size (.+?) Bytes', output)
> +    if m:
> +        try:
> +            global page_size
> +            page_size = int(m.group(1))
> +        except ValueError:
> +            pytest.fail("QSPI page size not recognized")
> +
> +        print 'Page size is: ' + str(page_size) + " B"

page_size appears to be used unconditionally elsewhere. I think that 
should be written as:

m = re.search(...)
if not m:
     raise Exception(...)
try:
    global page_size
...

I wonder if it makes sense to apply further validation to these values. 
i.e. at least check the sizes aren't zero or at least some minimal size 
and alignment, so that later test code is guaranteed not to do nothing 
or fail in unexpected ways. Perhaps the boardenv should list the values 
that the HW is expected to show, so the test can validate that the probe 
command generates the expected output? I'm not totally sure on that last 
point, but maybe.

Rather than using print, perhaps use u_boot_console.log.info() so that 
the text shows up separately from any stream content in the log file?

> +    global qspi_detected
> +    qspi_detected = True

I think you can drop that. Rather, qspi_pre_commands should raise an 
exception (either error or skip) in any case where the test should not 
continue ...

> +# Read the whole QSPI flash twice, random_size till full flash size, random till page size
> + at pytest.mark.buildconfigspec('cmd_sf')
> + at pytest.mark.buildconfigspec('cmd_memory')
> +def test_qspi_read_twice(u_boot_console):
> +    qspi_pre_commands(u_boot_console)
> +
> +    if not qspi_detected:
> +        pytest.skip('QSPI not detected')

... which would allow that if condition to be removed from all test 
functions.

> +    expected_read = "Read: OK"

I'm not totally sure it's worth putting that into a string; while in 
this case it slightly reduces duplication (although in other cases in 
this file where the same style is used, there's no duplication being 
avoided), it makes it harder to see what the asserts are actually 
checking later in the code.

Simon Glass has requested that all strings use ' not ", except perhaps 
where there's an embedded ' in the string. This comment applies 
elsewhere in the file too.

> +    # TODO maybe add alignment and different start for pages
> +    for size in random.randint(4, page_size), random.randint(4, total_size), total_size:

I'm not sure it's a good idea for tests to use random numbers and hence 
operate differently each time. This can lead to intermittent failures 
that are hard to track down. Rather, if there are known corner-cases, 
the test should always test them all. Take a look at the DFU test (and 
its example boardenv content) for how that's handled there.

> +        # FIXME using 0 is failing for me
> +        output = u_boot_console.run_command('sf read %x 0 %x' % (addr + total_size, size))

"using 0" for what fails? That's confusing since the second parameter is 
"0" so I wonder if this test fails or passes for you? Perhaps that 
comment applies to the address parameter? If so, explicitly mentioning 
the parameter name would be useful to remove the ambiguity. Are you 
investigating the problem?

> +        assert expected_read in output
> +        output = u_boot_console.run_command('crc32 %x %x' % (addr + total_size, size))
> +        m = re.search('==> (.+?)', output)
> +        if not m:
> +            pytest.fail("CRC32 failed")
> +        expected_crc32 = m.group(1)
> +        output = u_boot_console.run_command('sf read %x 0 %x' % (addr + total_size + 10, size))
> +        assert expected_read in output
> +        output = u_boot_console.run_command('crc32 %x %x' % (addr + total_size + 10, size))
> +        assert expected_crc32 in output

That just proves that the SPI read wrote the same data into memory both 
times, or failed to write anything both times thus leaving the same 
pre-existing content in memory each time. It would be better to at least 
clear the memory before each sf read (via mw I imagine) to ensure that 
sf read actually wrote something to memory, and even better to compare 
against some known crc32 rather than just assuming that anything read 
from SPI is correct. The latter could be achieved by either:

a) In boardenv, specify the offset/length/crc32 of a region to be 
verified by read. This would make sense if at least part of the flash 
content is invariant across test runs.

b) Do a write-then-read test of known data. Something like:

1) erase the region
2) read the region, validate crc mismatches (i.e. the erase worked)
3) write the region
4) read the region, validate crc matches written data

Make sure that in steps 2 and 4, the memory buffer used is cleared 
before the sf read to guarantee that sf read actually read the data, 
rather than simply leaving existing RAM content in place.

> + at pytest.mark.buildconfigspec('cmd_sf')
> +def test_qspi_erase_all(u_boot_console):
> +    qspi_pre_commands(u_boot_console)
> +
> +    if not qspi_detected:
> +        pytest.skip('QSPI not detected')
> +
> +    expected_erase = "Erased: OK"
> +    start = 0
> +    output = u_boot_console.run_command('sf erase 0 ' + str(hex(total_size)))
> +    assert expected_erase in output

That should probably read the region and validate that the erase 
actually happened?