mbox series

[RFC,00/18] mtd: spi-nor: Proposal for 8-8-8 mode support

Message ID 20181012084825.23697-1-boris.brezillon@bootlin.com (mailing list archive)
Headers show
Series mtd: spi-nor: Proposal for 8-8-8 mode support | expand

Message

Boris Brezillon Oct. 12, 2018, 8:48 a.m. UTC
Hello,

The trend has been around octo (or octal) SPI NOR lately, and both
Yogesh and Vignesh proposed patches to support 1-1-8 and 1-8-8 modes
on Micron SPI NORs (plus the associated patches to support that on the
Cadence and FlesSPI controllers). I'll probably take their patches in
4.20 since adding support for stateless octo modes is not invasive, but
I wanted to start a discussion on how we should support stateful modes
(X-X-X and XD-XD-XD, where X is the bus width and D means Double
Transfer Rate).

First question that might come to mind is why should we support such
stateful modes? If you think about it, the gain of transmitting the
opcode on 8 IO lines is rather small compared to the pain it is to
teach the SPI NOR framework how to deal with that. The problem is,
some SPI NOR manufacturers (Macronix for instance) only implement 1-1-1
and 8-8-8 (or 8D-8D-8D), and they want to be able to use their NORs in
8-8-8 mode when the controller supports it.

So, this patchset is aiming at starting a wider discussion on how you
think those modes should be supported. This is just a proposal, and I'm
of course open to other suggestions, as long as the answer is not as
definitive as "too bad for them, they should have supported 1-8-8 or
1-1-8 modes".

Just a few more details about the patches in this series. Some of them
are clearly conflicting with patches posted by Yogesh and Vignesh.
Don't worry guys, I'm still planning to take yours before, it's just
that I was too lazy to rebase on top of your modifications. Also, some
patches might not pass checkpatch or might trigger kbuild errors, but
before fixing those problems, I'd like to get your opinion on the
general approach.

It's also worth mentioning that I focused on spi-mem support only,
because on the long run, I want all SPI NOR controller drivers
converted to this interface, and if I can limit support of octo modes
to those implementing the spi-mem interface, that's an incentive to
push developers to do the conversion. This part is still open to
discussion though.

Also, in this patchset, I merge the m25p80 driver code in spi-nor.c,
which is something I wanted to do for quite some. Indeed, the m25p80 is
just a simple SPI NOR controller driver (a wrapper around the SPI mem
API). Not only it shouldn't be named after a specific SPI NOR chip, but
it also doesn't deserve a specific driver IMO, especially if the end
goal is to get rid of SPI NOR controller drivers found in
drivers/mtd/spi-nor/ and replace them by SPI mem drivers (which would
be placed in drivers/spi/). With this solution, we declare the SPI NOR
driver as a spi_mem_driver, just like the SPI NAND layer is declared as
a spi_mem driver. This solution also allows us to check at a
fined-grain level (thanks to the spi_mem_supports_op() function) which
operations are supported and which ones are not, while the original
m25p80 logic was basing this decision on the
SPI_{RX,TX}_{DUAL,QUAD,OCTO} flags only.

The last set of patches in the series are modifying the framework to
allow entering X-X-X modes in a chip-specific way, and then adds
support for 8-8-8 and 8D-8D-8D modes on a Macronix chip (mx25uw51245g).

Yogesh, Vignesh, Tudor (and maybe Cyrille and Marek if you have some
time), please have a look at this patch series and tell me what you
think.

Regards,

Boris

Boris Brezillon (18):
  mtd: spi-nor: Add a flash_info entry for Macronix mx25uw51245g
  spi: Prepare things for octo mode support
  spi: spi-mem: Prepare things for DTR mode support
  spi: spi-mem: Prepare things for dual bytes opcodes support
  spi: spi-mem: mxic: Add support for DTR and Octo mode
  mtd: spi-nor: Move m25p80 code in spi-nor.c
  mtd: spi-nor: Rework hwcaps selection for the spi-mem case
  mtd: spi-nor: Define the DPI, QPI and OPI hwcaps
  mtd: spi-nor: Add spi_nor_{read,write}_reg() helpers
  mtd: spi-nor: Add support for X-X-X modes
  mtd: spi-nor: Prepare things for 2byte opcodes
  mtd: spi-nor: Provide a hook to tweak flash parameters
  mtd: spi-nor: Add 8-8-8 mode support to Macronix mx25uw51245g
  mtd: spi-nor: Clarify where DTR mode applies
  mtd: spi-nor: Add DTR support to the spi-mem logic
  mtd: spi-nor: Add the concept of full DTR modes
  mtd: spi-nor: Add 8D-8D-8D mode
  mtd: spi-nor: Make sure the 8D-8D-8D can be selected on mx25uw51245g

 drivers/mtd/devices/Kconfig         |   18 -
 drivers/mtd/devices/Makefile        |    1 -
 drivers/mtd/devices/m25p80.c        |  324 ---------
 drivers/mtd/spi-nor/Kconfig         |    3 +-
 drivers/mtd/spi-nor/atmel-quadspi.c |    5 +-
 drivers/mtd/spi-nor/spi-nor.c       | 1331 +++++++++++++++++++++++++++++++----
 drivers/spi/spi-mem.c               |   15 +-
 drivers/spi/spi-mxic.c              |   27 +-
 drivers/spi/spi.c                   |   12 +-
 include/linux/mtd/spi-nor.h         |  168 ++++-
 include/linux/spi/spi-mem.h         |   69 +-
 include/linux/spi/spi.h             |    2 +
 12 files changed, 1424 insertions(+), 551 deletions(-)
 delete mode 100644 drivers/mtd/devices/m25p80.c

