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

[Rob Herring <robh@xxxxxxxxxx>]

On Thu, Dec 16, 2021 at 9:02 AM Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:
>
> Hi Rob,
>
> robh@xxxxxxxxxx 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@xxxxxxxxxxx>
> > > ---
> > >  .../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