Message-ID: <20240812103812.72763f69@xps-13>
Date: Mon, 12 Aug 2024 10:38:12 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@....com>
Cc: Tudor Ambarus <tudor.ambarus@...aro.org>, "broonie@...nel.org"
 <broonie@...nel.org>, "pratyush@...nel.org" <pratyush@...nel.org>,
 "richard@....at" <richard@....at>, "vigneshr@...com" <vigneshr@...com>,
 "sbinding@...nsource.cirrus.com" <sbinding@...nsource.cirrus.com>,
 "lee@...nel.org" <lee@...nel.org>, "james.schulman@...rus.com"
 <james.schulman@...rus.com>, "david.rhodes@...rus.com"
 <david.rhodes@...rus.com>, "rf@...nsource.cirrus.com"
 <rf@...nsource.cirrus.com>, "perex@...ex.cz" <perex@...ex.cz>,
 "tiwai@...e.com" <tiwai@...e.com>, "linux-spi@...r.kernel.org"
 <linux-spi@...r.kernel.org>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, "michael@...le.cc" <michael@...le.cc>,
 "linux-mtd@...ts.infradead.org" <linux-mtd@...ts.infradead.org>,
 "nicolas.ferre@...rochip.com" <nicolas.ferre@...rochip.com>,
 "alexandre.belloni@...tlin.com" <alexandre.belloni@...tlin.com>,
 "claudiu.beznea@...on.dev" <claudiu.beznea@...on.dev>, "Simek, Michal"
 <michal.simek@....com>, "linux-arm-kernel@...ts.infradead.org"
 <linux-arm-kernel@...ts.infradead.org>, "alsa-devel@...a-project.org"
 <alsa-devel@...a-project.org>, "patches@...nsource.cirrus.com"
 <patches@...nsource.cirrus.com>, "linux-sound@...r.kernel.org"
 <linux-sound@...r.kernel.org>, "git (AMD-Xilinx)" <git@....com>,
 "amitrkcian2002@...il.com" <amitrkcian2002@...il.com>, Conor Dooley
 <conor.dooley@...rochip.com>, "beanhuo@...ron.com" <beanhuo@...ron.com>
Subject: Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support in
 spi-nor

Hi,

> Hello Everyone,
> 
> I would like to propose another approach for handling stacked and 
> parallel connection modes and would appreciate your thoughts on it. 
> But before that, here is some background on what we are trying to achieve.
> 
> The AMD QSPI controller supports two advanced connection modes(Stacked and 
> Dual Parallel) which allow the controller to treat two different flashes 
> as one storage.
> 
> Stacked:
> Flashes share the same SPI bus, but different CS line, controller asserts 
> the CS of the flash to which it needs to communicate.
> 
> Dual Parallel:
> Both the flashes have their separate SPI bus CS of both the flashes will 
> be asserted/de-asserted at the same time. In this mode data will be split 
> across both the flashes by enabling the STRIPE setting in the controller. 
> If STRIPE is not enabled, then same data will be sent to both the devices.
> 
> For more information on the modes please feel free to go through the 
> controller flash interface below
> https://docs.amd.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Device-Interface
> 
> Mirochip QSPI controller also supports "Dual Parallel 8-bit IO mode", but 
> they call it "Twin Quad Mode".
> https://ww1.microchip.com/downloads/aemDocuments/documents/MPU32/ProductDocuments/DataSheets/SAMA7G5-Series-Data-Sheet-DS60001765.pdf
> 
> DT binding changes were added through the following commits:
> https://github.com/torvalds/linux/commit/f89504300e94524d5d5846ff8b728592ac72cec4
> https://github.com/torvalds/linux/commit/eba5368503b4291db7819512600fa014ea17c5a8
> https://github.com/torvalds/linux/commit/e2edd1b64f1c79e8abda365149ed62a2a9a494b4
> 
> SPI core changes were adds through the following commit:
> https://github.com/torvalds/linux/commit/4d8ff6b0991d5e86b17b235fc46ec62e9195cb9b
> 
> Based on the inputs/suggestions from Tudor, i am planning to add a new 
> layer between the SPI-NOR and MTD layers to support stacked and parallel 
> configurations. This new layer will be part of the spi-nor and located in 
> mtd/spi-nor/

