[RFC 0/3] mtd: spi-nor: dealing with reused JEDEC id c22016

[RFC 0/3] mtd: spi-nor: dealing with reused JEDEC id c22016

[Rasmus Villemoes]

We use the Macronix chip mx25l3233f in a number of
products.

Unfortunately, it has the same JEDEC id as another chip which is
already listed in macronix_parts[]. Since that other one does not
support SFDP, and its data sheet warns against issuing commands not
explicitly listed, we can't just do RDSFDP anyway and decide that it's
an mx25l3205d when the chip returns garbage.

For lack of better alternative, start allowing multiple entries with
the same JEDEC id in the parts tables. That allows a correctly written
device tree to specify the right chip, without being overruled by the
"JEDEC knows better" heuristic, while being backwards-compatible (as
long as new chips with recycled ids get added after the existing
ones).

While a step forward, this isn't quite a complete solution for our case:

Some of our platforms are based on LS1021A, thus using the
spi-fsl-qspi driver. Back in the 4.19 kernel, when the driver was
fsl-quadspi, we couldn't get the flash recognized unless we
monkey-patch-replaced the mx25l3205d entry with the mx25l3233f one
(i.e. added the SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ bits) - we'd
fail in spi_nor_select_read() because
shared_hwcaps&SNOR_HWCAPS_READ_MASK would be empty. In contrast, with
current master, the chip works with or without the third patch in this
series, i.e. whether it is detected as a mx25l3205d or mx25l3233f. But
the read performance is ~3 times worse than in our patched 4.19 - I
haven't quite figured out why quad read doesn't seem to be used or
work.


Rasmus Villemoes (3):
  mtd: spi-nor: core: create helper to compare JEDEC id to struct
    flash_info
  mtd: spi-nor: core: compare JEDEC bytes to already found flash_info
  mtd: spi-nor: macronix: add entry for mx25l3233f

 drivers/mtd/spi-nor/core.c     | 18 +++++++++++++-----
 drivers/mtd/spi-nor/macronix.c |  3 +++
 2 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.31.1

[RFC 0/3] mtd: spi-nor: dealing with reused JEDEC id c22016

[Rasmus Villemoes]

We use the Macronix chip mx25l3233f in a number of
products.

Unfortunately, it has the same JEDEC id as another chip which is
already listed in macronix_parts[]. Since that other one does not
support SFDP, and its data sheet warns against issuing commands not
explicitly listed, we can't just do RDSFDP anyway and decide that it's
an mx25l3205d when the chip returns garbage.

For lack of better alternative, start allowing multiple entries with
the same JEDEC id in the parts tables. That allows a correctly written
device tree to specify the right chip, without being overruled by the
"JEDEC knows better" heuristic, while being backwards-compatible (as
long as new chips with recycled ids get added after the existing
ones).

While a step forward, this isn't quite a complete solution for our case:

Some of our platforms are based on LS1021A, thus using the
spi-fsl-qspi driver. Back in the 4.19 kernel, when the driver was
fsl-quadspi, we couldn't get the flash recognized unless we
monkey-patch-replaced the mx25l3205d entry with the mx25l3233f one
(i.e. added the SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ bits) - we'd
fail in spi_nor_select_read() because
shared_hwcaps&SNOR_HWCAPS_READ_MASK would be empty. In contrast, with
current master, the chip works with or without the third patch in this
series, i.e. whether it is detected as a mx25l3205d or mx25l3233f. But
the read performance is ~3 times worse than in our patched 4.19 - I
haven't quite figured out why quad read doesn't seem to be used or
work.


Rasmus Villemoes (3):
  mtd: spi-nor: core: create helper to compare JEDEC id to struct
    flash_info
  mtd: spi-nor: core: compare JEDEC bytes to already found flash_info
  mtd: spi-nor: macronix: add entry for mx25l3233f

 drivers/mtd/spi-nor/core.c     | 18 +++++++++++++-----
 drivers/mtd/spi-nor/macronix.c |  3 +++
 2 files changed, 16 insertions(+), 5 deletions(-)

-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[RFC 1/3] mtd: spi-nor: core: create helper to compare JEDEC id to struct flash_info

[Rasmus Villemoes]

This check will be used elsewhere in a later patch, so factor out the
logic to a helper function.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/mtd/spi-nor/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index bd2c7717eb10..6a1adef0fe9f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1850,6 +1850,11 @@ static const struct spi_nor_manufacturer *manufacturers[] = {
 	&spi_nor_xmc,
 };
 
+static bool spi_nor_match_part(const struct flash_info *part, const u8 *id)
+{
+	return part->id_len && !memcmp(part->id, id, part->id_len);
+}
+
 static const struct flash_info *
 spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
 			  const u8 *id)
@@ -1857,8 +1862,7 @@ spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
 	unsigned int i;
 
 	for (i = 0; i < nparts; i++) {
-		if (parts[i].id_len &&
-		    !memcmp(parts[i].id, id, parts[i].id_len))
+		if (spi_nor_match_part(&parts[i], id))
 			return &parts[i];
 	}
 
-- 
2.31.1

[RFC 1/3] mtd: spi-nor: core: create helper to compare JEDEC id to struct flash_info

[Rasmus Villemoes]

This check will be used elsewhere in a later patch, so factor out the
logic to a helper function.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/mtd/spi-nor/core.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index bd2c7717eb10..6a1adef0fe9f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1850,6 +1850,11 @@ static const struct spi_nor_manufacturer *manufacturers[] = {
 	&spi_nor_xmc,
 };
 
+static bool spi_nor_match_part(const struct flash_info *part, const u8 *id)
+{
+	return part->id_len && !memcmp(part->id, id, part->id_len);
+}
+
 static const struct flash_info *
 spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
 			  const u8 *id)
@@ -1857,8 +1862,7 @@ spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
 	unsigned int i;
 
 	for (i = 0; i < nparts; i++) {
-		if (parts[i].id_len &&
-		    !memcmp(parts[i].id, id, parts[i].id_len))
+		if (spi_nor_match_part(&parts[i], id))
 			return &parts[i];
 	}
 
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Rasmus Villemoes]

Macronix engineers, in their infinite wisdom, have a habit of reusing
JEDEC ids for different chips. There's already one
workaround (MX25L25635F v MX25L25635E), but the same problem exists
for MX25L3205D v MX25L3233F, the latter of which is not currently
supported by linux.

AFAICT, that case cannot really be handled with any of the ->fixup
machinery: The correct entry for the MX25L3233F would read

        { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },

while the existing one is

	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },

So in spi_nor_init_params(), we won't even try reading the sfdp
info (i.e. call spi_nor_sfdp_init_params), and hence
spi_nor_post_sfdp_fixups() has no way of distinguishing the
chips.

Replacing the existing entry with the mx25l3233f one to coerce the
core into issuing the SPINOR_OP_RDSFDP is also not really an option,
because the data sheet for the mx25l3205d explicitly says not to issue
any commands not listed ("It is not recommended to adopt any other
code not in the command definition table, which will potentially enter
the hidden mode.", whatever that means).

In order to support such cases, extend the logic in spi_nor_read_id()
a little so that if we already have a struct flash_info* from the name
in device tree, check the JEDEC bytes against that, and if it is a
match, accept that (device tree compatible + matching JEDEC bytes) is
stronger than merely matching JEDEC bytes.

This also makes initialization slightly faster in the common case
where the flash_info was found from the name and the JEDEC bytes do
match - it avoids a second linear search over all known chips.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/mtd/spi-nor/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 6a1adef0fe9f..21ae655d6a6f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1869,7 +1869,8 @@ spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
 	return NULL;
 }
 
-static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
+static const struct flash_info *
+spi_nor_read_id(struct spi_nor *nor, const struct flash_info *guess)
 {
 	const struct flash_info *info;
 	u8 *id = nor->bouncebuf;
@@ -1893,6 +1894,9 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
 		return ERR_PTR(ret);
 	}
 
