[PATCH v2 0/4] mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode

[PATCH v2 0/4] mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode

[Tudor Ambarus]

Get the 4-Byte Address mode method from BFPT and favor it in the detriment
of the "default" set_4byte_addr_mode method or the methods set by vendors.
This may introduce some regressions if flashes have wrong BFPT data. The
fix is to introduce post_bfpt() hooks and fix where needed. We should let
the core/sfdp do the params initialization, and do vendor specific updates
just where needed.

This patch set depends on:
https://lore.kernel.org/lkml/20220304093011.198173-1-tudor.ambarus@microchip.com/
which depends on:
https://lore.kernel.org/lkml/20220228111712.111737-1-tudor.ambarus@microchip.com/

You can find a branch containing the entire chain at:
git@github.com:ambarus/linux-0day.git spi-nor/next-bfpt-4byte-addr

v2: drop quad enable patch

Tudor Ambarus (4):
  mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode methods
  mtd: spi-nor: Update name and description of the set_4byte_addr_mode
    BFPT methods
  mtd: spi-nor: Favor the BFPT-parsed set_4byte_addr_mode method
  mtd: spi-nor: sfdp: Keep SFDP definitions private

 drivers/mtd/spi-nor/core.c      |  70 +----------
 drivers/mtd/spi-nor/core.h      |   1 -
 drivers/mtd/spi-nor/macronix.c  |   9 +-
 drivers/mtd/spi-nor/micron-st.c |  32 +----
 drivers/mtd/spi-nor/sfdp.c      | 199 ++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/sfdp.h      |  57 +--------
 drivers/mtd/spi-nor/winbond.c   |  18 +--
 7 files changed, 234 insertions(+), 152 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode methods

[Tudor Ambarus]

BFPT[DWORD(16)] defines the methods to enter and exit the 4-Byte Address
Mode. Parse BFPT to determine the method. Will rename the methods with
generic names in a further patch, to keep things trackable in this one.

Some regressions may be introduced by this patch, because the
params->set_4byte_addr_mode method that was set either in
spi_nor_init_default_params() or later overwritten in default_init() hooks,
may now be overwritten with a different value based on the BFPT data. If
that's the case, the fix is to introduce a post_bfpt fixup hook where one
should fix the wrong BFPT info.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c      |  63 ---------------
 drivers/mtd/spi-nor/core.h      |   1 -
 drivers/mtd/spi-nor/micron-st.c |  24 ------
 drivers/mtd/spi-nor/sfdp.c      | 139 ++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/sfdp.h      |   3 +
 drivers/mtd/spi-nor/winbond.c   |  16 ++--
 6 files changed, 152 insertions(+), 94 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 98cda4063c07..452d0f91a8df 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -506,69 +506,6 @@ int spi_nor_read_cr(struct spi_nor *nor, u8 *cr)
 	return ret;
 }
 
