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

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

[Tudor Ambarus]

Depends on:
https://lore.kernel.org/lkml/20220411091033.98754-1-tudor.ambarus@microchip.com/

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.

v3:
- Move the newly introduced BFPT definitions in sfdp.h, as they may be used by
manufacturer drivers to handle flash ID collisions.
- Drop "mtd: spi-nor: sfdp: Keep SFDP definitions private" patch for the same
reason as above
- Collect R-b tags

v2: Drop quad enable patch

Tudor Ambarus (3):
  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

 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      | 113 ++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/sfdp.h      |  30 +++++++++
 drivers/mtd/spi-nor/winbond.c   |  18 +++--
 7 files changed, 174 insertions(+), 99 deletions(-)
-- 
2.25.1

[PATCH v3 1/3] 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>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
v3: move BFPT definitions in sfdp.h as they may be used by manufacturer
drivers to handle flash ID collisions.

 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      | 105 ++++++++++++++++++++++++++++++++
 drivers/mtd/spi-nor/sfdp.h      |  29 +++++++++
 drivers/mtd/spi-nor/winbond.c   |  16 +++--
 6 files changed, 144 insertions(+), 94 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index fe853532204c..2b26a8cef0c3 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -502,69 +502,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 26b9a1c2309d..d2903c5b5b87 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -298,30 +298,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..2e40eba3744d 100644
