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

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

[Venkatesh Yadav Abbarapu]

Update the spi_nor_read() function only for parallel configuration
when config SPI_STACKED_PARALLEL is enabled and keep the default
code as is. For parallel configurations fix the read issue for
4byte address width by passing the entire length to the read
function, split the memory of 16MB size banks only when the
address width is 3byte. Also update the size when the
configuration is stacked.

Fixes: 5d40b3d384 ("mtd: spi-nor: Add parallel and stacked memories support")
Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
---
Changes in v2:
- Update the nor read for parallel configuration and for other case
  keep the code as is.
- Fixed the commit description.
- Tested the changes with s25fl128s flash
Zynq> sf probe 0 0 0
SF: Detected s25fl128s with page size 256 Bytes, erase size 64 KiB, total 16 MiB
Zynq> time sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;time sf write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;time sf read 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x1000000
SF: 16777216 bytes @ 0x0 Erased: OK

time: 32.464 seconds
device 0 whole chip
SF: 16777216 bytes @ 0x0 Written: OK

time: 15.515 seconds
device 0 whole chip
SF: 16777216 bytes @ 0x0 Read: OK

time: 1.557 seconds
Total of 16777216 byte(s) were the same
---
 drivers/mtd/spi/spi-nor-core.c | 50 ++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index ec841fb13b..8d7087b5c9 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 				nor->spi->flags &= ~SPI_XFER_U_PAGE;
 		}
 #ifdef CONFIG_SPI_FLASH_BAR
-		ret = write_bar(nor, addr);
+		ret = write_bar(nor, offset);
 		if (ret < 0)
 			goto erase_err;
 #endif
@@ -1152,7 +1152,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 		    !(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
 			ret = spi_nor_erase_chip(nor);
 		} else {
-			ret = spi_nor_erase_sector(nor, addr);
+			ret = spi_nor_erase_sector(nor, offset);
 		}
 		if (ret < 0)
 			goto erase_err;
@@ -1578,8 +1578,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 +1593,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;
 		offset = from;
 
+	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
+		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 +1618,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;
+		if (len < remain_len)
 			read_len = len;
 		else
