[RFC PATCH 0/2] Add support for stacked and parallel memories

[RFC PATCH 0/2] Add support for stacked and parallel memories

[Amit Kumar Mahapatra]

Hello Everyone,

Following an email discussion with Miquel regarding the binding changes
and overall architecture for implementing support for stacked and parallel
memories, I’m sharing this RFC to initiate a discussion on the proposed
updates to current bindings and to finalize the implementation
architecture.

Before diving into the main topic, here is some background on stacked and
parallel memories.

The AMD QSPI controller supports two advanced connection modes(Stacked and
Parallel) which allow the controller to treat two different flashes as one
storage.

Stacked:
Flashes share the same SPI bus, but different CS line, controller driver
asserts the CS of the flash to which it needs to communicate. Stacked mode
is a software abstraction rather than a controller feature or capability.
At any given time, the controller communicates with one of the two
connected flash devices, as determined by the requested address and data
length. If an operation starts on one flash and ends on the other, the
core needs to split it into two separate operations and adjust the data
length accordingly.

Parallel(Multi-CS):
Both the flashes have their separate SPI bus, CS of both the flashes will
be asserted/de-asserted at the same time. In this mode data will be split
across both the flashes by enabling the STRIPE setting in the controller.
Parallel mode is a controller feature where if the STRIPE bit is set then
the controller internally handles the data split during data write to the
flashes and while reading data from the flash the controller internally
merges data from both the flashes before writing to the controller FIFO.
If STRIPE is not enabled, then same data will be sent to both the devices.
In parallel mode both the flashes should be identical.

For more information on the modes please feel free to go through the
controller flash interface below [1].

Mirochip QSPI controller[2] also supports "Dual Parallel 8-bit IO mode",
but they call it "Twin Quad Mode".

Initially in [3] [4] [5] Miquel had tried to extend MTD-CONCAT driver to
support Stacked mode, but the bindings were not accepted. So, the
MTD-CONCAT approach was dropped and the DT bindings that got accepted
[6] [7] [8] that describes the two flash devices as being one. SPI core
changes to support the above bindings were added [9]. While adding the
support in SPI-NOR  Tudor provided additional feedback, leading to a
discussion on updating the current stacked and parallel DT bindings.

Proposed Solution:
The solution has two parts:

1. Update MTD-CONCAT
   Update MTD-CONCAT to create virtual concatinated mtd devices as defined
   in the device tree.

2. Add a New Layer
   Add a new layer between the SPI-NOR and MTD layers to support stacked
   and parallel configurations. This new layer will be part of spi-nor,
   located in mtd/spi-nor/, can be included/excluded via Kconfig, will be
   maintained by AMD and will:

   - During probing, store information from all connected flashes in
     stacked or parallel mode and present them as a single device to the
	 MTD layer.
   - Register callbacks and manage MTD device registration within the new
     layer instead of spi-nor/core.c.
   - Make minimal changes in spi-nor/core.c, as stacked and parallel
     handling will be managed by the new layer on top of SPI-NOR.
   - Handle odd byte count requests from the MTD layer during flash
     operations in parallel mode.

[1] https://docs.amd.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Device-Interface
[2] https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA7G5-Series-Data-Sheet-DS60001765.pdf
[3] https://lore.kernel.org/all/20191113171505.26128-4-miquel.raynal@bootlin.com/
[4] https://lore.kernel.org/all/20191127105522.31445-5-miquel.raynal@bootlin.com/
[5]https://lore.kernel.org/all/20211112152411.818321-1-miquel.raynal@bootlin.com/
[6] https://github.com/torvalds/linux/commit/f89504300e94524d5d5846ff8b728592ac72cec4
[7] https://github.com/torvalds/linux/commit/eba5368503b4291db7819512600fa014ea17c5a8
[8] https://github.com/torvalds/linux/commit/e2edd1b64f1c79e8abda365149ed62a2a9a494b4
[9]https://github.com/torvalds/linux/commit/4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b

Thanks,
Amit

Amit Kumar Mahapatra (2):
  dt-bindings: mtd: Add bindings for describing concatinated MTD devices
  dt-bindings: spi: Update stacked and parallel bindings

 .../mtd/partitions/fixed-partitions.yaml      | 18 +++++++++++++++
 .../bindings/mtd/partitions/partitions.yaml   |  6 +++++
 .../bindings/spi/spi-controller.yaml          | 23 +++++++++++++++++--
 .../bindings/spi/spi-peripheral-props.yaml    |  9 +++-----
 4 files changed, 48 insertions(+), 8 deletions(-)

-- 
2.34.1

[RFC PATCH 1/2] dt-bindings: mtd: Add bindings for describing concatinated MTD devices

[Amit Kumar Mahapatra]

This approach was suggested by Rob [1] during a discussion on Miquel's
initial approach [2] to extend the MTD-CONCAT driver to support stacked
memories.
Define each flash node separately with its respective partitions, and add
a 'concat-parts' binding to link the partitions of the two flash nodes that
need to be concatenated.

