[PATCH 1/3] mtd: spi-nor: core: Add manuafacturer/chip specific operations

[PATCH 1/3] mtd: spi-nor: core: Add manuafacturer/chip specific operations

[yaliang.wang]

From: Yaliang Wang <Yaliang.Wang@windriver.com>

This is quite similar to the patch made by Takahiro Kuwano[1], except
replacing the bare ->ready() hook with a structure spi_nor_ops.

The purpose of this introduction is to provide a more flexible method,
so that we can expand it when needed. Also Basing on this, the
manufacturer specific checking ready codes can be shifted into their own
file.

[1]http://lists.infradead.org/pipermail/linux-mtd/2021-March/085741.html

Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
---
 drivers/mtd/spi-nor/core.c | 8 +++++---
 drivers/mtd/spi-nor/core.h | 9 +++++++++
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 0522304f52fa..6dc8bd0a6bd4 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -785,12 +785,13 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
 }
 
 /**
- * spi_nor_ready() - Query the flash to see if it is ready for new commands.
+ * spi_nor_default_ready() - Query the flash to see if it is ready for new
+ * commands.
  * @nor:	pointer to 'struct spi_nor'.
  *
  * Return: 1 if ready, 0 if not ready, -errno on errors.
  */
-static int spi_nor_ready(struct spi_nor *nor)
+static int spi_nor_default_ready(struct spi_nor *nor)
 {
 	int sr, fsr;
 
@@ -826,7 +827,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
 		if (time_after_eq(jiffies, deadline))
 			timeout = 1;
 
-		ret = spi_nor_ready(nor);
+		ret = nor->params->ops.ready(nor);
 		if (ret < 0)
 			return ret;
 		if (ret)
@@ -2920,6 +2921,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
 	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
 	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
 	params->setup = spi_nor_default_setup;
+	params->ops.ready = spi_nor_default_ready;
 	/* Default to 16-bit Write Status (01h) Command */
 	nor->flags |= SNOR_F_HAS_16BIT_SR;
 
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index 4a3f7f150b5d..bc042a0ef94e 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -187,6 +187,14 @@ struct spi_nor_locking_ops {
 	int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
 };
 
+/**
+ * struct spi_nor_ops - SPI manuafacturer/chip specific operations
+ * @ready:	query if a chip is ready.
+ */
+struct spi_nor_ops {
+	int (*ready)(struct spi_nor *nor);
+};
+
 /**
  * struct spi_nor_flash_parameter - SPI NOR flash parameters and settings.
  * Includes legacy flash parameters and settings that can be overwritten
@@ -232,6 +240,7 @@ struct spi_nor_flash_parameter {
 	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
 
 	struct spi_nor_erase_map        erase_map;
+	struct spi_nor_ops				ops;
 
 	int (*octal_dtr_enable)(struct spi_nor *nor, bool enable);
 	int (*quad_enable)(struct spi_nor *nor);
-- 
2.25.1


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

[PATCH 2/3] mtd: spi-nor: core: reuse spi_nor_clear_sr function

[yaliang.wang]

From: Yaliang Wang <Yaliang.Wang@windriver.com>

spi_nor_clear_fsr() and spi_nor_clear_sr() are almost the same, except
the opcode they used, the codes can be easily reused without much
changing.

Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
---
 drivers/mtd/spi-nor/core.c | 40 +++++++-------------------------------
 1 file changed, 7 insertions(+), 33 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index 6dc8bd0a6bd4..dbd6adb6aa0b 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -652,14 +652,15 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
 /**
  * spi_nor_clear_sr() - Clear the Status Register.
  * @nor:	pointer to 'struct spi_nor'.
+ * @opcode:	the SPI command op code to clear status register.
  */
-static void spi_nor_clear_sr(struct spi_nor *nor)
+static void spi_nor_clear_sr(struct spi_nor *nor, u8 opcode)
 {
 	int ret;
 
 	if (nor->spimem) {
 		struct spi_mem_op op =
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
+			SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
 				   SPI_MEM_OP_NO_ADDR,
 				   SPI_MEM_OP_NO_DUMMY,
 				   SPI_MEM_OP_NO_DATA);
@@ -668,12 +669,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
 
 		ret = spi_mem_exec_op(nor->spimem, &op);
 	} else {
-		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
+		ret = spi_nor_controller_ops_write_reg(nor, opcode,
 						       NULL, 0);
 	}
 
 	if (ret)
-		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
+		dev_dbg(nor->dev, "error %d clearing status\n", ret);
 }
 
 /**
@@ -697,7 +698,7 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
 		else
 			dev_err(nor->dev, "Programming Error occurred\n");
 
-		spi_nor_clear_sr(nor);
+		spi_nor_clear_sr(nor, SPINOR_OP_CLSR);
 
 		/*
 		 * WEL bit remains set to one when an erase or page program
@@ -715,33 +716,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
 	return !(nor->bouncebuf[0] & SR_WIP);
 }
 
-/**
- * spi_nor_clear_fsr() - Clear the Flag Status Register.
- * @nor:	pointer to 'struct spi_nor'.
- */
-static void spi_nor_clear_fsr(struct spi_nor *nor)
-{
-	int ret;
-
-	if (nor->spimem) {
-		struct spi_mem_op op =
-			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
-				   SPI_MEM_OP_NO_ADDR,
-				   SPI_MEM_OP_NO_DUMMY,
-				   SPI_MEM_OP_NO_DATA);
-
-		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_CLFSR,
-						       NULL, 0);
-	}
-
-	if (ret)
-		dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
-}
-
 /**
  * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
  * ready for new commands.
@@ -766,7 +740,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
 			dev_err(nor->dev,
 			"Attempted to modify a protected sector.\n");
 
-		spi_nor_clear_fsr(nor);
+		spi_nor_clear_sr(nor, SPINOR_OP_CLFSR);
 
 		/*
 		 * WEL bit remains set to one when an erase or page program
-- 
2.25.1


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

[PATCH 3/3] mtd: spi-nor: core: move Spansion checking ready codes into spansion.c

[yaliang.wang]

From: Yaliang Wang <Yaliang.Wang@windriver.com>

For historical reasons, Many manufacturer codes exist in the core, to
make the core a more dedicated place, this commit clear the
Spansion-specific checking SR ready codes out of the core.

Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
---
 drivers/mtd/spi-nor/core.c     | 26 +-----------------
 drivers/mtd/spi-nor/core.h     |  2 +-
 drivers/mtd/spi-nor/spansion.c | 48 ++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h    |  1 -
 4 files changed, 50 insertions(+), 27 deletions(-)

diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
index dbd6adb6aa0b..4e22aa3884db 100644
--- a/drivers/mtd/spi-nor/core.c
+++ b/drivers/mtd/spi-nor/core.c
@@ -654,7 +654,7 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
  * @nor:	pointer to 'struct spi_nor'.
  * @opcode:	the SPI command op code to clear status register.
  */
