[PATCH v5 0/3] mtd: spi-nor: mx25l25635f: Use 4B opcodes

[PATCH v5 0/3] mtd: spi-nor: mx25l25635f: Use 4B opcodes

[Boris Brezillon]

Hello,

This is another attempt at fixing the bug reported by Alexandre and
impacting mx25l25635f flashes that do not have the reset line properly
wired. Since mx25l25635f and mx25l25635e share the same JEDEC-ID, and
only mx25l25635f supports 4B opcodes, the core decides to enter 4B
mode, and when a reset occurs, the NOR stays in this mode and the ROM
code (why only supports 3 byte addressing) can't read the flash
anymore.

Adding broken-flash-reset in the DT fixes the reboot issue but prints
a WARN_ON() backtrace at boot time. The proper solution is to use 4B
opcodes when available, but we need a way to flag the F variant of the
chip as supporting this feature.

This patchset paves the way for more SFDP-related fixups hooks by
adding a spi_nor_fixups struct which can be provided on a per-chip
basis (and soon on a per-manufacturer basis).

In this series we add a single hook called post_bfpt() to allow fixups
just after the BFPT parsing has occurred. Thanks to the BFPT array, we
are able to differentiate the F and E variant and add the 4B_OPCODE
flag when appropriate.

With this infrastructure in place, we'll be able to fix SFPD tables at
runtime instead setting the SKIP_SFDP on NORs with broken SFDP.

Feel free to comment on the general approach or implementation details.

Regards,

Boris

Boris Brezillon (3):
  mtd: spi-nor: Add the SNOR_F_4B_OPCODES flag
  mtd: spi-nor: Add a post BFPT parsing fixup hook
  mtd: spi-nor: Add a post BFPT fixup for MX25L25635E

 drivers/mtd/spi-nor/spi-nor.c | 437 +++++++++++++++++++---------------
 include/linux/mtd/spi-nor.h   |   1 +
 2 files changed, 249 insertions(+), 189 deletions(-)

-- 
2.17.1

[PATCH v5 1/3] mtd: spi-nor: Add the SNOR_F_4B_OPCODES flag

[Boris Brezillon]

Some flash_info entries have the SPI_NOR_4B_OPCODES flag set to let the
core know that the flash supports 4B opcode. While this solution works
fine for id-based caps detection, it doesn't work that well when relying
on SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so
that the SFDP parsing code can set it when appropriate.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v5:
- Cosmetic changes

Changes in v4:
- Set SNOR_F_4B_OPCODES flag outside of the if (mtd->size > 0x1000000)
  block
- Do not set SNOR_F_4B_OPCODES when BFPT_DWORD1_ADDRESS_BYTES_4_ONLY,
  because 4byte only does not imply 4B opcodes are supported

Changes in v3:
- Clear SNOR_F_4B_OPCODES flag when SFDP fails
- Add Alexandre R-b

Changes in v2:
- Fix the commit message
- Fix the ->addr_width check
- Add a comma at the end of the SNOR_F_4B_OPCODES definition
---
 drivers/mtd/spi-nor/spi-nor.c | 21 +++++++++++----------
 include/linux/mtd/spi-nor.h   |  1 +
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 8f07430ffffa..bea122e0d731 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3272,6 +3272,7 @@ static int spi_nor_init_params(struct spi_nor *nor,
 
 		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
 			nor->addr_width = 0;
+			nor->flags &= ~SNOR_F_4B_OPCODES;
 			/* restore previous erase map */
 			memcpy(&nor->erase_map, &prev_map,
 			       sizeof(nor->erase_map));
@@ -3572,9 +3573,7 @@ static int spi_nor_init(struct spi_nor *nor)
 		}
 	}
 
-	if ((nor->addr_width == 4) &&
-	    (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) &&
-	    !(nor->info->flags & SPI_NOR_4B_OPCODES)) {
+	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
 		/*
 		 * If the RESET# pin isn't hooked up properly, or the system
 		 * otherwise doesn't perform a reset command in the boot
@@ -3606,10 +3605,8 @@ static void spi_nor_resume(struct mtd_info *mtd)
 void spi_nor_restore(struct spi_nor *nor)
 {
 	/* restore the addressing mode */
-	if ((nor->addr_width == 4) &&
-	    (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) &&
-	    !(nor->info->flags & SPI_NOR_4B_OPCODES) &&
-	    (nor->flags & SNOR_F_BROKEN_RESET))
+	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
+	    nor->flags & SNOR_F_BROKEN_RESET)
 		set_4byte(nor, nor->info, 0);
 }
 EXPORT_SYMBOL_GPL(spi_nor_restore);
@@ -3765,13 +3762,17 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 	} else if (mtd->size > 0x1000000) {
 		/* enable 4-byte addressing if the device exceeds 16MiB */
 		nor->addr_width = 4;
-		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
-		    info->flags & SPI_NOR_4B_OPCODES)
-			spi_nor_set_4byte_opcodes(nor, info);
 	} else {
 		nor->addr_width = 3;
 	}
 
