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

["Mahapatra, Amit Kumar" <amit.kumar-mahapatra@xxxxxxx>]

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. For parallel configurations, there are other controllers from 
Microchip and some flash devices that operate similarly to AMD's parallel 
mode.

> 
> > 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 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.

> 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.

> 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.

Agree

Regards,
Amit

> 
> 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