-static void spi_nor_clear_sr(struct spi_nor *nor, u8 opcode)
+void spi_nor_clear_sr(struct spi_nor *nor, u8 opcode)
 {
 	int ret;
 
@@ -691,28 +691,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
 	if (ret)
 		return ret;
 
-	if (nor->flags & SNOR_F_USE_CLSR &&
-	    nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
-		if (nor->bouncebuf[0] & SR_E_ERR)
-			dev_err(nor->dev, "Erase Error occurred\n");
-		else
-			dev_err(nor->dev, "Programming Error occurred\n");
-
-		spi_nor_clear_sr(nor, SPINOR_OP_CLSR);
-
-		/*
-		 * WEL bit remains set to one when an erase or page program
-		 * error occurs. Issue a Write Disable command to protect
-		 * against inadvertent writes that can possibly corrupt the
-		 * contents of the memory.
-		 */
-		ret = spi_nor_write_disable(nor);
-		if (ret)
-			return ret;
-
-		return -EIO;
-	}
-
 	return !(nor->bouncebuf[0] & SR_WIP);
 }
 
@@ -3488,8 +3466,6 @@ int spi_nor_scan(struct spi_nor *nor, const char *name,
 
 	if (info->flags & NO_CHIP_ERASE)
 		nor->flags |= SNOR_F_NO_OP_CHIP_ERASE;
-	if (info->flags & USE_CLSR)
-		nor->flags |= SNOR_F_USE_CLSR;
 	if (info->flags & SPI_NOR_SWP_IS_VOLATILE)
 		nor->flags |= SNOR_F_SWP_IS_VOLATILE;
 
diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
index bc042a0ef94e..b60585950288 100644
--- a/drivers/mtd/spi-nor/core.h
+++ b/drivers/mtd/spi-nor/core.h
@@ -16,7 +16,6 @@ enum spi_nor_option_flags {
 	SNOR_F_HAS_SR_TB	= BIT(1),
 	SNOR_F_NO_OP_CHIP_ERASE	= BIT(2),
 	SNOR_F_READY_XSR_RDY	= BIT(3),
-	SNOR_F_USE_CLSR		= BIT(4),
 	SNOR_F_BROKEN_RESET	= BIT(5),
 	SNOR_F_4B_OPCODES	= BIT(6),
 	SNOR_F_HAS_4BAIT	= BIT(7),
@@ -453,6 +452,7 @@ int spi_nor_read_sr(struct spi_nor *nor, u8 *sr);
 int spi_nor_read_cr(struct spi_nor *nor, u8 *cr);
 int spi_nor_write_sr(struct spi_nor *nor, const u8 *sr, size_t len);
 int spi_nor_write_sr_and_check(struct spi_nor *nor, u8 sr1);
+void spi_nor_clear_sr(struct spi_nor *nor, u8 opcode);
 
 int spi_nor_xread_sr(struct spi_nor *nor, u8 *sr);
 ssize_t spi_nor_read_data(struct spi_nor *nor, loff_t from, size_t len,
diff --git a/drivers/mtd/spi-nor/spansion.c b/drivers/mtd/spi-nor/spansion.c
index b0c5521c1e27..f90e0d8f9361 100644
--- a/drivers/mtd/spi-nor/spansion.c
+++ b/drivers/mtd/spi-nor/spansion.c
@@ -18,6 +18,48 @@
 #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_EN	0x3
 #define SPINOR_REG_CYPRESS_CFR5V_OCT_DTR_DS	0
 #define SPINOR_OP_CYPRESS_RD_FAST		0xee
+#define SPINOR_OP_SPANSION_CLSR			0x30
+
+/**
+ * spansion_sr_ready() - Spansion specific method for querying the flash to
+ * see if it is ready for new commands.
+ * @nor:   pointer to 'struct spi_nor'.
+ *
+ * Return: 1 if ready, 0 if not ready, -errno on errors.
+ */
+static int spansion_sr_ready(struct spi_nor *nor)
+{
+	u8 *sr = nor->bouncebuf;
+	int ret;
+
+	ret = spi_nor_read_sr(nor, sr);
+	if (ret)
+		return ret;
+
+	if (nor->info->flags & USE_CLSR &&
+	    nor->bouncebuf[0] & (SR_E_ERR | SR_P_ERR)) {
+		if (nor->bouncebuf[0] & SR_E_ERR)
+			dev_err(nor->dev, "Erase Error occurred\n");
+		else
+			dev_err(nor->dev, "Programming Error occurred\n");
+
+		spi_nor_clear_sr(nor, SPINOR_OP_SPANSION_CLSR);
+
+		/*
+		 * WEL bit remains set to one when an erase or page program
+		 * error occurs. Issue a Write Disable command to protect
+		 * against inadvertent writes that can possibly corrupt the
+		 * contents of the memory.
+		 */
+		ret = spi_nor_write_disable(nor);
+		if (ret)
+			return ret;
+
+		return -EIO;
+	}
+
+	return !(sr[0] & SR_WIP);
+}
 
 /**
  * spi_nor_cypress_octal_dtr_enable() - Enable octal DTR on Cypress flashes.
@@ -289,8 +331,14 @@ static void spansion_post_sfdp_fixups(struct spi_nor *nor)
 	nor->mtd.erasesize = nor->info->sector_size;
 }
 
+static void spansion_default_init(struct spi_nor *nor)
+{
+	nor->params->ops.ready = spansion_sr_ready;
+}
+
 static const struct spi_nor_fixups spansion_fixups = {
 	.post_sfdp = spansion_post_sfdp_fixups,
+	.default_init = spansion_default_init,
 };
 
 const struct spi_nor_manufacturer spi_nor_spansion = {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index a0d572855444..87e03943ba94 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -101,7 +101,6 @@
 
 /* Used for Spansion flashes only. */
 #define SPINOR_OP_BRWR		0x17	/* Bank register write */
-#define SPINOR_OP_CLSR		0x30	/* Clear status register 1 */
 
 /* Used for Micron flashes only. */
 #define SPINOR_OP_RD_EVCR      0x65    /* Read EVCR register */
-- 
2.25.1


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

Re: [PATCH 1/3] mtd: spi-nor: core: Add manuafacturer/chip specific operations

[Pratyush Yadav]

On 02/04/21 03:50AM, yaliang.wang@windriver.com wrote:
> From: Yaliang Wang <Yaliang.Wang@windriver.com>
> 
> This is quite similar to the patch made by Takahiro Kuwano[1], except
> replacing the bare ->ready() hook with a structure spi_nor_ops.

I don't think this belongs in the commit message. The commit message 
should describe the change in isolation. All the "metadata" like 
"depends on patch X", or "rework of patch Y", etc. should go below the 3 
dashes [2].

> 
> The purpose of this introduction is to provide a more flexible method,
> so that we can expand it when needed. Also Basing on this, the
> manufacturer specific checking ready codes can be shifted into their own
> file.
> 
> [1]http://lists.infradead.org/pipermail/linux-mtd/2021-March/085741.html
> 
> Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
> ---

[2] Here.

>  drivers/mtd/spi-nor/core.c | 8 +++++---
>  drivers/mtd/spi-nor/core.h | 9 +++++++++
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 0522304f52fa..6dc8bd0a6bd4 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -785,12 +785,13 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
>  }
>  
>  /**
> - * spi_nor_ready() - Query the flash to see if it is ready for new commands.
> + * spi_nor_default_ready() - Query the flash to see if it is ready for new
> + * commands.
>   * @nor:	pointer to 'struct spi_nor'.
>   *
>   * Return: 1 if ready, 0 if not ready, -errno on errors.
>   */
> -static int spi_nor_ready(struct spi_nor *nor)
> +static int spi_nor_default_ready(struct spi_nor *nor)
>  {
>  	int sr, fsr;
>  
> @@ -826,7 +827,7 @@ static int spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor,
>  		if (time_after_eq(jiffies, deadline))
>  			timeout = 1;
>  
> -		ret = spi_nor_ready(nor);
> +		ret = nor->params->ops.ready(nor);
>  		if (ret < 0)
>  			return ret;
>  		if (ret)
> @@ -2920,6 +2921,7 @@ static void spi_nor_info_init_params(struct spi_nor *nor)
>  	params->quad_enable = spi_nor_sr2_bit1_quad_enable;
>  	params->set_4byte_addr_mode = spansion_set_4byte_addr_mode;
>  	params->setup = spi_nor_default_setup;
> +	params->ops.ready = spi_nor_default_ready;
>  	/* Default to 16-bit Write Status (01h) Command */
>  	nor->flags |= SNOR_F_HAS_16BIT_SR;
>  
> diff --git a/drivers/mtd/spi-nor/core.h b/drivers/mtd/spi-nor/core.h
> index 4a3f7f150b5d..bc042a0ef94e 100644
> --- a/drivers/mtd/spi-nor/core.h
> +++ b/drivers/mtd/spi-nor/core.h
> @@ -187,6 +187,14 @@ struct spi_nor_locking_ops {
>  	int (*is_locked)(struct spi_nor *nor, loff_t ofs, uint64_t len);
>  };
>  
> +/**
> + * struct spi_nor_ops - SPI manuafacturer/chip specific operations
> + * @ready:	query if a chip is ready.
> + */
> +struct spi_nor_ops {
> +	int (*ready)(struct spi_nor *nor);

I don't understand how this is more flexible than just having the 
ready() hook in spi_nor_flash_parameter like Takahiro's patch did. I'm 
not completely opposed to the idea but I'd like to understand your 
thought process first.

Also...

> +};
> +
>  /**
>   * struct spi_nor_flash_parameter - SPI NOR flash parameters and settings.
>   * Includes legacy flash parameters and settings that can be overwritten
> @@ -232,6 +240,7 @@ struct spi_nor_flash_parameter {
>  	struct spi_nor_pp_command	page_programs[SNOR_CMD_PP_MAX];
>  
>  	struct spi_nor_erase_map        erase_map;
> +	struct spi_nor_ops				ops;
>  
>  	int (*octal_dtr_enable)(struct spi_nor *nor, bool enable);
>  	int (*quad_enable)(struct spi_nor *nor);

... shouldn't octal_dtr_enable(), quad_enable(), set_4byte_addr_mode(), 
convert_addr(), and setup() hooks also be moved into spi_nor_ops? Why is 
the ready() hook different from these hooks?

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 1/3] mtd: spi-nor: core: Add manuafacturer/chip specific operations

[Tudor.Ambarus]

On 4/5/21 11:54 AM, Pratyush Yadav wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you 
> know the content is safe
> 
> On 02/04/21 03:50AM, yaliang.wang@windriver.com wrote:
>> From: Yaliang Wang <Yaliang.Wang@windriver.com>
>> 
>> This is quite similar to the patch made by Takahiro Kuwano[1], 
>> except replacing the bare ->ready() hook with a structure 
>> spi_nor_ops.
> 
> I don't think this belongs in the commit message. The commit message
>  should describe the change in isolation. All the "metadata" like 
> "depends on patch X", or "rework of patch Y", etc. should go below 
> the 3 dashes [2].
> 
>> 
>> The purpose of this introduction is to provide a more flexible 
>> method, so that we can expand it when needed. Also Basing on this, 
>> the manufacturer specific checking ready codes can be shifted into 
>> their own file.
>> 
>> [1]http://lists.infradead.org/pipermail/linux-mtd/2021-March/085741.html
>>
>>
>>
>> 
Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
>> ---
> 
> [2] Here.
> 
>> drivers/mtd/spi-nor/core.c | 8 +++++--- drivers/mtd/spi-nor/core.h 
>> | 9 +++++++++ 2 files changed, 14 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/mtd/spi-nor/core.c 
>> b/drivers/mtd/spi-nor/core.c index 0522304f52fa..6dc8bd0a6bd4 
>> 100644 --- a/drivers/mtd/spi-nor/core.c +++ 
>> b/drivers/mtd/spi-nor/core.c @@ -785,12 +785,13 @@ static int 
>> spi_nor_fsr_ready(struct spi_nor *nor) }
>> 
>> /** - * spi_nor_ready() - Query the flash to see if it is ready
>> for new commands. + * spi_nor_default_ready() - Query the flash to
>> see if it is ready for new + * commands. * @nor:     pointer to
>> 'struct spi_nor'. * * Return: 1 if ready, 0 if not ready, -errno
>> on errors. */ -static int spi_nor_ready(struct spi_nor *nor)
>> +static int spi_nor_default_ready(struct spi_nor *nor) { int sr,
>> fsr;
>> 
>> @@ -826,7 +827,7 @@ static int 
>> spi_nor_wait_till_ready_with_timeout(struct spi_nor *nor, if 
>> (time_after_eq(jiffies, deadline)) timeout = 1;
>> 
>> -             ret = spi_nor_ready(nor); +             ret = 
>> nor->params->ops.ready(nor); if (ret < 0) return ret; if (ret) @@ 
>> -2920,6 +2921,7 @@ static void spi_nor_info_init_params(struct 
>> spi_nor *nor) params->quad_enable = spi_nor_sr2_bit1_quad_enable; 
>> params->set_4byte_addr_mode = spansion_set_4byte_addr_mode; 
>> params->setup = spi_nor_default_setup; +     params->ops.ready = 
>> spi_nor_default_ready; /* Default to 16-bit Write Status (01h) 
>> Command */ nor->flags |= SNOR_F_HAS_16BIT_SR;
>> 
>> diff --git a/drivers/mtd/spi-nor/core.h 
>> b/drivers/mtd/spi-nor/core.h index 4a3f7f150b5d..bc042a0ef94e 
>> 100644 --- a/drivers/mtd/spi-nor/core.h +++ 
>> b/drivers/mtd/spi-nor/core.h @@ -187,6 +187,14 @@ struct 
>> spi_nor_locking_ops { int (*is_locked)(struct spi_nor *nor, loff_t 
>> ofs, uint64_t len); };
>> 
>> +/** + * struct spi_nor_ops - SPI manuafacturer/chip specific 
>> operations + * @ready:   query if a chip is ready. + */ +struct 
>> spi_nor_ops { +     int (*ready)(struct spi_nor *nor);
> 
> I don't understand how this is more flexible than just having the 
> ready() hook in spi_nor_flash_parameter like Takahiro's patch did. 
> I'm not completely opposed to the idea but I'd like to understand 
> your thought process first.
> 

When I proposed the introduction of spi_nor_ops I thought about having one
place for storing different register operations or command opcodes. For example,
macronix uses for SPINOR_OP_RDCR a 0x15 value instead of 0x35. But maybe we'll
find a better place for these when parsing SCCR SFDP table. I'm ok with dropping
spi_nor_ops.

> Also...
> 
>> +}; + /** * struct spi_nor_flash_parameter - SPI NOR flash 
>> parameters and settings. * Includes legacy flash parameters and 
>> settings that can be overwritten @@ -232,6 +240,7 @@ struct 
>> spi_nor_flash_parameter { struct spi_nor_pp_command 
>> page_programs[SNOR_CMD_PP_MAX];
>> 
>> struct spi_nor_erase_map        erase_map; +     struct
>> spi_nor_ops ops;
>> 
>> int (*octal_dtr_enable)(struct spi_nor *nor, bool enable); int 
>> (*quad_enable)(struct spi_nor *nor);
> 
> ... shouldn't octal_dtr_enable(), quad_enable(), 
> set_4byte_addr_mode(), convert_addr(), and setup() hooks also be 
> moved into spi_nor_ops? Why is the ready() hook different from these 
> hooks?
> 

Right, maybe ready() resembles more the ones that you indicated. As the
spi_nor_flash_parameter is not yet big, we can keep all scattered for now.
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 2/3] mtd: spi-nor: core: reuse spi_nor_clear_sr function

[Pratyush Yadav]

On 02/04/21 03:50AM, yaliang.wang@windriver.com wrote:
> From: Yaliang Wang <Yaliang.Wang@windriver.com>
> 
> spi_nor_clear_fsr() and spi_nor_clear_sr() are almost the same, except
> the opcode they used, the codes can be easily reused without much
> changing.

Honestly, I am not sure how I feel about this. Yes, it would reduce some 
code duplication but at the same time reduces the readability slightly. 
spi_nor_clear_fsr(nor) is easier to read than spi_nor_clear_sr(nor, OP_CLFSR). 
I won't blame someone for missing that 'F' and thinking that it actually 
simply clears the SR. Dunno...

Anyway, if we do decide to go through with this change, the code changes 
look good to me.

> 
> Signed-off-by: Yaliang Wang <Yaliang.Wang@windriver.com>
> ---
>  drivers/mtd/spi-nor/core.c | 40 +++++++-------------------------------
>  1 file changed, 7 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> index 6dc8bd0a6bd4..dbd6adb6aa0b 100644
> --- a/drivers/mtd/spi-nor/core.c
> +++ b/drivers/mtd/spi-nor/core.c
> @@ -652,14 +652,15 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
>  /**
>   * spi_nor_clear_sr() - Clear the Status Register.
>   * @nor:	pointer to 'struct spi_nor'.
> + * @opcode:	the SPI command op code to clear status register.
>   */
> -static void spi_nor_clear_sr(struct spi_nor *nor)
> +static void spi_nor_clear_sr(struct spi_nor *nor, u8 opcode)
>  {
>  	int ret;
>  
>  	if (nor->spimem) {
>  		struct spi_mem_op op =
> -			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> +			SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
>  				   SPI_MEM_OP_NO_ADDR,
>  				   SPI_MEM_OP_NO_DUMMY,
>  				   SPI_MEM_OP_NO_DATA);
> @@ -668,12 +669,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>  
>  		ret = spi_mem_exec_op(nor->spimem, &op);
>  	} else {
> -		ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> +		ret = spi_nor_controller_ops_write_reg(nor, opcode,
>  						       NULL, 0);
>  	}
>  
>  	if (ret)
> -		dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> +		dev_dbg(nor->dev, "error %d clearing status\n", ret);
>  }
>  
>  /**
> @@ -697,7 +698,7 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
>  		else
>  			dev_err(nor->dev, "Programming Error occurred\n");
>  
> -		spi_nor_clear_sr(nor);
> +		spi_nor_clear_sr(nor, SPINOR_OP_CLSR);
>  
>  		/*
>  		 * WEL bit remains set to one when an erase or page program
> @@ -715,33 +716,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
>  	return !(nor->bouncebuf[0] & SR_WIP);
>  }
>  
> -/**
> - * spi_nor_clear_fsr() - Clear the Flag Status Register.
> - * @nor:	pointer to 'struct spi_nor'.
> - */
> -static void spi_nor_clear_fsr(struct spi_nor *nor)
> -{
> -	int ret;
> -
> -	if (nor->spimem) {
> -		struct spi_mem_op op =
> -			SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
> -				   SPI_MEM_OP_NO_ADDR,
> -				   SPI_MEM_OP_NO_DUMMY,
> -				   SPI_MEM_OP_NO_DATA);
> -
> -		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_CLFSR,
> -						       NULL, 0);
> -	}
> -
> -	if (ret)
> -		dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
> -}
> -
>  /**
>   * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
>   * ready for new commands.
> @@ -766,7 +740,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
>  			dev_err(nor->dev,
>  			"Attempted to modify a protected sector.\n");
>  
> -		spi_nor_clear_fsr(nor);
> +		spi_nor_clear_sr(nor, SPINOR_OP_CLFSR);
>  
>  		/*
>  		 * WEL bit remains set to one when an erase or page program

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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

Re: [PATCH 2/3] mtd: spi-nor: core: reuse spi_nor_clear_sr function

[Yaliang.Wang]

On 4/6/21 7:07 PM, Pratyush Yadav wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On 02/04/21 03:50AM,yaliang.wang@windriver.com  wrote:
>> From: Yaliang Wang<Yaliang.Wang@windriver.com>
>>
>> spi_nor_clear_fsr() and spi_nor_clear_sr() are almost the same, except
>> the opcode they used, the codes can be easily reused without much
>> changing.
> Honestly, I am not sure how I feel about this. Yes, it would reduce some
> code duplication but at the same time reduces the readability slightly.
> spi_nor_clear_fsr(nor) is easier to read than spi_nor_clear_sr(nor, OP_CLFSR).
> I won't blame someone for missing that 'F' and thinking that it actually
> simply clears the SR. Dunno...
>
> Anyway, if we do decide to go through with this change, the code changes
> look good to me.
The reason why am I doing this is quite simple, the contents of the 
function  are relatively  common, no other manufacturer specific codes 
involved, and it can be easily expanded to other manufacturers.  
Actually there is an instant example, "is25wp256" chip needs to clear 
error bits with using CLEAR EXTENDED READ REGISTER OPERATION (CLERP, 
82h) [1].

With that said, the name of the function do can be revised, maybe the 
name "spi_nor_clear_status()" is more proper?

[1]: https://www.issi.com/WW/pdf/25LP-WP256.pdf ; Page21,Page22 ;
>> Signed-off-by: Yaliang Wang<Yaliang.Wang@windriver.com>
>> ---
>>   drivers/mtd/spi-nor/core.c | 40 +++++++-------------------------------
>>   1 file changed, 7 insertions(+), 33 deletions(-)
>>
>> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
>> index 6dc8bd0a6bd4..dbd6adb6aa0b 100644
>> --- a/drivers/mtd/spi-nor/core.c
>> +++ b/drivers/mtd/spi-nor/core.c
>> @@ -652,14 +652,15 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
>>   /**
>>    * spi_nor_clear_sr() - Clear the Status Register.
>>    * @nor:     pointer to 'struct spi_nor'.
>> + * @opcode:  the SPI command op code to clear status register.
>>    */
>> -static void spi_nor_clear_sr(struct spi_nor *nor)
>> +static void spi_nor_clear_sr(struct spi_nor *nor, u8 opcode)
>>   {
>>        int ret;
>>
>>        if (nor->spimem) {
>>                struct spi_mem_op op =
>> -                     SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
>> +                     SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
>>                                   SPI_MEM_OP_NO_ADDR,
>>                                   SPI_MEM_OP_NO_DUMMY,
>>                                   SPI_MEM_OP_NO_DATA);
>> @@ -668,12 +669,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
>>
>>                ret = spi_mem_exec_op(nor->spimem, &op);
>>        } else {
>> -             ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
>> +             ret = spi_nor_controller_ops_write_reg(nor, opcode,
>>                                                       NULL, 0);
>>        }
>>
>>        if (ret)
>> -             dev_dbg(nor->dev, "error %d clearing SR\n", ret);
>> +             dev_dbg(nor->dev, "error %d clearing status\n", ret);
>>   }
>>
>>   /**
>> @@ -697,7 +698,7 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
>>                else
>>                        dev_err(nor->dev, "Programming Error occurred\n");
>>
>> -             spi_nor_clear_sr(nor);
>> +             spi_nor_clear_sr(nor, SPINOR_OP_CLSR);
>>
>>                /*
>>                 * WEL bit remains set to one when an erase or page program
>> @@ -715,33 +716,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
>>        return !(nor->bouncebuf[0] & SR_WIP);
>>   }
>>
>> -/**
>> - * spi_nor_clear_fsr() - Clear the Flag Status Register.
>> - * @nor:     pointer to 'struct spi_nor'.
>> - */
>> -static void spi_nor_clear_fsr(struct spi_nor *nor)
>> -{
>> -     int ret;
>> -
>> -     if (nor->spimem) {
>> -             struct spi_mem_op op =
>> -                     SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
>> -                                SPI_MEM_OP_NO_ADDR,
>> -                                SPI_MEM_OP_NO_DUMMY,
>> -                                SPI_MEM_OP_NO_DATA);
>> -
>> -             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_CLFSR,
>> -                                                    NULL, 0);
>> -     }
>> -
>> -     if (ret)
>> -             dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
>> -}
>> -
>>   /**
>>    * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
>>    * ready for new commands.
>> @@ -766,7 +740,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
>>                        dev_err(nor->dev,
>>                        "Attempted to modify a protected sector.\n");
>>
>> -             spi_nor_clear_fsr(nor);
>> +             spi_nor_clear_sr(nor, SPINOR_OP_CLFSR);
>>
>>                /*
>>                 * WEL bit remains set to one when an erase or page program
> --
> Regards,
> Pratyush Yadav
> Texas Instruments Inc.


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

Re: [PATCH 2/3] mtd: spi-nor: core: reuse spi_nor_clear_sr function

[Pratyush Yadav]

On 06/04/21 10:52PM, Yaliang.Wang wrote:
> 
> On 4/6/21 7:07 PM, Pratyush Yadav wrote:
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> > 
> > On 02/04/21 03:50AM,yaliang.wang@windriver.com  wrote:
> > > From: Yaliang Wang<Yaliang.Wang@windriver.com>
> > > 
> > > spi_nor_clear_fsr() and spi_nor_clear_sr() are almost the same, except
> > > the opcode they used, the codes can be easily reused without much
> > > changing.
> > Honestly, I am not sure how I feel about this. Yes, it would reduce some
> > code duplication but at the same time reduces the readability slightly.
> > spi_nor_clear_fsr(nor) is easier to read than spi_nor_clear_sr(nor, OP_CLFSR).
> > I won't blame someone for missing that 'F' and thinking that it actually
> > simply clears the SR. Dunno...
> > 
> > Anyway, if we do decide to go through with this change, the code changes
> > look good to me.
> The reason why am I doing this is quite simple, the contents of the
> function  are relatively  common, no other manufacturer specific codes
> involved, and it can be easily expanded to other manufacturers.  Actually
> there is an instant example, "is25wp256" chip needs to clear error bits with
> using CLEAR EXTENDED READ REGISTER OPERATION (CLERP, 82h) [1].

Ok.

> 
> With that said, the name of the function do can be revised, maybe the name
> "spi_nor_clear_status()" is more proper?

Yes, this is certainly better. While you're doing this then you might 
also want to look into spi_nor_read_sr() and spi_nor_read_fsr() and see 
if we can exploit similarities there as well.

> 
> [1]: https://www.issi.com/WW/pdf/25LP-WP256.pdf ; Page21,Page22 ;
> > > Signed-off-by: Yaliang Wang<Yaliang.Wang@windriver.com>
> > > ---
> > >   drivers/mtd/spi-nor/core.c | 40 +++++++-------------------------------
> > >   1 file changed, 7 insertions(+), 33 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c
> > > index 6dc8bd0a6bd4..dbd6adb6aa0b 100644
> > > --- a/drivers/mtd/spi-nor/core.c
> > > +++ b/drivers/mtd/spi-nor/core.c
> > > @@ -652,14 +652,15 @@ static int spi_nor_xsr_ready(struct spi_nor *nor)
> > >   /**
> > >    * spi_nor_clear_sr() - Clear the Status Register.
> > >    * @nor:     pointer to 'struct spi_nor'.
> > > + * @opcode:  the SPI command op code to clear status register.
> > >    */
> > > -static void spi_nor_clear_sr(struct spi_nor *nor)
> > > +static void spi_nor_clear_sr(struct spi_nor *nor, u8 opcode)
> > >   {
> > >        int ret;
> > > 
> > >        if (nor->spimem) {
> > >                struct spi_mem_op op =
> > > -                     SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLSR, 0),
> > > +                     SPI_MEM_OP(SPI_MEM_OP_CMD(opcode, 0),
> > >                                   SPI_MEM_OP_NO_ADDR,
> > >                                   SPI_MEM_OP_NO_DUMMY,
> > >                                   SPI_MEM_OP_NO_DATA);
> > > @@ -668,12 +669,12 @@ static void spi_nor_clear_sr(struct spi_nor *nor)
> > > 
> > >                ret = spi_mem_exec_op(nor->spimem, &op);
> > >        } else {
> > > -             ret = spi_nor_controller_ops_write_reg(nor, SPINOR_OP_CLSR,
> > > +             ret = spi_nor_controller_ops_write_reg(nor, opcode,
> > >                                                       NULL, 0);
> > >        }
> > > 
> > >        if (ret)
> > > -             dev_dbg(nor->dev, "error %d clearing SR\n", ret);
> > > +             dev_dbg(nor->dev, "error %d clearing status\n", ret);
> > >   }
> > > 
> > >   /**
> > > @@ -697,7 +698,7 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
> > >                else
> > >                        dev_err(nor->dev, "Programming Error occurred\n");
> > > 
> > > -             spi_nor_clear_sr(nor);
> > > +             spi_nor_clear_sr(nor, SPINOR_OP_CLSR);
> > > 
> > >                /*
> > >                 * WEL bit remains set to one when an erase or page program
> > > @@ -715,33 +716,6 @@ static int spi_nor_sr_ready(struct spi_nor *nor)
> > >        return !(nor->bouncebuf[0] & SR_WIP);
> > >   }
> > > 
> > > -/**
> > > - * spi_nor_clear_fsr() - Clear the Flag Status Register.
> > > - * @nor:     pointer to 'struct spi_nor'.
> > > - */
> > > -static void spi_nor_clear_fsr(struct spi_nor *nor)
> > > -{
> > > -     int ret;
> > > -
> > > -     if (nor->spimem) {
> > > -             struct spi_mem_op op =
> > > -                     SPI_MEM_OP(SPI_MEM_OP_CMD(SPINOR_OP_CLFSR, 0),
> > > -                                SPI_MEM_OP_NO_ADDR,
> > > -                                SPI_MEM_OP_NO_DUMMY,
> > > -                                SPI_MEM_OP_NO_DATA);
> > > -
> > > -             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_CLFSR,
> > > -                                                    NULL, 0);
> > > -     }
> > > -
> > > -     if (ret)
> > > -             dev_dbg(nor->dev, "error %d clearing FSR\n", ret);
> > > -}
> > > -
> > >   /**
> > >    * spi_nor_fsr_ready() - Query the Flag Status Register to see if the flash is
> > >    * ready for new commands.
> > > @@ -766,7 +740,7 @@ static int spi_nor_fsr_ready(struct spi_nor *nor)
> > >                        dev_err(nor->dev,
> > >                        "Attempted to modify a protected sector.\n");
> > > 
> > > -             spi_nor_clear_fsr(nor);
> > > +             spi_nor_clear_sr(nor, SPINOR_OP_CLFSR);
> > > 
> > >                /*
> > >                 * WEL bit remains set to one when an erase or page program
> > --
> > Regards,
> > Pratyush Yadav
> > Texas Instruments Inc.
> 

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

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