-			read_len = rem_bank_len;
-
+			read_len = remain_len;
+#endif
+		if (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;
 
-- 
2.34.1

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

[Jon Humphreys]

Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com> writes:

> Update the spi_nor_read() function only for parallel configuration
> when config SPI_STACKED_PARALLEL is enabled and keep the default
> code as is. For parallel configurations fix the read issue for
> 4byte address width by passing the entire length to the read
> function, split the memory of 16MB size banks only when the
> address width is 3byte. Also update the size when the
> configuration is stacked.
>
> Fixes: 5d40b3d384 ("mtd: spi-nor: Add parallel and stacked memories support")
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> ---
> Changes in v2:
> - Update the nor read for parallel configuration and for other case
>   keep the code as is.
> - Fixed the commit description.
> - Tested the changes with s25fl128s flash
> Zynq> sf probe 0 0 0
> SF: Detected s25fl128s with page size 256 Bytes, erase size 64 KiB, total 16 MiB
> Zynq> time sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;time sf write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;time sf read 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x1000000
> SF: 16777216 bytes @ 0x0 Erased: OK
>
> time: 32.464 seconds
> device 0 whole chip
> SF: 16777216 bytes @ 0x0 Written: OK
>
> time: 15.515 seconds
> device 0 whole chip
> SF: 16777216 bytes @ 0x0 Read: OK
>
> time: 1.557 seconds
> Total of 16777216 byte(s) were the same
> ---
>  drivers/mtd/spi/spi-nor-core.c | 50 ++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index ec841fb13b..8d7087b5c9 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>  				nor->spi->flags &= ~SPI_XFER_U_PAGE;
>  		}
>  #ifdef CONFIG_SPI_FLASH_BAR
> -		ret = write_bar(nor, addr);
> +		ret = write_bar(nor, offset);
>  		if (ret < 0)
>  			goto erase_err;
>  #endif
> @@ -1152,7 +1152,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>  		    !(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
>  			ret = spi_nor_erase_chip(nor);
>  		} else {
> -			ret = spi_nor_erase_sector(nor, addr);
> +			ret = spi_nor_erase_sector(nor, offset);
>  		}
>  		if (ret < 0)
>  			goto erase_err;
> @@ -1578,8 +1578,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 +1593,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;
>  		offset = from;
>  
> +	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> +		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 +1618,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;
> +		if (len < remain_len)
>  			read_len = len;
>  		else
> -			read_len = rem_bank_len;
> -
> +			read_len = remain_len;
> +#endif
> +		if (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;
>  
> -- 
> 2.34.1

Tested-by: Jonathan Humphreys <j-humphreys@ti.com>

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

[Marek Vasut]

On 12/11/24 1:06 PM, Venkatesh Yadav Abbarapu wrote:
> Update the spi_nor_read() function only for parallel configuration
> when config SPI_STACKED_PARALLEL is enabled and keep the default
> code as is. For parallel configurations fix the read issue for
> 4byte address width by passing the entire length to the read
> function, split the memory of 16MB size banks only when the
> address width is 3byte. Also update the size when the
> configuration is stacked.
> 
> Fixes: 5d40b3d384 ("mtd: spi-nor: Add parallel and stacked memories support")
> Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> ---
> Changes in v2:
> - Update the nor read for parallel configuration and for other case
>    keep the code as is.
> - Fixed the commit description.
> - Tested the changes with s25fl128s flash
> Zynq> sf probe 0 0 0
> SF: Detected s25fl128s with page size 256 Bytes, erase size 64 KiB, total 16 MiB
> Zynq> time sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;time sf write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;time sf read 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x1000000
> SF: 16777216 bytes @ 0x0 Erased: OK
> 
> time: 32.464 seconds
> device 0 whole chip
> SF: 16777216 bytes @ 0x0 Written: OK
> 
> time: 15.515 seconds
> device 0 whole chip
> SF: 16777216 bytes @ 0x0 Read: OK
> 
> time: 1.557 seconds
> Total of 16777216 byte(s) were the same
> ---
>   drivers/mtd/spi/spi-nor-core.c | 50 ++++++++++++++++++++++------------
>   1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index ec841fb13b..8d7087b5c9 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>   				nor->spi->flags &= ~SPI_XFER_U_PAGE;
>   		}
>   #ifdef CONFIG_SPI_FLASH_BAR
> -		ret = write_bar(nor, addr);
> +		ret = write_bar(nor, offset);

This change is really inobvious, the code above likely needs to be 
compiled out if the SPI_STACKED_PARALLEL stuff is disabled ?

"
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 8d7087b5c95..691962ef271 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -1130,6 +1130,7 @@ static int spi_nor_erase(struct mtd_info *mtd, 
struct erase_info *instr)
                         goto erase_err;
                 }
                 offset = addr;
+               if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
                         if (nor->flags & SNOR_F_HAS_PARALLEL)
                                 offset /= 2;

@@ -1139,6 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd, 
struct erase_info *instr)
                                 else
                                         nor->spi->flags &= 
~SPI_XFER_U_PAGE;
                         }
+               }
  #ifdef CONFIG_SPI_FLASH_BAR
                 ret = write_bar(nor, offset);
                 if (ret < 0)
"

>   		if (ret < 0)
>   			goto erase_err;
>   #endif
> @@ -1152,7 +1152,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
>   		    !(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
>   			ret = spi_nor_erase_chip(nor);
>   		} else {
> -			ret = spi_nor_erase_sector(nor, addr);
> +			ret = spi_nor_erase_sector(nor, offset);
>   		}
>   		if (ret < 0)
>   			goto erase_err;
> @@ -1578,8 +1578,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 +1593,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;
>   		offset = from;
>   
> +	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> +		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 +1618,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;
> +		if (len < remain_len)
>   			read_len = len;
>   		else
> -			read_len = rem_bank_len;
> -
> +			read_len = remain_len;
> +#endif
> +		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> +			if (len < rem_bank_len)
> +				read_len = len;
> +			else
> +				read_len = rem_bank_len;
> +		}

If I look at this change with 'git show -w' , the change looks like this:

"
  #ifdef CONFIG_SPI_FLASH_BAR
+               u32 remain_len;
+
                 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 (len < rem_bank_len)
                                 read_len = len;
                         else
                                 read_len = rem_bank_len;