--- a/drivers/mtd/spi-nor/sfdp.c
+++ b/drivers/mtd/spi-nor/sfdp.c
@@ -401,6 +401,93 @@ 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);
+}
+
 /**
  * spi_nor_parse_bfpt() - read and parse the Basic Flash Parameter Table.
  * @nor:		pointer to a 'struct spi_nor'
@@ -606,6 +693,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..3a3744da78ba 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -90,6 +90,32 @@ struct sfdp_bfpt {
 #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)
+#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)
 #define BFPT_DWORD16_SWRST_EN_RST		BIT(12)
 
 #define BFPT_DWORD18_CMD_EXT_MASK		GENMASK(30, 29)
@@ -107,6 +133,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 v3 1/3] mtd: spi-nor: Parse BFPT to determine the 4-Byte Address Mode methods

[Michael Walle]

> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
> index a5211543d30d..2e40eba3744d 100644
> --- a/drivers/mtd/spi-nor/sfdp.c
> +++ b/drivers/mtd/spi-nor/sfdp.c
> @@ -401,6 +401,93 @@ 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)

Mh, so now some callback functions are in the core like the quad enable
methods and some are in sfdp.c. I think there should be only the parsing
in sfdp.c and all the callback methods should be in core.c; as they are
potentially used by the vendor modules.

> @@ -606,6 +693,24 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>  		break;
>  	}
> 
> +	switch (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK) 
> {

I was wondering why this is encoded as single bits and not as an
enumeration. To me it looks like it is intended to support
different methods at the same time. Thus I think there might be
multiple bits set in each entry and exit mask at once. The spec
lists all the entries as x_xxx1, x_xx1x, ..

> +	case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
.. then this will only match if exactly these two bits are set.


> +		params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;

Shouldn't this be getting a neutral name? The quad_enable methods
were renamed once, too.

-michael

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

[Michael Walle]

>> +		params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
> 
> Shouldn't this be getting a neutral name? The quad_enable methods
> were renamed once, too.

Scrap that, I see it is done in patch 2.

-michael

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

[Tudor.Ambarus]

On 4/14/22 12:03, Michael Walle wrote:

Hi!

> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>> index a5211543d30d..2e40eba3744d 100644
>> --- a/drivers/mtd/spi-nor/sfdp.c
>> +++ b/drivers/mtd/spi-nor/sfdp.c
>> @@ -401,6 +401,93 @@ 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)
> 
> Mh, so now some callback functions are in the core like the quad enable
> methods and some are in sfdp.c. I think there should be only the parsing
> in sfdp.c and all the callback methods should be in core.c; as they are
> potentially used by the vendor modules.

All set_4byte_addr_mode methods are defined in sfdp.c and declared in sfdp.h.
I kept the methods defined in sfdp.c because SFDP defines their behavior/how
they work. Vendors already have access to all these methods when including
core.h (which includes sfdp.h). No need to move them to core, as they are
SFDP specific.

> 
>> @@ -606,6 +693,24 @@ static int spi_nor_parse_bfpt(struct spi_nor *nor,
>>               break;
>>       }
>>
>> +     switch (bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK)
>> {
> 
> I was wondering why this is encoded as single bits and not as an
> enumeration. To me it looks like it is intended to support

because I parse 2 bits and check if both the enter and the exit methods of
the 4b addr mode are specified.

> different methods at the same time. Thus I think there might be
> multiple bits set in each entry and exit mask at once. The spec
> lists all the entries as x_xxx1, x_xx1x, ..
> 
>> +     case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
> .. then this will only match if exactly these two bits are set.
> 

these 2 bits are:
drivers/mtd/spi-nor/sfdp.h:#define BFPT_DWORD16_4B_ADDR_MODE_BRWR                       \
drivers/mtd/spi-nor/sfdp.h-     (BFPT_DWORD16_EN4B_BRWR | BFPT_DWORD16_EX4B_BRWR)

When both specified, I set the method.

Cheers,
ta

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

[Michael Walle]

>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>> index a5211543d30d..2e40eba3744d 100644
>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>> @@ -401,6 +401,93 @@ 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)
>> 
>> Mh, so now some callback functions are in the core like the quad 
>> enable
>> methods and some are in sfdp.c. I think there should be only the 
>> parsing
>> in sfdp.c and all the callback methods should be in core.c; as they 
>> are
>> potentially used by the vendor modules.
> 
> All set_4byte_addr_mode methods are defined in sfdp.c and declared in 
> sfdp.h.
> I kept the methods defined in sfdp.c because SFDP defines their 
> behavior/how
> they work. Vendors already have access to all these methods when 
> including
> core.h (which includes sfdp.h). No need to move them to core, as they 
> are
> SFDP specific.

The same is true for the quad enable method and they reside in core.c.
Again, I think sfdp.c should be about the parsing, not the flash 
handling
itself. (And sfdp.h should be the equivalent to the spec in terms of
constants). I mean SFDP will eventually cover everything, so will you 
move
all methods over to sfdp.c? All these methods can be used with flashes
without SFDP.

SFDP just collects all the different methods used by flash manufacturers
and put them into a table. I don't see how SFDP is a spec where they 
specify
a paricular method and all the flash manufacturer pick that up. I think 
it
is the other way around, a flash manufacturer does something
proprietary and then it eventually ends up in the SFDP spec.

>>> @@ -606,6 +693,24 @@ static int spi_nor_parse_bfpt(struct spi_nor 
>>> *nor,
>>>               break;
>>>       }
>>> 
>>> +     switch (bfpt.dwords[BFPT_DWORD(16)] & 
>>> BFPT_DWORD16_4B_ADDR_MODE_MASK)
>>> {
>> 
>> I was wondering why this is encoded as single bits and not as an
>> enumeration. To me it looks like it is intended to support
> 
> because I parse 2 bits and check if both the enter and the exit methods 
> of
> the 4b addr mode are specified.

No, I'm only speaking of either the entry or the exit mask. See
below.

>> different methods at the same time. Thus I think there might be
>> multiple bits set in each entry and exit mask at once. The spec
>> lists all the entries as x_xxx1, x_xx1x, ..
>> 
>>> +     case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
>> .. then this will only match if exactly these two bits are set.
>> 
> 
> these 2 bits are:
> drivers/mtd/spi-nor/sfdp.h:#define BFPT_DWORD16_4B_ADDR_MODE_BRWR
>                  \
> drivers/mtd/spi-nor/sfdp.h-     (BFPT_DWORD16_EN4B_BRWR |
> BFPT_DWORD16_EX4B_BRWR)

I know this are two bits, but IMHO there can be multiple bits
set in *each* of these masks. Eg. the enter mask could be
0b00011 which would indicate that both the first and the second
enter method is supported.
(I'd expect that the exit mask will then be 0b00011, too).

-michael

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

[Tudor.Ambarus]

On 4/14/22 12:51, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c b/drivers/mtd/spi-nor/sfdp.c
>>>> index a5211543d30d..2e40eba3744d 100644
>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>> @@ -401,6 +401,93 @@ 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)
>>>
>>> Mh, so now some callback functions are in the core like the quad
>>> enable
>>> methods and some are in sfdp.c. I think there should be only the
>>> parsing
>>> in sfdp.c and all the callback methods should be in core.c; as they
>>> are
>>> potentially used by the vendor modules.
>>
>> All set_4byte_addr_mode methods are defined in sfdp.c and declared in
>> sfdp.h.
>> I kept the methods defined in sfdp.c because SFDP defines their
>> behavior/how
>> they work. Vendors already have access to all these methods when
>> including
>> core.h (which includes sfdp.h). No need to move them to core, as they
>> are
>> SFDP specific.
> 
> The same is true for the quad enable method and they reside in core.c.
> Again, I think sfdp.c should be about the parsing, not the flash
> handling
> itself. (And sfdp.h should be the equivalent to the spec in terms of
> constants). I mean SFDP will eventually cover everything, so will you
> move
> all methods over to sfdp.c? All these methods can be used with flashes
> without SFDP.
> 
> SFDP just collects all the different methods used by flash manufacturers
> and put them into a table. I don't see how SFDP is a spec where they
> specify
> a paricular method and all the flash manufacturer pick that up. I think
> it
> is the other way around, a flash manufacturer does something
> proprietary and then it eventually ends up in the SFDP spec.

It doesn't matter who was first to define the method. What matters is that
the method is backed-up by a standard. The JEDEC standard "[..] provides a
consistent method of describing the functional and feature capabilities of
serial flash devices [...]".

So if the functional method is described by the standard, let's use the
"standard" methods, and keep them in the standard's code.

> 
>>>> @@ -606,6 +693,24 @@ static int spi_nor_parse_bfpt(struct spi_nor
>>>> *nor,
>>>>               break;
>>>>       }
>>>>
>>>> +     switch (bfpt.dwords[BFPT_DWORD(16)] &
>>>> BFPT_DWORD16_4B_ADDR_MODE_MASK)
>>>> {
>>>
>>> I was wondering why this is encoded as single bits and not as an
>>> enumeration. To me it looks like it is intended to support
>>
>> because I parse 2 bits and check if both the enter and the exit methods
>> of
>> the 4b addr mode are specified.
> 
> No, I'm only speaking of either the entry or the exit mask. See
> below.
> 
>>> different methods at the same time. Thus I think there might be
>>> multiple bits set in each entry and exit mask at once. The spec
>>> lists all the entries as x_xxx1, x_xx1x, ..
>>>
>>>> +     case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
>>> .. then this will only match if exactly these two bits are set.
>>>
>>
>> these 2 bits are:
>> drivers/mtd/spi-nor/sfdp.h:#define BFPT_DWORD16_4B_ADDR_MODE_BRWR
>>                  \
>> drivers/mtd/spi-nor/sfdp.h-     (BFPT_DWORD16_EN4B_BRWR |
>> BFPT_DWORD16_EX4B_BRWR)
> 
> I know this are two bits, but IMHO there can be multiple bits
> set in *each* of these masks. Eg. the enter mask could be
> 0b00011 which would indicate that both the first and the second
> enter method is supported.
> (I'd expect that the exit mask will then be 0b00011, too).

I see. I can't contradict you here. How about:

u32 dword;

dword = bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK;
if ((dword & BFPT_DWORD16_4B_ADDR_MODE_BRWR) == BFPT_DWORD16_4B_ADDR_MODE_BRWR)
	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
else if ((dword & BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B) == BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B)
	params->set_4byte_addr_mode = micron_st_nor_set_4byte_addr_mode;
...
else
	dev_dbg(nor->dev, "BFPT: 4-Byte Address Mode method is not recognized or not implemented\n");

The first method hit will be used, regardless of how many methods are supported.

thanks,
ta

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

[Michael Walle]

Am 2022-04-14 13:05, schrieb Tudor.Ambarus@microchip.com:
> On 4/14/22 12:51, Michael Walle wrote:
>> EXTERNAL EMAIL: Do not click links or open attachments unless you know 
>> the content is safe
>> 
>>>>> diff --git a/drivers/mtd/spi-nor/sfdp.c 
>>>>> b/drivers/mtd/spi-nor/sfdp.c
>>>>> index a5211543d30d..2e40eba3744d 100644
>>>>> --- a/drivers/mtd/spi-nor/sfdp.c
>>>>> +++ b/drivers/mtd/spi-nor/sfdp.c
>>>>> @@ -401,6 +401,93 @@ 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)
>>>> 
>>>> Mh, so now some callback functions are in the core like the quad
>>>> enable
>>>> methods and some are in sfdp.c. I think there should be only the
>>>> parsing
>>>> in sfdp.c and all the callback methods should be in core.c; as they
>>>> are
>>>> potentially used by the vendor modules.
>>> 
>>> All set_4byte_addr_mode methods are defined in sfdp.c and declared in
>>> sfdp.h.
>>> I kept the methods defined in sfdp.c because SFDP defines their
>>> behavior/how
>>> they work. Vendors already have access to all these methods when
>>> including
>>> core.h (which includes sfdp.h). No need to move them to core, as they
>>> are
>>> SFDP specific.
>> 
>> The same is true for the quad enable method and they reside in core.c.
>> Again, I think sfdp.c should be about the parsing, not the flash
>> handling
>> itself. (And sfdp.h should be the equivalent to the spec in terms of
>> constants). I mean SFDP will eventually cover everything, so will you
>> move
>> all methods over to sfdp.c? All these methods can be used with flashes
>> without SFDP.
>> 
>> SFDP just collects all the different methods used by flash 
>> manufacturers
>> and put them into a table. I don't see how SFDP is a spec where they
>> specify
>> a paricular method and all the flash manufacturer pick that up. I 
>> think
>> it
>> is the other way around, a flash manufacturer does something
>> proprietary and then it eventually ends up in the SFDP spec.
> 
> It doesn't matter who was first to define the method. What matters is 
> that
> the method is backed-up by a standard. The JEDEC standard "[..] 
> provides a
> consistent method of describing the functional and feature capabilities 
> of
> serial flash devices [...]".
> 
> So if the functional method is described by the standard, let's use the
> "standard" methods, and keep them in the standard's code.

Fair enough, but what is then the difference between core.c and sfdp.c?

The core already holds all the "standard methods" because everything
else will be moved or was already moved into the vendor modules. Take
write_enable() for example, that is also covered by the SFDP standard,
should it be moved to sfdp.c too? And probably 90% of the code in
core.c is covered by the standard. I really don't see any advantages
for just moving the code around. Sorry if I repeat myself, but I already
find it really hard to actually use git blame inside mtd/spi-nor/ due to
the all the refactoring.

>>>>> @@ -606,6 +693,24 @@ static int spi_nor_parse_bfpt(struct spi_nor
>>>>> *nor,
>>>>>               break;
>>>>>       }
>>>>> 
>>>>> +     switch (bfpt.dwords[BFPT_DWORD(16)] &
>>>>> BFPT_DWORD16_4B_ADDR_MODE_MASK)
>>>>> {
>>>> 
>>>> I was wondering why this is encoded as single bits and not as an
>>>> enumeration. To me it looks like it is intended to support
>>> 
>>> because I parse 2 bits and check if both the enter and the exit 
>>> methods
>>> of
>>> the 4b addr mode are specified.
>> 
>> No, I'm only speaking of either the entry or the exit mask. See
>> below.
>> 
>>>> different methods at the same time. Thus I think there might be
>>>> multiple bits set in each entry and exit mask at once. The spec
>>>> lists all the entries as x_xxx1, x_xx1x, ..
>>>> 
>>>>> +     case BFPT_DWORD16_4B_ADDR_MODE_BRWR:
>>>> .. then this will only match if exactly these two bits are set.
>>>> 
>>> 
>>> these 2 bits are:
>>> drivers/mtd/spi-nor/sfdp.h:#define BFPT_DWORD16_4B_ADDR_MODE_BRWR
>>>                  \
>>> drivers/mtd/spi-nor/sfdp.h-     (BFPT_DWORD16_EN4B_BRWR |
>>> BFPT_DWORD16_EX4B_BRWR)
>> 
>> I know this are two bits, but IMHO there can be multiple bits
>> set in *each* of these masks. Eg. the enter mask could be
>> 0b00011 which would indicate that both the first and the second
>> enter method is supported.
>> (I'd expect that the exit mask will then be 0b00011, too).
> 
> I see. I can't contradict you here. How about:
> 
> u32 dword;
> 
> dword = bfpt.dwords[BFPT_DWORD(16)] & BFPT_DWORD16_4B_ADDR_MODE_MASK;
> if ((dword & BFPT_DWORD16_4B_ADDR_MODE_BRWR) == 
> BFPT_DWORD16_4B_ADDR_MODE_BRWR)
> 	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
> else if ((dword & BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B) ==
> BFPT_DWORD16_4B_ADDR_MODE_WREN_EN4B_EX4B)
> 	params->set_4byte_addr_mode = micron_st_nor_set_4byte_addr_mode;
> ...

Maybe with a bit more syntactic sugar :) Too bad this is already a
special case because it combines two different fields. The SFDP
standard seems to use the xxxx_xxx1, xxxx_xx1x, .. pattern quite
often. So for all the other fields you could have:

#define sfdp_bitfield(x) ((x) ? BIT(ffs(x)) : 0)

And an example for the SR status polling:

switch (sfdp_bitfield(bfpt.dwords[BFPT_DWORD(14)] & 
BFPT_DWORD14_SR_POLLING_MASK)) {
case BFPT_DWORD14_SR_POLLING_LEGACY:
  ...
case BFPT_DWORD14_SR_POLLING_SR_BIT7:
  ...
}

> else
> 	dev_dbg(nor->dev, "BFPT: 4-Byte Address Mode method is not recognized
> or not implemented\n");
> 
> The first method hit will be used, regardless of how many methods
> are supported.

sure.

-michael

[PATCH v3 2/3] 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>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
v3: no changes

 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 2b26a8cef0c3..4d45cda4f9d3 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2490,7 +2490,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 d2903c5b5b87..2d1cbb1f37c8 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -414,7 +414,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 2e40eba3744d..c5dd79ef75c8 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;
 
@@ -695,15 +701,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 3a3744da78ba..9606610569e0 100644
--- a/drivers/mtd/spi-nor/sfdp.h
+++ b/drivers/mtd/spi-nor/sfdp.h
@@ -133,9 +133,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 v3 2/3] mtd: spi-nor: Update name and description of the set_4byte_addr_mode BFPT methods

[Michael Walle]

Am 2022-04-11 14:53, schrieb 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>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>

Maybe we should mention which method is typically used by which
vendor in the function doc.

Reviewed-by: Michael Walle <michael@walle.cc>

[PATCH v3 3/3] 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>
Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
---
v3: no changes

 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 4d45cda4f9d3..888516d98884 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -2416,6 +2416,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);
@@ -2423,6 +2425,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);
 
@@ -2490,7 +2496,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 2d1cbb1f37c8..9e9b107f2018 100644
--- a/drivers/mtd/spi-nor/micron-st.c
+++ b/drivers/mtd/spi-nor/micron-st.c
@@ -414,14 +414,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 v3 3/3] mtd: spi-nor: Favor the BFPT-parsed set_4byte_addr_mode method

[Michael Walle]

Am 2022-04-11 14:53, schrieb 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>
> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
> ---
> v3: no changes
> 
>  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 4d45cda4f9d3..888516d98884 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -2416,6 +2416,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);
> @@ -2423,6 +2425,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;

Can this be moved past..

> +
>  	spi_nor_init_flags(nor);
>  	spi_nor_init_fixup_flags(nor);

.. these two lines, so it is next to the "set default
locking ops"?

> 
> @@ -2490,7 +2496,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;

This is more of a general question. Can we have this in one
line? IMHO this looks awful and since linux nowadays relaxed the
80 chars rule a bit and we have such long names.. I think it makes
sense to allow some lines to be longer than 80 chars.

>  }
> 
>  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 2d1cbb1f37c8..9e9b107f2018 100644
> --- a/drivers/mtd/spi-nor/micron-st.c
> +++ b/drivers/mtd/spi-nor/micron-st.c
> @@ -414,14 +414,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;

same here.

-michael

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

[Tudor.Ambarus]

On 4/14/22 12:21, Michael Walle wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Am 2022-04-11 14:53, schrieb 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>
>> Reviewed-by: Pratyush Yadav <p.yadav@ti.com>
>> ---
>> v3: no changes
>>
>>  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 4d45cda4f9d3..888516d98884 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -2416,6 +2416,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);
>> @@ -2423,6 +2425,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;
> 
> Can this be moved past..
> 
>> +
>>       spi_nor_init_flags(nor);
>>       spi_nor_init_fixup_flags(nor);
> 
> .. these two lines, so it is next to the "set default
> locking ops"?
> 

sure

>>
>> @@ -2490,7 +2496,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;
> 
> This is more of a general question. Can we have this in one
> line? IMHO this looks awful and since linux nowadays relaxed the
> 80 chars rule a bit and we have such long names.. I think it makes
> sense to allow some lines to be longer than 80 chars.

I still split my terminal in multiple windows and pretty much read
code in a 80 chars limit. But I won't argue with you, it seems you
care about it, so I'll change it.

> 
>>  }
>>
>>  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 2d1cbb1f37c8..9e9b107f2018 100644
>> --- a/drivers/mtd/spi-nor/micron-st.c
>> +++ b/drivers/mtd/spi-nor/micron-st.c
>> @@ -414,14 +414,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;
> 
> same here.
> 

sure

Thanks,
ta

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

[Pratyush Yadav]

On 14/04/22 09:32AM, Tudor.Ambarus@microchip.com wrote:
> On 4/14/22 12:21, Michael Walle wrote:
> >> +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;
> > 
> > This is more of a general question. Can we have this in one
> > line? IMHO this looks awful and since linux nowadays relaxed the
> > 80 chars rule a bit and we have such long names.. I think it makes
> > sense to allow some lines to be longer than 80 chars.
> 
> I still split my terminal in multiple windows and pretty much read
> code in a 80 chars limit. But I won't argue with you, it seems you
> care about it, so I'll change it.

FWIW, I do the same. I won't like it much if line lengths get too long. 
I won't take any sides here though, either should be fine as long as the 
result at least fits in 100 chars.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.