Re: Add stacked and parallel memories support in spi-nor

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Amit,

amit.kumar-mahapatra@amd.com wrote on Thu, 10 Oct 2024 10:35:06 +0000:

> Hello Miquel,
> 
> > -----Original Message-----
> > From: Miquel Raynal <miquel.raynal@bootlin.com>
> > Sent: Thursday, October 10, 2024 2:58 PM
> > To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@amd.com>
> > Cc: Tudor Ambarus <tudor.ambarus@linaro.org>; michael@walle.cc;
> > broonie@kernel.org; pratyush@kernel.org; richard@nod.at; vigneshr@ti.com; Rob
> > Herring <robh@kernel.org>; cornor+dt@kernel.org; krzk+dt@kernel.org; linux-
> > spi@vger.kernel.org; linux-kernel@vger.kernel.org; linux-mtd@lists.infradead.org;
> > nicolas.ferre@microchip.com; alexandre.belloni@bootlin.com;
> > claudiu.beznea@tuxon.dev; Simek, Michal <michal.simek@amd.com>; linux-arm-
> > kernel@lists.infradead.org; alsa-devel@alsa-project.org;
> > patches@opensource.cirrus.com; linux-sound@vger.kernel.org; git (AMD-Xilinx)
> > <git@amd.com>; amitrkcian2002@gmail.com; Conor Dooley
> > <conor.dooley@microchip.com>; beanhuo@micron.com
> > Subject: Re: Add stacked and parallel memories support in spi-nor
> > 
> > Hi Amit,
> > 
> > amit.kumar-mahapatra@amd.com wrote on Thu, 10 Oct 2024 09:17:58 +0000:
> >   
> > > Hello Miquel,
> > >  
> > > > > - The stacked-memories DT bindings will contain the phandles of
> > > > > the flash nodes  
> > > > connected in stacked mode.  
> > > > >
> > > > > - The first flash node will contain the mtd partition that would
> > > > > have the cross over memory staring at a memory location in the
> > > > > first flash and ending at some memory location of the 2nd flash  
> > > >
> > > > I don't like that much. Describing partitions past the actual device
> > > > sounds wrong. If you look into [1] there is a suggestion from Rob to
> > > > handle this case using a property that tells us there is a
> > > > continuation, so from a software perspective we can easily make the link, but on  
> > the hardware description side the information are correct.  
> > >
> > > I reviewed Rob's suggestions in [1], and I need to examine the MTD
> > > layer to determine how this can be implemented from a software perspective.
> > > For reference, here is Rob's suggestion:
> > >
> > > Describe each device and partition separately and add link(s) from one
> > > partition to the next
> > >
> > > flash0 {
> > >   partitions {
> > >     compatible = "fixed-partitions";
> > >     concat-partition = <&flash1_partitions>;
> > >     ...
> > >   };
> > > };
> > >
> > > flash1 {
> > >   flash1_partition: partitions {
> > >     compatible = "fixed-partitions";
> > >     ...
> > >   };
> > > };
> > >  
> > > >
> > > > If this description is accepted, then fine, you can deprecate the "stacked-  
> > memories"  
> > > > property.  
> > >
> > > I believe that in addition to Rob's description, we should also
> > > include the 'stacked-memories' property. This property helps us
> > > identify which flashes are stacked, while Rob's suggestion explains
> > > how the partitions within the stacked flashes are connected.
> > >
> > > For example, if we have three flashes connected to an SPI host, with
> > > flash@0 and flash@1 operating in stacked mode and flash@2 functioning
> > > as a standalone flash, the Device Tree binding might look something like this:
> > > Please share your thoughts on this.
> > >
> > > spi@0 {
> > >   ...
> > >   flash@0 {
> > >     compatible = "jedec,spi-nor"
> > >     reg = <0x00>;
> > >     stacked-memories = <&flash@0 &flash@1>;
> > >     spi-max-frequency = <50000000>;
> > >     ...
> > >         flash0_partition: partitions {
> > >             compatible = "fixed-partitions";
> > > 	concat-partition = <&flash1_partitions>;
> > >         	partition@0 {
> > >           	    label = "Stacked-Flash-1";
> > >                 reg = <0x0 0x800000>;
> > > 	}
> > >         }
> > >     }
> > >   flash@1 {
> > >     compatible = "jedec,spi-nor"
> > >      reg = <0x01>;
> > >     spi-max-frequency = <50000000>;
> > >     ...
> > >         flash1_partition: partitions {
> > >             compatible = "fixed-partitions";
> > > 	concat-partition = <&flash0_partitions>;
> > >         	partition@0 {
> > >           	    label = " Stacked-Flash-2";
> > >                 reg = <0x0 0x800000>;
> > > 	}
> > >         }
> > >   }
> > >
> > >   flash@2 {
> > >     compatible = "jedec,spi-nor"
> > >      reg = <0x01>;
> > >     spi-max-frequency = <50000000>;
> > >     ...
> > >         partitions {
> > >             compatible = "fixed-partitions";
> > > 	concat-partition = <&flash0_partitions>;
> > >         	partition@0 {
> > >           	    label = "Single-Flash";
> > >                 reg = <0x0 0x800000>;
> > > 	}
> > >         }
> > >   }  
> > 
> > I'm sorry but this is pretty messed up. The alignments are wrong, I believe the labels
> > are wrong, the reg properties as well. Can you please work on this example and
> > send an updated version?  
> 
> Apologies for that. Here's the updated version along with the explanation.

Thanks for the update.

> spi@0 {
> 	...
>   	flash@0 {
>   		compatible = "jedec,spi-nor"
>     		reg = <0x00>;
>     		stacked-memories = <&flash@0 &flash@1>;

The same property should, IMHO, also be expected...

>     		spi-max-frequency = <50000000>;
>     		...
>         		partitions {
>             		compatible = "fixed-partitions";
>         			concat-partition = <&flash1_partition>; /* Link to the flash@1 partition@0 */
>                 		flash0_partition: partition@0 {
>                     			label = "part0_0";
>                 			reg = <0x0 0x800000>;
>         			}
>         		}
>     	}
>   	flash@1 {
>     		compatible = "jedec,spi-nor"
>      		reg = <0x01>;

... here.

>     		spi-max-frequency = <50000000>;
>     		...
>         		partitions {
>             		compatible = "fixed-partitions";
>         			concat-partition = <&flash0_partition>; /* Link to the flash@0 partition@0 */
>                 		flash1_partition: partition@0 {
>                     			label = "part0_1";
>                 			reg = <0x0 0x800000>;
>         			}
>         		}
>   	}
> 
>   	flash@2 {
>     		compatible = "jedec,spi-nor"
>      		reg = <0x02>;
>     		spi-max-frequency = <50000000>;
>     		...
>         		partitions {
>             		compatible = "fixed-partitions";       
>                 		partition@0 {
>                     			label = "part1_0";
>                 			reg = <0x0 0x800000>;
>         			}
>         		}
>   	}
> }

Otherwise, okay for me.

Thanks,
Miquèl