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

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

[Prabhakar Kushwaha]

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. 
 
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.   

Regards,
Prabhakar

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

[Boris Brezillon]

+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 :-/.

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

[Prabhakar Kushwaha]

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
	}
};
};

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);
}  


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

fsl_qspi.c
	 probe() 
  		 for_each_available_child_of_node(dev->of_node, np)
       			- if type == "serial-nor"
 				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

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

--pk

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

[Boris Brezillon]

+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

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

[Jonas Gorski]

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 ;-)


Regards
Jonas

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

[Boris Brezillon]

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 ;-).

> 
> 
> Regards
> Jonas

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

[Jonas Gorski]

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.


Regards
Jonas

[1] http://elixir.free-electrons.com/linux/v4.15-rc6/source/drivers/net/dsa/qca8k.c#L148

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

[Prabhakar Kushwaha]

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
                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 {
                                                ...
                                };
                };

    };
};


There will be only one file i.e. fsl_qspi.c handing NOR and NAND. QSPI controller will have SPI NOR and a SPI NAND controller embedded.
Question: What should be the location of this file?  driver/mtd/spi-nor definitely is not right place??


Change in Data structures
----------------------------------

struct mtd_info {
	----
	struct mutex *hybrid_flock;
 	---
}

struct fsl_qspi {
	---
	struct spi_nor nor[FSL_QSPI_MAX_CHIP];
	struct spinand_device spinand[FSL_QSPI_MAX_CHIP];
	---
}

Function implementation
----------------------------------

fsl_qspi.c
	 probe() {
		--
		--
  		 for_each_available_child_of_node(dev->of_node, np) {
			if compatible "hybrid" {
				hybrid_flock = malloc(sizeof(struct mutex));
       				- if compatible "jedec,spi-nor" 
					spi_nor_scan()
       				- if compatible "jedec,spi-nand"
					spinand_init()
				
				mtd_device_config_hybrid_lock(spi_nor->mtd, hybrid_flock);
				mtd_device_config_hybrid_lock(spinand->mtd, hybrid_flock);
								
			} else {
       				- if compatible "jedec,spi-nor"
					spi_nor_scan()
       				- if compatible "jedec,spi-nand"
					spinand_init()
			}
		}
  		--
		--	
	}


Whenever read/write/erase come from MTD layer , Take MTD layer lock in function definition i.e. mtd->_read_oob, mtd->_write_oob. mtd->_erase... 
For eg. 
spi_nor_read (struct mtd_info *mtd ...) {
	
	if (hybrid_flock)
		mutex_lock(&mtd-> hybrid_flock);
	spi_nor_lock_and_prep();
	--
	--
	spi_nor_unlock_and_unprep();
	if (hybrid_flock)
		mutex_unlock(&mtd-> hybrid_flock);
}

static int spinand_mtd_read(struct mtd_info *mtd, ...)
{
	if (hybrid_flock)
		mutex_lock(&mtd-> hybrid_flock);

	mutex_lock(&spinand->lock);

	---
	---

	mutex_unlock(&spinand->lock);
	if (hybrid_flock)
		mutex_unlock(&mtd-> hybrid_flock);
}

Please let me know your view on this. 

--pk

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

[Boris Brezillon]

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.

> 
>     };
> };



> 
> 
> There will be only one file i.e. fsl_qspi.c handing NOR and NAND. QSPI controller will have SPI NOR and a SPI NAND controller embedded.
> Question: What should be the location of this file?  driver/mtd/spi-nor definitely is not right place??

It stay in driver/mtd/spi-nor until we create a spi-flash layer
(driver/mtd/spi-flash).

> 
> 
> Change in Data structures
> ----------------------------------
> 
> struct mtd_info {
> 	----
> 	struct mutex *hybrid_flock;
>  	---
> }

Nope, the hybrid_flock should not be in mtd_info. We should create an
hybrid_spi_flash struct containing this lock and an array of SPI flash.

struct hybrid_spi_flash;

enum spi_flash_type {
	SPI_NOR,
	SPI_NAND,
};

struct spi_flash {
	enum spi_flash_type type;
	struct hybrid_spi_flash *master;
};

struct hybrid_spi_flash {
	struct mutex lock;
	struct spi_flash **subdevs;
};

struct spinand_device {
	struct spi_flash spiflash;
	...
};

struct spi_nor {
	struct spi_flash spiflash;
	...
};

static inline void spi_flash_master_lock(struct spi_flash *flash)
{
	if (flash->master)
		mutex_lock(&flash->master->lock);
}

static inline void spi_flash_master_unlock(struct spi_flash *flash)
{
	if (flash->master)
		mutex_unlock(&flash->master->lock);
}