+	if (info->flags & SPI_NOR_4B_OPCODES ||
+	    (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
+		nor->flags |= SNOR_F_4B_OPCODES;
+
+	if (nor->addr_width == 4 && nor->flags & SNOR_F_4B_OPCODES)
+		spi_nor_set_4byte_opcodes(nor, info);
+
 	if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) {
 		dev_err(dev, "address width is too large: %u\n",
 			nor->addr_width);
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 8b1acf68b7ac..981d628305a2 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -237,6 +237,7 @@ enum spi_nor_option_flags {
 	SNOR_F_READY_XSR_RDY	= BIT(4),
 	SNOR_F_USE_CLSR		= BIT(5),
 	SNOR_F_BROKEN_RESET	= BIT(6),
+	SNOR_F_4B_OPCODES	= BIT(7),
 };
 
 /**
-- 
2.17.1

Re: [PATCH v5 1/3] mtd: spi-nor: Add the SNOR_F_4B_OPCODES flag

[Tudor.Ambarus]

On 12/06/2018 12:37 PM, Boris Brezillon wrote:
> Some flash_info entries have the SPI_NOR_4B_OPCODES flag set to let the
> core know that the flash supports 4B opcode. While this solution works
> fine for id-based caps detection, it doesn't work that well when relying
> on SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so
> that the SFDP parsing code can set it when appropriate.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

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

> ---
> Changes in v5:
> - Cosmetic changes
> 
> Changes in v4:
> - Set SNOR_F_4B_OPCODES flag outside of the if (mtd->size > 0x1000000)
>   block
> - Do not set SNOR_F_4B_OPCODES when BFPT_DWORD1_ADDRESS_BYTES_4_ONLY,
>   because 4byte only does not imply 4B opcodes are supported
> 
> Changes in v3:
> - Clear SNOR_F_4B_OPCODES flag when SFDP fails
> - Add Alexandre R-b
> 
> Changes in v2:
> - Fix the commit message
> - Fix the ->addr_width check
> - Add a comma at the end of the SNOR_F_4B_OPCODES definition
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 21 +++++++++++----------
>  include/linux/mtd/spi-nor.h   |  1 +
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 8f07430ffffa..bea122e0d731 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -3272,6 +3272,7 @@ static int spi_nor_init_params(struct spi_nor *nor,
>  
>  		if (spi_nor_parse_sfdp(nor, &sfdp_params)) {
>  			nor->addr_width = 0;
> +			nor->flags &= ~SNOR_F_4B_OPCODES;
>  			/* restore previous erase map */
>  			memcpy(&nor->erase_map, &prev_map,
>  			       sizeof(nor->erase_map));
> @@ -3572,9 +3573,7 @@ static int spi_nor_init(struct spi_nor *nor)
>  		}
>  	}
>  
> -	if ((nor->addr_width == 4) &&
> -	    (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) &&
> -	    !(nor->info->flags & SPI_NOR_4B_OPCODES)) {
> +	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES)) {
>  		/*
>  		 * If the RESET# pin isn't hooked up properly, or the system
>  		 * otherwise doesn't perform a reset command in the boot
> @@ -3606,10 +3605,8 @@ static void spi_nor_resume(struct mtd_info *mtd)
>  void spi_nor_restore(struct spi_nor *nor)
>  {
>  	/* restore the addressing mode */
> -	if ((nor->addr_width == 4) &&
> -	    (JEDEC_MFR(nor->info) != SNOR_MFR_SPANSION) &&
> -	    !(nor->info->flags & SPI_NOR_4B_OPCODES) &&
> -	    (nor->flags & SNOR_F_BROKEN_RESET))
> +	if (nor->addr_width == 4 && !(nor->flags & SNOR_F_4B_OPCODES) &&
> +	    nor->flags & SNOR_F_BROKEN_RESET)
>  		set_4byte(nor, nor->info, 0);
>  }
>  EXPORT_SYMBOL_GPL(spi_nor_restore);
> @@ -3765,13 +3762,17 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
>  	} else if (mtd->size > 0x1000000) {
>  		/* enable 4-byte addressing if the device exceeds 16MiB */
>  		nor->addr_width = 4;
> -		if (JEDEC_MFR(info) == SNOR_MFR_SPANSION ||
> -		    info->flags & SPI_NOR_4B_OPCODES)
> -			spi_nor_set_4byte_opcodes(nor, info);
>  	} else {
>  		nor->addr_width = 3;
>  	}
>  
> +	if (info->flags & SPI_NOR_4B_OPCODES ||
> +	    (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
> +		nor->flags |= SNOR_F_4B_OPCODES;
> +
> +	if (nor->addr_width == 4 && nor->flags & SNOR_F_4B_OPCODES)
> +		spi_nor_set_4byte_opcodes(nor, info);
> +
>  	if (nor->addr_width > SPI_NOR_MAX_ADDR_WIDTH) {
>  		dev_err(dev, "address width is too large: %u\n",
>  			nor->addr_width);
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 8b1acf68b7ac..981d628305a2 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -237,6 +237,7 @@ enum spi_nor_option_flags {
>  	SNOR_F_READY_XSR_RDY	= BIT(4),
>  	SNOR_F_USE_CLSR		= BIT(5),
>  	SNOR_F_BROKEN_RESET	= BIT(6),
> +	SNOR_F_4B_OPCODES	= BIT(7),
>  };
>  
>  /**
> 

Re: [v5,1/3] mtd: spi-nor: Add the SNOR_F_4B_OPCODES flag

[Boris Brezillon]

On Thu, 2018-12-06 at 10:37:34 UTC, Boris Brezillon wrote:
> Some flash_info entries have the SPI_NOR_4B_OPCODES flag set to let the
> core know that the flash supports 4B opcode. While this solution works
> fine for id-based caps detection, it doesn't work that well when relying
> on SFDP-based caps detection. Let's add an SNOR_F_4B_OPCODES flag so
> that the SFDP parsing code can set it when appropriate.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to http://git.infradead.org/linux-mtd.git spi-nor/next.

Boris

[PATCH v5 2/3] mtd: spi-nor: Add a post BFPT parsing fixup hook

[Boris Brezillon]

Experience has proven that SFDP tables are sometimes wrong, and parsing
of these broken tables can lead to erroneous flash config.

This leaves us 2 options:

1/ set the SPI_NOR_SKIP_SFDP flag and completely ignore SFDP parsing
2/ fix things at runtime

While #1 should always work, it might imply extra work if most of the
SFDP is correct. #2 has the benefit of keeping the generic SFDP parsing
logic almost untouched while allowing SPI NOR manufacturer drivers to
fix the broken bits.

Add a spi_nor_fixups struct where we'll put all our fixup hooks, each
of them being called at a different point in the scan process.

We start a hook called just after the BFPT parsing to allow fixing up
info extracted from the BFPT section. More hooks will be added if other
sections need to be fixed.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v5:
- None

Changes in v4:
- New patch
---
 drivers/mtd/spi-nor/spi-nor.c | 387 ++++++++++++++++++----------------
 1 file changed, 209 insertions(+), 178 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index bea122e0d731..7bc9887d9c0a 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -42,6 +42,196 @@
 #define SPI_NOR_MAX_ID_LEN	6
 #define SPI_NOR_MAX_ADDR_WIDTH	4
 
+struct spi_nor_read_command {
+	u8			num_mode_clocks;
+	u8			num_wait_states;
+	u8			opcode;
+	enum spi_nor_protocol	proto;
+};
+
+struct spi_nor_pp_command {
+	u8			opcode;
+	enum spi_nor_protocol	proto;
+};
+
+enum spi_nor_read_command_index {
+	SNOR_CMD_READ,
+	SNOR_CMD_READ_FAST,
+	SNOR_CMD_READ_1_1_1_DTR,
+
+	/* Dual SPI */
+	SNOR_CMD_READ_1_1_2,
+	SNOR_CMD_READ_1_2_2,
+	SNOR_CMD_READ_2_2_2,
+	SNOR_CMD_READ_1_2_2_DTR,
+
+	/* Quad SPI */
+	SNOR_CMD_READ_1_1_4,
+	SNOR_CMD_READ_1_4_4,
+	SNOR_CMD_READ_4_4_4,
+	SNOR_CMD_READ_1_4_4_DTR,
+
+	/* Octo SPI */
+	SNOR_CMD_READ_1_1_8,
+	SNOR_CMD_READ_1_8_8,
+	SNOR_CMD_READ_8_8_8,
+	SNOR_CMD_READ_1_8_8_DTR,
+
+	SNOR_CMD_READ_MAX
+};
+
+enum spi_nor_pp_command_index {
+	SNOR_CMD_PP,
+
+	/* Quad SPI */
+	SNOR_CMD_PP_1_1_4,
+	SNOR_CMD_PP_1_4_4,
+	SNOR_CMD_PP_4_4_4,
+
+	/* Octo SPI */
+	SNOR_CMD_PP_1_1_8,
+	SNOR_CMD_PP_1_8_8,
+	SNOR_CMD_PP_8_8_8,
+
+	SNOR_CMD_PP_MAX
+};
+
+struct spi_nor_flash_parameter {
+	u64				size;
+	u32				page_size;
+
+	struct spi_nor_hwcaps		hwcaps;
+	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
+	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
+
+	int (*quad_enable)(struct spi_nor *nor);
+};
+
+struct sfdp_parameter_header {
+	u8		id_lsb;
+	u8		minor;
+	u8		major;
+	u8		length; /* in double words */
+	u8		parameter_table_pointer[3]; /* byte address */
+	u8		id_msb;
+};
+
+#define SFDP_PARAM_HEADER_ID(p)	(((p)->id_msb << 8) | (p)->id_lsb)
+#define SFDP_PARAM_HEADER_PTP(p) \
+	(((p)->parameter_table_pointer[2] << 16) | \
+	 ((p)->parameter_table_pointer[1] <<  8) | \
+	 ((p)->parameter_table_pointer[0] <<  0))
+
+#define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
+#define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
+
+#define SFDP_SIGNATURE		0x50444653U
+#define SFDP_JESD216_MAJOR	1
+#define SFDP_JESD216_MINOR	0
+#define SFDP_JESD216A_MINOR	5
+#define SFDP_JESD216B_MINOR	6
+
+struct sfdp_header {
+	u32		signature; /* Ox50444653U <=> "SFDP" */
+	u8		minor;
+	u8		major;
+	u8		nph; /* 0-base number of parameter headers */
+	u8		unused;
+
+	/* Basic Flash Parameter Table. */
+	struct sfdp_parameter_header	bfpt_header;
+};
+
+/* Basic Flash Parameter Table */
+
+/*
+ * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
+ * They are indexed from 1 but C arrays are indexed from 0.
+ */
+#define BFPT_DWORD(i)		((i) - 1)
+#define BFPT_DWORD_MAX		16
+
+/* The first version of JESB216 defined only 9 DWORDs. */
+#define BFPT_DWORD_MAX_JESD216			9
+
+/* 1st DWORD. */
+#define BFPT_DWORD1_FAST_READ_1_1_2		BIT(16)
+#define BFPT_DWORD1_ADDRESS_BYTES_MASK		GENMASK(18, 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY	(0x0UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4	(0x1UL << 17)
+#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY	(0x2UL << 17)
+#define BFPT_DWORD1_DTR				BIT(19)
+#define BFPT_DWORD1_FAST_READ_1_2_2		BIT(20)
+#define BFPT_DWORD1_FAST_READ_1_4_4		BIT(21)
+#define BFPT_DWORD1_FAST_READ_1_1_4		BIT(22)
+
+/* 5th DWORD. */
+#define BFPT_DWORD5_FAST_READ_2_2_2		BIT(0)
+#define BFPT_DWORD5_FAST_READ_4_4_4		BIT(4)
+
+/* 11th DWORD. */
+#define BFPT_DWORD11_PAGE_SIZE_SHIFT		4
+#define BFPT_DWORD11_PAGE_SIZE_MASK		GENMASK(7, 4)
+
+/* 15th DWORD. */
+
+/*
+ * (from JESD216 rev B)
+ * Quad Enable Requirements (QER):
+ * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-4
+ *         reads based on instruction. DQ3/HOLD# functions are hold during
+ *         instruction phase.
+ * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ *         Writing only one byte to the status register has the side-effect of
+ *         clearing status register 2, including the QE bit. The 100b code is
+ *         used if writing one byte to the status register does not modify
+ *         status register 2.
+ * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
+ *         one data byte where bit 6 is one.
+ *         [...]
+ * - 011b: QE is bit 7 of status register 2. It is set via Write status
+ *         register 2 instruction 3Eh with one data byte where bit 7 is one.
+ *         [...]
+ *         The status register 2 is read using instruction 3Fh.
+ * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ *         In contrast to the 001b code, writing one byte to the status
+ *         register does not modify status register 2.
+ * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
+ *         Read Status instruction 05h. Status register2 is read using
+ *         instruction 35h. QE is set via Writ Status instruction 01h with
+ *         two data bytes where bit 1 of the second byte is one.
+ *         [...]
+ */
+#define BFPT_DWORD15_QER_MASK			GENMASK(22, 20)
+#define BFPT_DWORD15_QER_NONE			(0x0UL << 20) /* Micron */
+#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY		(0x1UL << 20)
+#define BFPT_DWORD15_QER_SR1_BIT6		(0x2UL << 20) /* Macronix */
+#define BFPT_DWORD15_QER_SR2_BIT7		(0x3UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD		(0x4UL << 20)
+#define BFPT_DWORD15_QER_SR2_BIT1		(0x5UL << 20) /* Spansion */
+
+struct sfdp_bfpt {
+	u32	dwords[BFPT_DWORD_MAX];
+};
+
+/**
+ * struct spi_nor_fixups - SPI NOR fixup hooks
+ * @post_bfpt: called after the BFPT table has been parsed
+ *
+ * Those hooks can be used to tweak the SPI NOR configuration when the SFDP
+ * table is broken or not available.
+ */
+struct spi_nor_fixups {
+	int (*post_bfpt)(struct spi_nor *nor,
+			 const struct sfdp_parameter_header *bfpt_header,
+			 const struct sfdp_bfpt *bfpt,
+			 struct spi_nor_flash_parameter *params);
+};
+
 struct flash_info {
 	char		*name;
 
@@ -91,6 +281,9 @@ struct flash_info {
 #define SPI_NOR_SKIP_SFDP	BIT(13)	/* Skip parsing of SFDP tables */
 #define USE_CLSR		BIT(14)	/* use CLSR command */
 
+	/* Part specific fixup hooks. */
+	const struct spi_nor_fixups *fixups;
+
 	int	(*quad_enable)(struct spi_nor *nor);
 };
 
@@ -2076,71 +2269,6 @@ static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor)
 	return 0;
 }
 
-struct spi_nor_read_command {
-	u8			num_mode_clocks;
-	u8			num_wait_states;
-	u8			opcode;
-	enum spi_nor_protocol	proto;
-};
-
-struct spi_nor_pp_command {
-	u8			opcode;
-	enum spi_nor_protocol	proto;
-};
-
-enum spi_nor_read_command_index {
-	SNOR_CMD_READ,
-	SNOR_CMD_READ_FAST,
-	SNOR_CMD_READ_1_1_1_DTR,
-
-	/* Dual SPI */
-	SNOR_CMD_READ_1_1_2,
-	SNOR_CMD_READ_1_2_2,
-	SNOR_CMD_READ_2_2_2,
-	SNOR_CMD_READ_1_2_2_DTR,
-
-	/* Quad SPI */
-	SNOR_CMD_READ_1_1_4,
-	SNOR_CMD_READ_1_4_4,
-	SNOR_CMD_READ_4_4_4,
-	SNOR_CMD_READ_1_4_4_DTR,
-
-	/* Octo SPI */
-	SNOR_CMD_READ_1_1_8,
-	SNOR_CMD_READ_1_8_8,
-	SNOR_CMD_READ_8_8_8,
-	SNOR_CMD_READ_1_8_8_DTR,
-
-	SNOR_CMD_READ_MAX
-};
-
-enum spi_nor_pp_command_index {
-	SNOR_CMD_PP,
-
-	/* Quad SPI */
-	SNOR_CMD_PP_1_1_4,
-	SNOR_CMD_PP_1_4_4,
-	SNOR_CMD_PP_4_4_4,
-
-	/* Octo SPI */
-	SNOR_CMD_PP_1_1_8,
-	SNOR_CMD_PP_1_8_8,
-	SNOR_CMD_PP_8_8_8,
-
-	SNOR_CMD_PP_MAX
-};
-
-struct spi_nor_flash_parameter {
-	u64				size;
-	u32				page_size;
-
-	struct spi_nor_hwcaps		hwcaps;
-	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
-	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
-
-	int (*quad_enable)(struct spi_nor *nor);
-};
-
 static void
 spi_nor_set_read_settings(struct spi_nor_read_command *read,
 			  u8 num_mode_clocks,
@@ -2263,117 +2391,6 @@ static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
 	return ret;
 }
 
-struct sfdp_parameter_header {
-	u8		id_lsb;
-	u8		minor;
-	u8		major;
-	u8		length; /* in double words */
-	u8		parameter_table_pointer[3]; /* byte address */
-	u8		id_msb;
-};
-
-#define SFDP_PARAM_HEADER_ID(p)	(((p)->id_msb << 8) | (p)->id_lsb)
-#define SFDP_PARAM_HEADER_PTP(p) \
-	(((p)->parameter_table_pointer[2] << 16) | \
-	 ((p)->parameter_table_pointer[1] <<  8) | \
-	 ((p)->parameter_table_pointer[0] <<  0))
-
-#define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
-#define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
-
-#define SFDP_SIGNATURE		0x50444653U
-#define SFDP_JESD216_MAJOR	1
-#define SFDP_JESD216_MINOR	0
-#define SFDP_JESD216A_MINOR	5
-#define SFDP_JESD216B_MINOR	6
-
-struct sfdp_header {
-	u32		signature; /* Ox50444653U <=> "SFDP" */
-	u8		minor;
-	u8		major;
-	u8		nph; /* 0-base number of parameter headers */
-	u8		unused;
-
-	/* Basic Flash Parameter Table. */
-	struct sfdp_parameter_header	bfpt_header;
-};
-
-/* Basic Flash Parameter Table */
-
-/*
- * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
- * They are indexed from 1 but C arrays are indexed from 0.
- */
-#define BFPT_DWORD(i)		((i) - 1)
-#define BFPT_DWORD_MAX		16
-
-/* The first version of JESB216 defined only 9 DWORDs. */
-#define BFPT_DWORD_MAX_JESD216			9
-
-/* 1st DWORD. */
-#define BFPT_DWORD1_FAST_READ_1_1_2		BIT(16)
-#define BFPT_DWORD1_ADDRESS_BYTES_MASK		GENMASK(18, 17)
-#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY	(0x0UL << 17)
-#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4	(0x1UL << 17)
-#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY	(0x2UL << 17)
-#define BFPT_DWORD1_DTR				BIT(19)
-#define BFPT_DWORD1_FAST_READ_1_2_2		BIT(20)
-#define BFPT_DWORD1_FAST_READ_1_4_4		BIT(21)
-#define BFPT_DWORD1_FAST_READ_1_1_4		BIT(22)
-
-/* 5th DWORD. */
-#define BFPT_DWORD5_FAST_READ_2_2_2		BIT(0)
-#define BFPT_DWORD5_FAST_READ_4_4_4		BIT(4)
-
-/* 11th DWORD. */
-#define BFPT_DWORD11_PAGE_SIZE_SHIFT		4
-#define BFPT_DWORD11_PAGE_SIZE_MASK		GENMASK(7, 4)
-
-/* 15th DWORD. */
-
-/*
- * (from JESD216 rev B)
- * Quad Enable Requirements (QER):
- * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-4
- *         reads based on instruction. DQ3/HOLD# functions are hold during
- *         instruction phase.
- * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
- *         two data bytes where bit 1 of the second byte is one.
- *         [...]
- *         Writing only one byte to the status register has the side-effect of
- *         clearing status register 2, including the QE bit. The 100b code is
- *         used if writing one byte to the status register does not modify
- *         status register 2.
- * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
- *         one data byte where bit 6 is one.
- *         [...]
- * - 011b: QE is bit 7 of status register 2. It is set via Write status
- *         register 2 instruction 3Eh with one data byte where bit 7 is one.
- *         [...]
- *         The status register 2 is read using instruction 3Fh.
- * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
- *         two data bytes where bit 1 of the second byte is one.
- *         [...]
- *         In contrast to the 001b code, writing one byte to the status
- *         register does not modify status register 2.
- * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
- *         Read Status instruction 05h. Status register2 is read using
- *         instruction 35h. QE is set via Writ Status instruction 01h with
- *         two data bytes where bit 1 of the second byte is one.
- *         [...]
- */
-#define BFPT_DWORD15_QER_MASK			GENMASK(22, 20)
-#define BFPT_DWORD15_QER_NONE			(0x0UL << 20) /* Micron */
-#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY		(0x1UL << 20)
-#define BFPT_DWORD15_QER_SR1_BIT6		(0x2UL << 20) /* Macronix */
-#define BFPT_DWORD15_QER_SR2_BIT7		(0x3UL << 20)
-#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD		(0x4UL << 20)
-#define BFPT_DWORD15_QER_SR2_BIT1		(0x5UL << 20) /* Spansion */
-
-struct sfdp_bfpt {
-	u32	dwords[BFPT_DWORD_MAX];
-};
-
 /* Fast Read settings. */
 
 static inline void
@@ -2595,6 +2612,19 @@ static void spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
 	map->uniform_erase_type = erase_mask;
 }
 
