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

[Mahapatra, Amit Kumar]

Hello Tudor,

> -----Original Message-----
> From: Tudor Ambarus <tudor.ambarus@xxxxxxxxxx>
> Sent: Tuesday, December 12, 2023 8:33 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 13:37, Mahapatra, Amit Kumar wrote:
> 
> Hi!
> 
> cut
> 
> >>>>>>>  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.
> 
> haven't yet read the thread from above, but I skimmed over the AMD
> controller datasheet.
> 
> >>
> >>> Following a series of discussions, the below bindings were
> >>> eventually merged.
> >>> https://lore.kernel.org/all/20220126112608.955728-4-miquel.raynal@bo
> >>> ot
> >>> 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-Contr
> >>> ol
> >>> 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-Inter
> > face-Diagrams
> 
> Thanks! Can you share with us what flashes you used for testing in the
> stacked and parallel configurations?

I used SPI-NOR QSPI flashes for testing stacked and parallel.

> 
> >> can combine a SPI NOR with a SPI NAND in stacked configuration?
> >
> > No, Xilinx/AMD QSPI controllers doesn't support this configuration.
> >
> 
> 2 SPI NANDs shall work with the AMD controller, right?

We never tested 2 SPI-NAND with AMD controller.
> 
> I skimmed over the QSPI controller datasheet and wondered why one would
> get complicated with 2 Quad Flashes when we have Octal. But then I saw that
> the same SoC can configure an Octal controller (the Octal and Quad
> controllers seems mutual exclusive) and that the Octal one can operate 2
> octal flashes in stacked mode :).

Thats correct .

Please let me know how you want me to proceed on this.

Regards,
Amit
> 
> Cheers,
> ta