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

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

[Miquel Raynal]

Hi Amit,

> For implementing this the current DT binding[1] [2] [3] need to be updated as follows.
> 
> 
> 
> stacked-memories DT changes:
> 
> - Flash size information can be retrieved directly from the flash, so it has been removed from the DT binding.
> 
> - Each stacked flash will have its own flash node. This approach allows flashes of different makes and sizes to be stacked together, as each flash will be probed individually.
> 
> -  Each of the flash node will have its own "reg" property that will contain its physical CS.

These three first points are just describing the existing bindings for
non-concatenated situations.

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

If this description is accepted, then fine, you can deprecate the 
"stacked-memories" property.

>  - The new layer will update the mtd->size and other mtd_info parameters after both the flashes are probed and will call mtd_device_register with the combined information.

Okay, this is back to the mtd-concat thing I initially proposed, but I
believe it can work.

[...]

> parallel-memories DT changes:
> 
> - Flash size information can be retrieved directly from the flash, so it has been removed from the DT binding.

It's not really about the size but more about the fact that two
memories are in use. If the stacked situation does not require anything
specific besides the partitions trick, then you can assume that double
reg flashes are just two flashes and this can be your way to
discriminate the data organization. But I don't like much this shortcut
because it is not future proof, and instead I'd keep the stacked-memory
property. If you don't like the size, I don't really care, just use it
as a boolean. But I believe we need some naming to tell the OS that the
data is spread in a specific way inside the memory devices.

> - Each flash connected in parallel mode should be identical and will have one flash node for both the flash devices.

This is already the case.

> - The "reg" prop will contain the physical CS number for both the connected flashes.

This is already the case.

> - The new layer will double the mtd-> size and register it with the mtd layer.

This is not a DT change.


Thanks,
Miquèl

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

[Mahapatra, Amit Kumar]

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

> 
> >  - The new layer will update the mtd->size and other mtd_info parameters after
> both the flashes are probed and will call mtd_device_register with the combined
> information.
> 
> Okay, this is back to the mtd-concat thing I initially proposed, but I believe it can
> work.

I was considering handling this by only adding a new layer between the MTD 
layer and SPI NOR without modifying the MTD layer. However, with Rob's 
suggestion, we would also need to update the mtd-concat in addition to 
adding the new layer.

> 
> [...]
> 
> > parallel-memories DT changes:
> >
> > - Flash size information can be retrieved directly from the flash, so it has been
> removed from the DT binding.
> 
> It's not really about the size but more about the fact that two memories are in use. If
> the stacked situation does not require anything specific besides the partitions trick,
> then you can assume that double reg flashes are just two flashes and this can be
> your way to discriminate the data organization. But I don't like much this shortcut
> because it is not future proof, and instead I'd keep the stacked-memory property. If

I agree, as explained above, I believe we will need the 'stacked-memories' 
property.

> you don't like the size, I don't really care, just use it as a boolean. But I believe we
> need some naming to tell the OS that the data is spread in a specific way inside the

Agree.


Regards,
Amit

> memory devices.
> 
> > - Each flash connected in parallel mode should be identical and will have one flash
> node for both the flash devices.
> 
> This is already the case.
> 
> > - The "reg" prop will contain the physical CS number for both the connected
> flashes.
> 
> This is already the case.
> 
> > - The new layer will double the mtd-> size and register it with the mtd layer.
> 
> This is not a DT change.
> 
> 
> Thanks,
> Miquèl

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

[Miquel Raynal]

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?

Thanks,
Miquèl

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

[Mahapatra, Amit Kumar]

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.

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. 
I also need to examine the MTD layer to determine how Rob's suggestion 
can be implemented from a software perspective.

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

Regards,
Amit
> 
> Thanks,
> Miquèl

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

[Miquel Raynal]

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