[PATCH v4 0/3] Stacked/parallel memories bindings

[PATCH v4 0/3] Stacked/parallel memories bindings

[Miquel Raynal]

Hello Rob, Mark, Tudor & Pratyush,

Here is a fourth versions for these bindings, which applies on top of
Pratyush's work:
https://lore.kernel.org/all/20211109181911.2251-1-p.yadav@ti.com/

Cheers,
Miquèl

Changes in v4:
* Changed the type of properties to uint64-arrays in order to be able to
  describe the size of each element in the array.
* Updated the example accordingly.

Changes in v3:
* Rebased on top of Pratyush's recent changes.
* Dropped the commit allowing to provide two reg entries on the node
  name.
* Dropped the commit referencing spi-controller.yaml from
  jedec,spi-nor.yaml, now replaced by spi-peripheral-props.yaml and
  already done in Pratyush's series.
* Added Rob's Ack.
* Enhanced a commit message.
* Moved the new properties to the new SPI peripheral binding file.

Changes in v2:
* Dropped the dtc changes for now.
* Moved the properties in the device's nodes, not the controller's.
* Dropped the useless #address-cells change.
* Added a missing "minItems".
* Moved the new properties in the spi-controller.yaml file.
* Added an example using two stacked memories in the
  spi-controller.yaml file.
* Renamed the properties to drop the Xilinx prefix.
* Added a patch to fix the spi-nor jedec yaml file.

Miquel Raynal (3):
  dt-bindings: mtd: spi-nor: Allow two CS per device
  spi: dt-bindings: Describe stacked/parallel memories modes
  spi: dt-bindings: Add an example with two stacked flashes

 .../bindings/mtd/jedec,spi-nor.yaml           |  3 +-
 .../bindings/spi/spi-controller.yaml          |  7 +++++
 .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
 3 files changed, 38 insertions(+), 1 deletion(-)

-- 
2.27.0

[PATCH v4 1/3] dt-bindings: mtd: spi-nor: Allow two CS per device

[Miquel Raynal]

The Xilinx QSPI controller has two advanced modes which allow the
controller to behave differently and consider two flashes as one single
storage.

One of these two modes is quite complex to support from a binding point
of view and is the dual parallel memories. In this mode, each byte of
data is stored in both devices: the even bits in one, the odd bits in
the other. The split is automatically handled by the QSPI controller and
is transparent for the user.

The other mode is simpler to support, it is called dual stacked
memories. The controller shares the same SPI bus but each of the devices
contain half of the data. Once in this mode, the controller does not
follow CS requests but instead internally wires the two CS levels with
the value of the most significant address bit.

Supporting these two modes will involve core changes which include the
possibility of providing two CS for a single SPI device

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
index 39421f7233e4..4abfb4cfc157 100644
--- a/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
+++ b/Documentation/devicetree/bindings/mtd/jedec,spi-nor.yaml
@@ -47,7 +47,8 @@ properties:
       identified by the JEDEC READ ID opcode (0x9F).
 
   reg:
-    maxItems: 1
+    minItems: 1
+    maxItems: 2
 
   spi-max-frequency: true
   spi-rx-bus-width: true
-- 
2.27.0

[PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Miquel Raynal]

Describe two new memories modes:
- A stacked mode when the bus is common but the address space extended
  with an additinals wires.
- A parallel mode with parallel busses accessing parallel flashes where
  the data is spread.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 5dd209206e88..4194fee8f556 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -82,6 +82,35 @@ properties:
     description:
       Delay, in microseconds, after a write transfer.
 
+  stacked-memories:
+    $ref: /schemas/types.yaml#/definitions/uint64-matrix
+    description: Several SPI memories can be wired in stacked mode.
+      This basically means that either a device features several chip
+      selects, or that different devices must be seen as a single
+      bigger chip. This basically doubles (or more) the total address
+      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.
+    minItems: 2
+    maxItems: 2
+    items:
+      maxItems: 1
+
+  parallel-memories:
+    $ref: /schemas/types.yaml#/definitions/uint64-matrix
+    description: Several SPI memories can be wired in parallel mode.
+      The devices are physically on a different buses but will always
+      act synchronously as each data word is spread across the
+      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.
+    minItems: 2
+    maxItems: 2
+    items:
+      maxItems: 1
+
 # The controller specific properties go here.
 allOf:
   - $ref: cdns,qspi-nor-peripheral-props.yaml#
-- 
2.27.0

Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Rob Herring]

On Fri, Dec 10, 2021 at 09:10:38PM +0100, Miquel Raynal wrote:
> Describe two new memories modes:
> - A stacked mode when the bus is common but the address space extended
>   with an additinals wires.
> - A parallel mode with parallel busses accessing parallel flashes where
>   the data is spread.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 5dd209206e88..4194fee8f556 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -82,6 +82,35 @@ properties:
>      description:
>        Delay, in microseconds, after a write transfer.
>  
> +  stacked-memories:
> +    $ref: /schemas/types.yaml#/definitions/uint64-matrix

matrix or...

> +    description: Several SPI memories can be wired in stacked mode.
> +      This basically means that either a device features several chip
> +      selects, or that different devices must be seen as a single
> +      bigger chip. This basically doubles (or more) the total address
> +      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.

array?

Sounds like an array from the description as there is only 1 element, 
the size.

> +    minItems: 2
> +    maxItems: 2
> +    items:
> +      maxItems: 1

This says you can only have 2 64-bit entries. Probably not what you 
want. This looks like a case for a maxItems 'should be enough for now' 
type of value.