-/**
- * spi_nor_set_4byte_addr_mode() - Enter/Exit 4-byte address mode.
- * @nor:	pointer to 'struct spi_nor'.
- * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
- *		address mode.
- *
- * Return: 0 on success, -errno otherwise.
- */
-int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
-{
-	int ret;
-
-	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_EN4B_EX4B_OP(enable);
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_write_reg(nor,
-						       enable ? SPINOR_OP_EN4B :
-								SPINOR_OP_EX4B,
-						       NULL, 0);
-	}
-
-	if (ret)
-		dev_dbg(nor->dev, "error %d setting 4-byte mode\n", ret);
-
-	return ret;
-}
-
-/**
- * spansion_set_4byte_addr_mode() - Set 4-byte address mode for Spansion
- * flashes.
- * @nor:	pointer to 'struct spi_nor'.
- * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
- *		address mode.
- *
- * Return: 0 on success, -errno otherwise.
- */
-static int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
-{
-	int ret;
-
-	nor->bouncebuf[0] = enable << 7;
-
-	if (nor->spimem) {
-		struct spi_mem_op op = SPI_NOR_BRWR_OP(nor->bouncebuf);
-
-		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
-
-		ret = spi_mem_exec_op(nor->spimem, &op);
-	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_BRWR,
-						       nor->bouncebuf, 1);
-	}
-
-	if (ret)
-		dev_dbg(nor->dev, "error %d setting 4-byte mode\n", ret);
-
-	return ret;
-}
-
 /**
  * spi_nor_write_ear() - Write Extended Address Register.
  * @nor:	pointer to 'struct spi_nor'.
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 8b7e597fd38c..c83d5e75c563 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -634,7 +634,6 @@ void spi_nor_spimem_setup_op(const struct spi_nor *nor,
 			     const enum spi_nor_protocol proto);
 int spi_nor_write_enable(struct spi_nor *nor);
 int spi_nor_write_disable(struct spi_nor *nor);
-int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
 int spi_nor_write_ear(struct spi_nor *nor, u8 ear);
 int spi_nor_wait_till_ready(struct spi_nor *nor);
 int spi_nor_global_block_unlock(struct spi_nor *nor);
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index dcdf461e2b53..c348419d24a0 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -294,30 +294,6 @@ static const struct flash_info st_nor_parts[] = {
 	{ "m25px80",    INFO(0x207114,  0, 64 * 1024, 16) },
 };
 
-/**
- * micron_st_nor_set_4byte_addr_mode() - Set 4-byte address mode for ST and
- * Micron flashes.
- * @nor:	pointer to 'struct spi_nor'.
- * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
- *		address mode.
- *
- * Return: 0 on success, -errno otherwise.
- */
-static int micron_st_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
-{
-	int ret;
-
-	ret = spi_nor_write_enable(nor);
-	if (ret)
-		return ret;
-
-	ret = spi_nor_set_4byte_addr_mode(nor, enable);
-	if (ret)
-		return ret;
-
-	return spi_nor_write_disable(nor);
-}
-
 /**
  * micron_st_nor_read_fsr() - Read the Flag Status Register.
  * @nor:	pointer to 'struct spi_nor'
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index a5211543d30d..108a74ce38e0 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -401,6 +401,127 @@ static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
 	}
 }
 
+/**
+ * spansion_set_4byte_addr_mode() - Set 4-byte address mode for Spansion
+ * flashes.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
+ *		address mode.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
+{
+	int ret;
+
+	nor->bouncebuf[0] = enable << 7;
+
+	if (nor->spimem) {
+		struct spi_mem_op op = SPI_NOR_BRWR_OP(nor->bouncebuf);
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_BRWR,
+						       nor->bouncebuf, 1);
+	}
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d setting 4-byte mode\n", ret);
+
+	return ret;
+}
+
+/**
+ * spi_nor_set_4byte_addr_mode() - Enter/Exit 4-byte address mode.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
+ *		address mode.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
+{
+	int ret;
+
+	if (nor->spimem) {
+		struct spi_mem_op op = SPI_NOR_EN4B_EX4B_OP(enable);
+
+		spi_nor_spimem_setup_op(nor, &op, nor->reg_proto);
+
+		ret = spi_mem_exec_op(nor->spimem, &op);
+	} else {
+		ret = spi_nor_controller_ops_write_reg(nor,
+						       enable ? SPINOR_OP_EN4B :
+								SPINOR_OP_EX4B,
+						       NULL, 0);
+	}
+
+	if (ret)
+		dev_dbg(nor->dev, "error %d setting 4-byte mode\n", ret);
+
+	return ret;
+}
+
+/**
+ * micron_st_nor_set_4byte_addr_mode() - Set 4-byte address mode for ST and
+ * Micron flashes.
+ * @nor:	pointer to 'struct spi_nor'.
+ * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
+ *		address mode.
+ *
+ * Return: 0 on success, -errno otherwise.
+ */
+int micron_st_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
+{
+	int ret;
+
+	ret = spi_nor_write_enable(nor);
+	if (ret)
+		return ret;
+
+	ret = spi_nor_set_4byte_addr_mode(nor, enable);
+	if (ret)
+		return ret;
+
+	return spi_nor_write_disable(nor);
+}
+
+#define BFPT_DWORD16_EN4B_MASK			GENMASK(31, 24)
+#define BFPT_DWORD16_EN4B_ALWAYS_4B		BIT(30)
+#define BFPT_DWORD16_EN4B_4B_OPCODES		BIT(29)
+#define BFPT_DWORD16_EN4B_16BIT_NV_CR		BIT(28)
+#define BFPT_DWORD16_EN4B_BRWR			BIT(27)
+#define BFPT_DWORD16_EN4B_WREAR			BIT(26)
+#define BFPT_DWORD16_EN4B_WREN_EN4B		BIT(25)
+#define BFPT_DWORD16_EN4B_EN4B			BIT(24)
+
+#define BFPT_DWORD16_EX4B_MASK			GENMASK(18, 14)
+#define BFPT_DWORD16_EX4B_16BIT_NV_CR		BIT(18)
+#define BFPT_DWORD16_EX4B_BRWR			BIT(17)
+#define BFPT_DWORD16_EX4B_WREAR			BIT(16)
+#define BFPT_DWORD16_EX4B_WREN_EX4B		BIT(15)
+#define BFPT_DWORD16_EX4B_EX4B			BIT(14)
+
+#define BFPT_DWORD16_4B_ADDR_MODE_MASK			\
+	(BFPT_DWORD16_EN4B_MASK | BFPT_DWORD16_EX4B_MASK)
+
+#define BFPT_DWORD16_4B_ADDR_MODE_16BIT_NV_CR		\
+	(BFPT_DWORD16_EN4B_16BIT_NV_CR | BFPT_DWORD16_EX4B_16BIT_NV_CR)
+
+#define BFPT_DWORD16_4B_ADDR_MODE_BRWR			\
+	(BFPT_DWORD16_EN4B_BRWR | BFPT_DWORD16_EX4B_BRWR)
+
+#define BFPT_DWORD16_4B_ADDR_MODE_WREAR			\
+	(BFPT_DWORD16_EN4B_WREAR | BFPT_DWORD16_EX4B_WREAR)
+
+#define BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B	\
+	(BFPT_DWORD16_EN4B_WREN_EN4B | BFPT_DWORD16_EX4B_WREN_EX4B)
+
+#define BFPT_DWORD16_4B_ADDR_MODE_EN4B_EX4B		\
+	(BFPT_DWORD16_EN4B_EN4B | BFPT_DWORD16_EX4B_EX4B)
+
 /**
  * spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
  * @nor:		pointer to a 'struct spi_nor'
@@ -606,6 +727,24 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 		break;
 	}
 
+	switch (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK) {
+	case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
+		params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
+		break;
+
+	case BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B:
+		params->set_4byte_addr_mode = micron_st_nor_set_4byte_addr_mode;
+		break;
+
+	case BFPT_DWORD16_4B_ADDR_MODE_EN4B_EX4B:
+		params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode;
+		break;
+
+	default:
+		dev_dbg(nor->dev, "BFPT: 4-Byte Address Mode method is not recognized or not implemented\n");
+		break;
+	}
+
 	/* Soft Reset support. */
 	if (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_SWRST_EN_RST)
 		nor->flags |= SNOR_F_SOFT_RESET;
diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index bbf80d2990ab..b56849079aea 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -107,6 +107,9 @@ struct sfdp_parameter_header {
 	u8		id_msb;
 };
 
+int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
+int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
+int micron_st_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
 int spi_nor_parse_sfdp(struct spi_nor *nor);
 
 #endif /* __LINUX_MTD_SFDP_H */
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index fe80dffc2e70..374ba82bff49 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -170,19 +170,23 @@ static const struct spi_nor_otp_ops winbond_nor_otp_ops = {
 	.is_locked = spi_nor_otp_is_locked_sr2,
 };
 
-static void winbond_nor_default_init(struct spi_nor *nor)
-{
-	nor->params->set_4byte_addr_mode = winbond_nor_set_4byte_addr_mode;
-}
-
 static void winbond_nor_late_init(struct spi_nor *nor)
 {
 	if (nor->params->otp.org->n_regions)
 		nor->params->otp.ops = &winbond_nor_otp_ops;
+
+	/*
+	 * Winbond seems to require that the Extended Address Register to be set
+	 * to zero when exiting the 4-Byte Address Mode, at least for W25Q256FV.
+	 * This requirement is not described in the JESD216 SFDP standard, thus
+	 * it is Winbond specific. Since we do not know if other Winbond flashes
+	 * have the same requirement, play safe and overwrite the method parsed
+	 * from BFPT, if any.
+	 */
+	nor->params->set_4byte_addr_mode = winbond_nor_set_4byte_addr_mode;
 }
 
 static const struct spi_nor_fixups winbond_nor_fixups = {
-	.default_init = winbond_nor_default_init,
 	.late_init = winbond_nor_late_init,
 };
 
-- 
2.25.1

Re: [PATCH v2 1/4] mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode methods

[Pratyush Yadav]

On 09/03/22 04:42PM, Tudor Ambarus wrote:
> BFPT[DWORD(16)] defines the methods to enter and exit the 4-Byte Address
> Mode. Parse BFPT to determine the method. Will rename the methods with
> generic names in a further patch, to keep things trackable in this one.
> 
> Some regressions may be introduced by this patch, because the
> params->set_4byte_addr_mode method that was set either in
> spi_nor_init_default_params() or later overwritten in default_init() hooks,
> may now be overwritten with a different value based on the BFPT data. If
> that's the case, the fix is to introduce a post_bfpt fixup hook where one
> should fix the wrong BFPT info.

Okay. Let's try to queue this series (and its dependencies) early in the 
rc cycle to give some time to catch bugs.

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v2 2/4] mtd: spi-nor: Update name and description of the set_4byte_addr_mode BFPT methods

[Tudor Ambarus]

BFPT defines some standard methods to enter and exit the 4-Byte Address
Mode. Use generic names for these methods and update their description.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c      |  2 +-
 drivers/mtd/spi-nor/macronix.c  |  3 ++-
 drivers/mtd/spi-nor/micron-st.c |  3 ++-
 drivers/mtd/spi-nor/sfdp.c      | 32 ++++++++++++++++++++------------
 drivers/mtd/spi-nor/sfdp.h      |  7 ++++---
 drivers/mtd/spi-nor/winbond.c   |  2 +-
 6 files changed, 30 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 452d0f91a8df..91d3754baa59 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2492,7 +2492,7 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
 	struct device_node *np = spi_nor_get_flash_node(nor);
 
 	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
-	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
+	params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
 	params->otp.org = &info->otp_org;
 
 	/* Default to 16-bit Write Status (01h) Command */
diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index d81a4cb2812b..85e8655d362c 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -105,7 +105,8 @@ static const struct flash_info macronix_nor_parts[] = {
 static void macronix_nor_default_init(struct spi_nor *nor)
 {
 	nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
-	nor->params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode;
+	nor->params->set_4byte_addr_mode =
+		spi_nor_set_4byte_addr_mode_en4b_ex4b;
 }
 
 static const struct spi_nor_fixups macronix_nor_fixups = {
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index c348419d24a0..4baa9dce04f9 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -410,7 +410,8 @@ static void micron_st_nor_default_init(struct spi_nor *nor)
 	nor->flags |= SNOR_F_HAS_LOCK;
 	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
 	nor->params->quad_enable = NULL;
-	nor->params->set_4byte_addr_mode = micron_st_nor_set_4byte_addr_mode;
+	nor->params->set_4byte_addr_mode =
+		spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
 }
 
 static void micron_st_nor_late_init(struct spi_nor *nor)
diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 108a74ce38e0..01e35354db3e 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -402,15 +402,20 @@ static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
 }
 
 /**
- * spansion_set_4byte_addr_mode() - Set 4-byte address mode for Spansion
- * flashes.
+ * spi_nor_set_4byte_addr_mode_brwr() - Set 4-byte address mode using
+ * SPINOR_OP_BRWR.
  * @nor:	pointer to 'struct spi_nor'.
  * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
  *		address mode.
  *
+ * 8-bit volatile bank register used to define A[30:A24] bits. MSB (bit[7]) is
+ * used to enable/disable 4-byte address mode. When MSB is set to ‘1’, 4-byte
+ * address mode is active and A[30:24] bits are don’t care. Write instruction is
+ * SPINOR_OP_BRWR(17h) with 1 byte of data.
+ *
  * Return: 0 on success, -errno otherwise.
  */