> 
> struct fsl_qspi {
> 	---
> 	struct spi_nor nor[FSL_QSPI_MAX_CHIP];
> 	struct spinand_device spinand[FSL_QSPI_MAX_CHIP];

	struct spi_flash *flashes[FSL_QSPI_MAX_CHIP];

> 	---
> }
> 
> Function implementation
> ----------------------------------
> 
> fsl_qspi.c
> 	 probe() {
> 		--
> 		--
>   		 for_each_available_child_of_node(dev->of_node, np) {
> 			if compatible "hybrid" {
> 				hybrid_flock = malloc(sizeof(struct mutex));
>        				- if compatible "jedec,spi-nor" 
> 					spi_nor_scan()
>        				- if compatible "jedec,spi-nand"
> 					spinand_init()
> 				
> 				mtd_device_config_hybrid_lock(spi_nor->mtd, hybrid_flock);
> 				mtd_device_config_hybrid_lock(spinand->mtd, hybrid_flock);
> 								
> 			} else {
>        				- if compatible "jedec,spi-nor"
> 					spi_nor_scan()
>        				- if compatible "jedec,spi-nand"
> 					spinand_init()
> 			}

No, the parsing and hybrid flash creation should be done in some common
code, otherwise we'll have to duplicate it in each an every controller
driver that needs to support this kind of chip.

> 		}
>   		--
> 		--	
> 	}
> 
> 
> Whenever read/write/erase come from MTD layer , Take MTD layer lock in function definition i.e. mtd->_read_oob, mtd->_write_oob. mtd->_erase... 
> For eg. 
> spi_nor_read (struct mtd_info *mtd ...) {
> 	
> 	if (hybrid_flock)
> 		mutex_lock(&mtd-> hybrid_flock);


	spi_flash_master_lock(&spinor->spiflash);

> 	spi_nor_lock_and_prep();
> 	--
> 	--
> 	spi_nor_unlock_and_unprep();
> 	if (hybrid_flock)
> 		mutex_unlock(&mtd-> hybrid_flock);

	spi_flash_master_unlock(&spinor->spiflash);

> }
> 
> static int spinand_mtd_read(struct mtd_info *mtd, ...)
> {
> 	if (hybrid_flock)
> 		mutex_lock(&mtd-> hybrid_flock);

	spi_flash_master_lock(&spinand->spiflash);

> 
> 	mutex_lock(&spinand->lock);
> 
> 	---
> 	---
> 
> 	mutex_unlock(&spinand->lock);
> 	if (hybrid_flock)
> 		mutex_unlock(&mtd-> hybrid_flock);

	spi_flash_master_unlock(&spinand->spiflash);

> }
> 
> Please let me know your view on this. 
> 
> --pk

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

[Jonas Gorski]

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.


Regards
Jonas

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

[Boris Brezillon]

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.

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

[Frieder Schrempf]

Hi Boris,

> On Mon, 8 Jan 2018 08:42:32 +0000
> Prabhakar Kushwaha <prabhakar.kushwaha at 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 at gmail.com]
>> > Sent: Friday, January 05, 2018 7:28 PM
>> > To: Boris Brezillon <boris.brezillon at free-electrons.com>
>> > Cc: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>; Marek Vasut
>> > <marex at denx.de>; Richard Weinberger <richard at nod.at>; Brian Norris
>> > <computersforpeace at gmail.com>; linux-mtd at lists.infradead.org; Cyrille
>> > Pitchen <cyrille.pitchen at 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 at free-electrons.com> wrote:  
>> > > On Fri, 5 Jan 2018 14:38:48 +0100
>> > > Jonas Gorski <jonas.gorski at gmail.com> wrote:
>> > >  
>> > >> On 5 January 2018 at 11:21, Prabhakar Kushwaha
>> > >> <prabhakar.kushwaha at nxp.com> wrote:  
>> > >> > Thanks Boris for the encouragement.
>> > >> >  
>> > >> >> -----Original Message-----
>> > >> >> From: Boris Brezillon [mailto:boris.brezillon at free-electrons.com]
>> > >> >> Sent: Thursday, January 04, 2018 11:17 PM
>> > >> >> To: Prabhakar Kushwaha <prabhakar.kushwaha at nxp.com>
>> > >> >> Cc: linux-mtd at lists.infradead.org; Cyrille Pitchen  
>> > <cyrille.pitchen at wedev4u.fr>;  
>> > >> >> Richard Weinberger <richard at nod.at>; Marek Vasut <marex at denx.de>;  
>> > Brian  
>> > >> >> Norris <computersforpeace at 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 at 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 at 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 at 0 {
>> > >>                 compatible = "winbond,w25q16fw";
>> > >>                 reg = <0>;
>> > >>                 spi-max-frequency = <20000000>;
>> > >>                 #address-cells = <1>;
>> > >>                 #size-cells = <0>;
>> > >>
>> > >>                 nor at 0 {
>> > >>                                 compatible = "jedec,spi-nor";
>> > >>                                 reg = <0>;
>> > >>                                 #address-cells = <1>;
>> > >>                                 #size-cells = <1>;
>> > >>
>> > >>                                 partitions {
>> > >>                                                 ...
>> > >>                                 };
>> > >>                 };
>> > >>
>> > >>                 nand at 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 ;-).  