flash@0 {
        compatible = "jedec,spi-nor"
        ...
                partitions {
                compatible = "fixed-partitions";
                        concat-partition = <&flash0_partition &flash1_partition>;
                        flash0_partition: partition@0 {
                                label = "part0_0";
                                reg = <0x0 0x800000>;
                        }
                }
}
flash@1 {
        compatible = "jedec,spi-nor"
        ...
                partitions {
                compatible = "fixed-partitions";
                        concat-partition = <&flash0_partition &flash1_partition>;
                        flash1_partition: partition@0 {
                                label = "part0_1";
                                reg = <0x0 0x800000>;
                        }
                }
}

Based on the bindings the MTD-CONCAT driver need to be updated to create
virtual mtd-concat devices.

[1] https://lore.kernel.org/all/20191118221341.GA30937@bogus/
[2] https://lore.kernel.org/all/20191113171505.26128-4-miquel.raynal@bootlin.com/

Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
---
 .../mtd/partitions/fixed-partitions.yaml       | 18 ++++++++++++++++++
 .../bindings/mtd/partitions/partitions.yaml    |  6 ++++++
 2 files changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
index 058253d6d889..df4ccb3dfeba 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
@@ -183,3 +183,21 @@ examples:
             read-only;
         };
     };
+
+  - |
+    partitions {
+        compatible = "fixed-partitions";
+        #address-cells = <1>;
+        #size-cells = <1>;
+        concat-parts = <&part0 &part1>;
+
+        part0: partition@0 {
+            label = "flash0-part0";
+            reg = <0x0000000 0x100000>;
+        };
+
+        part1: partition@100000 {
+            label = "flash1-part0";
+            reg = <0x0100000 0x200000>;
+        };
+    };
diff --git a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
index 1dda2c80747b..86bbd83c3f6d 100644
--- a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
+++ b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
@@ -32,6 +32,12 @@ properties:
   '#size-cells':
     enum: [1, 2]
 
+  concat-parts:
+    description: List of MTD partitions phandles that should be concatenated.
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    minItems: 2
+    maxItems: 4
+
 patternProperties:
   "^partition(-.+|@[0-9a-f]+)$":
     $ref: partition.yaml
-- 
2.34.1

Re: [RFC PATCH 1/2] dt-bindings: mtd: Add bindings for describing concatinated MTD devices

[Krzysztof Kozlowski]

On 26/10/2024 09:53, Amit Kumar Mahapatra wrote:
> This approach was suggested by Rob [1] during a discussion on Miquel's
> initial approach [2] to extend the MTD-CONCAT driver to support stacked
> memories.
> Define each flash node separately with its respective partitions, and add
> a 'concat-parts' binding to link the partitions of the two flash nodes that
> need to be concatenated.

I understand this was not sent to proper addresses for review because it
is a RFC. It's fine then.

If this was not the intention and this should be reviewed (and tested,
although I assume you tested these patches first), then please read the
standard form bellow:

<form letter>
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.
</form letter>

Best regards,
Krzysztof

RE: [RFC PATCH 1/2] dt-bindings: mtd: Add bindings for describing concatinated MTD devices

[Mahapatra, Amit Kumar]

Hello Krzysztof

> > This approach was suggested by Rob [1] during a discussion on Miquel's
> > initial approach [2] to extend the MTD-CONCAT driver to support
> > stacked memories.
> > Define each flash node separately with its respective partitions, and
> > add a 'concat-parts' binding to link the partitions of the two flash
> > nodes that need to be concatenated.
> 
> I understand this was not sent to proper addresses for review because it is a RFC.

Yes, that’s correct.

Regards,
Amit