-int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
+int spi_nor_set_4byte_addr_mode_brwr(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
@@ -434,14 +439,15 @@ int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 }
 
 /**
- * spi_nor_set_4byte_addr_mode() - Enter/Exit 4-byte address mode.
+ * spi_nor_set_4byte_addr_mode_en4b_ex4b() - Enter/Exit 4-byte address mode
+ * using SPINOR_OP_EN4B/SPINOR_OP_EX4B.
  * @nor:	pointer to 'struct spi_nor'.
  * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
  *		address mode.
  *
  * Return: 0 on success, -errno otherwise.
  */
-int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
+int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
@@ -465,15 +471,15 @@ int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 }
 
 /**
- * micron_st_nor_set_4byte_addr_mode() - Set 4-byte address mode for ST and
- * Micron flashes.
+ * spi_nor_set_4byte_addr_mode_wren_en4b_ex4b() - Set 4-byte address mode usingf
+ * SPINOR_OP_WREN followed by SPINOR_OP_EN4B or SPINOR_OP_EX4B.
  * @nor:	pointer to 'struct spi_nor'.
  * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
  *		address mode.
  *
  * Return: 0 on success, -errno otherwise.
  */
-int micron_st_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
+int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
@@ -481,7 +487,7 @@ int micron_st_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 	if (ret)
 		return ret;
 
-	ret = spi_nor_set_4byte_addr_mode(nor, enable);
+	ret = spi_nor_set_4byte_addr_mode_en4b_ex4b(nor, enable);
 	if (ret)
 		return ret;
 
@@ -729,15 +735,17 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
 
 	switch (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK) {
 	case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
-		params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
+		params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
 		break;
 
 	case BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B:
-		params->set_4byte_addr_mode = micron_st_nor_set_4byte_addr_mode;
+		params->set_4byte_addr_mode =
+			spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
 		break;
 
 	case BFPT_DWORD16_4B_ADDR_MODE_EN4B_EX4B:
-		params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode;
+		params->set_4byte_addr_mode =
+			spi_nor_set_4byte_addr_mode_en4b_ex4b;
 		break;
 
 	default:
diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index b56849079aea..da2d7ec2e0aa 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -107,9 +107,10 @@ struct sfdp_parameter_header {
 	u8		id_msb;
 };
 
-int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
-int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
-int micron_st_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
+int spi_nor_set_4byte_addr_mode_brwr(struct spi_nor *nor, bool enable);
+int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, bool enable);
+int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor,
+					       bool enable);
 int spi_nor_parse_sfdp(struct spi_nor *nor);
 
 #endif /* __LINUX_MTD_SFDP_H */
diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
index 374ba82bff49..590e4d2c99d7 100644
--- a/drivers/mtd/spi-nor/winbond.c
+++ b/drivers/mtd/spi-nor/winbond.c
@@ -142,7 +142,7 @@ static int winbond_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
 {
 	int ret;
 
-	ret = spi_nor_set_4byte_addr_mode(nor, enable);
+	ret = spi_nor_set_4byte_addr_mode_en4b_ex4b(nor, enable);
 	if (ret || enable)
 		return ret;
 
-- 
2.25.1

Re: [PATCH v2 2/4] mtd: spi-nor: Update name and description of the set_4byte_addr_mode BFPT methods

[Pratyush Yadav]

On 09/03/22 04:42PM, Tudor Ambarus wrote:
> BFPT defines some standard methods to enter and exit the 4-Byte Address
> Mode. Use generic names for these methods and update their description.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/core.c      |  2 +-
>  drivers/mtd/spi-nor/macronix.c  |  3 ++-
>  drivers/mtd/spi-nor/micron-st.c |  3 ++-
>  drivers/mtd/spi-nor/sfdp.c      | 32 ++++++++++++++++++++------------
>  drivers/mtd/spi-nor/sfdp.h      |  7 ++++---
>  drivers/mtd/spi-nor/winbond.c   |  2 +-
>  6 files changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 452d0f91a8df..91d3754baa59 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2492,7 +2492,7 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
>  	struct device_node *np = spi_nor_get_flash_node(nor);
>  
>  	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
> -	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
> +	params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
>  	params->otp.org = &info->otp_org;
>  
>  	/* Default to 16-bit Write Status (01h) Command */
> diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
> index d81a4cb2812b..85e8655d362c 100644
> --- a/drivers/mtd/spi-nor/macronix.c
> +++ b/drivers/mtd/spi-nor/macronix.c
> @@ -105,7 +105,8 @@ static const struct flash_info macronix_nor_parts[] = {
>  static void macronix_nor_default_init(struct spi_nor *nor)
>  {
>  	nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
> -	nor->params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode;
> +	nor->params->set_4byte_addr_mode =
> +		spi_nor_set_4byte_addr_mode_en4b_ex4b;
>  }
>  
>  static const struct spi_nor_fixups macronix_nor_fixups = {
> diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
> index c348419d24a0..4baa9dce04f9 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -410,7 +410,8 @@ static void micron_st_nor_default_init(struct spi_nor *nor)
>  	nor->flags |= SNOR_F_HAS_LOCK;
>  	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
>  	nor->params->quad_enable = NULL;
> -	nor->params->set_4byte_addr_mode = micron_st_nor_set_4byte_addr_mode;
> +	nor->params->set_4byte_addr_mode =
> +		spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
>  }
>  
>  static void micron_st_nor_late_init(struct spi_nor *nor)
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 108a74ce38e0..01e35354db3e 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -402,15 +402,20 @@ static void spi_nor_regions_sort_erase_types(struct spi_nor_erase_map *map)
>  }
>  
>  /**
> - * spansion_set_4byte_addr_mode() - Set 4-byte address mode for Spansion
> - * flashes.
> + * spi_nor_set_4byte_addr_mode_brwr() - Set 4-byte address mode using
> + * SPINOR_OP_BRWR.
>   * @nor:	pointer to 'struct spi_nor'.
>   * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
>   *		address mode.
>   *
> + * 8-bit volatile bank register used to define A[30:A24] bits. MSB (bit[7]) is

Typo. s/30:A24/30:24/

Other than this,

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

> + * used to enable/disable 4-byte address mode. When MSB is set to ‘1’, 4-byte
> + * address mode is active and A[30:24] bits are don’t care. Write instruction is
> + * SPINOR_OP_BRWR(17h) with 1 byte of data.
> + *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
> +int spi_nor_set_4byte_addr_mode_brwr(struct spi_nor *nor, bool enable)
>  {
>  	int ret;
>  
> @@ -434,14 +439,15 @@ int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  }
>  
>  /**
> - * spi_nor_set_4byte_addr_mode() - Enter/Exit 4-byte address mode.
> + * spi_nor_set_4byte_addr_mode_en4b_ex4b() - Enter/Exit 4-byte address mode
> + * using SPINOR_OP_EN4B/SPINOR_OP_EX4B.
>   * @nor:	pointer to 'struct spi_nor'.
>   * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
>   *		address mode.
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
> +int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, bool enable)
>  {
>  	int ret;
>  
> @@ -465,15 +471,15 @@ int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  }
>  
>  /**
> - * micron_st_nor_set_4byte_addr_mode() - Set 4-byte address mode for ST and
> - * Micron flashes.
> + * spi_nor_set_4byte_addr_mode_wren_en4b_ex4b() - Set 4-byte address mode usingf
> + * SPINOR_OP_WREN followed by SPINOR_OP_EN4B or SPINOR_OP_EX4B.
>   * @nor:	pointer to 'struct spi_nor'.
>   * @enable:	true to enter the 4-byte address mode, false to exit the 4-byte
>   *		address mode.
>   *
>   * Return: 0 on success, -errno otherwise.
>   */
> -int micron_st_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
> +int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor, bool enable)
>  {
>  	int ret;
>  
> @@ -481,7 +487,7 @@ int micron_st_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  	if (ret)
>  		return ret;
>  
> -	ret = spi_nor_set_4byte_addr_mode(nor, enable);
> +	ret = spi_nor_set_4byte_addr_mode_en4b_ex4b(nor, enable);
>  	if (ret)
>  		return ret;
>  
> @@ -729,15 +735,17 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  
>  	switch (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK) {
>  	case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
> -		params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
> +		params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
>  		break;
>  
>  	case BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B:
> -		params->set_4byte_addr_mode = micron_st_nor_set_4byte_addr_mode;
> +		params->set_4byte_addr_mode =
> +			spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
>  		break;
>  
>  	case BFPT_DWORD16_4B_ADDR_MODE_EN4B_EX4B:
> -		params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode;
> +		params->set_4byte_addr_mode =
> +			spi_nor_set_4byte_addr_mode_en4b_ex4b;
>  		break;
>  
>  	default:
> diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
> index b56849079aea..da2d7ec2e0aa 100644
> --- a/drivers/mtd/spi-nor/sfdp.h
> +++ b/drivers/mtd/spi-nor/sfdp.h
> @@ -107,9 +107,10 @@ struct sfdp_parameter_header {
>  	u8		id_msb;
>  };
>  
> -int spansion_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
> -int spi_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
> -int micron_st_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable);
> +int spi_nor_set_4byte_addr_mode_brwr(struct spi_nor *nor, bool enable);
> +int spi_nor_set_4byte_addr_mode_en4b_ex4b(struct spi_nor *nor, bool enable);
> +int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor,
> +					       bool enable);
>  int spi_nor_parse_sfdp(struct spi_nor *nor);
>  
>  #endif /* __LINUX_MTD_SFDP_H */
> diff --git a/drivers/mtd/spi-nor/winbond.c b/drivers/mtd/spi-nor/winbond.c
> index 374ba82bff49..590e4d2c99d7 100644
> --- a/drivers/mtd/spi-nor/winbond.c
> +++ b/drivers/mtd/spi-nor/winbond.c
> @@ -142,7 +142,7 @@ static int winbond_nor_set_4byte_addr_mode(struct spi_nor *nor, bool enable)
>  {
>  	int ret;
>  
> -	ret = spi_nor_set_4byte_addr_mode(nor, enable);
> +	ret = spi_nor_set_4byte_addr_mode_en4b_ex4b(nor, enable);
>  	if (ret || enable)
>  		return ret;
>  
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v2 3/4] mtd: spi-nor: Favor the BFPT-parsed set_4byte_addr_mode method

[Tudor Ambarus]

