Add support for flag status register on Micron chips.

Are you sure the n25q512a doesn't use FSR ? Do n25q512a{1,8}3 share the same IDs?

Quoted 2 lines · Marek Vasut

Are you sure the n25q512a doesn't use FSR ? Do n25q512a{1,8}3 share the same IDs?

I looked at the datasheet and the n25q512a *does* have the same FSR usage note, so I suppose I should add USE_FSR to it as well. But how is it working now?

Maybe nobody is actually using that chip.
Yes, n25q512a{1,8}3 share the same id, 0x20ba20.

Quoted 2 lines · Marek Vasut

Are you sure the n25q512a doesn't use FSR ? Do n25q512a{1,8}3 share the same IDs?

I looked at the datasheet and the n25q512a *does* have the same FSR usage note, so I suppose I should add USE_FSR to it as well. But how is it working now? Maybe nobody is actually using that chip. Yes, n25q512a{1,8}3 share the same id, 0x20ba20.

Quoted 4 lines · Graham Moore

Quoted 2 lines · Marek Vasut

Are you sure the n25q512a doesn't use FSR ? Do n25q512a{1,8}3 share the same IDs?

Quoted 4 lines

I looked at the datasheet and the n25q512a *does* have the same FSR usage note, so I suppose I should add USE_FSR to it as well. But how is it working now? Maybe nobody is actually using that chip. Yes, n25q512a{1,8}3 share the same id, 0x20ba20.

could use the ext_id to distinguish them ? just as we did for Spansion NOR.

i dumped out 5 bytes of my n25q256a: 0x20, 0xba, 0x19, 0x10, 0x00.

---
V3:
Rebase to l2-mtd spinor branch.
V2:
Remove leading underscore in function names. Remove type cast in dev_err call and use the proper format specifier instead.

[ ... ]
+ * Read the flag status register, returning its value in the location
+ * Return the status register value.
+ * Returns negative if error occurred.
+ */

drivers/mtd/spi-nor/spi-nor.c +1 -0

+static int read_fsr(struct spi_nor *nor)

drivers/mtd/spi-nor/spi-nor.c +15 -0

@@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor *nor)
 	return -ETIMEDOUT;
 }
 
+static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
+{
+	unsigned long deadline;
+	int sr;
+	int fsr;
+
+	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
+
+	do {
+		cond_resched();
+
+		sr = read_sr(nor);
+		if (sr < 0)
+			break;
+		else if (!(sr & SR_WIP)) {

the patch appears to not have dev_err() references, were they removed? see below

this emits a message that an error has occured, but doesn't tell where it occured -- can you dev_err() here to make the message even more helpful?

this logic always returns "timed out" when the ready flag is not seen, even in the case of read errors -- can you "preset" the error code with "timed out", and update it with something more appropriate before returning when other errors are seen?

though this is an internal helper, and callers may not tell the situations apart in the first place, so this might be a minor nit

Some new Micron flash chips require reading the flag status register to determine when operations have completed.

Furthermore, chips with multi-die stacks of the 65nm 256Mb QSPI also require reading the status register before reading the flag status register.

This patch adds support for the flag status register in the n25q512ax3 and n25q00 Micron QSPI flash chips.

Signed-off-by Graham Moore <[email protected]>

---
V3:
Rebase to l2-mtd spinor branch.
V2:
Remove leading underscore in function names. Remove type cast in dev_err call and use the proper format specifier instead.

---
 drivers/mtd/spi-nor/spi-nor.c |   51 +++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nor.h   |    4 ++++
 2 files changed, 55 insertions(+)

drivers/mtd/spi-nor/spi-nor.c +48 -0

index d6f44d5..24b84d8 100644
@@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
 }
 
 /*
+ * Read the flag status register, returning its value in the location
+ * Return the status register value.
+ * Returns negative if error occurred.
+ */
+static int read_fsr(struct spi_nor *nor)
+{
+	int ret;
+	u8 val;
+
+	ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
+	if (ret < 0) {
+		pr_err("error %d reading FSR\n", ret);
+		return ret;
+	}
+
+	return val;
+}
+
+/*
  * Read configuration register, returning its value in the
  * location. Return the configuration register value.
  * Returns negative if error occured.

@@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor *nor)
 	return -ETIMEDOUT;
 }
 
+static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
+{
+	unsigned long deadline;
+	int sr;
+	int fsr;
+
+	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
+
+	do {
+		cond_resched();
+
+		sr = read_sr(nor);
+		if (sr < 0)
+			break;
+		else if (!(sr & SR_WIP)) {
+			fsr = read_fsr(nor);
+			if (fsr < 0)
+				break;
+			if (fsr & FSR_READY)
+				return 0;			
+		}
+	} while (!time_after_eq(jiffies, deadline));
+
+	return -ETIMEDOUT;
+}
+
 /*
  * Service routine to read status register until ready, or timeout occurs.
  * Returns non-zero if error.

@@ -402,6 +447,7 @@ struct flash_info {
 #define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works uniformly */
 #define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
 #define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
+#define	USE_FSR			0x80	/* use flag status register */
 };
 
 #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\

@@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_ids[] = {
 	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
 	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
 	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
+	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, USE_FSR) },
+	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR) },
 
 	/* PMC */
 	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },

Quoted 93 lines · Graham Moore

Some new Micron flash chips require reading the flag status register to determine when operations have completed.

Furthermore, chips with multi-die stacks of the 65nm 256Mb QSPI also require reading the status register before reading the flag status register.

This patch adds support for the flag status register in the n25q512ax3 and n25q00 Micron QSPI flash chips.

Signed-off-by Graham Moore <[email protected]>

---
V3:
Rebase to l2-mtd spinor branch.
V2:
Remove leading underscore in function names. Remove type cast in dev_err call and use the proper format specifier instead.
---

 drivers/mtd/spi-nor/spi-nor.c |   51
 +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h  
 |    4 ++++
 2 files changed, 55 insertions(+)

(quoted diff) +0 -0

b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644

drivers/mtd/spi-nor/spi-nor.c +19 -0

@@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
 
 }
 
 /*
 
+ * Read the flag status register, returning its value in the location
+ * Return the status register value.
+ * Returns negative if error occurred.
+ */
+static int read_fsr(struct spi_nor *nor)
+{
+	int ret;
+	u8 val;
+
+	ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
+	if (ret < 0) {
+		pr_err("error %d reading FSR\n", ret);
+		return ret;
+	}
+
+	return val;
+}
+
+/*
 

* Read configuration register, returning its value in the
* location. Return the configuration register value.
* Returns negative if error occured.

drivers/mtd/spi-nor/spi-nor.c +26 -0

@@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor
 *nor)
 
 	return -ETIMEDOUT;
 
 }
 
+static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
+{
+	unsigned long deadline;
+	int sr;
+	int fsr;
+
+	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
+
+	do {
+		cond_resched();
+
+		sr = read_sr(nor);
+		if (sr < 0)
+			break;
+		else if (!(sr & SR_WIP)) {
+			fsr = read_fsr(nor);
+			if (fsr < 0)
+				break;
+			if (fsr & FSR_READY)
+				return 0;
+		}
+	} while (!time_after_eq(jiffies, deadline));
+
+	return -ETIMEDOUT;
+}
+

drivers/mtd/spi-nor/spi-nor.c struct flash_info { · line 448

Quoted 8 lines

/*

* Service routine to read status register until ready, or timeout occurs. * Returns non-zero if error.

drivers/mtd/spi-nor/spi-nor.c +0 -0

@@ -402,6 +447,7 @@ struct flash_info {
 
 #define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works 

Quoted 22 lines

drivers/mtd/spi-nor/spi-nor.c +1 -0

 #define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
 #define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
 
+#define	USE_FSR			0x80	/* use flag status register */
 

};

#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \

drivers/mtd/spi-nor/spi-nor.c +2 -0

@@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_ids[] = {
 
 	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
 	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
 	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
 
+	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, USE_FSR) },
+	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR) },
 