-
+               }
                 if (read_len == 0)
                         return -EIO;
"

Why is there this part of code twice now, ifdeffed out differently in 
each case ?

"
                         if (len < rem_bank_len)
                                 read_len = len;
                         else
                                 read_len = rem_bank_len;
"

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

[Abbarapu, Venkatesh]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, December 16, 2024 5:21 AM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de;
> tudor.ambarus@linaro.org; j-humphreys@ti.com
> Cc: Simek, Michal <michal.simek@amd.com>; jagan@amarulasolutions.com;
> vigneshr@ti.com; u-kumar1@ti.com; trini@konsulko.com; seanga2@gmail.com;
> caleb.connolly@linaro.org; sjg@chromium.org; william.zhang@broadcom.com;
> stefan_b@posteo.net; quentin.schulz@cherry.de; Takahiro.Kuwano@infineon.com;
> p-mantena@ti.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v2] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
> 
> On 12/11/24 1:06 PM, Venkatesh Yadav Abbarapu wrote:
> > Update the spi_nor_read() function only for parallel configuration
> > when config SPI_STACKED_PARALLEL is enabled and keep the default code
> > as is. For parallel configurations fix the read issue for 4byte
> > address width by passing the entire length to the read function, split
> > the memory of 16MB size banks only when the address width is 3byte.
> > Also update the size when the configuration is stacked.
> >
> > Fixes: 5d40b3d384 ("mtd: spi-nor: Add parallel and stacked memories
> > support")
> > Signed-off-by: Venkatesh Yadav Abbarapu <venkatesh.abbarapu@amd.com>
> > ---
> > Changes in v2:
> > - Update the nor read for parallel configuration and for other case
> >    keep the code as is.
> > - Fixed the commit description.
> > - Tested the changes with s25fl128s flash
> > Zynq> sf probe 0 0 0
> > SF: Detected s25fl128s with page size 256 Bytes, erase size 64 KiB,
> > total 16 MiB
> > Zynq> time sf erase 0x0 0x1000000;mw.b 0x8000 aabbccdd 0x1000000;time
> > Zynq> sf write 0x8000 0x0 0x1000000;mw.b 0x8008000 0x0 0x1000000;time
> > Zynq> sf read 0x8008000 0x0 0x1000000;cmp.b 0x8000 0x8008000 0x1000000
> > SF: 16777216 bytes @ 0x0 Erased: OK
> >
> > time: 32.464 seconds
> > device 0 whole chip
> > SF: 16777216 bytes @ 0x0 Written: OK
> >
> > time: 15.515 seconds
> > device 0 whole chip
> > SF: 16777216 bytes @ 0x0 Read: OK
> >
> > time: 1.557 seconds
> > Total of 16777216 byte(s) were the same
> > ---
> >   drivers/mtd/spi/spi-nor-core.c | 50 ++++++++++++++++++++++------------
> >   1 file changed, 33 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/mtd/spi/spi-nor-core.c
> > b/drivers/mtd/spi/spi-nor-core.c index ec841fb13b..8d7087b5c9 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct
> erase_info *instr)
> >   				nor->spi->flags &= ~SPI_XFER_U_PAGE;
> >   		}
> >   #ifdef CONFIG_SPI_FLASH_BAR
> > -		ret = write_bar(nor, addr);
> > +		ret = write_bar(nor, offset);
> 
> This change is really inobvious, the code above likely needs to be compiled out if the
> SPI_STACKED_PARALLEL stuff is disabled ?

