RE: [PATCH v11 00/10] spi: Add support for stacked/parallel memories

[Mahapatra, Amit Kumar]

Hello Michael,

> -----Original Message-----
> From: Michael Walle <michael@xxxxxxxx>
> Sent: Tuesday, December 12, 2023 6:05 PM
> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@xxxxxxx>
> Cc: broonie@xxxxxxxxxx; tudor.ambarus@xxxxxxxxxx; pratyush@xxxxxxxxxx;
> miquel.raynal@xxxxxxxxxxx; richard@xxxxxx; vigneshr@xxxxxx;
> sbinding@xxxxxxxxxxxxxxxxxxxxx; lee@xxxxxxxxxx;
> james.schulman@xxxxxxxxxx; david.rhodes@xxxxxxxxxx;
> rf@xxxxxxxxxxxxxxxxxxxxx; perex@xxxxxxxx; tiwai@xxxxxxxx; linux-
> spi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> mtd@xxxxxxxxxxxxxxxxxxx; nicolas.ferre@xxxxxxxxxxxxx;
> alexandre.belloni@xxxxxxxxxxx; claudiu.beznea@xxxxxxxxx; Simek, Michal
> <michal.simek@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; alsa-
> devel@xxxxxxxxxxxxxxxx; patches@xxxxxxxxxxxxxxxxxxxxx; linux-
> sound@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>;
> amitrkcian2002@xxxxxxxxx
> Subject: Re: [PATCH v11 00/10] spi: Add support for stacked/parallel
> memories
> 
> > This patch series updated the spi-nor, spi core and the AMD-Xilinx
> > GQSPI driver to add stacked and parallel memories support.
> 
> Honestly, I'm not thrilled about how this is implemented in the core and what
> the restrictions are.
> First, the pattern "if (n==1) then { old behavior } else { copy old code modify
> for n==2 }" is hard to maintain. There should be no copy and the old code
> shall be adapted to work for both n=1 and n>1.

Stacked mode serves as an extension of single device mode concerning data 
handling and CS line assertion. In both scenarios, the driver only asserts 
one CS line at any given time. The existing code has been expanded to 
determine the CS line to be asserted based on the requested address and 
data length. This modification accommodates both single (legacy) and 
stacked use cases.

In contrast, parallel mode differs from the legacy (single) mode in terms 
of data handling. In parallel mode, each byte of data is stored in both 
devices, with even bits in the lower flash and odd bits in the upper flash. 
During the transfer, multiple CS lines need to be asserted simultaneously. 
Consequently, special handling is necessary for parallel mode.

> 
> But I agree with Tudor, some kind of abstraction (layer) would be nice.

I agree too.

> 
> Also, you hardcode n=2 everywhere. Please generalize that one.
> 
> Are you aware of any other controller supporting such a feature? I've seen

Currently, I am familiar only with the AMD-Xilinx QSPI controllers that 
support parallel/stacked configurations and AMD-Xilinx OSPI controllers, 
which support stacked configuration. However, it's important to highlight 
certain aspects of these configurations. In parallel mode, each byte of 
data is stored in both flash devices, and the QSPI controller 
automatically handles the byte split and the simultaneous 
assertion/de-assertion of multiple CS lines. Hence, it can be stated that 
parallel operation is a controller feature, and other controllers wishing 
to operate flashes in parallel mode should be capable of data splitting 
and asserting multiple CS lines simultaneously. This characteristic might 
be specific to the AMD-Xilinx controller.

In contrast, in stacked mode, only one CS pin is asserted at any given 
time, determined by the memory address and the accessed data length. 
Stacked mode, unlike parallel mode, functions as a software abstraction.
Once implemented, any SPI controller with multiple CS lines or with a 
combination of native-CS and GPIO-CS can operate two or more flashes in 
stacked mode.

> you also need to modify the spi controller and intercept some commands.

Command interception occurs exclusively in parallel mode, not in stacked 
mode. In parallel mode, data must be split during flash memory read/write 
operations. However, during Flash register read/write operations, there 
should be no data split, as the identical data needs to be written to 
(or read from) the register of both flashes. Consequently, the driver has 
to intercept the command before activating the data split feature of the 
controller.

> Can everything be moved there?

In stacked mode, determining which flash device needs to be asserted is 
based on the flash address and the length of the requested data. This 
information is handled by the spi-nor core. If the operation spans across 
multiple flashes, the command, address, dummy (if required), and residual 
data must be issued to multiple flashes. This process should be carried 
out in the spi-nor core layer( or before spi-mem) and not in the driver.

 That is why > 
> I'm not sure we are implementing controller specific things in the core. Hard

As explained earlier the parallel mode of operation can be controller specific,
But the stacked mode is controller independent.

> to judge without seeing other controllers doing a similar thing. I'd like to avoid
> that.
> 
> If we had some kind of abstraction here, that might be easier to adapt in the
> future, but just putting everything into the core will make it really hard to
> maintain. So if everything related to stacked and parallel memory would be in
> drivers/mtd/spi-nor/stacked.c, we'd have at least everything in one place with
> a proper interface between that and the core.

I agree.

Regards
Amit
> 
> -michael