> +
> +  parallel-memories:
> +    $ref: /schemas/types.yaml#/definitions/uint64-matrix
> +    description: Several SPI memories can be wired in parallel mode.
> +      The devices are physically on a different buses but will always
> +      act synchronously as each data word is spread across the
> +      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.
> +    minItems: 2
> +    maxItems: 2
> +    items:
> +      maxItems: 1
> +
>  # The controller specific properties go here.
>  allOf:
>    - $ref: cdns,qspi-nor-peripheral-props.yaml#
> -- 
> 2.27.0
> 
> 

Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Miquel Raynal]

Hi Rob,

robh@kernel.org wrote on Tue, 14 Dec 2021 11:32:56 -0600:

> On Fri, Dec 10, 2021 at 09:10:38PM +0100, Miquel Raynal wrote:
> > Describe two new memories modes:
> > - A stacked mode when the bus is common but the address space extended
> >   with an additinals wires.
> > - A parallel mode with parallel busses accessing parallel flashes where
> >   the data is spread.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > index 5dd209206e88..4194fee8f556 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > @@ -82,6 +82,35 @@ properties:
> >      description:
> >        Delay, in microseconds, after a write transfer.
> >  
> > +  stacked-memories:
> > +    $ref: /schemas/types.yaml#/definitions/uint64-matrix  
> 
> matrix or...
> 
> > +    description: Several SPI memories can be wired in stacked mode.
> > +      This basically means that either a device features several chip
> > +      selects, or that different devices must be seen as a single
> > +      bigger chip. This basically doubles (or more) the total address
> > +      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.  
> 
> array?
> 
> Sounds like an array from the description as there is only 1 element, 
> the size.

Well, what I expected to have was something like:

dt:		<property> = <uint64>, <uint64>;

It seemed like the only possible way (that the tooling would validate)
was to use:

bindings:	$ref: /schemas/types.yaml#/definitions/uint64-matrix

So I assumed I was defining a matrix of AxB elements, where A is the
number of devices I want to "stack" and B is the number of values
needed to describe its size, so 1.

I realized that the following example, which I was expecting to work,
was failing:

bindings:	$ref: /schemas/types.yaml#/definitions/uint64-array
dt:		<property> = <uint64>, <uint64>;

Indeed, as you propose, this actually works but describes two values
(tied somehow) into a single element, which is not exactly what I
wanted:

bindings: 	$ref: /schemas/types.yaml#/definitions/uint64-array
dt:		<property> = <uint64 uint64>;

But more disturbing, all the following constructions worked, when using
32-bits values instead:

bindings: 	$ref: /schemas/types.yaml#/definitions/uint32-array
dt:		<property> = <uint32 uint32>;

bindings: 	$ref: /schemas/types.yaml#/definitions/uint32-array
dt:		<property> = <uint32>, <uint32>;

bindings: 	$ref: /schemas/types.yaml#/definitions/uint32-matrix
dt:		<property> = <uint32 uint32>;

bindings: 	$ref: /schemas/types.yaml#/definitions/uint32-matrix
dt:		<property> = <uint32>, <uint32>;

I am fine waiting a bit if you think there is a need for some tooling
update on your side. Otherwise, do you really think that this solution
is the one we should really use?

bindings: 	$ref: /schemas/types.yaml#/definitions/uint64-array
dt:		<property> = <uint64 uint64>;

Because from my point of view it does not match what we usually do for
other "types" of elements, such as:

dt:		<property> = <phandle1 index1>, <phandle2 index2>;

or

dt:		<property> = <small-val1>, <small-val2>;

> 
> > +    minItems: 2
> > +    maxItems: 2
> > +    items:
> > +      maxItems: 1  
> 
> This says you can only have 2 64-bit entries. Probably not what you 
> want. This looks like a case for a maxItems 'should be enough for now' 
> type of value.

Yes, that is what I wanted to describe.

In my recent contributions you always preferred to bound things as much
as possible, even though later it might become necessary to loosen the
constraint. Right now I see the use of these properties for 2 devices,
but in theory there is no limit.

Of course if we switch to the array representation I suppose I should
stick to:

+    minItems: 2
+    maxItems: 2

> 
> > +
> > +  parallel-memories:
> > +    $ref: /schemas/types.yaml#/definitions/uint64-matrix
> > +    description: Several SPI memories can be wired in parallel mode.
> > +      The devices are physically on a different buses but will always
> > +      act synchronously as each data word is spread across the
> > +      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.
> > +    minItems: 2
> > +    maxItems: 2
> > +    items:
> > +      maxItems: 1
> > +
> >  # The controller specific properties go here.
> >  allOf:
> >    - $ref: cdns,qspi-nor-peripheral-props.yaml#
> > -- 
> > 2.27.0
> > 
> >   


Thanks,
Miquèl

Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Miquel Raynal]

Hi Rob,

miquel.raynal@bootlin.com wrote on Thu, 16 Dec 2021 16:02:26 +0100:

