> -----Original Message-----
> From: Marek Vasut <[email protected]>
> Sent: Monday, December 23, 2024 10:22 PM
> 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 v3] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
>
> On 12/20/24 3:30 PM, Venkatesh Yadav Abbarapu wrote:
>
> [...]
>
> > @@ -1578,8 +1580,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 +1595,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;
>
> size_t read_len should be declared at the beginning of this function.
>
> > offset = from;
> >
> > + if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) {
>
> The indent here needs to be pushed on tab RIGHT -> .
>
> Do you need this functionality in SPL ? If so, you might need a matching
> Kconfig
> SPL_SPI_STACKED_PARALLEL symbol .
This functionality is not needed for SPL.
>
> > + 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 +1620,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;
>
> Is this calculation correct?
>
> It used to be calculated as:
>
> bank = (u32)from / SZ_16M;
> rem_bank_len = SZ_16M * (bank + 1);
> rem_bank_len -= from;
>
> Now the bank is calculated as:
>
> bank = nor->bank_curr + 1;
>
> ?
>
> Why not keep calculating bank from offset ?
>
> Maybe this will work ?
>
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index
> 691962ef271..bdfbd44f30c 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -1578,10 +1578,11 @@ 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 rem_bank_len = 0;
> u32 stack_shift = 0;
> + size_t read_len;
> + int ret;
> u8 bank;
> bool is_ofst_odd = false;
>
> @@ -1595,7 +1596,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t
> from,
> size_t len,
> }
>
> while (len) {
> - size_t read_len = len;
> + read_len = len;
> offset = from;
>
> if (CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL)) { @@ -
> 1627,24 +1628,24 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from,
> size_t len,
> offset /= 2;
> }
>
> -#ifdef CONFIG_SPI_FLASH_BAR
> - u32 remain_len;
> + if (CONFIG_IS_ENABLED(SPI_FLASH_BAR)) {
> + bank = (u32)from / SZ_16M;
> + rem_bank_len = SZ_16M * (bank + 1);
> + rem_bank_len -= from;
>
> 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 (CONFIG_IS_ENABLED(SPI_FLASH_BAR) ||
> + 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;
>
> I am still worried about using stacked parallel and BAR together, is that even
> possible or are they mutually exclusive ?
Will check the above implementation and update the patch.
FLASH_BAR and stacked parallel configuration doesn’t depend on each other.
Thanks
Venkatesh