JESD216 SFDP defines in the BFPT standard methods to enter and exit the
4-Byte Address Mode. The flash parameters and settings that are retrieved
from SFDP have higher precedence than the static initialized ones, because
they should be more accurate and less error prone than those initialized
statically. Favor the BFPT-parsed set_4byte_addr_mode method and use the
generic core methods where possible.
This patch may introduce regressions in case BFPT contains wrong data. The
fix is to introduce a post_bfpt() fixup hook and update the wrong BFPT
data.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/core.c      |  7 ++++++-
 drivers/mtd/spi-nor/macronix.c  | 10 ++++++++--
 drivers/mtd/spi-nor/micron-st.c |  9 ++++++---
 3 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 91d3754baa59..5de46a786cc5 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2418,6 +2418,8 @@ static void spi_nor_init_fixup_flags(struct spi_nor *nor)
  */
 static void spi_nor_late_init_params(struct spi_nor *nor)
 {
+	struct spi_nor_flash_parameter *params = nor->params;
+
 	if (nor->manufacturer && nor->manufacturer->fixups &&
 	    nor->manufacturer->fixups->late_init)
 		nor->manufacturer->fixups->late_init(nor);
@@ -2425,6 +2427,10 @@ static void spi_nor_late_init_params(struct spi_nor *nor)
 	if (nor->info->fixups && nor->info->fixups->late_init)
 		nor->info->fixups->late_init(nor);
 
+	/* Default method kept for backward compatibility. */
+	if (!params->set_4byte_addr_mode)
+		params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
+
 	spi_nor_init_flags(nor);
 	spi_nor_init_fixup_flags(nor);
 
@@ -2492,7 +2498,6 @@ static void spi_nor_init_default_params(struct spi_nor *nor)
 	struct device_node *np = spi_nor_get_flash_node(nor);
 
 	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
-	params->set_4byte_addr_mode = spi_nor_set_4byte_addr_mode_brwr;
 	params->otp.org = &info->otp_org;
 
 	/* Default to 16-bit Write Status (01h) Command */
diff --git a/drivers/mtd/spi-nor/macronix.c b/drivers/mtd/spi-nor/macronix.c
index 85e8655d362c..c267cbcc7f1d 100644
--- a/drivers/mtd/spi-nor/macronix.c
+++ b/drivers/mtd/spi-nor/macronix.c
@@ -105,12 +105,18 @@ static const struct flash_info macronix_nor_parts[] = {
 static void macronix_nor_default_init(struct spi_nor *nor)
 {
 	nor->params->quad_enable = spi_nor_sr1_bit6_quad_enable;
-	nor->params->set_4byte_addr_mode =
-		spi_nor_set_4byte_addr_mode_en4b_ex4b;
+}
+
+static void macronix_nor_late_init(struct spi_nor *nor)
+{
+	if (!nor->params->set_4byte_addr_mode)
+		nor->params->set_4byte_addr_mode =
+			spi_nor_set_4byte_addr_mode_en4b_ex4b;
 }
 
 static const struct spi_nor_fixups macronix_nor_fixups = {
 	.default_init = macronix_nor_default_init,
+	.late_init = macronix_nor_late_init,
 };
 
 const struct spi_nor_manufacturer spi_nor_macronix = {
diff --git a/drivers/mtd/spi-nor/micron-st.c b/drivers/mtd/spi-nor/micron-st.c
index 4baa9dce04f9..a23d2774f166 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -410,14 +410,17 @@ static void micron_st_nor_default_init(struct spi_nor *nor)
 	nor->flags |= SNOR_F_HAS_LOCK;
 	nor->flags &= ~SNOR_F_HAS_16BIT_SR;
 	nor->params->quad_enable = NULL;
-	nor->params->set_4byte_addr_mode =
-		spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
 }
 
 static void micron_st_nor_late_init(struct spi_nor *nor)
 {
+	struct spi_nor_flash_parameter *params = nor->params;
+
 	if (nor->info->mfr_flags & USE_FSR)
-		nor->params->ready = micron_st_nor_ready;
+		params->ready = micron_st_nor_ready;
+	if (!params->set_4byte_addr_mode)
+		params->set_4byte_addr_mode =
+			spi_nor_set_4byte_addr_mode_wren_en4b_ex4b;
 }
 
 static const struct spi_nor_fixups micron_st_nor_fixups = {
-- 
2.25.1

Re: [PATCH v2 3/4] mtd: spi-nor: Favor the BFPT-parsed set_4byte_addr_mode method

[Pratyush Yadav]

On 09/03/22 04:42PM, Tudor Ambarus wrote:
> JESD216 SFDP defines in the BFPT standard methods to enter and exit the
> 4-Byte Address Mode. The flash parameters and settings that are retrieved
> from SFDP have higher precedence than the static initialized ones, because
> they should be more accurate and less error prone than those initialized
> statically. Favor the BFPT-parsed set_4byte_addr_mode method and use the
> generic core methods where possible.
> This patch may introduce regressions in case BFPT contains wrong data. The
> fix is to introduce a post_bfpt() fixup hook and update the wrong BFPT
> data.
> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>

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

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

[PATCH v2 4/4] mtd: spi-nor: sfdp: Keep SFDP definitions private

[Tudor Ambarus]

Keep the SFDP definitions private and expose just the definitions that are
required by the core and manufacturer drivers.

Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
---
 drivers/mtd/spi-nor/sfdp.c | 52 +++++++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/sfdp.h | 53 --------------------------------------
 2 files changed, 52 insertions(+), 53 deletions(-)

diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
index 01e35354db3e..f5432cbd3daf 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -494,6 +494,50 @@ int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor, bool enable)
 	return spi_nor_write_disable(nor);
 }
 
+/* 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 Write 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 */
+
 #define BFPT_DWORD16_EN4B_MASK			GENMASK(31, 24)
 #define BFPT_DWORD16_EN4B_ALWAYS_4B		BIT(30)
 #define BFPT_DWORD16_EN4B_4B_OPCODES		BIT(29)
@@ -528,6 +572,14 @@ int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor, bool enable)
 #define BFPT_DWORD16_4B_ADDR_MODE_EN4B_EX4B		\
 	(BFPT_DWORD16_EN4B_EN4B | BFPT_DWORD16_EX4B_EX4B)
 
+#define BFPT_DWORD16_SWRST_EN_RST		BIT(12)
+
+#define BFPT_DWORD18_CMD_EXT_MASK		GENMASK(30, 29)
+#define BFPT_DWORD18_CMD_EXT_REP		(0x0UL << 29) /* Repeat */
+#define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */
+#define BFPT_DWORD18_CMD_EXT_RES		(0x2UL << 29) /* Reserved */
+#define BFPT_DWORD18_CMD_EXT_16B		(0x3UL << 29) /* 16-bit opcode */
+
 /**
  * spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
  * @nor:		pointer to a 'struct spi_nor'
diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
index da2d7ec2e0aa..fd7e3c935960 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -45,59 +45,6 @@ struct sfdp_bfpt {
 #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 Write 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 */