With operations, that consist of several SPI messages, do you mean 
something like NAND page program?

Because I'm quite sure something like this should be possible (and all 
of these commands consist only of one spi message, don't they?):
1. Select second (NAND) die
2. Send data to the NAND page buffer (PROGRAM)
3. Select first (NOR) die
4. Program data to a NOR block
5. Select second (NAND) die
6. Send command to transfer page buffer to flash (PROGRAM EXECUTE)

>> > 
>> > 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 at 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 at 0 {
>>                                 compatible = "jedec,spi-nor";
>>                                 reg = <0>;
>>                                 #address-cells = <1>;
>>                                 #size-cells = <1>;
>> 
>>                                 partitions {
>>                                                 ...
>>                                 };
>>                 };
>> 
>>                 nand at 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.
> 
>> 
>>     };
>> };
> 
> 
> 
>> 
>> 
>> There will be only one file i.e. fsl_qspi.c handing NOR and NAND. QSPI controller will have SPI NOR and a SPI NAND controller embedded.
>> Question: What should be the location of this file?  driver/mtd/spi-nor definitely is not right place??
> 
> It stay in driver/mtd/spi-nor until we create a spi-flash layer
> (driver/mtd/spi-flash).

Can it really stay there even if it would have NAND support implemented 
and be used by the SPI NAND framework?

How does the longterm plan for implementing SPI NAND, FSL QSPI NAND/NOR 
and spi-flash layer look like?

I would propose something like this, but I'm not sure if this is 
appropriate:

1. Adding SPI NAND framework (with Micron and generic SPI)
2. Adding FSL QSPI as a SPI NAND controller in mtd/nand/spi/controllers
3. Merging FSL QSPI NAND driver from mtd/nand/spi/controllers with NOR 
driver at mtd/spi-nor/fsl_quadspi.c
4. Creating spi-flash layer and move FSL QSPI NOR/NAND driver to 
mtd/spi-flash

The support for hybrid NAND/NOR chips would not be available until the 
spi-flash layer is done, right?

> 
>> 
>> 
>> Change in Data structures
>> ----------------------------------
>> 
>> struct mtd_info {
>> 	----
>> 	struct mutex *hybrid_flock;
>>  	---
>> }
> 
> Nope, the hybrid_flock should not be in mtd_info. We should create an
> hybrid_spi_flash struct containing this lock and an array of SPI flash.
> 
> struct hybrid_spi_flash;
> 
> enum spi_flash_type {
> 	SPI_NOR,
> 	SPI_NAND,
> };
> 
> struct spi_flash {
> 	enum spi_flash_type type;
> 	struct hybrid_spi_flash *master;
> };
> 
> struct hybrid_spi_flash {
> 	struct mutex lock;
> 	struct spi_flash **subdevs;
> };
> 
> struct spinand_device {
> 	struct spi_flash spiflash;
> 	...
> };
> 
> struct spi_nor {
> 	struct spi_flash spiflash;
> 	...
> };
> 
> static inline void spi_flash_master_lock(struct spi_flash *flash)
> {
> 	if (flash->master)
> 		mutex_lock(&flash->master->lock);
> }
> 
> static inline void spi_flash_master_unlock(struct spi_flash *flash)
> {
> 	if (flash->master)
> 		mutex_unlock(&flash->master->lock);
> }
> 
>> 
>> struct fsl_qspi {
>> 	---
>> 	struct spi_nor nor[FSL_QSPI_MAX_CHIP];
>> 	struct spinand_device spinand[FSL_QSPI_MAX_CHIP];
> 
> 	struct spi_flash *flashes[FSL_QSPI_MAX_CHIP];
> 
>> 	---
>> }
>> 
>> Function implementation
>> ----------------------------------
>> 
>> fsl_qspi.c
>> 	 probe() {
>> 		--
>> 		--
>>   		 for_each_available_child_of_node(dev->of_node, np) {
>> 			if compatible "hybrid" {
>> 				hybrid_flock = malloc(sizeof(struct mutex));
>>        				- if compatible "jedec,spi-nor" 
>> 					spi_nor_scan()
>>        				- if compatible "jedec,spi-nand"
>> 					spinand_init()
>> 				
>> 				mtd_device_config_hybrid_lock(spi_nor->mtd, hybrid_flock);
>> 				mtd_device_config_hybrid_lock(spinand->mtd, hybrid_flock);
>> 								
>> 			} else {
>>        				- if compatible "jedec,spi-nor"
>> 					spi_nor_scan()
>>        				- if compatible "jedec,spi-nand"
>> 					spinand_init()
>> 			}
> 
> No, the parsing and hybrid flash creation should be done in some common
> code, otherwise we'll have to duplicate it in each an every controller
> driver that needs to support this kind of chip.
> 
>> 		}
>>   		--
>> 		--	
>> 	}
>> 
>> 
>> Whenever read/write/erase come from MTD layer , Take MTD layer lock in function definition i.e. mtd->_read_oob, mtd->_write_oob. mtd->_erase... 
>> For eg. 
>> spi_nor_read (struct mtd_info *mtd ...) {
>> 	
>> 	if (hybrid_flock)
>> 		mutex_lock(&mtd-> hybrid_flock);
> 
> 
> 	spi_flash_master_lock(&spinor->spiflash);
> 
>> 	spi_nor_lock_and_prep();
>> 	--
>> 	--
>> 	spi_nor_unlock_and_unprep();
>> 	if (hybrid_flock)
>> 		mutex_unlock(&mtd-> hybrid_flock);
> 
> 	spi_flash_master_unlock(&spinor->spiflash);
> 
>> }
>> 
>> static int spinand_mtd_read(struct mtd_info *mtd, ...)
>> {
>> 	if (hybrid_flock)
>> 		mutex_lock(&mtd-> hybrid_flock);
> 
> 	spi_flash_master_lock(&spinand->spiflash);
> 
>> 
>> 	mutex_lock(&spinand->lock);
>> 
>> 	---
>> 	---
>> 
>> 	mutex_unlock(&spinand->lock);
>> 	if (hybrid_flock)
>> 		mutex_unlock(&mtd-> hybrid_flock);
> 
> 	spi_flash_master_unlock(&spinand->spiflash);
> 
>> }
>> 
>> Please let me know your view on this. 
>> 
>> --pk
Thanks,

