From: Alexander Sverdlin alexander.sverdlin@nokia.com
Spansion S25FS-S family has an issue in Basic Flash Parameter Table: DWORD-11 bits 7-4 specify write page size 512 bytes. In reality this is configurable in the non-volatile CR3NV register and even factory default configuration is "wrap at 256 bytes". So blind relying on BFPT breaks write operation on these Flashes.
All this story is vendor-specific, so add the corresponding fixup hook which first restores the safe page size of 256 bytes from struct flash_info but checks is more performant 512 bytes configuration is active and adjusts the page_size accordingly.
To read CR3V RDAR command is required which in turn requires addr width and read latency to be configured, which was not the case before. Setting these parameters is also later required for sector map selection, because:
JESD216 allows "variable address length" and "variable latency" in Configuration Detection Command Descriptors, in other words "as-is". And they are still unset during Sector Map Parameter Table parsing, which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.
New warning is added to catch the potential misconfiguration with other chips.
Link: http://lists.infradead.org/pipermail/linux-mtd/2020-February/093950.html Link: http://lists.infradead.org/pipermail/linux-mtd/2018-December/085956.html Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables") Cc: stable@vger.kernel.org Signed-off-by: Alexander Sverdlin alexander.sverdlin@nokia.com --- Yes, this is a combination of two previously sent patches as it turned out, width/dummy quirk is necessary even earlier, during post_bfpt fixup.
drivers/mtd/spi-nor/spi-nor.c | 132 +++++++++++++++++++++++++++++++++++++++++- include/linux/mtd/spi-nor.h | 11 ++++ 2 files changed, 141 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 1224247..1d0e2ef 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -2326,6 +2326,122 @@ static struct spi_nor_fixups gd25q256_fixups = { .default_init = gd25q256_default_init, };
+/* + * Return true if it was possible to read something known and non-zero with + * the probed parameters. struct spi_nor is updated in this case as well. + */ +static bool spi_nor_probe_width_and_dummy(struct spi_nor *nor, u8 width, + u8 dummy) +{ + u8 read_opcode = nor->read_opcode; + u8 savedw = nor->addr_width; + u8 savedd = nor->read_dummy; + int ret; + u8 buf; + + nor->addr_width = width; + nor->read_dummy = dummy; + nor->read_opcode = SPINOR_OP_RDAR; + ret = spi_nor_read_data(nor, SPINOR_REG_CR2V, 1, &buf); + nor->read_opcode = read_opcode; + + if (ret == 1 && (CR2V_RL(buf) == dummy) && + (!!(buf & CR2V_AL) == (width == 4))) + return true; + + nor->addr_width = savedw; + nor->read_dummy = savedd; + + return false; +} + +/* + * JESD216 allows to omit particular address length or latency specification in + * the header and at this point they are still unset, so we need some + * heuristics. One example is S25FS128S. + * + * It was observed that RDAR with incorrect parameters result in all-zeroes or + * all-ones reads. That's why probed dummy is limited to 14 and loops are built + * in a way to probe width 3 and 0 dummy bits last to avoid false-positive + * (refer to CR2 mapping). 8 dummy bits are probed on the first iteration. + */ +static void spi_nor_fixup_width_and_dummy(struct spi_nor *nor) +{ + u8 width_min = 3; + u8 width_max = 4; + u8 dummy_min = 0; + u8 dummy_max = 14; + u8 w, d; + + if (nor->addr_width && nor->read_dummy) + return; + + if (nor->addr_width) { + width_min = nor->addr_width; + width_max = nor->addr_width; + } + if (nor->read_dummy) { + dummy_min = nor->read_dummy; + dummy_max = nor->read_dummy; + } + + for (w = width_min; w <= width_max; ++w) + for (d = 8; d <= dummy_max; ++d) + if (d >= dummy_min && + spi_nor_probe_width_and_dummy(nor, w, d)) + return; + for (w = width_max; w >= width_min; --w) + for (d = 7; d >= dummy_min; --d) + if (d <= dummy_max && + spi_nor_probe_width_and_dummy(nor, w, d)) + return; +} + +/* Spansion S25FS-S SFDP workarounds */ +static int s25fs_s_post_bfpt_fixups(struct spi_nor *nor, + const struct sfdp_parameter_header *bfpt_header, + const struct sfdp_bfpt *bfpt, + struct spi_nor_flash_parameter *params) +{ + const struct flash_info *info = nor->info; + u8 read_opcode, buf; + int ret; + + /* + * RDAR command below requires nor->addr_width and nor->dummy correctly + * set. spi_nor_get_map_in_use() later requires them as well. + */ + spi_nor_fixup_width_and_dummy(nor); + + /* Default is safe */ + params->page_size = info->page_size; + + /* + * But is the chip configured for more performant 512 bytes write page + * size? + */ + read_opcode = nor->read_opcode; + + nor->read_opcode = SPINOR_OP_RDAR; + ret = spi_nor_read_data(nor, SPINOR_REG_CR3V, 1, &buf); + nor->read_opcode = read_opcode; + + switch (ret) { + case 0: + return -EIO; + case 1: + if (buf & CR3V_02H_V) + params->page_size = 512; + return 0; + } + + return ret; +} + +static const struct spi_nor_fixups s25fs_s_fixups = { + .post_bfpt = s25fs_s_post_bfpt_fixups, +}; + /* NOTE: double check command sets and memory organization when you add * more nor chips. This current list focusses on newer chips, which * have been converging on command sets which including JEDEC ID. @@ -2560,7 +2676,8 @@ static const struct flash_info spi_nor_ids[] = { SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, - { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) }, + { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) + .fixups = &s25fs_s_fixups, }, { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | @@ -2570,7 +2687,8 @@ static const struct flash_info spi_nor_ids[] = { { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256, 0) }, { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024, 64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, - { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, + { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) + .fixups = &s25fs_s_fixups, }, { "s25sl004a", INFO(0x010212, 0, 64 * 1024, 8, 0) }, { "s25sl008a", INFO(0x010213, 0, 64 * 1024, 16, 0) }, { "s25sl016a", INFO(0x010214, 0, 64 * 1024, 32, 0) }, @@ -3897,6 +4015,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]); addr = smpt[i + 1];
+ switch (nor->read_opcode) { + case SPINOR_OP_RDAR: + if (nor->read_dummy && nor->addr_width) + break; + dev_warn(nor->dev, "OP 0x%02x width %u dummy %u\n", + nor->read_opcode, nor->addr_width, + nor->read_dummy); + break; + } + err = spi_nor_read_raw(nor, addr, 1, buf); if (err) { ret = ERR_PTR(err); diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index de90724..1e21592 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -116,6 +116,7 @@ /* Used for Spansion flashes only. */ #define SPINOR_OP_BRWR 0x17 /* Bank register write */ #define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */ +#define SPINOR_OP_RDAR 0x65 /* Read Any Register */
/* Used for Micron flashes only. */ #define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */ @@ -152,6 +153,16 @@ #define SR2_QUAD_EN_BIT1 BIT(1) #define SR2_QUAD_EN_BIT7 BIT(7)
+/* Used for Spansion flashes RDAR command only. */ +#define SPINOR_REG_CR2V 0x800003 +#define CR2V_AL BIT(7) /* Address Length */ +/* Read Latency */ +#define CR2V_RL_MASK GENMASK(3, 0) +#define CR2V_RL(_nbits) ((_nbits) & CR2V_RL_MASK) + +#define SPINOR_REG_CR3V 0x800004 +#define CR3V_02H_V BIT(4) /* Page Buffer Wrap */ + /* Supported SPI protocols */ #define SNOR_PROTO_INST_MASK GENMASK(23, 16) #define SNOR_PROTO_INST_SHIFT 16
Hi，
在 2020/2/27 20:36, Alexander A Sverdlin 写道:
From: Alexander Sverdlin alexander.sverdlin@nokia.com
Spansion S25FS-S family has an issue in Basic Flash Parameter Table: DWORD-11 bits 7-4 specify write page size 512 bytes. In reality this is configurable in the non-volatile CR3NV register and even factory default configuration is "wrap at 256 bytes". So blind relying on BFPT breaks write operation on these Flashes.
All this story is vendor-specific, so add the corresponding fixup hook which first restores the safe page size of 256 bytes from struct flash_info but checks is more performant 512 bytes configuration is active and adjusts the page_size accordingly.
To read CR3V RDAR command is required which in turn requires addr width and read latency to be configured, which was not the case before. Setting these parameters is also later required for sector map selection, because:
JESD216 allows "variable address length" and "variable latency" in Configuration Detection Command Descriptors, in other words "as-is". And they are still unset during Sector Map Parameter Table parsing, which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.
New warning is added to catch the potential misconfiguration with other chips.
Link: http://lists.infradead.org/pipermail/linux-mtd/2020-February/093950.html Link: http://lists.infradead.org/pipermail/linux-mtd/2018-December/085956.html Fixes: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables") Cc: stable@vger.kernel.org Signed-off-by: Alexander Sverdlin alexander.sverdlin@nokia.com
Yes, this is a combination of two previously sent patches as it turned out, width/dummy quirk is necessary even earlier, during post_bfpt fixup.
drivers/mtd/spi-nor/spi-nor.c | 132 +++++++++++++++++++++++++++++++++++++++++- include/linux/mtd/spi-nor.h | 11 ++++ 2 files changed, 141 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c index 1224247..1d0e2ef 100644 --- a/drivers/mtd/spi-nor/spi-nor.c +++ b/drivers/mtd/spi-nor/spi-nor.c @@ -2326,6 +2326,122 @@ static struct spi_nor_fixups gd25q256_fixups = { .default_init = gd25q256_default_init, }; +/*
- Return true if it was possible to read something known and non-zero with
- the probed parameters. struct spi_nor is updated in this case as well.
- */
+static bool spi_nor_probe_width_and_dummy(struct spi_nor *nor, u8 width,
u8 dummy)+{
- u8 read_opcode = nor->read_opcode;
- u8 savedw = nor->addr_width;
- u8 savedd = nor->read_dummy;
- int ret;
- u8 buf;
- nor->addr_width = width;
- nor->read_dummy = dummy;
- nor->read_opcode = SPINOR_OP_RDAR;
- ret = spi_nor_read_data(nor, SPINOR_REG_CR2V, 1, &buf);
- nor->read_opcode = read_opcode;
- if (ret == 1 && (CR2V_RL(buf) == dummy) &&
(!!(buf & CR2V_AL) == (width == 4)))return true;- nor->addr_width = savedw;
- nor->read_dummy = savedd;
- return false;
+}
+/*
- JESD216 allows to omit particular address length or latency specification in
- the header and at this point they are still unset, so we need some
- heuristics. One example is S25FS128S.
- It was observed that RDAR with incorrect parameters result in all-zeroes or
- all-ones reads. That's why probed dummy is limited to 14 and loops are built
- in a way to probe width 3 and 0 dummy bits last to avoid false-positive
- (refer to CR2 mapping). 8 dummy bits are probed on the first iteration.
- */
+static void spi_nor_fixup_width_and_dummy(struct spi_nor *nor) +{
- u8 width_min = 3;
- u8 width_max = 4;
- u8 dummy_min = 0;
- u8 dummy_max = 14;
- u8 w, d;
- if (nor->addr_width && nor->read_dummy)
return;- if (nor->addr_width) {
width_min = nor->addr_width;width_max = nor->addr_width;- }
- if (nor->read_dummy) {
dummy_min = nor->read_dummy;dummy_max = nor->read_dummy;- }
- for (w = width_min; w <= width_max; ++w)
for (d = 8; d <= dummy_max; ++d)if (d >= dummy_min &&spi_nor_probe_width_and_dummy(nor, w, d))return;- for (w = width_max; w >= width_min; --w)
for (d = 7; d >= dummy_min; --d)if (d <= dummy_max &&spi_nor_probe_width_and_dummy(nor, w, d))return;+}
+/* Spansion S25FS-S SFDP workarounds */ +static int s25fs_s_post_bfpt_fixups(struct spi_nor *nor,
- const struct sfdp_parameter_header *bfpt_header,
- const struct sfdp_bfpt *bfpt,
- struct spi_nor_flash_parameter *params)
+{
- const struct flash_info *info = nor->info;
- u8 read_opcode, buf;
- int ret;
- /*
* RDAR command below requires nor->addr_width and nor->dummy correctly* set. spi_nor_get_map_in_use() later requires them as well.*/- spi_nor_fixup_width_and_dummy(nor);
- /* Default is safe */
- params->page_size = info->page_size;
- /*
* But is the chip configured for more performant 512 bytes write page* size?*/- read_opcode = nor->read_opcode;
- nor->read_opcode = SPINOR_OP_RDAR;
- ret = spi_nor_read_data(nor, SPINOR_REG_CR3V, 1, &buf);
- nor->read_opcode = read_opcode;
- switch (ret) {
- case 0:
return -EIO;- case 1:
if (buf & CR3V_02H_V)params->page_size = 512;return 0;- }
- return ret;
+}
+static const struct spi_nor_fixups s25fs_s_fixups = {
- .post_bfpt = s25fs_s_post_bfpt_fixups,
+};
- /* NOTE: double check command sets and memory organization when you add
- more nor chips. This current list focusses on newer chips, which
- have been converging on command sets which including JEDEC ID.
@@ -2560,7 +2676,8 @@ static const struct flash_info spi_nor_ids[] = { SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
- { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR) },
- { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, USE_CLSR)
.fixups = &s25fs_s_fixups, },@@ -2570,7 +2687,8 @@ static const struct flash_info spi_nor_ids[] = { { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256, 0) }, { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024, 64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
- { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
- { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR)
.fixups = &s25fs_s_fixups, },
It seems SFDP is not supported on s25fl129p (you can check it on https://www.cypress.com/file/400586/download), so is it necessary to add this for this type flash?
{ "s25sl004a", INFO(0x010212, 0, 64 * 1024, 8, 0) }, { "s25sl008a", INFO(0x010213, 0, 64 * 1024, 16, 0) }, { "s25sl016a", INFO(0x010214, 0, 64 * 1024, 32, 0) }, @@ -3897,6 +4015,16 @@ static const u32 *spi_nor_get_map_in_use(struct spi_nor *nor, const u32 *smpt, nor->read_opcode = SMPT_CMD_OPCODE(smpt[i]); addr = smpt[i + 1];
switch (nor->read_opcode) {case SPINOR_OP_RDAR:if (nor->read_dummy && nor->addr_width)break;dev_warn(nor->dev, "OP 0x%02x width %u dummy %u\n",nor->read_opcode, nor->addr_width,nor->read_dummy);break;}- err = spi_nor_read_raw(nor, addr, 1, buf); if (err) { ret = ERR_PTR(err);
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h index de90724..1e21592 100644 --- a/include/linux/mtd/spi-nor.h +++ b/include/linux/mtd/spi-nor.h @@ -116,6 +116,7 @@ /* Used for Spansion flashes only. */ #define SPINOR_OP_BRWR 0x17 /* Bank register write */ #define SPINOR_OP_CLSR 0x30 /* Clear status register 1 */ +#define SPINOR_OP_RDAR 0x65 /* Read Any Register */ /* Used for Micron flashes only. */ #define SPINOR_OP_RD_EVCR 0x65 /* Read EVCR register */ @@ -152,6 +153,16 @@ #define SR2_QUAD_EN_BIT1 BIT(1) #define SR2_QUAD_EN_BIT7 BIT(7) +/* Used for Spansion flashes RDAR command only. */ +#define SPINOR_REG_CR2V 0x800003 +#define CR2V_AL BIT(7) /* Address Length */ +/* Read Latency */ +#define CR2V_RL_MASK GENMASK(3, 0) +#define CR2V_RL(_nbits) ((_nbits) & CR2V_RL_MASK)
+#define SPINOR_REG_CR3V 0x800004 +#define CR3V_02H_V BIT(4) /* Page Buffer Wrap */
- /* Supported SPI protocols */ #define SNOR_PROTO_INST_MASK GENMASK(23, 16) #define SNOR_PROTO_INST_SHIFT 16
Hi!
On 28/02/2020 04:01, chenxiang (M) wrote:
JESD216 allows "variable address length" and "variable latency" in Configuration Detection Command Descriptors, in other words "as-is". And they are still unset during Sector Map Parameter Table parsing, which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.
[...]
@@ -2570,7 +2687,8 @@ static const struct flash_info spi_nor_ids[] = { { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256, 0) }, { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024, 64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, - { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, + { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) + .fixups = &s25fs_s_fixups, },
It seems SFDP is not supported on s25fl129p (you can check it on https://www.cypress.com/file/400586/download), so is it necessary to add this for this type flash?
Yes, all of the above is necessary to repair S25FS128S, which supports SFDP and lands in the above table entry.
On 28/02/2020 06:45, Alexander Sverdlin wrote:
Hi!
On 28/02/2020 04:01, chenxiang (M) wrote:
JESD216 allows "variable address length" and "variable latency" in Configuration Detection Command Descriptors, in other words "as-is". And they are still unset during Sector Map Parameter Table parsing, which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.
[...]
@@ -2570,7 +2687,8 @@ static const struct flash_info spi_nor_ids[] = { { "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024, 64, 0) }, { "s25sl12801", INFO(0x012018, 0x0301, 64 * 1024, 256, 0) }, { "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024, 64, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, - { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, + { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) + .fixups = &s25fs_s_fixups, },
It seems SFDP is not supported on s25fl129p (you can check it on https://www.cypress.com/file/400586/download), so is it necessary to add this for this type flash?
Yes, all of the above is necessary to repair S25FS128S, which supports SFDP and lands in the above table entry.
So do you know how we can tell if the part is s25fl129p1 or S25FS128S? Is it based on family id? For the part of my board, it has the same id according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP signature is not present (signature reads as 0x4d182001 vs expected 0x50444653). I printed the family id, and it is 81h, which seems to align with S25FS (which should support SFDP). Confused.
What's more, the spi-nor probe is failing for this part since I enabled quad spi. So I am interested to know if there is some differences between these part families for that.
Thanks, John
Hi John,
On 02/03/2020 19:25, John Garry wrote:
- { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, + { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) + .fixups = &s25fs_s_fixups, },
It seems SFDP is not supported on s25fl129p (you can check it on https://www.cypress.com/file/400586/download),%C2%A0so%C2%A0is%C2%A0it%C2%A0...
Yes, all of the above is necessary to repair S25FS128S, which supports SFDP and lands in the above table entry.
So do you know how we can tell if the part is s25fl129p1 or S25FS128S? Is it based on family id? For the part of my board, it has the same id according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP signature is not present (signature reads as 0x4d182001 vs expected 0x50444653). I printed the family id, and it is 81h, which seems to align with S25FS (which should support SFDP). Confused.
What's more, the spi-nor probe is failing for this part since I enabled quad spi. So I am interested to know if there is some differences between these part families for that.
I'd say, one can distinguish them by the fact one does support SFDP and another doesn't. Is it really necessary to distinguish them?
On 03/03/2020 14:27, Alexander Sverdlin wrote:
Hi Alexander,
On 02/03/2020 19:25, John Garry wrote:
- { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, + { "s25fl129p1", INFO(0x012018, 0x4d01, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) + .fixups = &s25fs_s_fixups, },
It seems SFDP is not supported on s25fl129p (you can check it on https://www.cypress.com/file/400586/download),%C2%A0so%C2%A0is%C2%A0it%C2%A0...
Yes, all of the above is necessary to repair S25FS128S, which supports SFDP and lands in the above table entry.
So do you know how we can tell if the part is s25fl129p1 or S25FS128S? Is it based on family id? For the part of my board, it has the same id according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP signature is not present (signature reads as 0x4d182001 vs expected 0x50444653). I printed the family id, and it is 81h, which seems to align with S25FS (which should support SFDP). Confused.
What's more, the spi-nor probe is failing for this part since I enabled quad spi. So I am interested to know if there is some differences between these part families for that.
I'd say, one can distinguish them by the fact one does support SFDP and another doesn't. Is it really necessary to distinguish them?
Well it would help me to know for sure which part is on my board :)
As an example of a relevant difference, S25FS128S datasheet has CR1V and CR1NV, but S25FL129 only has a single configuration register, and this is related to quad mode enable, which I am debugging.
BTW, Have you tried to enable quad mode for your S25FS-S part?
Thanks, John
Hi, John,
On Monday, March 2, 2020 8:25:48 PM EEST John Garry wrote:
So do you know how we can tell if the part is s25fl129p1 or S25FS128S? Is it based on family id? For the part of my board, it has the same id according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP signature is not present (signature reads as 0x4d182001 vs expected
0x4d182001 looks like the flash id, but in reversed order.
0x50444653). I printed the family id, and it is 81h, which seems to align with S25FS (which should support SFDP). Confused.
We can differentiate between flashes by the family id: 80h for FL-S and 81h for FS-S. If I understood correctly your flash id is 0x01, 0x20, 0x18, 0x4d, 0x01, 0x81. According to the spansion datasheets, this should identify with a s25fs128s1 entry. Please check the patch from below, let me know if it's ok.
What's more, the spi-nor probe is failing for this part since I enabled quad spi. So I am interested to know if there is some differences between these part families for that.
In which conditions is it failing? Please open a separate thread.
Cheers, ta
Author: Tudor Ambarus tudor.ambarus@microchip.com Date: Sun Apr 26 17:59:23 2020 +0300
mtd: spi-nor: spansion: Add support for s25fs128s1
The old s25fl129p1 flash uses just five bytes for manufacturer and device identification, while newer spansion flashes use six bytes. s25fs128s1 was incorrectly identified as s25fl129p1. Use INFO6 to differentiate between them.
Signed-off-by: Tudor Ambarus tudor.ambarus@microchip.com
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c index 88183eba8ac1..ea72f0e5be73 100644 --- a/drivers/mtd/spi-nor/spansion.c +++ b/drivers/mtd/spi-nor/spansion.c @@ -22,6 +22,9 @@ static const struct flash_info spansion_parts[] = { { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, + { "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + USE_CLSR) }, { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
Hi, John,
On Sunday, April 26, 2020 6:15:24 PM EEST Tudor.Ambarus@microchip.com wrote:
0x50444653). I printed the family id, and it is 81h, which seems to align with S25FS (which should support SFDP). Confused.
We can differentiate between flashes by the family id: 80h for FL-S and 81h for FS-S. If I understood correctly your flash id is 0x01, 0x20, 0x18, 0x4d, 0x01, 0x81. According to the spansion datasheets, this should identify with a s25fs128s1 entry. Please check the patch from below, let me know if it's ok.
I see that Yicong already did such a patch, check https:// patchwork.ozlabs.org/project/linux-mtd/patch/1587546809-3797-1-git-send-email- yangyicong@hisilicon.com/
What's more, the spi-nor probe is failing for this part since I enabled quad spi. So I am interested to know if there is some differences between these part families for that.
Might fail because of the page_size problem. We're trying to fix it soon.
Cheers, ta
+ Yicong Yang
Hi Tudor,
On Monday, March 2, 2020 8:25:48 PM EEST John Garry wrote:
So do you know how we can tell if the part is s25fl129p1 or S25FS128S? Is it based on family id? For the part of my board, it has the same id according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP signature is not present (signature reads as 0x4d182001 vs expected
0x4d182001 looks like the flash id, but in reversed order.
0x50444653). I printed the family id, and it is 81h, which seems to align with S25FS (which should support SFDP). Confused.
We can differentiate between flashes by the family id: 80h for FL-S and 81h for FS-S. If I understood correctly your flash id is 0x01, 0x20, 0x18, 0x4d, 0x01, 0x81. According to the spansion datasheets, this should identify with a s25fs128s1 entry. Please check the patch from below, let me know if it's ok.
What's more, the spi-nor probe is failing for this part since I enabled quad spi. So I am interested to know if there is some differences between these part families for that.
In which conditions is it failing? Please open a separate thread.
So my colleague Yicon debugged this, and it seems to be an issue with our controller. The background is that we can blacklist certain commands in firmware, and some relevant commands were blacklisted such that quad enable failed.
But we have it working now, I think. Yicon can confirm (or start a thread please for outstanding issues).
Cheers, ta
Thanks, John
Author: Tudor Ambarus tudor.ambarus@microchip.com Date: Sun Apr 26 17:59:23 2020 +0300
mtd: spi-nor: spansion: Add support for s25fs128s1 The old s25fl129p1 flash uses just five bytes for manufacturer and device identification, while newer spansion flashes use six bytes. s25fs128s1 was incorrectly identified as s25fl129p1. Use INFO6 to differentiate between them. Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c index 88183eba8ac1..ea72f0e5be73 100644 --- a/drivers/mtd/spi-nor/spansion.c +++ b/drivers/mtd/spi-nor/spansion.c @@ -22,6 +22,9 @@ static const struct flash_info spansion_parts[] = { { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
{ "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256,SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |USE_CLSR) },
I wasn't sure if you wanted to add a separate entry if it has same properties as other part, due to extra maintenance. It is nice to know the exact part, though.
{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },.
Hi John and Tudor,
On 2020/4/27 15:27, John Garry wrote:
- Yicong Yang
Hi Tudor,
On Monday, March 2, 2020 8:25:48 PM EEST John Garry wrote:
So do you know how we can tell if the part is s25fl129p1 or S25FS128S? Is it based on family id? For the part of my board, it has the same id according to "s25fl129p1" entry in the spi-nor driver, yet the SFDP signature is not present (signature reads as 0x4d182001 vs expected
0x4d182001 looks like the flash id, but in reversed order.
0x50444653). I printed the family id, and it is 81h, which seems to align with S25FS (which should support SFDP). Confused.
We can differentiate between flashes by the family id: 80h for FL-S and 81h for FS-S. If I understood correctly your flash id is 0x01, 0x20, 0x18, 0x4d, 0x01, 0x81. According to the spansion datasheets, this should identify with a s25fs128s1 entry. Please check the patch from below, let me know if it's ok.
What's more, the spi-nor probe is failing for this part since I enabled quad spi. So I am interested to know if there is some differences between these part families for that.
In which conditions is it failing? Please open a separate thread.
So my colleague Yicon debugged this, and it seems to be an issue with our controller. The background is that we can blacklist certain commands in firmware, and some relevant commands were blacklisted such that quad enable failed.
But we have it working now, I think. Yicon can confirm (or start a thread please for outstanding issues).
Yes, now the flash is fully enabled. It's the firmware matters, not about the kernel drivers nor the flash.
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c index 88183eba8ac1..ea72f0e5be73 100644 --- a/drivers/mtd/spi-nor/spansion.c +++ b/drivers/mtd/spi-nor/spansion.c @@ -22,6 +22,9 @@ static const struct flash_info spansion_parts[] = { { "s25fl128s1", INFO6(0x012018, 0x4d0180, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
{ "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256,SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |USE_CLSR) },I wasn't sure if you wanted to add a separate entry if it has same properties as other part, due to extra maintenance. It is nice to know the exact part, though.
The flash need some post bfpt fixup. As the page size may configured as 512byte, not the 256byte parsed from bfpt. So a separate entry is needed. Both Alexander and Sergei provide the solution, and I add it in my patch.
Regards, Yicong
{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },.
.
Hi
[This is an automated email]
This commit has been processed because it contains a "Fixes:" tag fixing commit: f384b352cbf0 ("mtd: spi-nor: parse Serial Flash Discoverable Parameters (SFDP) tables").
The bot has tested the following trees: v5.5.6, v5.4.22, v4.19.106, v4.14.171.
v5.5.6: Build OK! v5.4.22: Build OK! v4.19.106: Failed to apply! Possible dependencies: 1d5ceff25aa1 ("mtd: spi_nor: pass DMA-able buffer to spi_nor_read_raw()") 2aaa5f7e0c07 ("mtd: spi-nor: Add a post BFPT parsing fixup hook") 2bffa65da43e ("mtd: spi-nor: Add a post BFPT fixup for MX25L25635E") 48e4d973aefe ("mtd: spi-nor: Add a default_init() fixup hook for gd25q256") 50685024f273 ("mtd: spi-nor: split s25fl128s into s25fl128s0 and s25fl128s1") 5390a8df769e ("mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories") 815541713730 ("mtd: spi-nor: Add support for mx25u12835f") b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table") b9f07cc8207a ("mtd: spi-nor: don't overwrite errno in spi_nor_get_map_in_use()") c797bd81d10e ("mtd: spi-nor: fix iteration over smpt array")
v4.14.171: Failed to apply! Possible dependencies: 1d5ceff25aa1 ("mtd: spi_nor: pass DMA-able buffer to spi_nor_read_raw()") 2aaa5f7e0c07 ("mtd: spi-nor: Add a post BFPT parsing fixup hook") 2bffa65da43e ("mtd: spi-nor: Add a post BFPT fixup for MX25L25635E") 46dde01f6bab ("mtd: spi-nor: add spi_nor_init() function") 48e4d973aefe ("mtd: spi-nor: Add a default_init() fixup hook for gd25q256") 50685024f273 ("mtd: spi-nor: split s25fl128s into s25fl128s0 and s25fl128s1") 5390a8df769e ("mtd: spi-nor: add support to non-uniform SFDP SPI NOR flash memories") 65153846b18c ("mtd: spi-nor: add support for GD25Q256") 815541713730 ("mtd: spi-nor: Add support for mx25u12835f") b038e8e3be72 ("mtd: spi-nor: parse SFDP Sector Map Parameter Table") b9f07cc8207a ("mtd: spi-nor: don't overwrite errno in spi_nor_get_map_in_use()") c797bd81d10e ("mtd: spi-nor: fix iteration over smpt array") e27072851bf7 ("mtd: spi-nor: add a quad_enable callback in struct flash_info")
NOTE: The patch will not be queued to stable trees until it is upstream.
How should we proceed with this patch?
On Thursday, February 27, 2020 2:36:57 PM EEST Alexander A Sverdlin wrote:
Spansion S25FS-S family has an issue in Basic Flash Parameter Table:
But you modify the s25fl256s0 entry. We have to fix the flash identification first. How about the patch from below?
Author: Tudor Ambarus tudor.ambarus@microchip.com Date: Sun Apr 26 18:33:33 2020 +0300
mtd: spi-nor: spansion: Differentiate between s25fl256s and s25fs256s
s25fs256s was identified as s25fl256s. Differentiate between them by the Family ID using the INFO6 macro.
Fixes: c4b3eacc1dfe ("spi-nor: Recover from Spansion/Cypress errors") Signed-off-by: Tudor Ambarus tudor.ambarus@microchip.com
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c index ea72f0e5be73..8ea30491cdd7 100644 --- a/drivers/mtd/spi-nor/spansion.c +++ b/drivers/mtd/spi-nor/spansion.c @@ -25,15 +25,21 @@ static const struct flash_info spansion_parts[] = { { "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) }, - { "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128, - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | - USE_CLSR) }, - { "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512, - SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | - USE_CLSR) }, + { "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + USE_CLSR) }, + { "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + USE_CLSR) }, { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | USE_CLSR) }, + { "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + USE_CLSR) }, + { "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512, + SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | + USE_CLSR) }, { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
Hi!
On 26/04/2020 17:40, Tudor.Ambarus@microchip.com wrote:
On Thursday, February 27, 2020 2:36:57 PM EEST Alexander A Sverdlin wrote:
Spansion S25FS-S family has an issue in Basic Flash Parameter Table:
But you modify the s25fl256s0 entry. We have to fix the flash identification first. How about the patch from below?
Author: Tudor Ambarus tudor.ambarus@microchip.com Date: Sun Apr 26 18:33:33 2020 +0300
mtd: spi-nor: spansion: Differentiate between s25fl256s and s25fs256ss25fs256s was identified as s25fl256s. Differentiate between them by the Family ID using the INFO6 macro. Fixes: c4b3eacc1dfe ("spi-nor: Recover from Spansion/Cypress errors")
The patch itself looks fine to me except the "Fixes:" tag above, which has a potential for improvement. The mentioned commit is not related to FL-FS aliasing.
Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c index ea72f0e5be73..8ea30491cdd7 100644 --- a/drivers/mtd/spi-nor/spansion.c +++ b/drivers/mtd/spi-nor/spansion.c @@ -25,15 +25,21 @@ static const struct flash_info spansion_parts[] = { { "s25fs128s1", INFO6(0x012018, 0x4d0181, 64 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
{ "s25fl256s0", INFO(0x010219, 0x4d00, 256 * 1024, 128,SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |USE_CLSR) },{ "s25fl256s1", INFO(0x010219, 0x4d01, 64 * 1024, 512,SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |USE_CLSR) },
{ "s25fl256s0", INFO6(0x010219, 0x4d0080, 256 * 1024, 128,SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |USE_CLSR) },{ "s25fl256s1", INFO6(0x010219, 0x4d0180, 64 * 1024, 512,SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |USE_CLSR) }, { "s25fl512s", INFO6(0x010220, 0x4d0080, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_HAS_LOCK | USE_CLSR) },{ "s25fs256s0", INFO6(0x010219, 0x4d0081, 256 * 1024, 128,SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |USE_CLSR) },{ "s25fs256s1", INFO6(0x010219, 0x4d0181, 64 * 1024, 512,SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |USE_CLSR) }, { "s25fs512s", INFO6(0x010220, 0x4d0081, 256 * 1024, 256, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | USE_CLSR) },
Hi, Alexander,
On Monday, April 27, 2020 11:23:40 AM EEST Alexander Sverdlin wrote:
Fixes: c4b3eacc1dfe ("spi-nor: Recover from Spansion/Cypress errors")
The patch itself looks fine to me except the "Fixes:" tag above, which has a potential for improvement. The mentioned commit is not related to FL-FS aliasing.
Oh, right, the flash was added with the addition of the SPI NOR framework, will use the following instead: Fixes: b199489d37b ("mtd: spi-nor: add the framework for SPI NOR")
Have you seen my other reply? https://www.spinics.net/lists/stable/ msg382714.html
Cheers, ta
Hi, Alexander,
On Thursday, February 27, 2020 2:36:57 PM EEST Alexander A Sverdlin wrote:
From: Alexander Sverdlin alexander.sverdlin@nokia.com
Spansion S25FS-S family has an issue in Basic Flash Parameter Table: DWORD-11 bits 7-4 specify write page size 512 bytes. In reality this is configurable in the non-volatile CR3NV register and even factory default configuration is "wrap at 256 bytes". So blind relying on BFPT breaks write operation on these Flashes.
All this story is vendor-specific, so add the corresponding fixup hook which first restores the safe page size of 256 bytes from struct flash_info but checks is more performant 512 bytes configuration is active and adjusts the page_size accordingly.
Right, clear.
To read CR3V RDAR command is required which in turn requires addr width and read latency to be configured, which was not the case before. Setting these parameters is also later required for sector map selection, because:
JESD216 allows "variable address length" and "variable latency" in Configuration Detection Command Descriptors, in other words "as-is". And they are still unset during Sector Map Parameter Table parsing, which led to "map_id" determined erroneously as 0 for, e.g. S25FS128S.
Please let me know I I get this right. You need to determine the addr_width and read_dummy for the RDAR command, in order to determine if the 512 page_size is active and use that instead. addr_width is not yet set because you probably are in the BFPT_DWORD1_ADDRESS_BYTES_3_OR_4 case, and as of now the case is not treated. nor->read_dummy is not set because the read operation is not yet selected.
Can we safely assume that the read_dummy for the RDAR command has the same value as the read_dummy value used for all the array read commands except Read (zero latency cycles) and RSFDP (8 latency cycles)? If yes, we can determine read_dummy from BFPT and get rid of the try and error iteration.
Next, we can probably use the same values at the SMPT parsing, if you hit the SMPT_CMD_ADDRESS_LEN_USE_CURRENT and SMPT_CMD_READ_DUMMY_IS_VARIABLE cases.
Cheers, ta
linux-stable-mirror@lists.linaro.org
-
Alexander A Sverdlin -
Alexander Sverdlin
-
chenxiang (M) -
John Garry -
Sasha Levin -
Tudor.Ambarus＠microchip.com -
Yicong Yang