In the spi_nor_erase() 
	offset = addr;
	if(PARALLEL)
		offset/=2;
	so for parallel or single configuration we need to pass "offset" to write_bar()
	write_bar(nor, offset");



> 
> "
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index
> 8d7087b5c95..691962ef271 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1130,6 +1130,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct
> erase_info *instr)
>                          goto erase_err;
>                  }
>                  offset = addr;
> +               if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
>                          if (nor->flags & SNOR_F_HAS_PARALLEL)
>                                  offset /= 2;
> 
> @@ -1139,6 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct
> erase_info *instr)
>                                  else
>                                          nor->spi->flags &= ~SPI_XFER_U_PAGE;
>                          }
> +               }
>   #ifdef CONFIG_SPI_FLASH_BAR
>                  ret = write_bar(nor, offset);
>                  if (ret < 0)
> "
> 
> >   		if (ret < 0)
> >   			goto erase_err;
> >   #endif
> > @@ -1152,7 +1152,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct
> erase_info *instr)
> >   		    !(nor->flags & SNOR_F_NO_OP_CHIP_ERASE)) {
> >   			ret = spi_nor_erase_chip(nor);
> >   		} else {
> > -			ret = spi_nor_erase_sector(nor, addr);
> > +			ret = spi_nor_erase_sector(nor, offset);
> >   		}
> >   		if (ret < 0)
> >   			goto erase_err;
> > @@ -1578,8 +1578,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 +1593,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;
> >   		offset = from;
> >
> > +	if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> > +		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 +1618,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;
> > +		if (len < remain_len)
> >   			read_len = len;
> >   		else
> > -			read_len = rem_bank_len;
> > -
> > +			read_len = remain_len;
> > +#endif
> > +		if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
> > +			if (len < rem_bank_len)
> > +				read_len = len;
> > +			else
> > +				read_len = rem_bank_len;
> > +		}
> 
> If I look at this change with 'git show -w' , the change looks like this:
> 
> "
>   #ifdef CONFIG_SPI_FLASH_BAR
> +               u32 remain_len;
> +
>                  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 (len < rem_bank_len)
>                                  read_len = len;
>                          else
>                                  read_len = rem_bank_len;
> -
> +               }
>                  if (read_len == 0)
>                          return -EIO;
> "
> 
> Why is there this part of code twice now, ifdeffed out differently in each case ?
> 
> "
>                          if (len < rem_bank_len)
>                                  read_len = len;
>                          else
>                                  read_len = rem_bank_len; "

For parallel/stacked configuration and address width the "rem_bank_len" will vary and as we don't want to disturb the default read functionality added the ifdef separately.

Thanks
Venkatesh

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

[Marek Vasut]

On 12/16/24 5:16 AM, Abbarapu, Venkatesh wrote:

[...]

>>> +++ b/drivers/mtd/spi/spi-nor-core.c
>>> @@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct
>> erase_info *instr)
>>>    				nor->spi->flags &= ~SPI_XFER_U_PAGE;
>>>    		}
>>>    #ifdef CONFIG_SPI_FLASH_BAR
>>> -		ret = write_bar(nor, addr);
>>> +		ret = write_bar(nor, offset);
>>
>> This change is really inobvious, the code above likely needs to be compiled out if the
>> SPI_STACKED_PARALLEL stuff is disabled ?
> 
> In the spi_nor_erase()
> 	offset = addr;
> 	if(PARALLEL)
> 		offset/=2;
> 	so for parallel or single configuration we need to pass "offset" to write_bar()
> 	write_bar(nor, offset");

The code above likely needs to be compiled out if the 
SPI_STACKED_PARALLEL stuff is disabled ?

[...]

>> If I look at this change with 'git show -w' , the change looks like this:
>>
>> "
>>    #ifdef CONFIG_SPI_FLASH_BAR
>> +               u32 remain_len;
>> +
>>                   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 (len < rem_bank_len)
>>                                   read_len = len;
>>                           else
>>                                   read_len = rem_bank_len;
>> -
>> +               }
>>                   if (read_len == 0)
>>                           return -EIO;
>> "
>>
>> Why is there this part of code twice now, ifdeffed out differently in each case ?
>>
>> "
>>                           if (len < rem_bank_len)
>>                                   read_len = len;
>>                           else
>>                                   read_len = rem_bank_len; "
> 
> For parallel/stacked configuration and address width the "rem_bank_len" will vary and as we don't want to disturb the default read functionality added the ifdef separately.
What would happen if both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are 
enabled on a system that only has one SPI NOR attached 
(non-stacked/parallel) ? I noticed the second "copy" of the code behaves 
slightly differently in the else branch, so does that mean this would 
break such setup ?

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