drivers/mtd/spi-nor/spi-nor.c +0 -0

 	/* PMC */
 	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct · line 1021

Quoted 9 lines

drivers/mtd/spi-nor/spi-nor.c +3 -0

@@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct
 spi_device_id *id,
 
 	else
 
 		mtd->_write = spi_nor_write;
 
+	if (info->flags & USE_FSR)
+		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
+

the drivers may fills this hook itself, so the code should like this:

   --------------------------------------------------
	if ((info->flags & USE_FSR) &&
	       	nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
   --------------------------------------------------

Quoted 94 lines · Huang Shijie

Quoted 93 lines · Graham Moore

Some new Micron flash chips require reading the flag status register to determine when operations have completed.

Furthermore, chips with multi-die stacks of the 65nm 256Mb QSPI also require reading the status register before reading the flag status register.

This patch adds support for the flag status register in the n25q512ax3 and n25q00 Micron QSPI flash chips.

Signed-off-by Graham Moore <[email protected]>

---
V3:
Rebase to l2-mtd spinor branch.
V2:
Remove leading underscore in function names. Remove type cast in dev_err call and use the proper format specifier instead.
---

 drivers/mtd/spi-nor/spi-nor.c |   51
 +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h  
 |    4 ++++
 2 files changed, 55 insertions(+)

(quoted diff) +0 -0

b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644

drivers/mtd/spi-nor/spi-nor.c +19 -0

@@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
 
 }
 
 /*
 
+ * Read the flag status register, returning its value in the location
+ * Return the status register value.
+ * Returns negative if error occurred.
+ */
+static int read_fsr(struct spi_nor *nor)
+{
+	int ret;
+	u8 val;
+
+	ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
+	if (ret < 0) {
+		pr_err("error %d reading FSR\n", ret);
+		return ret;
+	}
+
+	return val;
+}
+
+/*
 

* Read configuration register, returning its value in the
* location. Return the configuration register value.
* Returns negative if error occured.

drivers/mtd/spi-nor/spi-nor.c +26 -0

@@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor
 *nor)
 
 	return -ETIMEDOUT;
 
 }
 
+static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
+{
+	unsigned long deadline;
+	int sr;
+	int fsr;
+
+	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
+
+	do {
+		cond_resched();
+
+		sr = read_sr(nor);
+		if (sr < 0)
+			break;
+		else if (!(sr & SR_WIP)) {
+			fsr = read_fsr(nor);
+			if (fsr < 0)
+				break;
+			if (fsr & FSR_READY)
+				return 0;
+		}
+	} while (!time_after_eq(jiffies, deadline));
+
+	return -ETIMEDOUT;
+}
+

Quoted 8 lines

drivers/mtd/spi-nor/spi-nor.c struct flash_info { · line 448

Quoted 8 lines

/*

* Service routine to read status register until ready, or timeout occurs. * Returns non-zero if error.

drivers/mtd/spi-nor/spi-nor.c +0 -0

@@ -402,6 +447,7 @@ struct flash_info {
 
 #define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works 

uniformly */

Quoted 33 lines

Quoted 22 lines

drivers/mtd/spi-nor/spi-nor.c +1 -0

 #define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
 #define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
 
+#define	USE_FSR			0x80	/* use flag status register */
 

};

#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \

drivers/mtd/spi-nor/spi-nor.c +2 -0

@@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_ids[] = {
 
 	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
 	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
 	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
 
+	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, USE_FSR) },
+	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR) },
 

drivers/mtd/spi-nor/spi-nor.c +0 -0

 	/* PMC */
 	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct · line 1021

Quoted 9 lines

drivers/mtd/spi-nor/spi-nor.c +3 -0

@@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct
 spi_device_id *id,
 
 	else
 
 		mtd->_write = spi_nor_write;
 
+	if (info->flags & USE_FSR)
+		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
+

the drivers may fills this hook itself, so the code should like this:

Quoted 5 lines

   --------------------------------------------------
	if ((info->flags & USE_FSR) &&
	       	nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
   --------------------------------------------------

I sense a misdesign of the SPI NOR subsystem here. The subsystem and the driver compete for a function pointer here ? I guess one should have precedence in some way then ... and also, they should be two different pointers, where the subsystem decides which to use.

Quoted 98 lines · Marek Vasut

Quoted 97 lines · Huang Shijie

Quoted 96 lines · Graham Moore

Some new Micron flash chips require reading the flag status register to determine when operations have completed.

Furthermore, chips with multi-die stacks of the 65nm 256Mb QSPI also require reading the status register before reading the flag status register.

This patch adds support for the flag status register in the n25q512ax3 and n25q00 Micron QSPI flash chips.

Signed-off-by Graham Moore <[email protected]>

---
V3:
Rebase to l2-mtd spinor branch.
V2:
Remove leading underscore in function names. Remove type cast in dev_err call and use the proper format specifier instead.
---

 drivers/mtd/spi-nor/spi-nor.c |   51

+++++++++++++++++++++++++++++++++++++++++
include/linux/mtd/spi-nor.h

| 4 ++++

 2 files changed, 55 insertions(+)

(quoted diff) +0 -0

b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644

drivers/mtd/spi-nor/spi-nor.c +1 -0

@@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
 
 }
 
 /*
 
+ * Read the flag status register, returning its value in the

location + * Return the status register value.
+ * Returns negative if error occurred.
+ */

drivers/mtd/spi-nor/spi-nor.c +41 -0

+static int read_fsr(struct spi_nor *nor)
+{
+	int ret;
+	u8 val;
+
+	ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
+	if (ret < 0) {
+		pr_err("error %d reading FSR\n", ret);
+		return ret;
+	}
+
+	return val;
+}
+
+/*
 
  * Read configuration register, returning its value in the
  * location. Return the configuration register value.
  * Returns negative if error occured.

@@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct
 spi_nor *nor)
 
 	return -ETIMEDOUT;
 
 }
 
+static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
+{
+	unsigned long deadline;
+	int sr;
+	int fsr;
+
+	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
+
+	do {
+		cond_resched();
+
+		sr = read_sr(nor);
+		if (sr < 0)
+			break;
+		else if (!(sr & SR_WIP)) {
+			fsr = read_fsr(nor);
+			if (fsr < 0)
+				break;
+			if (fsr & FSR_READY)
+				return 0;
+		}
+	} while (!time_after_eq(jiffies, deadline));
+
+	return -ETIMEDOUT;
+}
+

Quoted 8 lines

Quoted 8 lines

drivers/mtd/spi-nor/spi-nor.c struct flash_info { · line 448

Quoted 8 lines

/*

* Service routine to read status register until ready, or timeout occurs. * Returns non-zero if error.

drivers/mtd/spi-nor/spi-nor.c +0 -0

@@ -402,6 +447,7 @@ struct flash_info {
 
 #define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC 

Quoted 4 lines

uniformly */

Quoted 1 line

Quoted 1 line

drivers/mtd/spi-nor/spi-nor.c +0 -0

 #define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual 

Quoted 1 line

Quoted 1 line

drivers/mtd/spi-nor/spi-nor.c struct flash_info { · line 450

Quoted 1 line

drivers/mtd/spi-nor/spi-nor.c +0 -0

 #define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad 

Quoted 2 lines

Quoted 2 lines

drivers/mtd/spi-nor/spi-nor.c struct flash_info { · line 451

Quoted 2 lines

drivers/mtd/spi-nor/spi-nor.c +1 -0

+#define	USE_FSR			0x80	/* use flag status 

Quoted 11 lines

Quoted 11 lines

Quoted 11 lines

drivers/mtd/spi-nor/spi-nor.c +0 -0

 };
 

#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \

drivers/mtd/spi-nor/spi-nor.c +0 -0

@@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_ids[] = {
 
 	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
 	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
 	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
 

Quoted 5 lines

Quoted 5 lines

drivers/mtd/spi-nor/spi-nor.c

Quoted 5 lines

drivers/mtd/spi-nor/spi-nor.c +2 -0

+	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, USE_FSR) },
+	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR) },
 

