[v3,01/14] mtd: spi-nor: remove micron_quad_enable()

Commit Message

Cyrille Pitchen Feb. 3, 2016, 1:26 p.m. UTC

This patch remove the micron_quad_enable() function which force the Quad
SPI mode. However, once this mode is enabled, the Micron memory expect ALL
commands to use the SPI 4-4-4 protocol. Hence a failure does occur when
calling spi_nor_wait_till_ready() right after the update of the Enhanced
Volatile Configuration Register (EVCR) in the micron_quad_enable() as
the SPI controller driver is not aware about the protocol change.

Since there is almost no performance increase using Fast Read 4-4-4
commands instead of Fast Read 1-1-4 commands, we rather keep on using the
Extended SPI mode than enabling the Quad SPI mode.

Let's take the example of the pretty standard use of 8 dummy cycles during
Fast Read operations on 64KB erase sectors:

Fast Read 1-1-4 requires 8 cycles for the command, then 24 cycles for the
3byte address followed by 8 dummy clock cycles and finally 65536*2 cycles
for the read data; so 131112 clock cycles.

On the other hand the Fast Read 4-4-4 would require 2 cycles for the
command, then 6 cycles for the 3byte address followed by 8 dummy clock
cycles and finally 65536*2 cycles for the read data. So 131088 clock
cycles. The theorical bandwidth increase is 0.0%.

Now using Fast Read operations on 512byte pages:
Fast Read 1-1-4 needs 8+24+8+(512*2) = 1064 clock cycles whereas Fast
Read 4-4-4 would requires 2+6+8+(512*2) = 1040 clock cycles. Hence the
theorical bandwidth increase is 2.3%.
Consecutive reads for non sequential pages is not a relevant use case so
The Quad SPI mode is not worth it.

mtd_speedtest seems to confirm these figures.

Signed-off-by: Cyrille Pitchen <[e-mail]>
Fixes: 548cd3ab54da ("mtd: spi-nor: Add quad I/O support for Micron SPI NOR")
---
 drivers/mtd/spi-nor/spi-nor.c | 46 +------------------------------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

Comments

Brian Norris Feb. 12, 2016, 9 p.m. UTC | #1

On Wed, Feb 03, 2016 at 02:26:46PM +0100, Cyrille Pitchen wrote:
> This patch remove the micron_quad_enable() function which force the Quad
> SPI mode. However, once this mode is enabled, the Micron memory expect ALL
> commands to use the SPI 4-4-4 protocol. Hence a failure does occur when
> calling spi_nor_wait_till_ready() right after the update of the Enhanced
> Volatile Configuration Register (EVCR) in the micron_quad_enable() as
> the SPI controller driver is not aware about the protocol change.
> 
> Since there is almost no performance increase using Fast Read 4-4-4
> commands instead of Fast Read 1-1-4 commands, we rather keep on using the
> Extended SPI mode than enabling the Quad SPI mode.
> 
> Let's take the example of the pretty standard use of 8 dummy cycles during
> Fast Read operations on 64KB erase sectors:
> 
> Fast Read 1-1-4 requires 8 cycles for the command, then 24 cycles for the
> 3byte address followed by 8 dummy clock cycles and finally 65536*2 cycles
> for the read data; so 131112 clock cycles.
> 
> On the other hand the Fast Read 4-4-4 would require 2 cycles for the
> command, then 6 cycles for the 3byte address followed by 8 dummy clock
> cycles and finally 65536*2 cycles for the read data. So 131088 clock
> cycles. The theorical bandwidth increase is 0.0%.

The assumption of whole-block reads is not always realistic. There are
some controllers that might not handle that length properly, and there
are also use cases that don't need to read that much at once.

> Now using Fast Read operations on 512byte pages:
> Fast Read 1-1-4 needs 8+24+8+(512*2) = 1064 clock cycles whereas Fast
> Read 4-4-4 would requires 2+6+8+(512*2) = 1040 clock cycles. Hence the
> theorical bandwidth increase is 2.3%.

This computation is probably more of a reasonable comparison.

> Consecutive reads for non sequential pages is not a relevant use case so
> The Quad SPI mode is not worth it.

So, I still agree with the conclusion.

Applied this patch to l2-mtd.git.

Brian

> mtd_speedtest seems to confirm these figures.
> 
> Signed-off-by: Cyrille Pitchen <[e-mail]>
> Fixes: 548cd3ab54da ("mtd: spi-nor: Add quad I/O support for Micron SPI NOR")

Patch

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index ed0c19c558b5..3028c06547c1 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -1100,45 +1100,6 @@  static int spansion_quad_enable(struct spi_nor *nor)
 	return 0;
 }
 
-static int micron_quad_enable(struct spi_nor *nor)
-{
-	int ret;
-	u8 val;
-
-	ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1);
-	if (ret < 0) {
-		dev_err(nor->dev, "error %d reading EVCR\n", ret);
-		return ret;
-	}
-
-	write_enable(nor);
-
-	/* set EVCR, enable quad I/O */
-	nor->cmd_buf[0] = val & ~EVCR_QUAD_EN_MICRON;
-	ret = nor->write_reg(nor, SPINOR_OP_WD_EVCR, nor->cmd_buf, 1);
-	if (ret < 0) {
-		dev_err(nor->dev, "error while writing EVCR register\n");
-		return ret;
-	}
-
-	ret = spi_nor_wait_till_ready(nor);
-	if (ret)
-		return ret;
-
-	/* read EVCR and check it */
-	ret = nor->read_reg(nor, SPINOR_OP_RD_EVCR, &val, 1);
-	if (ret < 0) {
-		dev_err(nor->dev, "error %d reading EVCR\n", ret);
-		return ret;
-	}
-	if (val & EVCR_QUAD_EN_MICRON) {
-		dev_err(nor->dev, "Micron EVCR Quad bit not clear\n");
-		return -EINVAL;
-	}
-
-	return 0;
-}
-
 static int set_quad_mode(struct spi_nor *nor, const struct flash_info *info)
 {
 	int status;
@@ -1152,12 +1113,7 @@  static int set_quad_mode(struct spi_nor *nor, const struct flash_info *info)
 		}
 		return status;
 	case SNOR_MFR_MICRON:
-		status = micron_quad_enable(nor);
-		if (status) {
-			dev_err(nor->dev, "Micron quad-read not enabled\n");
-			return -EINVAL;
-		}
-		return status;
+		return 0;
 	default:
 		status = spansion_quad_enable(nor);
 		if (status) {