RE: [RFC PATCH 2/2] dt-bindings: spi: Update stacked and parallel bindings

["Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com>]

> > 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 = <&flash0_part0 &flash1_part0>;
> >
> > 			flash0_part0: partition@0 {
> > 				label = "part0_0";
> > 				reg = <0x0 0x800000>;
> > 			};
> > 		};
> > 	};
> > 	flash@1 {
> > 		compatible = "jedec,spi-nor"
> > 		reg = <0x01>;
> > 		stacked-memories = <&flash@0 &flash@1>;
> > 		spi-max-frequency = <50000000>;
> > 		...
> > 		partitions {
> > 			compatible = "fixed-partitions";
> > 			concat-partition = <&flash0_part0 &flash1_part0>;
> >
> > 			flash1_part0: partition@0 {
> > 				label = "part0_1";
> > 				reg = <0x0 0x800000>;
> > 			};
> > 		};
> > 	};
> > };
> >
> >>
> >> >                         compatible = "fixed-partitions";
> >> >                                 concat-partition = <&flash0_partition &flash1_partition>;
> >> >                                 flash1_partition: partition@0 {
> >> >                                         label = "part0_1";
> >> >                                         reg = <0x0 0x800000>;
> >> >                                 }
> >> >                         }
> >> >         }
> >> >
> >> > }
> >> >
> >> > parallel-memories binding changes:
> >> > - Remove the size information from the bindings and change the type to
> >> >   boolen.
> >> > - Each flash connected in parallel mode should be identical and will have
> >> >   one flash node for both the flash devices.
> >> > - The “reg” prop will contain the physical CS number for both the connected
> >> >   flashes.
> >> >
> >> > The new layer will double the mtd-> size and register it with the
> >> > mtd layer.
> >>
> >> Not so sure about that, you'll need a new mtd device to capture the whole
> device.
> >> But this is implementation related, not relevant for binding.
> >>
> >> >
> >> > spi@1 {
> >> >         ...
> >> >         flash@3 {
> >> >                 compatible = "jedec,spi-nor"
> >> >                 reg = <0x00 0x01>;
> >> >                 paralle-memories ;
> >>
> >> Please fix the typos and the spacing (same above).
> >>
> >> >                 spi-max-frequency = <50000000>;
> >> >                 ...
> >> >                         partitions {
> >> >                         compatible = "fixed-partitions";
> >> >                                 flash0_partition: partition@0 {
> >> >                                         label = "part0_0";
> >> >                                         reg = <0x0 0x800000>;
> >> >                                 }
> >> >                         }
> >> >         }
> >> > }
> >> >
> >> > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
> >> > ---
> >> >  .../bindings/spi/spi-controller.yaml          | 23 +++++++++++++++++--
> >> >  .../bindings/spi/spi-peripheral-props.yaml    |  9 +++-----
> >> >  2 files changed, 24 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git
> >> > a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> >> > b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> >> > index 093150c0cb87..2d300f98dd72 100644
> >> > --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> >> > +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> >> > @@ -185,7 +185,26 @@ examples:
> >> >          flash@2 {
> >> >              compatible = "jedec,spi-nor";
> >> >              spi-max-frequency = <50000000>;
> >> > -            reg = <2>, <3>;
> >> > -            stacked-memories = /bits/ 64 <0x10000000 0x10000000>;
> >> > +            reg = <2>;
> >> > +            stacked-memories = <&flash0 &flash1>;
> >> >          };
> >>
> >> I'm sorry but this is not what you've talked about in this series.
> >> Either you have flash0 and flash1 and use the stacked-memories
> >> property in both of them (which is what you described) or you create
> >> a third virtual device which points to two other flashes. This
> >> example allows for an easier use of the partitions
> >
> > If I understand your point correctly, you're suggesting that we should
> > avoid using stacked-memories and concat-partition properties together
> > and instead choose one approach. Between the two, I believe
> > concat-partition would be the better option.
> 
> That's not exactly it, look at the reg properties above, they do not match the flash
> devices. Your example above invalid but it is not clear whether this is another typo
> or voluntary.

Ah, I see. It's a typo ☹. However, based on the discussion below, it seems 
we might not need the 'stacked-memories' property anymore.

> 
> > While looking into your mtdconcat patch [1], I noticed that it creates
> > a virtual MTD device that points to partitions on two different flash
> > nodes, which aligns perfectly with our requirements.
> >
> > However, there are two key concerns that, if addressed, could make
> > this patch suitable for the stacked mode:
> >
> > 1/ The creation of a virtual device that does not have a physical
> > existence.
> 
> We do already have:
> - the master mtd device (disabled by default for historical reasons, but
>   can be enabled with a Kconfig option).
> - an mtd device per partition
> 
> I don't see a problem in creating virtual mtd devices in the kernel.
> 
> > 2/ The creation of individual MTD devices that are concatenated to
> > form the virtual MTD device, which may not be needed by the user.
> 
> You can also get rid of them by default (or perhaps do the opposite and let a Kconfig
> option for that).

Ok, great! I'll look into it and try to integrate it into your 
previous patch [1]. If everything works out, then IMHO this(patch[1] + the 
above changes) should suffice for adding stacked support.


Regards,
Amit
> 
> > Regarding the first point, I currently cannot think of a better
> > generic way to support the stacked feature than creating a virtual device.
> > Please let me know you thoughts on this.
> >
> > For the second point, one possible solution is to hide the individual
> > MTD devices (that form the concatenated virtual MTD device) from the
> > user once the virtual device is created. Please let us know if you
> > have any other suggestions to address this issue.
> 
> That is what is done with the master device by default.
> 
> > [1]
> > https://lore.kernel.org/linux-mtd/20191127105522.31445-5-miquel.raynal
> > @bootlin.com/
> 
> Thanks,
> Miquèl