+static int
+spi_nor_post_bfpt_fixups(struct spi_nor *nor,
+			 const struct sfdp_parameter_header *bfpt_header,
+			 const struct sfdp_bfpt *bfpt,
+			 struct spi_nor_flash_parameter *params)
+{
+	if (nor->info->fixups && nor->info->fixups->post_bfpt)
+		return nor->info->fixups->post_bfpt(nor, bfpt_header, bfpt,
+						    params);
+
+	return 0;
+}
+
 /**
  * spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
  * @nor:		pointer to a 'struct spi_nor'
@@ -2747,7 +2777,8 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 
 	/* Stop here if not JESD216 rev A or later. */
 	if (bfpt_header->length < BFPT_DWORD_MAX)
-		return 0;
+		return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt,
+						params);
 
 	/* Page size: this field specifies 'N' so the page size = 2^N bytes. */
 	params->page_size = bfpt.dwords[BFPT_DWORD(11)];
@@ -2782,7 +2813,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 		return -EINVAL;
 	}
 
-	return 0;
+	return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt, params);
 }
 
 #define SMPT_CMD_ADDRESS_LEN_MASK		GENMASK(23, 22)
-- 
2.17.1

Re: [PATCH v5 2/3] mtd: spi-nor: Add a post BFPT parsing fixup hook

