search

adafruit / Logic2-SPIFlash

Basic SPI Flash command decoder for Saleae Logic 2
MIT License
21 stars 14 forks source link

Analyzer crashing in QSPI mode with valueError #2

Closed desertsagebrush closed 2 years ago

desertsagebrush commented 3 years ago

Hello!

So I've been playing around with a capture that includes some QSPI traffic (from a Micron M5QU128), and am running into a error whenever I try to instantiate the SPI Flash analyzer over the Simple Parallel analyzer:

SPI Flash error - ValueError('byte must be in range(0, 256)')
  File "/home/<path>/.config/Logic/Marketplace/33/SPIFlashAnalyzer.py", line 184, in decode
    self._miso_data.extend(fake_frame.data["miso"])

I haven't seen any values in the output of the parallel analyzer that are greater than a single byte (and have tried both with and without the CS line included). I've tried both the version that is downloadable through Logic (as seen above) and from getting a local clone of the repository, with the same results.

I'm glad to help debug this/submit a pull request, since I haven't managed to create a minimal test file (and can't share this one, sorry.) If you have any suggestions for where to start poking at this, I would greatly appreciate them.

Thanks!

tannewt commented 3 years ago

Hi Sage, what are you parallel analyzer settings? I don't think I documented it well.

Printing fake_frame.data["miso"] and then tracing back where the bad value comes from would be my next step.

desertsagebrush commented 3 years ago

The parallel analyzer settings are:

    D0--D3: DQ0--DQ3 (MOSI, MISO, WP#, HOLD#)
    D4--D14: not set
    D15: S# (CS)
    Clock: CLK
    Edge: Rising edge
    (Not shown in table/terminal, but that doesn't change anything)

From poking it a little bit, it does end up with a (fake) frame with {'mosi': [15467], 'miso': [768]}, which is indeed above the size for a byte.

I'm reasonably sure that the reason for this happening is that the chip is already using Quad-SPI commands, so the analyzer doesn't see a known command to enter QSPI mode and thus doesn't properly deal with data on all four lines.

These values in particular are coming from lines 137 and 138 https://github.com/adafruit/Logic2-SPIFlash/blob/c8b3dbdb902be6f7c0613ee388c567a9bd5b5a26/SPIFlashAnalyzer.py#L137-L138 which properly mask off the initial data. When the fake frame is created on line 146, self._clock_count = 7, self._mosi_out = 0b11110001101011 / 0x3c6b, and self._miso_in = 0b1100000000 / 0x300. From doing a bit more poking, this is due to the previous transaction being only 54 clock cycles, which wouldn't make sense if it were just normal SPI but since it is using fast quad-output reads (command 0x6b), the timings are different (8 cycles for the command, 24 cycles for the address, 4 dummy cycles, and then 7 bytes @ 2 cycles each = 54).

So, this is actually two separate but related bugs:

The second issue is pretty simple to fix. My thoughts on the first issue would be to extend the set of CONTINUE_COMMANDS and data commands to handle more commands from more chips. Depending on how much use this extension ends up getting, it might be worth parameterize this by chip manufacturer but that is more of a stretch goal.

desertsagebrush commented 3 years ago

Now that I've written out that slight novel on figuring out the issue, I would be glad to help fix these and put in a pull request. The first one seems reasonably simple to implement by just increasing the known commands, and the second one could be handled by zero'ing self._mosi_out/self._miso_in when a new transaction occurs around here https://github.com/adafruit/Logic2-SPIFlash/blob/c8b3dbdb902be6f7c0613ee388c567a9bd5b5a26/SPIFlashAnalyzer.py#L117-L122

Do these changes seem reasonable to you? I'm glad to implement them and give you a pull request.

tannewt commented 3 years ago

Ya, they seem totally reasonable. I'm happy to review and merge changes in.

It feels like we may want a setting that says "I'm already in a QSPI mode" if your data doesn't have any commands in it at all (just continuations.)

colinoflynn commented 2 years ago

Here is a .sal for a file which gives this error and is fixed by #3. Also attached is datasheet.

Note there is an additional fix needed for continuous decoding to work (in particular - the comparison at https://github.com/Wmyers559/Logic2-SPIFlash/blob/fix_qspi_crash/SPIFlashAnalyzer.py#L174 needs to be against 0xa5, technically the datasheet just says high/low nibbles will be complements, may be safer to just check high nibble only for 0xa and ignore low nibble in case other manufactures do this different).

spi qspi demo.zip Infineon-S25FL128S_S25FL256S_128Mb(16_MB)_256Mb(32_MB)_3.0V_SPI_Flash_Memory-DataSheet-v18_00-EN.pdf

colinoflynn commented 2 years ago

Here is my changes for reference too - https://github.com/colinoflynn/Logic2-SPIFlash/tree/qspi_updates_continous (I've merged in the PR to my repo)

tannewt commented 2 years ago

Thanks for the comment @colinoflynn. I've merged #3 and I think this is fixed by it. (I haven't been using this decoder recently.)