[PATCH v6] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled
Kumar, Udit
u-kumar1 at ti.com
Mon Dec 30 10:33:39 CET 2024
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 at 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;
> + }
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);
> 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)) {
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;
More information about the U-Boot
mailing list