[U-Boot] Regressions in MTD / SPI FLASH
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(-)
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 */
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.
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!
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.
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(-)
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(-)
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) },
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@lists.denx.de; uboot-snps-arc@synopsys.com; Alexey Brodkin; trini@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
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@lists.denx.de; uboot-snps-arc@synopsys.com; Alexey Brodkin; trini@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
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@lists.denx.de; uboot-snps-arc@synopsys.com; Alexey Brodkin; trini@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
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@lists.denx.de; Alexey Brodkin; trini@konsulko.com; uboot-snps-arc@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@lists.denx.de; uboot-snps-arc@synopsys.com; Alexey Brodkin; trini@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
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(-)
participants (5)
-
Eugeniy Paltsev -
Jagan Teki -
Schrempf Frieder -
Tom Rini -
Vignesh Raghavendra