Frieder

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

[Boris Brezillon]

Hi Frieder,

On Mon, 8 Jan 2018 12:02:19 +0100
Frieder Schrempf <frieder.schrempf@exceet.de> wrote:

> >> > >> 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 ;-).    
> 
> With operations, that consist of several SPI messages, do you mean 
> something like NAND page program?

Yep.

> 
> Because I'm quite sure something like this should be possible (and all 
> of these commands consist only of one spi message, don't they?):
> 1. Select second (NAND) die
> 2. Send data to the NAND page buffer (PROGRAM)
> 3. Select first (NOR) die
> 4. Program data to a NOR block
> 5. Select second (NAND) die
> 6. Send command to transfer page buffer to flash (PROGRAM EXECUTE)

Yes, and that's the problem, if you don't have a lock, the sequence you
describe above could be re-ordered like this:

1. Select second (NAND) die
2. Select first (NOR) die
3. Send data to the NAND page buffer (PROGRAM)
4. Program data to a NOR block
...

> 
> >> > 
> >> > 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 at 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 at 0 {
> >>                                 compatible = "jedec,spi-nor";
> >>                                 reg = <0>;
> >>                                 #address-cells = <1>;
> >>                                 #size-cells = <1>;
> >> 
> >>                                 partitions {
> >>                                                 ...
> >>                                 };
> >>                 };
> >> 
> >>                 nand at 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.
> >   
> >> 
> >>     };
> >> };  
> > 
> > 
> >   
> >> 
> >> 
> >> There will be only one file i.e. fsl_qspi.c handing NOR and NAND. QSPI controller will have SPI NOR and a SPI NAND controller embedded.
> >> Question: What should be the location of this file?  driver/mtd/spi-nor definitely is not right place??  
> > 
> > It stay in driver/mtd/spi-nor until we create a spi-flash layer
> > (driver/mtd/spi-flash).  
> 
> Can it really stay there even if it would have NAND support implemented 
> and be used by the SPI NAND framework?

Yes, as long as this is a temporary situation.

> 
> How does the longterm plan for implementing SPI NAND, FSL QSPI NAND/NOR 
> and spi-flash layer look like?
> 
> I would propose something like this, but I'm not sure if this is 
> appropriate:
> 
> 1. Adding SPI NAND framework (with Micron and generic SPI)

Yep, that's the first step, and I think we should move on with what we
have and work on improving the situation (to share more code between
SPI NOR and SPI NAND) in parallel.

> 2. Adding FSL QSPI as a SPI NAND controller in mtd/nand/spi/controllers
> 3. Merging FSL QSPI NAND driver from mtd/nand/spi/controllers with NOR 
> driver at mtd/spi-nor/fsl_quadspi.c