> Hi Rob,
> 
> robh@kernel.org wrote on Tue, 14 Dec 2021 11:32:56 -0600:
> 
> > On Fri, Dec 10, 2021 at 09:10:38PM +0100, Miquel Raynal wrote:  
> > > Describe two new memories modes:
> > > - A stacked mode when the bus is common but the address space extended
> > >   with an additinals wires.
> > > - A parallel mode with parallel busses accessing parallel flashes where
> > >   the data is spread.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > index 5dd209206e88..4194fee8f556 100644
> > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > @@ -82,6 +82,35 @@ properties:
> > >      description:
> > >        Delay, in microseconds, after a write transfer.
> > >  
> > > +  stacked-memories:
> > > +    $ref: /schemas/types.yaml#/definitions/uint64-matrix    
> > 
> > matrix or...
> >   
> > > +    description: Several SPI memories can be wired in stacked mode.
> > > +      This basically means that either a device features several chip
> > > +      selects, or that different devices must be seen as a single
> > > +      bigger chip. This basically doubles (or more) the total address
> > > +      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.    
> > 
> > array?
> > 
> > Sounds like an array from the description as there is only 1 element, 
> > the size.  
> 
> Well, what I expected to have was something like:
> 
> dt:		<property> = <uint64>, <uint64>;
> 
> It seemed like the only possible way (that the tooling would validate)
> was to use:
> 
> bindings:	$ref: /schemas/types.yaml#/definitions/uint64-matrix
> 
> So I assumed I was defining a matrix of AxB elements, where A is the
> number of devices I want to "stack" and B is the number of values
> needed to describe its size, so 1.
> 
> I realized that the following example, which I was expecting to work,
> was failing:
> 
> bindings:	$ref: /schemas/types.yaml#/definitions/uint64-array
> dt:		<property> = <uint64>, <uint64>;
> 
> Indeed, as you propose, this actually works but describes two values
> (tied somehow) into a single element, which is not exactly what I
> wanted:
> 
> bindings: 	$ref: /schemas/types.yaml#/definitions/uint64-array
> dt:		<property> = <uint64 uint64>;
> 
> But more disturbing, all the following constructions worked, when using
> 32-bits values instead:
> 
> bindings: 	$ref: /schemas/types.yaml#/definitions/uint32-array
> dt:		<property> = <uint32 uint32>;
> 
> bindings: 	$ref: /schemas/types.yaml#/definitions/uint32-array
> dt:		<property> = <uint32>, <uint32>;
> 
> bindings: 	$ref: /schemas/types.yaml#/definitions/uint32-matrix
> dt:		<property> = <uint32 uint32>;
> 
> bindings: 	$ref: /schemas/types.yaml#/definitions/uint32-matrix
> dt:		<property> = <uint32>, <uint32>;
> 
> I am fine waiting a bit if you think there is a need for some tooling
> update on your side. Otherwise, do you really think that this solution
> is the one we should really use?
> 
> bindings: 	$ref: /schemas/types.yaml#/definitions/uint64-array
> dt:		<property> = <uint64 uint64>;
> 
> Because from my point of view it does not match what we usually do for
> other "types" of elements, such as:
> 
> dt:		<property> = <phandle1 index1>, <phandle2 index2>;
> 
> or
> 
> dt:		<property> = <small-val1>, <small-val2>;

Sorry for bothering you, is this something you still have in mind? It
seems that the tooling is the culprit here and I would highly
appreciate your help on that point.

Thanks,
Miquèl

> 
> >   
> > > +    minItems: 2
> > > +    maxItems: 2
> > > +    items:
> > > +      maxItems: 1    
> > 
> > This says you can only have 2 64-bit entries. Probably not what you 
> > want. This looks like a case for a maxItems 'should be enough for now' 
> > type of value.  
> 
> Yes, that is what I wanted to describe.
> 
> In my recent contributions you always preferred to bound things as much
> as possible, even though later it might become necessary to loosen the
> constraint. Right now I see the use of these properties for 2 devices,
> but in theory there is no limit.
> 
> Of course if we switch to the array representation I suppose I should
> stick to:
> 
> +    minItems: 2
> +    maxItems: 2

Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Rob Herring]

On Thu, Dec 16, 2021 at 9:02 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>
> Hi Rob,
>
> robh@kernel.org wrote on Tue, 14 Dec 2021 11:32:56 -0600:
>
> > On Fri, Dec 10, 2021 at 09:10:38PM +0100, Miquel Raynal wrote:
> > > Describe two new memories modes:
> > > - A stacked mode when the bus is common but the address space extended
> > >   with an additinals wires.
> > > - A parallel mode with parallel busses accessing parallel flashes where
> > >   the data is spread.
> > >
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > index 5dd209206e88..4194fee8f556 100644
> > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > @@ -82,6 +82,35 @@ properties:
> > >      description:
> > >        Delay, in microseconds, after a write transfer.
> > >
> > > +  stacked-memories:
> > > +    $ref: /schemas/types.yaml#/definitions/uint64-matrix
> >
> > matrix or...
> >
> > > +    description: Several SPI memories can be wired in stacked mode.
> > > +      This basically means that either a device features several chip
> > > +      selects, or that different devices must be seen as a single
> > > +      bigger chip. This basically doubles (or more) the total address
> > > +      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.
> >
> > array?
> >
> > Sounds like an array from the description as there is only 1 element,
> > the size.
>
> Well, what I expected to have was something like:
>
> dt:             <property> = <uint64>, <uint64>;

The downside to this is you really need:

property = /bits/ 64 <uint64>, /bits/ 64 <uint64>;

And I just fixed some examples that had:

property = /bits/ 64 <uint64>, <uint64>;

The property length there is 12 bytes. That aspect of the syntax is
really not obvious. I only realized that somewhat recently.

You can even do:

property = /bits/ 64 <uint64>, "a string", /bits/ 8 <uint8>;

While dts syntax allows this, our sanity does not.

> It seemed like the only possible way (that the tooling would validate)
> was to use:
>
> bindings:       $ref: /schemas/types.yaml#/definitions/uint64-matrix
>
> So I assumed I was defining a matrix of AxB elements, where A is the
> number of devices I want to "stack" and B is the number of values
> needed to describe its size, so 1.

Yeah, that's well reasoned and I agree. The other array case is you
have N values where each value represents different data rather than
instances of the same data. The challenge is for the schema fixups to
recognize which is which without saying the schema must look like
exactly X or Y as there will be exceptions.

