mtd: spi-nor: refactor wait-till-ready

These functions were near-carbon-copies due to a small per-flash quirk. Let's add a new spi_nor::flags bitfield to support these types of quirks.

Signed-off-by Brian Norris <[email protected]>

Cc Graham Moore <[email protected]>

Cc Huang Shijie <[email protected]>

Signed-off-by Brian Norris <[email protected]>

These functions were near-carbon-copies due to a small per-flash quirk. Let's add a new spi_nor::flags bitfield to support these types of quirks.

Signed-off-by Brian Norris <[email protected]>

Cc Graham Moore <[email protected]>

Cc Huang Shijie <[email protected]>

Signed-off-by Brian Norris <[email protected]>

---
 drivers/mtd/spi-nor/spi-nor.c | 66 ++++++++++++++++++++++---------------------
 include/linux/mtd/spi-nor.h   |  6 ++++
 2 files changed, 40 insertions(+), 32 deletions(-)

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

index b5ad6bebf5e7..5825b8a12cee 100644
@@ -163,48 +163,51 @@ static inline int set_4byte(struct spi_nor *nor, u32 jedec_id, int enable)
 		return nor->write_reg(nor, SPINOR_OP_BRWR, nor->cmd_buf, 1, 0);
 	}
 }
-
-static int spi_nor_wait_till_ready(struct spi_nor *nor)
+static inline int spi_nor_sr_ready(struct spi_nor *nor)
 {
-	unsigned long deadline;
-	int sr;
-
-	deadline = jiffies + MAX_READY_WAIT_JIFFIES;
-
-	do {
-		cond_resched();
+	int sr = read_sr(nor);
+	if (sr < 0)
+		return sr;
+	else
+		return !(sr & SR_WIP);
+}
 
-		sr = read_sr(nor);
-		if (sr < 0)
-			break;
-		else if (!(sr & SR_WIP))
-			return 0;
-	} while (!time_after_eq(jiffies, deadline));
+static inline int spi_nor_fsr_ready(struct spi_nor *nor)
+{
+	int fsr = read_fsr(nor);
+	if (fsr < 0)
+		return fsr;
+	else
+		return fsr & FSR_READY;
+}
 

drivers/mtd/spi-nor/spi-nor.c static inline int set_4byte(struct spi_nor *nor, u32 jedec_id, int enable) · line 192

Quoted 3 lines · Brian Norris

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

+	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
+	if (fsr < 0)
+		return sr;

return "fsr" here.

The error label was unused here. It looks like we're missing at least one case that should be doing 'goto write_err'.

Signed-off-by Brian Norris <[email protected]>

The error label was unused here. It looks like we're missing at least one case that should be doing 'goto write_err'.

Signed-off-by Brian Norris <[email protected]>

---
 drivers/mtd/spi-nor/spi-nor.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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

index 5825b8a12cee..d77c93232b76 100644
@@ -809,7 +809,10 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 			if (page_size > nor->page_size)
 				page_size = nor->page_size;
 
-			wait_till_ready(nor);
+			ret = wait_till_ready(nor);
+			if (ret)
+				goto write_err;
+
 			write_enable(nor);
 
 			nor->write(nor, to + i, page_size, retlen, buf + i);

@@ -818,7 +821,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 write_err:
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
-	return 0;
+	return ret;
 }
 
 static int macronix_quad_enable(struct spi_nor *nor)

Signature · 1 line

1.9.1

We shouldn't have *every* function checking if a previous write is complete; this should be done synchronously after each write/erase.

Signed-off-by Brian Norris <[email protected]>

We shouldn't have *every* function checking if a previous write is complete; this should be done synchronously after each write/erase.

Quoted 2 lines · Brian Norris

We shouldn't have *every* function checking if a previous write is complete; this should be done synchronously after each write/erase.