[Tudor.Ambarus]

On 12/06/2018 12:37 PM, Boris Brezillon wrote:
> Experience has proven that SFDP tables are sometimes wrong, and parsing
> of these broken tables can lead to erroneous flash config.
> 
> This leaves us 2 options:
> 
> 1/ set the SPI_NOR_SKIP_SFDP flag and completely ignore SFDP parsing
> 2/ fix things at runtime
> 
> While #1 should always work, it might imply extra work if most of the
> SFDP is correct. #2 has the benefit of keeping the generic SFDP parsing
> logic almost untouched while allowing SPI NOR manufacturer drivers to
> fix the broken bits.
> 
> Add a spi_nor_fixups struct where we'll put all our fixup hooks, each
> of them being called at a different point in the scan process.
> 
> We start a hook called just after the BFPT parsing to allow fixing up
> info extracted from the BFPT section. More hooks will be added if other
> sections need to be fixed.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

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

> ---
> Changes in v5:
> - None
> 
> Changes in v4:
> - New patch
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 387 ++++++++++++++++++----------------
>  1 file changed, 209 insertions(+), 178 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index bea122e0d731..7bc9887d9c0a 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -42,6 +42,196 @@
>  #define SPI_NOR_MAX_ID_LEN	6
>  #define SPI_NOR_MAX_ADDR_WIDTH	4
>  
> +struct spi_nor_read_command {
> +	u8			num_mode_clocks;
> +	u8			num_wait_states;
> +	u8			opcode;
> +	enum spi_nor_protocol	proto;
> +};
> +
> +struct spi_nor_pp_command {
> +	u8			opcode;
> +	enum spi_nor_protocol	proto;
> +};
> +
> +enum spi_nor_read_command_index {
> +	SNOR_CMD_READ,
> +	SNOR_CMD_READ_FAST,
> +	SNOR_CMD_READ_1_1_1_DTR,
> +
> +	/* Dual SPI */
> +	SNOR_CMD_READ_1_1_2,
> +	SNOR_CMD_READ_1_2_2,
> +	SNOR_CMD_READ_2_2_2,
> +	SNOR_CMD_READ_1_2_2_DTR,
> +
> +	/* Quad SPI */
> +	SNOR_CMD_READ_1_1_4,
> +	SNOR_CMD_READ_1_4_4,
> +	SNOR_CMD_READ_4_4_4,
> +	SNOR_CMD_READ_1_4_4_DTR,
> +
> +	/* Octo SPI */
> +	SNOR_CMD_READ_1_1_8,
> +	SNOR_CMD_READ_1_8_8,
> +	SNOR_CMD_READ_8_8_8,
> +	SNOR_CMD_READ_1_8_8_DTR,
> +
> +	SNOR_CMD_READ_MAX
> +};
> +
> +enum spi_nor_pp_command_index {
> +	SNOR_CMD_PP,
> +
> +	/* Quad SPI */
> +	SNOR_CMD_PP_1_1_4,
> +	SNOR_CMD_PP_1_4_4,
> +	SNOR_CMD_PP_4_4_4,
> +
> +	/* Octo SPI */
> +	SNOR_CMD_PP_1_1_8,
> +	SNOR_CMD_PP_1_8_8,
> +	SNOR_CMD_PP_8_8_8,
> +
> +	SNOR_CMD_PP_MAX
> +};
> +
> +struct spi_nor_flash_parameter {
> +	u64				size;
> +	u32				page_size;
> +
> +	struct spi_nor_hwcaps		hwcaps;
> +	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
> +	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
> +
> +	int (*quad_enable)(struct spi_nor *nor);
> +};
> +
> +struct sfdp_parameter_header {
> +	u8		id_lsb;
> +	u8		minor;
> +	u8		major;
> +	u8		length; /* in double words */
> +	u8		parameter_table_pointer[3]; /* byte address */
> +	u8		id_msb;
> +};
> +
> +#define SFDP_PARAM_HEADER_ID(p)	(((p)->id_msb << 8) | (p)->id_lsb)
> +#define SFDP_PARAM_HEADER_PTP(p) \
> +	(((p)->parameter_table_pointer[2] << 16) | \
> +	 ((p)->parameter_table_pointer[1] <<  8) | \
> +	 ((p)->parameter_table_pointer[0] <<  0))
> +
> +#define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
> +#define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
> +
> +#define SFDP_SIGNATURE		0x50444653U
> +#define SFDP_JESD216_MAJOR	1
> +#define SFDP_JESD216_MINOR	0
> +#define SFDP_JESD216A_MINOR	5
> +#define SFDP_JESD216B_MINOR	6
> +
> +struct sfdp_header {
> +	u32		signature; /* Ox50444653U <=> "SFDP" */
> +	u8		minor;
> +	u8		major;
> +	u8		nph; /* 0-base number of parameter headers */
> +	u8		unused;
> +
> +	/* Basic Flash Parameter Table. */
> +	struct sfdp_parameter_header	bfpt_header;
> +};
> +
> +/* Basic Flash Parameter Table */
> +
> +/*
> + * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
> + * They are indexed from 1 but C arrays are indexed from 0.
> + */
> +#define BFPT_DWORD(i)		((i) - 1)
> +#define BFPT_DWORD_MAX		16
> +
> +/* The first version of JESB216 defined only 9 DWORDs. */
> +#define BFPT_DWORD_MAX_JESD216			9
> +
> +/* 1st DWORD. */
> +#define BFPT_DWORD1_FAST_READ_1_1_2		BIT(16)
> +#define BFPT_DWORD1_ADDRESS_BYTES_MASK		GENMASK(18, 17)
> +#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY	(0x0UL << 17)
> +#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4	(0x1UL << 17)
> +#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY	(0x2UL << 17)
> +#define BFPT_DWORD1_DTR				BIT(19)
> +#define BFPT_DWORD1_FAST_READ_1_2_2		BIT(20)
> +#define BFPT_DWORD1_FAST_READ_1_4_4		BIT(21)
> +#define BFPT_DWORD1_FAST_READ_1_1_4		BIT(22)
> +
> +/* 5th DWORD. */
> +#define BFPT_DWORD5_FAST_READ_2_2_2		BIT(0)
> +#define BFPT_DWORD5_FAST_READ_4_4_4		BIT(4)
> +
> +/* 11th DWORD. */
> +#define BFPT_DWORD11_PAGE_SIZE_SHIFT		4
> +#define BFPT_DWORD11_PAGE_SIZE_MASK		GENMASK(7, 4)
> +
> +/* 15th DWORD. */
> +
> +/*
> + * (from JESD216 rev B)
> + * Quad Enable Requirements (QER):
> + * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-4
> + *         reads based on instruction. DQ3/HOLD# functions are hold during
> + *         instruction phase.
> + * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
> + *         two data bytes where bit 1 of the second byte is one.
> + *         [...]
> + *         Writing only one byte to the status register has the side-effect of
> + *         clearing status register 2, including the QE bit. The 100b code is
> + *         used if writing one byte to the status register does not modify
> + *         status register 2.
> + * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
> + *         one data byte where bit 6 is one.
> + *         [...]
> + * - 011b: QE is bit 7 of status register 2. It is set via Write status
> + *         register 2 instruction 3Eh with one data byte where bit 7 is one.
> + *         [...]
> + *         The status register 2 is read using instruction 3Fh.
> + * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
> + *         two data bytes where bit 1 of the second byte is one.
> + *         [...]
> + *         In contrast to the 001b code, writing one byte to the status
> + *         register does not modify status register 2.
> + * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
> + *         Read Status instruction 05h. Status register2 is read using
> + *         instruction 35h. QE is set via Writ Status instruction 01h with
> + *         two data bytes where bit 1 of the second byte is one.
> + *         [...]
> + */
> +#define BFPT_DWORD15_QER_MASK			GENMASK(22, 20)
> +#define BFPT_DWORD15_QER_NONE			(0x0UL << 20) /* Micron */
> +#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY		(0x1UL << 20)
> +#define BFPT_DWORD15_QER_SR1_BIT6		(0x2UL << 20) /* Macronix */
> +#define BFPT_DWORD15_QER_SR2_BIT7		(0x3UL << 20)
> +#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD		(0x4UL << 20)
> +#define BFPT_DWORD15_QER_SR2_BIT1		(0x5UL << 20) /* Spansion */
> +
> +struct sfdp_bfpt {
> +	u32	dwords[BFPT_DWORD_MAX];
> +};
> +
> +/**
> + * struct spi_nor_fixups - SPI NOR fixup hooks
> + * @post_bfpt: called after the BFPT table has been parsed
> + *
> + * Those hooks can be used to tweak the SPI NOR configuration when the SFDP
> + * table is broken or not available.
> + */
> +struct spi_nor_fixups {
> +	int (*post_bfpt)(struct spi_nor *nor,
> +			 const struct sfdp_parameter_header *bfpt_header,
> +			 const struct sfdp_bfpt *bfpt,
> +			 struct spi_nor_flash_parameter *params);
> +};
> +
>  struct flash_info {
>  	char		*name;
>  
> @@ -91,6 +281,9 @@ struct flash_info {
>  #define SPI_NOR_SKIP_SFDP	BIT(13)	/* Skip parsing of SFDP tables */
>  #define USE_CLSR		BIT(14)	/* use CLSR command */
>  
> +	/* Part specific fixup hooks. */
> +	const struct spi_nor_fixups *fixups;
> +
>  	int	(*quad_enable)(struct spi_nor *nor);
>  };
>  
> @@ -2076,71 +2269,6 @@ static int s3an_nor_scan(const struct flash_info *info, struct spi_nor *nor)
>  	return 0;
>  }
>  
> -struct spi_nor_read_command {
> -	u8			num_mode_clocks;
> -	u8			num_wait_states;
> -	u8			opcode;
> -	enum spi_nor_protocol	proto;
> -};
> -
> -struct spi_nor_pp_command {
> -	u8			opcode;
> -	enum spi_nor_protocol	proto;
> -};
> -
> -enum spi_nor_read_command_index {
> -	SNOR_CMD_READ,
> -	SNOR_CMD_READ_FAST,
> -	SNOR_CMD_READ_1_1_1_DTR,
> -
> -	/* Dual SPI */
> -	SNOR_CMD_READ_1_1_2,
> -	SNOR_CMD_READ_1_2_2,
> -	SNOR_CMD_READ_2_2_2,
> -	SNOR_CMD_READ_1_2_2_DTR,
> -
> -	/* Quad SPI */
> -	SNOR_CMD_READ_1_1_4,
> -	SNOR_CMD_READ_1_4_4,
> -	SNOR_CMD_READ_4_4_4,
> -	SNOR_CMD_READ_1_4_4_DTR,
> -
> -	/* Octo SPI */
> -	SNOR_CMD_READ_1_1_8,
> -	SNOR_CMD_READ_1_8_8,
> -	SNOR_CMD_READ_8_8_8,
> -	SNOR_CMD_READ_1_8_8_DTR,
> -
> -	SNOR_CMD_READ_MAX
> -};
> -
> -enum spi_nor_pp_command_index {
> -	SNOR_CMD_PP,
> -
> -	/* Quad SPI */
> -	SNOR_CMD_PP_1_1_4,
> -	SNOR_CMD_PP_1_4_4,
> -	SNOR_CMD_PP_4_4_4,
> -
> -	/* Octo SPI */
> -	SNOR_CMD_PP_1_1_8,
> -	SNOR_CMD_PP_1_8_8,
> -	SNOR_CMD_PP_8_8_8,
> -
> -	SNOR_CMD_PP_MAX
> -};
> -
> -struct spi_nor_flash_parameter {
> -	u64				size;
> -	u32				page_size;
> -
> -	struct spi_nor_hwcaps		hwcaps;
> -	struct spi_nor_read_command	reads[SNOR_CMD_READ_MAX];
> -	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
> -
> -	int (*quad_enable)(struct spi_nor *nor);
> -};
> -
>  static void
>  spi_nor_set_read_settings(struct spi_nor_read_command *read,
>  			  u8 num_mode_clocks,
> @@ -2263,117 +2391,6 @@ static int spi_nor_read_sfdp_dma_unsafe(struct spi_nor *nor, u32 addr,
>  	return ret;
>  }
>  
> -struct sfdp_parameter_header {
> -	u8		id_lsb;
> -	u8		minor;
> -	u8		major;
> -	u8		length; /* in double words */
> -	u8		parameter_table_pointer[3]; /* byte address */
> -	u8		id_msb;
> -};
> -
> -#define SFDP_PARAM_HEADER_ID(p)	(((p)->id_msb << 8) | (p)->id_lsb)
> -#define SFDP_PARAM_HEADER_PTP(p) \
> -	(((p)->parameter_table_pointer[2] << 16) | \
> -	 ((p)->parameter_table_pointer[1] <<  8) | \
> -	 ((p)->parameter_table_pointer[0] <<  0))
> -
> -#define SFDP_BFPT_ID		0xff00	/* Basic Flash Parameter Table */
> -#define SFDP_SECTOR_MAP_ID	0xff81	/* Sector Map Table */
> -
> -#define SFDP_SIGNATURE		0x50444653U
> -#define SFDP_JESD216_MAJOR	1
> -#define SFDP_JESD216_MINOR	0
> -#define SFDP_JESD216A_MINOR	5
> -#define SFDP_JESD216B_MINOR	6
> -
> -struct sfdp_header {
> -	u32		signature; /* Ox50444653U <=> "SFDP" */
> -	u8		minor;
> -	u8		major;
> -	u8		nph; /* 0-base number of parameter headers */
> -	u8		unused;
> -
> -	/* Basic Flash Parameter Table. */
> -	struct sfdp_parameter_header	bfpt_header;
> -};
> -
> -/* Basic Flash Parameter Table */
> -
> -/*
> - * JESD216 rev B defines a Basic Flash Parameter Table of 16 DWORDs.
> - * They are indexed from 1 but C arrays are indexed from 0.
> - */
> -#define BFPT_DWORD(i)		((i) - 1)
> -#define BFPT_DWORD_MAX		16
> -
> -/* The first version of JESB216 defined only 9 DWORDs. */
> -#define BFPT_DWORD_MAX_JESD216			9
> -
> -/* 1st DWORD. */
> -#define BFPT_DWORD1_FAST_READ_1_1_2		BIT(16)
> -#define BFPT_DWORD1_ADDRESS_BYTES_MASK		GENMASK(18, 17)
> -#define BFPT_DWORD1_ADDRESS_BYTES_3_ONLY	(0x0UL << 17)
> -#define BFPT_DWORD1_ADDRESS_BYTES_3_OR_4	(0x1UL << 17)
> -#define BFPT_DWORD1_ADDRESS_BYTES_4_ONLY	(0x2UL << 17)
> -#define BFPT_DWORD1_DTR				BIT(19)
> -#define BFPT_DWORD1_FAST_READ_1_2_2		BIT(20)
> -#define BFPT_DWORD1_FAST_READ_1_4_4		BIT(21)
> -#define BFPT_DWORD1_FAST_READ_1_1_4		BIT(22)
> -
> -/* 5th DWORD. */
> -#define BFPT_DWORD5_FAST_READ_2_2_2		BIT(0)
> -#define BFPT_DWORD5_FAST_READ_4_4_4		BIT(4)
> -
> -/* 11th DWORD. */
> -#define BFPT_DWORD11_PAGE_SIZE_SHIFT		4
> -#define BFPT_DWORD11_PAGE_SIZE_MASK		GENMASK(7, 4)
> -
> -/* 15th DWORD. */
> -
> -/*
> - * (from JESD216 rev B)
> - * Quad Enable Requirements (QER):
> - * - 000b: Device does not have a QE bit. Device detects 1-1-4 and 1-4-4
> - *         reads based on instruction. DQ3/HOLD# functions are hold during
> - *         instruction phase.
> - * - 001b: QE is bit 1 of status register 2. It is set via Write Status with
> - *         two data bytes where bit 1 of the second byte is one.
> - *         [...]
> - *         Writing only one byte to the status register has the side-effect of
> - *         clearing status register 2, including the QE bit. The 100b code is
> - *         used if writing one byte to the status register does not modify
> - *         status register 2.
> - * - 010b: QE is bit 6 of status register 1. It is set via Write Status with
> - *         one data byte where bit 6 is one.
> - *         [...]
> - * - 011b: QE is bit 7 of status register 2. It is set via Write status
> - *         register 2 instruction 3Eh with one data byte where bit 7 is one.
> - *         [...]
> - *         The status register 2 is read using instruction 3Fh.
> - * - 100b: QE is bit 1 of status register 2. It is set via Write Status with
> - *         two data bytes where bit 1 of the second byte is one.
> - *         [...]
> - *         In contrast to the 001b code, writing one byte to the status
> - *         register does not modify status register 2.
> - * - 101b: QE is bit 1 of status register 2. Status register 1 is read using
> - *         Read Status instruction 05h. Status register2 is read using
> - *         instruction 35h. QE is set via Writ Status instruction 01h with
> - *         two data bytes where bit 1 of the second byte is one.
> - *         [...]
> - */
> -#define BFPT_DWORD15_QER_MASK			GENMASK(22, 20)
> -#define BFPT_DWORD15_QER_NONE			(0x0UL << 20) /* Micron */
> -#define BFPT_DWORD15_QER_SR2_BIT1_BUGGY		(0x1UL << 20)
> -#define BFPT_DWORD15_QER_SR1_BIT6		(0x2UL << 20) /* Macronix */
> -#define BFPT_DWORD15_QER_SR2_BIT7		(0x3UL << 20)
> -#define BFPT_DWORD15_QER_SR2_BIT1_NO_RD		(0x4UL << 20)
> -#define BFPT_DWORD15_QER_SR2_BIT1		(0x5UL << 20) /* Spansion */
> -
> -struct sfdp_bfpt {
> -	u32	dwords[BFPT_DWORD_MAX];
> -};
> -
>  /* Fast Read settings. */
>  
>  static inline void
> @@ -2595,6 +2612,19 @@ static void spi_nor_init_uniform_erase_map(struct spi_nor_erase_map *map,
>  	map->uniform_erase_type = erase_mask;
>  }
>  
> +static int
> +spi_nor_post_bfpt_fixups(struct spi_nor *nor,
> +			 const struct sfdp_parameter_header *bfpt_header,
> +			 const struct sfdp_bfpt *bfpt,
> +			 struct spi_nor_flash_parameter *params)
> +{
> +	if (nor->info->fixups && nor->info->fixups->post_bfpt)
> +		return nor->info->fixups->post_bfpt(nor, bfpt_header, bfpt,
> +						    params);
> +
> +	return 0;
> +}
> +
>  /**
>   * spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
>   * @nor:		pointer to a 'struct spi_nor'
> @@ -2747,7 +2777,8 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  
>  	/* Stop here if not JESD216 rev A or later. */
>  	if (bfpt_header->length < BFPT_DWORD_MAX)
> -		return 0;
> +		return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt,
> +						params);
>  
>  	/* Page size: this field specifies 'N' so the page size = 2^N bytes. */
>  	params->page_size = bfpt.dwords[BFPT_DWORD(11)];
> @@ -2782,7 +2813,7 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  		return -EINVAL;
>  	}
>  
> -	return 0;
> +	return spi_nor_post_bfpt_fixups(nor, bfpt_header, &bfpt, params);
>  }
>  
>  #define SMPT_CMD_ADDRESS_LEN_MASK		GENMASK(23, 22)
> 