> I realized that the following example, which I was expecting to work,
> was failing:
>
> bindings:       $ref: /schemas/types.yaml#/definitions/uint64-array
> dt:             <property> = <uint64>, <uint64>;
>
> Indeed, as you propose, this actually works but describes two values
> (tied somehow) into a single element, which is not exactly what I
> wanted:
>
> bindings:       $ref: /schemas/types.yaml#/definitions/uint64-array
> dt:             <property> = <uint64 uint64>;
>
> But more disturbing, all the following constructions worked, when using
> 32-bits values instead:
>
> bindings:       $ref: /schemas/types.yaml#/definitions/uint32-array
> dt:             <property> = <uint32 uint32>;
>
> bindings:       $ref: /schemas/types.yaml#/definitions/uint32-array
> dt:             <property> = <uint32>, <uint32>;
>
> bindings:       $ref: /schemas/types.yaml#/definitions/uint32-matrix
> dt:             <property> = <uint32 uint32>;
>
> bindings:       $ref: /schemas/types.yaml#/definitions/uint32-matrix
> dt:             <property> = <uint32>, <uint32>;

That works because there's some really ugly code to transform the
schema into both forms.

> I am fine waiting a bit if you think there is a need for some tooling
> update on your side. Otherwise, do you really think that this solution
> is the one we should really use?
>
> bindings:       $ref: /schemas/types.yaml#/definitions/uint64-array
> dt:             <property> = <uint64 uint64>;

Because of the /bits/ issue, yes.

More importantly, the bracketing in dts files is not going to matter
soon (from a validation perspective). I'm working on moving validation
from using the yaml encoded DT (which depends on and preserves
brackets) to using dtbs. This will use the schemas to decode the
property values into the right format/type.


> Because from my point of view it does not match what we usually do for
> other "types" of elements, such as:
>
> dt:             <property> = <phandle1 index1>, <phandle2 index2>;
>
> or
>
> dt:             <property> = <small-val1>, <small-val2>;
>
> >
> > > +    minItems: 2
> > > +    maxItems: 2
> > > +    items:
> > > +      maxItems: 1
> >
> > This says you can only have 2 64-bit entries. Probably not what you
> > want. This looks like a case for a maxItems 'should be enough for now'
> > type of value.
>
> Yes, that is what I wanted to describe.
>
> In my recent contributions you always preferred to bound things as much
> as possible, even though later it might become necessary to loosen the
> constraint. Right now I see the use of these properties for 2 devices,
> but in theory there is no limit.

Ok, as long as we're not loosening it frequently.

Rob

Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Miquel Raynal]

Hi Rob,

> > It seemed like the only possible way (that the tooling would validate)
> > was to use:
> >
> > bindings:       $ref: /schemas/types.yaml#/definitions/uint64-matrix
> >
> > So I assumed I was defining a matrix of AxB elements, where A is the
> > number of devices I want to "stack" and B is the number of values
> > needed to describe its size, so 1.  
> 
> Yeah, that's well reasoned and I agree. The other array case is you
> have N values where each value represents different data rather than
> instances of the same data. The challenge is for the schema fixups to
> recognize which is which without saying the schema must look like
> exactly X or Y as there will be exceptions.

Ok, now I see the problem on the tooling side and why you chose not to
use this syntax.

> > I realized that the following example, which I was expecting to work,
> > was failing:
> >
> > bindings:       $ref: /schemas/types.yaml#/definitions/uint64-array
> > dt:             <property> = <uint64>, <uint64>;
> >
> > Indeed, as you propose, this actually works but describes two values
> > (tied somehow) into a single element, which is not exactly what I
> > wanted:
> >
> > bindings:       $ref: /schemas/types.yaml#/definitions/uint64-array
> > dt:             <property> = <uint64 uint64>;
> >
> > But more disturbing, all the following constructions worked, when using
> > 32-bits values instead:
> >
> > bindings:       $ref: /schemas/types.yaml#/definitions/uint32-array
> > dt:             <property> = <uint32 uint32>;
> >
> > bindings:       $ref: /schemas/types.yaml#/definitions/uint32-array
> > dt:             <property> = <uint32>, <uint32>;
> >
> > bindings:       $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > dt:             <property> = <uint32 uint32>;
> >
> > bindings:       $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > dt:             <property> = <uint32>, <uint32>;  
> 
> That works because there's some really ugly code to transform the
> schema into both forms.

Good to know, this kind of puzzled me when I tried all the
configurations :)

> > I am fine waiting a bit if you think there is a need for some tooling
> > update on your side. Otherwise, do you really think that this solution
> > is the one we should really use?
> >
> > bindings:       $ref: /schemas/types.yaml#/definitions/uint64-array
> > dt:             <property> = <uint64 uint64>;  
> 
> Because of the /bits/ issue, yes.
> 
> More importantly, the bracketing in dts files is not going to matter
> soon (from a validation perspective). I'm working on moving validation
> from using the yaml encoded DT (which depends on and preserves
> brackets) to using dtbs. This will use the schemas to decode the
> property values into the right format/type.

Ok.

Well, thanks for the feedback, with the latest dt-schema the tooling
now validates the binding so I am going to send it as a v6 to collect
your Ack.

Thanks,
Miquèl

Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Pratyush Yadav]

Hi Miquel,

On 10/12/21 09:10PM, Miquel Raynal wrote:
> Describe two new memories modes:
> - A stacked mode when the bus is common but the address space extended
>   with an additinals wires.
> - A parallel mode with parallel busses accessing parallel flashes where
>   the data is spread.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> index 5dd209206e88..4194fee8f556 100644
> --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> @@ -82,6 +82,35 @@ properties:
>      description:
>        Delay, in microseconds, after a write transfer.
>  
> +  stacked-memories:
> +    $ref: /schemas/types.yaml#/definitions/uint64-matrix

