[PATCH v6] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled
Update the spi_nor_read() function based on the config SPI_FLASH_BAR and update the length and bank calculation by spliting the memory of 16MB size banks only when the address width is 3byte. Fix the read issue for 4byte address width by passing the entire length to the read function.
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
Changes in v3: - Compile out the parallel code in spi_nor_erase() if the SPI_STACKED_PARALLEL is enabled.
Changes in v4: - Update the bank calculation only when SPL_FLASH_BAR is enabled.
Changes in v5: - Update the parallel code in the spi_nor_read() only if the SPI_STACKED_PARALLEL config is enabled.
Changes in v6: - Remove the hardcoded address width for 4byte. - Move the length condition with rem_bank_len under SPI_FLASH_BAR flag. --- drivers/mtd/spi/spi-nor-core.c | 75 ++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 31 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index ec841fb13bd..6f352c5c0e2 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -1130,17 +1130,19 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) goto erase_err; } offset = addr; - if (nor->flags & SNOR_F_HAS_PARALLEL) - offset /= 2; - - if (nor->flags & SNOR_F_HAS_STACKED) { - if (offset >= (mtd->size / 2)) - nor->spi->flags |= SPI_XFER_U_PAGE; - else - nor->spi->flags &= ~SPI_XFER_U_PAGE; + if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) { + if (nor->flags & SNOR_F_HAS_PARALLEL) + offset /= 2; + + if (nor->flags & SNOR_F_HAS_STACKED) { + if (offset >= (mtd->size / 2)) + nor->spi->flags |= SPI_XFER_U_PAGE; + else + 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 +1154,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; @@ -1576,11 +1578,12 @@ 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 read_len = 0; u32 rem_bank_len = 0; + u32 stack_shift = 0; + size_t read_len; u8 bank; + int ret; bool is_ofst_odd = false;
dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); @@ -1593,39 +1596,49 @@ 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; - + read_len = len; offset = from;
- if (nor->flags & SNOR_F_HAS_STACKED) { - if (offset >= (mtd->size / 2)) { - offset = offset - (mtd->size / 2); - nor->spi->flags |= SPI_XFER_U_PAGE; - } else { - nor->spi->flags &= ~SPI_XFER_U_PAGE; + if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) { + bank = (u32)from / SZ_16M; + if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) { + if (nor->flags & SNOR_F_HAS_PARALLEL) + bank /= 2; + } + rem_bank_len = SZ_16M * (bank + 1); + if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) { + if (nor->flags & SNOR_F_HAS_PARALLEL) + rem_bank_len *= 2; } + rem_bank_len -= from; }
- if (nor->flags & SNOR_F_HAS_PARALLEL) - offset /= 2; + if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) { + 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; + } else { + nor->spi->flags &= ~SPI_XFER_U_PAGE; + } + } + } + + if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) { + if (nor->flags & SNOR_F_HAS_PARALLEL) + offset /= 2; + }
#ifdef CONFIG_SPI_FLASH_BAR ret = write_bar(nor, offset); if (ret < 0) return log_ret(ret); -#endif - if (len < rem_bank_len) read_len = len; else read_len = rem_bank_len; +#endif
if (read_len == 0) return -EIO;
Hello Venkatesh,
On 12/30/2024 12:32 PM, Venkatesh Yadav Abbarapu wrote:
Update the spi_nor_read() function based on the config SPI_FLASH_BAR and update the length and bank calculation by spliting the memory of 16MB size banks only when the address width is 3byte. Fix the read issue for 4byte address width by passing the entire length to the read function.
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
Changes in v3:
- Compile out the parallel code in spi_nor_erase() if the SPI_STACKED_PARALLEL is enabled.
Changes in v4:
- Update the bank calculation only when SPL_FLASH_BAR is enabled.
Changes in v5:
- Update the parallel code in the spi_nor_read() only if the SPI_STACKED_PARALLEL config is enabled.
Changes in v6:
- Remove the hardcoded address width for 4byte.
- Move the length condition with rem_bank_len under SPI_FLASH_BAR flag.
drivers/mtd/spi/spi-nor-core.c | 75 ++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 31 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index ec841fb13bd..6f352c5c0e2 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -1130,17 +1130,19 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) goto erase_err; } offset = addr;
if (nor->flags & SNOR_F_HAS_PARALLEL)offset /= 2;if (nor->flags & SNOR_F_HAS_STACKED) {if (offset >= (mtd->size / 2))nor->spi->flags |= SPI_XFER_U_PAGE;elsenor->spi->flags &= ~SPI_XFER_U_PAGE;
if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {if (nor->flags & SNOR_F_HAS_PARALLEL)offset /= 2;if (nor->flags & SNOR_F_HAS_STACKED) {if (offset >= (mtd->size / 2))nor->spi->flags |= SPI_XFER_U_PAGE;elsenor->spi->flags &= ~SPI_XFER_U_PAGE;}
I believe SNOR_F_HAS_PARALLEL and SNOR_F_HAS_STACKED flags are specific to your use case.
which should be set if flash is parallel and/or stacked, then why you want to protect with another
if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL).
if you want to reset flags, you can do at start of function (nor->spi->flags &= ~SPI_XFER_U_PAGE)
IMO, only flags conditions should be enough.
}#ifdef CONFIG_SPI_FLASH_BAR
ret = write_bar(nor, addr);
ret = write_bar(nor, offset);@@ -1152,7 +1154,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);@@ -1576,11 +1578,12 @@ 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 read_len = 0; u32 rem_bank_len = 0;
u32 stack_shift = 0;
size_t read_len; u8 bank;
int ret; bool is_ofst_odd = false;
dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len);
@@ -1593,39 +1596,49 @@ 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;
read_len = len;
if (nor->flags & SNOR_F_HAS_STACKED) {if (offset >= (mtd->size / 2)) {offset = offset - (mtd->size / 2);nor->spi->flags |= SPI_XFER_U_PAGE;} else {nor->spi->flags &= ~SPI_XFER_U_PAGE;
if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {
What if SPI_FLASH_BAR not set and SPI_STACKED_PARALLEL is set
bank = (u32)from / SZ_16M;if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {if (nor->flags & SNOR_F_HAS_PARALLEL)bank /= 2;}rem_bank_len = SZ_16M * (bank + 1);if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {if (nor->flags & SNOR_F_HAS_PARALLEL)rem_bank_len *= 2; }rem_bank_len -= from;
if (nor->flags & SNOR_F_HAS_PARALLEL)offset /= 2;
if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {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;} else {nor->spi->flags &= ~SPI_XFER_U_PAGE;}}
Can you think to remove additional condition around SPI_STACKED_PARALLEL, if flag SNOR_F_HAS_STACKED is sufficient.
}if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {if (nor->flags & SNOR_F_HAS_PARALLEL)offset /= 2;}
Same here, please review additional if condition
#ifdef CONFIG_SPI_FLASH_BAR ret = write_bar(nor, offset); if (ret < 0) return log_ret(ret); -#endif
- if (len < rem_bank_len) read_len = len; else read_len = rem_bank_len;
+#endif
if (read_len == 0) return -EIO;
On Mon, Dec 30, 2024 at 03:03:39PM +0530, Kumar, Udit wrote:
Hello Venkatesh,
On 12/30/2024 12:32 PM, Venkatesh Yadav Abbarapu wrote:
Update the spi_nor_read() function based on the config SPI_FLASH_BAR and update the length and bank calculation by spliting the memory of 16MB size banks only when the address width is 3byte. Fix the read issue for 4byte address width by passing the entire length to the read function.
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
Changes in v3:
- Compile out the parallel code in spi_nor_erase() if the SPI_STACKED_PARALLEL is enabled.
Changes in v4:
- Update the bank calculation only when SPL_FLASH_BAR is enabled.
Changes in v5:
- Update the parallel code in the spi_nor_read() only if the SPI_STACKED_PARALLEL config is enabled.
Changes in v6:
- Remove the hardcoded address width for 4byte.
- Move the length condition with rem_bank_len under SPI_FLASH_BAR flag.
drivers/mtd/spi/spi-nor-core.c | 75 ++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 31 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index ec841fb13bd..6f352c5c0e2 100644 --- a/drivers/mtd/spi/spi-nor-core.c +++ b/drivers/mtd/spi/spi-nor-core.c @@ -1130,17 +1130,19 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) goto erase_err; } offset = addr;
if (nor->flags & SNOR_F_HAS_PARALLEL)offset /= 2;if (nor->flags & SNOR_F_HAS_STACKED) {if (offset >= (mtd->size / 2))nor->spi->flags |= SPI_XFER_U_PAGE;elsenor->spi->flags &= ~SPI_XFER_U_PAGE;
if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {if (nor->flags & SNOR_F_HAS_PARALLEL)offset /= 2;if (nor->flags & SNOR_F_HAS_STACKED) {if (offset >= (mtd->size / 2))nor->spi->flags |= SPI_XFER_U_PAGE;elsenor->spi->flags &= ~SPI_XFER_U_PAGE;}I believe SNOR_F_HAS_PARALLEL and SNOR_F_HAS_STACKED flags are specific to your use case.
which should be set if flash is parallel and/or stacked, then why you want to protect with another
if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL).
Because at this point in the release cycle I really really want these isolated from the rest of the code paths, given the problems we've seen. I expect some cleanups for v2025.04 that will get wider testing than happened for what will be in v2025.01.
On Mon, 30 Dec 2024 12:32:06 +0530, Venkatesh Yadav Abbarapu wrote:
Update the spi_nor_read() function based on the config SPI_FLASH_BAR and update the length and bank calculation by spliting the memory of 16MB size banks only when the address width is 3byte. Fix the read issue for 4byte address width by passing the entire length to the read function.
[...]
Applied to u-boot/master, thanks!
participants (3)
-
Kumar, Udit -
Tom Rini -
Venkatesh Yadav Abbarapu