I'd really prefer to have a single driver supporting both NOR and NAND
devices. What's the problem with adding SPI NAND support to the
mtd/spi-nor/fsl_quadspi.c?

> 4. Creating spi-flash layer and move FSL QSPI NOR/NAND driver to 
> mtd/spi-flash

Yep, that's the long term goal.

> 
> The support for hybrid NAND/NOR chips would not be available until the 
> spi-flash layer is done, right?

Exactly.

Regards,

Boris

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

[Frieder Schrempf]

Hi Boris,

On 08.01.2018 13:31, Boris Brezillon wrote:
> Hi Frieder,
> 
> On Mon, 8 Jan 2018 12:02:19 +0100
> Frieder Schrempf <frieder.schrempf@exceet.de> wrote:
> 
>>>>>>> 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 ;-).
>>
>> With operations, that consist of several SPI messages, do you mean
>> something like NAND page program?
> 
> Yep.
> 
>>
>> Because I'm quite sure something like this should be possible (and all
>> of these commands consist only of one spi message, don't they?):
>> 1. Select second (NAND) die
>> 2. Send data to the NAND page buffer (PROGRAM)
>> 3. Select first (NOR) die
>> 4. Program data to a NOR block
>> 5. Select second (NAND) die
>> 6. Send command to transfer page buffer to flash (PROGRAM EXECUTE)
> 
> Yes, and that's the problem, if you don't have a lock, the sequence you
> describe above could be re-ordered like this:
> 
> 1. Select second (NAND) die
> 2. Select first (NOR) die
> 3. Send data to the NAND page buffer (PROGRAM)
> 4. Program data to a NOR block
> ...

Ok thanks. Now I got the problem.

> 
>>
>>>>>
>>>>> 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 at 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 at 0 {
>>>>                                  compatible = "jedec,spi-nor";
>>>>                                  reg = <0>;
>>>>                                  #address-cells = <1>;
>>>>                                  #size-cells = <1>;
>>>>
>>>>                                  partitions {
>>>>                                                  ...
>>>>                                  };
>>>>                  };
>>>>
>>>>                  nand at 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.
>>>    
>>>>
>>>>      };
>>>> };
>>>
>>>
>>>    
>>>>
>>>>
>>>> There will be only one file i.e. fsl_qspi.c handing NOR and NAND. QSPI controller will have SPI NOR and a SPI NAND controller embedded.
>>>> Question: What should be the location of this file?  driver/mtd/spi-nor definitely is not right place??
>>>
>>> It stay in driver/mtd/spi-nor until we create a spi-flash layer
>>> (driver/mtd/spi-flash).
>>
>> Can it really stay there even if it would have NAND support implemented
>> and be used by the SPI NAND framework?
> 
> Yes, as long as this is a temporary situation.

Ok.

> 
>>
>> How does the longterm plan for implementing SPI NAND, FSL QSPI NAND/NOR
>> and spi-flash layer look like?
>>
>> I would propose something like this, but I'm not sure if this is
>> appropriate:
>>
>> 1. Adding SPI NAND framework (with Micron and generic SPI)
> 
> Yep, that's the first step, and I think we should move on with what we
> have and work on improving the situation (to share more code between
> SPI NOR and SPI NAND) in parallel.

Ok.

> 
>> 2. Adding FSL QSPI as a SPI NAND controller in mtd/nand/spi/controllers
>> 3. Merging FSL QSPI NAND driver from mtd/nand/spi/controllers with NOR
>> driver at mtd/spi-nor/fsl_quadspi.c
> 
> I'd really prefer to have a single driver supporting both NOR and NAND
> devices. What's the problem with adding SPI NAND support to the
> mtd/spi-nor/fsl_quadspi.c?

There's not really a problem. I will see if can adapt my FSL QSPI NAND 
code to fit into the NOR driver.

> 
>> 4. Creating spi-flash layer and move FSL QSPI NOR/NAND driver to
>> mtd/spi-flash
> 
> Yep, that's the long term goal.
> 
>>
>> The support for hybrid NAND/NOR chips would not be available until the
>> spi-flash layer is done, right?
> 
> Exactly.
> 
> Regards,
> 
> Boris
> 

Thanks and regards,

Frieder

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

[Jonas Gorski]