drivers/mtd/spi-nor/spi-nor.c +0 -0

 	/* PMC */
 	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) 

Quoted 19 lines

Quoted 19 lines

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const · line 1021

Quoted 11 lines

drivers/mtd/spi-nor/spi-nor.c +3 -0

@@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const
 struct spi_device_id *id,
 
 	else
 
 		mtd->_write = spi_nor_write;
 
+	if (info->flags & USE_FSR)
+		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
+

the drivers may fills this hook itself, so the code should like this:

   --------------------------------------------------
	
	if ((info->flags & USE_FSR) &&
	
	       	nor->wait_till_ready == spi_nor_wait_till_fsr_ready)

Quoted 8 lines

Quoted 3 lines

		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;

   --------------------------------------------------

I sense a misdesign of the SPI NOR subsystem here. The subsystem and the driver compete for a function pointer here ? I guess one should have precedence in some way then ... and also, they should be two different pointers, where the subsystem decides which to use.

the subsystem do not decides which one to use, the driver decides which one to use.

If driver has its own @wait_till_ready , it means the driver knows the feature, and has implemented it in its own @wait_till_ready.

If the driver does not fill any wait_till_ready, it means the driver will use the default @wait_till_ready. We can treat the spi_nor_wait_till_fsr_ready as a default hook too.

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

the drivers may fills this hook itself, so the code should like this:

   --------------------------------------------------
	
	if ((info->flags & USE_FSR) &&
	
	       	nor->wait_till_ready == spi_nor_wait_till_fsr_ready)

Quoted 8 lines

Quoted 3 lines

		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;

   --------------------------------------------------

I sense a misdesign of the SPI NOR subsystem here. The subsystem and the driver compete for a function pointer here ? I guess one should have precedence in some way then ... and also, they should be two different pointers, where the subsystem decides which to use.

the subsystem do not decides which one to use, the driver decides which one to use.

Quoted 6 lines

If driver has its own @wait_till_ready , it means the driver knows the feature, and has implemented it in its own @wait_till_ready.

If the driver does not fill any wait_till_ready, it means the driver will use the default @wait_till_ready. We can treat the spi_nor_wait_till_fsr_ready as a default hook too.

I see the driver overwriting a hook previously set by the subsystem. This

Quoted 19 lines · Marek Vasut

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

the drivers may fills this hook itself, so the code should like this:

   --------------------------------------------------
	
	if ((info->flags & USE_FSR) &&
	
	       	nor->wait_till_ready == spi_nor_wait_till_fsr_ready)

Quoted 8 lines

Quoted 3 lines

		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;

   --------------------------------------------------

I sense a misdesign of the SPI NOR subsystem here. The subsystem and the driver compete for a function pointer here ? I guess one should have precedence in some way then ... and also, they should be two different pointers, where the subsystem decides which to use.

the subsystem do not decides which one to use, the driver decides which one to use.

Quoted 8 lines

Quoted 6 lines

If driver has its own @wait_till_ready , it means the driver knows the feature, and has implemented it in its own @wait_till_ready.

If the driver does not fill any wait_till_ready, it means the driver will use the default @wait_till_ready. We can treat the spi_nor_wait_till_fsr_ready as a default hook too.

I see the driver overwriting a hook previously set by the subsystem. This

not sure ;)

The driver set the hooks before the subsystem set these hooks.

If the driver has already set the @wait_till_ready hook before it calls the spi_nor_scan, the subsystem will not set the hook anymore.

Please see the spi_nor_check().

Quoted 33 lines · Huang Shijie

Quoted 19 lines · Marek Vasut

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

the drivers may fills this hook itself, so the code should like this:

   --------------------------------------------------
	
	if ((info->flags & USE_FSR) &&
	
	       	nor->wait_till_ready == spi_nor_wait_till_fsr_ready)

Quoted 8 lines

Quoted 3 lines

		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;

   --------------------------------------------------