IMHO, this is not a good idea. :(

this patch prevents the erase-suspend and program-suspend. We should add these two features for spi-nor in future.

Quoted 5 lines · Huang Shijie

Quoted 2 lines · Brian Norris

We shouldn't have *every* function checking if a previous write is complete; this should be done synchronously after each write/erase.

IMHO, this is not a good idea. :(

Do you mean you think it is a good idea for every unrelated function to check if the previous erase/write is complete?

Quoted 3 lines

this patch prevents the erase-suspend and program-suspend. We should add these two features for spi-nor in future.

OK, how would you propose that such features be implemented, and how would they be used to the benefit of higher layers?

Directed toward the former: specifically, how does leaving the SR/FSR-checking burden on all subsequent commands benefit a potential erase/program suspend implementation? The code is not written at all with erase/program suspend in mind, and the current patch solves a current problem; that we perform checking in all the wrong places.

To the latter: are file systems (e.g., UBIFS) aware of suspend-able program/erase? Would they have the knowledge to take advantage of

suspend-able program/erase? i.e., could they suspend an unimportant erase command in order to prioritize a read or write?

Finally, this patch mostly prepares for elimination of code from lower-level drivers (m25p80.c and fsl-quadspi, in the following two patches). These drivers should *not* be worrying about the details of command statuses; this should be handled by the generic code (spi-nor.c).

So, unless you can provide some outline as to how program/erase suspend can be implemented reasonably within this framework, and how this particular patch makes that so much more difficult, I plan to move forward with this.

spi-nor.c should be taking care of these now.

Signed-off-by Brian Norris <[email protected]>

spi-nor.c should be taking care of these now.

Signed-off-by Brian Norris <[email protected]>

Cc Huang Shijie <[email protected]>

We don't need to expose a 'wait-till-ready' interface to drivers. Status register polling should be handled by the core spi-nor.c library, and as of now, I see no need to provide a special driver-specific hook for it.

Signed-off-by Brian Norris <[email protected]>

We don't need to expose a 'wait-till-ready' interface to drivers. Status register polling should be handled by the core spi-nor.c library, and as of now, I see no need to provide a special driver-specific hook for it.

Quoted 3 lines · Brian Norris

We don't need to expose a 'wait-till-ready' interface to drivers. Status register polling should be handled by the core spi-nor.c library, and as of now, I see no need to provide a special driver-specific hook for it.

Please do not drop this hook, please see why we add this hook:
http://lists.infradead.org/pipermail/linux-mtd/2013-December/050561.html

"
The default implementation would do just as you suggest, and I would expect most H/W drivers to use the default implementation. However, some H/W Controllers can automate the polling of status registers, so having a hook allows the driver override the default behaviour.
"

The spi-nor framework will used by more drivers besides the m25p80 and fsl-quadspi. Some NOR flash controller may be very strange.

Quoted 15 lines · Huang Shijie

Quoted 3 lines · Brian Norris

We don't need to expose a 'wait-till-ready' interface to drivers. Status register polling should be handled by the core spi-nor.c library, and as of now, I see no need to provide a special driver-specific hook for it.

Please do not drop this hook, please see why we add this hook:
http://lists.infradead.org/pipermail/linux-mtd/2013-December/050561.html

"
The default implementation would do just as you suggest, and I would expect most H/W drivers to use the default implementation. However, some H/W Controllers can automate the polling of status registers, so having a hook allows the driver override the default behaviour.
"

Quoted 2 lines

The spi-nor framework will used by more drivers besides the m25p80 and fsl-quadspi. Some NOR flash controller may be very strange.

OK, but the wait-till-ready hook should not look like the current one. If it is used as an optimization of sorts, then we can provide it in *addition* to the checks we do, but not *instead of*. I sincerely doubt that any specialized SPI NOR controller will know how to check both SR and FSR where appropriate, and it probably won't understand other specialized sequences as they are developed / thrown on our lap by flash vendors.

So, the spi-nor.c "wait until ready" might have a sequence like this:

1. (Optionally) use low-level driver's "wait" function; skip if not present

2. use the read register hooks to check SR/FSR to confirm completion

I do not want to toss #2 into the low-level driver, so if there is a need for #1, it should be done in addition to our spi-nor.c code, not instead. (To this end, I believe Marek also complained about this when we were adding the FSR-checking code; we should not have drivers and spi-nor.c fighting over callback functions.)

So I'm inclined to at most address #1 through an optional callback, and

possibly even to ignore that until we see an actual driver that needs it.

Quoted 40 lines · Brian Norris

Quoted 15 lines · Huang Shijie

Quoted 3 lines · Brian Norris

We don't need to expose a 'wait-till-ready' interface to drivers. Status register polling should be handled by the core spi-nor.c library, and as of now, I see no need to provide a special driver-specific hook for it.

Please do not drop this hook, please see why we add this hook:
http://lists.infradead.org/pipermail/linux-mtd/2013-December/050561.html

"
The default implementation would do just as you suggest, and I would expect most H/W drivers to use the default implementation. However, some H/W Controllers can automate the polling of status registers, so having a hook allows the driver override the default behaviour.
"

Quoted 2 lines

The spi-nor framework will used by more drivers besides the m25p80 and fsl-quadspi. Some NOR flash controller may be very strange.

OK, but the wait-till-ready hook should not look like the current one. If it is used as an optimization of sorts, then we can provide it in *addition* to the checks we do, but not *instead of*. I sincerely doubt that any specialized SPI NOR controller will know how to check both SR and FSR where appropriate, and it probably won't understand other specialized sequences as they are developed / thrown on our lap by flash vendors.

So, the spi-nor.c "wait until ready" might have a sequence like this:

1. (Optionally) use low-level driver's "wait" function; skip if not present

2. use the read register hooks to check SR/FSR to confirm completion

I do not want to toss #2 into the low-level driver, so if there is a need for #1, it should be done in addition to our spi-nor.c code, not instead. (To this end, I believe Marek also complained about this when we were adding the FSR-checking code; we should not have drivers and spi-nor.c fighting over callback functions.)

Quoted 1 line

So I'm inclined to at most address #1 through an optional callback, and

but where to put the optional callback? in the spi_nor{} ?

Quoted 2 lines

possibly even to ignore that until we see an actual driver that needs it.

If you insist to remove it, i will be okay too. anyway, we can add it back when an actual driver appears.

We don't need to expose a 'wait-till-ready' interface to drivers. Status register polling should be handled by the core spi-nor.c library, and as of now, I see no need to provide a special driver-specific hook for it.

From what I can see, Atmel-specific code uses command 0xD7 (checking

Quoted 3 lines · Brian Norris

We don't need to expose a 'wait-till-ready' interface to drivers. Status register polling should be handled by the core spi-nor.c library, and as of now, I see no need to provide a special driver-specific hook for it.

Do you think we could use spi-nor for Broadcom's SPI Atmel flashes? If so, we could need this.

Broadcom's code includes support for two Atmel flashes: AT25F512 and AT25F2048.

From what I can see, Atmel-specific code uses command 0xD7 (checking for bit 0x80 to be unset). That is a bit different from "standard" flashes using command 0xD7 (checking for bit 0x80 to be unset).

write_enable() was being duplicated to both m25p80.c and fsl-quadspi.c. But this should be handled within the spi-nor abstraction layer.

At the same time, let's add write_disable() after erasing, so we don't leave the flash in a write-enabled state afterward.

Signed-off-by Brian Norris <[email protected]>

write_enable() was being duplicated to both m25p80.c and fsl-quadspi.c. But this should be handled within the spi-nor abstraction layer.

At the same time, let's add write_disable() after erasing, so we don't leave the flash in a write-enabled state afterward.

Signed-off-by Brian Norris <[email protected]>

---
 drivers/mtd/devices/m25p80.c      | 5 -----
 drivers/mtd/spi-nor/fsl-quadspi.c | 5 -----
 drivers/mtd/spi-nor/spi-nor.c     | 7 ++++---
 3 files changed, 4 insertions(+), 13 deletions(-)

drivers/mtd/devices/m25p80.c +0 -5

index 96226ea69f90..116d979ffdb9 100644
@@ -158,11 +158,6 @@ static int m25p80_erase(struct spi_nor *nor, loff_t offset)
 	dev_dbg(nor->dev, "%dKiB at 0x%08x\n",
 		flash->mtd.erasesize / 1024, (u32)offset);
 
-	/* Send write enable, then erase commands. */
-	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 0);
-	if (ret)
-		return ret;
-
 	/* Set up command buffer. */
 	flash->command[0] = nor->erase_opcode;
 	m25p_addr2cmd(nor, offset, flash->command);

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

index 9c13622a0c7a..07fbfb0a7738 100644

Quoted 34 lines · Brian Norris

write_enable() was being duplicated to both m25p80.c and fsl-quadspi.c. But this should be handled within the spi-nor abstraction layer.

At the same time, let's add write_disable() after erasing, so we don't leave the flash in a write-enabled state afterward.

Signed-off-by Brian Norris <[email protected]>

---
 drivers/mtd/devices/m25p80.c      | 5 -----
 drivers/mtd/spi-nor/fsl-quadspi.c | 5 -----
 drivers/mtd/spi-nor/spi-nor.c     | 7 ++++---
 3 files changed, 4 insertions(+), 13 deletions(-)

drivers/mtd/devices/m25p80.c +0 -5

index 96226ea69f90..116d979ffdb9 100644
@@ -158,11 +158,6 @@ static int m25p80_erase(struct spi_nor *nor, loff_t offset)
 	dev_dbg(nor->dev, "%dKiB at 0x%08x\n",
 		flash->mtd.erasesize / 1024, (u32)offset);
 
-	/* Send write enable, then erase commands. */
-	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 0);
-	if (ret)
-		return ret;
-
 	/* Set up command buffer. */
 	flash->command[0] = nor->erase_opcode;
 	m25p_addr2cmd(nor, offset, flash->command);

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

index 9c13622a0c7a..07fbfb0a7738 100644

drivers/mtd/spi-nor/fsl-quadspi.c static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs) · line 745

