* [U-Boot] Regressions in MTD / SPI FLASH
@ 2019-09-04 18:07 Eugeniy Paltsev
2019-09-04 18:07 ` [U-Boot] [PATCH 1/1] MTD: SPI: revert removing SST26* flash IC protection ops Eugeniy Paltsev
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Eugeniy Paltsev @ 2019-09-04 18:07 UTC (permalink / raw)
To: u-boot
We faced with regressions caused by
commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
This switch was performed by removing entire u-boot spi-flash
core implementation and copying it from another project.
However the switch is performed without proper testing and
investigations about fixes/improvements were made in u-boot
spi-flash core. This results in regressions.
One of regression we faced with is related to SST26 flash series which
is used on HSDK board. The cause is that SST26 protection ops
implementation was dropped. The fix of this regression is send
as a patch in this series.
However there are another regressions. I.E: we also faced with broken
SPI flash on another SNPS boards - AXS101 and AXS103. They use different
flash IC (n25q512ax3) and I didn't investigate the cause yet.
I can also expect regressions among other u-boot users
and I believe that subsystem changes mustn't be done such harmful way.
Eugeniy Paltsev (1):
MTD: SPI: revert removing SST26* flash IC protection ops
drivers/mtd/spi/sf_internal.h | 1 +
drivers/mtd/spi/spi-nor-core.c | 181 +++++++++++++++++++++++++++++++++
drivers/mtd/spi/spi-nor-ids.c | 8 +-
include/linux/mtd/spi-nor.h | 4 +
4 files changed, 190 insertions(+), 4 deletions(-)
--
2.21.0
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] [PATCH 1/1] MTD: SPI: revert removing SST26* flash IC protection ops
2019-09-04 18:07 [U-Boot] Regressions in MTD / SPI FLASH Eugeniy Paltsev
@ 2019-09-04 18:07 ` Eugeniy Paltsev
2019-09-07 3:40 ` Jagan Teki
2019-09-06 7:36 ` [U-Boot] Regressions in MTD / SPI FLASH Vignesh Raghavendra
2019-09-09 7:59 ` Schrempf Frieder
2 siblings, 1 reply; 15+ messages in thread
From: Eugeniy Paltsev @ 2019-09-04 18:07 UTC (permalink / raw)
To: u-boot
Commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
performs switch from previous 'spi_flash' infrastructure without
proper testing/investigations which results in regressions.
Fix regression related to SST26 flash IC series which is lacking
protection ops implementation which were introduced previously by
Commit 3d4fed87a5fa (mtd: sf: Add support of sst26wf* flash ICs
protection ops)
Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
---
Tom, could you please pick this patch to 2019.10?
drivers/mtd/spi/sf_internal.h | 1 +
drivers/mtd/spi/spi-nor-core.c | 181 +++++++++++++++++++++++++++++++++
drivers/mtd/spi/spi-nor-ids.c | 8 +-
include/linux/mtd/spi-nor.h | 4 +
4 files changed, 190 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
index a6bf734830a..e6da768bf36 100644
--- a/drivers/mtd/spi/sf_internal.h
+++ b/drivers/mtd/spi/sf_internal.h
@@ -65,6 +65,7 @@ struct flash_info {
#define NO_CHIP_ERASE BIT(12) /* Chip does not support chip erase */
#define SPI_NOR_SKIP_SFDP BIT(13) /* Skip parsing of SFDP tables */
#define USE_CLSR BIT(14) /* use CLSR command */
+#define SPI_NOR_HAS_SST26LOCK BIT(15) /* Flash supports lock/unlock via BPR */
};
extern const struct flash_info spi_nor_ids[];
diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
index 1acff745d1a..990e39d7c2f 100644
--- a/drivers/mtd/spi/spi-nor-core.c
+++ b/drivers/mtd/spi/spi-nor-core.c
@@ -945,6 +945,177 @@ read_err:
}
#ifdef CONFIG_SPI_FLASH_SST
+/*
+ * sst26 flash series has its own block protection implementation:
+ * 4x - 8 KByte blocks - read & write protection bits - upper addresses
+ * 1x - 32 KByte blocks - write protection bits
+ * rest - 64 KByte blocks - write protection bits
+ * 1x - 32 KByte blocks - write protection bits
+ * 4x - 8 KByte blocks - read & write protection bits - lower addresses
+ *
+ * We'll support only per 64k lock/unlock so lower and upper 64 KByte region
+ * will be treated as single block.
+ */
+#define SST26_BPR_8K_NUM 4
+#define SST26_MAX_BPR_REG_LEN (18 + 1)
+#define SST26_BOUND_REG_SIZE ((32 + SST26_BPR_8K_NUM * 8) * SZ_1K)
+
+enum lock_ctl {
+ SST26_CTL_LOCK,
+ SST26_CTL_UNLOCK,
+ SST26_CTL_CHECK
+};
+
+static bool sst26_process_bpr(u32 bpr_size, u8 *cmd, u32 bit, enum lock_ctl ctl)
+{
+ switch (ctl) {
+ case SST26_CTL_LOCK:
+ cmd[bpr_size - (bit / 8) - 1] |= BIT(bit % 8);
+ break;
+ case SST26_CTL_UNLOCK:
+ cmd[bpr_size - (bit / 8) - 1] &= ~BIT(bit % 8);
+ break;
+ case SST26_CTL_CHECK:
+ return !!(cmd[bpr_size - (bit / 8) - 1] & BIT(bit % 8));
+ }
+
+ return false;
+}
+
+/*
+ * Lock, unlock or check lock status of the flash region of the flash (depending
+ * on the lock_ctl value)
+ */
+static int sst26_lock_ctl(struct spi_nor *nor, loff_t ofs, uint64_t len, enum lock_ctl ctl)
+{
+ struct mtd_info *mtd = &nor->mtd;
+ u32 i, bpr_ptr, rptr_64k, lptr_64k, bpr_size;
+ bool lower_64k = false, upper_64k = false;
+ u8 bpr_buff[SST26_MAX_BPR_REG_LEN] = {};
+ int ret;
+
+ /* Check length and offset for 64k alignment */
+ if ((ofs & (SZ_64K - 1)) || (len & (SZ_64K - 1))) {
+ dev_err(nor->dev, "length or offset is not 64KiB allighned\n");
+ return -EINVAL;
+ }
+
+ if (ofs + len > mtd->size) {
+ dev_err(nor->dev, "range is more than device size: %#llx + %#llx > %#llx\n",
+ ofs, len, mtd->size);
+ return -EINVAL;
+ }
+
+ /* SST26 family has only 16 Mbit, 32 Mbit and 64 Mbit IC */
+ if (mtd->size != SZ_2M &&
+ mtd->size != SZ_4M &&
+ mtd->size != SZ_8M)
+ return -EINVAL;
+
+ bpr_size = 2 + (mtd->size / SZ_64K / 8);
+
+ ret = nor->read_reg(nor, SPINOR_OP_READ_BPR, bpr_buff, bpr_size);
+ if (ret < 0) {
+ dev_err(nor->dev, "fail to read block-protection register\n");
+ return ret;
+ }
+
+ rptr_64k = min_t(u32, ofs + len, mtd->size - SST26_BOUND_REG_SIZE);
+ lptr_64k = max_t(u32, ofs, SST26_BOUND_REG_SIZE);
+
+ upper_64k = ((ofs + len) > (mtd->size - SST26_BOUND_REG_SIZE));
+ lower_64k = (ofs < SST26_BOUND_REG_SIZE);
+
+ /* Lower bits in block-protection register are about 64k region */
+ bpr_ptr = lptr_64k / SZ_64K - 1;
+
+ /* Process 64K blocks region */
+ while (lptr_64k < rptr_64k) {
+ if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
+ return EACCES;
+
+ bpr_ptr++;
+ lptr_64k += SZ_64K;
+ }
+
+ /* 32K and 8K region bits in BPR are after 64k region bits */
+ bpr_ptr = (mtd->size - 2 * SST26_BOUND_REG_SIZE) / SZ_64K;
+
+ /* Process lower 32K block region */
+ if (lower_64k)
+ if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
+ return EACCES;
+
+ bpr_ptr++;
+
+ /* Process upper 32K block region */
+ if (upper_64k)
+ if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
+ return EACCES;
+
+ bpr_ptr++;
+
+ /* Process lower 8K block regions */
+ for (i = 0; i < SST26_BPR_8K_NUM; i++) {
+ if (lower_64k)
+ if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
+ return EACCES;
+
+ /* In 8K area BPR has both read and write protection bits */
+ bpr_ptr += 2;
+ }
+
+ /* Process upper 8K block regions */
+ for (i = 0; i < SST26_BPR_8K_NUM; i++) {
+ if (upper_64k)
+ if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
+ return EACCES;
+
+ /* In 8K area BPR has both read and write protection bits */
+ bpr_ptr += 2;
+ }
+
+ /* If we check region status we don't need to write BPR back */
+ if (ctl == SST26_CTL_CHECK)
+ return 0;
+
+ ret = nor->write_reg(nor, SPINOR_OP_WRITE_BPR, bpr_buff, bpr_size);
+ if (ret < 0) {
+ dev_err(nor->dev, "fail to write block-protection register\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int sst26_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+ return sst26_lock_ctl(nor, ofs, len, SST26_CTL_UNLOCK);
+}
+
+static int sst26_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+ return sst26_lock_ctl(nor, ofs, len, SST26_CTL_LOCK);
+}
+
+/*
+ * Returns EACCES (positive value) if region is locked, 0 if region is unlocked,
+ * and negative on errors.
+ */
+static int sst26_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
+{
+ /*
+ * is_locked function is used for check before reading or erasing flash
+ * region, so offset and length might be not 64k allighned, so adjust
+ * them to be 64k allighned as sst26_lock_ctl works only with 64k
+ * allighned regions.
+ */
+ ofs -= ofs & (SZ_64K - 1);
+ len = len & (SZ_64K - 1) ? (len & ~(SZ_64K - 1)) + SZ_64K : len;
+
+ return sst26_lock_ctl(nor, ofs, len, SST26_CTL_CHECK);
+}
+
static int sst_write_byteprogram(struct spi_nor *nor, loff_t to, size_t len,
size_t *retlen, const u_char *buf)
{
@@ -2302,6 +2473,16 @@ int spi_nor_scan(struct spi_nor *nor)
#endif
#ifdef CONFIG_SPI_FLASH_SST
+ /*
+ * sst26 series block protection implementation differs from other
+ * series.
+ */
+ if (info->flags & SPI_NOR_HAS_SST26LOCK) {
+ nor->flash_lock = sst26_lock;
+ nor->flash_unlock = sst26_unlock;
+ nor->flash_is_locked = sst26_is_locked;
+ }
+
/* sst nor chips use AAI word program */
if (info->flags & SST_WRITE)
mtd->_write = sst_write;
diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
index a3920ba520e..6996c0a2864 100644
--- a/drivers/mtd/spi/spi-nor-ids.c
+++ b/drivers/mtd/spi/spi-nor-ids.c
@@ -214,10 +214,10 @@ const struct flash_info spi_nor_ids[] = {
{ INFO("sst25wf040b", 0x621613, 0, 64 * 1024, 8, SECT_4K) },
{ INFO("sst25wf040", 0xbf2504, 0, 64 * 1024, 8, SECT_4K | SST_WRITE) },
{ INFO("sst25wf080", 0xbf2505, 0, 64 * 1024, 16, SECT_4K | SST_WRITE) },
- { INFO("sst26vf064b", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
- { INFO("sst26wf016", 0xbf2651, 0, 64 * 1024, 32, SECT_4K) },
- { INFO("sst26wf032", 0xbf2622, 0, 64 * 1024, 64, SECT_4K) },
- { INFO("sst26wf064", 0xbf2643, 0, 64 * 1024, 128, SECT_4K) },
+ { INFO("sst26vf064b", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_SST26LOCK | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+ { INFO("sst26wf016", 0xbf2651, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
+ { INFO("sst26wf032", 0xbf2622, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
+ { INFO("sst26wf064", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
#endif
#ifdef CONFIG_SPI_FLASH_STMICRO /* STMICRO */
/* ST Microelectronics -- newer production may have feature updates */
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 88e80af5794..709b49d3936 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -91,6 +91,10 @@
#define SPINOR_OP_WRDI 0x04 /* Write disable */
#define SPINOR_OP_AAI_WP 0xad /* Auto address increment word program */
+/* Used for SST26* flashes only. */
+#define SPINOR_OP_READ_BPR 0x72 /* Read block protection register */
+#define SPINOR_OP_WRITE_BPR 0x42 /* Write block protection register */
+
/* Used for S3AN flashes only */
#define SPINOR_OP_XSE 0x50 /* Sector erase */
#define SPINOR_OP_XPP 0x82 /* Page program */
--
2.21.0
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
2019-09-04 18:07 [U-Boot] Regressions in MTD / SPI FLASH Eugeniy Paltsev
2019-09-04 18:07 ` [U-Boot] [PATCH 1/1] MTD: SPI: revert removing SST26* flash IC protection ops Eugeniy Paltsev
@ 2019-09-06 7:36 ` Vignesh Raghavendra
2019-09-09 18:48 ` Eugeniy Paltsev
2019-09-09 7:59 ` Schrempf Frieder
2 siblings, 1 reply; 15+ messages in thread
From: Vignesh Raghavendra @ 2019-09-06 7:36 UTC (permalink / raw)
To: u-boot
Hi,
On 04/09/19 11:37 PM, Eugeniy Paltsev wrote:
> We faced with regressions caused by
> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
> This switch was performed by removing entire u-boot spi-flash
> core implementation and copying it from another project.
> However the switch is performed without proper testing and
> investigations about fixes/improvements were made in u-boot
> spi-flash core. This results in regressions.
>
Apologies for the trouble...
As stated in cover letter, this change was necessary as U-Boot SPI flash
stack at that time did not features such as 4 byte addressing, SFDP
parsing, SPI controllers with MMIO interfaces etc. Also there was need
to move to SPI MEM framework to support both SPI NAND and SPI NOR
flashes using a single SPI controller drivers.
I have to disagree on the part that there was no proper testing... As
evident from mailing list archives, patch series was reviewed by
multiple reviewers and tested on their platforms as well...
Unfortunately its impossible to get all boards owners to test it.
> One of regression we faced with is related to SST26 flash series which
> is used on HSDK board. The cause is that SST26 protection ops
> implementation was dropped. The fix of this regression is send
> as a patch in this series.
>
I retained most U-Boot specific code as is (like support for BANK
address registers, restriction in transfer sizes) but I somehow
overlooked this part. Sorry for that
> However there are another regressions. I.E: we also faced with broken
> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>
Could you provide more details here:
What exactly fails? Is the detected correctly? Does reads work fine? Is
Erase or Write broken?
Could you enable debug prints in your driver as well as debug prints in
spi_mem_exec_op() (in drivers/spi/spi-mem.c) and post the logs?
Regards
Vignesh
> I can also expect regressions among other u-boot users
> and I believe that subsystem changes mustn't be done such harmful way.
>
> Eugeniy Paltsev (1):
> MTD: SPI: revert removing SST26* flash IC protection ops
>
> drivers/mtd/spi/sf_internal.h | 1 +
> drivers/mtd/spi/spi-nor-core.c | 181 +++++++++++++++++++++++++++++++++
> drivers/mtd/spi/spi-nor-ids.c | 8 +-
> include/linux/mtd/spi-nor.h | 4 +
> 4 files changed, 190 insertions(+), 4 deletions(-)
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] [PATCH 1/1] MTD: SPI: revert removing SST26* flash IC protection ops
2019-09-04 18:07 ` [U-Boot] [PATCH 1/1] MTD: SPI: revert removing SST26* flash IC protection ops Eugeniy Paltsev
@ 2019-09-07 3:40 ` Jagan Teki
2019-09-07 17:04 ` Tom Rini
2019-09-09 18:55 ` Eugeniy Paltsev
0 siblings, 2 replies; 15+ messages in thread
From: Jagan Teki @ 2019-09-07 3:40 UTC (permalink / raw)
To: u-boot
On Wed, Sep 4, 2019 at 11:37 PM Eugeniy Paltsev
<Eugeniy.Paltsev@synopsys.com> wrote:
>
> Commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
> performs switch from previous 'spi_flash' infrastructure without
> proper testing/investigations which results in regressions.
>
> Fix regression related to SST26 flash IC series which is lacking
> protection ops implementation which were introduced previously by
> Commit 3d4fed87a5fa (mtd: sf: Add support of sst26wf* flash ICs
> protection ops)
>
> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> ---
> Tom, could you please pick this patch to 2019.10?
>
> drivers/mtd/spi/sf_internal.h | 1 +
> drivers/mtd/spi/spi-nor-core.c | 181 +++++++++++++++++++++++++++++++++
> drivers/mtd/spi/spi-nor-ids.c | 8 +-
> include/linux/mtd/spi-nor.h | 4 +
> 4 files changed, 190 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
> index a6bf734830a..e6da768bf36 100644
> --- a/drivers/mtd/spi/sf_internal.h
> +++ b/drivers/mtd/spi/sf_internal.h
> @@ -65,6 +65,7 @@ struct flash_info {
> #define NO_CHIP_ERASE BIT(12) /* Chip does not support chip erase */
> #define SPI_NOR_SKIP_SFDP BIT(13) /* Skip parsing of SFDP tables */
> #define USE_CLSR BIT(14) /* use CLSR command */
> +#define SPI_NOR_HAS_SST26LOCK BIT(15) /* Flash supports lock/unlock via BPR */
> };
>
> extern const struct flash_info spi_nor_ids[];
> diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> index 1acff745d1a..990e39d7c2f 100644
> --- a/drivers/mtd/spi/spi-nor-core.c
> +++ b/drivers/mtd/spi/spi-nor-core.c
> @@ -945,6 +945,177 @@ read_err:
> }
>
> #ifdef CONFIG_SPI_FLASH_SST
> +/*
> + * sst26 flash series has its own block protection implementation:
> + * 4x - 8 KByte blocks - read & write protection bits - upper addresses
> + * 1x - 32 KByte blocks - write protection bits
> + * rest - 64 KByte blocks - write protection bits
> + * 1x - 32 KByte blocks - write protection bits
> + * 4x - 8 KByte blocks - read & write protection bits - lower addresses
> + *
> + * We'll support only per 64k lock/unlock so lower and upper 64 KByte region
> + * will be treated as single block.
> + */
> +#define SST26_BPR_8K_NUM 4
> +#define SST26_MAX_BPR_REG_LEN (18 + 1)
> +#define SST26_BOUND_REG_SIZE ((32 + SST26_BPR_8K_NUM * 8) * SZ_1K)
> +
> +enum lock_ctl {
> + SST26_CTL_LOCK,
> + SST26_CTL_UNLOCK,
> + SST26_CTL_CHECK
> +};
> +
> +static bool sst26_process_bpr(u32 bpr_size, u8 *cmd, u32 bit, enum lock_ctl ctl)
> +{
> + switch (ctl) {
> + case SST26_CTL_LOCK:
> + cmd[bpr_size - (bit / 8) - 1] |= BIT(bit % 8);
> + break;
> + case SST26_CTL_UNLOCK:
> + cmd[bpr_size - (bit / 8) - 1] &= ~BIT(bit % 8);
> + break;
> + case SST26_CTL_CHECK:
> + return !!(cmd[bpr_size - (bit / 8) - 1] & BIT(bit % 8));
> + }
> +
> + return false;
> +}
> +
> +/*
> + * Lock, unlock or check lock status of the flash region of the flash (depending
> + * on the lock_ctl value)
> + */
> +static int sst26_lock_ctl(struct spi_nor *nor, loff_t ofs, uint64_t len, enum lock_ctl ctl)
> +{
> + struct mtd_info *mtd = &nor->mtd;
> + u32 i, bpr_ptr, rptr_64k, lptr_64k, bpr_size;
> + bool lower_64k = false, upper_64k = false;
> + u8 bpr_buff[SST26_MAX_BPR_REG_LEN] = {};
> + int ret;
> +
> + /* Check length and offset for 64k alignment */
> + if ((ofs & (SZ_64K - 1)) || (len & (SZ_64K - 1))) {
> + dev_err(nor->dev, "length or offset is not 64KiB allighned\n");
> + return -EINVAL;
> + }
> +
> + if (ofs + len > mtd->size) {
> + dev_err(nor->dev, "range is more than device size: %#llx + %#llx > %#llx\n",
> + ofs, len, mtd->size);
> + return -EINVAL;
> + }
> +
> + /* SST26 family has only 16 Mbit, 32 Mbit and 64 Mbit IC */
> + if (mtd->size != SZ_2M &&
> + mtd->size != SZ_4M &&
> + mtd->size != SZ_8M)
> + return -EINVAL;
> +
> + bpr_size = 2 + (mtd->size / SZ_64K / 8);
> +
> + ret = nor->read_reg(nor, SPINOR_OP_READ_BPR, bpr_buff, bpr_size);
> + if (ret < 0) {
> + dev_err(nor->dev, "fail to read block-protection register\n");
> + return ret;
> + }
> +
> + rptr_64k = min_t(u32, ofs + len, mtd->size - SST26_BOUND_REG_SIZE);
> + lptr_64k = max_t(u32, ofs, SST26_BOUND_REG_SIZE);
> +
> + upper_64k = ((ofs + len) > (mtd->size - SST26_BOUND_REG_SIZE));
> + lower_64k = (ofs < SST26_BOUND_REG_SIZE);
> +
> + /* Lower bits in block-protection register are about 64k region */
> + bpr_ptr = lptr_64k / SZ_64K - 1;
> +
> + /* Process 64K blocks region */
> + while (lptr_64k < rptr_64k) {
> + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> + return EACCES;
> +
> + bpr_ptr++;
> + lptr_64k += SZ_64K;
> + }
> +
> + /* 32K and 8K region bits in BPR are after 64k region bits */
> + bpr_ptr = (mtd->size - 2 * SST26_BOUND_REG_SIZE) / SZ_64K;
> +
> + /* Process lower 32K block region */
> + if (lower_64k)
> + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> + return EACCES;
> +
> + bpr_ptr++;
> +
> + /* Process upper 32K block region */
> + if (upper_64k)
> + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> + return EACCES;
> +
> + bpr_ptr++;
> +
> + /* Process lower 8K block regions */
> + for (i = 0; i < SST26_BPR_8K_NUM; i++) {
> + if (lower_64k)
> + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> + return EACCES;
> +
> + /* In 8K area BPR has both read and write protection bits */
> + bpr_ptr += 2;
> + }
> +
> + /* Process upper 8K block regions */
> + for (i = 0; i < SST26_BPR_8K_NUM; i++) {
> + if (upper_64k)
> + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> + return EACCES;
> +
> + /* In 8K area BPR has both read and write protection bits */
> + bpr_ptr += 2;
> + }
> +
> + /* If we check region status we don't need to write BPR back */
> + if (ctl == SST26_CTL_CHECK)
> + return 0;
> +
> + ret = nor->write_reg(nor, SPINOR_OP_WRITE_BPR, bpr_buff, bpr_size);
> + if (ret < 0) {
> + dev_err(nor->dev, "fail to write block-protection register\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int sst26_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> + return sst26_lock_ctl(nor, ofs, len, SST26_CTL_UNLOCK);
> +}
> +
> +static int sst26_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> + return sst26_lock_ctl(nor, ofs, len, SST26_CTL_LOCK);
> +}
> +
> +/*
> + * Returns EACCES (positive value) if region is locked, 0 if region is unlocked,
> + * and negative on errors.
> + */
> +static int sst26_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
> +{
> + /*
> + * is_locked function is used for check before reading or erasing flash
> + * region, so offset and length might be not 64k allighned, so adjust
> + * them to be 64k allighned as sst26_lock_ctl works only with 64k
> + * allighned regions.
> + */
> + ofs -= ofs & (SZ_64K - 1);
> + len = len & (SZ_64K - 1) ? (len & ~(SZ_64K - 1)) + SZ_64K : len;
> +
> + return sst26_lock_ctl(nor, ofs, len, SST26_CTL_CHECK);
> +}
> +
> static int sst_write_byteprogram(struct spi_nor *nor, loff_t to, size_t len,
> size_t *retlen, const u_char *buf)
> {
> @@ -2302,6 +2473,16 @@ int spi_nor_scan(struct spi_nor *nor)
> #endif
>
> #ifdef CONFIG_SPI_FLASH_SST
> + /*
> + * sst26 series block protection implementation differs from other
> + * series.
> + */
> + if (info->flags & SPI_NOR_HAS_SST26LOCK) {
> + nor->flash_lock = sst26_lock;
> + nor->flash_unlock = sst26_unlock;
> + nor->flash_is_locked = sst26_is_locked;
> + }
> +
> /* sst nor chips use AAI word program */
> if (info->flags & SST_WRITE)
> mtd->_write = sst_write;
> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
> index a3920ba520e..6996c0a2864 100644
> --- a/drivers/mtd/spi/spi-nor-ids.c
> +++ b/drivers/mtd/spi/spi-nor-ids.c
> @@ -214,10 +214,10 @@ const struct flash_info spi_nor_ids[] = {
> { INFO("sst25wf040b", 0x621613, 0, 64 * 1024, 8, SECT_4K) },
> { INFO("sst25wf040", 0xbf2504, 0, 64 * 1024, 8, SECT_4K | SST_WRITE) },
> { INFO("sst25wf080", 0xbf2505, 0, 64 * 1024, 16, SECT_4K | SST_WRITE) },
> - { INFO("sst26vf064b", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> - { INFO("sst26wf016", 0xbf2651, 0, 64 * 1024, 32, SECT_4K) },
> - { INFO("sst26wf032", 0xbf2622, 0, 64 * 1024, 64, SECT_4K) },
> - { INFO("sst26wf064", 0xbf2643, 0, 64 * 1024, 128, SECT_4K) },
> + { INFO("sst26vf064b", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_SST26LOCK | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + { INFO("sst26wf016", 0xbf2651, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
> + { INFO("sst26wf032", 0xbf2622, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
> + { INFO("sst26wf064", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
Thought the commit message says it is revert, the patch contents seems
adding new changes. So, better add them by saying missing
functionalities. If possible please break the patches into multiple
patches.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] [PATCH 1/1] MTD: SPI: revert removing SST26* flash IC protection ops
2019-09-07 3:40 ` Jagan Teki
@ 2019-09-07 17:04 ` Tom Rini
2019-09-09 18:55 ` Eugeniy Paltsev
1 sibling, 0 replies; 15+ messages in thread
From: Tom Rini @ 2019-09-07 17:04 UTC (permalink / raw)
To: u-boot
On Sat, Sep 07, 2019 at 09:10:39AM +0530, Jagan Teki wrote:
> On Wed, Sep 4, 2019 at 11:37 PM Eugeniy Paltsev
> <Eugeniy.Paltsev@synopsys.com> wrote:
> >
> > Commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
> > performs switch from previous 'spi_flash' infrastructure without
> > proper testing/investigations which results in regressions.
> >
> > Fix regression related to SST26 flash IC series which is lacking
> > protection ops implementation which were introduced previously by
> > Commit 3d4fed87a5fa (mtd: sf: Add support of sst26wf* flash ICs
> > protection ops)
> >
> > Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
> > ---
> > Tom, could you please pick this patch to 2019.10?
> >
> > drivers/mtd/spi/sf_internal.h | 1 +
> > drivers/mtd/spi/spi-nor-core.c | 181 +++++++++++++++++++++++++++++++++
> > drivers/mtd/spi/spi-nor-ids.c | 8 +-
> > include/linux/mtd/spi-nor.h | 4 +
> > 4 files changed, 190 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mtd/spi/sf_internal.h b/drivers/mtd/spi/sf_internal.h
> > index a6bf734830a..e6da768bf36 100644
> > --- a/drivers/mtd/spi/sf_internal.h
> > +++ b/drivers/mtd/spi/sf_internal.h
> > @@ -65,6 +65,7 @@ struct flash_info {
> > #define NO_CHIP_ERASE BIT(12) /* Chip does not support chip erase */
> > #define SPI_NOR_SKIP_SFDP BIT(13) /* Skip parsing of SFDP tables */
> > #define USE_CLSR BIT(14) /* use CLSR command */
> > +#define SPI_NOR_HAS_SST26LOCK BIT(15) /* Flash supports lock/unlock via BPR */
> > };
> >
> > extern const struct flash_info spi_nor_ids[];
> > diff --git a/drivers/mtd/spi/spi-nor-core.c b/drivers/mtd/spi/spi-nor-core.c
> > index 1acff745d1a..990e39d7c2f 100644
> > --- a/drivers/mtd/spi/spi-nor-core.c
> > +++ b/drivers/mtd/spi/spi-nor-core.c
> > @@ -945,6 +945,177 @@ read_err:
> > }
> >
> > #ifdef CONFIG_SPI_FLASH_SST
> > +/*
> > + * sst26 flash series has its own block protection implementation:
> > + * 4x - 8 KByte blocks - read & write protection bits - upper addresses
> > + * 1x - 32 KByte blocks - write protection bits
> > + * rest - 64 KByte blocks - write protection bits
> > + * 1x - 32 KByte blocks - write protection bits
> > + * 4x - 8 KByte blocks - read & write protection bits - lower addresses
> > + *
> > + * We'll support only per 64k lock/unlock so lower and upper 64 KByte region
> > + * will be treated as single block.
> > + */
> > +#define SST26_BPR_8K_NUM 4
> > +#define SST26_MAX_BPR_REG_LEN (18 + 1)
> > +#define SST26_BOUND_REG_SIZE ((32 + SST26_BPR_8K_NUM * 8) * SZ_1K)
> > +
> > +enum lock_ctl {
> > + SST26_CTL_LOCK,
> > + SST26_CTL_UNLOCK,
> > + SST26_CTL_CHECK
> > +};
> > +
> > +static bool sst26_process_bpr(u32 bpr_size, u8 *cmd, u32 bit, enum lock_ctl ctl)
> > +{
> > + switch (ctl) {
> > + case SST26_CTL_LOCK:
> > + cmd[bpr_size - (bit / 8) - 1] |= BIT(bit % 8);
> > + break;
> > + case SST26_CTL_UNLOCK:
> > + cmd[bpr_size - (bit / 8) - 1] &= ~BIT(bit % 8);
> > + break;
> > + case SST26_CTL_CHECK:
> > + return !!(cmd[bpr_size - (bit / 8) - 1] & BIT(bit % 8));
> > + }
> > +
> > + return false;
> > +}
> > +
> > +/*
> > + * Lock, unlock or check lock status of the flash region of the flash (depending
> > + * on the lock_ctl value)
> > + */
> > +static int sst26_lock_ctl(struct spi_nor *nor, loff_t ofs, uint64_t len, enum lock_ctl ctl)
> > +{
> > + struct mtd_info *mtd = &nor->mtd;
> > + u32 i, bpr_ptr, rptr_64k, lptr_64k, bpr_size;
> > + bool lower_64k = false, upper_64k = false;
> > + u8 bpr_buff[SST26_MAX_BPR_REG_LEN] = {};
> > + int ret;
> > +
> > + /* Check length and offset for 64k alignment */
> > + if ((ofs & (SZ_64K - 1)) || (len & (SZ_64K - 1))) {
> > + dev_err(nor->dev, "length or offset is not 64KiB allighned\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (ofs + len > mtd->size) {
> > + dev_err(nor->dev, "range is more than device size: %#llx + %#llx > %#llx\n",
> > + ofs, len, mtd->size);
> > + return -EINVAL;
> > + }
> > +
> > + /* SST26 family has only 16 Mbit, 32 Mbit and 64 Mbit IC */
> > + if (mtd->size != SZ_2M &&
> > + mtd->size != SZ_4M &&
> > + mtd->size != SZ_8M)
> > + return -EINVAL;
> > +
> > + bpr_size = 2 + (mtd->size / SZ_64K / 8);
> > +
> > + ret = nor->read_reg(nor, SPINOR_OP_READ_BPR, bpr_buff, bpr_size);
> > + if (ret < 0) {
> > + dev_err(nor->dev, "fail to read block-protection register\n");
> > + return ret;
> > + }
> > +
> > + rptr_64k = min_t(u32, ofs + len, mtd->size - SST26_BOUND_REG_SIZE);
> > + lptr_64k = max_t(u32, ofs, SST26_BOUND_REG_SIZE);
> > +
> > + upper_64k = ((ofs + len) > (mtd->size - SST26_BOUND_REG_SIZE));
> > + lower_64k = (ofs < SST26_BOUND_REG_SIZE);
> > +
> > + /* Lower bits in block-protection register are about 64k region */
> > + bpr_ptr = lptr_64k / SZ_64K - 1;
> > +
> > + /* Process 64K blocks region */
> > + while (lptr_64k < rptr_64k) {
> > + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> > + return EACCES;
> > +
> > + bpr_ptr++;
> > + lptr_64k += SZ_64K;
> > + }
> > +
> > + /* 32K and 8K region bits in BPR are after 64k region bits */
> > + bpr_ptr = (mtd->size - 2 * SST26_BOUND_REG_SIZE) / SZ_64K;
> > +
> > + /* Process lower 32K block region */
> > + if (lower_64k)
> > + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> > + return EACCES;
> > +
> > + bpr_ptr++;
> > +
> > + /* Process upper 32K block region */
> > + if (upper_64k)
> > + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> > + return EACCES;
> > +
> > + bpr_ptr++;
> > +
> > + /* Process lower 8K block regions */
> > + for (i = 0; i < SST26_BPR_8K_NUM; i++) {
> > + if (lower_64k)
> > + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> > + return EACCES;
> > +
> > + /* In 8K area BPR has both read and write protection bits */
> > + bpr_ptr += 2;
> > + }
> > +
> > + /* Process upper 8K block regions */
> > + for (i = 0; i < SST26_BPR_8K_NUM; i++) {
> > + if (upper_64k)
> > + if (sst26_process_bpr(bpr_size, bpr_buff, bpr_ptr, ctl))
> > + return EACCES;
> > +
> > + /* In 8K area BPR has both read and write protection bits */
> > + bpr_ptr += 2;
> > + }
> > +
> > + /* If we check region status we don't need to write BPR back */
> > + if (ctl == SST26_CTL_CHECK)
> > + return 0;
> > +
> > + ret = nor->write_reg(nor, SPINOR_OP_WRITE_BPR, bpr_buff, bpr_size);
> > + if (ret < 0) {
> > + dev_err(nor->dev, "fail to write block-protection register\n");
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int sst26_unlock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > +{
> > + return sst26_lock_ctl(nor, ofs, len, SST26_CTL_UNLOCK);
> > +}
> > +
> > +static int sst26_lock(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > +{
> > + return sst26_lock_ctl(nor, ofs, len, SST26_CTL_LOCK);
> > +}
> > +
> > +/*
> > + * Returns EACCES (positive value) if region is locked, 0 if region is unlocked,
> > + * and negative on errors.
> > + */
> > +static int sst26_is_locked(struct spi_nor *nor, loff_t ofs, uint64_t len)
> > +{
> > + /*
> > + * is_locked function is used for check before reading or erasing flash
> > + * region, so offset and length might be not 64k allighned, so adjust
> > + * them to be 64k allighned as sst26_lock_ctl works only with 64k
> > + * allighned regions.
> > + */
> > + ofs -= ofs & (SZ_64K - 1);
> > + len = len & (SZ_64K - 1) ? (len & ~(SZ_64K - 1)) + SZ_64K : len;
> > +
> > + return sst26_lock_ctl(nor, ofs, len, SST26_CTL_CHECK);
> > +}
> > +
> > static int sst_write_byteprogram(struct spi_nor *nor, loff_t to, size_t len,
> > size_t *retlen, const u_char *buf)
> > {
> > @@ -2302,6 +2473,16 @@ int spi_nor_scan(struct spi_nor *nor)
> > #endif
> >
> > #ifdef CONFIG_SPI_FLASH_SST
> > + /*
> > + * sst26 series block protection implementation differs from other
> > + * series.
> > + */
> > + if (info->flags & SPI_NOR_HAS_SST26LOCK) {
> > + nor->flash_lock = sst26_lock;
> > + nor->flash_unlock = sst26_unlock;
> > + nor->flash_is_locked = sst26_is_locked;
> > + }
> > +
> > /* sst nor chips use AAI word program */
> > if (info->flags & SST_WRITE)
> > mtd->_write = sst_write;
> > diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
> > index a3920ba520e..6996c0a2864 100644
> > --- a/drivers/mtd/spi/spi-nor-ids.c
> > +++ b/drivers/mtd/spi/spi-nor-ids.c
> > @@ -214,10 +214,10 @@ const struct flash_info spi_nor_ids[] = {
> > { INFO("sst25wf040b", 0x621613, 0, 64 * 1024, 8, SECT_4K) },
> > { INFO("sst25wf040", 0xbf2504, 0, 64 * 1024, 8, SECT_4K | SST_WRITE) },
> > { INFO("sst25wf080", 0xbf2505, 0, 64 * 1024, 16, SECT_4K | SST_WRITE) },
> > - { INFO("sst26vf064b", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > - { INFO("sst26wf016", 0xbf2651, 0, 64 * 1024, 32, SECT_4K) },
> > - { INFO("sst26wf032", 0xbf2622, 0, 64 * 1024, 64, SECT_4K) },
> > - { INFO("sst26wf064", 0xbf2643, 0, 64 * 1024, 128, SECT_4K) },
> > + { INFO("sst26vf064b", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_SST26LOCK | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> > + { INFO("sst26wf016", 0xbf2651, 0, 64 * 1024, 32, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
> > + { INFO("sst26wf032", 0xbf2622, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
> > + { INFO("sst26wf064", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
>
> Thought the commit message says it is revert, the patch contents seems
> adding new changes. So, better add them by saying missing
> functionalities. If possible please break the patches into multiple
> patches.
At this point in the release cycle, I would like to see us turn around
on this quickly such that the overall functionality is restored for this
upcoming release. Thanks in advance all!
--
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190907/ca133924/attachment.sig>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
2019-09-04 18:07 [U-Boot] Regressions in MTD / SPI FLASH Eugeniy Paltsev
2019-09-04 18:07 ` [U-Boot] [PATCH 1/1] MTD: SPI: revert removing SST26* flash IC protection ops Eugeniy Paltsev
2019-09-06 7:36 ` [U-Boot] Regressions in MTD / SPI FLASH Vignesh Raghavendra
@ 2019-09-09 7:59 ` Schrempf Frieder
2 siblings, 0 replies; 15+ messages in thread
From: Schrempf Frieder @ 2019-09-09 7:59 UTC (permalink / raw)
To: u-boot
Hi Eugeniy,
On 04.09.19 20:07, Eugeniy Paltsev wrote:
> We faced with regressions caused by
> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
> This switch was performed by removing entire u-boot spi-flash
> core implementation and copying it from another project.
> However the switch is performed without proper testing and
> investigations about fixes/improvements were made in u-boot
> spi-flash core. This results in regressions.
>
> One of regression we faced with is related to SST26 flash series which
> is used on HSDK board. The cause is that SST26 protection ops
> implementation was dropped. The fix of this regression is send
> as a patch in this series.
>
> However there are another regressions. I.E: we also faced with broken
> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>
> I can also expect regressions among other u-boot users
> and I believe that subsystem changes mustn't be done such harmful way.
Actually such changes are only as much "harmful" as they are not tested
on all of the hardware and this depends only on how many developers with
specific boards step up to test these changes.
As Vignesh already explained, syncing the framework from Linux to U-Boot
was desperately needed and I'm very glad, that he did the work.
Otherwise we would still be stuck with rather old and unmaintainable
code, that would even cause more issues in the long term.
Regards
Frieder
>
> Eugeniy Paltsev (1):
> MTD: SPI: revert removing SST26* flash IC protection ops
>
> drivers/mtd/spi/sf_internal.h | 1 +
> drivers/mtd/spi/spi-nor-core.c | 181 +++++++++++++++++++++++++++++++++
> drivers/mtd/spi/spi-nor-ids.c | 8 +-
> include/linux/mtd/spi-nor.h | 4 +
> 4 files changed, 190 insertions(+), 4 deletions(-)
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
2019-09-06 7:36 ` [U-Boot] Regressions in MTD / SPI FLASH Vignesh Raghavendra
@ 2019-09-09 18:48 ` Eugeniy Paltsev
2019-09-10 5:07 ` Vignesh Raghavendra
0 siblings, 1 reply; 15+ messages in thread
From: Eugeniy Paltsev @ 2019-09-09 18:48 UTC (permalink / raw)
To: u-boot
Hi!
Comments are inlined:
>On 04/09/19 11:37 PM, Eugeniy Paltsev wrote:
>> We faced with regressions caused by
>> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
>> This switch was performed by removing entire u-boot spi-flash
>> core implementation and copying it from another project.
>> However the switch is performed without proper testing and
>> investigations about fixes/improvements were made in u-boot
>> spi-flash core. This results in regressions.
>>
>
>Apologies for the trouble...
>As stated in cover letter, this change was necessary as U-Boot SPI flash
>stack at that time did not features such as 4 byte addressing, SFDP
>parsing, SPI controllers with MMIO interfaces etc. Also there was need
>to move to SPI MEM framework to support both SPI NAND and SPI NOR
>flashes using a single SPI controller drivers.
>I have to disagree on the part that there was no proper testing... As
>evident from mailing list archives, patch series was reviewed by
>multiple reviewers and tested on their platforms as well...
>Unfortunately its impossible to get all boards owners to test it.
I'm not talking about getting all customers board and testing changes on them.
It could be done another way - i.e. like it is done for u-boot driver-model migration:
by introducing the option for choosing which stack will be used and allow customers
to switch the flash IC they use to new stack until some deadline.
>> One of regression we faced with is related to SST26 flash series which
>> is used on HSDK board. The cause is that SST26 protection ops
>> implementation was dropped. The fix of this regression is send
>> as a patch in this series.
>>
>
>I retained most U-Boot specific code as is (like support for BANK
>address registers, restriction in transfer sizes) but I somehow
>overlooked this part. Sorry for that
>
>> However there are another regressions. I.E: we also faced with broken
>> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
>> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>>
>
>Could you provide more details here:
>What exactly fails? Is the detected correctly? Does reads work fine? Is
>Erase or Write broken?
It seems to me that something is wrong with protection ops.
The erase and write finish without errors however nothing actually happens.
>Could you enable debug prints in your driver as well as debug prints in
>spi_mem_exec_op() (in drivers/spi/spi-mem.c) and post the logs?
Here is it.
As you can see all commands finish successfully however erase and write don't
change flash content.
------------------------------------->8-----------------------------------------
AXS# sf probe && echo OK
spi_flash_std_probe: slave=9fdabfc0, cs=0
9f | [6B in] 20 ba 20 10 00 00 [ret 0]
SF: Detected n25q512ax3 with page size 256 Bytes, erase size 4 KiB, total 64 MiB
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
0c 00 18 00 00 | [16B in] 53 00 f8 d9 04 00 52 fc ff ff 80 de ad be af af [ret 0]
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: 53 00 f8 d9 04 00 52 fc ff ff 80 de ad be af af S.....R.........
AXS# sf protect unlock 0x0 0x4000000 && echo OK
05 | [1B in] 00 [ret 0]
OK
AXS# sf erase 0x180000 0x1000 && echo OK
06 | [0B -] [ret 0]
21 00 18 00 00 | [0B -] [ret 0]
05 | [1B in] 02 [ret 0]
70 | [1B in] 80 [ret 0]
04 | [0B -] [ret 0]
SF: 4096 bytes @ 0x180000 Erased: OK
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
0c 00 18 00 00 | [16B in] 53 00 f8 d9 04 00 52 fc ff ff 80 de ad be af af [ret 0]
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: 53 00 f8 d9 04 00 52 fc ff ff 80 de ad be af af S.....R.........
AXS# mw 0x81000000 0 5
AXS# sf write 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
06 | [0B -] [ret 0]
12 00 18 00 00 | [16B out] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ret 0]
05 | [1B in] 00 [ret 0]
70 | [1B in] 80 [ret 0]
SF: 16 bytes @ 0x180000 Written: OK
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
0c 00 18 00 00 | [16B in] 53 00 f8 d9 04 00 52 fc ff ff 80 de ad be af af [ret 0]
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: 53 00 f8 d9 04 00 52 fc ff ff 80 de ad be af af S.....R.........
------------------------------------->8-----------------------------------------
Here is how it should work (tested on v2018.09):
------------------------------------->8-----------------------------------------
AXS# sf probe && echo OK
SF: Detected n25q512 with page size 256 Bytes, erase size 4 KiB, total 64 MiB
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: 53 00 f8 d9 04 00 52 fc ff ff 80 de ad be af af S.....R.........
AXS# sf protect unlock 0x0 0x4000000 && echo OK
AXS#
AXS# sf erase 0x180000 0x1000 && echo OK
SF: 4096 bytes @ 0x180000 Erased: OK
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
AXS# mw 0x81000000 0 5
AXS# sf write 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
SF: 16 bytes @ 0x180000 Written: OK
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
------------------------------------->8-----------------------------------------
>
>Regards
>Vignesh
>
>> I can also expect regressions among other u-boot users
>> and I believe that subsystem changes mustn't be done such harmful way.
>>
>> Eugeniy Paltsev (1):
>> MTD: SPI: revert removing SST26* flash IC protection ops
>>
>> drivers/mtd/spi/sf_internal.h | 1 +
>> drivers/mtd/spi/spi-nor-core.c | 181 +++++++++++++++++++++++++++++++++
>> drivers/mtd/spi/spi-nor-ids.c | 8 +-
>> include/linux/mtd/spi-nor.h | 4 +
>> 4 files changed, 190 insertions(+), 4 deletions(-)
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] [PATCH 1/1] MTD: SPI: revert removing SST26* flash IC protection ops
2019-09-07 3:40 ` Jagan Teki
2019-09-07 17:04 ` Tom Rini
@ 2019-09-09 18:55 ` Eugeniy Paltsev
1 sibling, 0 replies; 15+ messages in thread
From: Eugeniy Paltsev @ 2019-09-09 18:55 UTC (permalink / raw)
To: u-boot
>On Wed, Sep 4, 2019 at 11:37 PM Eugeniy Paltsev
><Eugeniy.Paltsev@synopsys.com> wrote:
>>
>> Commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
>> performs switch from previous 'spi_flash' infrastructure without
>> proper testing/investigations which results in regressions.
>>
>> Fix regression related to SST26 flash IC series which is lacking
>> protection ops implementation which were introduced previously by
>> Commit 3d4fed87a5fa (mtd: sf: Add support of sst26wf* flash ICs
>> protection ops)
>>
>> Signed-off-by: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com>
>> ---
>> Tom, could you please pick this patch to 2019.10?
>>
[snip]
>> + { INFO("sst26wf032", 0xbf2622, 0, 64 * 1024, 64, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
>> + { INFO("sst26wf064", 0xbf2643, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_HAS_SST26LOCK) },
>
>Thought the commit message says it is revert, the patch contents seems
>adding new changes.
Revert of removing functionality is adding functionality, isn't it? ;)
> So, better add them by saying missing
>functionalities. If possible please break the patches into multiple
>patches.
Ok, I'll split this patch for adding SST26 protection ops part and
applying this ops to corresponding flash ICs.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
2019-09-09 18:48 ` Eugeniy Paltsev
@ 2019-09-10 5:07 ` Vignesh Raghavendra
2019-09-10 11:41 ` Eugeniy Paltsev
0 siblings, 1 reply; 15+ messages in thread
From: Vignesh Raghavendra @ 2019-09-10 5:07 UTC (permalink / raw)
To: u-boot
Hi,
On 10/09/19 12:18 AM, Eugeniy Paltsev wrote:
> Hi!
> Comments are inlined:
>
>> On 04/09/19 11:37 PM, Eugeniy Paltsev wrote:
>>> We faced with regressions caused by
>>> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
>>> This switch was performed by removing entire u-boot spi-flash
>>> core implementation and copying it from another project.
>>> However the switch is performed without proper testing and
>>> investigations about fixes/improvements were made in u-boot
>>> spi-flash core. This results in regressions.
>>>
>>
>> Apologies for the trouble...
>> As stated in cover letter, this change was necessary as U-Boot SPI flash
>> stack at that time did not features such as 4 byte addressing, SFDP
>> parsing, SPI controllers with MMIO interfaces etc. Also there was need
>> to move to SPI MEM framework to support both SPI NAND and SPI NOR
>> flashes using a single SPI controller drivers.
>> I have to disagree on the part that there was no proper testing... As
>> evident from mailing list archives, patch series was reviewed by
>> multiple reviewers and tested on their platforms as well...
>> Unfortunately its impossible to get all boards owners to test it.
>
> I'm not talking about getting all customers board and testing changes on them.
> It could be done another way - i.e. like it is done for u-boot driver-model migration:
> by introducing the option for choosing which stack will be used and allow customers
> to switch the flash IC they use to new stack until some deadline.
>
I did start off with this. But maintaining two stacks is too cumbersome
and adds to code size (which is a big factor for SPL stage)
>>> One of regression we faced with is related to SST26 flash series which
>>> is used on HSDK board. The cause is that SST26 protection ops
>>> implementation was dropped. The fix of this regression is send
>>> as a patch in this series.
>>>
>>
>> I retained most U-Boot specific code as is (like support for BANK
>> address registers, restriction in transfer sizes) but I somehow
>> overlooked this part. Sorry for that
>>
>>> However there are another regressions. I.E: we also faced with broken
>>> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
>>> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>>>
>>
>> Could you provide more details here:
>> What exactly fails? Is the detected correctly? Does reads work fine? Is
>> Erase or Write broken?
>
> It seems to me that something is wrong with protection ops.
> The erase and write finish without errors however nothing actually happens.
>
I doubt so, because if the blocks were protected, erase/write would have failed
and Read status/Read flag status register should have reported error values.
Anyways, I guess I found a wrt how 4 Byte addressing is handled wrt n25q512* series.
Could you try with below patch helps[1]?
If not please provide logs similar what you have provide now.
If below patch does not help, then please try enabling CONFIG_SPI_FLASH_BAR and see if that helps.
[1]
---8<-----
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
2019-09-10 5:07 ` Vignesh Raghavendra
@ 2019-09-10 11:41 ` Eugeniy Paltsev
2019-09-10 11:54 ` Vignesh Raghavendra
0 siblings, 1 reply; 15+ messages in thread
From: Eugeniy Paltsev @ 2019-09-10 11:41 UTC (permalink / raw)
To: u-boot
Hi Vignesh,
that patch helps - both erase and write works fine.
For n25q512ax3:
Tested-by: "Eugeniy Paltsev <paltsev@synopsys.com>"
---
Eugeniy Paltsev
________________________________________
From: Vignesh Raghavendra <vigneshr@ti.com>
Sent: Tuesday, September 10, 2019 08:07
To: Eugeniy Paltsev; Jagan Teki
Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com; Alexey Brodkin; trini at konsulko.com
Subject: Re: Regressions in MTD / SPI FLASH
Hi,
On 10/09/19 12:18 AM, Eugeniy Paltsev wrote:
> Hi!
> Comments are inlined:
>
>> On 04/09/19 11:37 PM, Eugeniy Paltsev wrote:
>>> We faced with regressions caused by
>>> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
>>> This switch was performed by removing entire u-boot spi-flash
>>> core implementation and copying it from another project.
>>> However the switch is performed without proper testing and
>>> investigations about fixes/improvements were made in u-boot
>>> spi-flash core. This results in regressions.
>>>
>>
>> Apologies for the trouble...
>> As stated in cover letter, this change was necessary as U-Boot SPI flash
>> stack at that time did not features such as 4 byte addressing, SFDP
>> parsing, SPI controllers with MMIO interfaces etc. Also there was need
>> to move to SPI MEM framework to support both SPI NAND and SPI NOR
>> flashes using a single SPI controller drivers.
>> I have to disagree on the part that there was no proper testing... As
>> evident from mailing list archives, patch series was reviewed by
>> multiple reviewers and tested on their platforms as well...
>> Unfortunately its impossible to get all boards owners to test it.
>
> I'm not talking about getting all customers board and testing changes on them.
> It could be done another way - i.e. like it is done for u-boot driver-model migration:
> by introducing the option for choosing which stack will be used and allow customers
> to switch the flash IC they use to new stack until some deadline.
>
I did start off with this. But maintaining two stacks is too cumbersome
and adds to code size (which is a big factor for SPL stage)
>>> One of regression we faced with is related to SST26 flash series which
>>> is used on HSDK board. The cause is that SST26 protection ops
>>> implementation was dropped. The fix of this regression is send
>>> as a patch in this series.
>>>
>>
>> I retained most U-Boot specific code as is (like support for BANK
>> address registers, restriction in transfer sizes) but I somehow
>> overlooked this part. Sorry for that
>>
>>> However there are another regressions. I.E: we also faced with broken
>>> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
>>> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>>>
>>
>> Could you provide more details here:
>> What exactly fails? Is the detected correctly? Does reads work fine? Is
>> Erase or Write broken?
>
> It seems to me that something is wrong with protection ops.
> The erase and write finish without errors however nothing actually happens.
>
I doubt so, because if the blocks were protected, erase/write would have failed
and Read status/Read flag status register should have reported error values.
Anyways, I guess I found a wrt how 4 Byte addressing is handled wrt n25q512* series.
Could you try with below patch helps[1]?
If not please provide logs similar what you have provide now.
If below patch does not help, then please try enabling CONFIG_SPI_FLASH_BAR and see if that helps.
[1]
---8<-----
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
2019-09-10 11:41 ` Eugeniy Paltsev
@ 2019-09-10 11:54 ` Vignesh Raghavendra
2019-09-10 12:27 ` Vignesh Raghavendra
0 siblings, 1 reply; 15+ messages in thread
From: Vignesh Raghavendra @ 2019-09-10 11:54 UTC (permalink / raw)
To: u-boot
On 10/09/19 5:11 PM, Eugeniy Paltsev wrote:
> Hi Vignesh,
>
> that patch helps - both erase and write works fine.
>
Thanks for testing! I will cleanup the patches and send formal patches
to the list with your tested by.
Regards
Vignesh
> For n25q512ax3:
> Tested-by: "Eugeniy Paltsev <paltsev@synopsys.com>"
>
> ---
> Eugeniy Paltsev
>
>
> ________________________________________
> From: Vignesh Raghavendra <vigneshr@ti.com>
> Sent: Tuesday, September 10, 2019 08:07
> To: Eugeniy Paltsev; Jagan Teki
> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com; Alexey Brodkin; trini at konsulko.com
> Subject: Re: Regressions in MTD / SPI FLASH
>
> Hi,
>
> On 10/09/19 12:18 AM, Eugeniy Paltsev wrote:
>> Hi!
>> Comments are inlined:
>>
>>> On 04/09/19 11:37 PM, Eugeniy Paltsev wrote:
>>>> We faced with regressions caused by
>>>> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
>>>> This switch was performed by removing entire u-boot spi-flash
>>>> core implementation and copying it from another project.
>>>> However the switch is performed without proper testing and
>>>> investigations about fixes/improvements were made in u-boot
>>>> spi-flash core. This results in regressions.
>>>>
>>>
>>> Apologies for the trouble...
>>> As stated in cover letter, this change was necessary as U-Boot SPI flash
>>> stack at that time did not features such as 4 byte addressing, SFDP
>>> parsing, SPI controllers with MMIO interfaces etc. Also there was need
>>> to move to SPI MEM framework to support both SPI NAND and SPI NOR
>>> flashes using a single SPI controller drivers.
>>> I have to disagree on the part that there was no proper testing... As
>>> evident from mailing list archives, patch series was reviewed by
>>> multiple reviewers and tested on their platforms as well...
>>> Unfortunately its impossible to get all boards owners to test it.
>>
>> I'm not talking about getting all customers board and testing changes on them.
>> It could be done another way - i.e. like it is done for u-boot driver-model migration:
>> by introducing the option for choosing which stack will be used and allow customers
>> to switch the flash IC they use to new stack until some deadline.
>>
>
> I did start off with this. But maintaining two stacks is too cumbersome
> and adds to code size (which is a big factor for SPL stage)
>
>
>>>> One of regression we faced with is related to SST26 flash series which
>>>> is used on HSDK board. The cause is that SST26 protection ops
>>>> implementation was dropped. The fix of this regression is send
>>>> as a patch in this series.
>>>>
>>>
>>> I retained most U-Boot specific code as is (like support for BANK
>>> address registers, restriction in transfer sizes) but I somehow
>>> overlooked this part. Sorry for that
>>>
>>>> However there are another regressions. I.E: we also faced with broken
>>>> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
>>>> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>>>>
>>>
>>> Could you provide more details here:
>>> What exactly fails? Is the detected correctly? Does reads work fine? Is
>>> Erase or Write broken?
>>
>> It seems to me that something is wrong with protection ops.
>> The erase and write finish without errors however nothing actually happens.
>>
>
> I doubt so, because if the blocks were protected, erase/write would have failed
> and Read status/Read flag status register should have reported error values.
> Anyways, I guess I found a wrt how 4 Byte addressing is handled wrt n25q512* series.
>
> Could you try with below patch helps[1]?
> If not please provide logs similar what you have provide now.
>
> If below patch does not help, then please try enabling CONFIG_SPI_FLASH_BAR and see if that helps.
>
> [1]
>
> ---8<-----
>
> From 1de4c447cd4b2590c98f9ceccf8ed32029b42d36 Mon Sep 17 00:00:00 2001
> From: Vignesh Raghavendra <vigneshr@ti.com>
> Date: Tue, 10 Sep 2019 10:25:17 +0530
> Subject: [TST PATCH] mtd: spi: spi-nor-ids: Disable SPI_NOR_4B_OPCODES
>
> Not all variants of n25q256* and n25q512* support 4 Byte stateless
> addressing and there is no easy way to discover this at runtime.
> Therefore don't set SPI_NOR_4B_OPCODES for these flashes
>
> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
> ---
> drivers/mtd/spi/spi-nor-ids.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
> index a3920ba520e0..66ac3256e8f5 100644
> --- a/drivers/mtd/spi/spi-nor-ids.c
> +++ b/drivers/mtd/spi/spi-nor-ids.c
> @@ -161,12 +161,10 @@ const struct flash_info spi_nor_ids[] = {
> { INFO("n25q064a", 0x20bb17, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_QUAD_READ) },
> { INFO("n25q128a11", 0x20bb18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
> { INFO("n25q128a13", 0x20ba18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
> - { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> - { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> - { INFO6("mt25qu512a", 0x20bb20, 0x104400, 64 * 1024, 1024,
> - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> - { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> - { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
> + { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
> + { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ) },
> + { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
> + { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
> { INFO("n25q00", 0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> { INFO("n25q00a", 0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> { INFO("mt25qu02g", 0x20bb22, 0, 64 * 1024, 4096, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
> --
> 2.23.0
>
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
2019-09-10 11:54 ` Vignesh Raghavendra
@ 2019-09-10 12:27 ` Vignesh Raghavendra
2019-09-12 12:29 ` Eugeniy Paltsev
0 siblings, 1 reply; 15+ messages in thread
From: Vignesh Raghavendra @ 2019-09-10 12:27 UTC (permalink / raw)
To: u-boot
Hi Eugeniy,
One more request:
I am trying to find a better way to identify parts that don't support
4byte addressing.
Could you enable CONFIG_SPI_FLASH_SFDP_SUPPORT and also enable debug
prints in spi_mem_exec_op() (in drivers/spi/spi-mem.c like before) and
provide logs?
Just logs of "sf probe" should be sufficient.
Regards
Vignesh
On 10/09/19 5:24 PM, Vignesh Raghavendra wrote:
>
>
> On 10/09/19 5:11 PM, Eugeniy Paltsev wrote:
>> Hi Vignesh,
>>
>> that patch helps - both erase and write works fine.
>>
>
> Thanks for testing! I will cleanup the patches and send formal patches
> to the list with your tested by.
>
> Regards
> Vignesh
>
>> For n25q512ax3:
>> Tested-by: "Eugeniy Paltsev <paltsev@synopsys.com>"
>>
>> ---
>> Eugeniy Paltsev
>>
>>
>> ________________________________________
>> From: Vignesh Raghavendra <vigneshr@ti.com>
>> Sent: Tuesday, September 10, 2019 08:07
>> To: Eugeniy Paltsev; Jagan Teki
>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com; Alexey Brodkin; trini at konsulko.com
>> Subject: Re: Regressions in MTD / SPI FLASH
>>
>> Hi,
>>
>> On 10/09/19 12:18 AM, Eugeniy Paltsev wrote:
>>> Hi!
>>> Comments are inlined:
>>>
>>>> On 04/09/19 11:37 PM, Eugeniy Paltsev wrote:
>>>>> We faced with regressions caused by
>>>>> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
>>>>> This switch was performed by removing entire u-boot spi-flash
>>>>> core implementation and copying it from another project.
>>>>> However the switch is performed without proper testing and
>>>>> investigations about fixes/improvements were made in u-boot
>>>>> spi-flash core. This results in regressions.
>>>>>
>>>>
>>>> Apologies for the trouble...
>>>> As stated in cover letter, this change was necessary as U-Boot SPI flash
>>>> stack at that time did not features such as 4 byte addressing, SFDP
>>>> parsing, SPI controllers with MMIO interfaces etc. Also there was need
>>>> to move to SPI MEM framework to support both SPI NAND and SPI NOR
>>>> flashes using a single SPI controller drivers.
>>>> I have to disagree on the part that there was no proper testing... As
>>>> evident from mailing list archives, patch series was reviewed by
>>>> multiple reviewers and tested on their platforms as well...
>>>> Unfortunately its impossible to get all boards owners to test it.
>>>
>>> I'm not talking about getting all customers board and testing changes on them.
>>> It could be done another way - i.e. like it is done for u-boot driver-model migration:
>>> by introducing the option for choosing which stack will be used and allow customers
>>> to switch the flash IC they use to new stack until some deadline.
>>>
>>
>> I did start off with this. But maintaining two stacks is too cumbersome
>> and adds to code size (which is a big factor for SPL stage)
>>
>>
>>>>> One of regression we faced with is related to SST26 flash series which
>>>>> is used on HSDK board. The cause is that SST26 protection ops
>>>>> implementation was dropped. The fix of this regression is send
>>>>> as a patch in this series.
>>>>>
>>>>
>>>> I retained most U-Boot specific code as is (like support for BANK
>>>> address registers, restriction in transfer sizes) but I somehow
>>>> overlooked this part. Sorry for that
>>>>
>>>>> However there are another regressions. I.E: we also faced with broken
>>>>> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
>>>>> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>>>>>
>>>>
>>>> Could you provide more details here:
>>>> What exactly fails? Is the detected correctly? Does reads work fine? Is
>>>> Erase or Write broken?
>>>
>>> It seems to me that something is wrong with protection ops.
>>> The erase and write finish without errors however nothing actually happens.
>>>
>>
>> I doubt so, because if the blocks were protected, erase/write would have failed
>> and Read status/Read flag status register should have reported error values.
>> Anyways, I guess I found a wrt how 4 Byte addressing is handled wrt n25q512* series.
>>
>> Could you try with below patch helps[1]?
>> If not please provide logs similar what you have provide now.
>>
>> If below patch does not help, then please try enabling CONFIG_SPI_FLASH_BAR and see if that helps.
>>
>> [1]
>>
>> ---8<-----
>>
>> From 1de4c447cd4b2590c98f9ceccf8ed32029b42d36 Mon Sep 17 00:00:00 2001
>> From: Vignesh Raghavendra <vigneshr@ti.com>
>> Date: Tue, 10 Sep 2019 10:25:17 +0530
>> Subject: [TST PATCH] mtd: spi: spi-nor-ids: Disable SPI_NOR_4B_OPCODES
>>
>> Not all variants of n25q256* and n25q512* support 4 Byte stateless
>> addressing and there is no easy way to discover this at runtime.
>> Therefore don't set SPI_NOR_4B_OPCODES for these flashes
>>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>> drivers/mtd/spi/spi-nor-ids.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>> index a3920ba520e0..66ac3256e8f5 100644
>> --- a/drivers/mtd/spi/spi-nor-ids.c
>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>> @@ -161,12 +161,10 @@ const struct flash_info spi_nor_ids[] = {
>> { INFO("n25q064a", 0x20bb17, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_QUAD_READ) },
>> { INFO("n25q128a11", 0x20bb18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
>> { INFO("n25q128a13", 0x20ba18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
>> - { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> - { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> - { INFO6("mt25qu512a", 0x20bb20, 0x104400, 64 * 1024, 1024,
>> - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> - { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> - { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> + { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>> + { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ) },
>> + { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
>> + { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
>> { INFO("n25q00", 0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>> { INFO("n25q00a", 0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>> { INFO("mt25qu02g", 0x20bb22, 0, 64 * 1024, 4096, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>> --
>> 2.23.0
>>
>
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
2019-09-10 12:27 ` Vignesh Raghavendra
@ 2019-09-12 12:29 ` Eugeniy Paltsev
[not found] ` <bc29f27e-7455-f820-659f-6eb557694506@ti.com>
0 siblings, 1 reply; 15+ messages in thread
From: Eugeniy Paltsev @ 2019-09-12 12:29 UTC (permalink / raw)
To: u-boot
Hi Vignesh,
I doesn't have access to board with n25q512ax3 currently, however I can test this on Monday (16.09)
---
Eugeniy Paltsev
________________________________________
From: Vignesh Raghavendra <vigneshr@ti.com>
Sent: Tuesday, September 10, 2019 15:27
To: Eugeniy Paltsev; Jagan Teki
Cc: u-boot at lists.denx.de; Alexey Brodkin; trini at konsulko.com; uboot-snps-arc at synopsys.com
Subject: Re: [U-Boot] Regressions in MTD / SPI FLASH
Hi Eugeniy,
One more request:
I am trying to find a better way to identify parts that don't support
4byte addressing.
Could you enable CONFIG_SPI_FLASH_SFDP_SUPPORT and also enable debug
prints in spi_mem_exec_op() (in drivers/spi/spi-mem.c like before) and
provide logs?
Just logs of "sf probe" should be sufficient.
Regards
Vignesh
On 10/09/19 5:24 PM, Vignesh Raghavendra wrote:
>
>
> On 10/09/19 5:11 PM, Eugeniy Paltsev wrote:
>> Hi Vignesh,
>>
>> that patch helps - both erase and write works fine.
>>
>
> Thanks for testing! I will cleanup the patches and send formal patches
> to the list with your tested by.
>
> Regards
> Vignesh
>
>> For n25q512ax3:
>> Tested-by: "Eugeniy Paltsev <paltsev@synopsys.com>"
>>
>> ---
>> Eugeniy Paltsev
>>
>>
>> ________________________________________
>> From: Vignesh Raghavendra <vigneshr@ti.com>
>> Sent: Tuesday, September 10, 2019 08:07
>> To: Eugeniy Paltsev; Jagan Teki
>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com; Alexey Brodkin; trini at konsulko.com
>> Subject: Re: Regressions in MTD / SPI FLASH
>>
>> Hi,
>>
>> On 10/09/19 12:18 AM, Eugeniy Paltsev wrote:
>>> Hi!
>>> Comments are inlined:
>>>
>>>> On 04/09/19 11:37 PM, Eugeniy Paltsev wrote:
>>>>> We faced with regressions caused by
>>>>> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
>>>>> This switch was performed by removing entire u-boot spi-flash
>>>>> core implementation and copying it from another project.
>>>>> However the switch is performed without proper testing and
>>>>> investigations about fixes/improvements were made in u-boot
>>>>> spi-flash core. This results in regressions.
>>>>>
>>>>
>>>> Apologies for the trouble...
>>>> As stated in cover letter, this change was necessary as U-Boot SPI flash
>>>> stack at that time did not features such as 4 byte addressing, SFDP
>>>> parsing, SPI controllers with MMIO interfaces etc. Also there was need
>>>> to move to SPI MEM framework to support both SPI NAND and SPI NOR
>>>> flashes using a single SPI controller drivers.
>>>> I have to disagree on the part that there was no proper testing... As
>>>> evident from mailing list archives, patch series was reviewed by
>>>> multiple reviewers and tested on their platforms as well...
>>>> Unfortunately its impossible to get all boards owners to test it.
>>>
>>> I'm not talking about getting all customers board and testing changes on them.
>>> It could be done another way - i.e. like it is done for u-boot driver-model migration:
>>> by introducing the option for choosing which stack will be used and allow customers
>>> to switch the flash IC they use to new stack until some deadline.
>>>
>>
>> I did start off with this. But maintaining two stacks is too cumbersome
>> and adds to code size (which is a big factor for SPL stage)
>>
>>
>>>>> One of regression we faced with is related to SST26 flash series which
>>>>> is used on HSDK board. The cause is that SST26 protection ops
>>>>> implementation was dropped. The fix of this regression is send
>>>>> as a patch in this series.
>>>>>
>>>>
>>>> I retained most U-Boot specific code as is (like support for BANK
>>>> address registers, restriction in transfer sizes) but I somehow
>>>> overlooked this part. Sorry for that
>>>>
>>>>> However there are another regressions. I.E: we also faced with broken
>>>>> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
>>>>> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>>>>>
>>>>
>>>> Could you provide more details here:
>>>> What exactly fails? Is the detected correctly? Does reads work fine? Is
>>>> Erase or Write broken?
>>>
>>> It seems to me that something is wrong with protection ops.
>>> The erase and write finish without errors however nothing actually happens.
>>>
>>
>> I doubt so, because if the blocks were protected, erase/write would have failed
>> and Read status/Read flag status register should have reported error values.
>> Anyways, I guess I found a wrt how 4 Byte addressing is handled wrt n25q512* series.
>>
>> Could you try with below patch helps[1]?
>> If not please provide logs similar what you have provide now.
>>
>> If below patch does not help, then please try enabling CONFIG_SPI_FLASH_BAR and see if that helps.
>>
>> [1]
>>
>> ---8<-----
>>
>> From 1de4c447cd4b2590c98f9ceccf8ed32029b42d36 Mon Sep 17 00:00:00 2001
>> From: Vignesh Raghavendra <vigneshr@ti.com>
>> Date: Tue, 10 Sep 2019 10:25:17 +0530
>> Subject: [TST PATCH] mtd: spi: spi-nor-ids: Disable SPI_NOR_4B_OPCODES
>>
>> Not all variants of n25q256* and n25q512* support 4 Byte stateless
>> addressing and there is no easy way to discover this at runtime.
>> Therefore don't set SPI_NOR_4B_OPCODES for these flashes
>>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>> ---
>> drivers/mtd/spi/spi-nor-ids.c | 10 ++++------
>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>> index a3920ba520e0..66ac3256e8f5 100644
>> --- a/drivers/mtd/spi/spi-nor-ids.c
>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>> @@ -161,12 +161,10 @@ const struct flash_info spi_nor_ids[] = {
>> { INFO("n25q064a", 0x20bb17, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_QUAD_READ) },
>> { INFO("n25q128a11", 0x20bb18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
>> { INFO("n25q128a13", 0x20ba18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
>> - { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> - { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> - { INFO6("mt25qu512a", 0x20bb20, 0x104400, 64 * 1024, 1024,
>> - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> - { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> - { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>> + { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>> + { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ) },
>> + { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
>> + { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
>> { INFO("n25q00", 0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>> { INFO("n25q00a", 0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>> { INFO("mt25qu02g", 0x20bb22, 0, 64 * 1024, 4096, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>> --
>> 2.23.0
>>
>
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
[not found] ` <bc29f27e-7455-f820-659f-6eb557694506@ti.com>
@ 2019-09-24 16:15 ` Eugeniy Paltsev
2019-09-25 7:56 ` Vignesh Raghavendra
0 siblings, 1 reply; 15+ messages in thread
From: Eugeniy Paltsev @ 2019-09-24 16:15 UTC (permalink / raw)
To: u-boot
Hi Vignesh,
sorry for delay, I was pretty busy with another project :(
I've tried to test SPI with CONFIG_SPI_FLASH_SFDP_SUPPORT enabled, however it doesn't seem to work.
CONFIG_SPI_FLASH_SFDP_SUPPORT enabled:
---------------------------------->8--------------------------------------------
AXS# sf probe && echo OK
9f | [6B in] 20 ba 20 10 00 00 [ret 0]
5a 00 00 00 | [16B in] 53 46 44 50 00 01 00 ff 00 00 01 09 30 00 00 ff [ret 0]
5a 00 00 30 | [36B in] e5 20 fb ff ff ff ff 1f 29 eb 27 6b 27 3b 27 bb ff ff ff ff ff ff 27 bb ff ff 29 eb 0c 20 10 d8 00 00 00 00 [ret 0]
06 | [0B -] [ret 0]
b7 | [0B -] [ret 0]
04 | [0B -] [ret 0]
SF: Detected n25q512ax3 with page size 256 Bytes, erase size 64 KiB, total 64 MiB
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
0b 00 18 00 00 | [16B in] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ret 0]
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
AXS# sf protect unlock 0x0 0x4000000 && echo OK
05 | [1B in] 00 [ret 0]
OK
AXS# sf erase 0x180000 0x1000 && echo OK
SF: Erase offset/length not multiple of erase size
SF: 4096 bytes @ 0x180000 Erased: ERROR
---------------------------------->8--------------------------------------------
CONFIG_SPI_FLASH_SFDP_SUPPORT disabled (everything OK):
---------------------------------->8--------------------------------------------
AXS# sf probe && echo OK
9f | [6B in] 20 ba 20 10 00 00 [ret 0]
06 | [0B -] [ret 0]
b7 | [0B -] [ret 0]
04 | [0B -] [ret 0]
SF: Detected n25q512ax3 with page size 256 Bytes, erase size 4 KiB, total 64 MiB
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
0b 00 18 00 00 | [16B in] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ret 0]
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
AXS# sf protect unlock 0x0 0x4000000 && echo OK
05 | [1B in] 00 [ret 0]
OK
AXS# sf erase 0x180000 0x1000 && echo OK
06 | [0B -] [ret 0]
20 00 18 00 00 | [0B -] [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 03 [ret 0]
70 | [1B in] 01 [ret 0]
05 | [1B in] 00 [ret 0]
70 | [1B in] 81 [ret 0]
04 | [0B -] [ret 0]
SF: 4096 bytes @ 0x180000 Erased: OK
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
0b 00 18 00 00 | [16B in] ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff [ret 0]
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
AXS# mw 0x81000000 0 5
AXS# sf write 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
06 | [0B -] [ret 0]
02 00 18 00 00 | [16B out] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ret 0]
05 | [1B in] 00 [ret 0]
70 | [1B in] 81 [ret 0]
SF: 16 bytes @ 0x180000 Written: OK
OK
AXS# sf read 0x81000000 0x180000 0x10 && echo OK
device 0 offset 0x180000, size 0x10
0b 00 18 00 00 | [16B in] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ret 0]
SF: 16 bytes @ 0x180000 Read: OK
OK
AXS# md.b 0x81000000 0x10
81000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
AXS#
---------------------------------->8--------------------------------------------
All tests were performed on 4b95daf5dca with your commit (for disabling 4B opcodes) applied.
---
Eugeniy Paltsev
________________________________________
From: Vignesh Raghavendra <vigneshr@ti.com>
Sent: Monday, September 23, 2019 08:31
To: Eugeniy Paltsev
Cc: uboot-snps-arc at synopsys.com; Alexey Brodkin
Subject: Re: [U-Boot] Regressions in MTD / SPI FLASH
Hi Eugeniy
On 12/09/19 5:59 PM, Eugeniy Paltsev wrote:
> Hi Vignesh,
>
> I doesn't have access to board with n25q512ax3 currently, however I can test this on Monday (16.09)
Could you provide please provide logs that I requested below?
"
> Could you enable CONFIG_SPI_FLASH_SFDP_SUPPORT and also enable debug
> prints in spi_mem_exec_op() (in drivers/spi/spi-mem.c like before) and
> provide logs?
"
There are some objections to the fixes[1] that I provided to you. Above
logs will helps me in justifying the fix or provide better patch. I
would like to close this as soon as possible before 2019.10 is out.
Appreciate you help!
[1] https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_1160501_&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=ZlJN1MriPUTkBKCrPSx67GmaplEUGcAEk9yPtCLdUXI&m=Yb1WwCtRxov5WTaRZMp5FA3-x1hLIH-G-192fad-opU&s=w3B4OJ4ml06qS7q7BjbvhzZXkLQswsLfXxuPt7McUU4&e=
>
> ---
> Eugeniy Paltsev
>
>
> ________________________________________
> From: Vignesh Raghavendra <vigneshr@ti.com>
> Sent: Tuesday, September 10, 2019 15:27
> To: Eugeniy Paltsev; Jagan Teki
> Cc: u-boot at lists.denx.de; Alexey Brodkin; trini at konsulko.com; uboot-snps-arc at synopsys.com
> Subject: Re: [U-Boot] Regressions in MTD / SPI FLASH
>
> Hi Eugeniy,
>
> One more request:
>
> I am trying to find a better way to identify parts that don't support
> 4byte addressing.
>
> Could you enable CONFIG_SPI_FLASH_SFDP_SUPPORT and also enable debug
> prints in spi_mem_exec_op() (in drivers/spi/spi-mem.c like before) and
> provide logs?
>
> Just logs of "sf probe" should be sufficient.
>
> Regards
> Vignesh
>
> On 10/09/19 5:24 PM, Vignesh Raghavendra wrote:
>>
>>
>> On 10/09/19 5:11 PM, Eugeniy Paltsev wrote:
>>> Hi Vignesh,
>>>
>>> that patch helps - both erase and write works fine.
>>>
>>
>> Thanks for testing! I will cleanup the patches and send formal patches
>> to the list with your tested by.
>>
>> Regards
>> Vignesh
>>
>>> For n25q512ax3:
>>> Tested-by: "Eugeniy Paltsev <paltsev@synopsys.com>"
>>>
>>> ---
>>> Eugeniy Paltsev
>>>
>>>
>>> ________________________________________
>>> From: Vignesh Raghavendra <vigneshr@ti.com>
>>> Sent: Tuesday, September 10, 2019 08:07
>>> To: Eugeniy Paltsev; Jagan Teki
>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com; Alexey Brodkin; trini at konsulko.com
>>> Subject: Re: Regressions in MTD / SPI FLASH
>>>
>>> Hi,
>>>
>>> On 10/09/19 12:18 AM, Eugeniy Paltsev wrote:
>>>> Hi!
>>>> Comments are inlined:
>>>>
>>>>> On 04/09/19 11:37 PM, Eugeniy Paltsev wrote:
>>>>>> We faced with regressions caused by
>>>>>> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
>>>>>> This switch was performed by removing entire u-boot spi-flash
>>>>>> core implementation and copying it from another project.
>>>>>> However the switch is performed without proper testing and
>>>>>> investigations about fixes/improvements were made in u-boot
>>>>>> spi-flash core. This results in regressions.
>>>>>>
>>>>>
>>>>> Apologies for the trouble...
>>>>> As stated in cover letter, this change was necessary as U-Boot SPI flash
>>>>> stack at that time did not features such as 4 byte addressing, SFDP
>>>>> parsing, SPI controllers with MMIO interfaces etc. Also there was need
>>>>> to move to SPI MEM framework to support both SPI NAND and SPI NOR
>>>>> flashes using a single SPI controller drivers.
>>>>> I have to disagree on the part that there was no proper testing... As
>>>>> evident from mailing list archives, patch series was reviewed by
>>>>> multiple reviewers and tested on their platforms as well...
>>>>> Unfortunately its impossible to get all boards owners to test it.
>>>>
>>>> I'm not talking about getting all customers board and testing changes on them.
>>>> It could be done another way - i.e. like it is done for u-boot driver-model migration:
>>>> by introducing the option for choosing which stack will be used and allow customers
>>>> to switch the flash IC they use to new stack until some deadline.
>>>>
>>>
>>> I did start off with this. But maintaining two stacks is too cumbersome
>>> and adds to code size (which is a big factor for SPL stage)
>>>
>>>
>>>>>> One of regression we faced with is related to SST26 flash series which
>>>>>> is used on HSDK board. The cause is that SST26 protection ops
>>>>>> implementation was dropped. The fix of this regression is send
>>>>>> as a patch in this series.
>>>>>>
>>>>>
>>>>> I retained most U-Boot specific code as is (like support for BANK
>>>>> address registers, restriction in transfer sizes) but I somehow
>>>>> overlooked this part. Sorry for that
>>>>>
>>>>>> However there are another regressions. I.E: we also faced with broken
>>>>>> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
>>>>>> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>>>>>>
>>>>>
>>>>> Could you provide more details here:
>>>>> What exactly fails? Is the detected correctly? Does reads work fine? Is
>>>>> Erase or Write broken?
>>>>
>>>> It seems to me that something is wrong with protection ops.
>>>> The erase and write finish without errors however nothing actually happens.
>>>>
>>>
>>> I doubt so, because if the blocks were protected, erase/write would have failed
>>> and Read status/Read flag status register should have reported error values.
>>> Anyways, I guess I found a wrt how 4 Byte addressing is handled wrt n25q512* series.
>>>
>>> Could you try with below patch helps[1]?
>>> If not please provide logs similar what you have provide now.
>>>
>>> If below patch does not help, then please try enabling CONFIG_SPI_FLASH_BAR and see if that helps.
>>>
>>> [1]
>>>
>>> ---8<-----
>>>
>>> From 1de4c447cd4b2590c98f9ceccf8ed32029b42d36 Mon Sep 17 00:00:00 2001
>>> From: Vignesh Raghavendra <vigneshr@ti.com>
>>> Date: Tue, 10 Sep 2019 10:25:17 +0530
>>> Subject: [TST PATCH] mtd: spi: spi-nor-ids: Disable SPI_NOR_4B_OPCODES
>>>
>>> Not all variants of n25q256* and n25q512* support 4 Byte stateless
>>> addressing and there is no easy way to discover this at runtime.
>>> Therefore don't set SPI_NOR_4B_OPCODES for these flashes
>>>
>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>>> ---
>>> drivers/mtd/spi/spi-nor-ids.c | 10 ++++------
>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>>> index a3920ba520e0..66ac3256e8f5 100644
>>> --- a/drivers/mtd/spi/spi-nor-ids.c
>>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>>> @@ -161,12 +161,10 @@ const struct flash_info spi_nor_ids[] = {
>>> { INFO("n25q064a", 0x20bb17, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_QUAD_READ) },
>>> { INFO("n25q128a11", 0x20bb18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
>>> { INFO("n25q128a13", 0x20ba18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
>>> - { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>> - { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>> - { INFO6("mt25qu512a", 0x20bb20, 0x104400, 64 * 1024, 1024,
>>> - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>> - { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>> - { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>> + { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>>> + { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ) },
>>> + { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
>>> + { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
>>> { INFO("n25q00", 0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>>> { INFO("n25q00a", 0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>>> { INFO("mt25qu02g", 0x20bb22, 0, 64 * 1024, 4096, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>>> --
>>> 2.23.0
>>>
>>
>
> --
> Regards
> Vignesh
>
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 15+ messages in thread
* [U-Boot] Regressions in MTD / SPI FLASH
2019-09-24 16:15 ` Eugeniy Paltsev
@ 2019-09-25 7:56 ` Vignesh Raghavendra
0 siblings, 0 replies; 15+ messages in thread
From: Vignesh Raghavendra @ 2019-09-25 7:56 UTC (permalink / raw)
To: u-boot
Hi
On 24/09/19 9:45 PM, Eugeniy Paltsev wrote:
> Hi Vignesh,
> sorry for delay, I was pretty busy with another project :(
>
> I've tried to test SPI with CONFIG_SPI_FLASH_SFDP_SUPPORT enabled, however it doesn't seem to work.
>
> CONFIG_SPI_FLASH_SFDP_SUPPORT enabled:
> ---------------------------------->8--------------------------------------------
> AXS# sf probe && echo OK
> 9f | [6B in] 20 ba 20 10 00 00 [ret 0]
> 5a 00 00 00 | [16B in] 53 46 44 50 00 01 00 ff 00 00 01 09 30 00 00 ff [ret 0]
> 5a 00 00 30 | [36B in] e5 20 fb ff ff ff ff 1f 29 eb 27 6b 27 3b 27 bb ff ff ff ff ff ff 27 bb ff ff 29 eb 0c 20 10 d8 00 00 00 00 [ret 0]
> 06 | [0B -] [ret 0]
> b7 | [0B -] [ret 0]
> 04 | [0B -] [ret 0]
> SF: Detected n25q512ax3 with page size 256 Bytes, erase size 64 KiB, total 64 MiB
Oops, the sector size is being set 64K here instead of 4K in this
case... Will send a fix for that
> OK,
> AXS# sf read 0x81000000 0x180000 0x10 && echo OK
> device 0 offset 0x180000, size 0x10
> 0b 00 18 00 00 | [16B in] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ret 0]
> SF: 16 bytes @ 0x180000 Read: OK
> OK
> AXS# md.b 0x81000000 0x10
> 81000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> AXS# sf protect unlock 0x0 0x4000000 && echo OK
> 05 | [1B in] 00 [ret 0]
> OK
> AXS# sf erase 0x180000 0x1000 && echo OK
> SF: Erase offset/length not multiple of erase size
> SF: 4096 bytes @ 0x180000 Erased: ERROR
> ---------------------------------->8--------------------------------------------
>
And therefore this failure... Thanks for the debug logs!
Regards
Vignesh
>
>
> CONFIG_SPI_FLASH_SFDP_SUPPORT disabled (everything OK):
> ---------------------------------->8--------------------------------------------
> AXS# sf probe && echo OK
> 9f | [6B in] 20 ba 20 10 00 00 [ret 0]
> 06 | [0B -] [ret 0]
> b7 | [0B -] [ret 0]
> 04 | [0B -] [ret 0]
> SF: Detected n25q512ax3 with page size 256 Bytes, erase size 4 KiB, total 64 MiB
> OK
> AXS# sf read 0x81000000 0x180000 0x10 && echo OK
> device 0 offset 0x180000, size 0x10
> 0b 00 18 00 00 | [16B in] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ret 0]
> SF: 16 bytes @ 0x180000 Read: OK
> OK
> AXS# md.b 0x81000000 0x10
> 81000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> AXS# sf protect unlock 0x0 0x4000000 && echo OK
> 05 | [1B in] 00 [ret 0]
> OK
> AXS# sf erase 0x180000 0x1000 && echo OK
> 06 | [0B -] [ret 0]
> 20 00 18 00 00 | [0B -] [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 03 [ret 0]
> 70 | [1B in] 01 [ret 0]
> 05 | [1B in] 00 [ret 0]
> 70 | [1B in] 81 [ret 0]
> 04 | [0B -] [ret 0]
> SF: 4096 bytes @ 0x180000 Erased: OK
> OK
> AXS# sf read 0x81000000 0x180000 0x10 && echo OK
> device 0 offset 0x180000, size 0x10
> 0b 00 18 00 00 | [16B in] ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff [ret 0]
> SF: 16 bytes @ 0x180000 Read: OK
> OK
> AXS# md.b 0x81000000 0x10
> 81000000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ................
> AXS# mw 0x81000000 0 5
> AXS# sf write 0x81000000 0x180000 0x10 && echo OK
> device 0 offset 0x180000, size 0x10
> 06 | [0B -] [ret 0]
> 02 00 18 00 00 | [16B out] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ret 0]
> 05 | [1B in] 00 [ret 0]
> 70 | [1B in] 81 [ret 0]
> SF: 16 bytes @ 0x180000 Written: OK
> OK
> AXS# sf read 0x81000000 0x180000 0x10 && echo OK
> device 0 offset 0x180000, size 0x10
> 0b 00 18 00 00 | [16B in] 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 [ret 0]
> SF: 16 bytes @ 0x180000 Read: OK
> OK
> AXS# md.b 0x81000000 0x10
> 81000000: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
> AXS#
> ---------------------------------->8--------------------------------------------
>
> All tests were performed on 4b95daf5dca with your commit (for disabling 4B opcodes) applied.
>
> ---
> Eugeniy Paltsev
>
>
> ________________________________________
> From: Vignesh Raghavendra <vigneshr@ti.com>
> Sent: Monday, September 23, 2019 08:31
> To: Eugeniy Paltsev
> Cc: uboot-snps-arc at synopsys.com; Alexey Brodkin
> Subject: Re: [U-Boot] Regressions in MTD / SPI FLASH
>
> Hi Eugeniy
>
> On 12/09/19 5:59 PM, Eugeniy Paltsev wrote:
>> Hi Vignesh,
>>
>> I doesn't have access to board with n25q512ax3 currently, however I can test this on Monday (16.09)
>
> Could you provide please provide logs that I requested below?
>
> "
>> Could you enable CONFIG_SPI_FLASH_SFDP_SUPPORT and also enable debug
>> prints in spi_mem_exec_op() (in drivers/spi/spi-mem.c like before) and
>> provide logs?
> "
>
> There are some objections to the fixes[1] that I provided to you. Above
> logs will helps me in justifying the fix or provide better patch. I
> would like to close this as soon as possible before 2019.10 is out.
> Appreciate you help!
>
> [1] https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_1160501_&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=ZlJN1MriPUTkBKCrPSx67GmaplEUGcAEk9yPtCLdUXI&m=Yb1WwCtRxov5WTaRZMp5FA3-x1hLIH-G-192fad-opU&s=w3B4OJ4ml06qS7q7BjbvhzZXkLQswsLfXxuPt7McUU4&e=
>
>>
>> ---
>> Eugeniy Paltsev
>>
>>
>> ________________________________________
>> From: Vignesh Raghavendra <vigneshr@ti.com>
>> Sent: Tuesday, September 10, 2019 15:27
>> To: Eugeniy Paltsev; Jagan Teki
>> Cc: u-boot at lists.denx.de; Alexey Brodkin; trini at konsulko.com; uboot-snps-arc at synopsys.com
>> Subject: Re: [U-Boot] Regressions in MTD / SPI FLASH
>>
>> Hi Eugeniy,
>>
>> One more request:
>>
>> I am trying to find a better way to identify parts that don't support
>> 4byte addressing.
>>
>> Could you enable CONFIG_SPI_FLASH_SFDP_SUPPORT and also enable debug
>> prints in spi_mem_exec_op() (in drivers/spi/spi-mem.c like before) and
>> provide logs?
>>
>> Just logs of "sf probe" should be sufficient.
>>
>> Regards
>> Vignesh
>>
>> On 10/09/19 5:24 PM, Vignesh Raghavendra wrote:
>>>
>>>
>>> On 10/09/19 5:11 PM, Eugeniy Paltsev wrote:
>>>> Hi Vignesh,
>>>>
>>>> that patch helps - both erase and write works fine.
>>>>
>>>
>>> Thanks for testing! I will cleanup the patches and send formal patches
>>> to the list with your tested by.
>>>
>>> Regards
>>> Vignesh
>>>
>>>> For n25q512ax3:
>>>> Tested-by: "Eugeniy Paltsev <paltsev@synopsys.com>"
>>>>
>>>> ---
>>>> Eugeniy Paltsev
>>>>
>>>>
>>>> ________________________________________
>>>> From: Vignesh Raghavendra <vigneshr@ti.com>
>>>> Sent: Tuesday, September 10, 2019 08:07
>>>> To: Eugeniy Paltsev; Jagan Teki
>>>> Cc: u-boot at lists.denx.de; uboot-snps-arc at synopsys.com; Alexey Brodkin; trini at konsulko.com
>>>> Subject: Re: Regressions in MTD / SPI FLASH
>>>>
>>>> Hi,
>>>>
>>>> On 10/09/19 12:18 AM, Eugeniy Paltsev wrote:
>>>>> Hi!
>>>>> Comments are inlined:
>>>>>
>>>>>> On 04/09/19 11:37 PM, Eugeniy Paltsev wrote:
>>>>>>> We faced with regressions caused by
>>>>>>> commit c4e8862308d4 (mtd: spi: Switch to new SPI NOR framework)
>>>>>>> This switch was performed by removing entire u-boot spi-flash
>>>>>>> core implementation and copying it from another project.
>>>>>>> However the switch is performed without proper testing and
>>>>>>> investigations about fixes/improvements were made in u-boot
>>>>>>> spi-flash core. This results in regressions.
>>>>>>>
>>>>>>
>>>>>> Apologies for the trouble...
>>>>>> As stated in cover letter, this change was necessary as U-Boot SPI flash
>>>>>> stack at that time did not features such as 4 byte addressing, SFDP
>>>>>> parsing, SPI controllers with MMIO interfaces etc. Also there was need
>>>>>> to move to SPI MEM framework to support both SPI NAND and SPI NOR
>>>>>> flashes using a single SPI controller drivers.
>>>>>> I have to disagree on the part that there was no proper testing... As
>>>>>> evident from mailing list archives, patch series was reviewed by
>>>>>> multiple reviewers and tested on their platforms as well...
>>>>>> Unfortunately its impossible to get all boards owners to test it.
>>>>>
>>>>> I'm not talking about getting all customers board and testing changes on them.
>>>>> It could be done another way - i.e. like it is done for u-boot driver-model migration:
>>>>> by introducing the option for choosing which stack will be used and allow customers
>>>>> to switch the flash IC they use to new stack until some deadline.
>>>>>
>>>>
>>>> I did start off with this. But maintaining two stacks is too cumbersome
>>>> and adds to code size (which is a big factor for SPL stage)
>>>>
>>>>
>>>>>>> One of regression we faced with is related to SST26 flash series which
>>>>>>> is used on HSDK board. The cause is that SST26 protection ops
>>>>>>> implementation was dropped. The fix of this regression is send
>>>>>>> as a patch in this series.
>>>>>>>
>>>>>>
>>>>>> I retained most U-Boot specific code as is (like support for BANK
>>>>>> address registers, restriction in transfer sizes) but I somehow
>>>>>> overlooked this part. Sorry for that
>>>>>>
>>>>>>> However there are another regressions. I.E: we also faced with broken
>>>>>>> SPI flash on another SNPS boards - AXS101 and AXS103. They use different
>>>>>>> flash IC (n25q512ax3) and I didn't investigate the cause yet.
>>>>>>>
>>>>>>
>>>>>> Could you provide more details here:
>>>>>> What exactly fails? Is the detected correctly? Does reads work fine? Is
>>>>>> Erase or Write broken?
>>>>>
>>>>> It seems to me that something is wrong with protection ops.
>>>>> The erase and write finish without errors however nothing actually happens.
>>>>>
>>>>
>>>> I doubt so, because if the blocks were protected, erase/write would have failed
>>>> and Read status/Read flag status register should have reported error values.
>>>> Anyways, I guess I found a wrt how 4 Byte addressing is handled wrt n25q512* series.
>>>>
>>>> Could you try with below patch helps[1]?
>>>> If not please provide logs similar what you have provide now.
>>>>
>>>> If below patch does not help, then please try enabling CONFIG_SPI_FLASH_BAR and see if that helps.
>>>>
>>>> [1]
>>>>
>>>> ---8<-----
>>>>
>>>> From 1de4c447cd4b2590c98f9ceccf8ed32029b42d36 Mon Sep 17 00:00:00 2001
>>>> From: Vignesh Raghavendra <vigneshr@ti.com>
>>>> Date: Tue, 10 Sep 2019 10:25:17 +0530
>>>> Subject: [TST PATCH] mtd: spi: spi-nor-ids: Disable SPI_NOR_4B_OPCODES
>>>>
>>>> Not all variants of n25q256* and n25q512* support 4 Byte stateless
>>>> addressing and there is no easy way to discover this at runtime.
>>>> Therefore don't set SPI_NOR_4B_OPCODES for these flashes
>>>>
>>>> Signed-off-by: Vignesh Raghavendra <vigneshr@ti.com>
>>>> ---
>>>> drivers/mtd/spi/spi-nor-ids.c | 10 ++++------
>>>> 1 file changed, 4 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/spi/spi-nor-ids.c b/drivers/mtd/spi/spi-nor-ids.c
>>>> index a3920ba520e0..66ac3256e8f5 100644
>>>> --- a/drivers/mtd/spi/spi-nor-ids.c
>>>> +++ b/drivers/mtd/spi/spi-nor-ids.c
>>>> @@ -161,12 +161,10 @@ const struct flash_info spi_nor_ids[] = {
>>>> { INFO("n25q064a", 0x20bb17, 0, 64 * 1024, 128, SECT_4K | SPI_NOR_QUAD_READ) },
>>>> { INFO("n25q128a11", 0x20bb18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
>>>> { INFO("n25q128a13", 0x20ba18, 0, 64 * 1024, 256, SECT_4K | SPI_NOR_QUAD_READ) },
>>>> - { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>>> - { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>>> - { INFO6("mt25qu512a", 0x20bb20, 0x104400, 64 * 1024, 1024,
>>>> - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>>> - { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>>> - { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
>>>> + { INFO("n25q256a", 0x20ba19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
>>>> + { INFO("n25q256ax1", 0x20bb19, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_QUAD_READ) },
>>>> + { INFO("n25q512a", 0x20bb20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
>>>> + { INFO("n25q512ax3", 0x20ba20, 0, 64 * 1024, 1024, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ) },
>>>> { INFO("n25q00", 0x20ba21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>>>> { INFO("n25q00a", 0x20bb21, 0, 64 * 1024, 2048, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>>>> { INFO("mt25qu02g", 0x20bb22, 0, 64 * 1024, 4096, SECT_4K | USE_FSR | SPI_NOR_QUAD_READ | NO_CHIP_ERASE) },
>>>> --
>>>> 2.23.0
>>>>
>>>
>>
>> --
>> Regards
>> Vignesh
>>
>
> --
> Regards
> Vignesh
>
--
Regards
Vignesh
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-09-25 7:56 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-04 18:07 [U-Boot] Regressions in MTD / SPI FLASH Eugeniy Paltsev
2019-09-04 18:07 ` [U-Boot] [PATCH 1/1] MTD: SPI: revert removing SST26* flash IC protection ops Eugeniy Paltsev
2019-09-07 3:40 ` Jagan Teki
2019-09-07 17:04 ` Tom Rini
2019-09-09 18:55 ` Eugeniy Paltsev
2019-09-06 7:36 ` [U-Boot] Regressions in MTD / SPI FLASH Vignesh Raghavendra
2019-09-09 18:48 ` Eugeniy Paltsev
2019-09-10 5:07 ` Vignesh Raghavendra
2019-09-10 11:41 ` Eugeniy Paltsev
2019-09-10 11:54 ` Vignesh Raghavendra
2019-09-10 12:27 ` Vignesh Raghavendra
2019-09-12 12:29 ` Eugeniy Paltsev
[not found] ` <bc29f27e-7455-f820-659f-6eb557694506@ti.com>
2019-09-24 16:15 ` Eugeniy Paltsev
2019-09-25 7:56 ` Vignesh Raghavendra
2019-09-09 7:59 ` Schrempf Frieder
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.