Message-ID:
 <IA0PR12MB7699670B7EE37C60C672FC5EDC872@IA0PR12MB7699.namprd12.prod.outlook.com>
Date: Wed, 14 Aug 2024 12:53:49 +0000
From: "Mahapatra, Amit Kumar" <amit.kumar-mahapatra@....com>
To: Miquel Raynal <miquel.raynal@...tlin.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

Hello Miquel,

> That's not what was initially discussed. The Xilinx use case was:
> a controller is managing two devices "at the same time" transparently from

Yes, but in stacked mode, the controller communicates with one of the two 
connected flash devices at any given time, based on the address and data 
length. It doesn't assert both chip selects simultaneously.

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

When adding stacked support to the SPI core, Mark also asked us to support 
the GPIO chip select use case, so it is not only restricted to native cs.

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

Stacked mode is a software abstraction rather than a controller feature or 
capability. At any given time, the controller communicates with one of the 
two connected flash devices, as determined by the requested address and data 
length. If an operation starts on one flash and ends on the other, the core 
needs to split it into two separate operations and adjust the data length 
accordingly.

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

As stated earlier stacked is not a controller feature but rather a software 
abstraction to assert the appropriate cs as per the requested address & data 
length.

Regards,
Amit
> 
> 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

Powered by blists - more mailing lists