-
-#define BFPT_DWORD16_SWRST_EN_RST		BIT(12)
-
-#define BFPT_DWORD18_CMD_EXT_MASK		GENMASK(30, 29)
-#define BFPT_DWORD18_CMD_EXT_REP		(0x0UL << 29) /* Repeat */
-#define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */
-#define BFPT_DWORD18_CMD_EXT_RES		(0x2UL << 29) /* Reserved */
-#define BFPT_DWORD18_CMD_EXT_16B		(0x3UL << 29) /* 16-bit opcode */
-
 struct sfdp_parameter_header {
 	u8		id_lsb;
 	u8		minor;
-- 
2.25.1

Re: [PATCH v2 4/4] mtd: spi-nor: sfdp: Keep SFDP definitions private

[Pratyush Yadav]

On 09/03/22 04:42PM, Tudor Ambarus wrote:
> Keep the SFDP definitions private and expose just the definitions that are
> required by the core and manufacturer drivers.

I am not so sure about this. Since the post_bfpt hook passes in the bfpt 
table to flash drivers, they might end up wanting to use these for some 
checks like issi.c does for DWORD 1. They would have to move them back 
to sfdp.h for that, which just causes extra churn, and also puts some 
BFPT related defines in sfdp.h and some in sfdp.c.

> 
> Signed-off-by: Tudor Ambarus <tudor.ambarus@microchip.com>
> ---
>  drivers/mtd/spi-nor/sfdp.c | 52 +++++++++++++++++++++++++++++++++++++
>  drivers/mtd/spi-nor/sfdp.h | 53 --------------------------------------
>  2 files changed, 52 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index 01e35354db3e..f5432cbd3daf 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -494,6 +494,50 @@ int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor, bool enable)
>  	return spi_nor_write_disable(nor);
>  }
>  
> +/* 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 Write 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 */
> +
>  #define BFPT_DWORD16_EN4B_MASK			GENMASK(31, 24)
>  #define BFPT_DWORD16_EN4B_ALWAYS_4B		BIT(30)
>  #define BFPT_DWORD16_EN4B_4B_OPCODES		BIT(29)
> @@ -528,6 +572,14 @@ int spi_nor_set_4byte_addr_mode_wren_en4b_ex4b(struct spi_nor *nor, bool enable)
>  #define BFPT_DWORD16_4B_ADDR_MODE_EN4B_EX4B		\
>  	(BFPT_DWORD16_EN4B_EN4B | BFPT_DWORD16_EX4B_EX4B)
>  
> +#define BFPT_DWORD16_SWRST_EN_RST		BIT(12)
> +
> +#define BFPT_DWORD18_CMD_EXT_MASK		GENMASK(30, 29)
> +#define BFPT_DWORD18_CMD_EXT_REP		(0x0UL << 29) /* Repeat */
> +#define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */
> +#define BFPT_DWORD18_CMD_EXT_RES		(0x2UL << 29) /* Reserved */
> +#define BFPT_DWORD18_CMD_EXT_16B		(0x3UL << 29) /* 16-bit opcode */
> +
>  /**
>   * spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
>   * @nor:		pointer to a 'struct spi_nor'
> diff --git a/drivers/mtd/spi-nor/sfdp.h b/drivers/mtd/spi-nor/sfdp.h
> index da2d7ec2e0aa..fd7e3c935960 100644
> --- a/drivers/mtd/spi-nor/sfdp.h
> +++ b/drivers/mtd/spi-nor/sfdp.h
> @@ -45,59 +45,6 @@ struct sfdp_bfpt {
>  #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 Write 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 */
> -
> -#define BFPT_DWORD16_SWRST_EN_RST		BIT(12)
> -
> -#define BFPT_DWORD18_CMD_EXT_MASK		GENMASK(30, 29)
> -#define BFPT_DWORD18_CMD_EXT_REP		(0x0UL << 29) /* Repeat */
> -#define BFPT_DWORD18_CMD_EXT_INV		(0x1UL << 29) /* Invert */
> -#define BFPT_DWORD18_CMD_EXT_RES		(0x2UL << 29) /* Reserved */
> -#define BFPT_DWORD18_CMD_EXT_16B		(0x3UL << 29) /* 16-bit opcode */
> -
>  struct sfdp_parameter_header {
>  	u8		id_lsb;
>  	u8		minor;
> -- 
> 2.25.1
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v2 4/4] mtd: spi-nor: sfdp: Keep SFDP definitions private

[Tudor.Ambarus]

On 4/1/22 23:01, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> On 09/03/22 04:42PM, Tudor Ambarus wrote:
>> Keep the SFDP definitions private and expose just the definitions that are
>> required by the core and manufacturer drivers.
> 
> I am not so sure about this. Since the post_bfpt hook passes in the bfpt
> table to flash drivers, they might end up wanting to use these for some
> checks like issi.c does for DWORD 1. They would have to move them back
> to sfdp.h for that, which just causes extra churn, and also puts some
> BFPT related defines in sfdp.h and some in sfdp.c.
> 

That's correct, but I think exposing just the public defines in sfdp.h is
the path to follow. We should keep private all the definitions that we can
private in sfdp.c and expose publicly in sfdp.h just the ones that are shared.
Flash collisions, and implicitly the need of public SFDP definitions, should be
an exception, so I expect sfdp.h to be short in size.

