RE: [PATCH linux-next v2 01/14] mtd: spi-nor: remove micron_quad_enable()

[Bean Huo 霍斌斌 (beanhuo)]

> Message-ID:
> 	<6177f7317fe11922e1d1b6dc4548afbaf0ccebdd.1452268345.git.cyrille.
> pitchen@xxxxxxxxx>
> 
> Content-Type: text/plain
> 
> 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 <cyrille.pitchen@xxxxxxxxx>
> 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(-)
> 
> 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) {
> --
> 1.8.2.2
> 
> 
> 
> 

Hi, Cyrlle
Micron_quad_enable() function is just for how to enable Micron spi nor Quad mode,
does not force to use SPI nor quad mode. if does not need SPI NOR quad mode,
why spi-nor.c need set_quad_mode(), according to your patch, this function also need to
be removed?
Another question, if there is user want to enable SPI nor quad mode, how to do?