+	if (guess && spi_nor_match_part(guess, id))
+		return guess;
+
 	for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
 		info = spi_nor_search_part_by_id(manufacturers[i]->parts,
 						 manufacturers[i]->nparts,
@@ -3031,7 +3035,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
 		info = spi_nor_match_id(nor, name);
 	/* Try to auto-detect if chip name wasn't specified or not found */
 	if (!info)
-		info = spi_nor_read_id(nor);
+		info = spi_nor_read_id(nor, NULL);
 	if (IS_ERR_OR_NULL(info))
 		return ERR_PTR(-ENOENT);
 
@@ -3042,7 +3046,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
 	if (name && info->id_len) {
 		const struct flash_info *jinfo;
 
-		jinfo = spi_nor_read_id(nor);
+		jinfo = spi_nor_read_id(nor, info);
 		if (IS_ERR(jinfo)) {
 			return jinfo;
 		} else if (jinfo != info) {
-- 
2.31.1

[RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Rasmus Villemoes]

Macronix engineers, in their infinite wisdom, have a habit of reusing
JEDEC ids for different chips. There's already one
workaround (MX25L25635F v MX25L25635E), but the same problem exists
for MX25L3205D v MX25L3233F, the latter of which is not currently
supported by linux.

AFAICT, that case cannot really be handled with any of the ->fixup
machinery: The correct entry for the MX25L3233F would read

        { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },

while the existing one is

	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },

So in spi_nor_init_params(), we won't even try reading the sfdp
info (i.e. call spi_nor_sfdp_init_params), and hence
spi_nor_post_sfdp_fixups() has no way of distinguishing the
chips.

Replacing the existing entry with the mx25l3233f one to coerce the
core into issuing the SPINOR_OP_RDSFDP is also not really an option,
because the data sheet for the mx25l3205d explicitly says not to issue
any commands not listed ("It is not recommended to adopt any other
code not in the command definition table, which will potentially enter
the hidden mode.", whatever that means).

In order to support such cases, extend the logic in spi_nor_read_id()
a little so that if we already have a struct flash_info* from the name
in device tree, check the JEDEC bytes against that, and if it is a
match, accept that (device tree compatible + matching JEDEC bytes) is
stronger than merely matching JEDEC bytes.

This also makes initialization slightly faster in the common case
where the flash_info was found from the name and the JEDEC bytes do
match - it avoids a second linear search over all known chips.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/mtd/spi-nor/core.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 6a1adef0fe9f..21ae655d6a6f 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1869,7 +1869,8 @@ spi_nor_search_part_by_id(const struct flash_info *parts, unsigned int nparts,
 	return NULL;
 }
 
-static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
+static const struct flash_info *
+spi_nor_read_id(struct spi_nor *nor, const struct flash_info *guess)
 {
 	const struct flash_info *info;
 	u8 *id = nor->bouncebuf;
@@ -1893,6 +1894,9 @@ static const struct flash_info *spi_nor_read_id(struct spi_nor *nor)
 		return ERR_PTR(ret);
 	}
 
+	if (guess && spi_nor_match_part(guess, id))
+		return guess;
+
 	for (i = 0; i < ARRAY_SIZE(manufacturers); i++) {
 		info = spi_nor_search_part_by_id(manufacturers[i]->parts,
 						 manufacturers[i]->nparts,
@@ -3031,7 +3035,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
 		info = spi_nor_match_id(nor, name);
 	/* Try to auto-detect if chip name wasn't specified or not found */
 	if (!info)
-		info = spi_nor_read_id(nor);
+		info = spi_nor_read_id(nor, NULL);
 	if (IS_ERR_OR_NULL(info))
 		return ERR_PTR(-ENOENT);
 
@@ -3042,7 +3046,7 @@ static const struct flash_info *spi_nor_get_flash_info(struct spi_nor *nor,
 	if (name && info->id_len) {
 		const struct flash_info *jinfo;
 
-		jinfo = spi_nor_read_id(nor);
+		jinfo = spi_nor_read_id(nor, info);
 		if (IS_ERR(jinfo)) {
 			return jinfo;
 		} else if (jinfo != info) {
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[RFC 3/3] mtd: spi-nor: macronix: add entry for mx25l3233f

[Rasmus Villemoes]

This has the same JEDEC id as mx25l3205d, so people will have to
provide a correct compatible string in DT in order to distinguish
this from the mx25l3205d. It cannot be done at run-time by parsing
SFDP tables because mx25l3205d does not support SFDP.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/mtd/spi-nor/macronix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 42c2cf31702e..8afbdb7c21b4 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -40,6 +40,9 @@ static const struct flash_info macronix_parts[] = {
 	{ "mx25l8005",   INFO(0xc22014, 0, 64 * 1024,  16, 0) },
 	{ "mx25l1606e",  INFO(0xc22015, 0, 64 * 1024,  32, SECT_4K) },
 	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
+	{ "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64,
+			      SECT_4K | SPI_NOR_DUAL_READ |
+			      SPI_NOR_QUAD_READ ) },
 	{ "mx25l3255e",  INFO(0xc29e16, 0, 64 * 1024,  64, SECT_4K) },
 	{ "mx25l6405d",  INFO(0xc22017, 0, 64 * 1024, 128, SECT_4K) },
 	{ "mx25u2033e",  INFO(0xc22532, 0, 64 * 1024,   4, SECT_4K) },
-- 
2.31.1

[RFC 3/3] mtd: spi-nor: macronix: add entry for mx25l3233f

[Rasmus Villemoes]

This has the same JEDEC id as mx25l3205d, so people will have to
provide a correct compatible string in DT in order to distinguish
this from the mx25l3205d. It cannot be done at run-time by parsing
SFDP tables because mx25l3205d does not support SFDP.

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/mtd/spi-nor/macronix.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 42c2cf31702e..8afbdb7c21b4 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -40,6 +40,9 @@ static const struct flash_info macronix_parts[] = {
 	{ "mx25l8005",   INFO(0xc22014, 0, 64 * 1024,  16, 0) },
 	{ "mx25l1606e",  INFO(0xc22015, 0, 64 * 1024,  32, SECT_4K) },
 	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
+	{ "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64,
+			      SECT_4K | SPI_NOR_DUAL_READ |
+			      SPI_NOR_QUAD_READ ) },
 	{ "mx25l3255e",  INFO(0xc29e16, 0, 64 * 1024,  64, SECT_4K) },
 	{ "mx25l6405d",  INFO(0xc22017, 0, 64 * 1024, 128, SECT_4K) },
 	{ "mx25u2033e",  INFO(0xc22532, 0, 64 * 1024,   4, SECT_4K) },
-- 
2.31.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC 0/3] mtd: spi-nor: dealing with reused JEDEC id c22016

[Rasmus Villemoes]

On 21/06/2021 17.23, Rasmus Villemoes wrote:
> We use the Macronix chip mx25l3233f in a number of
> products.
> 
> Unfortunately, it has the same JEDEC id as another chip which is
> already listed in macronix_parts[]. Since that other one does not
> support SFDP, and its data sheet warns against issuing commands not
> explicitly listed, we can't just do RDSFDP anyway and decide that it's
> an mx25l3205d when the chip returns garbage.
> 
> For lack of better alternative, start allowing multiple entries with
> the same JEDEC id in the parts tables. That allows a correctly written
> device tree to specify the right chip, without being overruled by the
> "JEDEC knows better" heuristic, while being backwards-compatible (as
> long as new chips with recycled ids get added after the existing
> ones).
> 
> While a step forward, this isn't quite a complete solution for our case:
> 
> Some of our platforms are based on LS1021A, thus using the
> spi-fsl-qspi driver. Back in the 4.19 kernel, when the driver was
> fsl-quadspi, we couldn't get the flash recognized unless we
> monkey-patch-replaced the mx25l3205d entry with the mx25l3233f one
> (i.e. added the SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ bits) - we'd
> fail in spi_nor_select_read() because
> shared_hwcaps&SNOR_HWCAPS_READ_MASK would be empty. In contrast, with
> current master, the chip works with or without the third patch in this
> series, i.e. whether it is detected as a mx25l3205d or mx25l3233f. But
> the read performance is ~3 times worse than in our patched 4.19 - I
> haven't quite figured out why quad read doesn't seem to be used or
> work.

Sorry about that last part, that's a PEBKAC. Adding proper
spi-rx-bus-width = <4> properties to DT got the performance back to what
it used to be.

However, I still do need the flashes to be recognized as mx25l3233f and
not mx25l3205d.

Rasmus

Re: [RFC 0/3] mtd: spi-nor: dealing with reused JEDEC id c22016

[Rasmus Villemoes]

On 21/06/2021 17.23, Rasmus Villemoes wrote:
> We use the Macronix chip mx25l3233f in a number of
> products.
> 
> Unfortunately, it has the same JEDEC id as another chip which is
> already listed in macronix_parts[]. Since that other one does not
> support SFDP, and its data sheet warns against issuing commands not
> explicitly listed, we can't just do RDSFDP anyway and decide that it's
> an mx25l3205d when the chip returns garbage.
> 
> For lack of better alternative, start allowing multiple entries with
> the same JEDEC id in the parts tables. That allows a correctly written
> device tree to specify the right chip, without being overruled by the
> "JEDEC knows better" heuristic, while being backwards-compatible (as
> long as new chips with recycled ids get added after the existing
> ones).
> 
> While a step forward, this isn't quite a complete solution for our case:
> 
> Some of our platforms are based on LS1021A, thus using the
> spi-fsl-qspi driver. Back in the 4.19 kernel, when the driver was
> fsl-quadspi, we couldn't get the flash recognized unless we
> monkey-patch-replaced the mx25l3205d entry with the mx25l3233f one
> (i.e. added the SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ bits) - we'd
> fail in spi_nor_select_read() because
> shared_hwcaps&SNOR_HWCAPS_READ_MASK would be empty. In contrast, with
> current master, the chip works with or without the third patch in this
> series, i.e. whether it is detected as a mx25l3205d or mx25l3233f. But
> the read performance is ~3 times worse than in our patched 4.19 - I
> haven't quite figured out why quad read doesn't seem to be used or
> work.

Sorry about that last part, that's a PEBKAC. Adding proper
spi-rx-bus-width = <4> properties to DT got the performance back to what
it used to be.

However, I still do need the flashes to be recognized as mx25l3233f and
not mx25l3205d.

Rasmus

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC 0/3] mtd: spi-nor: dealing with reused JEDEC id c22016

[Esben Haabendal]

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

> We use the Macronix chip mx25l3233f in a number of
> products.
>
> Unfortunately, it has the same JEDEC id as another chip which is
> already listed in macronix_parts[]. Since that other one does not
> support SFDP, and its data sheet warns against issuing commands not
> explicitly listed, we can't just do RDSFDP anyway and decide that it's
> an mx25l3205d when the chip returns garbage.
>
> For lack of better alternative, start allowing multiple entries with
> the same JEDEC id in the parts tables. That allows a correctly written
> device tree to specify the right chip, without being overruled by the
> "JEDEC knows better" heuristic, while being backwards-compatible (as
> long as new chips with recycled ids get added after the existing
> ones).
>
> While a step forward, this isn't quite a complete solution for our case:
>
> Some of our platforms are based on LS1021A, thus using the
> spi-fsl-qspi driver. Back in the 4.19 kernel, when the driver was
> fsl-quadspi, we couldn't get the flash recognized unless we
> monkey-patch-replaced the mx25l3205d entry with the mx25l3233f one
> (i.e. added the SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ bits) - we'd
> fail in spi_nor_select_read() because
> shared_hwcaps&SNOR_HWCAPS_READ_MASK would be empty. In contrast, with
> current master, the chip works with or without the third patch in this
> series, i.e. whether it is detected as a mx25l3205d or mx25l3233f. But
> the read performance is ~3 times worse than in our patched 4.19 - I
> haven't quite figured out why quad read doesn't seem to be used or
> work.
>
>
> Rasmus Villemoes (3):
>   mtd: spi-nor: core: create helper to compare JEDEC id to struct
>     flash_info
>   mtd: spi-nor: core: compare JEDEC bytes to already found flash_info
>   mtd: spi-nor: macronix: add entry for mx25l3233f
>
>  drivers/mtd/spi-nor/core.c     | 18 +++++++++++++-----
>  drivers/mtd/spi-nor/macronix.c |  3 +++
>  2 files changed, 16 insertions(+), 5 deletions(-)

For whole series:

Acked-by: Esben Haabendal <esben@geanix.com>

Re: [RFC 0/3] mtd: spi-nor: dealing with reused JEDEC id c22016

[Esben Haabendal]

Rasmus Villemoes <linux@rasmusvillemoes.dk> writes:

> We use the Macronix chip mx25l3233f in a number of
> products.
>
> Unfortunately, it has the same JEDEC id as another chip which is
> already listed in macronix_parts[]. Since that other one does not
> support SFDP, and its data sheet warns against issuing commands not
> explicitly listed, we can't just do RDSFDP anyway and decide that it's
> an mx25l3205d when the chip returns garbage.
>
> For lack of better alternative, start allowing multiple entries with
> the same JEDEC id in the parts tables. That allows a correctly written
> device tree to specify the right chip, without being overruled by the
> "JEDEC knows better" heuristic, while being backwards-compatible (as
> long as new chips with recycled ids get added after the existing
> ones).
>
> While a step forward, this isn't quite a complete solution for our case:
>
> Some of our platforms are based on LS1021A, thus using the
> spi-fsl-qspi driver. Back in the 4.19 kernel, when the driver was
> fsl-quadspi, we couldn't get the flash recognized unless we
> monkey-patch-replaced the mx25l3205d entry with the mx25l3233f one
> (i.e. added the SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ bits) - we'd
> fail in spi_nor_select_read() because
> shared_hwcaps&SNOR_HWCAPS_READ_MASK would be empty. In contrast, with
> current master, the chip works with or without the third patch in this
> series, i.e. whether it is detected as a mx25l3205d or mx25l3233f. But
> the read performance is ~3 times worse than in our patched 4.19 - I
> haven't quite figured out why quad read doesn't seem to be used or
> work.
>
>
> Rasmus Villemoes (3):
>   mtd: spi-nor: core: create helper to compare JEDEC id to struct
>     flash_info
>   mtd: spi-nor: core: compare JEDEC bytes to already found flash_info
>   mtd: spi-nor: macronix: add entry for mx25l3233f
>
>  drivers/mtd/spi-nor/core.c     | 18 +++++++++++++-----
>  drivers/mtd/spi-nor/macronix.c |  3 +++
>  2 files changed, 16 insertions(+), 5 deletions(-)

For whole series:

Acked-by: Esben Haabendal <esben@geanix.com>

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Michael Walle]

[+ some people from MXIC as they are ones who posted to the ML
lately. Feel free to forward this mail to the corresponding people.]

Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
> Macronix engineers, in their infinite wisdom, have a habit of reusing
> JEDEC ids for different chips. There's already one
> workaround (MX25L25635F v MX25L25635E), but the same problem exists
> for MX25L3205D v MX25L3233F, the latter of which is not currently
> supported by linux.
> 
> AFAICT, that case cannot really be handled with any of the ->fixup
> machinery: The correct entry for the MX25L3233F would read
> 
>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
> 
> while the existing one is
> 
> 	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
> 
> So in spi_nor_init_params(), we won't even try reading the sfdp
> info (i.e. call spi_nor_sfdp_init_params), and hence
> spi_nor_post_sfdp_fixups() has no way of distinguishing the
> chips.
> 
> Replacing the existing entry with the mx25l3233f one to coerce the
> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
> because the data sheet for the mx25l3205d explicitly says not to issue
> any commands not listed ("It is not recommended to adopt any other
> code not in the command definition table, which will potentially enter
> the hidden mode.", whatever that means).

Maybe we should ask Macronix if it is safe to send the RDSFDP command.
Can anyone from MXIC comment this?
This is also interesting because we are discussing reading the SFDP
without reading the ID first.

Of course this will not save us from two different devices sharing
the same ID and both having no RDSFDP support.

> In order to support such cases, extend the logic in spi_nor_read_id()
> a little so that if we already have a struct flash_info* from the name
> in device tree, check the JEDEC bytes against that, and if it is a
> match, accept that (device tree compatible + matching JEDEC bytes) is
> stronger than merely matching JEDEC bytes.

This won't help much without a proper dt schema. No in-tree devicetree
could use is because the DT validation would complain. So if this will
go in (and the maintainers are rather hesitant to add it, I tried
it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
get an ack from Rob.

> This also makes initialization slightly faster in the common case
> where the flash_info was found from the name and the JEDEC bytes do
> match - it avoids a second linear search over all known chips.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

-michael

[1] 
https://lore.kernel.org/linux-mtd/20200103223423.14025-1-michael@walle.cc/

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Michael Walle]

[+ some people from MXIC as they are ones who posted to the ML
lately. Feel free to forward this mail to the corresponding people.]

Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
> Macronix engineers, in their infinite wisdom, have a habit of reusing
> JEDEC ids for different chips. There's already one
> workaround (MX25L25635F v MX25L25635E), but the same problem exists
> for MX25L3205D v MX25L3233F, the latter of which is not currently
> supported by linux.
> 
> AFAICT, that case cannot really be handled with any of the ->fixup
> machinery: The correct entry for the MX25L3233F would read
> 
>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
> 
> while the existing one is
> 
> 	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
> 
> So in spi_nor_init_params(), we won't even try reading the sfdp
> info (i.e. call spi_nor_sfdp_init_params), and hence
> spi_nor_post_sfdp_fixups() has no way of distinguishing the
> chips.
> 
> Replacing the existing entry with the mx25l3233f one to coerce the
> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
> because the data sheet for the mx25l3205d explicitly says not to issue
> any commands not listed ("It is not recommended to adopt any other
> code not in the command definition table, which will potentially enter
> the hidden mode.", whatever that means).

Maybe we should ask Macronix if it is safe to send the RDSFDP command.
Can anyone from MXIC comment this?
This is also interesting because we are discussing reading the SFDP
without reading the ID first.

Of course this will not save us from two different devices sharing
the same ID and both having no RDSFDP support.

> In order to support such cases, extend the logic in spi_nor_read_id()
> a little so that if we already have a struct flash_info* from the name
> in device tree, check the JEDEC bytes against that, and if it is a
> match, accept that (device tree compatible + matching JEDEC bytes) is
> stronger than merely matching JEDEC bytes.

This won't help much without a proper dt schema. No in-tree devicetree
could use is because the DT validation would complain. So if this will
go in (and the maintainers are rather hesitant to add it, I tried
it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
get an ack from Rob.

> This also makes initialization slightly faster in the common case
> where the flash_info was found from the name and the JEDEC bytes do
> match - it avoids a second linear search over all known chips.
> 
> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>

-michael

[1] 
https://lore.kernel.org/linux-mtd/20200103223423.14025-1-michael@walle.cc/

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Rasmus Villemoes]

On 22/06/2021 13.57, Michael Walle wrote:
> [+ some people from MXIC as they are ones who posted to the ML
> lately. Feel free to forward this mail to the corresponding people.]
> 
> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>> JEDEC ids for different chips. There's already one
>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>> supported by linux.
>>
>> AFAICT, that case cannot really be handled with any of the ->fixup
>> machinery: The correct entry for the MX25L3233F would read
>>
>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>
>> while the existing one is
>>
>>     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>
>> So in spi_nor_init_params(), we won't even try reading the sfdp
>> info (i.e. call spi_nor_sfdp_init_params), and hence
>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>> chips.
>>
>> Replacing the existing entry with the mx25l3233f one to coerce the
>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>> because the data sheet for the mx25l3205d explicitly says not to issue
>> any commands not listed ("It is not recommended to adopt any other
>> code not in the command definition table, which will potentially enter
>> the hidden mode.", whatever that means).
> 
> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
> Can anyone from MXIC comment this?

Yeah, that would be useful to know, but I don't have any hopes
whatsoever of Macronix engineers being able to help sort out the mess
they've created by reusing IDs in the first place. They don't seem to
understand how that can possibly be a problem.

I, and my client, have contacted them on several occasions to ask how
we're supposed to deal with that. At one point, the answer was
"MX25L3233F support Serial Flash Discoverable Parameters (SFDP) mode,
MX25L3205D does not support.", but when I asked the obvious follow-up
("but the MX25L3205D datasheet warns against doing RDSFDP or any other
not explicitly allowed command"), I got no response.

Another response was

"I can only comment on Linux 4.4, as that is the only version that I
have supporting material for. Basically we have a patch for MTD/SPI-NOR
(see attached). This is to allow allow the MTD subsystem to cope with
devices that have the same ID (see below first paragraph of application
note attached). Please note that the MX25L3205D had an EOL notification
on 14th May 2010."

and that attached patch is a 173KB .patch file that made me taste my
breakfast again.

And they keep repeating the argument that when a chip is EOL, it's OK to
reuse its ID (because obviously nobody have used that chip in a product
that would receive OS updates, so any OS released later than that EOL
date can just include support for the newer chip and drop the old one...).

>> In order to support such cases, extend the logic in spi_nor_read_id()
>> a little so that if we already have a struct flash_info* from the name
>> in device tree, check the JEDEC bytes against that, and if it is a
>> match, accept that (device tree compatible + matching JEDEC bytes) is
>> stronger than merely matching JEDEC bytes.
> 
> This won't help much without a proper dt schema. No in-tree devicetree
> could use is because the DT validation would complain.

I can certainly extend the regexp in jedec,spi-nor.yaml to match this
new one. DT is supposed to describe the hardware, so I can't see how
that could possibly be controversial.

 So if this will
> go in (and the maintainers are rather hesitant to add it, I tried
> it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
> get an ack from Rob.

Thanks for that link. So it seems this isn't the first time recycled IDs
have come up, and not just for Macronix.

Yes, vendors shouldn't recycle IDs. They do. They should be punished by
people not using those chips in new designs. Doesn't work, hardware
designers do use them. Auto-detection is preferred over using hard-coded
values from DT. Sure, absolutely, and when the ID is known to be
ambiguous, by all means throw in all the heuristics and chip-specific
quirks one can think of to disambiguate. But at the end of the day,
there are chips out there which cannot be distinguished without help
from DT, and as DT is supposed to describe the hardware, why is that
such a big problem?

And I'm not suggesting any change whatsoever as to how a compatible
string of merely "jedec,spi-nor" would be handled - it would just take
the first chip with the read JEDEC id, then apply any appropriate quirks
and fixups.

Rasmus

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Rasmus Villemoes]

On 22/06/2021 13.57, Michael Walle wrote:
> [+ some people from MXIC as they are ones who posted to the ML
> lately. Feel free to forward this mail to the corresponding people.]
> 
> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>> JEDEC ids for different chips. There's already one
>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>> supported by linux.
>>
>> AFAICT, that case cannot really be handled with any of the ->fixup
>> machinery: The correct entry for the MX25L3233F would read
>>
>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>
>> while the existing one is
>>
>>     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>
>> So in spi_nor_init_params(), we won't even try reading the sfdp
>> info (i.e. call spi_nor_sfdp_init_params), and hence
>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>> chips.
>>
>> Replacing the existing entry with the mx25l3233f one to coerce the
>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>> because the data sheet for the mx25l3205d explicitly says not to issue
>> any commands not listed ("It is not recommended to adopt any other
>> code not in the command definition table, which will potentially enter
>> the hidden mode.", whatever that means).
> 
> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
> Can anyone from MXIC comment this?

Yeah, that would be useful to know, but I don't have any hopes
whatsoever of Macronix engineers being able to help sort out the mess
they've created by reusing IDs in the first place. They don't seem to
understand how that can possibly be a problem.

I, and my client, have contacted them on several occasions to ask how
we're supposed to deal with that. At one point, the answer was
"MX25L3233F support Serial Flash Discoverable Parameters (SFDP) mode,
MX25L3205D does not support.", but when I asked the obvious follow-up
("but the MX25L3205D datasheet warns against doing RDSFDP or any other
not explicitly allowed command"), I got no response.

Another response was

"I can only comment on Linux 4.4, as that is the only version that I
have supporting material for. Basically we have a patch for MTD/SPI-NOR
(see attached). This is to allow allow the MTD subsystem to cope with
devices that have the same ID (see below first paragraph of application
note attached). Please note that the MX25L3205D had an EOL notification
on 14th May 2010."

and that attached patch is a 173KB .patch file that made me taste my
breakfast again.

And they keep repeating the argument that when a chip is EOL, it's OK to
reuse its ID (because obviously nobody have used that chip in a product
that would receive OS updates, so any OS released later than that EOL
date can just include support for the newer chip and drop the old one...).

>> In order to support such cases, extend the logic in spi_nor_read_id()
>> a little so that if we already have a struct flash_info* from the name
>> in device tree, check the JEDEC bytes against that, and if it is a
>> match, accept that (device tree compatible + matching JEDEC bytes) is
>> stronger than merely matching JEDEC bytes.
> 
> This won't help much without a proper dt schema. No in-tree devicetree
> could use is because the DT validation would complain.

I can certainly extend the regexp in jedec,spi-nor.yaml to match this
new one. DT is supposed to describe the hardware, so I can't see how
that could possibly be controversial.

 So if this will
> go in (and the maintainers are rather hesitant to add it, I tried
> it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
> get an ack from Rob.

Thanks for that link. So it seems this isn't the first time recycled IDs
have come up, and not just for Macronix.

Yes, vendors shouldn't recycle IDs. They do. They should be punished by
people not using those chips in new designs. Doesn't work, hardware
designers do use them. Auto-detection is preferred over using hard-coded
values from DT. Sure, absolutely, and when the ID is known to be
ambiguous, by all means throw in all the heuristics and chip-specific
quirks one can think of to disambiguate. But at the end of the day,
there are chips out there which cannot be distinguished without help
from DT, and as DT is supposed to describe the hardware, why is that
such a big problem?

And I'm not suggesting any change whatsoever as to how a compatible
string of merely "jedec,spi-nor" would be handled - it would just take
the first chip with the read JEDEC id, then apply any appropriate quirks
and fixups.

Rasmus

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Tudor.Ambarus]

Hi,

On 6/22/21 11:58 PM, Rasmus Villemoes wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 22/06/2021 13.57, Michael Walle wrote:
>> [+ some people from MXIC as they are ones who posted to the ML
>> lately. Feel free to forward this mail to the corresponding people.]
>>
>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>> JEDEC ids for different chips. There's already one
>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>> supported by linux.
>>>
>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>> machinery: The correct entry for the MX25L3233F would read
>>>
>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>
>>> while the existing one is
>>>
>>>     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>
>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>> chips.
>>>
>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>> any commands not listed ("It is not recommended to adopt any other
>>> code not in the command definition table, which will potentially enter
>>> the hidden mode.", whatever that means).
>>
>> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
>> Can anyone from MXIC comment this?
> 
> Yeah, that would be useful to know, but I don't have any hopes
> whatsoever of Macronix engineers being able to help sort out the mess
> they've created by reusing IDs in the first place. They don't seem to
> understand how that can possibly be a problem.
> 
> I, and my client, have contacted them on several occasions to ask how
> we're supposed to deal with that. At one point, the answer was
> "MX25L3233F support Serial Flash Discoverable Parameters (SFDP) mode,
> MX25L3205D does not support.", but when I asked the obvious follow-up
> ("but the MX25L3205D datasheet warns against doing RDSFDP or any other
> not explicitly allowed command"), I got no response.
> 
> Another response was
> 
> "I can only comment on Linux 4.4, as that is the only version that I
> have supporting material for. Basically we have a patch for MTD/SPI-NOR
> (see attached). This is to allow allow the MTD subsystem to cope with
> devices that have the same ID (see below first paragraph of application
> note attached). Please note that the MX25L3205D had an EOL notification
> on 14th May 2010."
> 
> and that attached patch is a 173KB .patch file that made me taste my
> breakfast again.
> 
> And they keep repeating the argument that when a chip is EOL, it's OK to
> reuse its ID (because obviously nobody have used that chip in a product
> that would receive OS updates, so any OS released later than that EOL
> date can just include support for the newer chip and drop the old one...).
> 
>>> In order to support such cases, extend the logic in spi_nor_read_id()
>>> a little so that if we already have a struct flash_info* from the name
>>> in device tree, check the JEDEC bytes against that, and if it is a
>>> match, accept that (device tree compatible + matching JEDEC bytes) is
>>> stronger than merely matching JEDEC bytes.
>>
>> This won't help much without a proper dt schema. No in-tree devicetree
>> could use is because the DT validation would complain.
> 
> I can certainly extend the regexp in jedec,spi-nor.yaml to match this
> new one. DT is supposed to describe the hardware, so I can't see how
> that could possibly be controversial.

No, please don't go that path yet.

> 
>  So if this will
>> go in (and the maintainers are rather hesitant to add it, I tried
>> it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
>> get an ack from Rob.

I'm not hesitant, I'm keeping my NACK until we're sure there isn't any other way
to differentiate at run-time. I've contacted a macronix representative, asking
if RDSFDP is harmful for MX25L3205D, let's wait for a few days. Maybe Zhengxun or
ycllin in cc can help too. Is there anyone here having a MX25L3205D?

Cheers,
ta

> 
> Thanks for that link. So it seems this isn't the first time recycled IDs
> have come up, and not just for Macronix.
> 
> Yes, vendors shouldn't recycle IDs. They do. They should be punished by
> people not using those chips in new designs. Doesn't work, hardware
> designers do use them. Auto-detection is preferred over using hard-coded
> values from DT. Sure, absolutely, and when the ID is known to be
> ambiguous, by all means throw in all the heuristics and chip-specific
> quirks one can think of to disambiguate. But at the end of the day,
> there are chips out there which cannot be distinguished without help
> from DT, and as DT is supposed to describe the hardware, why is that
> such a big problem?
> 
> And I'm not suggesting any change whatsoever as to how a compatible
> string of merely "jedec,spi-nor" would be handled - it would just take
> the first chip with the read JEDEC id, then apply any appropriate quirks
> and fixups.
> 
> Rasmus
> 

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Tudor.Ambarus]

Hi,

On 6/22/21 11:58 PM, Rasmus Villemoes wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 22/06/2021 13.57, Michael Walle wrote:
>> [+ some people from MXIC as they are ones who posted to the ML
>> lately. Feel free to forward this mail to the corresponding people.]
>>
>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>> JEDEC ids for different chips. There's already one
>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>> supported by linux.
>>>
>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>> machinery: The correct entry for the MX25L3233F would read
>>>
>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>
>>> while the existing one is
>>>
>>>     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>
>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>> chips.
>>>
>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>> any commands not listed ("It is not recommended to adopt any other
>>> code not in the command definition table, which will potentially enter
>>> the hidden mode.", whatever that means).
>>
>> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
>> Can anyone from MXIC comment this?
> 
> Yeah, that would be useful to know, but I don't have any hopes
> whatsoever of Macronix engineers being able to help sort out the mess
> they've created by reusing IDs in the first place. They don't seem to
> understand how that can possibly be a problem.
> 
> I, and my client, have contacted them on several occasions to ask how
> we're supposed to deal with that. At one point, the answer was
> "MX25L3233F support Serial Flash Discoverable Parameters (SFDP) mode,
> MX25L3205D does not support.", but when I asked the obvious follow-up
> ("but the MX25L3205D datasheet warns against doing RDSFDP or any other
> not explicitly allowed command"), I got no response.
> 
> Another response was
> 
> "I can only comment on Linux 4.4, as that is the only version that I
> have supporting material for. Basically we have a patch for MTD/SPI-NOR
> (see attached). This is to allow allow the MTD subsystem to cope with
> devices that have the same ID (see below first paragraph of application
> note attached). Please note that the MX25L3205D had an EOL notification
> on 14th May 2010."
> 
> and that attached patch is a 173KB .patch file that made me taste my
> breakfast again.
> 
> And they keep repeating the argument that when a chip is EOL, it's OK to
> reuse its ID (because obviously nobody have used that chip in a product
> that would receive OS updates, so any OS released later than that EOL
> date can just include support for the newer chip and drop the old one...).
> 
>>> In order to support such cases, extend the logic in spi_nor_read_id()
>>> a little so that if we already have a struct flash_info* from the name
>>> in device tree, check the JEDEC bytes against that, and if it is a
>>> match, accept that (device tree compatible + matching JEDEC bytes) is
>>> stronger than merely matching JEDEC bytes.
>>
>> This won't help much without a proper dt schema. No in-tree devicetree
>> could use is because the DT validation would complain.
> 
> I can certainly extend the regexp in jedec,spi-nor.yaml to match this
> new one. DT is supposed to describe the hardware, so I can't see how
> that could possibly be controversial.

No, please don't go that path yet.

> 
>  So if this will
>> go in (and the maintainers are rather hesitant to add it, I tried
>> it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
>> get an ack from Rob.

I'm not hesitant, I'm keeping my NACK until we're sure there isn't any other way
to differentiate at run-time. I've contacted a macronix representative, asking
if RDSFDP is harmful for MX25L3205D, let's wait for a few days. Maybe Zhengxun or
ycllin in cc can help too. Is there anyone here having a MX25L3205D?

Cheers,
ta

> 
> Thanks for that link. So it seems this isn't the first time recycled IDs
> have come up, and not just for Macronix.
> 
> Yes, vendors shouldn't recycle IDs. They do. They should be punished by
> people not using those chips in new designs. Doesn't work, hardware
> designers do use them. Auto-detection is preferred over using hard-coded
> values from DT. Sure, absolutely, and when the ID is known to be
> ambiguous, by all means throw in all the heuristics and chip-specific
> quirks one can think of to disambiguate. But at the end of the day,
> there are chips out there which cannot be distinguished without help
> from DT, and as DT is supposed to describe the hardware, why is that
> such a big problem?
> 
> And I'm not suggesting any change whatsoever as to how a compatible
> string of merely "jedec,spi-nor" would be handled - it would just take
> the first chip with the read JEDEC id, then apply any appropriate quirks
> and fixups.
> 
> Rasmus
> 


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: 回信: Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Tudor.Ambarus]

On 6/23/21 10:17 AM, jaimeliao@mxic.com.tw wrote:
> You don't often get email from jaimeliao@mxic.com.tw. Learn why this is important<http://aka.ms/LearnAboutSenderIdentification>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> Hi Michael and Rasmus
> 
> 
>>
>> Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already
>> found flash_info
>>
>> [+ some people from MXIC as they are ones who posted to the ML
>> lately. Feel free to forward this mail to the corresponding people.]
>>
>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>> JEDEC ids for different chips. There's already one
>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>> supported by linux.
>>>
>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>> machinery: The correct entry for the MX25L3233F would read
>>>
>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>
>>> while the existing one is
>>>
>>>    { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>
>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>> chips.
>>>
>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>> any commands not listed ("It is not recommended to adopt any other
>>> code not in the command definition table, which will potentially enter
>>> the hidden mode.", whatever that means).
>>
>> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
>> Can anyone from MXIC comment this?
> 
> For this part, we have validate RDSFDP command on some different chips which
> are not support SFDP mode. The conclusion is that Flash didn't reply anything.
> But we still not suggest to using commands which are not supported on the Flash.
> 
>> This is also interesting because we are discussing reading the SFDP
>> without reading the ID first.
> 
> I think this idea is good but I am not sure how other vendor chips will react
> after getting the commands which are not supported on the flash.

Do any of these flashed define an extended ID, i.e. more that 3 bytes of ID?
Rasmus, would you please try to read more bytes of ID?

Re: 回信: Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Tudor.Ambarus]

On 6/23/21 10:17 AM, jaimeliao@mxic.com.tw wrote:
> You don't often get email from jaimeliao@mxic.com.tw. Learn why this is important<http://aka.ms/LearnAboutSenderIdentification>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> Hi Michael and Rasmus
> 
> 
>>
>> Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already
>> found flash_info
>>
>> [+ some people from MXIC as they are ones who posted to the ML
>> lately. Feel free to forward this mail to the corresponding people.]
>>
>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>> JEDEC ids for different chips. There's already one
>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>> supported by linux.
>>>
>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>> machinery: The correct entry for the MX25L3233F would read
>>>
>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>
>>> while the existing one is
>>>
>>>    { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>
>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>> chips.
>>>
>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>> any commands not listed ("It is not recommended to adopt any other
>>> code not in the command definition table, which will potentially enter
>>> the hidden mode.", whatever that means).
>>
>> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
>> Can anyone from MXIC comment this?
> 
> For this part, we have validate RDSFDP command on some different chips which
> are not support SFDP mode. The conclusion is that Flash didn't reply anything.
> But we still not suggest to using commands which are not supported on the Flash.
> 
>> This is also interesting because we are discussing reading the SFDP
>> without reading the ID first.
> 
> I think this idea is good but I am not sure how other vendor chips will react
> after getting the commands which are not supported on the flash.

Do any of these flashed define an extended ID, i.e. more that 3 bytes of ID?
Rasmus, would you please try to read more bytes of ID?

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: 回信: Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Rasmus Villemoes]

On 23/06/2021 10.33, Tudor.Ambarus@microchip.com wrote:
> On 6/23/21 10:17 AM, jaimeliao@mxic.com.tw wrote:
>> You don't often get email from jaimeliao@mxic.com.tw. Learn why this is important<http://aka.ms/LearnAboutSenderIdentification>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> Hi Michael and Rasmus
>>
>>
>>>
>>> Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already
>>> found flash_info
>>>
>>> [+ some people from MXIC as they are ones who posted to the ML
>>> lately. Feel free to forward this mail to the corresponding people.]
>>>
>>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>>> JEDEC ids for different chips. There's already one
>>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>>> supported by linux.
>>>>
>>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>>> machinery: The correct entry for the MX25L3233F would read
>>>>
>>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>>
>>>> while the existing one is
>>>>
>>>>    { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>>
>>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>>> chips.
>>>>
>>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>>> any commands not listed ("It is not recommended to adopt any other
>>>> code not in the command definition table, which will potentially enter
>>>> the hidden mode.", whatever that means).
>>>

> Do any of these flashed define an extended ID, i.e. more that 3 bytes of ID?
> Rasmus, would you please try to read more bytes of ID?
> 

No, neither mx25l3205d nor mx25l3233f do according to their data sheets.

And when actually reading more bytes, one gets as expected just the c2
20 16 repeating all over.

Rasmus

Re: 回信: Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Rasmus Villemoes]

On 23/06/2021 10.33, Tudor.Ambarus@microchip.com wrote:
> On 6/23/21 10:17 AM, jaimeliao@mxic.com.tw wrote:
>> You don't often get email from jaimeliao@mxic.com.tw. Learn why this is important<http://aka.ms/LearnAboutSenderIdentification>
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> Hi Michael and Rasmus
>>
>>
>>>
>>> Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already
>>> found flash_info
>>>
>>> [+ some people from MXIC as they are ones who posted to the ML
>>> lately. Feel free to forward this mail to the corresponding people.]
>>>
>>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>>> JEDEC ids for different chips. There's already one
>>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>>> supported by linux.
>>>>
>>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>>> machinery: The correct entry for the MX25L3233F would read
>>>>
>>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>>
>>>> while the existing one is
>>>>
>>>>    { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>>
>>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>>> chips.
>>>>
>>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>>> any commands not listed ("It is not recommended to adopt any other
>>>> code not in the command definition table, which will potentially enter
>>>> the hidden mode.", whatever that means).
>>>

> Do any of these flashed define an extended ID, i.e. more that 3 bytes of ID?
> Rasmus, would you please try to read more bytes of ID?
> 

No, neither mx25l3205d nor mx25l3233f do according to their data sheets.

And when actually reading more bytes, one gets as expected just the c2
20 16 repeating all over.

Rasmus

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Rasmus Villemoes]

On 23/06/2021 08.46, Tudor.Ambarus@microchip.com wrote:
> Hi,
> 
> On 6/22/21 11:58 PM, Rasmus Villemoes wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 22/06/2021 13.57, Michael Walle wrote:
>>> [+ some people from MXIC as they are ones who posted to the ML
>>> lately. Feel free to forward this mail to the corresponding people.]
>>>
>>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>>> JEDEC ids for different chips. There's already one
>>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>>> supported by linux.
>>>>
>>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>>> machinery: The correct entry for the MX25L3233F would read
>>>>
>>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>>
>>>> while the existing one is
>>>>
>>>>     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>>
>>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>>> chips.
>>>>
>>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>>> any commands not listed ("It is not recommended to adopt any other
>>>> code not in the command definition table, which will potentially enter
>>>> the hidden mode.", whatever that means).
>>>
>>> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
>>> Can anyone from MXIC comment this?
>>
>> Yeah, that would be useful to know, but I don't have any hopes
>> whatsoever of Macronix engineers being able to help sort out the mess
>> they've created by reusing IDs in the first place. They don't seem to
>> understand how that can possibly be a problem.
>>
>> I, and my client, have contacted them on several occasions to ask how
>> we're supposed to deal with that. At one point, the answer was
>> "MX25L3233F support Serial Flash Discoverable Parameters (SFDP) mode,
>> MX25L3205D does not support.", but when I asked the obvious follow-up
>> ("but the MX25L3205D datasheet warns against doing RDSFDP or any other
>> not explicitly allowed command"), I got no response.
>>
>> Another response was
>>
>> "I can only comment on Linux 4.4, as that is the only version that I
>> have supporting material for. Basically we have a patch for MTD/SPI-NOR
>> (see attached). This is to allow allow the MTD subsystem to cope with
>> devices that have the same ID (see below first paragraph of application
>> note attached). Please note that the MX25L3205D had an EOL notification
>> on 14th May 2010."
>>
>> and that attached patch is a 173KB .patch file that made me taste my
>> breakfast again.
>>
>> And they keep repeating the argument that when a chip is EOL, it's OK to
>> reuse its ID (because obviously nobody have used that chip in a product
>> that would receive OS updates, so any OS released later than that EOL
>> date can just include support for the newer chip and drop the old one...).
>>
>>>> In order to support such cases, extend the logic in spi_nor_read_id()
>>>> a little so that if we already have a struct flash_info* from the name
>>>> in device tree, check the JEDEC bytes against that, and if it is a
>>>> match, accept that (device tree compatible + matching JEDEC bytes) is
>>>> stronger than merely matching JEDEC bytes.
>>>
>>> This won't help much without a proper dt schema. No in-tree devicetree
>>> could use is because the DT validation would complain.
>>
>> I can certainly extend the regexp in jedec,spi-nor.yaml to match this
>> new one. DT is supposed to describe the hardware, so I can't see how
>> that could possibly be controversial.
> 
> No, please don't go that path yet.
> 
>>
>>  So if this will
>>> go in (and the maintainers are rather hesitant to add it, I tried
>>> it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
>>> get an ack from Rob.
> 
> I'm not hesitant, I'm keeping my NACK until we're sure there isn't any other way
> to differentiate at run-time. 

It seems that we have established that by now, right?

Rasmus

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Rasmus Villemoes]

On 23/06/2021 08.46, Tudor.Ambarus@microchip.com wrote:
> Hi,
> 
> On 6/22/21 11:58 PM, Rasmus Villemoes wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 22/06/2021 13.57, Michael Walle wrote:
>>> [+ some people from MXIC as they are ones who posted to the ML
>>> lately. Feel free to forward this mail to the corresponding people.]
>>>
>>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>>> JEDEC ids for different chips. There's already one
>>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>>> supported by linux.
>>>>
>>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>>> machinery: The correct entry for the MX25L3233F would read
>>>>
>>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>>
>>>> while the existing one is
>>>>
>>>>     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>>
>>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>>> chips.
>>>>
>>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>>> any commands not listed ("It is not recommended to adopt any other
>>>> code not in the command definition table, which will potentially enter
>>>> the hidden mode.", whatever that means).
>>>
>>> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
>>> Can anyone from MXIC comment this?
>>
>> Yeah, that would be useful to know, but I don't have any hopes
>> whatsoever of Macronix engineers being able to help sort out the mess
>> they've created by reusing IDs in the first place. They don't seem to
>> understand how that can possibly be a problem.
>>
>> I, and my client, have contacted them on several occasions to ask how
>> we're supposed to deal with that. At one point, the answer was
>> "MX25L3233F support Serial Flash Discoverable Parameters (SFDP) mode,
>> MX25L3205D does not support.", but when I asked the obvious follow-up
>> ("but the MX25L3205D datasheet warns against doing RDSFDP or any other
>> not explicitly allowed command"), I got no response.
>>
>> Another response was
>>
>> "I can only comment on Linux 4.4, as that is the only version that I
>> have supporting material for. Basically we have a patch for MTD/SPI-NOR
>> (see attached). This is to allow allow the MTD subsystem to cope with
>> devices that have the same ID (see below first paragraph of application
>> note attached). Please note that the MX25L3205D had an EOL notification
>> on 14th May 2010."
>>
>> and that attached patch is a 173KB .patch file that made me taste my
>> breakfast again.
>>
>> And they keep repeating the argument that when a chip is EOL, it's OK to
>> reuse its ID (because obviously nobody have used that chip in a product
>> that would receive OS updates, so any OS released later than that EOL
>> date can just include support for the newer chip and drop the old one...).
>>
>>>> In order to support such cases, extend the logic in spi_nor_read_id()
>>>> a little so that if we already have a struct flash_info* from the name
>>>> in device tree, check the JEDEC bytes against that, and if it is a
>>>> match, accept that (device tree compatible + matching JEDEC bytes) is
>>>> stronger than merely matching JEDEC bytes.
>>>
>>> This won't help much without a proper dt schema. No in-tree devicetree
>>> could use is because the DT validation would complain.
>>
>> I can certainly extend the regexp in jedec,spi-nor.yaml to match this
>> new one. DT is supposed to describe the hardware, so I can't see how
>> that could possibly be controversial.
> 
> No, please don't go that path yet.
> 
>>
>>  So if this will
>>> go in (and the maintainers are rather hesitant to add it, I tried
>>> it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
>>> get an ack from Rob.
> 
> I'm not hesitant, I'm keeping my NACK until we're sure there isn't any other way
> to differentiate at run-time. 

It seems that we have established that by now, right?

Rasmus

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Tudor.Ambarus]

On 7/2/21 4:17 PM, Rasmus Villemoes wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 23/06/2021 08.46, Tudor.Ambarus@microchip.com wrote:
>> Hi,
>>
>> On 6/22/21 11:58 PM, Rasmus Villemoes wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 22/06/2021 13.57, Michael Walle wrote:
>>>> [+ some people from MXIC as they are ones who posted to the ML
>>>> lately. Feel free to forward this mail to the corresponding people.]
>>>>
>>>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>>>> JEDEC ids for different chips. There's already one
>>>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>>>> supported by linux.
>>>>>
>>>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>>>> machinery: The correct entry for the MX25L3233F would read
>>>>>
>>>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>>>
>>>>> while the existing one is
>>>>>
>>>>>     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>>>
>>>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>>>> chips.
>>>>>
>>>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>>>> any commands not listed ("It is not recommended to adopt any other
>>>>> code not in the command definition table, which will potentially enter
>>>>> the hidden mode.", whatever that means).
>>>>
>>>> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
>>>> Can anyone from MXIC comment this?
>>>
>>> Yeah, that would be useful to know, but I don't have any hopes
>>> whatsoever of Macronix engineers being able to help sort out the mess
>>> they've created by reusing IDs in the first place. They don't seem to
>>> understand how that can possibly be a problem.
>>>
>>> I, and my client, have contacted them on several occasions to ask how
>>> we're supposed to deal with that. At one point, the answer was
>>> "MX25L3233F support Serial Flash Discoverable Parameters (SFDP) mode,
>>> MX25L3205D does not support.", but when I asked the obvious follow-up
>>> ("but the MX25L3205D datasheet warns against doing RDSFDP or any other
>>> not explicitly allowed command"), I got no response.
>>>
>>> Another response was
>>>
>>> "I can only comment on Linux 4.4, as that is the only version that I
>>> have supporting material for. Basically we have a patch for MTD/SPI-NOR
>>> (see attached). This is to allow allow the MTD subsystem to cope with
>>> devices that have the same ID (see below first paragraph of application
>>> note attached). Please note that the MX25L3205D had an EOL notification
>>> on 14th May 2010."
>>>
>>> and that attached patch is a 173KB .patch file that made me taste my
>>> breakfast again.
>>>
>>> And they keep repeating the argument that when a chip is EOL, it's OK to
>>> reuse its ID (because obviously nobody have used that chip in a product
>>> that would receive OS updates, so any OS released later than that EOL
>>> date can just include support for the newer chip and drop the old one...).
>>>
>>>>> In order to support such cases, extend the logic in spi_nor_read_id()
>>>>> a little so that if we already have a struct flash_info* from the name
>>>>> in device tree, check the JEDEC bytes against that, and if it is a
>>>>> match, accept that (device tree compatible + matching JEDEC bytes) is
>>>>> stronger than merely matching JEDEC bytes.
>>>>
>>>> This won't help much without a proper dt schema. No in-tree devicetree
>>>> could use is because the DT validation would complain.
>>>
>>> I can certainly extend the regexp in jedec,spi-nor.yaml to match this
>>> new one. DT is supposed to describe the hardware, so I can't see how
>>> that could possibly be controversial.
>>
>> No, please don't go that path yet.
>>
>>>
>>>  So if this will
>>>> go in (and the maintainers are rather hesitant to add it, I tried
>>>> it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
>>>> get an ack from Rob.
>>
>> I'm not hesitant, I'm keeping my NACK until we're sure there isn't any other way
>> to differentiate at run-time.
> 
> It seems that we have established that by now, right?
> 

It is unlikely that RDSFDP will cause any problems for the old MX25L3205D,
I would like to differentiate between the two flashes by parsing SFDP.

I'm preparing a patch set to address the all the ID collision thingy.
https://github.com/ambarus/linux-0day/commit/b760260efecb4f3678de3b78250f99338ecbad1b

Re: [RFC 2/3] mtd: spi-nor: core: compare JEDEC bytes to already found flash_info

[Tudor.Ambarus]

On 7/2/21 4:17 PM, Rasmus Villemoes wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 23/06/2021 08.46, Tudor.Ambarus@microchip.com wrote:
>> Hi,
>>
>> On 6/22/21 11:58 PM, Rasmus Villemoes wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 22/06/2021 13.57, Michael Walle wrote:
>>>> [+ some people from MXIC as they are ones who posted to the ML
>>>> lately. Feel free to forward this mail to the corresponding people.]
>>>>
>>>> Am 2021-06-21 17:23, schrieb Rasmus Villemoes:
>>>>> Macronix engineers, in their infinite wisdom, have a habit of reusing
>>>>> JEDEC ids for different chips. There's already one
>>>>> workaround (MX25L25635F v MX25L25635E), but the same problem exists
>>>>> for MX25L3205D v MX25L3233F, the latter of which is not currently
>>>>> supported by linux.
>>>>>
>>>>> AFAICT, that case cannot really be handled with any of the ->fixup
>>>>> machinery: The correct entry for the MX25L3233F would read
>>>>>
>>>>>         { "mx25l3233f",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K |
>>>>> SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ ) },
>>>>>
>>>>> while the existing one is
>>>>>
>>>>>     { "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SECT_4K) },
>>>>>
>>>>> So in spi_nor_init_params(), we won't even try reading the sfdp
>>>>> info (i.e. call spi_nor_sfdp_init_params), and hence
>>>>> spi_nor_post_sfdp_fixups() has no way of distinguishing the
>>>>> chips.
>>>>>
>>>>> Replacing the existing entry with the mx25l3233f one to coerce the
>>>>> core into issuing the SPINOR_OP_RDSFDP is also not really an option,
>>>>> because the data sheet for the mx25l3205d explicitly says not to issue
>>>>> any commands not listed ("It is not recommended to adopt any other
>>>>> code not in the command definition table, which will potentially enter
>>>>> the hidden mode.", whatever that means).
>>>>
>>>> Maybe we should ask Macronix if it is safe to send the RDSFDP command.
>>>> Can anyone from MXIC comment this?
>>>
>>> Yeah, that would be useful to know, but I don't have any hopes
>>> whatsoever of Macronix engineers being able to help sort out the mess
>>> they've created by reusing IDs in the first place. They don't seem to
>>> understand how that can possibly be a problem.
>>>
>>> I, and my client, have contacted them on several occasions to ask how
>>> we're supposed to deal with that. At one point, the answer was
>>> "MX25L3233F support Serial Flash Discoverable Parameters (SFDP) mode,
>>> MX25L3205D does not support.", but when I asked the obvious follow-up
>>> ("but the MX25L3205D datasheet warns against doing RDSFDP or any other
>>> not explicitly allowed command"), I got no response.
>>>
>>> Another response was
>>>
>>> "I can only comment on Linux 4.4, as that is the only version that I
>>> have supporting material for. Basically we have a patch for MTD/SPI-NOR
>>> (see attached). This is to allow allow the MTD subsystem to cope with
>>> devices that have the same ID (see below first paragraph of application
>>> note attached). Please note that the MX25L3205D had an EOL notification
>>> on 14th May 2010."
>>>
>>> and that attached patch is a 173KB .patch file that made me taste my
>>> breakfast again.
>>>
>>> And they keep repeating the argument that when a chip is EOL, it's OK to
>>> reuse its ID (because obviously nobody have used that chip in a product
>>> that would receive OS updates, so any OS released later than that EOL
>>> date can just include support for the newer chip and drop the old one...).
>>>
>>>>> In order to support such cases, extend the logic in spi_nor_read_id()
>>>>> a little so that if we already have a struct flash_info* from the name
>>>>> in device tree, check the JEDEC bytes against that, and if it is a
>>>>> match, accept that (device tree compatible + matching JEDEC bytes) is
>>>>> stronger than merely matching JEDEC bytes.
>>>>
>>>> This won't help much without a proper dt schema. No in-tree devicetree
>>>> could use is because the DT validation would complain.
>>>
>>> I can certainly extend the regexp in jedec,spi-nor.yaml to match this
>>> new one. DT is supposed to describe the hardware, so I can't see how
>>> that could possibly be controversial.
>>
>> No, please don't go that path yet.
>>
>>>
>>>  So if this will
>>>> go in (and the maintainers are rather hesitant to add it, I tried
>>>> it myself [1]), you'd also need to add it to jedec,spi-nor.yaml and
>>>> get an ack from Rob.
>>
>> I'm not hesitant, I'm keeping my NACK until we're sure there isn't any other way
>> to differentiate at run-time.
> 
> It seems that we have established that by now, right?
> 

It is unlikely that RDSFDP will cause any problems for the old MX25L3205D,
I would like to differentiate between the two flashes by parsing SFDP.

I'm preparing a patch set to address the all the ID collision thingy.
https://github.com/ambarus/linux-0day/commit/b760260efecb4f3678de3b78250f99338ecbad1b


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/