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

Thu Mar 17 08:30:13 CET 2016

Hello Jagan,

On Wed, 16 Mar 2016 22:04:23 +0530, Jagan Teki <jteki at openedev.com>
wrote:
> 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?

I believe you said spi-nor is not going to be submitted again until a
couple of weeks, and then there will be a review period; that may not
fit 2016.05. Cyrille's fixes could go out of the review phase before
spi-nor does, and possibly even be ready for 2016.05.

> and more over this series is on ML just now and need a proper 
> review as well and will take some time too.

Yes, but non-structural bugfixes are easier and quicker to test than
structural changes (which btw won't be entirely testable until the bug
fixes are applied IIUC).

> I have planned spi-nor 
> series for this release better work on this series - thanks!

But if spi-nor is released in 2016.05 without Cyrille's patches, there
will be known bugs unfixed, right?

> -- 
> Jagan

Amicalement,
-- 
Albert.