Re: [v5,2/3] mtd: spi-nor: Add a post BFPT parsing fixup hook

[Boris Brezillon]

On Thu, 2018-12-06 at 10:37:35 UTC, Boris Brezillon wrote:
> Experience has proven that SFDP tables are sometimes wrong, and parsing
> of these broken tables can lead to erroneous flash config.
> 
> This leaves us 2 options:
> 
> 1/ set the SPI_NOR_SKIP_SFDP flag and completely ignore SFDP parsing
> 2/ fix things at runtime
> 
> While #1 should always work, it might imply extra work if most of the
> SFDP is correct. #2 has the benefit of keeping the generic SFDP parsing
> logic almost untouched while allowing SPI NOR manufacturer drivers to
> fix the broken bits.
> 
> Add a spi_nor_fixups struct where we'll put all our fixup hooks, each
> of them being called at a different point in the scan process.
> 
> We start a hook called just after the BFPT parsing to allow fixing up
> info extracted from the BFPT section. More hooks will be added if other
> sections need to be fixed.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to http://git.infradead.org/linux-mtd.git spi-nor/next.

Boris

[PATCH v5 3/3] mtd: spi-nor: Add a post BFPT fixup for MX25L25635E

[Boris Brezillon]

MX25L25635F and MX25L25635E share the same JEDEC-ID, but the F variant
supports 4-byte opcodes while the E variant doesn't. We need a way to
differentiate those 2 chips and set the SNOR_F_4B_OPCODES flag only for
the F variant.

