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

18 Dec 2024


      ...
-----Original Message-----
From: Marek Vasut marex@denx.de
Sent: Wednesday, December 18, 2024 6:39 AM
To: Abbarapu, Venkatesh venkatesh.abbarapu@amd.com; u-boot@lists.denx.de;
tudor.ambarus@linaro.org; j-humphreys@ti.com
Cc: Simek, Michal michal.simek@amd.com; jagan@amarulasolutions.com;
vigneshr@ti.com; u-kumar1@ti.com; trini@konsulko.com; seanga2@gmail.com;
caleb.connolly@linaro.org; sjg@chromium.org; william.zhang@broadcom.com;
stefan_b@posteo.net; quentin.schulz@cherry.de; Takahiro.Kuwano@infineon.com;
p-mantena@ti.com; git (AMD-Xilinx) git@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

    