-----Original Message----- From: Marek Vasut marex@denx.de Sent: Monday, December 23, 2024 10:22 PM 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 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;elseread_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