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

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 +@pytest.mark.buildconfigspec('cmd_sf') +@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.

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