Discussion:
[PATCH] mtd: spi-nor: Provide default address width and latency for map selection
Sverdlin, Alexander (Nokia - DE/Ulm)
2018-11-29 18:15:34 UTC
Permalink
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.

Fixes: b038e8e3b ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
Signed-off-by: Alexander Sverdlin <***@nokia.com>
---
drivers/mtd/spi-nor/spi-nor.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 828d03e..5557c89 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2893,6 +2893,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];

+ /*
+ * 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.
+ */
+ if (!nor->addr_width)
+ nor->addr_width = 3;
+ if (!nor->read_dummy)
+ nor->read_dummy = 8;
+
err = spi_nor_read_raw(nor, addr, 1, &data_byte);
if (err)
goto out;
--
2.4.6
Boris Brezillon
2018-11-30 10:40:19 UTC
Permalink
On Thu, 29 Nov 2018 18:15:34 +0000
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
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.
Fixes: b038e8e3b ("mtd: spi-nor: parse SFDP Sector Map Parameter Table")
---
drivers/mtd/spi-nor/spi-nor.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 828d03e..5557c89 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -2893,6 +2893,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];
+ /*
+ * 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.
+ */
+ if (!nor->addr_width)
+ nor->addr_width = 3;
+ if (!nor->read_dummy)
+ nor->read_dummy = 8;
+
Looks like the same problem was reported by Yogesh here [1]. One more
proof that parsing SFDP is not trivial. I mean, what's the point of
defining a generic tables to describe NOR capabilities if you then
depend on vendor/chip specific init to read those tables...

Anyway, I think this sort of initialization should be placed in a
pre_sfdp() fixup hook, as getting the right values likely requires
reading some volatile/non-volatile regs.
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
err = spi_nor_read_raw(nor, addr, 1, &data_byte);
if (err)
goto out;
[1]https://lkml.org/lkml/2018/10/22/354
Sverdlin, Alexander (Nokia - DE/Ulm)
2018-12-03 08:03:18 UTC
Permalink
Hello Boris!
Post by Boris Brezillon
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
+ /*
+ * 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.
+ */
+ if (!nor->addr_width)
+ nor->addr_width = 3;
+ if (!nor->read_dummy)
+ nor->read_dummy = 8;
+
Looks like the same problem was reported by Yogesh here [1]. One more
This is indeed the same problem, although I was not aware of the [1] discussion.
Post by Boris Brezillon
proof that parsing SFDP is not trivial. I mean, what's the point of
defining a generic tables to describe NOR capabilities if you then
depend on vendor/chip specific init to read those tables...
Anyway, I think this sort of initialization should be placed in a
pre_sfdp() fixup hook, as getting the right values likely requires
This is still on my TODO list, to learn about new hooks.
Post by Boris Brezillon
reading some volatile/non-volatile regs.
This is the same instruction 65h which is used to read regs and which
appears in SMPT headers, it is a chicken-egg problem.

Therefore, I don't know if it's possible to provide smarter heuristics
here. But without it Spansion S25FS-S Family is now broken by SFDP.
Post by Boris Brezillon
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
err = spi_nor_read_raw(nor, addr, 1, &data_byte);
if (err)
goto out;
[1]https://lkml.org/lkml/2018/10/22/354
--
Best regards,
Alexander Sverdlin.
Boris Brezillon
2018-12-03 08:23:37 UTC
Permalink
On Mon, 3 Dec 2018 08:03:18 +0000
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Hello Boris!
Post by Boris Brezillon
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
+ /*
+ * 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.
+ */
+ if (!nor->addr_width)
+ nor->addr_width = 3;
+ if (!nor->read_dummy)
+ nor->read_dummy = 8;
+
Looks like the same problem was reported by Yogesh here [1]. One more
This is indeed the same problem, although I was not aware of the [1] discussion.
Post by Boris Brezillon
proof that parsing SFDP is not trivial. I mean, what's the point of
defining a generic tables to describe NOR capabilities if you then
depend on vendor/chip specific init to read those tables...
Anyway, I think this sort of initialization should be placed in a
pre_sfdp() fixup hook, as getting the right values likely requires
This is still on my TODO list, to learn about new hooks.
->pre_sfdp() does not exist yet, but I'm about to add it for other
reasons, and it looks like a good place to put this sort of
initialization.
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Post by Boris Brezillon
reading some volatile/non-volatile regs.
This is the same instruction 65h which is used to read regs and which
appears in SMPT headers, it is a chicken-egg problem.
Oh, right, I remember now. Not a smart decision from Spansion :-/.
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Therefore, I don't know if it's possible to provide smarter heuristics
here.
Maybe:

ref_cr1 = read_CR1_using_RDCR()

for_each_possible_dummy_and_addr_width
cr1 = read_CR1_using_RDAR()
if (cr1 == ref_cr1)
break;
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
But without it Spansion S25FS-S Family is now broken by SFDP.
Do you have [1] in your tree? IIRC, this fixed Yogesh issue.

[1]https://patchwork.ozlabs.org/patch/994765/
Sverdlin, Alexander (Nokia - DE/Ulm)
2018-12-03 08:34:27 UTC
Permalink
Hi!
Post by Boris Brezillon
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
This is the same instruction 65h which is used to read regs and which
appears in SMPT headers, it is a chicken-egg problem.
Oh, right, I remember now. Not a smart decision from Spansion :-/.
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Therefore, I don't know if it's possible to provide smarter heuristics
here.
ref_cr1 = read_CR1_using_RDCR()
for_each_possible_dummy_and_addr_width
cr1 = read_CR1_using_RDAR()
if (cr1 == ref_cr1)
break;
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
But without it Spansion S25FS-S Family is now broken by SFDP.
Do you have [1] in your tree? IIRC, this fixed Yogesh issue.
[1]https://patchwork.ozlabs.org/patch/994765/
This is definitely unrelated, in my case SMPT is happily processed leading
to completely wrong mapping because command 65h with addr_width == 0 always
returns 0.
--
Best regards,
Alexander Sverdlin.
Sverdlin, Alexander (Nokia - DE/Ulm)
2018-12-03 08:37:17 UTC
Permalink
Hi!
Post by Boris Brezillon
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
This is the same instruction 65h which is used to read regs and which
appears in SMPT headers, it is a chicken-egg problem.
Oh, right, I remember now. Not a smart decision from Spansion :-/.
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Therefore, I don't know if it's possible to provide smarter heuristics
here.
ref_cr1 = read_CR1_using_RDCR()
for_each_possible_dummy_and_addr_width
cr1 = read_CR1_using_RDAR()
if (cr1 == ref_cr1)
break;
This will not work, as default value for CR1(N)V is 0 and that is the value
one gets with incorrect addr_width as well.
--
Best regards,
Alexander Sverdlin.
Boris Brezillon
2018-12-03 09:08:14 UTC
Permalink
On Mon, 3 Dec 2018 08:37:17 +0000
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Hi!
Post by Boris Brezillon
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
This is the same instruction 65h which is used to read regs and which
appears in SMPT headers, it is a chicken-egg problem.
Oh, right, I remember now. Not a smart decision from Spansion :-/.
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Therefore, I don't know if it's possible to provide smarter heuristics
here.
ref_cr1 = read_CR1_using_RDCR()
for_each_possible_dummy_and_addr_width
cr1 = read_CR1_using_RDAR()
if (cr1 == ref_cr1)
break;
This will not work, as default value for CR1(N)V is 0 and that is the value
one gets with incorrect addr_width as well.
How about:

// set the WE bit in SR1 so that SR1 is not 0
write_enable()

ref_sr1 = read_SR1_using_RDSR1()
for_each_possible_dummy_and_addr_width
sr1 = read_SR1_using_RDAR()
if (sr1 == ref_sr1)
break;

write_disable()
Boris Brezillon
2018-12-03 09:34:50 UTC
Permalink
On Mon, 3 Dec 2018 10:08:14 +0100
Post by Boris Brezillon
On Mon, 3 Dec 2018 08:37:17 +0000
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Hi!
Post by Boris Brezillon
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
This is the same instruction 65h which is used to read regs and which
appears in SMPT headers, it is a chicken-egg problem.
Oh, right, I remember now. Not a smart decision from Spansion :-/.
Post by Sverdlin, Alexander (Nokia - DE/Ulm)
Therefore, I don't know if it's possible to provide smarter heuristics
here.
ref_cr1 = read_CR1_using_RDCR()
for_each_possible_dummy_and_addr_width
cr1 = read_CR1_using_RDAR()
if (cr1 == ref_cr1)
break;
This will not work, as default value for CR1(N)V is 0 and that is the value
one gets with incorrect addr_width as well.
// set the WE bit in SR1 so that SR1 is not 0
write_enable()
ref_sr1 = read_SR1_using_RDSR1()
for_each_possible_dummy_and_addr_width
sr1 = read_SR1_using_RDAR()
if (sr1 == ref_sr1)
break;
Oh, and you should read CR2 to make sure AL and RL fields match the
selected addr/dummy values.
Post by Boris Brezillon
write_disable()
Loading...