Why matrix? Can't you use array here? Sorry, I am not much familiar with 
JSON schema.

> +    description: Several SPI memories can be wired in stacked mode.
> +      This basically means that either a device features several chip
> +      selects, or that different devices must be seen as a single
> +      bigger chip. This basically doubles (or more) the total address
> +      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.
> +    minItems: 2
> +    maxItems: 2
> +    items:
> +      maxItems: 1

Thanks. This looks better to me.

But before we go ahead, I think there has been some confusion around 
what exactly your patches intend to support. Let's clear them up first. 
What type of setup do you want to support?

  1. One single flash but with multiple dies, with each die sitting on a 
     different CS.
  2. Two (or more) identical but independent flash memories to be 
     treated as one.
  3. Two (or more) different and independent flash memories to be 
     treated as one.

In our earlier exchanges you said you want to support 2. And when I 
wanted you to account for 3 as well you said we should use mtdconcat for 
that. So my question is, why can't we use mtdconcat for 2 as well, since 
it is just a special case of 3? And if we are using mtdconcat, then why 
do we need this at all? Shouldn't you then choose the chip at MTD layer 
and use the respective SPI device to get the CS value, which would make 
this property useless?

I can see this making sense for case 1. For that case you said you don't 
have an existing datasheet or device to propose. And if there is no real 
device doing it I see little point in figuring out a binding for it.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Miquel Raynal]

Hello Pratyush,

p.yadav@ti.com wrote on Wed, 15 Dec 2021 01:14:33 +0530:

> Hi Miquel,
> 
> On 10/12/21 09:10PM, Miquel Raynal wrote:
> > Describe two new memories modes:
> > - A stacked mode when the bus is common but the address space extended
> >   with an additinals wires.
> > - A parallel mode with parallel busses accessing parallel flashes where
> >   the data is spread.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > index 5dd209206e88..4194fee8f556 100644
> > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > @@ -82,6 +82,35 @@ properties:
> >      description:
> >        Delay, in microseconds, after a write transfer.
> >  
> > +  stacked-memories:
> > +    $ref: /schemas/types.yaml#/definitions/uint64-matrix  
> 
> Why matrix? Can't you use array here? Sorry, I am not much familiar with 
> JSON schema.

Yes, Rob also opened the discussion, I've answered there on this point,
but I agree, this should be define as an array, but the current tooling
refused to accept what I wanted from a dt-writing point of view. More
on that on the dedicated thread.

> > +    description: Several SPI memories can be wired in stacked mode.
> > +      This basically means that either a device features several chip
> > +      selects, or that different devices must be seen as a single
> > +      bigger chip. This basically doubles (or more) the total address
> > +      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.
> > +    minItems: 2
> > +    maxItems: 2
> > +    items:
> > +      maxItems: 1  
> 
> Thanks. This looks better to me.
> 
> But before we go ahead, I think there has been some confusion around 
> what exactly your patches intend to support. Let's clear them up first. 
> What type of setup do you want to support?

Before I try to clarify your questions below, the setup that I have is:

Xilinx ZynqMP QSPI controller + 2 flashes.

What I want to describe is the specific handling of what the Xilinx
QSPI controller is able to do. This controller has two modes when wired
to more than one flash:
- stacked
- parallel

I have not entered the documentation nor the code in details yet, but I
believe that handling two flashes in the stacked configuration, besides
a couple of possible optimizations that are possible by the hardware,
is close to what any controller would do. Maybe there is one difference
though, when in the "stacked" mode, this controller treats the two
flashes as a single one, so that is why, if we want to support this
"advanced" mode, we *need* a way to know that this mode should be used
because the controller expects a wider range of addresses.

About the parallel configuration, there is absolutely no doubt that we
have no other choice than describing how the data is spread across two
devices. We don't really care about the manner, but the controller
needs to know how to assert the two CS internally so this definitely
something that needs to be described.

Now the question you might ask is, why do we define these properties as
flash properties then? And this is a real good question, I think both
actually work as long as we consider that we can only have either a
spi-memory or any other type of device on a single bus, but not both at
the same time. In v1 I proposed a controller property. Mark proposed to
switch for a device property which I did in v2 onward.

>   1. One single flash but with multiple dies, with each die sitting on a 
>      different CS.

Yes.

>   2. Two (or more) identical but independent flash memories to be 
>      treated as one.

Yes.

>   3. Two (or more) different and independent flash memories to be 
>      treated as one.

I don't know. My first proposal excluded these, but I believe we can
handle them with the change your proposed (the device size as part of
the property).

> In our earlier exchanges you said you want to support 2. And when I 
> wanted you to account for 3 as well you said we should use mtdconcat for 
> that.

Not that we should, but that we could because from a core perspective
it's way more challenging than supporting identical devices. But the
conversation drifted about the software backend that we should use
rather than on the actual description, because mtdconcat is not a
feature which benefits from any kind of description yet, so even if we
use mtdconcat as backed, we will need some kind of description here as
well. So, as I told previously, I am fine considering these setups
in my proposal, that's why I updated my proposal to include the
devices size, even though that is way out of scope compared to my
initial target.

But here we are talking about driver code, which has nothing to do with
the bindings. If we focus on the bindings, I believe the solution with
the sizes covers all cases.

> So my question is, why can't we use mtdconcat for 2 as well, since 
> it is just a special case of 3? And if we are using mtdconcat, then why 
> do we need this at all? Shouldn't you then choose the chip at MTD layer 
> and use the respective SPI device to get the CS value, which would make 
> this property useless?

Reason 1:
As depicted above, while the stacked mode might more or less
fit the mtdconcat idea, it is absolutely not the case for the parallel
mode.