Cheers,
ta

Re: [PATCH v2 4/4] mtd: spi-nor: sfdp: Keep SFDP definitions private

[Pratyush Yadav]

On 04/04/22 06:19AM, Tudor.Ambarus@microchip.com wrote:
> On 4/1/22 23:01, Pratyush Yadav wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On 09/03/22 04:42PM, Tudor Ambarus wrote:
> >> Keep the SFDP definitions private and expose just the definitions that are
> >> required by the core and manufacturer drivers.
> > 
> > I am not so sure about this. Since the post_bfpt hook passes in the bfpt
> > table to flash drivers, they might end up wanting to use these for some
> > checks like issi.c does for DWORD 1. They would have to move them back
> > to sfdp.h for that, which just causes extra churn, and also puts some
> > BFPT related defines in sfdp.h and some in sfdp.c.
> > 
> 
> That's correct, but I think exposing just the public defines in sfdp.h is
> the path to follow. We should keep private all the definitions that we can
> private in sfdp.c and expose publicly in sfdp.h just the ones that are shared.
> Flash collisions, and implicitly the need of public SFDP definitions, should be
> an exception, so I expect sfdp.h to be short in size.

I disagree. I think we should keep everything in the same place. And 
since we need to expose this to manufacturer drivers, that place is 
sfdp.h. Who is going to cast the tiebreaking vote here? ;-)

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v2 4/4] mtd: spi-nor: sfdp: Keep SFDP definitions private

[Michael Walle]

Hi,

I didn't follow the series closely, so I might miss something.

Am 2022-04-05 21:31, schrieb Pratyush Yadav:
> On 04/04/22 06:19AM, Tudor.Ambarus@microchip.com wrote:
>> On 4/1/22 23:01, Pratyush Yadav wrote:
>> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> >
>> > On 09/03/22 04:42PM, Tudor Ambarus wrote:
>> >> Keep the SFDP definitions private and expose just the definitions that are
>> >> required by the core and manufacturer drivers.
>> >
>> > I am not so sure about this. Since the post_bfpt hook passes in the bfpt
>> > table to flash drivers, they might end up wanting to use these for some
>> > checks like issi.c does for DWORD 1. They would have to move them back
>> > to sfdp.h for that, which just causes extra churn, and also puts some
>> > BFPT related defines in sfdp.h and some in sfdp.c.
>> >

It's not the extra churn, but also git blame doesn't really work. In
fact, it is already really hard to follow all the commits in spi-nor/
with all the code moving around. So what is the actual adavantage of
having this stuff private? One clear disadvantage is that you need to
move it from sfdp.c to sfdp.h once you need it. And potentially, you
might need all the defines.

Also, if you pass the SFDP data to any fixups, the drivers are expected
to use them.

>> That's correct, but I think exposing just the public defines in sfdp.h 
>> is
>> the path to follow. We should keep private all the definitions that we 
>> can
>> private in sfdp.c and expose publicly in sfdp.h just the ones that are 
>> shared.
>> Flash collisions, and implicitly the need of public SFDP definitions, 
>> should be
>> an exception, so I expect sfdp.h to be short in size.
> 
> I disagree. I think we should keep everything in the same place. And
> since we need to expose this to manufacturer drivers, that place is
> sfdp.h. Who is going to cast the tiebreaking vote here? ;-)

I'm leaning towards Pratyush opinion unless there is a clear advantage
to move the defines.

-michael

Re: [PATCH v2 4/4] mtd: spi-nor: sfdp: Keep SFDP definitions private

[Tudor.Ambarus]

On 4/6/22 13:31, Michael Walle wrote:
> 
>>> That's correct, but I think exposing just the public defines in sfdp.h
>>> is
>>> the path to follow. We should keep private all the definitions that we
>>> can
>>> private in sfdp.c and expose publicly in sfdp.h just the ones that are
>>> shared.
>>> Flash collisions, and implicitly the need of public SFDP definitions,
>>> should be
>>> an exception, so I expect sfdp.h to be short in size.
>>
>> I disagree. I think we should keep everything in the same place. And
>> since we need to expose this to manufacturer drivers, that place is
>> sfdp.h. Who is going to cast the tiebreaking vote here? ;-)
> 
> I'm leaning towards Pratyush opinion unless there is a clear advantage
> to move the defines.

Okay. Then we should move all the table definitions to sfdp.h, right?

Re: [PATCH v2 4/4] mtd: spi-nor: sfdp: Keep SFDP definitions private

[Pratyush Yadav]

On 06/04/22 11:10AM, Tudor.Ambarus@microchip.com wrote:
> On 4/6/22 13:31, Michael Walle wrote:
> > 
> >>> That's correct, but I think exposing just the public defines in sfdp.h
> >>> is
> >>> the path to follow. We should keep private all the definitions that we
> >>> can
> >>> private in sfdp.c and expose publicly in sfdp.h just the ones that are
> >>> shared.
> >>> Flash collisions, and implicitly the need of public SFDP definitions,
> >>> should be
> >>> an exception, so I expect sfdp.h to be short in size.
> >>
> >> I disagree. I think we should keep everything in the same place. And
> >> since we need to expose this to manufacturer drivers, that place is
> >> sfdp.h. Who is going to cast the tiebreaking vote here? ;-)
> > 
> > I'm leaning towards Pratyush opinion unless there is a clear advantage
> > to move the defines.
> 
> Okay. Then we should move all the table definitions to sfdp.h, right?

We don't pass any other table to manufacturer drivers. Only BFPT. So 
those can stay private. But I don't mind either in this case.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.