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

[Jagan Teki]

Hi Albert,

On Wednesday 16 March 2016 09:53 PM, Albert ARIBAUD wrote:
> Hello Jagan,
>
> On Wed, 16 Mar 2016 19:44:26 +0530, Jagan Teki <jteki at openedev.com>
> 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.
>>
>> 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!
>
> If I understand Cyrille's post correctly, it is not about better
> tuning, it is about fixing existing issues, right? I mean, Cyrille
> is talking about situations where the code will be not simply slow, but
> plain wrong, correct?
>
> If so, and since it appears Cyrills's patch series are bug fixes which
> the framework would not properly work if it did not integrate them
> anyway, I would agree with Marek that it makes more sense applying
> Cyrille's patches first.

OK, if these are bug fixes then what is the issue on working on top of 
spi-nor? and more over this series is on ML just now and need a proper 
review as well and will take some time too. I have planned spi-nor 
series for this release better work on this series - thanks!

-- 
Jagan