Luckily, 4-byte opcode support is not the only difference: Fast Read
4-4-4 is only supported by the F variant, and this feature is
advertised in the BFPT table. Use this to decide when to set the
SNOR_F_4B_OPCODES flag.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
Changes in v5:
- Follow the flash_info entry format

Changes in v4:
- New patch
---
 drivers/mtd/spi-nor/spi-nor.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 7bc9887d9c0a..fdff18cb1196 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1447,6 +1447,31 @@ static int macronix_quad_enable(struct spi_nor *nor);
 		.addr_width = 3,					\
 		.flags = SPI_NOR_NO_FR | SPI_S3AN,
 
+static int
+mx25l25635_post_bfpt_fixups(struct spi_nor *nor,
+			    const struct sfdp_parameter_header *bfpt_header,
+			    const struct sfdp_bfpt *bfpt,
+			    struct spi_nor_flash_parameter *params)
+{
+	/*
+	 * MX25L25635F support 4B opcodes but MX25L25635E does not.
+	 * Unfortunately, Macronix has re-used the same JEDEC ID for both the
+	 * variants which prevents us from defining a new entry in the parts
+	 * table.
+	 * We need a way to differentiate MX25L25635E and MX25L25635F, and it
+	 * seems that the F version advertises support for Fast Read 4-4-4 in
+	 * its BFPT table.
+	 */
+	if (bfpt->dwords[BFPT_DWORD(5)] & BFPT_DWORD5_FAST_READ_4_4_4)
+		nor->flags |= SNOR_F_4B_OPCODES;
+
+	return 0;
+}
+
+static struct spi_nor_fixups mx25l25635_fixups = {
+	.post_bfpt = mx25l25635_post_bfpt_fixups,
+};
+
 /* NOTE: double check command sets and memory organization when you add
  * more nor chips.  This current list focusses on newer chips, which
  * have been converging on command sets which including JEDEC ID.
@@ -1581,7 +1606,9 @@ static const struct flash_info spi_nor_ids[] = {
 	{ "mx25l12855e", INFO(0xc22618, 0, 64 * 1024, 256, 0) },
 	{ "mx25u12835f", INFO(0xc22538, 0, 64 * 1024, 256,
 			 SECT_4K | SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
-	{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ) },
+	{ "mx25l25635e", INFO(0xc22019, 0, 64 * 1024, 512,
+			 SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ)
+			 .fixups = &mx25l25635_fixups },
 	{ "mx25u25635f", INFO(0xc22539, 0, 64 * 1024, 512, SECT_4K | SPI_NOR_4B_OPCODES) },
 	{ "mx25l25655e", INFO(0xc22619, 0, 64 * 1024, 512, 0) },
 	{ "mx66l51235l", INFO(0xc2201a, 0, 64 * 1024, 1024, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_4B_OPCODES) },
-- 
2.17.1

Re: [PATCH v5 3/3] mtd: spi-nor: Add a post BFPT fixup for MX25L25635E

[Tudor.Ambarus]

On 12/06/2018 12:37 PM, Boris Brezillon wrote:
> +	 * MX25L25635F support 4B opcodes but MX25L25635E does not.

s/support/supports

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

Re: [v5,3/3] mtd: spi-nor: Add a post BFPT fixup for MX25L25635E

[Boris Brezillon]

On Thu, 2018-12-06 at 10:37:36 UTC, Boris Brezillon wrote:
> MX25L25635F and MX25L25635E share the same JEDEC-ID, but the F variant
> supports 4-byte opcodes while the E variant doesn't. We need a way to
> differentiate those 2 chips and set the SNOR_F_4B_OPCODES flag only for
> the F variant.
> 
> Luckily, 4-byte opcode support is not the only difference: Fast Read
> 4-4-4 is only supported by the F variant, and this feature is
> advertised in the BFPT table. Use this to decide when to set the
> SNOR_F_4B_OPCODES flag.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Reviewed-by: Tudor Ambarus <tudor.ambarus@microchip.com>

Applied to http://git.infradead.org/linux-mtd.git spi-nor/next.

Boris