[PATCH v4] 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. 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
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. --- drivers/mtd/spi/spi-nor-core.c | 51 ++++++++++++++++++++-------------- 1 file changed, 30 insertions(+), 21 deletions(-)
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c index ec841fb13bd..a2ef37472d1 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,18 +1596,22 @@ 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; + read_len = len; + offset = from;
- rem_bank_len = SZ_16M * (bank + 1); - if (nor->flags & SNOR_F_HAS_PARALLEL) - rem_bank_len *= 2; - rem_bank_len -= from; + if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) { + bank = (u32)from / SZ_16M; + if (nor->flags & SNOR_F_HAS_PARALLEL) + bank /= 2;
- offset = from; + 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,6 +1620,9 @@ 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;
@@ -1621,7 +1631,6 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, if (ret < 0) return log_ret(ret); #endif - if (len < rem_bank_len) read_len = len; else
On 12/24/24 4:37 PM, Venkatesh Yadav Abbarapu wrote:
[...]
@@ -1593,18 +1596,22 @@ 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;
read_len = len;offset = from;
rem_bank_len = SZ_16M * (bank + 1);if (nor->flags & SNOR_F_HAS_PARALLEL)rem_bank_len *= 2;rem_bank_len -= from;
if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {bank = (u32)from / SZ_16M;if (nor->flags & SNOR_F_HAS_PARALLEL)bank /= 2;
Is this code which operates on (nor->flags & SNOR_F_HAS_PARALLEL) really supposed to be enabled if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) is SET or instead if STACKED_PARALLEL symbol is SET ?
Hi,
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Thursday, December 26, 2024 2:19 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 v4] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled
On 12/24/24 4:37 PM, Venkatesh Yadav Abbarapu wrote:
[...]
@@ -1593,18 +1596,22 @@ 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;
read_len = len;offset = from;
rem_bank_len = SZ_16M * (bank + 1);if (nor->flags & SNOR_F_HAS_PARALLEL)rem_bank_len *= 2;rem_bank_len -= from;
if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {bank = (u32)from / SZ_16M;if (nor->flags & SNOR_F_HAS_PARALLEL)bank /= 2;Is this code which operates on (nor->flags & SNOR_F_HAS_PARALLEL) really supposed to be enabled if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) is SET or instead if STACKED_PARALLEL symbol is SET ?
The FLASH_BAR and STACKED_PARALLEL configs are independent. They won't depend on each other.
Thanks Venkatesh
On Thu, Dec 26, 2024 at 03:34:33AM +0000, Abbarapu, Venkatesh wrote:
Hi,
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Thursday, December 26, 2024 2:19 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 v4] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled
On 12/24/24 4:37 PM, Venkatesh Yadav Abbarapu wrote:
[...]
@@ -1593,18 +1596,22 @@ 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;
read_len = len;offset = from;
rem_bank_len = SZ_16M * (bank + 1);if (nor->flags & SNOR_F_HAS_PARALLEL)rem_bank_len *= 2;rem_bank_len -= from;
if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {bank = (u32)from / SZ_16M;if (nor->flags & SNOR_F_HAS_PARALLEL)bank /= 2;Is this code which operates on (nor->flags & SNOR_F_HAS_PARALLEL) really supposed to be enabled if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) is SET or instead if STACKED_PARALLEL symbol is SET ?
The FLASH_BAR and STACKED_PARALLEL configs are independent. They won't depend on each other.
At this point in the release cycle, I'd really rather see things as: if (CONFIG_SPI_STACKED_PARALLEL) { ... anything needed for the new support mode .. } else { ... the way it used to be ... }
And for v2025.04 we can move the codebase forward. But I'm worried at this point I'm going to have to revert all of the stacked stuff, which could get messy, in order to avoid regressions elsewhere.
Hi,
-----Original Message----- From: Tom Rini trini@konsulko.com Sent: Thursday, December 26, 2024 8:47 PM To: Abbarapu, Venkatesh venkatesh.abbarapu@amd.com Cc: Marek Vasut marex@denx.de; u-boot@lists.denx.de; tudor.ambarus@linaro.org; j-humphreys@ti.com; Simek, Michal michal.simek@amd.com; jagan@amarulasolutions.com; vigneshr@ti.com; u- kumar1@ti.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 v4] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled
On Thu, Dec 26, 2024 at 03:34:33AM +0000, Abbarapu, Venkatesh wrote:
Hi,
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Thursday, December 26, 2024 2:19 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 v4] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled
On 12/24/24 4:37 PM, Venkatesh Yadav Abbarapu wrote:
[...]
@@ -1593,18 +1596,22 @@ 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;
read_len = len;offset = from;
rem_bank_len = SZ_16M * (bank + 1);if (nor->flags & SNOR_F_HAS_PARALLEL)rem_bank_len *= 2;rem_bank_len -= from;
if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {bank = (u32)from / SZ_16M;if (nor->flags & SNOR_F_HAS_PARALLEL)bank /= 2;Is this code which operates on (nor->flags & SNOR_F_HAS_PARALLEL) really supposed to be enabled if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) is SET or instead if STACKED_PARALLEL symbol is SET ?
The FLASH_BAR and STACKED_PARALLEL configs are independent. They
won't depend on each other.
At this point in the release cycle, I'd really rather see things as: if (CONFIG_SPI_STACKED_PARALLEL) { ... anything needed for the new support mode .. } else { ... the way it used to be ... }
And for v2025.04 we can move the codebase forward. But I'm worried at this point I'm going to have to revert all of the stacked stuff, which could get messy, in order to avoid regressions elsewhere.
I am updating the parallel/stacked code based on the config SPI_STACKED_PARALLEL and not touching the default code.
Thanks Venkatesh
-- Tom
On Fri, Dec 27, 2024 at 04:48:47AM +0000, Abbarapu, Venkatesh wrote:
Hi,
-----Original Message----- From: Tom Rini trini@konsulko.com Sent: Thursday, December 26, 2024 8:47 PM To: Abbarapu, Venkatesh venkatesh.abbarapu@amd.com Cc: Marek Vasut marex@denx.de; u-boot@lists.denx.de; tudor.ambarus@linaro.org; j-humphreys@ti.com; Simek, Michal michal.simek@amd.com; jagan@amarulasolutions.com; vigneshr@ti.com; u- kumar1@ti.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 v4] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled
On Thu, Dec 26, 2024 at 03:34:33AM +0000, Abbarapu, Venkatesh wrote:
Hi,
-----Original Message----- From: Marek Vasut marex@denx.de Sent: Thursday, December 26, 2024 2:19 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 v4] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled
On 12/24/24 4:37 PM, Venkatesh Yadav Abbarapu wrote:
[...]
@@ -1593,18 +1596,22 @@ 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;
read_len = len;offset = from;
rem_bank_len = SZ_16M * (bank + 1);if (nor->flags & SNOR_F_HAS_PARALLEL)rem_bank_len *= 2;rem_bank_len -= from;
if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {bank = (u32)from / SZ_16M;if (nor->flags & SNOR_F_HAS_PARALLEL)bank /= 2;Is this code which operates on (nor->flags & SNOR_F_HAS_PARALLEL) really supposed to be enabled if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) is SET or instead if STACKED_PARALLEL symbol is SET ?
The FLASH_BAR and STACKED_PARALLEL configs are independent. They
won't depend on each other.
At this point in the release cycle, I'd really rather see things as: if (CONFIG_SPI_STACKED_PARALLEL) { ... anything needed for the new support mode .. } else { ... the way it used to be ... }
And for v2025.04 we can move the codebase forward. But I'm worried at this point I'm going to have to revert all of the stacked stuff, which could get messy, in order to avoid regressions elsewhere.
I am updating the parallel/stacked code based on the config SPI_STACKED_PARALLEL and not touching the default code.
Right, but to be clear, are you bringing back the default code to the state / algorithms it was in for v2024.10?
On 12/24/24 4:37 PM, Venkatesh Yadav Abbarapu wrote:
@@ -1593,18 +1596,22 @@ 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;
read_len = len;offset = from;
rem_bank_len = SZ_16M * (bank + 1);if (nor->flags & SNOR_F_HAS_PARALLEL)rem_bank_len *= 2;rem_bank_len -= from;
if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {
^^^^^^^^^^^^^
bank = (u32)from / SZ_16M;if (nor->flags & SNOR_F_HAS_PARALLEL)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Is this really correct ?
Should this be wrapped in if (CONFIG_IS_ENABLED(STACKED_PARALLEL)) instead ?
[...]
participants (4)
-
Abbarapu, Venkatesh -
Marek Vasut -
Tom Rini -
Venkatesh Yadav Abbarapu