Quoted 8 lines

drivers/mtd/spi-nor/fsl-quadspi.c +0 -5

@@ -738,11 +738,6 @@ static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)
 	dev_dbg(nor->dev, "%dKiB at 0x%08x:0x%08x\n",
 		nor->mtd->erasesize / 1024, q->chip_base_addr, (u32)offs);
 
-	/* Send write enable, then erase commands. */
-	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 0);
-	if (ret)
-		return ret;
-

This write-enable is used for per-sector-erase, not for the whole chip erase.

So if you really want to remove this code, you should add an extra write-enable in the spi_nor_erase.

Quoted 46 lines · Huang Shijie

Quoted 34 lines · Brian Norris

write_enable() was being duplicated to both m25p80.c and fsl-quadspi.c. But this should be handled within the spi-nor abstraction layer.

At the same time, let's add write_disable() after erasing, so we don't leave the flash in a write-enabled state afterward.

Signed-off-by Brian Norris <[email protected]>

---
 drivers/mtd/devices/m25p80.c      | 5 -----
 drivers/mtd/spi-nor/fsl-quadspi.c | 5 -----
 drivers/mtd/spi-nor/spi-nor.c     | 7 ++++---
 3 files changed, 4 insertions(+), 13 deletions(-)

drivers/mtd/devices/m25p80.c +0 -5

