Re: [PATCH v2 10/13] mtd: spi-nor: Add stacked memories support in spi-nor

[Michal Simek <michal.simek@amd.com>]

On 1/19/23 19:53, 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.
> 
> 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@amd.com>
> ---
>   drivers/mtd/spi-nor/core.c  | 282 +++++++++++++++++++++++++++++-------
>   drivers/mtd/spi-nor/core.h  |   4 +
>   drivers/mtd/spi-nor/otp.c   |   4 +-
>   include/linux/mtd/spi-nor.h |  12 +-
>   4 files changed, 246 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 8a4a54bf2d0e..bb7326dc8b70 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1441,13 +1441,18 @@ static int spi_nor_erase_multi_sectors(struct spi_nor *nor, u64 addr, u32 len)
>   static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>   {
>   	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> -	u32 addr, len;
> +	struct spi_nor_flash_parameter *params;
> +	u32 addr, len, offset, cur_cs_num = 0;
>   	uint32_t rem;
>   	int ret;
> +	u64 sz;
>   
>   	dev_dbg(nor->dev, "at 0x%llx, len %lld\n", (long long)instr->addr,
>   			(long long)instr->len);
>   
> +	params = spi_nor_get_params(nor, 0);
> +	sz = params->size;
> +
>   	if (spi_nor_has_uniform_erase(nor)) {
>   		div_u64_rem(instr->len, mtd->erasesize, &rem);
>   		if (rem)
> @@ -1465,26 +1470,30 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>   	if (len == mtd->size && !(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
>   		unsigned long timeout;
>   
> -		ret = spi_nor_write_enable(nor);
> -		if (ret)
> -			goto erase_err;
> +		while (cur_cs_num < SNOR_FLASH_CNT_MAX && params) {
> +			nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
> +			ret = spi_nor_write_enable(nor);
> +			if (ret)
> +				goto erase_err;
>   
> -		ret = spi_nor_erase_chip(nor);
> -		if (ret)
> -			goto erase_err;
> +			ret = spi_nor_erase_chip(nor);
> +			if (ret)
> +				goto erase_err;
>   
> -		/*
> -		 * Scale the timeout linearly with the size of the flash, with
> -		 * a minimum calibrated to an old 2MB flash. We could try to
> -		 * pull these from CFI/SFDP, but these values should be good
> -		 * enough for now.
> -		 */
> -		timeout = max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
> -			      CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
> -			      (unsigned long)(mtd->size / SZ_2M));
> -		ret = spi_nor_wait_till_ready_with_timeout(nor, timeout);
> -		if (ret)
> -			goto erase_err;
> +			/*
> +			 * Scale the timeout linearly with the size of the flash, with
> +			 * a minimum calibrated to an old 2MB flash. We could try to
> +			 * pull these from CFI/SFDP, but these values should be good
> +			 * enough for now.
> +			 */
> +			timeout = max(CHIP_ERASE_2MB_READY_WAIT_JIFFIES,
> +				      CHIP_ERASE_2MB_READY_WAIT_JIFFIES *
> +				      (unsigned long)(params->size / SZ_2M));
> +			ret = spi_nor_wait_till_ready_with_timeout(nor, timeout);
> +			if (ret)
> +				goto erase_err;
> +			cur_cs_num++;
> +		}
>   
>   	/* REVISIT in some cases we could speed up erasing large regions
>   	 * by using SPINOR_OP_SE instead of SPINOR_OP_BE_4K.  We may have set up
> @@ -1493,12 +1502,26 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>   
>   	/* "sector"-at-a-time erase */
>   	} else if (spi_nor_has_uniform_erase(nor)) {
> +		/* Determine the flash from which the operation need to start */
> +		while ((cur_cs_num < SNOR_FLASH_CNT_MAX) && (addr > sz - 1) && params) {
> +			cur_cs_num++;
> +			params = spi_nor_get_params(nor, cur_cs_num);
> +			sz += params->size;
> +		}
> +
>   		while (len) {
> +			nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
>   			ret = spi_nor_write_enable(nor);
>   			if (ret)
>   				goto erase_err;
>   
> -			ret = spi_nor_erase_sector(nor, addr);
> +			offset = addr;
> +			if (nor->flags & SNOR_F_HAS_STACKED) {
> +				params = spi_nor_get_params(nor, cur_cs_num);
> +				offset -= (sz - params->size);
> +			}
> +
> +			ret = spi_nor_erase_sector(nor, offset);
>   			if (ret)
>   				goto erase_err;
>   
> @@ -1508,13 +1531,45 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>   
>   			addr += mtd->erasesize;
>   			len -= mtd->erasesize;
> +
> +			/*
> +			 * Flash cross over condition in stacked mode.
> +			 */
> +			if ((nor->flags & SNOR_F_HAS_STACKED) && (addr > sz - 1)) {
> +				cur_cs_num++;
> +				params = spi_nor_get_params(nor, cur_cs_num);
> +				sz += params->size;
> +			}
>   		}
>   
>   	/* erase multiple sectors */
>   	} else {
> -		ret = spi_nor_erase_multi_sectors(nor, addr, len);
> -		if (ret)
> -			goto erase_err;
> +		u64 erase_len = 0;
> +
> +		/* Determine the flash from which the operation need to start */
> +		while ((cur_cs_num < SNOR_FLASH_CNT_MAX) && (addr > sz - 1) && params) {
> +			cur_cs_num++;
> +			params = spi_nor_get_params(nor, cur_cs_num);
> +			sz += params->size;
> +		}
> +		/* perform multi sector erase onec per Flash*/
> +		while (len) {
> +			erase_len = (len > (sz - addr)) ? (sz - addr) : len;
> +			offset = addr;
> +			nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
> +			if (nor->flags & SNOR_F_HAS_STACKED) {
> +				params = spi_nor_get_params(nor, cur_cs_num);
> +				offset -= (sz - params->size);
> +			}
> +			ret = spi_nor_erase_multi_sectors(nor, offset, erase_len);
> +			if (ret)
> +				goto erase_err;
> +			len -= erase_len;
> +			addr += erase_len;
> +			cur_cs_num++;
> +			params = spi_nor_get_params(nor, cur_cs_num);
> +			sz += params->size;
> +		}
>   	}
>   
>   	ret = spi_nor_write_disable(nor);
> @@ -1713,7 +1768,10 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>   			size_t *retlen, u_char *buf)
>   {
>   	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> -	ssize_t ret;
> +	struct spi_nor_flash_parameter *params;
> +	ssize_t ret, read_len;
> +	u32 cur_cs_num = 0;
> +	u64 sz;
>   
>   	dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);
>   
> @@ -1721,9 +1779,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>   	if (ret)
>   		return ret;
>   
> +	params = spi_nor_get_params(nor, 0);
> +	sz = params->size;
> +
> +	/* Determine the flash from which the operation need to start */
> +	while ((cur_cs_num < SNOR_FLASH_CNT_MAX) && (from > sz - 1) && params) {
> +		cur_cs_num++;
> +		params = spi_nor_get_params(nor, cur_cs_num);
> +		sz += params->size;
> +	}
>   	while (len) {
>   		loff_t addr = from;
>   
> +		nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
> +		read_len = (len > (sz - addr)) ? (sz - addr) : len;
> +		params = spi_nor_get_params(nor, cur_cs_num);
> +		addr -= (sz - params->size);
> +
>   		addr = spi_nor_convert_addr(nor, addr);
>   
>   		ret = spi_nor_read_data(nor, addr, len, buf);
> @@ -1735,11 +1807,22 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
>   		if (ret < 0)
>   			goto read_err;
>   
> -		WARN_ON(ret > len);
> +		WARN_ON(ret > read_len);
>   		*retlen += ret;
>   		buf += ret;
>   		from += ret;
>   		len -= ret;
> +
> +		/*
> +		 * Flash cross over condition in stacked mode.
> +		 *
> +		 */
> +		if ((nor->flags & SNOR_F_HAS_STACKED) && (from > sz - 1)) {
> +			cur_cs_num++;
> +			params = spi_nor_get_params(nor, cur_cs_num);
> +			sz += params->size;
> +		}
> +
>   	}
>   	ret = 0;
>   
> @@ -1759,13 +1842,22 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>   	struct spi_nor *nor = mtd_to_spi_nor(mtd);
>   	struct spi_nor_flash_parameter *params;
>   	size_t page_offset, page_remain, i;
> +	u32 page_size, cur_cs_num = 0;
>   	ssize_t ret;
> -	u32 page_size;
> +	u64 sz;
>   
>   	dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len);
>   
>   	params = spi_nor_get_params(nor, 0);
>   	page_size = params->page_size;
> +	sz = params->size;
> +
> +	/* Determine the flash from which the operation need to start */
> +	while ((cur_cs_num < SNOR_FLASH_CNT_MAX) && (to > sz - 1) && params) {
> +		cur_cs_num++;
> +		params = spi_nor_get_params(nor, cur_cs_num);
> +		sz += params->size;
> +	}
>   
>   	ret = spi_nor_lock_and_prep(nor);
>   	if (ret)
> @@ -1790,6 +1882,10 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>   		/* the size of data remaining on the first page */
>   		page_remain = min_t(size_t, page_size - page_offset, len - i);
>   
> +		nor->spimem->spi->cs_index_mask = 0x01 << cur_cs_num;
> +		params = spi_nor_get_params(nor, cur_cs_num);
> +		addr -= (sz - params->size);
> +
>   		addr = spi_nor_convert_addr(nor, addr);
>   
>   		ret = spi_nor_write_enable(nor);
> @@ -1806,6 +1902,15 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
>   			goto write_err;
>   		*retlen += written;
>   		i += written;
> +
> +		/*
> +		 * Flash cross over condition in stacked mode.
> +		 */
> +		if ((nor->flags & SNOR_F_HAS_STACKED) && ((to + i) > sz - 1)) {
> +			cur_cs_num++;
> +			params = spi_nor_get_params(nor, cur_cs_num);
> +			sz += params->size;
> +		}
>   	}
>   
>   write_err:
> @@ -1918,8 +2023,6 @@ int spi_nor_hwcaps_pp2cmd(u32 hwcaps)
>   static int spi_nor_spimem_check_op(struct spi_nor *nor,
>   				   struct spi_mem_op *op)
>   {
> -	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
> -
>   	/*
>   	 * First test with 4 address bytes. The opcode itself might
>   	 * be a 3B addressing opcode but we don't care, because
> @@ -1928,7 +2031,7 @@ static int spi_nor_spimem_check_op(struct spi_nor *nor,
>   	 */
>   	op->addr.nbytes = 4;
>   	if (!spi_mem_supports_op(nor->spimem, op)) {
> -		if (params->size > SZ_16M)
> +		if (nor->mtd.size > SZ_16M)
>   			return -EOPNOTSUPP;
>   
>   		/* If flash size <= 16MB, 3 address bytes are sufficient */
> @@ -2516,6 +2619,10 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
>   static void spi_nor_late_init_params(struct spi_nor *nor)
>   {
>   	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
> +	struct device_node *np = spi_nor_get_flash_node(nor);
> +	u64 flash_size[SNOR_FLASH_CNT_MAX];
> +	u32 idx = 0, i = 0;
> +	int rc;
>   
>   	if (nor->manufacturer && nor->manufacturer->fixups &&
>   	    nor->manufacturer->fixups->late_init)
> @@ -2533,6 +2640,36 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
>   	 */
>   	if (nor->flags & SNOR_F_HAS_LOCK && !params->locking_ops)
>   		spi_nor_init_default_locking_ops(nor);
> +	/*
> +	 * 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 (i < SNOR_FLASH_CNT_MAX) {
> +		rc = of_property_read_u64_index(np, "stacked-memories", idx, &flash_size[i]);
> +		if (rc == -EINVAL) {
> +			break;
> +		} else if (rc == -EOVERFLOW) {
> +			idx++;
> +		} else {
> +			idx++;
> +			i++;
> +			if (!(nor->flags & SNOR_F_HAS_STACKED))
> +				nor->flags |= SNOR_F_HAS_STACKED;
> +		}
> +	}
> +	if (nor->flags & SNOR_F_HAS_STACKED) {
> +		for (idx = 1; idx < SNOR_FLASH_CNT_MAX; idx++) {
> +			params = spi_nor_get_params(nor, idx);
> +			params = devm_kzalloc(nor->dev, sizeof(*params), GFP_KERNEL);
> +			if (params) {
> +				memcpy(params, spi_nor_get_params(nor, 0), sizeof(*params));
> +				params->size = flash_size[idx];
> +				spi_nor_set_params(nor, idx, params);
> +			}
> +		}
> +	}
>   }
>   
>   /**
> @@ -2741,22 +2878,36 @@ static int spi_nor_octal_dtr_enable(struct spi_nor *nor, bool enable)
>    */
>   static int spi_nor_quad_enable(struct spi_nor *nor)
>   {
> -	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
> +	struct spi_nor_flash_parameter *params;
> +	int err, idx;
>   
> -	if (!params->quad_enable)
> -		return 0;
> +	for (idx = 0; idx < SNOR_FLASH_CNT_MAX; idx++) {
> +		params = spi_nor_get_params(nor, idx);
> +		if (params) {
> +			if (!params->quad_enable)
> +				return 0;
>   
> -	if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
> -	      spi_nor_get_protocol_width(nor->write_proto) == 4))
> -		return 0;
> +			if (!(spi_nor_get_protocol_width(nor->read_proto) == 4 ||
> +			      spi_nor_get_protocol_width(nor->write_proto) == 4))
> +				return 0;
> +			/*
> +			 * Set the appropriate CS index before
> +			 * issuing the command.
> +			 */
> +			nor->spimem->spi->cs_index_mask = 0x01 << idx;
>   
> -	return params->quad_enable(nor);
> +			err = params->quad_enable(nor);
> +			if (err)
> +				return err;
> +		}
> +	}
> +	return err;
>   }
>   
>   static int spi_nor_init(struct spi_nor *nor)
>   {
> -	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
> -	int err;
> +	struct spi_nor_flash_parameter *params;
> +	int err, idx;
>   
>   	err = spi_nor_octal_dtr_enable(nor, true);
>   	if (err) {
> @@ -2797,9 +2948,19 @@ static int spi_nor_init(struct spi_nor *nor)
>   		 */
>   		WARN_ONCE(nor->flags & SNOR_F_BROKEN_RESET,
>   			  "enabling reset hack; may not recover from unexpected reboots\n");
> -		err = params->set_4byte_addr_mode(nor, true);
> -		if (err && err != -ENOTSUPP)
> -			return err;
> +		for (idx = 0; idx < SNOR_FLASH_CNT_MAX; idx++) {
> +			params = spi_nor_get_params(nor, idx);
> +			if (params) {
> +				/*
> +				 * Select the appropriate CS index before
> +				 * issuing the command.
> +				 */
> +				nor->spimem->spi->cs_index_mask = 0x01 << idx;
> +				err = params->set_4byte_addr_mode(nor, true);
> +				if (err && err != -ENOTSUPP)
> +					return err;
> +			}
> +		}
>   	}
>   
>   	return 0;
> @@ -2915,19 +3076,31 @@ void spi_nor_restore(struct spi_nor *nor)
>   {
>   	struct spi_nor_flash_parameter *params;
>   	int ret;
> +	int idx;
>   
>   	/* restore the addressing mode */
>   	if (nor->addr_nbytes == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
>   	    nor->flags & SNOR_F_BROKEN_RESET) {
> -		params = spi_nor_get_params(nor, 0);
> -		ret = params->set_4byte_addr_mode(nor, false);
> -		if (ret)
> -			/*
> -			 * Do not stop the execution in the hope that the flash
> -			 * will default to the 3-byte address mode after the
> -			 * software reset.
> -			 */
> -			dev_err(nor->dev, "Failed to exit 4-byte address mode, err = %d\n", ret);
> +		for (idx = 0; idx < SNOR_FLASH_CNT_MAX; idx++) {
> +			params = spi_nor_get_params(nor, idx);
> +			if (params) {
> +				/*
> +				 * Select the appropriate CS index before
> +				 * issuing the command.
> +				 */
> +				nor->spimem->spi->cs_index_mask = 0x01 << idx;
> +				ret = params->set_4byte_addr_mode(nor, false);
> +				if (ret)
> +					/*
> +					 * Do not stop the execution in the hope that the flash
> +					 * will default to the 3-byte address mode after the
> +					 * software reset.
> +					 */
> +					dev_err(nor->dev,
> +						"Failed to exit 4-byte address mode, err = %d\n",
> +						ret);
> +			}
> +		}
>   	}
>   
>   	if (nor->flags & SNOR_F_SOFT_RESET)
> @@ -2995,6 +3168,8 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
>   	struct spi_nor_flash_parameter *params = spi_nor_get_params(nor, 0);
>   	struct mtd_info *mtd = &nor->mtd;
>   	struct device *dev = nor->dev;
> +	u64 total_sz = 0;
> +	int idx;
>   
>   	spi_nor_set_mtd_locking_ops(nor);
>   	spi_nor_set_mtd_otp_ops(nor);
> @@ -3010,7 +3185,12 @@ static void spi_nor_set_mtd_info(struct spi_nor *nor)
>   		mtd->_erase = spi_nor_erase;
>   	mtd->writesize = params->writesize;
>   	mtd->writebufsize = params->page_size;
> -	mtd->size = params->size;
> +	for (idx = 0; idx < SNOR_FLASH_CNT_MAX; idx++) {
> +		params = spi_nor_get_params(nor, idx);
> +		if (params)
> +			total_sz += params->size;
> +	}
> +	mtd->size = total_sz;
>   	mtd->_read = spi_nor_read;
>   	/* Might be already set by some SST flashes. */
>   	if (!mtd->_write)
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index f03b55cf7e6f..e94107cc465e 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -11,6 +11,9 @@
>   
>   #define SPI_NOR_MAX_ID_LEN	6
>   
> +/* In single configuration enable CS0 */
> +#define SPI_NOR_ENABLE_CS0     BIT(0)
> +
>   /* Standard SPI NOR flash operations. */
>   #define SPI_NOR_READID_OP(naddr, ndummy, buf, len)			\
>   	SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_RDID, 0),			\
> @@ -130,6 +133,7 @@ enum spi_nor_option_flags {
>   	SNOR_F_IO_MODE_EN_VOLATILE = BIT(11),
>   	SNOR_F_SOFT_RESET	= BIT(12),
>   	SNOR_F_SWP_IS_VOLATILE	= BIT(13),
> +	SNOR_F_HAS_STACKED      = BIT(14),
>   };
>   
>   struct spi_nor_read_command {
> diff --git a/drivers/mtd/spi-nor/otp.c b/drivers/mtd/spi-nor/otp.c
> index a9c0844d55ef..b8c7e085a90d 100644
> --- a/drivers/mtd/spi-nor/otp.c
> +++ b/drivers/mtd/spi-nor/otp.c
> @@ -11,8 +11,8 @@
>   
>   #include "core.h"
>   
> -#define spi_nor_otp_region_len(nor) ((nor)->params->otp.org->len)
> -#define spi_nor_otp_n_regions(nor) ((nor)->params->otp.org->n_regions)
> +#define spi_nor_otp_region_len(nor) ((nor)->params[0]->otp.org->len)
> +#define spi_nor_otp_n_regions(nor) ((nor)->params[0]->otp.org->n_regions)

I think this should be also converted to static inline functions and use
spi_nor_get_params(nor, 0); instead of using arrays.
Can be done on the top of this series if it is the only one problem with it.

Thanks,
Michal