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

[Mahapatra, Amit Kumar]

> -----Original Message-----
> From: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> Sent: Monday, December 11, 2023 3:05 PM
> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@xxxxxxx>;
> broonie@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
> Cc: linux-spi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> michael@xxxxxxxx; 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 07/10] mtd: spi-nor: Add stacked memories support
> in spi-nor
> 
> 
> 
> On 12/11/23 06:56, Mahapatra, Amit Kumar wrote:
> >
> >
> >> -----Original Message-----
> >> From: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> >> Sent: Monday, December 11, 2023 9:03 AM
> >> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@xxxxxxx>;
> >> broonie@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
> >> Cc: linux-spi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >> michael@xxxxxxxx; 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 07/10] mtd: spi-nor: Add stacked memories
> >> support in spi-nor
> >>
> >>
> >>
> >> On 12/8/23 17:05, Mahapatra, Amit Kumar wrote:
> >>> Hello Tudor,
> >>
> >> Hi!
> >
> > Hello Tudor,
> >
> 
> Hi!

Hello Tudor,
> 
> >>
> >>>
> >>>> -----Original Message-----
> >>>> From: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> >>>> Sent: Wednesday, December 6, 2023 8:00 PM
> >>>> To: Mahapatra, Amit Kumar <amit.kumar-mahapatra@xxxxxxx>;
> >>>> broonie@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
> >>>> Cc: linux-spi@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> >>>> michael@xxxxxxxx; 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 07/10] mtd: spi-nor: Add stacked memories
> >>>> support in spi-nor
> >>>>
> >>>> Hi, Amit,
> >>>>
> >>>> On 11/25/23 09:21, Amit Kumar Mahapatra wrote:
> >>>>> Each flash that is connected in stacked mode should have a
> >>>>> separate parameter structure. So, the flash parameter
> >>>>> member(*params) of the spi_nor structure is changed to an array
> >>>>> (*params[2]). The array is used to store the parameters of each
> >>>>> flash connected in stacked
> >>>> configuration.
> >>>>>
> >>>>> The current implementation assumes that a maximum of two flashes
> >>>>> are connected in stacked mode and both the flashes are of same
> >>>>> make but can differ in sizes. So, except the sizes all other flash
> >>>>> parameters of both the flashes are identical.
> >>>>
> >>>> Do you plan to add support for different flashes in stacked mode?
> >>>> If not,
> >>>
> >>> No, according to the current implementation, in stacked mode, both
> >>> flashes must be of the same make.
> >>>
> >>>> wouldn't it be simpler to have just an array of flash sizes instead
> >>>> of duplicating the entire params struct?
> >>>
> >>> Yes, that is accurate. In alignment with our current stacked support
> >>> use case we can have an array of flash sizes instead.
> >>> The primary purpose of having an array of params struct was to
> >>> facilitate potential future extensions, allowing the addition of
> >>> stacked support for different flashes
> >>>
> >>
> >> right. Don't do this change yet, let's decide on the overall architecture first.
> >
> > Sure.
> >>
> >>>>
> >>>>>
> >>>>> SPI-NOR is not aware of the chip_select values, for any incoming
> >>>>> request SPI-NOR will decide the flash index with the help of
> >>>>> individual flash size and the configuration type (single/stacked).
> >>>>> SPI-NOR will pass on the flash index information to the SPI core &
> >>>>> SPI driver by setting the appropriate bit in
> >>>>> nor->spimem->spi->cs_index_mask. For example, if nth bit of
> >>>>> nor->spimem->spi->cs_index_mask is set then the driver would
> >>>>> assert/de-assert spi->chip_slect[n].
> >>>>>
> >>>>> Signed-off-by: Amit Kumar Mahapatra <amit.kumar-
> >> mahapatra@xxxxxxx>
> >>>>> ---
> >>>>>  drivers/mtd/spi-nor/core.c  | 272
> >>>>> +++++++++++++++++++++++++++++-----
> >> --
> >>>>>  drivers/mtd/spi-nor/core.h  |   4 +
> >>>>>  include/linux/mtd/spi-nor.h |  15 +-
> >>>>>  3 files changed, 240 insertions(+), 51 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/mtd/spi-nor/core.c
> >>>>> b/drivers/mtd/spi-nor/core.c index 93ae69b7ff83..e990be7c7eb6
> >>>>> 100644
> >>>>> --- a/drivers/mtd/spi-nor/core.c
> >>>>> +++ b/drivers/mtd/spi-nor/core.c
> >>>>
> >>>> cut
> >>>>
> >>>>> @@ -2905,7 +3007,10 @@ static void spi_nor_init_fixup_flags(struct
> >>>>> spi_nor *nor)  static int spi_nor_late_init_params(struct spi_nor
> >>>>> *nor)  {
> >>>>>  	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor,
> >>>> 0);
> >>>>> -	int ret;
> >>>>> +	struct device_node *np = spi_nor_get_flash_node(nor);
> >>>>> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
> >>>>> +	u32 idx = 0;
> >>>>> +	int rc, ret;
> >>>>>
> >>>>>  	if (nor->manufacturer && nor->manufacturer->fixups &&
> >>>>>  	    nor->manufacturer->fixups->late_init) { @@ -2937,6 +3042,44
> >>>>> @@ static int spi_nor_late_init_params(struct spi_nor *nor)
> >>>>>  	if (params->n_banks > 1)
> >>>>>  		params->bank_size = div64_u64(params->size, params-
> >> n_banks);
> >>>>>
> >>>>> +	nor->num_flash = 0;
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * The flashes that are connected in stacked mode should be
> of
> >>>>> +same
> >>>> make.
> >>>>> +	 * Except the flash size all other properties are identical for all
> the
> >>>>> +	 * flashes connected in stacked mode.
> >>>>> +	 * The flashes that are connected in parallel mode should be
> identical.
> >>>>> +	 */
> >>>>> +	while (idx < SNOR_FLASH_CNT_MAX) {
> >>>>> +		rc = of_property_read_u64_index(np, "stacked-
> memories",
> >>>> idx,
> >>>>> +&flash_size[idx]);
> >>>>
> >>>> This is a little late in my opinion, as we don't have any sanity
> >>>> check on the flashes that are stacked on top of the first. We shall
> >>>> at least read and compare the ID for all.
> >>>
> >>> Alright, I will incorporate a sanity check for reading and comparing
> >>> the ID of the stacked flash. Subsequently, I believe this stacked
> >>> logic should be relocated to spi_nor_get_flash_info() where we
> >>> identify the first flash. Please share your thoughts on this.
> >>> Additionally, do you
> >>
> >> I'm wondering whether we can add a layer on top of the flash type to
> >> handle
> >
> > When you mention "on top," are you referring to incorporating it into
> > the MTD layer? Initially, Miquel had submitted this patch to address
> 
> I mean something above SPI MEM flashes, be it NOR, NANDs or whatever.
> Instead of treating the stacked flashes as a monolithic device and treat in SPI
> NOR some array of flashes, to have a layer above which probes the SPI MEM
> flash driver for each stacked flash. In your case SPI NOR would be probed
> twice, as you use 2 SPI NOR flashes.
> 
> > stacked/parallel handling in the MTD layer.
> > https://lore.kernel.org/linux-mtd/20200114191052.0a16d116@xps13/t/
> > However, the Device Tree bindings were initially not accepted.
> 
> Okay, thanks for the pointer. I'll take a look.
> 
> > Following a series of discussions, the below bindings were eventually
> > merged.
> > https://lore.kernel.org/all/20220126112608.955728-4-miquel.raynal@boot
> > lin.com/
> 
> I saw, thanks.
> 
> >
> >> the stacked/parallel modes. This way everything will become flash
> >> type independent. Would it be possible to stack 2 SPI NANDs? How
> >> about a SPI NOR and a SPI NAND?
> >>
> >> Is the datasheet of the controller public?
> >
> > Yes,
> > https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/Quad-SPI-Control
> > ler
> >
> 
> Wonderful, I'll take a look. I see two clocks there. Does this mean that the
> stacked flashes can be operated at different frequencies? Do you know if we

In stacked configuration, both flashes utilize a common clock (single 
clock line). In a parallel setup, the flashes share the same clock but 
have distinct clock lines. Please refer the diagrams in the sections 
below for reference.
https://docs.xilinx.com/r/en-US/am011-versal-acap-trm/QSPI-Flash-Interface-Diagrams
> can combine a SPI NOR with a SPI NAND in stacked configuration?

No, Xilinx/AMD QSPI controllers doesn't support this configuration.

Regards,
Amit
> 
> I need to study this a bit. I'll try to involve Michael and Pratyush too.
> 
> Cheers,
> ta