Discussion:
[PATCH v4 0/3] mtd: spi-nor: mx25l25635f: Use 4B opcodes
Boris Brezillon
2018-11-29 14:41:40 UTC
Permalink
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

P.S.: This series depends on [1]

[1]http://patchwork.ozlabs.org/project/linux-mtd/list/?series=78826

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 | 441 +++++++++++++++++++---------------
include/linux/mtd/spi-nor.h | 1 +
2 files changed, 253 insertions(+), 189 deletions(-)
--
2.17.1
Boris Brezillon
2018-11-29 14:41:42 UTC
Permalink
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 <***@bootlin.com>
---
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 98b8d9a778aa..316cd0287194 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -39,6 +39,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;

@@ -88,6 +278,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);
};

@@ -2066,71 +2259,6 @@ static int s3an_nor_scan(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,
@@ -2253,117 +2381,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 void
@@ -2634,6 +2651,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'
@@ -2786,7 +2816,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)];
@@ -2821,7 +2852,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
T***@microchip.com
2018-12-05 16:35:15 UTC
Permalink
Post by Boris Brezillon
+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);
+};
I like the idea. The code looks good. What I noticed is that the sfdp logic is
now split in different sections of spi-nor file. In order to avoid the burden of
moving the sfdp code inside spi-nor, you can have these 3 forward declarations
and do the sfdp move later on, in a separate file.

ta
Boris Brezillon
2018-12-05 16:47:53 UTC
Permalink
On Wed, 5 Dec 2018 16:35:15 +0000
Post by T***@microchip.com
Post by Boris Brezillon
+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);
+};
I like the idea. The code looks good. What I noticed is that the sfdp logic is
now split in different sections of spi-nor file. In order to avoid the burden of
moving the sfdp code inside spi-nor, you can have these 3 forward declarations
and do the sfdp move later on, in a separate file.
Hm, okay. People (me included) tend to forget about those forward decls
when moving things around, which is why I try to avoid using them when
unnecessary.
Boris Brezillon
2018-12-06 10:25:02 UTC
Permalink
On Wed, 5 Dec 2018 16:35:15 +0000
Post by T***@microchip.com
Post by Boris Brezillon
+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);
+};
I like the idea. The code looks good. What I noticed is that the sfdp logic is
now split in different sections of spi-nor file. In order to avoid the burden of
moving the sfdp code inside spi-nor, you can have these 3 forward declarations
and do the sfdp move later on, in a separate file.
I tried that, but then I need some of those definitions in my
mx25l25635_post_bfpt_fixups(), so I'm screwed anyway. Also, it's not
really a matter to have SFDP split in 2 sections as we're going to move
all the code in a separate file soon ;-).

Boris Brezillon
2018-11-29 14:41:41 UTC
Permalink
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.

Reported-by: Alexandre Belloni <***@bootlin.com>
Signed-off-by: Boris Brezillon <***@bootlin.com>
Tested-by: Alexandre Belloni <***@bootlin.com>
Reviewed-by: Tudor Ambarus <***@microchip.com>
---
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 | 22 ++++++++++++----------
include/linux/mtd/spi-nor.h | 1 +
2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c1d9c2e50bee..98b8d9a778aa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3311,6 +3311,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));
@@ -3561,9 +3562,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
@@ -3595,10 +3594,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, false);
}
EXPORT_SYMBOL_GPL(spi_nor_restore);
@@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (info->flags & SPI_NOR_NO_FR)
params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;

