[U-Boot] [PATCH 00/18] sf: fix support of QSPI memories and controllers

[Cyrille Pitchen]

Le 16/03/2016 15:14, Jagan Teki a écrit :
> On Wednesday 16 March 2016 07:00 PM, Cyrille Pitchen wrote:
>> Le 15/03/2016 19:21, Jagan Teki a écrit :
>>> On Tuesday 15 March 2016 11:42 PM, Cyrille Pitchen wrote:
>>>> Hi all,
>>>>
>>>> This series of patches fixes and extend the support of QSPI memories
>>>> in the SPI flash framework. The updates are split into many parts to
>>>> make it easier to understand and review but they should be considered
>>>> as a whole.
>>>>
>>>> This was tested on a Atmel sama5d2 xplained board with a Micron n25q128a
>>>> memory.
>>>>
>>>> Best regards,
>>>>
>>>> Cyrille
>>>>
>>>> Cyrille Pitchen (18):
>>>>     Revert "sf: Fix quad bit set for micron devices"
>>>>     sf: call spi_claim_bus() and spi_release_bus() only once per read,
>>>>       write or erase
>>>>     sf: replace spi_flash_read_common() calls by spi_flash_cmd_read()
>>>>     sf: remove spi_flash_write_common()
>>>>     sf: export spi_flash_wait_ready() function
>>>>     sf: share erase generic algorithm
>>>>     sf: share write generic algorithm
>>>>     sf: share read generic algorithm
>>>>     sf: add hooks to handle register read and write operations
>>>>     sf: move support of SST flash into generic spi_flash_write_alg()
>>>>     sf: fix selection of supported READ commands for QSPI memories
>>>>     sf: fix detection of QSPI memories when they boot in Quad or Dual mode
>>>>     sf: add helper function to set the number of dummy bytes
>>>>     sf: add 4byte address opcodes
>>>>     sf: prepare next fixes to support of QSPI memories by manufacturer
>>>>     sf: fix support of Micron memories
>>>>     ARM: at91: clock: add function to get QSPI clocks
>>>>     sf: add driver for Atmel QSPI controller
>>>
>>> Appreciate for the work, we're working on spi-nor framework[1] planning to push in couple of weeks. Will let you know once it merged so that you can add your changes on top of that.
>>>
>>> [1] http://git.denx.de/?p=u-boot/u-boot-spi.git;a=shortlog;h=refs/heads/spi-nor-next
>>>
>>
>> Hi Jagan,
>>
>> I've started to have a look on your branch. I hope it's not to late for few
>> comments:
>>
>> Globally I see the new code attend to match the spi-nor framework from Linux.
>> OK that's fine but please note the current spi-nor framework in Linux has
>> incomplete and sometime not working support of QSPI memories.
>>
>> First, after a discussion with Brian and Bean on linux-mtd [1], Bean's commit
>> to add support to Micron QSPI memories was reverted since it didn't work alone.
>> In his reply, Brian agreed the code was not tested and should not have been
>> merged.
>>
>> This highlights a more general issue: currently, there is no mean for the
>> spi-nor framework to notify the SPI controller driver about a SPI protocol
>> change at the QSPI memory side. This applies to Micron memories when they enter
>> their Quad I/O mode. If so, ALL commands, even JEDEC Read ID, Read Status
>> Register, ..., MUST use the SPI 4-4-4 protocol. Commands sent using SPI 1-x-y
>> protocols are no longer decoded properly.
>> This also applies to Macronix and Winbond memories if they enter their QPI
>> mode, which is the equivalent of Micron Quad I/O mode.
>> This is why I've suggested to add 4 new fields in the struct spi_nor:
>> - .reg_proto: the SPI protocol to be used by .read_reg() and .write_reg()
>>    hooks.
>> - .read_proto: the SPI protocol to be used by the .read() hooks, maybe by the
>>    .read_mmap() also.
>> - .write_proto: the SPI protocol to be used by the .write() hooks
>> - .erase_proto: the SPI protocol to be used by the .erase() hooks.
>>
>> (Q)SPI controller drivers cannot guess the protocol to be used from the command
>> op code. Indeed, taking the Micron case as un example, the very same 0xeb op
>> code may be used with the SPI 1-4-4 protocol (Micron Extended SPI mode) or
>> with the SPI 4-4-4 protocol (Micron Quad I/O mode).
>>
>>
>> Also just some words about the naming of SPI x-y-z protocols:
>> - x refers to the number of I/O lines used to send the op code byte
>> - y is the number of I/O lines used to send the address, mode/dummy cycles
>>    (if these cycles exist for the command op code)
>> - z is the number of I/O lines used to send/receive data (if needed)
>>
>> So the SNOR_OP_READ_1_1_2_IO macro for the Fast Read Dual I/O command (as
>> opposed to the macro SNOR_OP_READ_1_1_2 macro for the Fast Read Dual Output
>> command) doesn't make sense: it should be named SNOR_OP_READ_1_2_2.
>>
>>
>> Then about the value used for the dummy cycles, it's not always true that we
>> don't care about initializing them. Depending on the configuration of the
>> memory, some special dummy cycles, sometime called mode cycles, are used to
>> during Fast Read operations to make the memory enter/leaver its Continuous Read
>> mode. Once is Continuous Read mode, the op code byte is no longer sent, it is
>> implicit and the command actually starts from the address cycles. This mode
>> is mostly used for XIP applications hence is not relevant for mtd usage.
>> However we should take care not to enter this Continuous Mode by mistake.
>> Depending on the memory manufacturer, the Continuous Mode can be disabled by
>> updating the relevant bit in some configuration register (e.g. setting the XIP
>> bit in Micron Volatile Configuration Register) or by choosing the right op code
>> (e.g. for Winbond memories in QPI mode, both the 0x0b and 0xeb op codes can
>> be used for Fast Read 4-4-4 operation but only the 0xeb op code cares about
>> the dummy cycle value to enter/leave the Continuous Read mode).
>> Some Spansion memories use 6 dummy cycles for Fast Read 1-4-4 command as
>> factory default, not 8.
>>
>> Besides when sending a regular JEDEC Read ID (0x9f) command to probe the (Q)SPI
>> memory, the current spi-nor framework assumes the Quad I/O or QPI mode is not
>> already enabled. This not always true, some early bootloarders, such as the
>> sama5d2 ROM Code, enables the Micron Quad I/O mode when configured to boot from
>> the QSPI memory. If so, the QSPI memory no longer replies to the 0x9f command
>> in SPI 1-1-1 protocol but instead to the 0xaf command in SPI 4-4-4 protocol.
>>
>> Finally, about the proper way to describe the SPI controller capabilities,
>> the SPI_TX_{DUAL, QUAD} and SPI_RX_{DUAL, QUAD} mode flags are set in the
>> SPI framework based on the "spi-rx-bus-width" and "spi-tx-bus-width" DT
>> properties already used in Linux.
>> This is not enough to make the difference between the SPI 1-4-4 and SPI 4-4-4
>> protocols. Maybe some SPI controllers support the first protocol but not the
>> latest. It could be good to add another argument to spi_nor_scan() providing
>> an exhaustive list of SPI protocols supported by the SPI controller.
>> Then to match this list with the list of SPI protocols supported by the SPI
>> memory and select the proper protocol, this new argument should use the same
>> range of values as the .flash_read field in the struct spi_nor_info used to
>> describe the SPI memories.
>>
>> For backward compatibility, the m25p80 driver could then convert the SPI modes
>> into spi-nor read modes. Please have a look at patch 11 of my series; the
>> chunk related to spi_flash_probe_slave() in sf_probe.c:
>>
>>     /* Convert SPI mode_rx and mode to SPI flash read commands */
>> +    mode_rx = spi->mode_rx;
>> +    if (mode_rx & SPI_RX_QUAD) {
>> +        e_rd_cmd = RD_NORM | QUAD_OUTPUT_FAST;
>> +        if (spi->mode & SPI_TX_QUAD)
>> +            e_rd_cmd |= QUAD_IO_FAST;
>> +    } else if (mode_rx & SPI_RX_DUAL) {
>> +        e_rd_cmd = RD_NORM | DUAL_OUTPUT_FAST;
>> +        if (spi->mode & SPI_TX_DUAL)
>> +            e_rd_cmd |= DUAL_IO_FAST;
>> +    } else if ((mode_rx & (SPI_RX_SLOW | SPI_RX_FAST)) == SPI_RX_SLOW) {
>> +        e_rd_cmd = ARRAY_SLOW;
>> +    } else {
>> +        e_rd_cmd = RD_NORM;
>> +    }
>> +
>> [...]
>> -    ret = spi_flash_scan(flash);
>> +    ret = spi_flash_scan(flash, e_rd_cmd);
>>
>>
>> I've spent a lot of time working on the QSPI memory topic so I can tell you
>> that there are many other traps to avoid!
>> If I can help you on this topic during your rework of the SPI NOR support,
>> please let me know.
> 
> I understand your points, thanks for that and anyway this spi-nor work is a starting point for both syncing with Linux as well with new feature or for better tunning. And more over I started this work in 2014 end and it's been reviewing over and over and we finally landed up with MTD driver model.
> 
> So, please wait for sometime until this gets merged we definitely work together for better tunning, thanks!
> 

OK, no problem!

I will just send the additional patches I used to test with the sama5d2 xplained board.
They are not related with the spi_flash/spi-nor framework itself but they are part of a WIP update of the sama5d2_xplained board support.
If some people need those patches to unblock their own developments using QSPI memories on sama5d2 SoCs, all the material will be available :)

Best regards,

Cyrille