[PATCH v2] mtd: spi-nor: Fix the spi_nor_read() when config SPI_STACKED_PARALLEL is enabled

[Abbarapu, Venkatesh]

> -----Original Message-----
> From: Marek Vasut <marex at denx.de>
> Sent: Wednesday, December 18, 2024 6:39 AM
> To: Abbarapu, Venkatesh <venkatesh.abbarapu at amd.com>; u-boot at lists.denx.de;
> tudor.ambarus at linaro.org; j-humphreys at ti.com
> Cc: Simek, Michal <michal.simek at amd.com>; jagan at amarulasolutions.com;
> vigneshr at ti.com; u-kumar1 at ti.com; trini at konsulko.com; seanga2 at gmail.com;
> caleb.connolly at linaro.org; sjg at chromium.org; william.zhang at broadcom.com;
> stefan_b at posteo.net; quentin.schulz at cherry.de; Takahiro.Kuwano at infineon.com;
> p-mantena at ti.com; git (AMD-Xilinx) <git at amd.com>
> Subject: Re: [PATCH v2] mtd: spi-nor: Fix the spi_nor_read() when config
> SPI_STACKED_PARALLEL is enabled
> 
> On 12/16/24 2:58 PM, 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?
> 
> Yes, to compile the code out completely.

Ok...Will update the patch.
> 
> >>>> 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.
> Wouldn't read_len calculation be done twice ?
Yes.  As "rem_bank_len" will be changed based on parallel configuration, so added the additional code copy to not break the default code.

Thanks
Venkatesh