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

Boris Brezillon
2018-01-08T09:48:23Z

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.