> It's fine then.
> 
> If this was not the intention and this should be reviewed (and tested, although I
> assume you tested these patches first), then please read the standard form bellow:
> 
> <form letter>
> Please use scripts/get_maintainers.pl to get a list of necessary people and lists to
> CC. It might happen, that command when run on an older kernel, gives you outdated
> entries. Therefore please be sure you base your patches on recent Linux kernel.
> 
> Tools like b4 or scripts/get_maintainer.pl provide you proper list of people, so fix your
> workflow. Tools might also fail if you work on some ancient tree (don't, instead use
> mainline) or work on fork of kernel (don't, instead use mainline). Just use b4 and
> everything should be fine, although remember about `b4 prep --auto-to-cc` if you
> added new patches to the patchset.
> 
> You missed at least devicetree list (maybe more), so this won't be tested by
> automated tooling. Performing review on untested code might be a waste of time.
> 
> Please kindly resend and include all necessary To/Cc entries.
> </form letter>
> 
> Best regards,
> Krzysztof

Re: [RFC PATCH 1/2] dt-bindings: mtd: Add bindings for describing concatinated MTD devices

[Miquel Raynal]

On 26/10/2024 at 13:23:46 +0530, Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> wrote:

> This approach was suggested by Rob [1] during a discussion on Miquel's
> initial approach [2] to extend the MTD-CONCAT driver to support stacked
> memories.
> Define each flash node separately with its respective partitions, and add
> a 'concat-parts' binding to link the partitions of the two flash nodes that
> need to be concatenated.
>
> flash@0 {
>         compatible = "jedec,spi-nor"
>         ...
>                 partitions {

Wrong indentation here and below which makes the example hard to read.

>                 compatible = "fixed-partitions";
>                         concat-partition = <&flash0_partition &flash1_partition>;
>                         flash0_partition: partition@0 {
>                                 label = "part0_0";
>                                 reg = <0x0 0x800000>;
>                         }
>                 }
> }
> flash@1 {
>         compatible = "jedec,spi-nor"
>         ...
>                 partitions {
>                 compatible = "fixed-partitions";
>                         concat-partition = <&flash0_partition &flash1_partition>;
>                         flash1_partition: partition@0 {
>                                 label = "part0_1";
>                                 reg = <0x0 0x800000>;
>                         }
>                 }
> }

This approach has a limitation I didn't think about before: you cannot
use anything else than fixed partitions as partition parser.

> Based on the bindings the MTD-CONCAT driver need to be updated to create
> virtual mtd-concat devices.
>
> [1] https://lore.kernel.org/all/20191118221341.GA30937@bogus/
> [2] https://lore.kernel.org/all/20191113171505.26128-4-miquel.raynal@bootlin.com/
>
> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
> ---
>  .../mtd/partitions/fixed-partitions.yaml       | 18 ++++++++++++++++++
>  .../bindings/mtd/partitions/partitions.yaml    |  6 ++++++
>  2 files changed, 24 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> index 058253d6d889..df4ccb3dfeba 100644
> --- a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml
> @@ -183,3 +183,21 @@ examples:
>              read-only;
>          };
>      };
> +
> +  - |
> +    partitions {
> +        compatible = "fixed-partitions";
> +        #address-cells = <1>;
> +        #size-cells = <1>;

This is not strictly related but I believe we will soon have issues with
these, as we will soon cross the 4GiB boundary.

> +        concat-parts = <&part0 &part1>;
> +
> +        part0: partition@0 {
> +            label = "flash0-part0";
> +            reg = <0x0000000 0x100000>;
> +        };
> +
> +        part1: partition@100000 {
> +            label = "flash1-part0";
> +            reg = <0x0100000 0x200000>;
> +        };
> +    };
> diff --git a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> index 1dda2c80747b..86bbd83c3f6d 100644
> --- a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> +++ b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> @@ -32,6 +32,12 @@ properties:
>    '#size-cells':
>      enum: [1, 2]
>  
> +  concat-parts:
> +    description: List of MTD partitions phandles that should be concatenated.
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    minItems: 2
> +    maxItems: 4
> +
>  patternProperties:
>    "^partition(-.+|@[0-9a-f]+)$":
>      $ref: partition.yaml

Fine by me otherwise.

Thanks,
Miquèl

RE: [RFC PATCH 1/2] dt-bindings: mtd: Add bindings for describing concatinated MTD devices

[Mahapatra, Amit Kumar]

Hello Miquel,
 
> > This approach was suggested by Rob [1] during a discussion on Miquel's
> > initial approach [2] to extend the MTD-CONCAT driver to support
> > stacked memories.
> > Define each flash node separately with its respective partitions, and
> > add a 'concat-parts' binding to link the partitions of the two flash
> > nodes that need to be concatenated.
> >
> > flash@0 {
> >         compatible = "jedec,spi-nor"
> >         ...
> >                 partitions {
> 
> Wrong indentation here and below which makes the example hard to read.

Sorry about that. I am redefining both the flash nodes here with proper 
indentation.

flash@0 {
	compatible = "jedec,spi-nor"
	...
	partitions {
		compatible = "fixed-partitions";
		concat-partition = <&flash0_partition &flash1_partition>;
		
		flash0_partition: partition@0 {
			label = "part0_0";
			reg = <0x0 0x800000>;
		};
	};
};

flash@1 {
	compatible = "jedec,spi-nor"
	...
	partitions {
		compatible = "fixed-partitions";
		concat-partition = <&flash0_partition &flash1_partition>;
                        
		flash1_partition: partition@0 {
			label = "part0_1";
			reg = <0x0 0x800000>;
		};
	};
};

> 
> >                 compatible = "fixed-partitions";
> >                         concat-partition = <&flash0_partition &flash1_partition>;
> >                         flash0_partition: partition@0 {
> >                                 label = "part0_0";
> >                                 reg = <0x0 0x800000>;
> >                         }
> >                 }
> > }
> > flash@1 {
> >         compatible = "jedec,spi-nor"
> >         ...
> >                 partitions {
> >                 compatible = "fixed-partitions";
> >                         concat-partition = <&flash0_partition &flash1_partition>;
> >                         flash1_partition: partition@0 {
> >                                 label = "part0_1";
> >                                 reg = <0x0 0x800000>;
> >                         }
> >                 }
> > }
> 
> This approach has a limitation I didn't think about before: you cannot use anything
> else than fixed partitions as partition parser.