Reason 2:
mtdconcat is a software backend. Here we are discussing bindings.
No matter what backed we finally pick, there will be the need for a
proper description and so far there has been no binding for mtdconcat
(even though I tried to push in favor of it a while ago without
success). So no matter what software solution we we adopt, we
*will* need an upstream binding description at some point. But
mtdconcat really means "there are two devices we want to consider as a
single". While here the point is: we have two devices that get
abstracted by the controller, and we still must describe that.

> I can see this making sense for case 1. For that case you said you don't 
> have an existing datasheet or device to propose. And if there is no real 
> device doing it I see little point in figuring out a binding for it.

Yes, because somewhat we focused the debate over the devices, while I
was initially talking about a controller abstraction. There is (at
least) one controller doing such abstractions, the Xilinx ZynqMP
generic QSPI controller, the spec is public (chapter 24):
https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf

I hope all this clarifies the situation!

Cheers,
Miquèl

Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Pratyush Yadav]

On 16/12/21 05:25PM, Miquel Raynal wrote:
> Hello Pratyush,
> 
> p.yadav@ti.com wrote on Wed, 15 Dec 2021 01:14:33 +0530:
> 
> > Hi Miquel,
> > 
> > On 10/12/21 09:10PM, Miquel Raynal wrote:
> > > Describe two new memories modes:
> > > - A stacked mode when the bus is common but the address space extended
> > >   with an additinals wires.
> > > - A parallel mode with parallel busses accessing parallel flashes where
> > >   the data is spread.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > ---
> > >  .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
> > >  1 file changed, 29 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > index 5dd209206e88..4194fee8f556 100644
> > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > @@ -82,6 +82,35 @@ properties:
> > >      description:
> > >        Delay, in microseconds, after a write transfer.
> > >  
> > > +  stacked-memories:
> > > +    $ref: /schemas/types.yaml#/definitions/uint64-matrix  
> > 
> > Why matrix? Can't you use array here? Sorry, I am not much familiar with 
> > JSON schema.
> 
> Yes, Rob also opened the discussion, I've answered there on this point,
> but I agree, this should be define as an array, but the current tooling
> refused to accept what I wanted from a dt-writing point of view. More
> on that on the dedicated thread.
> 
> > > +    description: Several SPI memories can be wired in stacked mode.
> > > +      This basically means that either a device features several chip
> > > +      selects, or that different devices must be seen as a single
> > > +      bigger chip. This basically doubles (or more) the total address
> > > +      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.
> > > +    minItems: 2
> > > +    maxItems: 2
> > > +    items:
> > > +      maxItems: 1  
> > 
> > Thanks. This looks better to me.
> > 
> > But before we go ahead, I think there has been some confusion around 
> > what exactly your patches intend to support. Let's clear them up first. 
> > What type of setup do you want to support?
> 
> Before I try to clarify your questions below, the setup that I have is:
> 
> Xilinx ZynqMP QSPI controller + 2 flashes.
> 
> What I want to describe is the specific handling of what the Xilinx
> QSPI controller is able to do. This controller has two modes when wired
> to more than one flash:
> - stacked
> - parallel
> 
> I have not entered the documentation nor the code in details yet, but I
> believe that handling two flashes in the stacked configuration, besides
> a couple of possible optimizations that are possible by the hardware,
> is close to what any controller would do. Maybe there is one difference
> though, when in the "stacked" mode, this controller treats the two
> flashes as a single one, so that is why, if we want to support this
> "advanced" mode, we *need* a way to know that this mode should be used
> because the controller expects a wider range of addresses.

Ok.

> 
> About the parallel configuration, there is absolutely no doubt that we
> have no other choice than describing how the data is spread across two
> devices. We don't really care about the manner, but the controller
> needs to know how to assert the two CS internally so this definitely
> something that needs to be described.

Yes. And with device sizes as part of the property we should be able to 
do this.

> 
> Now the question you might ask is, why do we define these properties as
> flash properties then? And this is a real good question, I think both
> actually work as long as we consider that we can only have either a
> spi-memory or any other type of device on a single bus, but not both at
> the same time. In v1 I proposed a controller property. Mark proposed to
> switch for a device property which I did in v2 onward.

I also think making it a device property makes more sense. You are 
describing how the flashes are laid out, which is independent of the 
controller. With the same SoC, I could have one board with a single 
flash and another with a dual-die flash.

> 
> >   1. One single flash but with multiple dies, with each die sitting on a 
> >      different CS.
> 
> Yes.
> 
> >   2. Two (or more) identical but independent flash memories to be 
> >      treated as one.
> 
> Yes.
> 
> >   3. Two (or more) different and independent flash memories to be 
> >      treated as one.
> 
> I don't know. My first proposal excluded these, but I believe we can
> handle them with the change your proposed (the device size as part of
> the property).
> 
> > In our earlier exchanges you said you want to support 2. And when I 
> > wanted you to account for 3 as well you said we should use mtdconcat for 
> > that.
> 
> Not that we should, but that we could because from a core perspective
> it's way more challenging than supporting identical devices. But the
> conversation drifted about the software backend that we should use
> rather than on the actual description, because mtdconcat is not a
> feature which benefits from any kind of description yet, so even if we
> use mtdconcat as backed, we will need some kind of description here as
> well. So, as I told previously, I am fine considering these setups
> in my proposal, that's why I updated my proposal to include the
> devices size, even though that is way out of scope compared to my
> initial target.

Right, and I appreciate that :-)

> 
> But here we are talking about driver code, which has nothing to do with
> the bindings. If we focus on the bindings, I believe the solution with
> the sizes covers all cases.

I think it is important to discuss _how_ we would implement this in the 
driver because this could help identify flaws or shortcomings in the 
bindings. In this case, I agree with you that the solution with sizes 
should cover all 3 cases and we now have lot more flexibility in how we 
design the driver.

