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: Wednesday, December 6, 2023 8:14 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/6/23 14:30, Tudor Ambarus wrote:
> > 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, wouldn't it be simpler to have just an array of flash sizes
> > instead of duplicating the entire params struct?
> >
> >>
> >> 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]);
> 
> also, it's not clear to me why you read this property multiple times.
> Have you sent a device tree patch somewhere? It will help me understand
> what you're trying to achieve.

Miquel submitted the device tree patch; here is the series.
https://lore.kernel.org/all/20220126112608.955728-1-miquel.raynal@xxxxxxxxxxx/

Regards,
Amit
> 
> >
> > 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.
> >
> > Cheers,
> > ta