Yes, that's correct—it won't function when partitions are defined via the 
command line. In my opinion, we should start by adding support for fixed 
partitions, add comments in code stating the same. If needed, we can later 
extend the support to dynamic partitions as well.

Regards,
Amit

> 
> > Based on the bindings the MTD-CONCAT driver need to be updated to
> > create virtual mtd-concat devices.
> >
> > [1] https://lore.kernel.org/all/20191118221341.GA30937@bogus/
> > [2]
> > https://lore.kernel.org/all/20191113171505.26128-4-miquel.raynal@bootl
> > in.com/
> >
> > Signed-off-by: Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com>
> > ---
> >  .../mtd/partitions/fixed-partitions.yaml       | 18 ++++++++++++++++++
> >  .../bindings/mtd/partitions/partitions.yaml    |  6 ++++++
> >  2 files changed, 24 insertions(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.ya
> > ml
> > b/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.ya
> > ml index 058253d6d889..df4ccb3dfeba 100644
> > ---
> > a/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.ya
> > ml
> > +++ b/Documentation/devicetree/bindings/mtd/partitions/fixed-partition
> > +++ s.yaml
> > @@ -183,3 +183,21 @@ examples:
> >              read-only;
> >          };
> >      };
> > +
> > +  - |
> > +    partitions {
> > +        compatible = "fixed-partitions";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> 
> This is not strictly related but I believe we will soon have issues with these, as we
> will soon cross the 4GiB boundary.
> 
> > +        concat-parts = <&part0 &part1>;
> > +
> > +        part0: partition@0 {
> > +            label = "flash0-part0";
> > +            reg = <0x0000000 0x100000>;
> > +        };
> > +
> > +        part1: partition@100000 {
> > +            label = "flash1-part0";
> > +            reg = <0x0100000 0x200000>;
> > +        };
> > +    };
> > diff --git
> > a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> > b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> > index 1dda2c80747b..86bbd83c3f6d 100644
> > --- a/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> > +++ b/Documentation/devicetree/bindings/mtd/partitions/partitions.yaml
> > @@ -32,6 +32,12 @@ properties:
> >    '#size-cells':
> >      enum: [1, 2]
> >
> > +  concat-parts:
> > +    description: List of MTD partitions phandles that should be concatenated.
> > +    $ref: /schemas/types.yaml#/definitions/phandle-array
> > +    minItems: 2
> > +    maxItems: 4
> > +
> >  patternProperties:
> >    "^partition(-.+|@[0-9a-f]+)$":
> >      $ref: partition.yaml
> 
> Fine by me otherwise.
> 
> Thanks,
> Miquèl

Re: [RFC PATCH 1/2] dt-bindings: mtd: Add bindings for describing concatinated MTD devices

[Miquel Raynal]

On 19/11/2024 at 17:02:33 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com> wrote:

> Hello Miquel,
>  
>> > This approach was suggested by Rob [1] during a discussion on Miquel's
>> > initial approach [2] to extend the MTD-CONCAT driver to support
>> > stacked memories.
>> > Define each flash node separately with its respective partitions, and
>> > add a 'concat-parts' binding to link the partitions of the two flash
>> > nodes that need to be concatenated.
>> >
>> > flash@0 {
>> >         compatible = "jedec,spi-nor"
>> >         ...
>> >                 partitions {
>> 
>> Wrong indentation here and below which makes the example hard to read.
>
> Sorry about that. I am redefining both the flash nodes here with proper 
> indentation.
>
> flash@0 {
> 	compatible = "jedec,spi-nor"
> 	...
> 	partitions {
> 		compatible = "fixed-partitions";
> 		concat-partition = <&flash0_partition &flash1_partition>;
> 		
> 		flash0_partition: partition@0 {
> 			label = "part0_0";
> 			reg = <0x0 0x800000>;
> 		};
> 	};
> };
>
> flash@1 {
> 	compatible = "jedec,spi-nor"
> 	...
> 	partitions {
> 		compatible = "fixed-partitions";
> 		concat-partition = <&flash0_partition &flash1_partition>;
>                         
> 		flash1_partition: partition@0 {
> 			label = "part0_1";
> 			reg = <0x0 0x800000>;
> 		};
> 	};
> };
>
>> 
>> >                 compatible = "fixed-partitions";
>> >                         concat-partition = <&flash0_partition &flash1_partition>;
>> >                         flash0_partition: partition@0 {
>> >                                 label = "part0_0";
>> >                                 reg = <0x0 0x800000>;
>> >                         }
>> >                 }
>> > }
>> > flash@1 {
>> >         compatible = "jedec,spi-nor"
>> >         ...
>> >                 partitions {
>> >                 compatible = "fixed-partitions";
>> >                         concat-partition = <&flash0_partition &flash1_partition>;
>> >                         flash1_partition: partition@0 {
>> >                                 label = "part0_1";
>> >                                 reg = <0x0 0x800000>;
>> >                         }
>> >                 }
>> > }
>> 
>> This approach has a limitation I didn't think about before: you cannot use anything
>> else than fixed partitions as partition parser.
>
> Yes, that's correct—it won't function when partitions are defined via the 
> command line. In my opinion, we should start by adding support for fixed 
> partitions, add comments in code stating the same. If needed, we can later 
> extend the support to dynamic partitions as well.

