Re: mtd layer: support of hybrid flash(W25M161AW) having both NOR and NAND flash

Boris Brezillon
2018-01-05T13:35:21Z

+Peter and Frieder who are working on the SPI NAND aspects.

On Fri, 5 Jan 2018 10:21:48 +0000
Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com> wrote:

> Thanks Boris for the encouragement.
>
> > -----Original Message-----
> > From: Boris Brezillon [mailto:boris.brezillon@free-electrons.com]
> > Sent: Thursday, January 04, 2018 11:17 PM
> > To: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
> > Cc: linux-mtd@lists.infradead.org; Cyrille Pitchen <cyrille.pitchen@wedev4u.fr>;
> > Richard Weinberger <richard@nod.at>; Marek Vasut <marex@denx.de>; Brian
> > Norris <computersforpeace@gmail.com>
> > Subject: Re: mtd layer: support of hybrid flash(W25M161AW) having both NOR
> > and NAND flash
> >
> > +MTD maintainers.
> >
> > On Thu, 4 Jan 2018 14:08:42 +0000
> > Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com> wrote:
> >
> > > Hi All,
> > >
> > > Winbond has come up with special flash i.e. W25M161AW. It consist of Serial
> > NOR(Die #0) and Serial NAND(Die #1) flash.
> > > Means both NOR, NAND flashes are placed in W25M161AW controlled by
> > single chip-select.
> > >
> > > "Software Die Select (C2h)" command is being used to switch die or flash.
> >
> > Why are they so mean to us?! :-)
> >
> > >
> > > It looks to be quite unique chip and wondering if any kind framework or work in
> > progress available to handle it.
> > > I know that SPI-NAND framework discussions is still in progress.
> >
> > Well, nothing impossible to handle, we just need to declare 2 MTD
> > devices (one NAND and one NOR). This being said, it looks like we'll
> > need this spi-flash abstraction we have been talking about with Marek
> > and Cyrille to properly support these use cases: flash devices will be
> > exposed through different sub-layers (spi-nor or spi-nand), but we need
> > a common way to detect those spi-flash chips. I looked at a few SPI
> > NAND and SPI NOR chips, and from what I've seen so far they were quite
> > different (the opcodes and CMD+ADDR+DATA sequences were quite
> > different) so I thought we were safe to start with a completely
> > unconnected SPI NAND framework and merge some bits in a spi-flash layer
> > afterwards, but this chip proves me wrong :-/.
>
> I am thinking of following changes with fsl_qspi.c as controller
>
> &qspi {
> num-cs = <2>;
> bus-num = <0>;
> status = "okay";
> compatible = " fsl,ls1021a-qspi ", "fsl,ls1021a-qspi-nand"; <-- updated compatibility for drivers

Nope, "fsl,ls1021a-qspi" should be enough. The fact that this
controller is able to control SPI NAND chips is just an internal detail
(actually, I hope all QSPI controllers are able to support both NOR and
NAND devices, but I wouldn't be surprised if some vendors hardcode the
set of commands they support in their controller :-/)

> qflash0: w25q16fw @0 {
> #address-cells = <1>;
> #size-cells = <1>;
> spi-max-frequency = <20000000>;
> reg = <0>;
> type = "serial-nor" <-- Proposed New binding

No, this should be placed in a compatible:

compatible = "jedec,spi-nor";

> is-hybrid <-- Proposed New binding
> die-num <-- Proposed New binding
> };
>
> qflash1: w25n01gw@1 {
> #address-cells = <1>;
> #size-cells = <1>;
> spi-max-frequency = <20000000>;
> reg = <1>;

This is wrong, the NAND and NOR dies use the same CS line.

> type = "serial-nand" <-- Proposed New binding
> is-hybrid <-- Proposed New binding
> die-num <-- Proposed New binding
> }
> };
> };

Not really the DT representation I had in mind. Let's try to keep
things as simple as possible. How about:

&qspi {
/* compatible is unchanged and has been defined in a dtsi */
/* same goes for #size-cells, #address-cells, ... */
status = "okay";

qflash0: w25m161aw@0 {
/*
* Thanks to this compatible, the spi-flash logic will
* know that this chip is an hybrid flash exposing a
* NOR and a NAND, that the NOR is assigned id 0 an
* the NAND id 1.
*/
compatible = "winbond,w25m161aw";
reg = <0>;
};
};

>
> struct spi_nor {
> --
> --
> u8 is_hybrid
> u8 select_die_opcode
> u8 die_num;
> --
> --
> }
>
> struct spinand_device {
> --
> --
> u8 is_hybrid
> u8 die_num;
> }
>
> struct spinand_manufacturer_ops { <-- this will be populated in flash file like drivers/mtd/nand/spi/winbond.c
> int (*die_select)(struct spinand_device *spinand);
> }

Frieder is working on such a ->select_die() interface, but that's not
sufficient for this kind of hybrid chips. We need a lock to prevent
the spi-nor and spi-nand layer from stepping on each other's toes:
when an operation is done on the NAND die, the NOR layer should not
try to select the NOR die.

>
>
> fsl_qspi.c <-- existing file to handle NOR
> fsl_qspinandc <-- proposed file to handler NAND

Hm, not sure this is a good idea. Let's keep one driver and add support
for NAND in there.

>
> fsl_qspi.c
> probe()
> for_each_available_child_of_node(dev->of_node, np)
> - if type == "serial-nor"

Should be based on the compatible.

> spi_nor_scan()
> - if (is_hybrid)
> Set is_hybrid , select_die_opcode and die-num field of struct spi_nor
> fsl_qspinand.c
> probe()
> for_each_available_child_of_node(dev->of_node, np)
> - if type == "serial-nand"
> spinand_init()
> - if (is_hybrid)
> Set is_hybrid and die-num field of struct spinand_device

Hm, I really hate this situation. What we should be doing is declare a
qspi controller that embeds a SPI NOR and a SPI NAND controller and
then let the core register each chip, deciding which one is a NAND (and
should be attached to the NAND logic) and which one is a NOR (and
should be attached to the NOR logic).

>
> Whenever read/write/erase come from MTD layer
> - if is_hybrid
> Change the die using select_die_opcode or die_select function pointer

As said above, that's not enough: you need a lock to prevent concurrent
accesses from the NOR and NAND logic.

Regards,

Boris