index 96226ea69f90..116d979ffdb9 100644
@@ -158,11 +158,6 @@ static int m25p80_erase(struct spi_nor *nor, loff_t offset)
 	dev_dbg(nor->dev, "%dKiB at 0x%08x\n",
 		flash->mtd.erasesize / 1024, (u32)offset);
 
-	/* Send write enable, then erase commands. */
-	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 0);
-	if (ret)
-		return ret;
-
 	/* Set up command buffer. */
 	flash->command[0] = nor->erase_opcode;
 	m25p_addr2cmd(nor, offset, flash->command);

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

index 9c13622a0c7a..07fbfb0a7738 100644

drivers/mtd/spi-nor/fsl-quadspi.c static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs) · line 745

Quoted 8 lines

drivers/mtd/spi-nor/fsl-quadspi.c +0 -5

@@ -738,11 +738,6 @@ static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)
 	dev_dbg(nor->dev, "%dKiB at 0x%08x:0x%08x\n",
 		nor->mtd->erasesize / 1024, q->chip_base_addr, (u32)offs);
 
-	/* Send write enable, then erase commands. */
-	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 0);
-	if (ret)
-		return ret;
-

This write-enable is used for per-sector-erase, not for the whole chip erase.

Quoted 2 lines

So if you really want to remove this code, you should add an extra write-enable in the spi_nor_erase.