For now I don't think you need a totally generic implementation. As
long as there is only one controller supporting these modes, I'd say
this is not super relevant.

> This layer would perform the following tasks:
>  - During probing, store information from all the connected flashes, 
>    whether in stacked or parallel mode, and present it as a single device 
>    to the MTD layer.
>  - Register callbacks through this new layer instead of spi-nor/core.c and 
>    handle MTD device registration.
>  - In stacked mode, select the appropriate spi-nor flash based on the 
>    address provided by the MTD layer during flash operations.
>  - Manage flash crossover operations in stacked mode.
>  - Ensure both connected flashes are identical in parallel mode.
>  - Handle odd byte count requests from the MTD layer during flash 
>    operations in parallel mode.
> 
> For implementing this the current DT binding need to be updated as 
> follows.

So you want to go back to step 1 and redefine bindings? Is that worth?

> stacked-memories DT changes:
>  - Flash size information can be retrieved directly from the flash, so it 
>    has been removed from the DT binding.
>  - Each stacked flash will have its own flash node. This approach allows 
>    flashes of different makes and sizes to be stacked together, as each 
>    flash will be probed individually.

And how will you define partitions crossing device boundaries? I
believe this constraint has been totally forgotten in this proposal.
The whole idea of stacking two devices this way was to simplify the
user's life with a single device exposed and the controller handling
itself the CS changes. That is precisely what the current binding do.
The final goal being to double your storage transparently. If your goal
was to put two chips aside, then none of this was actually needed. If
you don't care about that anymore, then all the energy put into
discussing the bindings initially was useless and a controller property
could also have made the trick.

>  - The stacked-memories DT bindings will contain the phandles of the flash 
>    nodes connected in stacked mode.
> 
> spi@0 {
>   
>   flash@0 {
>     compatible = "jedec,spi-nor"
>     reg = <0x00>;
>     stacked-memories = <&flash@0 &flash@1>;
>     spi-max-frequency = <50000000>;
>     ...
>               partition@0 { 
>         label = "qspi-0";
>         reg = <0x0 0xf00000>;
>     };
>                         
> 
>   }
>   
>   flash@1 {
>     compatible = "jedec,spi-nor"
>               reg = <0x01>;
>     spi-max-frequency = <50000000>;
>     ...
>               partition@0 { 
>         label = "qspi-1";
>         reg = <0x0 0x800000>;
>     };
>   }
> }
> 
> parallel-memories DT changes:
>  - Flash size information can be retrieved directly from the flash, so it 
>    has been removed from the DT binding.
>  - Each flash connected in parallel mode will have its own flash node. 
>    This allows us to verify that both flashes connected in parallel are 
>    identical, as each flash node will be probed separately.

Well, you don't have to verify that. It's a basic hardware design
constraint for using this mode.

Otherwise same comment as above, this description doesn't allow
correct partitioning and that was one of the main constraints back when
we discussed these needs.

>  - The parallel-memories DT bindings will contain the phandles of the 
>    flash nodes connected in parallel.
> 
> spi@0 {
>   
>   flash@0 {
>     compatible = "jedec,spi-nor"
>     reg = <0x00>;
>     parallel-memories = <&flash@0 &flash@1>;
>     spi-max-frequency = <50000000>;
>     ...
>               partition@0 { 
>         label = "qspi-0";
>         reg = <0x0 0xf00000>;
>     };
>                         
> 
>   }
>   
>   flash@1 {
>     compatible = "jedec,spi-nor"
>               reg = <0x01>;
>     spi-max-frequency = <50000000>;
>     ...
>               partition@0 { 
>         label = "qspi-1";
>         reg = <0x0 0x800000>;
>     };
>   }
> }

Thanks,
Miquèl

Powered by blists - more mailing lists