New thought. What if it was a pure fixed-partition capability? That's
actually what we want: defining fixed partitions through device
boundaries. It automatically removes the need for further dynamic
partition extensions.

Thanks,
Miquèl

RE: [RFC PATCH 1/2] dt-bindings: mtd: Add bindings for describing concatinated MTD devices

[Mahapatra, Amit Kumar]

> > Sorry about that. I am redefining both the flash nodes here with
> > proper indentation.
> >
> > flash@0 {
> > 	compatible = "jedec,spi-nor"
> > 	...
> > 	partitions {
> > 		compatible = "fixed-partitions";
> > 		concat-partition = <&flash0_partition &flash1_partition>;
> >
> > 		flash0_partition: partition@0 {
> > 			label = "part0_0";
> > 			reg = <0x0 0x800000>;
> > 		};
> > 	};
> > };
> >
> > flash@1 {
> > 	compatible = "jedec,spi-nor"
> > 	...
> > 	partitions {
> > 		compatible = "fixed-partitions";
> > 		concat-partition = <&flash0_partition &flash1_partition>;
> >
> > 		flash1_partition: partition@0 {
> > 			label = "part0_1";
> > 			reg = <0x0 0x800000>;
> > 		};
> > 	};
> > };
> >
> >>
> >> >                 compatible = "fixed-partitions";
> >> >                         concat-partition = <&flash0_partition &flash1_partition>;
> >> >                         flash0_partition: partition@0 {
> >> >                                 label = "part0_0";
> >> >                                 reg = <0x0 0x800000>;
> >> >                         }
> >> >                 }
> >> > }
> >> > flash@1 {
> >> >         compatible = "jedec,spi-nor"
> >> >         ...
> >> >                 partitions {
> >> >                 compatible = "fixed-partitions";
> >> >                         concat-partition = <&flash0_partition &flash1_partition>;
> >> >                         flash1_partition: partition@0 {
> >> >                                 label = "part0_1";
> >> >                                 reg = <0x0 0x800000>;
> >> >                         }
> >> >                 }
> >> > }
> >>
> >> This approach has a limitation I didn't think about before: you
> >> cannot use anything else than fixed partitions as partition parser.
> >
> > Yes, that's correct—it won't function when partitions are defined via
> > the command line. In my opinion, we should start by adding support for
> > fixed partitions, add comments in code stating the same. If needed, we
> > can later extend the support to dynamic partitions as well.
> 
> New thought. What if it was a pure fixed-partition capability? That's actually what we

Yes, I agree—it’s better to present it as a purely fixed-partition capability.


Regards,
Amit
> want: defining fixed partitions through device boundaries. It automatically removes
> the need for further dynamic partition extensions.
> 
> Thanks,
> Miquèl

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

[Amit Kumar Mahapatra]

For implementing the proposed solution the current 'stacked-memories' &
'parallel-memories' bindings need to be updated as follow.

stacked-memories binding changes:
- Each 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.
- Remove the size information from the bindings as it can be retrived
  drirectly from the flash.
- The stacked-memories DT bindings will contain the phandles of the flash
  nodes connected in stacked mode.

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.

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_partition &flash1_partition>;
                                flash0_partition: 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_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.

spi@1 {
        ...
        flash@3 {
                compatible = "jedec,spi-nor"
                reg = <0x00 0x01>;
                paralle-memories ;
                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>;
         };
+
     };
+
+  - |
+    spi@90010000 {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        compatible = "fsl,imx28-spi";
+        reg = <0x90010000 0x2000>;
+        interrupts = <96>;
+        dmas = <&dma_apbh 0>;
+        dma-names = "rx-tx";
+
+        flash@0 {
+            compatible = "jedec,spi-nor";
+            spi-max-frequency = <50000000>;
+            reg = <0>, <1>;
+            parallel-memories;
+        };
+      };
diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 15938f81fdce..2a014160d701 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -96,7 +96,7 @@ properties:
       space with only a single additional wire, while still needing
       to repeat the commands when crossing a chip boundary. The size of
       each chip should be provided as members of the array.
-    $ref: /schemas/types.yaml#/definitions/uint64-array
+    $ref: /schemas/types.yaml#/definitions/phandle-array
     minItems: 2
     maxItems: 4
 
@@ -107,11 +107,8 @@ properties:
       different memories (eg. even bits are stored in one memory, odd
       bits in the other). This basically doubles the address space and
       the throughput while greatly complexifying the wiring because as
-      many busses as devices must be wired. The size of each chip should
-      be provided as members of the array.
-    $ref: /schemas/types.yaml#/definitions/uint64-array
-    minItems: 2
-    maxItems: 4
+      many busses as devices must be wired.
+    $ref: /schemas/types.yaml#/definitions/flag
 
   st,spi-midi-ns:
     description: |