I don't understand your comment.

Before this patch, there is a write-enable command in:
* m25p80.c, per-sector erase
* fsl-quadspi, per-sector erase
* spi-nor.c, whole-chip erase

With this patch, I am factoring all of these out into spi_nor_erase().

What is the problem?

Quoted 23 lines · Brian Norris

Quoted 17 lines · Huang Shijie

drivers/mtd/spi-nor/fsl-quadspi.c static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs) · line 745

Quoted 13 lines · Brian Norris

drivers/mtd/spi-nor/fsl-quadspi.c +0 -5

index 9c13622a0c7a..07fbfb0a7738 100644
@@ -738,11 +738,6 @@ static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)
 	dev_dbg(nor->dev, "%dKiB at 0x%08x:0x%08x\n",
 		nor->mtd->erasesize / 1024, q->chip_base_addr, (u32)offs);
 
-	/* Send write enable, then erase commands. */
-	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 0);
-	if (ret)
-		return ret;
-

This write-enable is used for per-sector-erase, not for the whole chip erase.

Quoted 2 lines

So if you really want to remove this code, you should add an extra write-enable in the spi_nor_erase.

I don't understand your comment.

Quoted 8 lines

Before this patch, there is a write-enable command in:
* m25p80.c, per-sector erase
* fsl-quadspi, per-sector erase
* spi-nor.c, whole-chip erase

With this patch, I am factoring all of these out into spi_nor_erase().

What is the problem?

Hi Brian:
For the spi_nor_erase(), the patch should like this:

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

index c130bf7..26c48bc 100644
@@ -324,6 +324,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 
 	/* whole-chip erase? */
 	if (len == mtd->size) {
+		write_enable(nor);
 		if (erase_chip(nor)) {
 			ret = -EIO;
 			goto erase_err;

Hi Huang,

Quoted 41 lines · Huang Shijie

Quoted 23 lines · Brian Norris

Quoted 17 lines · Huang Shijie

drivers/mtd/spi-nor/fsl-quadspi.c static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs) · line 745

Quoted 13 lines · Brian Norris

drivers/mtd/spi-nor/fsl-quadspi.c +0 -5

index 9c13622a0c7a..07fbfb0a7738 100644
@@ -738,11 +738,6 @@ static int fsl_qspi_erase(struct spi_nor *nor, loff_t offs)
 	dev_dbg(nor->dev, "%dKiB at 0x%08x:0x%08x\n",
 		nor->mtd->erasesize / 1024, q->chip_base_addr, (u32)offs);
 
-	/* Send write enable, then erase commands. */
-	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 0);
-	if (ret)
-		return ret;
-

This write-enable is used for per-sector-erase, not for the whole chip erase.

Quoted 2 lines

So if you really want to remove this code, you should add an extra write-enable in the spi_nor_erase.

I don't understand your comment.

Quoted 8 lines

Before this patch, there is a write-enable command in:
* m25p80.c, per-sector erase
* fsl-quadspi, per-sector erase
* spi-nor.c, whole-chip erase

With this patch, I am factoring all of these out into spi_nor_erase().

What is the problem?

Hi Brian:
For the spi_nor_erase(), the patch should like this:

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

index c130bf7..26c48bc 100644
@@ -324,6 +324,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) · line 341

