> -----Original Message-----
> From: Marek Vasut <[email protected]>
> Sent: Monday, December 16, 2024 5:21 AM
> To: Abbarapu, Venkatesh <[email protected]>; [email protected];
> [email protected]; [email protected]
> Cc: Simek, Michal <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; git (AMD-Xilinx) <[email protected]>
> 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 <[email protected]>
> > ---
> > 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
