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

From: Pratyush Yadav <p.yadav@ti.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Richard Weinberger <richard@nod.at>,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>,
	Michael Walle <michael@walle.cc>, <linux-mtd@lists.infradead.org>,
	Michal Simek <monstr@monstr.eu>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Rob Herring <robh+dt@kernel.org>, <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: Wed, 15 Dec 2021 01:14:33 +0530	[thread overview]
Message-ID: <20211214194431.4kpwfgvju6msh5d4@ti.com> (raw)
In-Reply-To: <20211210201039.729961-3-miquel.raynal@bootlin.com>

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.