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

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Rob Herring <robh@kernel.org>
Cc: Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Pratyush Yadav <p.yadav@ti.com>, Michael Walle <michael@walle.cc>,
	linux-mtd@lists.infradead.org, Michal Simek <monstr@monstr.eu>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	devicetree@vger.kernel.org, Mark Brown <broonie@kernel.org>,
	linux-spi@vger.kernel.org
Subject: Re: [PATCH v4 2/3] spi: dt-bindings: Describe stacked/parallel memories modes
Date: Thu, 16 Dec 2021 16:02:26 +0100	[thread overview]
Message-ID: <20211216160226.4fac5ccc@xps13> (raw)
In-Reply-To: <YbjVSNAC8M5Y1nHp@robh.at.kernel.org>

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