[Abbarapu, Venkatesh]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Monday, December 16, 2024 4:06 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de;
> tudor.ambarus@linaro.org; j-humphreys@ti.com
> Cc: Simek, Michal <michal.simek@amd.com>; jagan@amarulasolutions.com;
> vigneshr@ti.com; u-kumar1@ti.com; trini@konsulko.com; seanga2@gmail.com;
> caleb.connolly@linaro.org; sjg@chromium.org; william.zhang@broadcom.com;
> stefan_b@posteo.net; quentin.schulz@cherry.de; Takahiro.Kuwano@infineon.com;
> p-mantena@ti.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v2] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
> 
> On 12/16/24 5:16 AM, Abbarapu, Venkatesh wrote:
> 
> [...]
> 
> >>> +++ b/drivers/mtd/spi/spi-nor-core.c
> >>> @@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd,
> >>> struct
> >> erase_info *instr)
> >>>    				nor->spi->flags &= ~SPI_XFER_U_PAGE;
> >>>    		}
> >>>    #ifdef CONFIG_SPI_FLASH_BAR
> >>> -		ret = write_bar(nor, addr);
> >>> +		ret = write_bar(nor, offset);
> >>
> >> This change is really inobvious, the code above likely needs to be
> >> compiled out if the SPI_STACKED_PARALLEL stuff is disabled ?
> >
> > In the spi_nor_erase()
> > 	offset = addr;
> > 	if(PARALLEL)
> > 		offset/=2;
> > 	so for parallel or single configuration we need to pass "offset" to write_bar()
> > 	write_bar(nor, offset");
> 
> The code above likely needs to be compiled out if the SPI_STACKED_PARALLEL
> stuff is disabled ?
> 
> [...]
> 
Already the code here is being checked with the flags SNOR_F_HAS_PARALLEL and SNOR_F_HAS_STACKED. Do you want to add the check SPI_STACKED_PARALLEL apart from these flags?


> >> If I look at this change with 'git show -w' , the change looks like this:
> >>
> >> "
> >>    #ifdef CONFIG_SPI_FLASH_BAR
> >> +               u32 remain_len;
> >> +
> >>                   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 (len < rem_bank_len)
> >>                                   read_len = len;
> >>                           else
> >>                                   read_len = rem_bank_len;
> >> -
> >> +               }
> >>                   if (read_len == 0)
> >>                           return -EIO; "
> >>
> >> Why is there this part of code twice now, ifdeffed out differently in each case ?
> >>
> >> "
> >>                           if (len < rem_bank_len)
> >>                                   read_len = len;
> >>                           else
> >>                                   read_len = rem_bank_len; "
> >
> > For parallel/stacked configuration and address width the "rem_bank_len" will vary
> and as we don't want to disturb the default read functionality added the ifdef
> separately.
> What would happen if both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are
> enabled on a system that only has one SPI NOR attached
> (non-stacked/parallel) ? I noticed the second "copy" of the code behaves slightly
> differently in the else branch, so does that mean this would break such setup ?

If both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are enabled, the "rem_bank_len" manipulation is done under the CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL) code and this won't break any default functionality.

Thanks
Venkatesh

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

[Marek Vasut]

On 12/16/24 2:58 PM, Abbarapu, Venkatesh wrote:

>>>>> +++ b/drivers/mtd/spi/spi-nor-core.c
>>>>> @@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info *mtd,
>>>>> struct
>>>> erase_info *instr)
>>>>>     				nor->spi->flags &= ~SPI_XFER_U_PAGE;
>>>>>     		}
>>>>>     #ifdef CONFIG_SPI_FLASH_BAR
>>>>> -		ret = write_bar(nor, addr);
>>>>> +		ret = write_bar(nor, offset);
>>>>
>>>> This change is really inobvious, the code above likely needs to be
>>>> compiled out if the SPI_STACKED_PARALLEL stuff is disabled ?
>>>
>>> In the spi_nor_erase()
>>> 	offset = addr;
>>> 	if(PARALLEL)
>>> 		offset/=2;
>>> 	so for parallel or single configuration we need to pass "offset" to write_bar()
>>> 	write_bar(nor, offset");
>>
>> The code above likely needs to be compiled out if the SPI_STACKED_PARALLEL
>> stuff is disabled ?
>>
>> [...]
>>
> Already the code here is being checked with the flags SNOR_F_HAS_PARALLEL and SNOR_F_HAS_STACKED. Do you want to add the check SPI_STACKED_PARALLEL apart from these flags?

Yes, to compile the code out completely.

>>>> If I look at this change with 'git show -w' , the change looks like this:
>>>>
>>>> "
>>>>     #ifdef CONFIG_SPI_FLASH_BAR
>>>> +               u32 remain_len;
>>>> +
>>>>                    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 (len < rem_bank_len)
>>>>                                    read_len = len;
>>>>                            else
>>>>                                    read_len = rem_bank_len;
>>>> -
>>>> +               }
>>>>                    if (read_len == 0)
>>>>                            return -EIO; "
>>>>
>>>> Why is there this part of code twice now, ifdeffed out differently in each case ?
>>>>
>>>> "
>>>>                            if (len < rem_bank_len)
>>>>                                    read_len = len;
>>>>                            else
>>>>                                    read_len = rem_bank_len; "
>>>
>>> For parallel/stacked configuration and address width the "rem_bank_len" will vary
>> and as we don't want to disturb the default read functionality added the ifdef
>> separately.
>> What would happen if both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are
>> enabled on a system that only has one SPI NOR attached
>> (non-stacked/parallel) ? I noticed the second "copy" of the code behaves slightly
>> differently in the else branch, so does that mean this would break such setup ?
> 
> If both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are enabled, the "rem_bank_len" manipulation is done under the CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL) code and this won't break any default functionality.
Wouldn't read_len calculation be done twice ?

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

[Abbarapu, Venkatesh]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, December 18, 2024 6:39 AM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de;
> tudor.ambarus@linaro.org; j-humphreys@ti.com
> Cc: Simek, Michal <michal.simek@amd.com>; jagan@amarulasolutions.com;
> vigneshr@ti.com; u-kumar1@ti.com; trini@konsulko.com; seanga2@gmail.com;
> caleb.connolly@linaro.org; sjg@chromium.org; william.zhang@broadcom.com;
> stefan_b@posteo.net; quentin.schulz@cherry.de; Takahiro.Kuwano@infineon.com;
> p-mantena@ti.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v2] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
> 
> On 12/16/24 2:58 PM, Abbarapu, Venkatesh wrote:
> 
> >>>>> +++ b/drivers/mtd/spi/spi-nor-core.c
> >>>>> @@ -1140,7 +1140,7 @@ static int spi_nor_erase(struct mtd_info
> >>>>> *mtd, struct
> >>>> erase_info *instr)
> >>>>>     				nor->spi->flags &= ~SPI_XFER_U_PAGE;
> >>>>>     		}
> >>>>>     #ifdef CONFIG_SPI_FLASH_BAR
> >>>>> -		ret = write_bar(nor, addr);
> >>>>> +		ret = write_bar(nor, offset);
> >>>>
> >>>> This change is really inobvious, the code above likely needs to be
> >>>> compiled out if the SPI_STACKED_PARALLEL stuff is disabled ?
> >>>
> >>> In the spi_nor_erase()
> >>> 	offset = addr;
> >>> 	if(PARALLEL)
> >>> 		offset/=2;
> >>> 	so for parallel or single configuration we need to pass "offset" to write_bar()
> >>> 	write_bar(nor, offset");
> >>
> >> The code above likely needs to be compiled out if the
> >> SPI_STACKED_PARALLEL stuff is disabled ?
> >>
> >> [...]
> >>
> > Already the code here is being checked with the flags SNOR_F_HAS_PARALLEL
> and SNOR_F_HAS_STACKED. Do you want to add the check
> SPI_STACKED_PARALLEL apart from these flags?
> 
> Yes, to compile the code out completely.

Ok...Will update the patch.
> 
> >>>> If I look at this change with 'git show -w' , the change looks like this:
> >>>>
> >>>> "
> >>>>     #ifdef CONFIG_SPI_FLASH_BAR
> >>>> +               u32 remain_len;
> >>>> +
> >>>>                    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 (len < rem_bank_len)
> >>>>                                    read_len = len;
> >>>>                            else
> >>>>                                    read_len = rem_bank_len;
> >>>> -
> >>>> +               }
> >>>>                    if (read_len == 0)
> >>>>                            return -EIO; "
> >>>>
> >>>> Why is there this part of code twice now, ifdeffed out differently in each case ?
> >>>>
> >>>> "
> >>>>                            if (len < rem_bank_len)
> >>>>                                    read_len = len;
> >>>>                            else
> >>>>                                    read_len = rem_bank_len; "
> >>>
> >>> For parallel/stacked configuration and address width the
> >>> "rem_bank_len" will vary
> >> and as we don't want to disturb the default read functionality added
> >> the ifdef separately.
> >> What would happen if both SPI_FLASH_BAR and SPI_STACKED_PARALLEL
> are
> >> enabled on a system that only has one SPI NOR attached
> >> (non-stacked/parallel) ? I noticed the second "copy" of the code
> >> behaves slightly differently in the else branch, so does that mean this would
> break such setup ?
> >
> > If both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are enabled, the
> "rem_bank_len" manipulation is done under the
> CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL) code and this won't break any
> default functionality.
> Wouldn't read_len calculation be done twice ?
Yes.  As "rem_bank_len" will be changed based on parallel configuration, so added the additional code copy to not break the default code.

