[PATCH 0/7] mtd: spi-nor: Handle flash ID collisions

[PATCH 0/7] mtd: spi-nor: Handle flash ID collisions

[Tudor Ambarus]

I've started working on the flash ID collisions problems, and also on the
read SFDP first idea, to stop the spagetti init of params. I see the
ID collision thing is a hot topic, so I post the first part of the work,
to gather comments. Entire series is just compile tested.

Introduce the manuf-id-collisions driver in order to address ID collisions
between manufacturers. manuf-id-collisions will be the place to add new
flash additions, where the manufacturer is ignorant enough, to not
implement the ID continuation scheme. flash_info entries will be added in
a first come, first served manner. Differentiation between flashes will be
done at runtime if possible. Where runtime differentiation is not possible,
new compatibles will be introduced, but this will be done as a last resort.
Every new flash addition that define the SFDP tables, should dump its SFDP
tables in the patch's comment section below the --- line, so that we can
reference it in case of collisions.

I can't dump the SFDP tables for the new flash additions, I'll need
support in case someone wants to add them.

Tudor Ambarus (7):
  mtd: spi-nor: core: Introduce SPI_NOR_PARSE_SFDP
  mtd: spi-nor: core: Report correct name in case of ID collisions
  mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and
    MX25L3205D
  mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and
    MX25L12835F
  mtd: spi-nor: Introduce Manufacturer ID collisions driver
  mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b
  mtd: spi-nor: manuf-id-collisions: Add support for xm25qh64c

 drivers/mtd/spi-nor/Makefile              |  1 +
 drivers/mtd/spi-nor/core.c                |  9 ++++-
 drivers/mtd/spi-nor/core.h                |  5 +++
 drivers/mtd/spi-nor/macronix.c            | 49 +++++++++++++++++++++--
 drivers/mtd/spi-nor/manuf-id-collisions.c | 24 +++++++++++
 include/linux/mtd/spi-nor.h               |  2 +
 6 files changed, 85 insertions(+), 5 deletions(-)
 create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c

-- 
2.25.1


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

[PATCH 1/7] mtd: spi-nor: core: Introduce SPI_NOR_PARSE_SFDP

[Tudor Ambarus]

SPI NOR flashes that statically declare one of the
SPI_NOR_{DUAL, QUAD, OCTAL, OCTAL_DTR}_READ flags, and do not support
the RDSFDP command, are gratuiously receiving the RDSFDP command,
in the attempt of parsing the SFDP tables. It is not desirable to issue
comands that are not supported, so introduce a flag to help on this
situation.

New flash additions that support the SFDP standard should be declared
using SPI_NOR_PARSE_SFDP. Support that can be discovered when parsing
SFDP should not be duplicated by explicit flags at flash declaration.
All the flash parameters will be discovered when parsing SFDP.
Sometimes manufacturers wrongly define some fields in the SFDP tables.
If that's the case, SFDP data can be ammended with the fixups() hooks.
It is not common, but if the SFDP tables are entirely wrong, and it
does not worth the hassle to tweak the SFDP parameters by using the
fixups hooks, or if the flash does not define the SFDP tables at all,
then statically init the flash with the SPI_NOR_SKIP_SFDP flag and
specify the reset of flash capabilities with the flash info flags.

With time, we want to convert all flashes to SPI_NOR_PARSE_SFDP and
stop triggering the SFDP parsing with the
SPI_NOR_{DUAL, QUAD, OCTAL*}_READ flags. Getting rid of the
SPI_NOR_{OCTAL, OCTAL_DTR}_READ trigger is easy achievable, the reset
are a long term goal.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c | 3 ++-
 drivers/mtd/spi-nor/core.h | 4 ++++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index cc08bd707378..3d9f3698fb7b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2726,7 +2726,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
 
 	spi_nor_manufacturer_init_params(nor);
 