I sense a misdesign of the SPI NOR subsystem here. The subsystem and the driver compete for a function pointer here ? I guess one should have precedence in some way then ... and also, they should be two different pointers, where the subsystem decides which to use.

the subsystem do not decides which one to use, the driver decides which one to use.

Quoted 8 lines

Quoted 6 lines

If driver has its own @wait_till_ready , it means the driver knows the feature, and has implemented it in its own @wait_till_ready.

If the driver does not fill any wait_till_ready, it means the driver will use the default @wait_till_ready. We can treat the spi_nor_wait_till_fsr_ready as a default hook too.

I see the driver overwriting a hook previously set by the subsystem. This

not sure ;)

The driver set the hooks before the subsystem set these hooks.

Quoted 4 lines

If the driver has already set the @wait_till_ready hook before it calls the spi_nor_scan, the subsystem will not set the hook anymore.

Please see the spi_nor_check().

Two things competing over the same pointer looks misdesigned to me. I will need to dig into this one more time ...

Quoted 3 lines · Marek Vasut

Two things competing over the same pointer looks misdesigned to me. I will need to dig into this one more time ...

Please refer to the code for NAND chip, the nand_get_flash_type() :

-----------------------------------------------------------

        /* Do not replace user supplied command function! */
        if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
                chip->cmdfunc = nand_command_lp;

-----------------------------------------------------------

It uses the same logic:
" Do not replace user supplied command function!"

thanks
Huang Shijie

Quoted 12 lines · Huang Shijie

Quoted 3 lines · Marek Vasut

Two things competing over the same pointer looks misdesigned to me. I will need to dig into this one more time ...

Please refer to the code for NAND chip, the nand_get_flash_type() :

-----------------------------------------------------------

        /* Do not replace user supplied command function! */
        if (mtd->writesize > 512 && chip->cmdfunc == nand_command)
                chip->cmdfunc = nand_command_lp;

-----------------------------------------------------------

Quoted 5 lines

It uses the same logic:
" Do not replace user supplied command function!"

thanks
Huang Shijie

I would like to set the @wait_till_ready in m25p80.c, because the USE_FSR flag is only for Micron chips. But the m25p80 driver doesn't

Quoted 33 lines · Huang Shijie

Quoted 19 lines · Marek Vasut

Quoted 19 lines

Quoted 7 lines

Quoted 7 lines

the drivers may fills this hook itself, so the code should like this:

   --------------------------------------------------
	
	if ((info->flags & USE_FSR) &&
	
	       	nor->wait_till_ready == spi_nor_wait_till_fsr_ready)

Quoted 8 lines

Quoted 3 lines

		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;

   --------------------------------------------------

I sense a misdesign of the SPI NOR subsystem here. The subsystem and the driver compete for a function pointer here ? I guess one should have precedence in some way then ... and also, they should be two different pointers, where the subsystem decides which to use.

the subsystem do not decides which one to use, the driver decides which one to use.

Quoted 8 lines

Quoted 6 lines

If driver has its own @wait_till_ready , it means the driver knows the feature, and has implemented it in its own @wait_till_ready.

If the driver does not fill any wait_till_ready, it means the driver will use the default @wait_till_ready. We can treat the spi_nor_wait_till_fsr_ready as a default hook too.

I see the driver overwriting a hook previously set by the subsystem. This

not sure ;)

The driver set the hooks before the subsystem set these hooks.

Quoted 4 lines

If the driver has already set the @wait_till_ready hook before it calls the spi_nor_scan, the subsystem will not set the hook anymore.

Please see the spi_nor_check().

Two things competing over the same pointer looks misdesigned to me. I will need to dig into this one more time ...

Quoted 96 lines · Huang Shijie

Quoted 95 lines · Graham Moore

Some new Micron flash chips require reading the flag status register to determine when operations have completed.

Furthermore, chips with multi-die stacks of the 65nm 256Mb QSPI also require reading the status register before reading the flag status register.

This patch adds support for the flag status register in the n25q512ax3 and n25q00 Micron QSPI flash chips.

Signed-off-by Graham Moore <[email protected]>