> 
> > So my question is, why can't we use mtdconcat for 2 as well, since 
> > it is just a special case of 3? And if we are using mtdconcat, then why 
> > do we need this at all? Shouldn't you then choose the chip at MTD layer 
> > and use the respective SPI device to get the CS value, which would make 
> > this property useless?
> 
> Reason 1:
> As depicted above, while the stacked mode might more or less
> fit the mtdconcat idea, it is absolutely not the case for the parallel
> mode.

Right. My question was mostly about the stacked mode.

> 
> Reason 2:
> mtdconcat is a software backend. Here we are discussing bindings.
> No matter what backed we finally pick, there will be the need for a
> proper description and so far there has been no binding for mtdconcat
> (even though I tried to push in favor of it a while ago without
> success). So no matter what software solution we we adopt, we
> *will* need an upstream binding description at some point. But
> mtdconcat really means "there are two devices we want to consider as a
> single". While here the point is: we have two devices that get
> abstracted by the controller, and we still must describe that.
> 
> > I can see this making sense for case 1. For that case you said you don't 
> > have an existing datasheet or device to propose. And if there is no real 
> > device doing it I see little point in figuring out a binding for it.
> 
> Yes, because somewhat we focused the debate over the devices, while I
> was initially talking about a controller abstraction. There is (at
> least) one controller doing such abstractions, the Xilinx ZynqMP
> generic QSPI controller, the spec is public (chapter 24):
> https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf

I think this _is_ a device problem first. The controller part should be 
easy to do once we solve the device problem. Once we clearly describe 
how the devices are laid out it should be simple for the controller side 
to figure out what to do.

> 
> I hope all this clarifies the situation!

Thanks. I am quite happy with this patch. I'll drop my R-by once you 
figure out what type to use with these properties in v5.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes

[Miquel Raynal]

Hello Pratyush,

p.yadav@ti.com wrote on Fri, 17 Dec 2021 18:09:10 +0530:

> On 16/12/21 05:25PM, Miquel Raynal wrote:
> > Hello Pratyush,
> > 
> > p.yadav@ti.com wrote on Wed, 15 Dec 2021 01:14:33 +0530:
> >   
> > > Hi Miquel,
> > > 
> > > On 10/12/21 09:10PM, Miquel Raynal wrote:  
> > > > Describe two new memories modes:
> > > > - A stacked mode when the bus is common but the address space extended
> > > >   with an additinals wires.
> > > > - A parallel mode with parallel busses accessing parallel flashes where
> > > >   the data is spread.
> > > > 
> > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > > > ---
> > > >  .../bindings/spi/spi-peripheral-props.yaml    | 29 +++++++++++++++++++
> > > >  1 file changed, 29 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > > index 5dd209206e88..4194fee8f556 100644
> > > > --- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > > +++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
> > > > @@ -82,6 +82,35 @@ properties:
> > > >      description:
> > > >        Delay, in microseconds, after a write transfer.
> > > >  
> > > > +  stacked-memories:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint64-matrix    
> > > 
> > > Why matrix? Can't you use array here? Sorry, I am not much familiar with 
> > > JSON schema.  
> > 
> > Yes, Rob also opened the discussion, I've answered there on this point,
> > but I agree, this should be define as an array, but the current tooling
> > refused to accept what I wanted from a dt-writing point of view. More
> > on that on the dedicated thread.
> >   
> > > > +    description: Several SPI memories can be wired in stacked mode.
> > > > +      This basically means that either a device features several chip
> > > > +      selects, or that different devices must be seen as a single
> > > > +      bigger chip. This basically doubles (or more) the total address
> > > > +      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.
> > > > +    minItems: 2
> > > > +    maxItems: 2
> > > > +    items:
> > > > +      maxItems: 1    
> > > 
> > > Thanks. This looks better to me.
> > > 
> > > But before we go ahead, I think there has been some confusion around 
> > > what exactly your patches intend to support. Let's clear them up first. 
> > > What type of setup do you want to support?  
> > 
> > Before I try to clarify your questions below, the setup that I have is:
> > 
> > Xilinx ZynqMP QSPI controller + 2 flashes.
> > 
> > What I want to describe is the specific handling of what the Xilinx
> > QSPI controller is able to do. This controller has two modes when wired
> > to more than one flash:
> > - stacked
> > - parallel
> > 
> > I have not entered the documentation nor the code in details yet, but I
> > believe that handling two flashes in the stacked configuration, besides
> > a couple of possible optimizations that are possible by the hardware,
> > is close to what any controller would do. Maybe there is one difference
> > though, when in the "stacked" mode, this controller treats the two
> > flashes as a single one, so that is why, if we want to support this
> > "advanced" mode, we *need* a way to know that this mode should be used
> > because the controller expects a wider range of addresses.  
> 
> Ok.
> 
> > 
> > About the parallel configuration, there is absolutely no doubt that we
> > have no other choice than describing how the data is spread across two
> > devices. We don't really care about the manner, but the controller
> > needs to know how to assert the two CS internally so this definitely
> > something that needs to be described.  
> 
> Yes. And with device sizes as part of the property we should be able to 
> do this.
> 
> > 
> > Now the question you might ask is, why do we define these properties as
> > flash properties then? And this is a real good question, I think both
> > actually work as long as we consider that we can only have either a
> > spi-memory or any other type of device on a single bus, but not both at
> > the same time. In v1 I proposed a controller property. Mark proposed to
> > switch for a device property which I did in v2 onward.  
> 
> I also think making it a device property makes more sense. You are 
> describing how the flashes are laid out, which is independent of the 
> controller. With the same SoC, I could have one board with a single 
> flash and another with a dual-die flash.
> 
> >   
> > >   1. One single flash but with multiple dies, with each die sitting on a 
> > >      different CS.  
> > 
> > Yes.
> >   
> > >   2. Two (or more) identical but independent flash memories to be 
> > >      treated as one.  
> > 
> > Yes.
> >   
> > >   3. Two (or more) different and independent flash memories to be 
> > >      treated as one.  
> > 
> > I don't know. My first proposal excluded these, but I believe we can
> > handle them with the change your proposed (the device size as part of
> > the property).
> >   
> > > In our earlier exchanges you said you want to support 2. And when I 
> > > wanted you to account for 3 as well you said we should use mtdconcat for 
> > > that.  
> > 
> > Not that we should, but that we could because from a core perspective
> > it's way more challenging than supporting identical devices. But the
> > conversation drifted about the software backend that we should use
> > rather than on the actual description, because mtdconcat is not a
> > feature which benefits from any kind of description yet, so even if we
> > use mtdconcat as backed, we will need some kind of description here as
> > well. So, as I told previously, I am fine considering these setups
> > in my proposal, that's why I updated my proposal to include the
> > devices size, even though that is way out of scope compared to my
> > initial target.  
> 
> Right, and I appreciate that :-)
> 
> > 
> > But here we are talking about driver code, which has nothing to do with
> > the bindings. If we focus on the bindings, I believe the solution with
> > the sizes covers all cases.  
> 
> I think it is important to discuss _how_ we would implement this in the 
> driver because this could help identify flaws or shortcomings in the 
> bindings. In this case, I agree with you that the solution with sizes 
> should cover all 3 cases and we now have lot more flexibility in how we 
> design the driver.
> 
> >   
> > > So my question is, why can't we use mtdconcat for 2 as well, since 
> > > it is just a special case of 3? And if we are using mtdconcat, then why 
> > > do we need this at all? Shouldn't you then choose the chip at MTD layer 
> > > and use the respective SPI device to get the CS value, which would make 
> > > this property useless?  
> > 
> > Reason 1:
> > As depicted above, while the stacked mode might more or less
> > fit the mtdconcat idea, it is absolutely not the case for the parallel
> > mode.  
> 
> Right. My question was mostly about the stacked mode.
> 
> > 
> > Reason 2:
> > mtdconcat is a software backend. Here we are discussing bindings.
> > No matter what backed we finally pick, there will be the need for a
> > proper description and so far there has been no binding for mtdconcat
> > (even though I tried to push in favor of it a while ago without
> > success). So no matter what software solution we we adopt, we
> > *will* need an upstream binding description at some point. But
> > mtdconcat really means "there are two devices we want to consider as a
> > single". While here the point is: we have two devices that get
> > abstracted by the controller, and we still must describe that.
> >   
> > > I can see this making sense for case 1. For that case you said you don't 
> > > have an existing datasheet or device to propose. And if there is no real 
> > > device doing it I see little point in figuring out a binding for it.  
> > 
> > Yes, because somewhat we focused the debate over the devices, while I
> > was initially talking about a controller abstraction. There is (at
> > least) one controller doing such abstractions, the Xilinx ZynqMP
> > generic QSPI controller, the spec is public (chapter 24):
> > https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf  
> 
> I think this _is_ a device problem first. The controller part should be 
> easy to do once we solve the device problem. Once we clearly describe 
> how the devices are laid out it should be simple for the controller side 
> to figure out what to do.
> 
> > 
> > I hope all this clarifies the situation!  
> 
> Thanks. I am quite happy with this patch. I'll drop my R-by once you 
> figure out what type to use with these properties in v5.