Comments

Mark Brown Oct. 19, 2018, 12:25 p.m. UTC | #1
On Fri, Oct 12, 2018 at 10:48:07AM +0200, Boris Brezillon wrote:

> First question that might come to mind is why should we support such
> stateful modes? If you think about it, the gain of transmitting the
> opcode on 8 IO lines is rather small compared to the pain it is to
> teach the SPI NOR framework how to deal with that. The problem is,
> some SPI NOR manufacturers (Macronix for instance) only implement 1-1-1
> and 8-8-8 (or 8D-8D-8D), and they want to be able to use their NORs in
> 8-8-8 mode when the controller supports it.

The SPI framework changes definitely look OK to me, if everyone agrees
that this is a good way to go from a MTD point of view I'm happy to
apply them.  I have no strong opinion on the MTD bits of the series.
Boris Brezillon Oct. 19, 2018, 12:59 p.m. UTC | #2
Hi Mark,

On Fri, 19 Oct 2018 13:25:27 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Oct 12, 2018 at 10:48:07AM +0200, Boris Brezillon wrote:
> 
> > First question that might come to mind is why should we support such
> > stateful modes? If you think about it, the gain of transmitting the
> > opcode on 8 IO lines is rather small compared to the pain it is to
> > teach the SPI NOR framework how to deal with that. The problem is,
> > some SPI NOR manufacturers (Macronix for instance) only implement 1-1-1
> > and 8-8-8 (or 8D-8D-8D), and they want to be able to use their NORs in
> > 8-8-8 mode when the controller supports it.  
> 
> The SPI framework changes definitely look OK to me, if everyone agrees
> that this is a good way to go from a MTD point of view I'm happy to
> apply them.  I have no strong opinion on the MTD bits of the series.

Actually, Yogesh posted similar patches before me, so maybe you can
look at this series [1]. The spi/spi-mem side of things is rather
uncontroversial. Feel free to apply them if you think they're good
enough to go in.

Thanks for your review.

Boris

[1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=70822
Mark Brown Oct. 21, 2018, 1:36 p.m. UTC | #3
On Fri, Oct 19, 2018 at 02:59:20PM +0200, Boris Brezillon wrote:
> Mark Brown <broonie@kernel.org> wrote:

> > The SPI framework changes definitely look OK to me, if everyone agrees
> > that this is a good way to go from a MTD point of view I'm happy to
> > apply them.  I have no strong opinion on the MTD bits of the series.

> Actually, Yogesh posted similar patches before me, so maybe you can
> look at this series [1]. The spi/spi-mem side of things is rather
> uncontroversial. Feel free to apply them if you think they're good
> enough to go in.

Ugh, unfortunately he didn't send me them so I'll have to try to find
them on the list and it looks like there's been no review from any of
the other MTD people.  It'd be good to get some consensus, yours seems a
bit more complete so my inclination would be to go that way.
Boris Brezillon Oct. 22, 2018, 8:21 a.m. UTC | #4
On Sun, 21 Oct 2018 14:36:00 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Fri, Oct 19, 2018 at 02:59:20PM +0200, Boris Brezillon wrote:
> > Mark Brown <broonie@kernel.org> wrote:  
> 
> > > The SPI framework changes definitely look OK to me, if everyone agrees
> > > that this is a good way to go from a MTD point of view I'm happy to
> > > apply them.  I have no strong opinion on the MTD bits of the series.  
> 
> > Actually, Yogesh posted similar patches before me, so maybe you can
> > look at this series [1]. The spi/spi-mem side of things is rather
> > uncontroversial. Feel free to apply them if you think they're good
> > enough to go in.  
> 
> Ugh, unfortunately he didn't send me them so I'll have to try to find
> them on the list and it looks like there's been no review from any of
> the other MTD people.  It'd be good to get some consensus, yours seems a
> bit more complete so my inclination would be to go that way.

Indeed, looks like Yogesh version is not patching spi_setup() to
support octal mode. Anyway, it seems unfair to push for my own version
while I was clearly aware of Yogesh's patchset before posting my RFC
(the only reason I did not use his patches is laziness on my side: I had
my own working version, and the RFC was not really about these
spi/spi-mem aspects, more the spi-nor side of things :-)).

So, if you don't mind, I'll ask him to send a new version addressing
this problem (which should make our patches almost identical, except for
the naming: OCTAL vs OCTO), and I'll put my R-b.
Mark Brown Oct. 22, 2018, 12:01 p.m. UTC | #5
On Mon, Oct 22, 2018 at 10:21:26AM +0200, Boris Brezillon wrote:

> So, if you don't mind, I'll ask him to send a new version addressing
> this problem (which should make our patches almost identical, except for
> the naming: OCTAL vs OCTO), and I'll put my R-b.

That sounds great, I'll apply them on a branch and give you a pull
request after the merge window.