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

[Miquel Raynal <miquel.raynal@bootlin.com>]

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