[U-Boot] [PATCH 00/18] sf: fix support of QSPI memories and controllers
Cyrille Pitchen
cyrille.pitchen at atmel.com
Mon Mar 21 14:13:57 CET 2016
Hi Jagan,
Le 21/03/2016 10:07, Jagan Teki a écrit :
> Hi Cyrille,
>
> On 18 March 2016 at 20:48, Cyrille Pitchen <cyrille.pitchen at atmel.com> wrote:
>> Hi Stefan,
>>
>> Le 18/03/2016 14:48, Stefan Roese a écrit :
>>> Hi All,
>>>
>>> please excuse the late reply to this thread. But I'm very interested
>>> in QSPI, as one of my customers uses Micron QSPI NOR and really wants
>>> to take full advantage of the device (quad access) to speed up the
>>> overall boot process. This is on SoCFPGA btw.
>>>
>>> Please allow me a few comments below.
>>>
>>> On 16.03.2016 15:14, Jagan Teki wrote:
>>>> 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.
>>>
>>> Thanks Cyrille, for this very detailed and good explanation. This is
>>> really appreciated. And I really hope that Jagan will get back to
>>> your generous offer, to assist with any such QSPI NOR related problems.
>>> I'll definitely remember this and might bug you at some time... ;)
>>>
>>>> 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!
>>>
>>> Jagan, what is the current plan here? I have to admit that I didn't
>>> check closely on your SPI-NOR work lately, sorry. So I can't really
>>> comment with a profound deep knowledge. But from what I read in
>>> Cyrille's patchset and explanation above, his patchset fixes some
>>> issues and brings us also more in line with the (upcoming) Linux
>>> framework. So I would really love to see this integrated into
>>> mainline quickly. Allowing a broader test basis on multiple platforms.
>>> Will it be a big task for Cyrille to rebase his patchset on top of
>>> your patches? Can you perhaps assist Cyrille with this? Or would it
>>> perhaps make sense to postpone your patchset and integrate Cyrille's
>>> first instead?
>>>
>>> Cyrille, if Jagans rework goes in first, do you already have a plan
>>> on when to rebase your patchset on top of this? Or do you see some
>>> "show-stoppers" here?
>>>
>>
>> Well if I need to rebase my patchset, for sure I will wait for Jagan's
>> rework to be completed and accepted first. I don't think it would be
>> efficient to rebase the QSPI series on the fly before the rework is
>> totally stabilized.
>>
>> One of the commit message from u-boot-spi.git can sum up the global rework:
>> "sf: Drop entire spi-flash framework" [1]
>>
>> Indeed all files from drivers/mtd/spi are deleted, the next commits replace
>> them by new files in the spi-nor folder. These new files are almost some
>> "copy/paste" of drivers/mtd/spi-nor files from Linux.
>
> Yeah.. the idea of using Linux spi-nor is like u-boot also facing
> similar issue where Linux facing like spi drivers became more flash
> centric, and it's not totally same design as Linux. We took the idea
> from Linux and have MTD driver model on top of it. The whole idea to
> add this is to follow the u-boot driver model principle and if the
> stack look some how similar to Linux then the development is as faster
> as, when
> compared to maintain separate thread of development - this is very
> legacy story I have been saying on this on ML for year back.
>
> And did you find any issues with this spi-nor series?
>
Well, the spi-nor series is based on Linux code, which already lacks support of
QSPI memories.
Moreover, the Linux version of m25p80.c uses the tx_nbits and rx_bits fields
of the struct spi_transfer to tell the SPI controller driver about the number
of I/O lines to be used to send or receive the command bytes. However there is
no equivalent mechanism with the u-boot version which relies on calls of
spi_write_then_read(). The u-boot version has no mean to support SPI protocols
other than SPI 1-1-1 and the SPI controller drivers should not try to guess the
SPI protocol from the op code.
Also be aware that the design chosen by the Linux m25p80 driver, that is to say
the use of spi_sync_transfer() or spi_sync(), is not suited to all hardware
QSPI controllers. For instance the Atmel QSPI controller expects separated
inputs for the command op code, the address, the dummy cycles and the data.
spi_write_then_read() calls spi_xfer() twice: the first time to send the op
code, address and dummy cycles then the second time to send the data.
How can a SPI controller driver make the difference between the two calls if
it should decode the input buffer to extract the op code, the address then the
dummy cycles on the first call but send raw data for the second call?
In addition, maybe it could be better to call spi_claim_bus() /
spi_release_bus() once for all at the beginning / end of spi_nor_read(),
spi_nor_erase() and spi_nor_write() instead of calling them for each register
read/write operation. I would avoid so many calls.
About the support the 16MiB memories, I see that commit 795c078c4563
("mtd: spi-nor: Add 4-byte addressing") copies the Linux version except for
the special case of Spansion memories which uses the dedicated 4byte address
op codes for Fast Read, Page Program and Sector Erase.
Please note that entering the 4byte address mode, like set_4byte() does,
changes the internal state of the SPI memory which has side effects: if a
CPU reset occurs and if early bootloaders don't support this address mode,
sending only 3 address bytes inside Fast Read commands, they fail reading
from the SPI memory. That's why I've sent a patch to implement an alternative
(thus optional) method to support >16MiB memories by converting the 3byte
address op codes to their associated 4byte version:
0x02 -> 0x12 Page Program
0x03 -> 0x13 Read
0x0b -> 0x0c Fast Read
0x6b -> 0x6c Fast Read Quad Output 1-1-4
0xd8 -> 0xdc Sector Erase
...
>>
>> OK, why not but it also implies that I can't simply rebase my patchset.
>> Since the all files are different it means I have to write a new series of
>> patches from scratch. As I said, I won't start such a work before the new
>> framework is totally accepted and integrated in next releases of u-boot.
>>
>> I don't say using the Linux code as a reference is a bad idea, indeed there are
>> good things in the spi-nor framework to be taken. For instance, in my own
>> series I added .read_reg() and .write_reg() hooks the same way as done in the
>> Linux source.
>> I just warn developers that currently the Linux code misses some stuff to
>> support QSPI memories efficiently. As an example, spi_nor_scan() fails to read
>> the JEDEC ID if the QSPI memory is already in QPI mode (Micron, Macronix,
>> Winbond, ...).
>> This issue didn't exist with regular SPI NOR flash memories as they are
>> almost "stateless" as compared to QSPI memories. SPI memories always use a
>> single protocol, SPI 1-1-1 with MISO and MOSI I/O lines, for all commands.
>> On the other hand, QSPI memories are much complex: you have to deal with
>> many different protocols and find a way to guess the current internal state
>> of the QSPI memory before sending commands to it.
>
> OK, I understand that you sent separate patches, are these patches fix
> this issue?
>
> https://patchwork.ozlabs.org/patch/598527/
> https://patchwork.ozlabs.org/patch/598528/
> https://patchwork.ozlabs.org/patch/598530/
Those 3 patches only deal with support of the sama5d2 xplained board.
They prepare the migration to the DM model and DT support since the Atmel QSPI
controller driver uses the DM model. One of them also configures the pin muxing
as needed to use the QSPI controller.
They are Work In Progress and not related with the mtd sub-system.
I only provide them to unblock some customers who are waiting for support of
QSPI memories in u-boot on sama5d2 SoCs.
> https://patchwork.ozlabs.org/patch/598532/
I guess this last one is not related with the mtd sub-system itself.
It fixes an issue I found using the "sf probe" command. However it might need
some review because I don't understand why the "device_unbind(new)" call was
there at first so maybe I miss something. I guess there was a good reason to
add this call and I want to avoid introducing regression.
>
> If so, then I will start reviewing these and will push these changes
> first and will rebase spi-nor on top of this.
>
> thanks!
>
Best regards,
Cyrille
More information about the U-Boot
mailing list