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

[Albert ARIBAUD]

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.

> -- 
> Jagan

Amicalement,
-- 
Albert.