Re: [PATCH v11 07/10] mtd: spi-nor: Add stacked memories support in spi-nor

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

Hi Amit,

amit.kumar-mahapatra@amd.com wrote on Wed, 14 Aug 2024 07:13:35 +0000:

> Hello Miquel,
> 
> > > 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.  
> 
> IMHO, there should be a general solution since this isn't limited to just 
> one controller. Any controller can support stacked mode with multiple 
> native-cs or multiple gpio-cs, or with a combination of a native-cs and a 
> gpio-cs.

That's not what was initially discussed. The Xilinx use case was:
a controller is managing two devices "at the same time" transparently
from the host. So the two flashes appear like a single one and thus,
are described like a single one.

You don't need anything in the bindings nor in the core to manage two
flashes connected to the same controller otherwise. The only use case
the Xilinx model was bringing, was to consider the storage bigger from
a host perspective and thus be able to store files across the device
boundary natively.

> For parallel configurations, there are other controllers from 
> Microchip and some flash devices that operate similarly to AMD's parallel 
> mode.

Yes, Tudor reminded me about these.

> > > 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?  
> 
> The current bindings are effective if we only support identical 
> flash devices or flashes of the same make but with different sizes 
> connected in stacked mode. However, if we want to extend stacked support 
> to include flashes of different makes in stacked mode,

The only actual feature the stacked mode brings is the ability to
consider two devices like one. This is abstracted by hardware, this is
a controller capability. The only way this can work is if the two
storage devices are of the same kind and accept the same commands/init
cycles.

If you consider two different devices, then there is no hardware
abstraction anymore, the controller is no longer "smart" enough and you
are back to the standard situation with two devices connected using
their own independent CS, known by the host. In this case the
"stacked-mode" bindings no longer apply. You need to describe the two
chips independently in the DT, and your stacked feature in the
controller can no longer be used.

You need other bindings to support this case because it is a different
situation. For this case, there was a mtd-concat approach (which was
never merged). But this is really something different than the stacked
mode in your controller because there is no specific hardware feature
involved, it's just pure software.

> the current 
> bindings may not be adequate. That's why I suggested updating the bindings 
> to accommodate all possible scenario.
> 
> >   
> > > 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.  
> 
> According to the new binding proposal, one of the two flash nodes will 
> have a partition that crosses the device boundary.

From a bindings perspective, it feels very awkward and I doubt it will
be accepted. From a code perspective, you're gonna need to butcher the
core...

> > 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.  
> 
> That's true, but as I mentioned earlier, if we're not inclined to support 
> stacked mode for flashes of different makes, then the current bindings 
> are sufficient.

Thanks,
Miquèl