On 8 January 2018 at 13:31, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Frieder,
>
> On Mon, 8 Jan 2018 12:02:19 +0100
> Frieder Schrempf <frieder.schrempf@exceet.de> wrote:
>
>> >> > >> 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 ;-).
>>
>> With operations, that consist of several SPI messages, do you mean
>> something like NAND page program?
>
> Yep.
>
>>
>> Because I'm quite sure something like this should be possible (and all
>> of these commands consist only of one spi message, don't they?):
>> 1. Select second (NAND) die
>> 2. Send data to the NAND page buffer (PROGRAM)
>> 3. Select first (NOR) die
>> 4. Program data to a NOR block
>> 5. Select second (NAND) die
>> 6. Send command to transfer page buffer to flash (PROGRAM EXECUTE)
>
> Yes, and that's the problem, if you don't have a lock, the sequence you
> describe above could be re-ordered like this:
>
> 1. Select second (NAND) die
> 2. Select first (NOR) die
> 3. Send data to the NAND page buffer (PROGRAM)
> 4. Program data to a NOR block
> ...

At least with what I was thinking of (with the spi-multiplexer) this
should be impossible, as the (virtual) spi bus message pump should
ensure that any messages from spi-nand and spi-nor are properly
serialized so they could only end up as in Frieder's example, unless
I'm overlooking something.

So my virtual's spi implementation would look like this:

struct W25M161AW {
        struct spi_device *spi; /* we at the parent bus */
};

int W25M161AW_transfer_one_message(master, message)
{
        W25M161AW *w = spi_master_get_devdata(master);
        spi_device *spi = message->spi;

        W25M161AW_send_die_select(w->spi, spi->chip_select);

        /* pass on the message to the parent bus */
        spi_sync(w->spi, msg);
        spi_finalize_current_message(master);

        return 0;
}

(this is a very simplified version, of course we can't pass on the
message directly to the parent bus but need to clone it)

This way AFAICT there should be a guarantee that there can be no other
message to the flash scheduled between the die select and the message
to the die.

Of course if we need to ensure that there is no die change between the
PROGRAM and the PROGRAM EXECUTE we need the extra bus lock. Without a
public datasheet I can't tell though.


Regards
Jonas

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

[Boris Brezillon]

On Mon, 8 Jan 2018 14:43:56 +0100
Jonas Gorski <jonas.gorski@gmail.com> wrote:

> On 8 January 2018 at 13:31, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Hi Frieder,
> >
> > On Mon, 8 Jan 2018 12:02:19 +0100
> > Frieder Schrempf <frieder.schrempf@exceet.de> wrote:
> >  
> >> >> > >> 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 ;-).  
> >>
> >> With operations, that consist of several SPI messages, do you mean
> >> something like NAND page program?  
> >
> > Yep.
> >  
> >>
> >> Because I'm quite sure something like this should be possible (and all
> >> of these commands consist only of one spi message, don't they?):
> >> 1. Select second (NAND) die
> >> 2. Send data to the NAND page buffer (PROGRAM)
> >> 3. Select first (NOR) die
> >> 4. Program data to a NOR block
> >> 5. Select second (NAND) die
> >> 6. Send command to transfer page buffer to flash (PROGRAM EXECUTE)  
> >
> > Yes, and that's the problem, if you don't have a lock, the sequence you
> > describe above could be re-ordered like this:
> >
> > 1. Select second (NAND) die
> > 2. Select first (NOR) die
> > 3. Send data to the NAND page buffer (PROGRAM)
> > 4. Program data to a NOR block
> > ...  
> 
> At least with what I was thinking of (with the spi-multiplexer) this
> should be impossible, as the (virtual) spi bus message pump should
> ensure that any messages from spi-nand and spi-nor are properly
> serialized so they could only end up as in Frieder's example, unless
> I'm overlooking something.
> 
> So my virtual's spi implementation would look like this:
> 
> struct W25M161AW {
>         struct spi_device *spi; /* we at the parent bus */
> };
> 
> int W25M161AW_transfer_one_message(master, message)
> {
>         W25M161AW *w = spi_master_get_devdata(master);
>         spi_device *spi = message->spi;
> 
>         W25M161AW_send_die_select(w->spi, spi->chip_select);
> 
>         /* pass on the message to the parent bus */
>         spi_sync(w->spi, msg);
>         spi_finalize_current_message(master);
> 
>         return 0;
> }

I wish all QSPI controllers were exposed as regular SPI controllers with
QSPI capabilities, unfortunately most of them are standalone drivers
that simply does not implement the generic SPI interface and instead
directly register to their flash devices to SPI NOR layer, thus
preventing the generic logic you're proposing here :-(.

> 
> (this is a very simplified version, of course we can't pass on the
> message directly to the parent bus but need to clone it)
> 
> This way AFAICT there should be a guarantee that there can be no other
> message to the flash scheduled between the die select and the message
> to the die.
> 
> Of course if we need to ensure that there is no die change between the
> PROGRAM and the PROGRAM EXECUTE we need the extra bus lock. Without a
> public datasheet I can't tell though.