---
V3:
Rebase to l2-mtd spinor branch.
V2:
Remove leading underscore in function names. Remove type cast in dev_err call and use the proper format specifier instead.
---

 drivers/mtd/spi-nor/spi-nor.c |   51
 +++++++++++++++++++++++++++++++++++++++++ include/linux/mtd/spi-nor.h  
 |    4 ++++
 2 files changed, 55 insertions(+)

(quoted diff) +0 -0

b/drivers/mtd/spi-nor/spi-nor.c index d6f44d5..24b84d8 100644

drivers/mtd/spi-nor/spi-nor.c +19 -0

@@ -48,6 +48,25 @@ static int read_sr(struct spi_nor *nor)
 
 }
 
 /*
 
+ * Read the flag status register, returning its value in the location
+ * Return the status register value.
+ * Returns negative if error occurred.
+ */
+static int read_fsr(struct spi_nor *nor)
+{
+	int ret;
+	u8 val;
+
+	ret = nor->read_reg(nor, SPINOR_OP_RDFSR, &val, 1);
+	if (ret < 0) {
+		pr_err("error %d reading FSR\n", ret);
+		return ret;
+	}
+
+	return val;
+}
+
+/*
 

* Read configuration register, returning its value in the
* location. Return the configuration register value.
* Returns negative if error occured.

drivers/mtd/spi-nor/spi-nor.c +26 -0

@@ -165,6 +184,32 @@ static int spi_nor_wait_till_ready(struct spi_nor
 *nor)
 
 	return -ETIMEDOUT;
 
 }
 
+static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
+{
+	unsigned long deadline;
+	int sr;
+	int fsr;
+
+	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
+
+	do {
+		cond_resched();
+
+		sr = read_sr(nor);
+		if (sr < 0)
+			break;
+		else if (!(sr & SR_WIP)) {
+			fsr = read_fsr(nor);
+			if (fsr < 0)
+				break;
+			if (fsr & FSR_READY)
+				return 0;
+		}
+	} while (!time_after_eq(jiffies, deadline));
+
+	return -ETIMEDOUT;
+}
+

/*

Quoted 6 lines

drivers/mtd/spi-nor/spi-nor.c struct flash_info { · line 448

Quoted 6 lines

* Service routine to read status register until ready, or timeout occurs. * Returns non-zero if error.

drivers/mtd/spi-nor/spi-nor.c +0 -0

@@ -402,6 +447,7 @@ struct flash_info {
 
 #define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works 

uniformly */

Quoted 38 lines

Quoted 22 lines

drivers/mtd/spi-nor/spi-nor.c +1 -0

 #define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
 #define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
 
+#define	USE_FSR			0x80	/* use flag status register */
 

};

#define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags) \

drivers/mtd/spi-nor/spi-nor.c +2 -0

@@ -488,6 +534,8 @@ const struct spi_device_id spi_nor_ids[] = {
 
 	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
 	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
 	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
 
+	{ "n25q512ax3",  INFO(0x20ba20, 0, 64 * 1024, 1024, USE_FSR) },
+	{ "n25q00",      INFO(0x20ba21, 0, 64 * 1024, 2048, USE_FSR) },
 

drivers/mtd/spi-nor/spi-nor.c +0 -0

 	/* PMC */
 	{ "pm25lv512",   INFO(0,        0, 32 * 1024,    2, SECT_4K_PMC) },

drivers/mtd/spi-nor/spi-nor.c int spi_nor_scan(struct spi_nor *nor, const struct · line 1021

Quoted 9 lines

drivers/mtd/spi-nor/spi-nor.c +3 -0

@@ -965,6 +1013,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct
 spi_device_id *id,
 
 	else
 
 		mtd->_write = spi_nor_write;
 
+	if (info->flags & USE_FSR)
+		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
+

the drivers may fills this hook itself, so the code should like this:

   --------------------------------------------------
	if ((info->flags & USE_FSR) &&
	       	nor->wait_till_ready == spi_nor_wait_till_fsr_ready)
		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
   --------------------------------------------------