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

[Michal Simek]

On 17.5.2016 19:30, Stephen Warren wrote:
> 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.

Ah yeah. I have put there but remove it before sending this out.

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

ok. No problem to change it.


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

ok. Good idea. It can be added as separate patch on the top of this.


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

Ok Good idea - add protection. Is this just space from start?
Or does it make sense to design it in more generic way - start-end
regions which you can't rewrite?

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

It can be like that. The question is if make sense to add all of these
features to this particular patch or just create patches on the top of
this one.

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

ok.

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

ok. Just keep in your mind that I am not python expert.
I did use print for my debugging purpose. :-)

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

ok


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

ok.

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

ok will fix.


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


What do you mean by this?

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


I choose to use random sizes to have more variants because it can
discover problem.
I am not saying that there shouldn't be tests for fixed sizes but I
still think that some sort of randomization is good for testing because
it can discover errors which you didn't cover.


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

size 0 was causing problem. That's why it is starting from 4. Not sure
what's the standard sf behavior if you want to save 0 size.

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

That's why there is offset 10. It means it is not rewriting the same data.

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

What should be transfer mechanism to get data to the board to be able to
say it has this crc? For ethernet this is easy for qspi not sure.

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

This is good one. I should check this for sure that flash was erased.

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

Is this really needed because I do use small offset that data is not
saved to the same address space.

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

Agree. Read back and check should be there.
Is using mw addr val size, crc over it, erase, read back, crc, compare
good for you?

Thanks,
Michal