Quoted 11 lines

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

 	/* whole-chip erase? */
 	if (len == mtd->size) {
+		write_enable(nor);
 		if (erase_chip(nor)) {
 			ret = -EIO;
 			goto erase_err;

@@ -337,6 +338,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	/* "sector"-at-a-time erase */
 	} else {
 		while (len) {
+			write_enable(nor);

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) · line 344

Quoted 4 lines

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

 			if (nor->erase(nor, addr)) {
 				ret = -EIO;
 				goto erase_err;

How is your patch any different than mine? Mine has the exact same code, except it covers both paths by putting the write_enable() outside the conditional entirely.

Quoted 9 lines · Brian Norris

Quoted 8 lines · Huang Shijie

For the spi_nor_erase(), the patch should like this:

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

index c130bf7..26c48bc 100644
@@ -324,6 +324,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 

Quoted 11 lines

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) · line 341

Quoted 11 lines

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

 	/* whole-chip erase? */
 	if (len == mtd->size) {
+		write_enable(nor);
 		if (erase_chip(nor)) {
 			ret = -EIO;
 			goto erase_err;

@@ -337,6 +338,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	/* "sector"-at-a-time erase */
 	} else {
 		while (len) {
+			write_enable(nor);

The difference is here.

you miss a write_enable for each sector's erase.

Quoted 8 lines

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) · line 344

Quoted 4 lines

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

 			if (nor->erase(nor, addr)) {
 				ret = -EIO;
 				goto erase_err;

How is your patch any different than mine? Mine has the exact same code, except it covers both paths by putting the write_enable() outside the conditional entirely.

Quoted 10 lines · Huang Shijie

Quoted 9 lines · Brian Norris

Quoted 8 lines · Huang Shijie

For the spi_nor_erase(), the patch should like this:

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

index c130bf7..26c48bc 100644
@@ -324,6 +324,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 

Quoted 12 lines

Quoted 11 lines

drivers/mtd/spi-nor/spi-nor.c static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) · line 341

Quoted 11 lines

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

 	/* whole-chip erase? */
 	if (len == mtd->size) {
+		write_enable(nor);
 		if (erase_chip(nor)) {
 			ret = -EIO;
 			goto erase_err;

@@ -337,6 +338,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	/* "sector"-at-a-time erase */
 	} else {
 		while (len) {
+			write_enable(nor);

The difference is here.

OK.

Quoted 1 line

you miss a write_enable for each sector's erase.

But is that necessary? I thought 'write-enabled' was retained across operations, so why would you have to perform it before each sector's erase?

Or do you have a flash datasheet which says you must send WREN before each sector erase?

I do see, now that I'm looking a little closer, that spi-nor doesn't actually have any concurrency protection (!). So it looks like we could potentially have other operations interleaved in this sequence of sector erasures, potentially running a write_disable() in the midst of this loop.

I really hope I'm wrong about that last paragraph.

If I'm correct though, the solution to this is not, AIUI, to add more write_enable() calls in this loop; the solution is to add some kind of concurrency protections, a la nand_get_device().

Quoted 8 lines · Brian Norris

Quoted 3 lines · Huang Shijie

Quoted 1 line · Brian Norris

On Tue, Aug 12, 2014 at 08:59:12AM +0800, Huang Shijie wrote:

you miss a write_enable for each sector's erase.

But is that necessary? I thought 'write-enabled' was retained across operations, so why would you have to perform it before each sector's erase?

The legacy code did so.

Quoted 3 lines

Or do you have a flash datasheet which says you must send WREN before each sector erase?

See the belowing from Spansion Nor S25fl129:

"
The Sector Erase (SE) command sets all bits at all addresses within a specified sector to a logic 1. A WREN
command is required prior to writing the SE command.
"

It does not tell we send a WREN for each sector erase, but i am not sure if we can remove it.

write_enable() was being duplicated to both m25p80.c and fsl-quadspi.c. But this should be handled within the spi-nor abstraction layer.

At the same time, let's add write_disable() after erasing, so we don't leave the flash in a write-enabled state afterward.

Signed-off-by Brian Norris <[email protected]>

write_enable() was being duplicated to both m25p80.c and fsl-quadspi.c. But this should be handled within the spi-nor abstraction layer.

At the same time, let's add write_disable() after erasing, so we don't leave the flash in a write-enabled state afterward.

Signed-off-by Brian Norris <[email protected]>

I hacked around the wait-till-ready sequencing just now, so let's add some debug checks for now.

These probably shouldn't be included in mainline (or at least they should be toned down to something less drastic; WARN_ON() perhaps?). I'm just using these for test purposes.

Signed-off-by Brian Norris <[email protected]>

Hi all,

This patch series tackles a few pieces of the spi-nor framework that weren't abstracted very well. I did some limited testing on m25p80.c, and you can see patch 8, which adds a few test BUG_ON()'s. If any of them fail for you, please holler. I don't plan to merge that patch, although I could be convinced to rework it if it adds value to someone...

I'm especially interested in getting test coverage on fsl-quadspi.

Thanks,
Brian