[PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[Abbarapu, Venkatesh]

> -----Original Message-----
> From: Marek Vasut <marex at denx.de>
> Sent: Monday, December 23, 2024 10:22 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>; u-boot at lists.denx.de;
> tudor.ambarus at linaro.org; j-humphreys at ti.com
> Cc: Simek, Michal <michal.simek at amd.com>; jagan at amarulasolutions.com;
> vigneshr at ti.com; u-kumar1 at ti.com; trini at konsulko.com; seanga2 at gmail.com;
> caleb.connolly at linaro.org; sjg at chromium.org; william.zhang at broadcom.com;
> stefan_b at posteo.net; quentin.schulz at cherry.de; Takahiro.Kuwano at infineon.com;
> p-mantena at ti.com; git (AMD-Xilinx) <git at amd.com>
> Subject: Re: [PATCH v3] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
> 
> On 12/20/24 3:30 PM, Venkatesh Yadav Abbarapu wrote:
> 
> [...]
> 
> > @@ -1578,8 +1580,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from,
> size_t len,
> >   	struct spi_nor *nor = mtd_to_spi_nor(mtd);
> >   	int ret;
> >   	loff_t offset = from;
> > -	u32 read_len = 0;
> >   	u32 rem_bank_len = 0;
> > +	u32 stack_shift = 0;
> >   	u8 bank;
> >   	bool is_ofst_odd = false;
> >
> > @@ -1593,18 +1595,23 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t
> from, size_t len,
> >   	}
> >
> >   	while (len) {
> > -		bank = (u32)from / SZ_16M;
> > -		if (nor->flags & SNOR_F_HAS_PARALLEL)
> > -			bank /= 2;
> > -
> > -		rem_bank_len = SZ_16M * (bank + 1);
> > -		if (nor->flags & SNOR_F_HAS_PARALLEL)
> > -			rem_bank_len *= 2;
> > -		rem_bank_len -= from;
> > -
> > +		size_t read_len = len;
> 
> size_t read_len should be declared at the beginning of this function.
> 
> >   		offset = from;
> >
> > +	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> 
> The indent here needs to be pushed on tab RIGHT -> .
> 
> Do you need this functionality in SPL ? If so, you might need a matching Kconfig
> SPL_SPI_STACKED_PARALLEL symbol .

This functionality is not needed for SPL.

> 
> > +		if (nor->addr_width == 3) {
> > +			bank = (u32)from / SZ_16M;
> > +			if (nor->flags & SNOR_F_HAS_PARALLEL)
> > +				bank /= 2;
> > +
> > +			rem_bank_len = SZ_16M * (bank + 1);
> > +			if (nor->flags & SNOR_F_HAS_PARALLEL)
> > +				rem_bank_len *= 2;
> > +			rem_bank_len -= from;
> > +		}
> > +
> >   		if (nor->flags & SNOR_F_HAS_STACKED) {
> > +			stack_shift = 1;
> >   			if (offset >= (mtd->size / 2)) {
> >   				offset = offset - (mtd->size / 2);
> >   				nor->spi->flags |= SPI_XFER_U_PAGE; @@ -
> 1613,20 +1620,31 @@
> > static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
> >   			}
> >   		}
> >
> > +		if (nor->addr_width == 4)
> > +			rem_bank_len = (mtd->size >> stack_shift) - offset;
> > +
> >   		if (nor->flags & SNOR_F_HAS_PARALLEL)
> >   			offset /= 2;
> > +	}
> >
> >   #ifdef CONFIG_SPI_FLASH_BAR
> > +		u32 remain_len;
> > +
> >   		ret = write_bar(nor, offset);
> >   		if (ret < 0)
> >   			return log_ret(ret);
> > -#endif
> > -
> > -		if (len < rem_bank_len)
> > +		remain_len = (SZ_16M * (nor->bank_curr + 1)) - offset;
> 
> Is this calculation correct?
> 
> It used to be calculated as:
> 
> bank = (u32)from / SZ_16M;
> rem_bank_len = SZ_16M * (bank + 1);
> rem_bank_len -= from;
> 
> Now the bank is calculated as:
> 
> bank = nor->bank_curr + 1;
> 
> ?
> 
> Why not keep calculating bank from offset ?
> 
> Maybe this will work ?
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index
> 691962ef271..bdfbd44f30c 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1578,10 +1578,11 @@ 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);
> -	int ret;
>   	loff_t offset = from;
>   	u32 rem_bank_len = 0;
>   	u32 stack_shift = 0;
> +	size_t read_len;
> +	int ret;
>   	u8 bank;
>   	bool is_ofst_odd = false;
> 
> @@ -1595,7 +1596,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from,
> size_t len,
>   	}
> 
>   	while (len) {
> -		size_t read_len = len;
> +		read_len = len;
>   		offset = from;
> 
>   		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) { @@ -
> 1627,24 +1628,24 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from,
> size_t len,
>   				offset /= 2;
>   		}
> 
> -#ifdef CONFIG_SPI_FLASH_BAR
> -		u32 remain_len;
> +		if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {
> +			bank = (u32)from / SZ_16M;
> +			rem_bank_len = SZ_16M * (bank + 1);
> +			rem_bank_len -= from;
> 
>   			ret = write_bar(nor, offset);
>   			if (ret < 0)
>   				return log_ret(ret);
> -		remain_len = (SZ_16M * (nor->bank_curr + 1)) - offset;
> -		if (len < remain_len)
> -			read_len = len;
> -		else
> -			read_len = remain_len;
> -#endif
> -		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> +		}
> +
> +		if (CONFIG_IS_ENABLED(SPI_FLASH_BAR) ||
> +		    CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
>   			if (len < rem_bank_len)
>   				read_len = len;
>   			else
>   				read_len = rem_bank_len;
>   		}
> +
>   		if (read_len == 0)
>   			return -EIO;
> 
> I am still worried about using stacked parallel and BAR together, is that even
> possible or are they mutually exclusive ?

Will check the above implementation and update the patch.
FLASH_BAR and stacked parallel configuration doesn’t depend on each other.

Thanks
Venkatesh