-	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+	if ((nor->info->flags & (SPI_NOR_PARSE_SFDP |
+				 SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 				 SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
 	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
 		spi_nor_sfdp_init_params(nor);
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 3348e1dd1445..55fceb0ec894 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -382,6 +382,10 @@ struct flash_info {
 					 * protection bits. Usually these will
 					 * power-up in a write-protected state.
 					 */
+#define SPI_NOR_PARSE_SFDP	BIT(23) /*
+					 * Flash initialized based on the SFDP
+					 * tables.
+					 */
 
 	const struct spi_nor_otp_organization otp_org;
 
-- 
2.25.1


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

[PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

[Tudor Ambarus]

Provide a way to report the correct flash name in case of ID collisions.
There will be a single flash_info entry when flash IDs collide, and the
differentiation between the flash types will be made at runtime
if possible.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c  | 5 ++++-
 include/linux/mtd/spi-nor.h | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 3d9f3698fb7b..d528e28995c6 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -3208,7 +3208,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	/* Configure OTP parameters and ops */
 	spi_nor_otp_init(nor);
 
-	dev_info(dev, "%s (%lld Kbytes)\n", info->name,
+	if (!nor->name)
+		nor->name = info->name;
+
+	dev_info(dev, "%s (%lld Kbytes)\n", nor->name,
 			(long long)mtd->size >> 10);
 
 	dev_dbg(dev,
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index f67457748ed8..bd92acdd1899 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -369,6 +369,7 @@ struct spi_nor_flash_parameter;
  * @bouncebuf:		bounce buffer used when the buffer passed by the MTD
  *                      layer is not DMA-able
  * @bouncebuf_size:	size of the bounce buffer
+ * @name:		used to point to correct name in case of ID collisions.
  * @info:		SPI NOR part JEDEC MFR ID and other info
  * @manufacturer:	SPI NOR manufacturer
  * @page_size:		the page size of the SPI NOR
@@ -399,6 +400,7 @@ struct spi_nor {
 	struct spi_mem		*spimem;
 	u8			*bouncebuf;
 	size_t			bouncebuf_size;
+	const char *name;
 	const struct flash_info	*info;
 	const struct spi_nor_manufacturer *manufacturer;
 	u32			page_size;
-- 
2.25.1


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

[PATCH 3/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Tudor Ambarus]

Macronix has a bad habbit of reusing flash IDs. While MX25L3233F supports
RDSFDP opcode, MX25L3205D does not support it and does not recommend
issuing opcodes that are not supported ("It is not recommended to adopt
any other code not in the command definition table, which will potentially
enter the hidden mode.").

We tested the RDSFDP on the MX25L3205D and the conclusion is that the
flash didn't reply anything. Given that it is unlikely that RDSFDP will
cause any problems for the old MX25L3205D, differentiate between the two
flashes by parsing SFDP.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/macronix.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 27498ed0cc0d..83a097708949 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -8,6 +8,26 @@
 
 #include "core.h"
 
+static const char *mx25l3233f = "mx25l3233f";
+
+static int mx25l3233f_post_bfpt_fixups(struct spi_nor *nor,
+				const struct sfdp_parameter_header *bfpt_header,
+				const struct sfdp_bfpt *bfpt)
+{
+	/*
+	 * Macronix has a bad habit of reusing flash IDs: MX25L3233F collides
+	 * with MX25L3205D. MX25L3233F defines SFDP tables, while the older
+	 * variant does not.
+	 */
+	nor->name = mx25l3233f;
+
+	return 0;
+}
+
+static struct spi_nor_fixups mx25l3233f_fixups = {
+	.post_bfpt = mx25l3233f_post_bfpt_fixups,
+};
+
 static int
 mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
 			    const struct sfdp_parameter_header *bfpt_header,
@@ -39,7 +59,9 @@ static const struct flash_info macronix_parts[] = {
 	{ "mx25l4005a",  INFO(0xc22013, 0, 64 * 1024,   8, SECT_4K) },
 	{ "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) },
+	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SPI_NOR_PARSE_SFDP |
+			      SECT_4K)
+		.fixups = &mx25l3233f_fixups },
 	{ "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.25.1


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

[PATCH 4/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Tudor Ambarus]

MX25L12835F define SFDP, while MX25L12805D does not.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/macronix.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 83a097708949..38701347bf04 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -8,6 +8,26 @@
 
 #include "core.h"
 
+static const char *mx25l12835f = "mx25l12835f";
+
+static int mx25l12835f_post_bfpt_fixups(struct spi_nor *nor,
+				const struct sfdp_parameter_header *bfpt_header,
+				const struct sfdp_bfpt *bfpt)
+{
+	/*
+	 * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides
+	 * with MX25L12805D. MX25L12835F defines SFDP tables, while the older
+	 * variant does not.
+	 */
+	nor->name = mx25l12835f;
+
+	return 0;
+}
+
+static struct spi_nor_fixups mx25l12835f_fixups = {
+	.post_bfpt = mx25l12835f_post_bfpt_fixups,
+};
+
 static const char *mx25l3233f = "mx25l3233f";
 
 static int mx25l3233f_post_bfpt_fixups(struct spi_nor *nor,
@@ -71,8 +91,9 @@ static const struct flash_info macronix_parts[] = {
 	{ "mx25u4035",   INFO(0xc22533, 0, 64 * 1024,   8, SECT_4K) },
 	{ "mx25u8035",   INFO(0xc22534, 0, 64 * 1024,  16, SECT_4K) },
 	{ "mx25u6435f",  INFO(0xc22537, 0, 64 * 1024, 128, SECT_4K) },
-	{ "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SECT_4K |
-			      SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) },
+	{ "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP |
+			      SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)
+		.fixups = &mx25l12835f_fixups },
 	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
 	{ "mx25r1635f",  INFO(0xc22815, 0, 64 * 1024,  32,
 			      SECT_4K | SPI_NOR_DUAL_READ |
-- 
2.25.1


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

[PATCH 5/7] mtd: spi-nor: Introduce Manufacturer ID collisions driver

[Tudor Ambarus]

Some manufacturers completely ignore the manufacturer's identification code
standard (JEP106) and do not define the manufacturer ID continuation
scheme. This will result in manufacturer ID collisions.

An an example, JEP106BA requires Boya that it's manufacturer ID to be
preceded by 8 continuation codes. Boya's identification code must be:
0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x68. But Boya ignores the
continuation scheme and its ID collides with the manufacturer defined in
bank one: Convex Computer.

Introduce the manuf-id-collisions driver in order to address ID collisions
between manufacturers. flash_info entries will be added in a first come,
first served manner. Differentiation between flashes will be done at
runtime if possible. Where runtime differentiation is not possible, new
compatibles will be introduced, but this will be done as a last resort.
Every new flash addition that define the SFDP tables, should dump its SFDP
tables in the patch's comment section below the --- line, so that we can
reference it in case of collisions.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/Makefile              |  1 +
 drivers/mtd/spi-nor/core.c                |  1 +
 drivers/mtd/spi-nor/core.h                |  1 +
 drivers/mtd/spi-nor/manuf-id-collisions.c | 17 +++++++++++++++++
 4 files changed, 20 insertions(+)
 create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c

diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
index 6b904e439372..48763d10daad 100644
--- a/drivers/mtd/spi-nor/Makefile
+++ b/drivers/mtd/spi-nor/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 spi-nor-objs			:= core.o sfdp.o swp.o otp.o sysfs.o
+spi-nor-objs			+= manuf-id-collisions.o
 spi-nor-objs			+= atmel.o
 spi-nor-objs			+= catalyst.o
 spi-nor-objs			+= eon.o
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index d528e28995c6..206a54c20fef 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -1829,6 +1829,7 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor)
 }
 
 static const struct spi_nor_manufacturer *manufacturers[] = {
+	&spi_nor_manuf_id_collisions,
 	&spi_nor_atmel,
 	&spi_nor_catalyst,
 	&spi_nor_eon,
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 55fceb0ec894..e9896cd60695 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -476,6 +476,7 @@ struct sfdp {
 };
 
 /* Manufacturer drivers. */
+extern const struct spi_nor_manufacturer spi_nor_manuf_id_collisions;
 extern const struct spi_nor_manufacturer spi_nor_atmel;
 extern const struct spi_nor_manufacturer spi_nor_catalyst;
 extern const struct spi_nor_manufacturer spi_nor_eon;
diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c b/drivers/mtd/spi-nor/manuf-id-collisions.c
new file mode 100644
index 000000000000..9efcba9d18a0
--- /dev/null
+++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/mtd/spi-nor.h>
+#include "core.h"
+
+static const struct flash_info id_collision_parts[] = {
+	/* Boya */
+	{ "by25q128as", INFO(0x684018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP |
+			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
+			     SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
+};
+
+const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = {
+	.name = "manufacturer ID collisions",
+	.parts = id_collision_parts,
+	.nparts = ARRAY_SIZE(id_collision_parts),
+};
-- 
2.25.1


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

[PATCH 6/7] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Tudor Ambarus]

Flash does not support continuation codes and may collide with a flash
of other manufacturer, Intersil being an example .

This flash addition is just for demonstration purposes. As I don't
have the flash, I assumed that it supports SFDP.
If the flash does not define the SFDP tables, it should be statically
initialized with:
        /* XTX (XTX Technology Limited) */
        { "XT25F128B", INFO(0x0b4018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP |
                            SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
                            SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
};

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/manuf-id-collisions.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c b/drivers/mtd/spi-nor/manuf-id-collisions.c
index 9efcba9d18a0..c473c7d40d09 100644
--- a/drivers/mtd/spi-nor/manuf-id-collisions.c
+++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
@@ -8,6 +8,10 @@ static const struct flash_info id_collision_parts[] = {
 	{ "by25q128as", INFO(0x684018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP |
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			     SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
+
+	/* XTX (XTX Technology Limited) */
+	{ "xt25f128b", INFO(0x0b4018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP |
+			    SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
 };
 
 const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = {
-- 
2.25.1


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

[PATCH 7/7] mtd: spi-nor: manuf-id-collisions: Add support for xm25qh64c

[Tudor Ambarus]

Do not apply! SFDP needs to be dumped.
mw:
'''
NB. XMC ignores the continuation codes and this particular device will
collide with M25PE64/M45PE64. Although I couldn't find any datasheet,
so I don't know if these devices actually exist.
'''

The data sheets can be found here:
http://xmcwh.com/Uploads/2020-12-17/XM25QH64C_Ver1.1.pdf

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/manuf-id-collisions.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c b/drivers/mtd/spi-nor/manuf-id-collisions.c
index c473c7d40d09..c41d6e8f2f95 100644
--- a/drivers/mtd/spi-nor/manuf-id-collisions.c
+++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
@@ -9,6 +9,9 @@ static const struct flash_info id_collision_parts[] = {
 			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
 			     SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
 
+	/* XMC (Wuhan Xinxin Semiconductor Manufacturing Corp.) */
+	{ "xm25qh64c", INFO(0x204017, 0, 64 * 1024, 128, SPI_NOR_PARSE_SFDP) },
+
 	/* XTX (XTX Technology Limited) */
 	{ "xt25f128b", INFO(0x0b4018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP |
 			    SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
-- 
2.25.1


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

Re: [PATCH 1/7] mtd: spi-nor: core: Introduce SPI_NOR_PARSE_SFDP

[Heiko Thiery]

Hi Tudor,

Am Fr., 2. Juli 2021 um 16:41 Uhr schrieb Tudor Ambarus
<tudor.ambarus@microchip.com>:
>
> SPI NOR flashes that statically declare one of the
> SPI_NOR_{DUAL, QUAD, OCTAL, OCTAL_DTR}_READ flags, and do not support
> the RDSFDP command, are gratuiously receiving the RDSFDP command,
> in the attempt of parsing the SFDP tables. It is not desirable to issue
> comands that are not supported, so introduce a flag to help on this
> situation.
>
> New flash additions that support the SFDP standard should be declared
> using SPI_NOR_PARSE_SFDP. Support that can be discovered when parsing
> SFDP should not be duplicated by explicit flags at flash declaration.
> All the flash parameters will be discovered when parsing SFDP.
> Sometimes manufacturers wrongly define some fields in the SFDP tables.
> If that's the case, SFDP data can be ammended with the fixups() hooks.
> It is not common, but if the SFDP tables are entirely wrong, and it
> does not worth the hassle to tweak the SFDP parameters by using the
> fixups hooks, or if the flash does not define the SFDP tables at all,
> then statically init the flash with the SPI_NOR_SKIP_SFDP flag and
> specify the reset of flash capabilities with the flash info flags.
>
> With time, we want to convert all flashes to SPI_NOR_PARSE_SFDP and
> stop triggering the SFDP parsing with the
> SPI_NOR_{DUAL, QUAD, OCTAL*}_READ flags. Getting rid of the
> SPI_NOR_{OCTAL, OCTAL_DTR}_READ trigger is easy achievable, the reset
> are a long term goal.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Reviewed-by: Heiko Thiery <heiko.thiery@gmail.com>

> ---
>  drivers/mtd/spi-nor/core.c | 3 ++-
>  drivers/mtd/spi-nor/core.h | 4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index cc08bd707378..3d9f3698fb7b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2726,7 +2726,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
>
>         spi_nor_manufacturer_init_params(nor);
>
> -       if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +       if ((nor->info->flags & (SPI_NOR_PARSE_SFDP |
> +                                SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>                                  SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
>             !(nor->info->flags & SPI_NOR_SKIP_SFDP))
>                 spi_nor_sfdp_init_params(nor);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 3348e1dd1445..55fceb0ec894 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -382,6 +382,10 @@ struct flash_info {
>                                          * protection bits. Usually these will
>                                          * power-up in a write-protected state.
>                                          */
> +#define SPI_NOR_PARSE_SFDP     BIT(23) /*
> +                                        * Flash initialized based on the SFDP
> +                                        * tables.
> +                                        */
>
>         const struct spi_nor_otp_organization otp_org;
>

Thank you

--
Heiko

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

Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

[Heiko Thiery]

Hi Tudor,

Am Fr., 2. Juli 2021 um 16:41 Uhr schrieb Tudor Ambarus
<tudor.ambarus@microchip.com>:
>
> Provide a way to report the correct flash name in case of ID collisions.
> There will be a single flash_info entry when flash IDs collide, and the
> differentiation between the flash types will be made at runtime
> if possible.
>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Reviewed-by: Heiko Thiery <heiko.thiery@gmail.com>

> ---
>  drivers/mtd/spi-nor/core.c  | 5 ++++-
>  include/linux/mtd/spi-nor.h | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 3d9f3698fb7b..d528e28995c6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3208,7 +3208,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>         /* Configure OTP parameters and ops */
>         spi_nor_otp_init(nor);
>
> -       dev_info(dev, "%s (%lld Kbytes)\n", info->name,
> +       if (!nor->name)
> +               nor->name = info->name;
> +
> +       dev_info(dev, "%s (%lld Kbytes)\n", nor->name,
>                         (long long)mtd->size >> 10);
>
>         dev_dbg(dev,
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index f67457748ed8..bd92acdd1899 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -369,6 +369,7 @@ struct spi_nor_flash_parameter;
>   * @bouncebuf:         bounce buffer used when the buffer passed by the MTD
>   *                      layer is not DMA-able
>   * @bouncebuf_size:    size of the bounce buffer
> + * @name:              used to point to correct name in case of ID collisions.
>   * @info:              SPI NOR part JEDEC MFR ID and other info
>   * @manufacturer:      SPI NOR manufacturer
>   * @page_size:         the page size of the SPI NOR
> @@ -399,6 +400,7 @@ struct spi_nor {
>         struct spi_mem          *spimem;
>         u8                      *bouncebuf;
>         size_t                  bouncebuf_size;
> +       const char *name;
>         const struct flash_info *info;
>         const struct spi_nor_manufacturer *manufacturer;
>         u32                     page_size;

Thank you


--
Heiko

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

Re: [PATCH 4/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Heiko Thiery]

Hi Tudor,

Am Fr., 2. Juli 2021 um 16:41 Uhr schrieb Tudor Ambarus
<tudor.ambarus@microchip.com>:
>
> MX25L12835F define SFDP, while MX25L12805D does not.

I tested this on the kontron pitx-imx8m board with a MX25L12835F equipped.

[    1.213448] spi-nor spi0.0: mx25l12835f (16384 Kbytes)

>
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Testd-by: Heiko Thiery <heiko.thiery@gmail.com>

> ---
>  drivers/mtd/spi-nor/macronix.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 83a097708949..38701347bf04 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -8,6 +8,26 @@
>
>  #include "core.h"
>
> +static const char *mx25l12835f = "mx25l12835f";
> +
> +static int mx25l12835f_post_bfpt_fixups(struct spi_nor *nor,
> +                               const struct sfdp_parameter_header *bfpt_header,
> +                               const struct sfdp_bfpt *bfpt)
> +{
> +       /*
> +        * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides
> +        * with MX25L12805D. MX25L12835F defines SFDP tables, while the older
> +        * variant does not.
> +        */
> +       nor->name = mx25l12835f;
> +
> +       return 0;
> +}
> +
> +static struct spi_nor_fixups mx25l12835f_fixups = {
> +       .post_bfpt = mx25l12835f_post_bfpt_fixups,
> +};
> +
>  static const char *mx25l3233f = "mx25l3233f";
>
>  static int mx25l3233f_post_bfpt_fixups(struct spi_nor *nor,
> @@ -71,8 +91,9 @@ static const struct flash_info macronix_parts[] = {
>         { "mx25u4035",   INFO(0xc22533, 0, 64 * 1024,   8, SECT_4K) },
>         { "mx25u8035",   INFO(0xc22534, 0, 64 * 1024,  16, SECT_4K) },
>         { "mx25u6435f",  INFO(0xc22537, 0, 64 * 1024, 128, SECT_4K) },
> -       { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SECT_4K |
> -                             SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) },
> +       { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP |
> +                             SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)
> +               .fixups = &mx25l12835f_fixups },
>         { "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
>         { "mx25r1635f",  INFO(0xc22815, 0, 64 * 1024,  32,
>                               SECT_4K | SPI_NOR_DUAL_READ |

Thank you


--
Heiko

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

Re: [PATCH 4/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Tudor.Ambarus]

On 7/2/21 5:41 PM, Tudor Ambarus wrote:
> +	{ "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP |
> +			      SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)

I should have removed SECT_4K, since 4k erase should be set when parsing SFDP.
If SFDP is wrong, we can amend it in the post_bfpt hook.

Heiko, can you test 4k erase with SECT_4K removed? Also, would you please dump the
SFDP tables using Michael's sysfs patches? They were applied. Every new flash addition
should have the SFDP tables dumped in the patch's comment section, under the --- line.
I don't have the flash, so I'll need your help.

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

Re: [PATCH 4/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Heiko Thiery]

Hi Tudor,

Am Mo., 5. Juli 2021 um 09:51 Uhr schrieb <Tudor.Ambarus@microchip.com>:
>
> On 7/2/21 5:41 PM, Tudor Ambarus wrote:
> > +     { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP |
> > +                           SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)
>
> I should have removed SECT_4K, since 4k erase should be set when parsing SFDP.
> If SFDP is wrong, we can amend it in the post_bfpt hook.

But what about the 4K support for the "old" mx25l12805d that does not
support reading SFDP? As far as I understand, the post_bfpt hook is
only called when the SFDP was successfully read and this would mean
the "old" one would never get this setting.

> Heiko, can you test 4k erase with SECT_4K removed? Also, would you please dump the
> SFDP tables using Michael's sysfs patches? They were applied. Every new flash addition
> should have the SFDP tables dumped in the patch's comment section, under the --- line.
> I don't have the flash, so I'll need your help.

Nevertheless I removed the SECT_4K entry from the info table. After
that the correct value still was set due to the parsing of the SFDP
data.
# cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0
.0/mtd/mtd0/erasesize
4096

And here is the requested dump:

# cat /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0
.0/spi-nor/sfdp | xxd -p
53464450000101ff00000109300000ffc2000104600000ffffffffffffff
ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
ffffffffffff003600279df9c06485cbffffffffffff

Thanks
-- 
Heiko

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

Re: [PATCH 4/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Michael Walle]

Hi,

Am 2021-07-05 12:45, schrieb Heiko Thiery:
> # cat 
> /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0
> .0/spi-nor/sfdp | xxd -p
> 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
> ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
> 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
> ffffffffffff003600279df9c06485cbffffffffffff

Ok, I like this output esp, because you can reverse it with
"xxd -r -p". Maybe we should also request an md5sum just to
be sure we have the same binary.

so the requested commands would be something like:

# xxd -p /path/to/sfdp
# md5sum /path/to/sfdp
# cat /path/to/jedec_id
# cat /path/to/partname
# cat /path/to/manufacturer

-michael

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

Re: [PATCH 4/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Heiko Thiery]

Hi Michael,

Am Mo., 5. Juli 2021 um 12:59 Uhr schrieb Michael Walle <michael@walle.cc>:
>
> Hi,
>
> Am 2021-07-05 12:45, schrieb Heiko Thiery:
> > # cat
> > /sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0
> > .0/spi-nor/sfdp | xxd -p
> > 53464450000101ff00000109300000ffc2000104600000ffffffffffffff
> > ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
> > 083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
> > ffffffffffff003600279df9c06485cbffffffffffff
>
> Ok, I like this output esp, because you can reverse it with
> "xxd -r -p". Maybe we should also request an md5sum just to
> be sure we have the same binary.
>
> so the requested commands would be something like:
>
> # xxd -p /path/to/sfdp
> # md5sum /path/to/sfdp
> # cat /path/to/jedec_id
> # cat /path/to/partname
> # cat /path/to/manufacturer

this will be the output for your suggestion:

# P=/sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor
&&  xxd -p $P/sfdp && md5sum $P/sfdp && cat $P/jedec_id && cat
$P/partname && cat $P/manufacturer
53464450000101ff00000109300000ffc2000104600000ffffffffffffff
ffffffffffffffffffffffffffffffffffffe520f1ffffffff0744eb086b
083b04bbfeffffffffff00ffffff44eb0c200f5210d800ffffffffffffff
ffffffffffff003600279df9c06485cbffffffffffff
f7a09ec0b60c9a0acd01bf2759f0d1f4
/sys/devices/platform/soc@0/30800000.bus/30bb0000.spi/spi_master/spi0/spi0.0/spi-nor/sfdp
c22018
mx25l12805d
macronix

-- 
Heiko

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

Re: [PATCH 4/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Tudor.Ambarus]

On 7/5/21 1:45 PM, Heiko Thiery wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Tudor,
> 
> Am Mo., 5. Juli 2021 um 09:51 Uhr schrieb <Tudor.Ambarus@microchip.com>:
>>
>> On 7/2/21 5:41 PM, Tudor Ambarus wrote:
>>> +     { "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP |
>>> +                           SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)
>>
>> I should have removed SECT_4K, since 4k erase should be set when parsing SFDP.
>> If SFDP is wrong, we can amend it in the post_bfpt hook.
> 
> But what about the 4K support for the "old" mx25l12805d that does not
> support reading SFDP? As far as I understand, the post_bfpt hook is
> only called when the SFDP was successfully read and this would mean
> the "old" one would never get this setting.

You're absolutely correct, we'll keep SECT_4K in order to not break backward
compatibility with the old flash. I thought it was a new flash addition. That's
me replying to emails while being distracted. Anyway, I'll have a documentation
written somewhere about guidelines on how to propose new flash addition or how
to handle flash collisions. Everything seems fine here.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/7] mtd: spi-nor: core: Introduce SPI_NOR_PARSE_SFDP

[Pratyush Yadav]

Hi Tudor,

I only have small commit message nitpicks/improvements for this patch.

On 02/07/21 05:41PM, Tudor Ambarus wrote:
> SPI NOR flashes that statically declare one of the
> SPI_NOR_{DUAL, QUAD, OCTAL, OCTAL_DTR}_READ flags, and do not support
> the RDSFDP command, are gratuiously receiving the RDSFDP command,

I think you should drop all the commas in this sentence. They seem 
unnecessary to me.

> in the attempt of parsing the SFDP tables. It is not desirable to issue
> comands that are not supported, so introduce a flag to help on this

s/comands/commands/

> situation.
> 
> New flash additions that support the SFDP standard should be declared
> using SPI_NOR_PARSE_SFDP. Support that can be discovered when parsing
> SFDP should not be duplicated by explicit flags at flash declaration.
> All the flash parameters will be discovered when parsing SFDP.
> Sometimes manufacturers wrongly define some fields in the SFDP tables.
> If that's the case, SFDP data can be ammended with the fixups() hooks.

s/ammended/amended/

> It is not common, but if the SFDP tables are entirely wrong, and it
> does not worth the hassle to tweak the SFDP parameters by using the
> fixups hooks, or if the flash does not define the SFDP tables at all,
> then statically init the flash with the SPI_NOR_SKIP_SFDP flag and
> specify the reset of flash capabilities with the flash info flags.

s/reset/rest/ ?

> 
> With time, we want to convert all flashes to SPI_NOR_PARSE_SFDP and
> stop triggering the SFDP parsing with the
> SPI_NOR_{DUAL, QUAD, OCTAL*}_READ flags. Getting rid of the
> SPI_NOR_{OCTAL, OCTAL_DTR}_READ trigger is easy achievable, the reset

s/easy/easily/

s/reset/rest/ ?

> are a long term goal.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c | 3 ++-
>  drivers/mtd/spi-nor/core.h | 4 ++++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index cc08bd707378..3d9f3698fb7b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2726,7 +2726,8 @@ static int spi_nor_init_params(struct spi_nor *nor)
>  
>  	spi_nor_manufacturer_init_params(nor);
>  
> -	if ((nor->info->flags & (SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +	if ((nor->info->flags & (SPI_NOR_PARSE_SFDP |
> +				 SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |

Ok.

>  				 SPI_NOR_OCTAL_READ | SPI_NOR_OCTAL_DTR_READ)) &&
>  	    !(nor->info->flags & SPI_NOR_SKIP_SFDP))
>  		spi_nor_sfdp_init_params(nor);
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 3348e1dd1445..55fceb0ec894 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -382,6 +382,10 @@ struct flash_info {
>  					 * protection bits. Usually these will
>  					 * power-up in a write-protected state.
>  					 */
> +#define SPI_NOR_PARSE_SFDP	BIT(23) /*
> +					 * Flash initialized based on the SFDP
> +					 * tables.
> +					 */

We will be running out of bits to use soon at this rate. But I don't 
think it is too much of a worry since it should be easy to make flags 
u64.

Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

>  
>  	const struct spi_nor_otp_organization otp_org;
>  
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

[Pratyush Yadav]

On 02/07/21 05:41PM, Tudor Ambarus wrote:
> Provide a way to report the correct flash name in case of ID collisions.
> There will be a single flash_info entry when flash IDs collide, and the
> differentiation between the flash types will be made at runtime
> if possible.

I am not sure if having one entry for all flashes with a collision is a 
good idea. For example, say we have one flash which supports SFDP and 
that's all we need for it. Another flash with the same ID does not 
support SFDP so it needs the SPI_NOR_DUAL_READ, etc. flags. How would 
you handle this case with the same entry? You would have to set all the 
flags in the disambiguation function. And nor->info is declared as const 
so you can't change the flags in there. Any code checking for 
info->flags would not work properly for these type of flashes. Wouldn't 
it be better to have multiple entries with the same ID and just pick the 
one we need in the disambiguation function?

Anyway, if you do decide to go with this approach, comments below.

> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c  | 5 ++++-
>  include/linux/mtd/spi-nor.h | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 3d9f3698fb7b..d528e28995c6 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -3208,7 +3208,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	/* Configure OTP parameters and ops */
>  	spi_nor_otp_init(nor);
>  
> -	dev_info(dev, "%s (%lld Kbytes)\n", info->name,
> +	if (!nor->name)
> +		nor->name = info->name;
> +
> +	dev_info(dev, "%s (%lld Kbytes)\n", nor->name,
>  			(long long)mtd->size >> 10);

You also need to update the usage of info->name in 
spi_nor_debugfs_init() and partname_show() in sysfs.c

>  
>  	dev_dbg(dev,
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index f67457748ed8..bd92acdd1899 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -369,6 +369,7 @@ struct spi_nor_flash_parameter;
>   * @bouncebuf:		bounce buffer used when the buffer passed by the MTD
>   *                      layer is not DMA-able
>   * @bouncebuf_size:	size of the bounce buffer
> + * @name:		used to point to correct name in case of ID collisions.
>   * @info:		SPI NOR part JEDEC MFR ID and other info
>   * @manufacturer:	SPI NOR manufacturer
>   * @page_size:		the page size of the SPI NOR
> @@ -399,6 +400,7 @@ struct spi_nor {
>  	struct spi_mem		*spimem;
>  	u8			*bouncebuf;
>  	size_t			bouncebuf_size;
> +	const char *name;
>  	const struct flash_info	*info;
>  	const struct spi_nor_manufacturer *manufacturer;
>  	u32			page_size;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

[Tudor.Ambarus]

On 7/5/21 4:13 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 02/07/21 05:41PM, Tudor Ambarus wrote:
>> Provide a way to report the correct flash name in case of ID collisions.
>> There will be a single flash_info entry when flash IDs collide, and the
>> differentiation between the flash types will be made at runtime
>> if possible.
> 
> I am not sure if having one entry for all flashes with a collision is a
> good idea. For example, say we have one flash which supports SFDP and
> that's all we need for it. Another flash with the same ID does not
> support SFDP so it needs the SPI_NOR_DUAL_READ, etc. flags. How would
> you handle this case with the same entry? You would have to set all the
> flags in the disambiguation function. And nor->info is declared as const
> so you can't change the flags in there. Any code checking for
> info->flags would not work properly for these type of flashes. Wouldn't
> it be better to have multiple entries with the same ID and just pick the
> one we need in the disambiguation function?

Your scenario is hit in patches 3/7 and 4/7. In case of ID collisions between
a SFDP and a non-SFDP compliant flash, I propose to set SPI_NOR_PARSE_SFDP, as
well as all the other required flash info flags that statically init the flash.
The SFDP flash will init all its NOR flags at runtime, while the non-SFDP flash
will issue a RDSFDP command that will fail, and then it will init all the NOR
flags based on the flash info flags.

> 
> Anyway, if you do decide to go with this approach, comments below.

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

Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

[Tudor.Ambarus]

On 7/5/21 4:24 PM, Tudor Ambarus - M18064 wrote:
> On 7/5/21 4:13 PM, Pratyush Yadav wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>
>> On 02/07/21 05:41PM, Tudor Ambarus wrote:
>>> Provide a way to report the correct flash name in case of ID collisions.
>>> There will be a single flash_info entry when flash IDs collide, and the
>>> differentiation between the flash types will be made at runtime
>>> if possible.
>>
>> I am not sure if having one entry for all flashes with a collision is a
>> good idea. For example, say we have one flash which supports SFDP and
>> that's all we need for it. Another flash with the same ID does not
>> support SFDP so it needs the SPI_NOR_DUAL_READ, etc. flags. How would
>> you handle this case with the same entry? You would have to set all the
>> flags in the disambiguation function. And nor->info is declared as const
>> so you can't change the flags in there. Any code checking for
>> info->flags would not work properly for these type of flashes. Wouldn't
>> it be better to have multiple entries with the same ID and just pick the
>> one we need in the disambiguation function?
> 
> Your scenario is hit in patches 3/7 and 4/7. In case of ID collisions between
> a SFDP and a non-SFDP compliant flash, I propose to set SPI_NOR_PARSE_SFDP, as
> well as all the other required flash info flags that statically init the flash.
> The SFDP flash will init all its NOR flags at runtime, while the non-SFDP flash

s/runtime/based on SFDP data

> will issue a RDSFDP command that will fail, and then it will init all the NOR
> flags based on the flash info flags.
> 
>>
>> Anyway, if you do decide to go with this approach, comments below.
> 
> Right, thanks.
> 

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

Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

[Pratyush Yadav]

On 05/07/21 01:24PM, Tudor.Ambarus@microchip.com wrote:
> On 7/5/21 4:13 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 02/07/21 05:41PM, Tudor Ambarus wrote:
> >> Provide a way to report the correct flash name in case of ID collisions.
> >> There will be a single flash_info entry when flash IDs collide, and the
> >> differentiation between the flash types will be made at runtime
> >> if possible.
> > 
> > I am not sure if having one entry for all flashes with a collision is a
> > good idea. For example, say we have one flash which supports SFDP and
> > that's all we need for it. Another flash with the same ID does not
> > support SFDP so it needs the SPI_NOR_DUAL_READ, etc. flags. How would
> > you handle this case with the same entry? You would have to set all the
> > flags in the disambiguation function. And nor->info is declared as const
> > so you can't change the flags in there. Any code checking for
> > info->flags would not work properly for these type of flashes. Wouldn't
> > it be better to have multiple entries with the same ID and just pick the
> > one we need in the disambiguation function?
> 
> Your scenario is hit in patches 3/7 and 4/7. In case of ID collisions between
> a SFDP and a non-SFDP compliant flash, I propose to set SPI_NOR_PARSE_SFDP, as
> well as all the other required flash info flags that statically init the flash.
> The SFDP flash will init all its NOR flags at runtime, while the non-SFDP flash
> will issue a RDSFDP command that will fail, and then it will init all the NOR
> flags based on the flash info flags.

spi_nor_info_init_params() is called before spi_nor_sfdp_init_params(). 
So if flash A sets SPI_NOR_QUAD_READ, flash B will also inherit it even 
though it might not actually support quad mode reads. You would need to 
refactor the parameter initialization path to do SFDP first and skip the 
flags based init.

But that doesn't solve all the problems. What about the flags that can't 
be discovered by SFDP? I see SPI_NOR_NO_ERASE, SPI_NOR_BP3_SR_BIT6, etc. 
that are not set by the SFDP path. Some of those might be discoverable, 
and we just don't do it yet, but I am not convinced all of them can be. 
For example, I don't see any way to discover SPI_NOR_NO_ERASE via SFDP. 
Erase type 1 field in BFPT is not optional and has to be specified.

How will you differentiate from the SFDP-discoverable and 
non-SFDP-discoverable flags? How can you which belongs to which flash? I 
know this is a bit far fetched but let's solve this problem once and for 
all.

> 
> > 
> > Anyway, if you do decide to go with this approach, comments below.
> 
> Right, thanks.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

[Tudor.Ambarus]

On 7/5/21 7:09 PM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 05/07/21 01:24PM, Tudor.Ambarus@microchip.com wrote:
>> On 7/5/21 4:13 PM, Pratyush Yadav wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> On 02/07/21 05:41PM, Tudor Ambarus wrote:
>>>> Provide a way to report the correct flash name in case of ID collisions.
>>>> There will be a single flash_info entry when flash IDs collide, and the
>>>> differentiation between the flash types will be made at runtime
>>>> if possible.
>>>
>>> I am not sure if having one entry for all flashes with a collision is a
>>> good idea. For example, say we have one flash which supports SFDP and
>>> that's all we need for it. Another flash with the same ID does not
>>> support SFDP so it needs the SPI_NOR_DUAL_READ, etc. flags. How would
>>> you handle this case with the same entry? You would have to set all the
>>> flags in the disambiguation function. And nor->info is declared as const
>>> so you can't change the flags in there. Any code checking for
>>> info->flags would not work properly for these type of flashes. Wouldn't
>>> it be better to have multiple entries with the same ID and just pick the
>>> one we need in the disambiguation function?
>>
>> Your scenario is hit in patches 3/7 and 4/7. In case of ID collisions between
>> a SFDP and a non-SFDP compliant flash, I propose to set SPI_NOR_PARSE_SFDP, as
>> well as all the other required flash info flags that statically init the flash.
>> The SFDP flash will init all its NOR flags at runtime, while the non-SFDP flash
>> will issue a RDSFDP command that will fail, and then it will init all the NOR
>> flags based on the flash info flags.
> 
> spi_nor_info_init_params() is called before spi_nor_sfdp_init_params().
> So if flash A sets SPI_NOR_QUAD_READ, flash B will also inherit it even
> though it might not actually support quad mode reads. You would need to
> refactor the parameter initialization path to do SFDP first and skip the
> flags based init.
> 
> But that doesn't solve all the problems. What about the flags that can't
> be discovered by SFDP? I see SPI_NOR_NO_ERASE, SPI_NOR_BP3_SR_BIT6, etc.
> that are not set by the SFDP path. Some of those might be discoverable,
> and we just don't do it yet, but I am not convinced all of them can be.
> For example, I don't see any way to discover SPI_NOR_NO_ERASE via SFDP.
> Erase type 1 field in BFPT is not optional and has to be specified.
> 
> How will you differentiate from the SFDP-discoverable and
> non-SFDP-discoverable flags? How can you which belongs to which flash? I
> know this is a bit far fetched but let's solve this problem once and for
> all.
> 

Yeah, that's my goal, to solve all these once and for all. I've started working on it,
and refactored the params init sequence. I have a WIP branch at:
https://github.com/ambarus/linux-0day/commits/spi-nor/next-id-collisions.
I'll post everything after I finish, we can ignore all this for the moment.

Thanks Pratyush for your insights!
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L3233F and MX25L3205D

[Pratyush Yadav]

On 02/07/21 05:41PM, Tudor Ambarus wrote:
> Macronix has a bad habbit of reusing flash IDs. While MX25L3233F supports
> RDSFDP opcode, MX25L3205D does not support it and does not recommend
> issuing opcodes that are not supported ("It is not recommended to adopt
> any other code not in the command definition table, which will potentially
> enter the hidden mode.").
> 
> We tested the RDSFDP on the MX25L3205D and the conclusion is that the
> flash didn't reply anything. Given that it is unlikely that RDSFDP will
> cause any problems for the old MX25L3205D, differentiate between the two
> flashes by parsing SFDP.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/macronix.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 27498ed0cc0d..83a097708949 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -8,6 +8,26 @@
>  
>  #include "core.h"
>  
> +static const char *mx25l3233f = "mx25l3233f";
> +
> +static int mx25l3233f_post_bfpt_fixups(struct spi_nor *nor,
> +				const struct sfdp_parameter_header *bfpt_header,
> +				const struct sfdp_bfpt *bfpt)
> +{
> +	/*
> +	 * Macronix has a bad habit of reusing flash IDs: MX25L3233F collides
> +	 * with MX25L3205D. MX25L3233F defines SFDP tables, while the older
> +	 * variant does not.
> +	 */
> +	nor->name = mx25l3233f;

Do we need to use the const char pointer here? Won't just doing 
nor->name = "mx2513233f"; do the trick?

> +
> +	return 0;
> +}
> +
> +static struct spi_nor_fixups mx25l3233f_fixups = {
> +	.post_bfpt = mx25l3233f_post_bfpt_fixups,
> +};
> +
>  static int
>  mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
>  			    const struct sfdp_parameter_header *bfpt_header,
> @@ -39,7 +59,9 @@ static const struct flash_info macronix_parts[] = {
>  	{ "mx25l4005a",  INFO(0xc22013, 0, 64 * 1024,   8, SECT_4K) },
>  	{ "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) },
> +	{ "mx25l3205d",  INFO(0xc22016, 0, 64 * 1024,  64, SPI_NOR_PARSE_SFDP |

Maybe add a comment here listing all the flashes that could collide with 
this ID?

Other than this,

Acked-by: Pratyush Yadav <p.yadav@ti.com>

> +			      SECT_4K)
> +		.fixups = &mx25l3233f_fixups },
>  	{ "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.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 4/7] mtd: spi-nor: macronix: Handle ID collision b/w MX25L12805D and MX25L12835F

[Pratyush Yadav]

On 02/07/21 05:41PM, Tudor Ambarus wrote:
> MX25L12835F define SFDP, while MX25L12805D does not.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/macronix.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index 83a097708949..38701347bf04 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -8,6 +8,26 @@
>  
>  #include "core.h"
>  
> +static const char *mx25l12835f = "mx25l12835f";
> +
> +static int mx25l12835f_post_bfpt_fixups(struct spi_nor *nor,
> +				const struct sfdp_parameter_header *bfpt_header,
> +				const struct sfdp_bfpt *bfpt)
> +{
> +	/*
> +	 * Macronix has a bad habit of reusing flash IDs: MX25L12835F collides
> +	 * with MX25L12805D. MX25L12835F defines SFDP tables, while the older
> +	 * variant does not.
> +	 */
> +	nor->name = mx25l12835f;

Same question as patch 3/7. Do we need to declare a separate pointer?

> +
> +	return 0;
> +}
> +
> +static struct spi_nor_fixups mx25l12835f_fixups = {
> +	.post_bfpt = mx25l12835f_post_bfpt_fixups,
> +};
> +
>  static const char *mx25l3233f = "mx25l3233f";
>  
>  static int mx25l3233f_post_bfpt_fixups(struct spi_nor *nor,
> @@ -71,8 +91,9 @@ static const struct flash_info macronix_parts[] = {
>  	{ "mx25u4035",   INFO(0xc22533, 0, 64 * 1024,   8, SECT_4K) },
>  	{ "mx25u8035",   INFO(0xc22534, 0, 64 * 1024,  16, SECT_4K) },
>  	{ "mx25u6435f",  INFO(0xc22537, 0, 64 * 1024, 128, SECT_4K) },
> -	{ "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SECT_4K |
> -			      SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP) },
> +	{ "mx25l12805d", INFO(0xc22018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP |

Same comment as patch 3/7. Consider adding a comment here listing all 
flashes with collision on this ID.

Other than these two nitpicks,

Acked-by: Pratyush Yadav <p.yadav@ti.com>

> +			      SECT_4K | SPI_NOR_HAS_LOCK | SPI_NOR_4BIT_BP)
> +		.fixups = &mx25l12835f_fixups },
>  	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
>  	{ "mx25r1635f",  INFO(0xc22815, 0, 64 * 1024,  32,
>  			      SECT_4K | SPI_NOR_DUAL_READ |
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 6/7] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Pratyush Yadav]

On 02/07/21 05:41PM, Tudor Ambarus wrote:
> Flash does not support continuation codes and may collide with a flash
> of other manufacturer, Intersil being an example .
> 
> This flash addition is just for demonstration purposes. As I don't
> have the flash, I assumed that it supports SFDP.

I found a datasheet for this flash via Google [0]. It does indeed 
support SFDP (section 6.36). The data in the SFDP table is not listed so 
we can't say if it is completely correct though.

I am not sure if we should add support for an untested flash. We ask 
people to only add tested flashes and I think it is a good principle to 
stick to.

> If the flash does not define the SFDP tables, it should be statically
> initialized with:
>         /* XTX (XTX Technology Limited) */
>         { "XT25F128B", INFO(0x0b4018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP |
>                             SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>                             SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> };
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/manuf-id-collisions.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c b/drivers/mtd/spi-nor/manuf-id-collisions.c
> index 9efcba9d18a0..c473c7d40d09 100644
> --- a/drivers/mtd/spi-nor/manuf-id-collisions.c
> +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
> @@ -8,6 +8,10 @@ static const struct flash_info id_collision_parts[] = {
>  	{ "by25q128as", INFO(0x684018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP |
>  			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
>  			     SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> +
> +	/* XTX (XTX Technology Limited) */
> +	{ "xt25f128b", INFO(0x0b4018, 0, 64 * 1024, 256, SPI_NOR_PARSE_SFDP |
> +			    SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
>  };
>  
>  const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = {
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 5/7] mtd: spi-nor: Introduce Manufacturer ID collisions driver

[Pratyush Yadav]

On 02/07/21 05:41PM, Tudor Ambarus wrote:
> Some manufacturers completely ignore the manufacturer's identification code
> standard (JEP106) and do not define the manufacturer ID continuation
> scheme. This will result in manufacturer ID collisions.
> 
> An an example, JEP106BA requires Boya that it's manufacturer ID to be
> preceded by 8 continuation codes. Boya's identification code must be:
> 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x68. But Boya ignores the
> continuation scheme and its ID collides with the manufacturer defined in
> bank one: Convex Computer.
> 
> Introduce the manuf-id-collisions driver in order to address ID collisions
> between manufacturers. flash_info entries will be added in a first come,

So all manufacturers with an ID collision will be placed in this file, 
and won't get a separate file of their own. Ok.

> first served manner. Differentiation between flashes will be done at
> runtime if possible. Where runtime differentiation is not possible, new
> compatibles will be introduced, but this will be done as a last resort.
> Every new flash addition that define the SFDP tables, should dump its SFDP
> tables in the patch's comment section below the --- line, so that we can
> reference it in case of collisions.

Can we have this rule documented somewhere? Anyone who doesn't closely 
follow the list won't know about this rule, and having to ask for it on 
every new patch would be tedious. I am not sure if we have a place to 
document this that has high visibility though.

> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/Makefile              |  1 +
>  drivers/mtd/spi-nor/core.c                |  1 +
>  drivers/mtd/spi-nor/core.h                |  1 +
>  drivers/mtd/spi-nor/manuf-id-collisions.c | 17 +++++++++++++++++
>  4 files changed, 20 insertions(+)
>  create mode 100644 drivers/mtd/spi-nor/manuf-id-collisions.c
> 
> diff --git a/drivers/mtd/spi-nor/Makefile b/drivers/mtd/spi-nor/Makefile
> index 6b904e439372..48763d10daad 100644
> --- a/drivers/mtd/spi-nor/Makefile
> +++ b/drivers/mtd/spi-nor/Makefile
> @@ -1,6 +1,7 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  spi-nor-objs			:= core.o sfdp.o swp.o otp.o sysfs.o
> +spi-nor-objs			+= manuf-id-collisions.o
>  spi-nor-objs			+= atmel.o
>  spi-nor-objs			+= catalyst.o
>  spi-nor-objs			+= eon.o
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index d528e28995c6..206a54c20fef 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -1829,6 +1829,7 @@ int spi_nor_sr2_bit7_quad_enable(struct spi_nor *nor)
>  }
>  
>  static const struct spi_nor_manufacturer *manufacturers[] = {
> +	&spi_nor_manuf_id_collisions,
>  	&spi_nor_atmel,
>  	&spi_nor_catalyst,
>  	&spi_nor_eon,
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 55fceb0ec894..e9896cd60695 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -476,6 +476,7 @@ struct sfdp {
>  };
>  
>  /* Manufacturer drivers. */
> +extern const struct spi_nor_manufacturer spi_nor_manuf_id_collisions;
>  extern const struct spi_nor_manufacturer spi_nor_atmel;
>  extern const struct spi_nor_manufacturer spi_nor_catalyst;
>  extern const struct spi_nor_manufacturer spi_nor_eon;
> diff --git a/drivers/mtd/spi-nor/manuf-id-collisions.c b/drivers/mtd/spi-nor/manuf-id-collisions.c
> new file mode 100644
> index 000000000000..9efcba9d18a0
> --- /dev/null
> +++ b/drivers/mtd/spi-nor/manuf-id-collisions.c
> @@ -0,0 +1,17 @@
> +// SPDX-License-Identifier: GPL-2.0

Please add a description for this file. Why it is needed, what should go 
in here, etc.

Other than this,

Acked-by: Pratyush Yadav <p.yadav@ti.com>

> +
> +#include <linux/mtd/spi-nor.h>
> +#include "core.h"
> +
> +static const struct flash_info id_collision_parts[] = {
> +	/* Boya */
> +	{ "by25q128as", INFO(0x684018, 0, 64 * 1024, 256, SPI_NOR_SKIP_SFDP |
> +			     SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ |
> +			     SPI_NOR_HAS_LOCK | SPI_NOR_HAS_TB) },
> +};
> +
> +const struct spi_nor_manufacturer spi_nor_manuf_id_collisions = {
> +	.name = "manufacturer ID collisions",
> +	.parts = id_collision_parts,
> +	.nparts = ARRAY_SIZE(id_collision_parts),
> +};
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 6/7] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Tudor.Ambarus]

On 7/6/21 9:28 AM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 02/07/21 05:41PM, Tudor Ambarus wrote:
>> Flash does not support continuation codes and may collide with a flash
>> of other manufacturer, Intersil being an example .
>>
>> This flash addition is just for demonstration purposes. As I don't
>> have the flash, I assumed that it supports SFDP.
> 
> I found a datasheet for this flash via Google [0]. It does indeed
> support SFDP (section 6.36). The data in the SFDP table is not listed so
> we can't say if it is completely correct though.
> 
> I am not sure if we should add support for an untested flash. We ask
> people to only add tested flashes and I think it is a good principle to
> stick to.
> 

We won't apply it if not tested and without the SFDP tables dump. This applies
for all the flashes. Chris Morgan (in Cc) proposed to introduce support for this
flash a while ago. If we get support from Chris, we'll consider applying it, if not,
not.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/7] mtd: spi-nor: core: Report correct name in case of ID collisions

[Pratyush Yadav]

On 06/07/21 05:19AM, Tudor.Ambarus@microchip.com wrote:
> On 7/5/21 7:09 PM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 05/07/21 01:24PM, Tudor.Ambarus@microchip.com wrote:
> >> On 7/5/21 4:13 PM, Pratyush Yadav wrote:
> >>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> >>>
> >>> On 02/07/21 05:41PM, Tudor Ambarus wrote:
> >>>> Provide a way to report the correct flash name in case of ID collisions.
> >>>> There will be a single flash_info entry when flash IDs collide, and the
> >>>> differentiation between the flash types will be made at runtime
> >>>> if possible.
> >>>
> >>> I am not sure if having one entry for all flashes with a collision is a
> >>> good idea. For example, say we have one flash which supports SFDP and
> >>> that's all we need for it. Another flash with the same ID does not
> >>> support SFDP so it needs the SPI_NOR_DUAL_READ, etc. flags. How would
> >>> you handle this case with the same entry? You would have to set all the
> >>> flags in the disambiguation function. And nor->info is declared as const
> >>> so you can't change the flags in there. Any code checking for
> >>> info->flags would not work properly for these type of flashes. Wouldn't
> >>> it be better to have multiple entries with the same ID and just pick the
> >>> one we need in the disambiguation function?
> >>
> >> Your scenario is hit in patches 3/7 and 4/7. In case of ID collisions between
> >> a SFDP and a non-SFDP compliant flash, I propose to set SPI_NOR_PARSE_SFDP, as
> >> well as all the other required flash info flags that statically init the flash.
> >> The SFDP flash will init all its NOR flags at runtime, while the non-SFDP flash
> >> will issue a RDSFDP command that will fail, and then it will init all the NOR
> >> flags based on the flash info flags.
> > 
> > spi_nor_info_init_params() is called before spi_nor_sfdp_init_params().
> > So if flash A sets SPI_NOR_QUAD_READ, flash B will also inherit it even
> > though it might not actually support quad mode reads. You would need to
> > refactor the parameter initialization path to do SFDP first and skip the
> > flags based init.
> > 
> > But that doesn't solve all the problems. What about the flags that can't
> > be discovered by SFDP? I see SPI_NOR_NO_ERASE, SPI_NOR_BP3_SR_BIT6, etc.
> > that are not set by the SFDP path. Some of those might be discoverable,
> > and we just don't do it yet, but I am not convinced all of them can be.
> > For example, I don't see any way to discover SPI_NOR_NO_ERASE via SFDP.
> > Erase type 1 field in BFPT is not optional and has to be specified.
> > 
> > How will you differentiate from the SFDP-discoverable and
> > non-SFDP-discoverable flags? How can you which belongs to which flash? I
> > know this is a bit far fetched but let's solve this problem once and for
> > all.
> > 
> 
> Yeah, that's my goal, to solve all these once and for all. I've started working on it,
> and refactored the params init sequence. I have a WIP branch at:
> https://github.com/ambarus/linux-0day/commits/spi-nor/next-id-collisions.
> I'll post everything after I finish, we can ignore all this for the moment.

Fair enough. Let's pick this topic up again once you have everything 
ready.

> 
> Thanks Pratyush for your insights!

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 6/7] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Pratyush Yadav]

On 06/07/21 06:39AM, Tudor.Ambarus@microchip.com wrote:
> On 7/6/21 9:28 AM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 02/07/21 05:41PM, Tudor Ambarus wrote:
> >> Flash does not support continuation codes and may collide with a flash
> >> of other manufacturer, Intersil being an example .
> >>
> >> This flash addition is just for demonstration purposes. As I don't
> >> have the flash, I assumed that it supports SFDP.
> > 
> > I found a datasheet for this flash via Google [0]. It does indeed
> > support SFDP (section 6.36). The data in the SFDP table is not listed so
> > we can't say if it is completely correct though.
> > 
> > I am not sure if we should add support for an untested flash. We ask
> > people to only add tested flashes and I think it is a good principle to
> > stick to.
> > 
> 
> We won't apply it if not tested and without the SFDP tables dump. This applies
> for all the flashes. Chris Morgan (in Cc) proposed to introduce support for this
> flash a while ago. If we get support from Chris, we'll consider applying it, if not,
> not.

Ok. Makes sense.

BTW, I forgot to add the reference [0] earlier. So here it is below.

[0] https://datasheet.lcsc.com/szlcsc/2005251035_XTX-XT25F128BWOIGT_C558846.pdf

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 5/7] mtd: spi-nor: Introduce Manufacturer ID collisions driver

[Tudor.Ambarus]

On 7/6/21 9:34 AM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 02/07/21 05:41PM, Tudor Ambarus wrote:
>> Some manufacturers completely ignore the manufacturer's identification code
>> standard (JEP106) and do not define the manufacturer ID continuation
>> scheme. This will result in manufacturer ID collisions.
>>
>> An an example, JEP106BA requires Boya that it's manufacturer ID to be
>> preceded by 8 continuation codes. Boya's identification code must be:
>> 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x7f, 0x68. But Boya ignores the
>> continuation scheme and its ID collides with the manufacturer defined in
>> bank one: Convex Computer.
>>
>> Introduce the manuf-id-collisions driver in order to address ID collisions
>> between manufacturers. flash_info entries will be added in a first come,
> 
> So all manufacturers with an ID collision will be placed in this file,
> and won't get a separate file of their own. Ok.

manuf-id-collisions will be the place to add new flash additions where the
manufacturer is ignorant enough to not implement the ID continuation scheme.

Collisions between flashes of the same manufacturer can be handled in their
own manufacturer driver, macronix being an example.

> 
>> first served manner. Differentiation between flashes will be done at
>> runtime if possible. Where runtime differentiation is not possible, new
>> compatibles will be introduced, but this will be done as a last resort.
>> Every new flash addition that define the SFDP tables, should dump its SFDP
>> tables in the patch's comment section below the --- line, so that we can
>> reference it in case of collisions.
> 
> Can we have this rule documented somewhere? Anyone who doesn't closely
> follow the list won't know about this rule, and having to ask for it on
> every new patch would be tedious. I am not sure if we have a place to
> document this that has high visibility though.

Yes, I thought of adding some guidelines on how to propose a new flash addition.
We can add them in Documentation/driver-api/mtd/spi-nor.rst
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 6/7] mtd: spi-nor: manuf-id-collisions: Add support for xt25f128b

[Chris Morgan]

On Tue, Jul 06, 2021 at 06:39:09AM +0000, Tudor.Ambarus@microchip.com wrote:
> On 7/6/21 9:28 AM, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 02/07/21 05:41PM, Tudor Ambarus wrote:
> >> Flash does not support continuation codes and may collide with a flash
> >> of other manufacturer, Intersil being an example .
> >>
> >> This flash addition is just for demonstration purposes. As I don't
> >> have the flash, I assumed that it supports SFDP.
> > 
> > I found a datasheet for this flash via Google [0]. It does indeed
> > support SFDP (section 6.36). The data in the SFDP table is not listed so
> > we can't say if it is completely correct though.
> > 
> > I am not sure if we should add support for an untested flash. We ask
> > people to only add tested flashes and I think it is a good principle to
> > stick to.
> > 
> 
> We won't apply it if not tested and without the SFDP tables dump. This applies
> for all the flashes. Chris Morgan (in Cc) proposed to introduce support for this
> flash a while ago. If we get support from Chris, we'll consider applying it, if not,
> not.

I've tested the xt25f128b driver here, it works.  Note though that I
think the flags of SPI_NOR_HAS_LOCK and SPI_NOR_HAS_TB may not be
needed, as I believe on a cursory glance they correspond to different
bits than the datasheet has defined.

Also note that I have previously dumped the SFDC table here, if you need a more
recent dump please let me know:

http://lists.infradead.org/pipermail/linux-mtd/2021-April/085976.html

0000000 4653 5044 0100 ff01 0000 0901 0030 ff00
0000010 000b 0301 0060 ff00 ffff ffff ffff ffff
0000020 ffff ffff ffff ffff ffff ffff ffff ffff
0000030 20e5 fff1 ffff 07ff eb44 6b08 3b08 bb42
0000040 ffee ffff ffff ff00 ffff ff00 200c 520f
0000050 d810 ff00 ffff ffff ffff ffff ffff ffff
0000060 3600 2700 f99f 6477 e8d9 ffff

Thank you.

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