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

[Marek Vasut <marex@denx.de>]

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 .

> +		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 ?