I found this one [1] if you want to have a look.

Regards,

Boris

[1]http://www.winbond.com.tw/resource-files/w25m161av%20combo%20reva%20091317%20mod%20final.pdf

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

[Jonas Gorski]

On 8 January 2018 at 15:32, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Mon, 8 Jan 2018 14:43:56 +0100
> Jonas Gorski <jonas.gorski@gmail.com> wrote:
>
>> On 8 January 2018 at 13:31, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> > Hi Frieder,
>> >
>> > On Mon, 8 Jan 2018 12:02:19 +0100
>> > Frieder Schrempf <frieder.schrempf@exceet.de> wrote:
>> >
>> >> >> > >> 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 ;-).
>> >>
>> >> With operations, that consist of several SPI messages, do you mean
>> >> something like NAND page program?
>> >
>> > Yep.
>> >
>> >>
>> >> Because I'm quite sure something like this should be possible (and all
>> >> of these commands consist only of one spi message, don't they?):
>> >> 1. Select second (NAND) die
>> >> 2. Send data to the NAND page buffer (PROGRAM)
>> >> 3. Select first (NOR) die
>> >> 4. Program data to a NOR block
>> >> 5. Select second (NAND) die
>> >> 6. Send command to transfer page buffer to flash (PROGRAM EXECUTE)
>> >
>> > Yes, and that's the problem, if you don't have a lock, the sequence you
>> > describe above could be re-ordered like this:
>> >
>> > 1. Select second (NAND) die
>> > 2. Select first (NOR) die
>> > 3. Send data to the NAND page buffer (PROGRAM)
>> > 4. Program data to a NOR block
>> > ...
>>
>> At least with what I was thinking of (with the spi-multiplexer) this
>> should be impossible, as the (virtual) spi bus message pump should
>> ensure that any messages from spi-nand and spi-nor are properly
>> serialized so they could only end up as in Frieder's example, unless
>> I'm overlooking something.
>>
>> So my virtual's spi implementation would look like this:
>>
>> struct W25M161AW {
>>         struct spi_device *spi; /* we at the parent bus */
>> };
>>
>> int W25M161AW_transfer_one_message(master, message)
>> {
>>         W25M161AW *w = spi_master_get_devdata(master);
>>         spi_device *spi = message->spi;
>>
>>         W25M161AW_send_die_select(w->spi, spi->chip_select);
>>
>>         /* pass on the message to the parent bus */
>>         spi_sync(w->spi, msg);
>>         spi_finalize_current_message(master);
>>
>>         return 0;
>> }
>
> I wish all QSPI controllers were exposed as regular SPI controllers with
> QSPI capabilities, unfortunately most of them are standalone drivers
> that simply does not implement the generic SPI interface and instead
> directly register to their flash devices to SPI NOR layer, thus
> preventing the generic logic you're proposing here :-(.
>
>>
>> (this is a very simplified version, of course we can't pass on the
>> message directly to the parent bus but need to clone it)
>>
>> This way AFAICT there should be a guarantee that there can be no other
>> message to the flash scheduled between the die select and the message
>> to the die.
>>
>> Of course if we need to ensure that there is no die change between the
>> PROGRAM and the PROGRAM EXECUTE we need the extra bus lock. Without a
>> public datasheet I can't tell though.
>
> I found this one [1] if you want to have a look.
>
> Regards,
>
> Boris
>
> [1]http://www.winbond.com.tw/resource-files/w25m161av%20combo%20reva%20091317%20mod%20final.pdf

Thanks, that helped a lot. If I understand it correctly, we don't need
to care about interleaving e.g. the PROGRAM and PROGRAM EXECUTE with
NOR commands. "5. CONCURRENT OPERATION DESCRIPTIONS" (Page 7) says:

"However, the
inactive/idle die can still perform internal Program/Erase operation
which was initiated when the die was
active. Therefore, “Read (on Active die) while Program/Erase (on Idle
die)” and “Multi-die Program/Erase
(both Active & Idle dice)” concurrent operations are feasible in the
SpiStack ® series. “Software Die Select
(C2h)” instruction will only change the active/idle status of the
stacked dice, and it will not interrupt any on-
going Program/Erase operations."

So we shouldn't need to lock the bus for multiple related SPI messages.

Also since the Software Die Select command takes an 8-bit die id, it
might make sense to model the dies with an address that matches the
die id.


Regards
Jonas

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

[Boris Brezillon]

On Mon, 8 Jan 2018 16:18:33 +0100
Jonas Gorski <jonas.gorski@gmail.com> wrote:

> On 8 January 2018 at 15:32, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Mon, 8 Jan 2018 14:43:56 +0100
> > Jonas Gorski <jonas.gorski@gmail.com> wrote:
> >  
> >> On 8 January 2018 at 13:31, Boris Brezillon
> >> <boris.brezillon@free-electrons.com> wrote:  
> >> > Hi Frieder,
> >> >
> >> > On Mon, 8 Jan 2018 12:02:19 +0100
> >> > Frieder Schrempf <frieder.schrempf@exceet.de> wrote:
> >> >  
> >> >> >> > >> 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 ;-).  
> >> >>
> >> >> With operations, that consist of several SPI messages, do you mean
> >> >> something like NAND page program?  
> >> >
> >> > Yep.
> >> >  
> >> >>
> >> >> Because I'm quite sure something like this should be possible (and all
> >> >> of these commands consist only of one spi message, don't they?):
> >> >> 1. Select second (NAND) die
> >> >> 2. Send data to the NAND page buffer (PROGRAM)
> >> >> 3. Select first (NOR) die
> >> >> 4. Program data to a NOR block
> >> >> 5. Select second (NAND) die
> >> >> 6. Send command to transfer page buffer to flash (PROGRAM EXECUTE)  
> >> >
> >> > Yes, and that's the problem, if you don't have a lock, the sequence you
> >> > describe above could be re-ordered like this:
> >> >
> >> > 1. Select second (NAND) die
> >> > 2. Select first (NOR) die
> >> > 3. Send data to the NAND page buffer (PROGRAM)
> >> > 4. Program data to a NOR block
> >> > ...  
> >>
> >> At least with what I was thinking of (with the spi-multiplexer) this
> >> should be impossible, as the (virtual) spi bus message pump should
> >> ensure that any messages from spi-nand and spi-nor are properly
> >> serialized so they could only end up as in Frieder's example, unless
> >> I'm overlooking something.
> >>
> >> So my virtual's spi implementation would look like this:
> >>
> >> struct W25M161AW {
> >>         struct spi_device *spi; /* we at the parent bus */
> >> };
> >>
> >> int W25M161AW_transfer_one_message(master, message)
> >> {
> >>         W25M161AW *w = spi_master_get_devdata(master);
> >>         spi_device *spi = message->spi;
> >>
> >>         W25M161AW_send_die_select(w->spi, spi->chip_select);
> >>
> >>         /* pass on the message to the parent bus */
> >>         spi_sync(w->spi, msg);
> >>         spi_finalize_current_message(master);
> >>
> >>         return 0;
> >> }  
> >
> > I wish all QSPI controllers were exposed as regular SPI controllers with
> > QSPI capabilities, unfortunately most of them are standalone drivers
> > that simply does not implement the generic SPI interface and instead
> > directly register to their flash devices to SPI NOR layer, thus
> > preventing the generic logic you're proposing here :-(.
> >  
> >>
> >> (this is a very simplified version, of course we can't pass on the
> >> message directly to the parent bus but need to clone it)
> >>
> >> This way AFAICT there should be a guarantee that there can be no other
> >> message to the flash scheduled between the die select and the message
> >> to the die.
> >>
> >> Of course if we need to ensure that there is no die change between the
> >> PROGRAM and the PROGRAM EXECUTE we need the extra bus lock. Without a
> >> public datasheet I can't tell though.  
> >
> > I found this one [1] if you want to have a look.
> >
> > Regards,
> >
> > Boris
> >
> > [1]http://www.winbond.com.tw/resource-files/w25m161av%20combo%20reva%20091317%20mod%20final.pdf  
> 
> Thanks, that helped a lot. If I understand it correctly, we don't need
> to care about interleaving e.g. the PROGRAM and PROGRAM EXECUTE with
> NOR commands. "5. CONCURRENT OPERATION DESCRIPTIONS" (Page 7) says:
> 
> "However, the
> inactive/idle die can still perform internal Program/Erase operation
> which was initiated when the die was
> active. Therefore, “Read (on Active die) while Program/Erase (on Idle
> die)” and “Multi-die Program/Erase
> (both Active & Idle dice)” concurrent operations are feasible in the
> SpiStack ® series. “Software Die Select
> (C2h)” instruction will only change the active/idle status of the
> stacked dice, and it will not interrupt any on-
> going Program/Erase operations."
> 
> So we shouldn't need to lock the bus for multiple related SPI messages.
> 
> Also since the Software Die Select command takes an 8-bit die id, it
> might make sense to model the dies with an address that matches the
> die id.

Sure, and I thinks that's what you and Prabhakar suggested. I just
overlooked the case where partitions are defined in the DT, hence my
initial suggestion to not define dies in the DT. So, in the end, the
following looks like a valid representation:

&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 = "spi-nand";
			reg = <1>;
			#address-cells = <1>;
			#size-cells = <1>;

			partitions {
				...
			};
		};
	};
};