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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Mon, 8 Jan 2018 10:39:19 +0100
Jonas Gorski <jonas.gorski@gmail.com> wrote:

> On 8 January 2018 at 10:14, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Mon, 8 Jan 2018 08:42:32 +0000
> > Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com> wrote:
> >  
> >> Thanks Jonas, Boris,
> >>
> >> Let me try to put my understanding and some proposal based on discussions.
> >>  
> >> > -----Original Message-----
> >> > From: Jonas Gorski [mailto:jonas.gorski@gmail.com]
> >> > Sent: Friday, January 05, 2018 7:28 PM
> >> > To: Boris Brezillon <boris.brezillon@free-electrons.com>
> >> > Cc: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>; Marek Vasut
> >> > <marex@denx.de>; Richard Weinberger <richard@nod.at>; Brian Norris
> >> > <computersforpeace@gmail.com>; linux-mtd@lists.infradead.org; Cyrille
> >> > Pitchen <cyrille.pitchen@wedev4u.fr>
> >> > Subject: Re: mtd layer: support of hybrid flash(W25M161AW) having both NOR
> >> > and NAND flash
> >> >
> >> > On 5 January 2018 at 14:44, Boris Brezillon
> >> > <boris.brezillon@free-electrons.com> wrote:  
> >> > > On Fri, 5 Jan 2018 14:38:48 +0100
> >> > > Jonas Gorski <jonas.gorski@gmail.com> wrote:
> >> > >  
> >> > >> On 5 January 2018 at 11:21, 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  
> >> > >> >         qflash0: w25q16fw @0 {
> >> > >> >                 #address-cells = <1>;
> >> > >> >                 #size-cells = <1>;
> >> > >> >                 spi-max-frequency = <20000000>;
> >> > >> >                 reg = <0>;
> >> > >> >                 type = "serial-nor"   <-- Proposed New binding
> >> > >> >                 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>;
> >> > >> >                 type = "serial-nand" <-- Proposed New binding
> >> > >> >                 is-hybrid <-- Proposed New binding
> >> > >> >                 die-num <-- Proposed New binding
> >> > >> >         }
> >> > >> > };  
> >> > >>
> >> > >> assuming the NOR and NAND parts behave like "normal" SPI-NOR /
> >> > >> SPI-NAND chips when selected, a more appropriate binding might be
> >> > >>
> >> > >> &qspi {
> >> > >>     ...
> >> > >>         qflash0: dual-flash@0 {
> >> > >>                 compatible = "winbond,w25q16fw";
> >> > >>                 reg = <0>;
> >> > >>                 spi-max-frequency = <20000000>;
> >> > >>                 #address-cells = <1>;
> >> > >>                 #size-cells = <0>;
> >> > >>
> >> > >>                 nor@0 {
> >> > >>                                 compatible = "jedec,spi-nor";
> >> > >>                                 reg = <0>;
> >> > >>                                 #address-cells = <1>;
> >> > >>                                 #size-cells = <1>;
> >> > >>
> >> > >>                                 partitions {
> >> > >>                                                 ...
> >> > >>                                 };
> >> > >>                 };
> >> > >>
> >> > >>                 nand@1 {
> >> > >>                                 compatible = "jedec,spi-nand"; /* or
> >> > >> whatever the correct nand-compatible would be */
> >> > >>                                 reg = <1>;
> >> > >>                                 #address-cells = <1>;
> >> > >>                                 #size-cells = <1>;
> >> > >>
> >> > >>                                 partitions {
> >> > >>                                                 ...
> >> > >>                                 };
> >> > >>                 };
> >> > >>
> >> > >>     };
> >> > >> };
> >> > >>
> >> > >> with the "windbond,w25q16fw" driver modeled as a simple
> >> > >> "spi-multiplexer" that registers its own virtual spi-bus. Then when
> >> > >> spi-nor or spi-nand tries to communicate with their appropriate die,
> >> > >> it sends the Software Die Select command if needed and then passes on
> >> > >> the message to its parent bus.
> >> > >>
> >> > >> That way there should be no changes needed for spi-nor / spi-nand
> >> > >> themselves. (The devil is probably in the details ;-)  
> >> > >
> >> > > Yep, I thought about this approach, and it's indeed quite elegant, but
> >> > > we're missing the lock I was mentioning in my previous reply. We need
> >> > > to prevent die selection not only for the time we're sending a single
> >> > > SPI message, but for the whole operation (which can be formed of
> >> > > several SPI messages). Or maybe I'm wrong, and operations can actually
> >> > > be interleaved, but I wouldn't bet on that ;-).  
> >> >
> >> > Ah, I missed that. I thought about it, and then tried to hand wave it
> >> > away with the "if they behave like normal chips" ;-)
> >> >
> >> > The mdio-bus supports nested locking, so you can do something like this:
> >> >
> >> > mutex_lock_nested(bus->mdio_lock, MDIO_BUS_NESTED);
> >> > bus->write();
> >> > bus->read();
> >> > mutex_unlock(bus->mdio_lock);
> >> >
> >> > without worrying someone else using the bus in between. [1] for an example
> >> > user.
> >> >
> >> > So going a similar approach with flagging the appropriate chips in
> >> > spi-nor/spi-nand as needing nested locking and then doing it for the
> >> > appropriate commands should solve that issue.
> >> >
> >> >  
> >>
> >> Device tree update:-
> >>
> >> &qspi {
> >>     ...
> >>         qflash0: dual-flash@0 {
> >>                 compatible = "winbond,w25q16fw", "hybrid"; <-- new compatibility value  
> >
> > "hybrid" is not needed, you know that the flash is hybrid with the
> > "winbond,w25q16fw" string.
> >  
> >>                 reg = <0>;
> >>                 spi-max-frequency = <20000000>;
> >>                 #address-cells = <1>;
> >>                 #size-cells = <0>;
> >>
> >>                 nor@0 {
> >>                                 compatible = "jedec,spi-nor";
> >>                                 reg = <0>;
> >>                                 #address-cells = <1>;
> >>                                 #size-cells = <1>;
> >>
> >>                                 partitions {
> >>                                                 ...
> >>                                 };
> >>                 };
> >>
> >>                 nand@1 {
> >>                                 compatible = "jedec,spi-nand"; /* or
> >> whatever the correct nand-compatible would be */
> >>                                 reg = <1>;
> >>                                 #address-cells = <1>;
> >>                                 #size-cells = <1>;
> >>
> >>                                 partitions {
> >>                                                 ...
> >>                                 };
> >>                 };  
> >
> > Not sure exposing the dies in the DT is such a good idea. You should
> > have a specific handling for "winbond,w25q16fw" which registers one
> > NAND and one NOR.  
> 
> How would you define (fixed) partitions in the dts then? This was one
> of my main motivations of having them there.

You're right. So let's keep the subnodes describing the NAND and NOR
dies.