-----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; }
}"
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);@@ -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;
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;elseread_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;elseread_len = remain_len;
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