-- 
2.34.1

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

[Miquel Raynal]

Hi Amit,

On 26/10/2024 at 13:23:47 +0530, Amit Kumar Mahapatra <amit.kumar-mahapatra@amd.com> wrote:

> For implementing the proposed solution the current 'stacked-memories' &
> 'parallel-memories' bindings need to be updated as follow.
>
> stacked-memories binding changes:
> - Each 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.
> - Remove the size information from the bindings as it can be retrived
>   drirectly from the flash.
> - The stacked-memories DT bindings will contain the phandles of the flash
>   nodes connected in stacked mode.
>
> 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.
>
> 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_partition &flash1_partition>;
>                                 flash0_partition: 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 {

Same comment as before here.

>                         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 mechanism on top of a virtual mtd
device but, heh, you're now describing a virtual mtd device, which is
not a physical device as it "should" be.

Thanks,
Miquèl

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

[Mahapatra, Amit Kumar]

Hello Miquel,

> >         flash@1 {
> >                 compatible = "jedec,spi-nor"
> >                 reg = <0x01>;
> >                 stacked-memories = <&flash@0 &flash@1>;
> >                 spi-max-frequency = <50000000>;
> >                 ...
> >                         partitions {
> 
> Same comment as before here.

Sorry again 

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.

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.

2/ The creation of individual MTD devices that are concatenated to form 
the virtual MTD device, which may not be needed by the user.

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.

[1] https://lore.kernel.org/linux-mtd/20191127105522.31445-5-miquel.raynal@bootlin.com/ 

Regards,
Amit

> mechanism on top of a virtual mtd device but, heh, you're now describing a virtual
> mtd device, which is not a physical device as it "should" be.
> 
> Thanks,
> Miquèl

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

[Miquel Raynal]

On 19/11/2024 at 17:02:45 GMT, "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@amd.com> wrote:

> Hello Miquel,
>
>> >         flash@1 {
>> >                 compatible = "jedec,spi-nor"
>> >                 reg = <0x01>;
>> >                 stacked-memories = <&flash@0 &flash@1>;
>> >                 spi-max-frequency = <50000000>;
>> >                 ...
>> >                         partitions {
>> 
>> Same comment as before here.
>
> Sorry again 
>
> 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.

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

> 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

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

[Mahapatra, Amit Kumar]

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

RE: [RFC PATCH 0/2] Add support for stacked and parallel memories

[Mahapatra, Amit Kumar]

Hello,

> > Stacked:
> > Flashes share the same SPI bus, but different CS line, controller
> > driver asserts the CS of the flash to which it needs to communicate.
> > Stacked mode is a software abstraction rather than a controller feature or
> capability.
> > At any given time, the controller communicates with one of the two
> > connected flash devices, as determined by the requested address and
> > data length. If an operation starts on one flash and ends on the
> > other, the core needs to split it into two separate operations and
> > adjust the data length accordingly.
> >
> > Parallel(Multi-CS):
> > Both the flashes have their separate SPI bus, CS of both the flashes
> > will be asserted/de-asserted at the same time. In this mode data will
> > be split across both the flashes by enabling the STRIPE setting in the controller.
> > Parallel mode is a controller feature where if the STRIPE bit is set
> > then the controller internally handles the data split during data
> > write to the flashes and while reading data from the flash the
> > controller internally merges data from both the flashes before writing to the
> controller FIFO.
> > If STRIPE is not enabled, then same data will be sent to both the devices.
> > In parallel mode both the flashes should be identical.
> 
> Interesting.
> 
> What's the practical use case ? Some kind of RAID directly on raw flashes ? Could it

In a parallel configuration, the user can double the flash capacity 
and data throughput.

Users disable STRIPE when they need to access the flash device 
registers for reading or writing.
 
> help for protecting from a broken boot flash ?

No, because each read/write operation communicates with both flash 
devices simultaneously, meaning the image will be distributed across 
both flashes and read from both during boot.

Regards,
Amit

> 
> 
> --mtx
> 
> --
> ---
> Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert werden !
> Für eine vertrauliche Kommunikation senden Sie bitte ihren GPG/PGP-Schlüssel zu.
> ---
> Enrico Weigelt, metux IT consult
> Free software and Linux embedded engineering info@metux.net -- +49-151-
> 27565287

RE: [RFC PATCH 0/2] Add support for stacked and parallel memories

[Mahapatra, Amit Kumar]

Hello Tudor, Michael, Mark, Rob, Pratyush & Krzysztof

Please share your thoughts on the proposed binding changes and overall 
architecture, so we can discuss and finalize the same.

Regards,
Amit

> Subject: [RFC PATCH 0/2] Add support for stacked and parallel memories
> 
> Hello Everyone,
> 
> Following an email discussion with Miquel regarding the binding changes and overall
> architecture for implementing support for stacked and parallel memories, I’m sharing
> this RFC to initiate a discussion on the proposed updates to current bindings and to
> finalize the implementation architecture.
> 
> Before diving into the main topic, here is some background on stacked and parallel
> memories.
> 
> The AMD QSPI controller supports two advanced connection modes(Stacked and
> Parallel) which allow the controller to treat two different flashes as one storage.
> 
> Stacked:
> Flashes share the same SPI bus, but different CS line, controller driver asserts the
> CS of the flash to which it needs to communicate. Stacked mode is a software
> abstraction rather than a controller feature or capability.
> At any given time, the controller communicates with one of the two connected flash
> devices, as determined by the requested address and data length. If an operation
> starts on one flash and ends on the other, the core needs to split it into two separate
> operations and adjust the data length accordingly.
> 
> Parallel(Multi-CS):
> Both the flashes have their separate SPI bus, CS of both the flashes will be
> asserted/de-asserted at the same time. In this mode data will be split across both the
> flashes by enabling the STRIPE setting in the controller.
> Parallel mode is a controller feature where if the STRIPE bit is set then the controller
> internally handles the data split during data write to the flashes and while reading
> data from the flash the controller internally merges data from both the flashes before
> writing to the controller FIFO.
> If STRIPE is not enabled, then same data will be sent to both the devices.
> In parallel mode both the flashes should be identical.
> 
> For more information on the modes please feel free to go through the controller flash
> interface below [1].
> 
> Mirochip QSPI controller[2] also supports "Dual Parallel 8-bit IO mode", but they call
> it "Twin Quad Mode".
> 
> Initially in [3] [4] [5] Miquel had tried to extend MTD-CONCAT driver to support
> Stacked mode, but the bindings were not accepted. So, the MTD-CONCAT
> approach was dropped and the DT bindings that got accepted [6] [7] [8] that
> describes the two flash devices as being one. SPI core changes to support the
> above bindings were added [9]. While adding the support in SPI-NOR  Tudor
> provided additional feedback, leading to a discussion on updating the current
> stacked and parallel DT bindings.
> 
> Proposed Solution:
> The solution has two parts:
> 
> 1. Update MTD-CONCAT
>    Update MTD-CONCAT to create virtual concatinated mtd devices as defined
>    in the device tree.
> 
> 2. Add a New Layer
>    Add a new layer between the SPI-NOR and MTD layers to support stacked
>    and parallel configurations. This new layer will be part of spi-nor,
>    located in mtd/spi-nor/, can be included/excluded via Kconfig, will be
>    maintained by AMD and will:
> 
>    - During probing, store information from all connected flashes in
>      stacked or parallel mode and present them as a single device to the
> 	 MTD layer.
>    - Register callbacks and manage MTD device registration within the new
>      layer instead of spi-nor/core.c.
>    - Make minimal changes in spi-nor/core.c, as stacked and parallel
>      handling will be managed by the new layer on top of SPI-NOR.
>    - Handle odd byte count requests from the MTD layer during flash
>      operations in parallel mode.
> 
> [1] https://docs.amd.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Device-
> Interface
> [2]
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductD
> ocuments/DataSheets/SAMA7G5-Series-Data-Sheet-DS60001765.pdf
> [3] https://lore.kernel.org/all/20191113171505.26128-4-miquel.raynal@bootlin.com/
> [4] https://lore.kernel.org/all/20191127105522.31445-5-miquel.raynal@bootlin.com/
> [5]https://lore.kernel.org/all/20211112152411.818321-1-miquel.raynal@bootlin.com/
> [6]
> https://github.com/torvalds/linux/commit/f89504300e94524d5d5846ff8b728592ac72c
> ec4
> [7]
> https://github.com/torvalds/linux/commit/eba5368503b4291db7819512600fa014ea17
> c5a8
> [8]
> https://github.com/torvalds/linux/commit/e2edd1b64f1c79e8abda365149ed62a2a9a4
> 94b4
> [9]https://github.com/torvalds/linux/commit/4d8ff6b0991d5e86b17b235fc46ec62e919
> 5cb9b
> 
> Thanks,
> Amit
> 
> Amit Kumar Mahapatra (2):
>   dt-bindings: mtd: Add bindings for describing concatinated MTD devices
>   dt-bindings: spi: Update stacked and parallel bindings
> 
>  .../mtd/partitions/fixed-partitions.yaml      | 18 +++++++++++++++
>  .../bindings/mtd/partitions/partitions.yaml   |  6 +++++
>  .../bindings/spi/spi-controller.yaml          | 23 +++++++++++++++++--
>  .../bindings/spi/spi-peripheral-props.yaml    |  9 +++-----
>  4 files changed, 48 insertions(+), 8 deletions(-)
> 
> --
> 2.34.1

Re: [RFC PATCH 0/2] Add support for stacked and parallel memories

[Pratyush Yadav]

On Sat, Oct 26 2024, Amit Kumar Mahapatra wrote:

Hi Amit,

I've been meaning to look into this proposal for some time now, but one
thing or another kept coming up and I never got around to it. Well, I'll
try to get some of my thoughts out with this reply. I still haven't
looked very deeply into the past discussions, so apologies if I bring up
something that was already discussed.

> Hello Everyone,
>
> Following an email discussion with Miquel regarding the binding changes
> and overall architecture for implementing support for stacked and parallel
> memories, I’m sharing this RFC to initiate a discussion on the proposed
> updates to current bindings and to finalize the implementation
> architecture.
>
> Before diving into the main topic, here is some background on stacked and
> parallel memories.
>
> The AMD QSPI controller supports two advanced connection modes(Stacked and
> Parallel) which allow the controller to treat two different flashes as one
> storage.
>
> Stacked:
> Flashes share the same SPI bus, but different CS line, controller driver
> asserts the CS of the flash to which it needs to communicate. Stacked mode
> is a software abstraction rather than a controller feature or capability.
> At any given time, the controller communicates with one of the two
> connected flash devices, as determined by the requested address and data
> length. If an operation starts on one flash and ends on the other, the
> core needs to split it into two separate operations and adjust the data
> length accordingly.
>
> Parallel(Multi-CS):
> Both the flashes have their separate SPI bus, CS of both the flashes will
> be asserted/de-asserted at the same time. In this mode data will be split
> across both the flashes by enabling the STRIPE setting in the controller.
> Parallel mode is a controller feature where if the STRIPE bit is set then
> the controller internally handles the data split during data write to the
> flashes and while reading data from the flash the controller internally
> merges data from both the flashes before writing to the controller FIFO.
> If STRIPE is not enabled, then same data will be sent to both the devices.
> In parallel mode both the flashes should be identical.
>
> For more information on the modes please feel free to go through the
> controller flash interface below [1].
>
> Mirochip QSPI controller[2] also supports "Dual Parallel 8-bit IO mode",
> but they call it "Twin Quad Mode".
>
> Initially in [3] [4] [5] Miquel had tried to extend MTD-CONCAT driver to
> support Stacked mode, but the bindings were not accepted. So, the
> MTD-CONCAT approach was dropped and the DT bindings that got accepted
> [6] [7] [8] that describes the two flash devices as being one. SPI core
> changes to support the above bindings were added [9]. While adding the
> support in SPI-NOR  Tudor provided additional feedback, leading to a
> discussion on updating the current stacked and parallel DT bindings.
>
> Proposed Solution:
> The solution has two parts:
>
> 1. Update MTD-CONCAT
>    Update MTD-CONCAT to create virtual concatinated mtd devices as defined
>    in the device tree.

From a very quick look, it seems mtdconcat should already do most of
what you want with "stacked mode". The tricky bits might be devicetree
design, but from the software perspective, I think mtdconcat makes
perfect sense. You leave all the complexity to the SPI NOR layer since
it already handles them, and just use the higher-level MTD layer to
virtually concatenate devices. Adding a new layer between MTD and SPI
NOR makes little sense because mtdconcat is already that layer. Another
benefit of this would be you can just as easily concatenate any kinds of
flashes you want; they don't have to be the same.

I think this is a much simpler problem to solve compared to parallel
mode. Once you figure out the devicetree stuff, and I think the
mtdconcat changes should be simple and not too controversial. So I think
you should split the parallel and stacked support into two independent
series. This way you make progress without having to wait for
discussions around parallel mode support to settle.

>
> 2. Add a New Layer
>    Add a new layer between the SPI-NOR and MTD layers to support stacked
>    and parallel configurations. This new layer will be part of spi-nor,
>    located in mtd/spi-nor/, can be included/excluded via Kconfig, will be
>    maintained by AMD and will:
>
>    - During probing, store information from all connected flashes in
>      stacked or parallel mode and present them as a single device to the
> 	 MTD layer.

As I mentioned above, I do not think you should do stacked flashes this
way.

>    - Register callbacks and manage MTD device registration within the new
>      layer instead of spi-nor/core.c.
>    - Make minimal changes in spi-nor/core.c, as stacked and parallel
>      handling will be managed by the new layer on top of SPI-NOR.
>    - Handle odd byte count requests from the MTD layer during flash
>      operations in parallel mode.

You'd also probably have to add support in SPI MEM to signal the
controller to use parallel mode. You don't want to use parallel mode all
the time since you'd need to do "normal" operations as well such as
reading/writing status registers, reading flash ID, SFDP, etc. That is
yet another "cost" parallel mode support has -- it adds another thing to
SPI MEM (I'm not saying the cost isn't necessarily worth it -- just
pointing it out).

From the SPI NOR side, this layer would have to make sure both flashes
get configured with the exact same settings. That would need SPI NOR to
export how it configured a flash, ideally in a stable, well-defined
format. It would also have to ask SPI NOR to construct the SPI MEM ops
for it, and then edit them to set the "parallel mode" bit. Perhaps the
dirmap op templates can come in handy here.

It's hard to say much more without seeing the code. It would be
interesting to see how you can manage to get this layer work without too
much intrusion in the core.

[...]

-- 
Regards,
Pratyush Yadav