+ if (info->flags & SPI_NOR_4B_OPCODES ||
+ (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
+ nor->flags |= SNOR_F_4B_OPCODES;
+
/*
* Configure the SPI memory:
* - select op codes for (Fast) Read, Page Program and Sector Erase.
@@ -3768,13 +3769,14 @@ 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);
} else {
nor->addr_width = 3;
}

+ if (nor->addr_width == 4 &&
+ nor->flags & SNOR_F_4B_OPCODES)
+ spi_nor_set_4byte_opcodes(nor);
+
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
T***@microchip.com
2018-12-05 15:08:49 UTC
Permalink
Hi, Boris,
Post by 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.
---
- 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
and you got rid of the superfluous check "(JEDEC_MFR(nor->info) !=
SNOR_MFR_SPANSION)", which is good, thanks.

And I guess you should have dropped my rb tag, because I haven't reviewed v4
yet. No worries, just saying ...
Post by Boris Brezillon
- Clear SNOR_F_4B_OPCODES flag when SFDP fails
- Add Alexandre R-b
- 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 | 22 ++++++++++++----------
include/linux/mtd/spi-nor.h | 1 +
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c1d9c2e50bee..98b8d9a778aa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3311,6 +3311,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));
@@ -3561,9 +3562,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
@@ -3595,10 +3594,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, false);
}
EXPORT_SYMBOL_GPL(spi_nor_restore);
@@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (info->flags & SPI_NOR_NO_FR)
params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
+ if (info->flags & SPI_NOR_4B_OPCODES ||
+ (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
+ nor->flags |= SNOR_F_4B_OPCODES;
+
you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I
suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)"
block.
Post by Boris Brezillon
/*
* - select op codes for (Fast) Read, Page Program and Sector Erase.
@@ -3768,13 +3769,14 @@ 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);
} else {
nor->addr_width = 3;
}
+ if (nor->addr_width == 4 &&
+ nor->flags & SNOR_F_4B_OPCODES)
the conditions fit in a single line

Cheers,
ta
Boris Brezillon
2018-12-05 15:19:02 UTC
Permalink
On Wed, 5 Dec 2018 15:08:49 +0000
Post by T***@microchip.com
Hi, Boris,
Post by 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.
---
- 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
and you got rid of the superfluous check "(JEDEC_MFR(nor->info) !=
SNOR_MFR_SPANSION)", which is good, thanks.
And I guess you should have dropped my rb tag, because I haven't reviewed v4
yet. No worries, just saying ...
Yes, my bad.
Post by T***@microchip.com
Post by Boris Brezillon
- Clear SNOR_F_4B_OPCODES flag when SFDP fails
- Add Alexandre R-b
- 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 | 22 ++++++++++++----------
include/linux/mtd/spi-nor.h | 1 +
2 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c1d9c2e50bee..98b8d9a778aa 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -3311,6 +3311,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));
@@ -3561,9 +3562,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
@@ -3595,10 +3594,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, false);
}
EXPORT_SYMBOL_GPL(spi_nor_restore);
@@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (info->flags & SPI_NOR_NO_FR)
params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
+ if (info->flags & SPI_NOR_4B_OPCODES ||
+ (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
+ nor->flags |= SNOR_F_4B_OPCODES;
+
you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I
suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)"
block.
Shouldn't we override this value anyway? I mean, I thought flash_info
flags had precedence on the SFDP ones. Also, just because the flash is
smaller than 16MB, doesn't mean it does not support 4B opcodes. We
probably won't use the 4B opcodes in that case, but still.
Post by T***@microchip.com
Post by Boris Brezillon
/*
* - select op codes for (Fast) Read, Page Program and Sector Erase.
@@ -3768,13 +3769,14 @@ 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);
} else {
nor->addr_width = 3;
}
+ if (nor->addr_width == 4 &&
+ nor->flags & SNOR_F_4B_OPCODES)
the conditions fit in a single line
Will fix that.
T***@microchip.com
2018-12-05 15:48:46 UTC
Permalink
Post by Boris Brezillon
Post by T***@microchip.com
Post by Boris Brezillon
@@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (info->flags & SPI_NOR_NO_FR)
params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
+ if (info->flags & SPI_NOR_4B_OPCODES ||
+ (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
+ nor->flags |= SNOR_F_4B_OPCODES;
+
you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I
suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)"
block.
Shouldn't we override this value anyway? I mean, I thought flash_info
flags had precedence on the SFDP ones. Also, just because the flash is
I tend to say that we shouldn't. We have some "JEDEC knows better" attitude in
the code: we are overwriting platform ID if we find a different ID in sfdp, we
choose addr_width from SFDP even if set in info->addr_width, and we are
overwriting all the settings based on flash_info when sfdp parsing succeeds in
spi_nor_init_params().
Post by Boris Brezillon
smaller than 16MB, doesn't mean it does not support 4B opcodes. We
probably won't use the 4B opcodes in that case, but still.
I agree that manufacturers have a sense of humor and this might be possible. But
there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help
here too.

Cheers,
ta
Boris Brezillon
2018-12-05 16:00:25 UTC
Permalink
On Wed, 5 Dec 2018 15:48:46 +0000
Post by T***@microchip.com
Post by Boris Brezillon
Post by T***@microchip.com
Post by Boris Brezillon
@@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (info->flags & SPI_NOR_NO_FR)
params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
+ if (info->flags & SPI_NOR_4B_OPCODES ||
+ (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
+ nor->flags |= SNOR_F_4B_OPCODES;
+
you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I
suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)"
block.
Shouldn't we override this value anyway? I mean, I thought flash_info
flags had precedence on the SFDP ones. Also, just because the flash is
I tend to say that we shouldn't. We have some "JEDEC knows better" attitude in
the code: we are overwriting platform ID if we find a different ID in sfdp, we
choose addr_width from SFDP even if set in info->addr_width, and we are
overwriting all the settings based on flash_info when sfdp parsing succeeds in
spi_nor_init_params().
Given all the "broken SFDP" problems we had, I'm not sure this was the
right decision, but that's another topic.

For this specific one, I'd really prefer to keep this code as is. Note
that the "JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M"
is later moved to a post SFDP fixup hook in my rework, which means we'll
anyway override the decision taken by the SFDP parsing.
Post by T***@microchip.com
Post by Boris Brezillon
smaller than 16MB, doesn't mean it does not support 4B opcodes. We
probably won't use the 4B opcodes in that case, but still.
I agree that manufacturers have a sense of humor and this might be possible. But
there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help
here too.
Except there's nothing to fix in this case, we just won't use 4B
opcodes if we don't need to, that's all.
T***@microchip.com
2018-12-05 16:26:06 UTC
Permalink
Post by Boris Brezillon
On Wed, 5 Dec 2018 15:48:46 +0000
Post by T***@microchip.com
Post by Boris Brezillon
Post by T***@microchip.com
Post by Boris Brezillon
@@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (info->flags & SPI_NOR_NO_FR)
params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
+ if (info->flags & SPI_NOR_4B_OPCODES ||
+ (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
+ nor->flags |= SNOR_F_4B_OPCODES;
+
you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I
suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)"
block.
Shouldn't we override this value anyway? I mean, I thought flash_info
flags had precedence on the SFDP ones. Also, just because the flash is
I tend to say that we shouldn't. We have some "JEDEC knows better" attitude in
the code: we are overwriting platform ID if we find a different ID in sfdp, we
choose addr_width from SFDP even if set in info->addr_width, and we are
overwriting all the settings based on flash_info when sfdp parsing succeeds in
spi_nor_init_params().
Given all the "broken SFDP" problems we had, I'm not sure this was the
right decision, but that's another topic.
For this specific one, I'd really prefer to keep this code as is. Note
that the "JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M"
is later moved to a post SFDP fixup hook in my rework, which means we'll
anyway override the decision taken by the SFDP parsing.
Post by T***@microchip.com
Post by Boris Brezillon
smaller than 16MB, doesn't mean it does not support 4B opcodes. We
probably won't use the 4B opcodes in that case, but still.
I agree that manufacturers have a sense of humor and this might be possible. But
there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help
here too.
Except there's nothing to fix in this case, we just won't use 4B
opcodes if we don't need to, that's all.
you'll have an extra byte of address that has a tiny impact on performance on
small requests. I see it as a fix, we should do what's best to do. anyway ...
Boris Brezillon
2018-12-05 16:32:54 UTC
Permalink
On Wed, 5 Dec 2018 16:26:06 +0000
Post by T***@microchip.com
Post by Boris Brezillon
On Wed, 5 Dec 2018 15:48:46 +0000
Post by T***@microchip.com
Post by Boris Brezillon
Post by T***@microchip.com
Post by Boris Brezillon
@@ -3750,6 +3747,10 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
if (info->flags & SPI_NOR_NO_FR)
params.hwcaps.mask &= ~SNOR_HWCAPS_READ_FAST;
+ if (info->flags & SPI_NOR_4B_OPCODES ||
+ (JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M))
+ nor->flags |= SNOR_F_4B_OPCODES;
+
you are potentially overwriting the SNOR_F_4B_OPCODES that may be set in SFDP. I
suggest to set SNOR_F_4B_OPCODES flag inside of the "if (mtd->size > 0x1000000)"
block.
Shouldn't we override this value anyway? I mean, I thought flash_info
flags had precedence on the SFDP ones. Also, just because the flash is
I tend to say that we shouldn't. We have some "JEDEC knows better" attitude in
the code: we are overwriting platform ID if we find a different ID in sfdp, we
choose addr_width from SFDP even if set in info->addr_width, and we are
overwriting all the settings based on flash_info when sfdp parsing succeeds in
spi_nor_init_params().
Given all the "broken SFDP" problems we had, I'm not sure this was the
right decision, but that's another topic.
For this specific one, I'd really prefer to keep this code as is. Note
that the "JEDEC_MFR(info) == SNOR_MFR_SPANSION && mtd->size > SZ_16M"
is later moved to a post SFDP fixup hook in my rework, which means we'll
anyway override the decision taken by the SFDP parsing.
Post by T***@microchip.com
Post by Boris Brezillon
smaller than 16MB, doesn't mean it does not support 4B opcodes. We
probably won't use the 4B opcodes in that case, but still.
I agree that manufacturers have a sense of humor and this might be possible. But
there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help
here too.
Except there's nothing to fix in this case, we just won't use 4B
opcodes if we don't need to, that's all.
you'll have an extra byte of address that has a tiny impact on performance on
small requests. I see it as a fix, we should do what's best to do. anyway ...
No, see the checks that are done in this patch: to use 4B_CODES the
flag should be set and the NOR should be larger than 16MB. The flag
does not mean "use 4B opcodes", it meas "4B opcodes are supported".
Boris Brezillon
2018-12-05 16:54:26 UTC
Permalink
On Wed, 5 Dec 2018 17:32:54 +0100
Post by Boris Brezillon
Post by T***@microchip.com
Post by Boris Brezillon
Post by T***@microchip.com
Post by Boris Brezillon
smaller than 16MB, doesn't mean it does not support 4B opcodes. We
probably won't use the 4B opcodes in that case, but still.
I agree that manufacturers have a sense of humor and this might be possible. But
there's no need to use 4B opcodes in this case, so a post_sfdp fixup will help
here too.
Except there's nothing to fix in this case, we just won't use 4B
opcodes if we don't need to, that's all.
you'll have an extra byte of address that has a tiny impact on performance on
small requests. I see it as a fix, we should do what's best to do. anyway ...
No, see the checks that are done in this patch: to use 4B_CODES the
flag should be set and the NOR should be larger than 16MB. The flag
does not mean "use 4B opcodes", it meas "4B opMcodes are supported".
Maybe I should rename the flag SNOR_F_SUPPORTS_4B_OPCODES to make it
clear.
Boris Brezillon
2018-11-29 14:41:43 UTC
Permalink
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 <***@bootlin.com>
---
Changes in v4:
- New patch
---
drivers/mtd/spi-nor/spi-nor.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 316cd0287194..dd2a8a4f703c 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1477,6 +1477,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.
@@ -1607,7 +1632,12 @@ 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
T***@microchip.com
2018-12-05 16:41:05 UTC
Permalink
Post by 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.
Looks good. I'm not very keen of seeing yet another style when declaring spi nor
ids, but I can live with it.

ta
Boris Brezillon
2018-12-05 16:50:46 UTC
Permalink
On Wed, 5 Dec 2018 16:41:05 +0000
Post by T***@microchip.com
Post by 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.
Looks good. I'm not very keen of seeing yet another style when declaring spi nor
ids, but I can live with it.
Oops. Will fix that in v2.
Loading...