Thanks
Venkatesh

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

[Marek Vasut]

On 12/18/24 10:22 AM, Abbarapu, Venkatesh wrote:

>>>>> For parallel/stacked configuration and address width the
>>>>> "rem_bank_len" will vary
>>>> and as we don't want to disturb the default read functionality added
>>>> the ifdef separately.
>>>> What would happen if both SPI_FLASH_BAR and SPI_STACKED_PARALLEL
>> are
>>>> enabled on a system that only has one SPI NOR attached
>>>> (non-stacked/parallel) ? I noticed the second "copy" of the code
>>>> behaves slightly differently in the else branch, so does that mean this would
>> break such setup ?
>>>
>>> If both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are enabled, the
>> "rem_bank_len" manipulation is done under the
>> CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL) code and this won't break any
>> default functionality.
>> Wouldn't read_len calculation be done twice ?
> Yes.  As "rem_bank_len" will be changed based on parallel configuration, so added the additional code copy to not break the default code.
Can you please also update it to avoid the code duplication ?

Thank you

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

[Abbarapu, Venkatesh]

> -----Original Message-----
> From: Marek Vasut <marex@denx.de>
> Sent: Wednesday, December 18, 2024 7:28 PM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu@amd.com>; u-boot@lists.denx.de;
> tudor.ambarus@linaro.org; j-humphreys@ti.com
> Cc: Simek, Michal <michal.simek@amd.com>; jagan@amarulasolutions.com;
> vigneshr@ti.com; u-kumar1@ti.com; trini@konsulko.com; seanga2@gmail.com;
> caleb.connolly@linaro.org; sjg@chromium.org; william.zhang@broadcom.com;
> stefan_b@posteo.net; quentin.schulz@cherry.de; Takahiro.Kuwano@infineon.com;
> p-mantena@ti.com; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH v2] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
> 
> On 12/18/24 10:22 AM, Abbarapu, Venkatesh wrote:
> 
> >>>>> For parallel/stacked configuration and address width the
> >>>>> "rem_bank_len" will vary
> >>>> and as we don't want to disturb the default read functionality
> >>>> added the ifdef separately.
> >>>> What would happen if both SPI_FLASH_BAR and
> SPI_STACKED_PARALLEL
> >> are
> >>>> enabled on a system that only has one SPI NOR attached
> >>>> (non-stacked/parallel) ? I noticed the second "copy" of the code
> >>>> behaves slightly differently in the else branch, so does that mean
> >>>> this would
> >> break such setup ?
> >>>
> >>> If both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are enabled, the
> >> "rem_bank_len" manipulation is done under the
> >> CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL) code and this won't break
> any
> >> default functionality.
> >> Wouldn't read_len calculation be done twice ?
> > Yes.  As "rem_bank_len" will be changed based on parallel configuration, so
> added the additional code copy to not break the default code.
> Can you please also update it to avoid the code duplication ?
The code is entirely separated out based on CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL), not sure I can remove this additional code copy. Do you have any better way to avoid this code duplication?

Thanks
Venkatesh
> 
> Thank you