> -----Original Message-----
> From: Marek Vasut <[email protected]>
> Sent: Monday, December 16, 2024 4:06 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 v2] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
>
> On 12/16/24 5:16 AM, Abbarapu, Venkatesh wrote:
>
> [...]
>
> >>> +++ 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");
>
> The code above likely needs to be compiled out if the SPI_STACKED_PARALLEL
> stuff is disabled ?
>
> [...]
>
Already the code here is being checked with the flags SNOR_F_HAS_PARALLEL and
SNOR_F_HAS_STACKED. Do you want to add the check SPI_STACKED_PARALLEL apart
from these flags?
> >> 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.
> What would happen if both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are
> enabled on a system that only has one SPI NOR attached
> (non-stacked/parallel) ? I noticed the second "copy" of the code behaves
> slightly
> differently in the else branch, so does that mean this would break such setup
> ?
If both SPI_FLASH_BAR and SPI_STACKED_PARALLEL are enabled, the "rem_bank_len"
manipulation is done under the CONFIG_IS_ENABLED(SPI_STACKED_PARALLEL) code and
this won't break any default functionality.
Thanks
Venkatesh