Great! Thank you very much for this discussion, it forced me to clarify
things in my head as well :-)

Thanks,
Miquèl

[PATCH v4 3/3] spi: dt-bindings: Add an example with two stacked flashes

[Miquel Raynal]

Provide an example of how to describe two flashes in eg. stacked mode.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/spi/spi-controller.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
index 36b72518f565..048d2bbc74a5 100644
--- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
@@ -139,4 +139,11 @@ examples:
             spi-max-frequency = <100000>;
             reg = <1>;
         };
+
+        flash@2 {
+          compatible = "jedec,spi-nor";
+          spi-max-frequency = <50000000>;
+          reg = <2>, <3>;
+          stacked-memories = /bits/ 64 <0x10000000>, /bits/ 64 <0x10000000>;
+        };
     };
-- 
2.27.0

Re: [PATCH v4 3/3] spi: dt-bindings: Add an example with two stacked flashes

[Rob Herring]

On Fri, Dec 10, 2021 at 09:10:39PM +0100, Miquel Raynal wrote:
> Provide an example of how to describe two flashes in eg. stacked mode.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/spi/spi-controller.yaml | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/spi-controller.yaml b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> index 36b72518f565..048d2bbc74a5 100644
> --- a/Documentation/devicetree/bindings/spi/spi-controller.yaml
> +++ b/Documentation/devicetree/bindings/spi/spi-controller.yaml
> @@ -139,4 +139,11 @@ examples:
>              spi-max-frequency = <100000>;
>              reg = <1>;
>          };
> +
> +        flash@2 {
> +          compatible = "jedec,spi-nor";
> +          spi-max-frequency = <50000000>;
> +          reg = <2>, <3>;
> +          stacked-memories = /bits/ 64 <0x10000000>, /bits/ 64 <0x10000000>;

stacked-memories = /bits/ 64 <0x10000000 0x10000000>;

While the source syntax allows for multiple (and mixed) /bits/ 
annotations, that's not something we support really.

If you have problems with the schema tools on this, let me know. The 
handling of this is not the greatest.

Rob