[PATCH 0/8] mtd: spi-nor: refactor wait-till-ready

[PATCH 0/8] mtd: spi-nor: refactor wait-till-ready

[Brian Norris]

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

Brian Norris (8):
  mtd: spi-nor: eliminate duplicate spi_nor_wait_till_{,fsr}_ready()
    code
  mtd: spi-nor: handle timeout errors in spi_nor_write()
  mtd: spi-nor: move "wait-till-ready" checks into erase/write functions
  mtd: m25p80: drop wait-till-ready checks
  mtd: fsl-quadspi: drop wait-till-ready checks
  mtd: spi-nor: drop replaceable wait-till-ready function pointer
  mtd: spi-nor: factor out write_enable() for erase commands
  debug: mtd: spi-nor: add BUG_ON() prints to check for !ready

 drivers/mtd/devices/m25p80.c      |  17 -----
 drivers/mtd/spi-nor/fsl-quadspi.c |  16 -----
 drivers/mtd/spi-nor/spi-nor.c     | 145 ++++++++++++++++++--------------------
 include/linux/mtd/spi-nor.h       |   8 ++-
 4 files changed, 76 insertions(+), 110 deletions(-)

-- 
1.9.1

[PATCH 1/8] mtd: spi-nor: eliminate duplicate spi_nor_wait_till_{, fsr}_ready() code

[Brian Norris]

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 <computersforpeace@gmail.com>
Cc: Graham Moore <grmoore@altera.com>
Cc: Huang Shijie <b32955@freescale.com>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 66 ++++++++++++++++++++++---------------------
 include/linux/mtd/spi-nor.h   |  6 ++++
 2 files changed, 40 insertions(+), 32 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index b5ad6bebf5e7..5825b8a12cee 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -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;
+}
 
-	return -ETIMEDOUT;
+static int spi_nor_ready(struct spi_nor *nor)
+{
+	int sr, fsr;
+	sr = spi_nor_sr_ready(nor);
+	if (sr < 0)
+		return sr;
+	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
+	if (fsr < 0)
+		return sr;
+	return sr && fsr;
 }
 
-static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
+static int spi_nor_wait_till_ready(struct spi_nor *nor)
 {
 	unsigned long deadline;
-	int sr;
-	int fsr;
+	int ret;
 
 	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;
-		}
+		ret = spi_nor_ready(nor);
+		if (ret < 0)
+			return ret;
+		if (ret)
+			return 0;
 	} while (!time_after_eq(jiffies, deadline));
 
 	return -ETIMEDOUT;
@@ -1014,9 +1017,8 @@ 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_ready)
-		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
+	if (info->flags & USE_FSR)
+		nor->flags |= SNOR_F_USE_FSR;
 
 	/* prefer "small sector" erase if possible */
 	if (info->flags & SECT_4K) {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 9e6294f32ba8..603ac0306663 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -116,6 +116,10 @@ enum spi_nor_ops {
 	SPI_NOR_OPS_UNLOCK,
 };
 
+enum spi_nor_option_flags {
+	SNOR_F_USE_FSR		= BIT(0),
+};
+
 /**
  * struct spi_nor - Structure for defining a the SPI NOR layer
  * @mtd:		point to a mtd_info structure
@@ -129,6 +133,7 @@ enum spi_nor_ops {
  * @program_opcode:	the program opcode
  * @flash_read:		the mode of the read
  * @sst_write_second:	used by the SST write operation
+ * @flags:		flag options for the current SPI-NOR (SNOR_F_*)
  * @cfg:		used by the read_xfer/write_xfer
  * @cmd_buf:		used by the write_reg
  * @prepare:		[OPTIONAL] do some preparations for the
@@ -160,6 +165,7 @@ struct spi_nor {
 	u8			program_opcode;
 	enum read_mode		flash_read;
 	bool			sst_write_second;
+	u32			flags;
 	struct spi_nor_xfer_cfg	cfg;
 	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
 
-- 
1.9.1

Re: [PATCH 1/8] mtd: spi-nor: eliminate duplicate spi_nor_wait_till_{, fsr}_ready() code

[Marek Vasut]

On Thursday, August 07, 2014 at 03:16:55 AM, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>
> Cc: Graham Moore <grmoore@altera.com>
> Cc: Huang Shijie <b32955@freescale.com>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Thanks for the cleanup,

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 1/8] mtd: spi-nor: eliminate duplicate spi_nor_wait_till_{, fsr}_ready() code

[Huang Shijie]

On Wed, Aug 06, 2014 at 06:16:55PM -0700, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>
> Cc: Graham Moore <grmoore@altera.com>
> Cc: Huang Shijie <b32955@freescale.com>
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 66 ++++++++++++++++++++++---------------------
>  include/linux/mtd/spi-nor.h   |  6 ++++
>  2 files changed, 40 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index b5ad6bebf5e7..5825b8a12cee 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -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;
> +}
>  
> -	return -ETIMEDOUT;
> +static int spi_nor_ready(struct spi_nor *nor)
> +{
> +	int sr, fsr;
> +	sr = spi_nor_sr_ready(nor);
> +	if (sr < 0)
> +		return sr;
> +	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
> +	if (fsr < 0)
> +		return sr;
return "fsr" here.

Beside this, i am ok with this patch.	
	
thanks
Huang Shijie

Re: [PATCH 1/8] mtd: spi-nor: eliminate duplicate spi_nor_wait_till_{, fsr}_ready() code

[Brian Norris]

On Sat, Aug 09, 2014 at 02:25:45PM +0800, Huang Shijie wrote:
> On Wed, Aug 06, 2014 at 06:16:55PM -0700, Brian Norris wrote:
> > +	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
> > +	if (fsr < 0)
> > +		return sr;
> return "fsr" here.

Will fix, thanks.

Brian

[PATCH v2 1/10] mtd: spi-nor: eliminate duplicate spi_nor_wait_till_{, fsr}_ready() code

[Brian Norris]

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 <computersforpeace@gmail.com>
Reviewed-by: Marek Vasut <marex@denx.de>
Cc: Graham Moore <grmoore@altera.com>
Cc: Huang Shijie <shijie8@gmail.com>
Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
v2: replace 'sr' with 'fsr' (typo)

I think this was the only patch (of the first 7) that required changes due to
review. I may rework patch 8 eventually, but it was not meant for inclusion in
its current form.

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

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 03e0ab8b2086..ba324e293645 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -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;
+}
 
-	return -ETIMEDOUT;
+static int spi_nor_ready(struct spi_nor *nor)
+{
+	int sr, fsr;
+	sr = spi_nor_sr_ready(nor);
+	if (sr < 0)
+		return sr;
+	fsr = nor->flags & SNOR_F_USE_FSR ? spi_nor_fsr_ready(nor) : 1;
+	if (fsr < 0)
+		return fsr;
+	return sr && fsr;
 }
 
-static int spi_nor_wait_till_fsr_ready(struct spi_nor *nor)
+static int spi_nor_wait_till_ready(struct spi_nor *nor)
 {
 	unsigned long deadline;
-	int sr;
-	int fsr;
+	int ret;
 
 	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;
-		}
+		ret = spi_nor_ready(nor);
+		if (ret < 0)
+			return ret;
+		if (ret)
+			return 0;
 	} while (!time_after_eq(jiffies, deadline));
 
 	return -ETIMEDOUT;
@@ -1009,9 +1012,8 @@ 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_ready)
-		nor->wait_till_ready = spi_nor_wait_till_fsr_ready;
+	if (info->flags & USE_FSR)
+		nor->flags |= SNOR_F_USE_FSR;
 
 	/* prefer "small sector" erase if possible */
 	if (info->flags & SECT_4K) {
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 9e6294f32ba8..603ac0306663 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -116,6 +116,10 @@ enum spi_nor_ops {
 	SPI_NOR_OPS_UNLOCK,
 };
 
+enum spi_nor_option_flags {
+	SNOR_F_USE_FSR		= BIT(0),
+};
+
 /**
  * struct spi_nor - Structure for defining a the SPI NOR layer
  * @mtd:		point to a mtd_info structure
@@ -129,6 +133,7 @@ enum spi_nor_ops {
  * @program_opcode:	the program opcode
  * @flash_read:		the mode of the read
  * @sst_write_second:	used by the SST write operation
+ * @flags:		flag options for the current SPI-NOR (SNOR_F_*)
  * @cfg:		used by the read_xfer/write_xfer
  * @cmd_buf:		used by the write_reg
  * @prepare:		[OPTIONAL] do some preparations for the
@@ -160,6 +165,7 @@ struct spi_nor {
 	u8			program_opcode;
 	enum read_mode		flash_read;
 	bool			sst_write_second;
+	u32			flags;
 	struct spi_nor_xfer_cfg	cfg;
 	u8			cmd_buf[SPI_NOR_MAX_CMD_SIZE];
 
-- 
1.9.1

[PATCH 2/8] mtd: spi-nor: handle timeout errors in spi_nor_write()

[Brian Norris]

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 <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 5825b8a12cee..d77c93232b76 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -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)
-- 
1.9.1

Re: [PATCH 2/8] mtd: spi-nor: handle timeout errors in spi_nor_write()

[Marek Vasut]

On Thursday, August 07, 2014 at 03:16:56 AM, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 2/8] mtd: spi-nor: handle timeout errors in spi_nor_write()

[Huang Shijie]

On Wed, Aug 06, 2014 at 06:16:56PM -0700, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index 5825b8a12cee..d77c93232b76 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -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)
> -- 
> 1.9.1
Acked-by: Huang Shijie <shijie8@gmail.com>

Huang Shijie

[PATCH 3/8] mtd: spi-nor: move "wait-till-ready" checks into erase/write functions

[Brian Norris]

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 <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 36 +++++++++---------------------------
 1 file changed, 9 insertions(+), 27 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d77c93232b76..227e2743203e 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -229,15 +229,8 @@ static int wait_till_ready(struct spi_nor *nor)
  */
 static int erase_chip(struct spi_nor *nor)
 {
-	int ret;
-
 	dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd->size >> 10));
 
-	/* Wait until finished previous write command. */
-	ret = wait_till_ready(nor);
-	if (ret)
-		return ret;
-
 	/* Send write enable, then erase commands. */
 	write_enable(nor);
 
@@ -300,6 +293,10 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 			goto erase_err;
 		}
 
+		ret = spi_nor_wait_till_ready(nor);
+		if (ret)
+			goto erase_err;
+
 	/* REVISIT in some cases we could speed up erasing large regions
 	 * by using SPINOR_OP_SE instead of SPINOR_OP_BE_4K.  We may have set up
 	 * to use "small sector erase", but that's not always optimal.
@@ -315,6 +312,10 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 
 			addr += mtd->erasesize;
 			len -= mtd->erasesize;
+
+			ret = spi_nor_wait_till_ready(nor);
+			if (ret)
+				goto erase_err;
 		}
 	}
 
@@ -342,11 +343,6 @@ static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	if (ret)
 		return ret;
 
-	/* Wait until finished previous command */
-	ret = wait_till_ready(nor);
-	if (ret)
-		goto err;
-
 	status_old = read_sr(nor);
 
 	if (offset < mtd->size - (mtd->size / 2))
@@ -389,11 +385,6 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	if (ret)
 		return ret;
 
-	/* Wait until finished previous command */
-	ret = wait_till_ready(nor);
-	if (ret)
-		goto err;
-
 	status_old = read_sr(nor);
 
 	if (offset+len > mtd->size - (mtd->size / 64))
@@ -710,11 +701,6 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (ret)
 		return ret;
 
-	/* Wait until finished previous write command. */
-	ret = wait_till_ready(nor);
-	if (ret)
-		goto time_out;
-
 	write_enable(nor);
 
 	nor->sst_write_second = false;
@@ -786,11 +772,6 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (ret)
 		return ret;
 
-	/* Wait until finished previous write command. */
-	ret = wait_till_ready(nor);
-	if (ret)
-		goto write_err;
-
 	write_enable(nor);
 
 	page_offset = to & (nor->page_size - 1);
@@ -819,6 +800,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 		}
 	}
 
+	ret = spi_nor_wait_till_ready(nor);
 write_err:
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_WRITE);
 	return ret;
-- 
1.9.1

Re: [PATCH 3/8] mtd: spi-nor: move "wait-till-ready" checks into erase/write functions

[Marek Vasut]

On Thursday, August 07, 2014 at 03:16:57 AM, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>

Nice!

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 3/8] mtd: spi-nor: move "wait-till-ready" checks into erase/write functions

[Huang Shijie]

On Wed, Aug 06, 2014 at 06:16:57PM -0700, Brian Norris wrote:
> 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.

thanks
Huang Shijie

Re: [PATCH 3/8] mtd: spi-nor: move "wait-till-ready" checks into erase/write functions

[Brian Norris]

On Sat, Aug 09, 2014 at 04:42:24PM +0800, Huang Shijie wrote:
> On Wed, Aug 06, 2014 at 06:16:57PM -0700, Brian Norris wrote:
> > 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?

> 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.

Thanks,
Brian

Re: [PATCH 3/8] mtd: spi-nor: move "wait-till-ready" checks into erase/write functions

[Huang Shijie]

On Mon, Aug 11, 2014 at 11:23:04AM -0700, Brian Norris wrote:
> On Sat, Aug 09, 2014 at 04:42:24PM +0800, Huang Shijie wrote:
> > On Wed, Aug 06, 2014 at 06:16:57PM -0700, Brian Norris wrote:
> > > 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?

i guess only the read function should need the suspend.
I does not dig this issue too.
> 
> > 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?

Please refer to cfi_cmdset_0002.c. The parallel NOR has implemented this
feature. 
> 
> 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
The file systems should not know the suspend feature.

It is transparent to them. The suspend feature will make the file system
run much faster. sorry, i have forgotten how did i test this feature, it
was long time ago.


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

they do not suspend the erase, the SPI-NOR framework should do the job.

> 
> 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.
I do not have time to implement this feature recently. Please go
forward with your patch. I guess the one who want to add this feature
will change the code himself.


thanks
Huang Shijie

[PATCH 4/8] mtd: m25p80: drop wait-till-ready checks

[Brian Norris]

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

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index ed7e0a1bed3c..96226ea69f90 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -129,12 +129,6 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	struct spi_transfer t[2];
 	struct spi_message m;
 	int dummy = nor->read_dummy;
-	int ret;
-
-	/* Wait till previous write/erase is done. */
-	ret = nor->wait_till_ready(nor);
-	if (ret)
-		return ret;
 
 	spi_message_init(&m);
 	memset(t, 0, (sizeof t));
@@ -160,16 +154,10 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 static int m25p80_erase(struct spi_nor *nor, loff_t offset)
 {
 	struct m25p *flash = nor->priv;
-	int ret;
 
 	dev_dbg(nor->dev, "%dKiB at 0x%08x\n",
 		flash->mtd.erasesize / 1024, (u32)offset);
 
-	/* Wait until finished previous write command. */
-	ret = nor->wait_till_ready(nor);
-	if (ret)
-		return ret;
-
 	/* Send write enable, then erase commands. */
 	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 0);
 	if (ret)
-- 
1.9.1

Re: [PATCH 4/8] mtd: m25p80: drop wait-till-ready checks

[Marek Vasut]

On Thursday, August 07, 2014 at 03:16:58 AM, Brian Norris wrote:
> spi-nor.c should be taking care of these now.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[PATCH 5/8] mtd: fsl-quadspi: drop wait-till-ready checks

[Brian Norris]

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

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
Cc: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/spi-nor/fsl-quadspi.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 8d659a2888d5..9c13622a0c7a 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -719,16 +719,10 @@ static int fsl_qspi_read(struct spi_nor *nor, loff_t from,
 {
 	struct fsl_qspi *q = nor->priv;
 	u8 cmd = nor->read_opcode;
-	int ret;
 
 	dev_dbg(q->dev, "cmd [%x],read from (0x%p, 0x%.8x, 0x%.8x),len:%d\n",
 		cmd, q->ahb_base, q->chip_base_addr, (unsigned int)from, len);
 
-	/* Wait until the previous command is finished. */
-	ret = nor->wait_till_ready(nor);
-	if (ret)
-		return ret;
-
 	/* Read out the data directly from the AHB buffer.*/
 	memcpy(buf, q->ahb_base + q->chip_base_addr + from, len);
 
@@ -744,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);
 
-	/* Wait until finished previous write command. */
-	ret = nor->wait_till_ready(nor);
-	if (ret)
-		return ret;
-
 	/* Send write enable, then erase commands. */
 	ret = nor->write_reg(nor, SPINOR_OP_WREN, NULL, 0, 0);
 	if (ret)
-- 
1.9.1

Re: [PATCH 5/8] mtd: fsl-quadspi: drop wait-till-ready checks

[Marek Vasut]

On Thursday, August 07, 2014 at 03:16:59 AM, Brian Norris wrote:
> spi-nor.c should be taking care of these now.
> 
> Signed-off-by: Brian Norris <computersforpeace@gmail.com>
> Cc: Huang Shijie <b32955@freescale.com>

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[PATCH 6/8] mtd: spi-nor: drop replaceable wait-till-ready function pointer

[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.

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
 drivers/mtd/spi-nor/spi-nor.c | 27 ++++++++++-----------------
 include/linux/mtd/spi-nor.h   |  2 --
 2 files changed, 10 insertions(+), 19 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 227e2743203e..874e6d9a0b02 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -193,6 +193,10 @@ static int spi_nor_ready(struct spi_nor *nor)
 	return sr && fsr;
 }
 
+/*
+ * Service routine to read status register until ready, or timeout occurs.
+ * Returns non-zero if error.
+ */
 static int spi_nor_wait_till_ready(struct spi_nor *nor)
 {
 	unsigned long deadline;
@@ -214,15 +218,6 @@ static int spi_nor_wait_till_ready(struct spi_nor *nor)
 }
 
 /*
- * Service routine to read status register until ready, or timeout occurs.
- * Returns non-zero if error.
- */
-static int wait_till_ready(struct spi_nor *nor)
-{
-	return nor->wait_till_ready(nor);
-}
-
-/*
  * Erase the whole flash memory
  *
  * Returns 0 if successful, non-zero otherwise.
@@ -712,7 +707,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 		/* write one byte. */
 		nor->write(nor, to, 1, retlen, buf);
-		ret = wait_till_ready(nor);
+		ret = spi_nor_wait_till_ready(nor);
 		if (ret)
 			goto time_out;
 	}
@@ -724,7 +719,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 
 		/* write two bytes. */
 		nor->write(nor, to, 2, retlen, buf + actual);
-		ret = wait_till_ready(nor);
+		ret = spi_nor_wait_till_ready(nor);
 		if (ret)
 			goto time_out;
 		to += 2;
@@ -733,7 +728,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 	nor->sst_write_second = false;
 
 	write_disable(nor);
-	ret = wait_till_ready(nor);
+	ret = spi_nor_wait_till_ready(nor);
 	if (ret)
 		goto time_out;
 
@@ -744,7 +739,7 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 		nor->program_opcode = SPINOR_OP_BP;
 		nor->write(nor, to, 1, retlen, buf + actual);
 
-		ret = wait_till_ready(nor);
+		ret = spi_nor_wait_till_ready(nor);
 		if (ret)
 			goto time_out;
 		write_disable(nor);
@@ -790,7 +785,7 @@ 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;
 
-			ret = wait_till_ready(nor);
+			ret = spi_nor_wait_till_ready(nor);
 			if (ret)
 				goto write_err;
 
@@ -816,7 +811,7 @@ static int macronix_quad_enable(struct spi_nor *nor)
 	nor->cmd_buf[0] = val | SR_QUAD_EN_MX;
 	nor->write_reg(nor, SPINOR_OP_WRSR, nor->cmd_buf, 1, 0);
 
-	if (wait_till_ready(nor))
+	if (spi_nor_wait_till_ready(nor))
 		return 1;
 
 	ret = read_sr(nor);
@@ -898,8 +893,6 @@ static int spi_nor_check(struct spi_nor *nor)
 
 	if (!nor->read_id)
 		nor->read_id = spi_nor_read_id;
-	if (!nor->wait_till_ready)
-		nor->wait_till_ready = spi_nor_wait_till_ready;
 
 	return 0;
 }
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 603ac0306663..d7c12506d7ed 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -146,7 +146,6 @@ enum spi_nor_option_flags {
  * @write_reg:		[DRIVER-SPECIFIC] write data to the register
  * @read_id:		[REPLACEABLE] read out the ID data, and find
  *			the proper spi_device_id
- * @wait_till_ready:	[REPLACEABLE] wait till the NOR becomes ready
  * @read:		[DRIVER-SPECIFIC] read data from the SPI NOR
  * @write:		[DRIVER-SPECIFIC] write data to the SPI NOR
  * @erase:		[DRIVER-SPECIFIC] erase a sector of the SPI NOR
@@ -179,7 +178,6 @@ struct spi_nor {
 	int (*write_reg)(struct spi_nor *nor, u8 opcode, u8 *buf, int len,
 			int write_enable);
 	const struct spi_device_id *(*read_id)(struct spi_nor *nor);
-	int (*wait_till_ready)(struct spi_nor *nor);
 
 	int (*read)(struct spi_nor *nor, loff_t from,
 			size_t len, size_t *retlen, u_char *read_buf);
-- 
1.9.1

Re: [PATCH 6/8] mtd: spi-nor: drop replaceable wait-till-ready function pointer

[Marek Vasut]

On Thursday, August 07, 2014 at 03:17:00 AM, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>

Very nice,

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 6/8] mtd: spi-nor: drop replaceable wait-till-ready function pointer

[Huang Shijie]

On Wed, Aug 06, 2014 at 06:17:00PM -0700, Brian Norris wrote:
> 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.

thanks
Huang Shijie

Re: [PATCH 6/8] mtd: spi-nor: drop replaceable wait-till-ready function pointer

[Brian Norris]

On Sat, Aug 09, 2014 at 05:53:03PM +0800, Huang Shijie wrote:
> On Wed, Aug 06, 2014 at 06:17:00PM -0700, Brian Norris wrote:
> > 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.

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.

Brian

Re: [PATCH 6/8] mtd: spi-nor: drop replaceable wait-till-ready function pointer

[Huang Shijie]

On Mon, Aug 11, 2014 at 11:43:11AM -0700, Brian Norris wrote:
> On Sat, Aug 09, 2014 at 05:53:03PM +0800, Huang Shijie wrote:
> > On Wed, Aug 06, 2014 at 06:17:00PM -0700, Brian Norris wrote:
> > > 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.
> 
> 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

but where to put the optional callback? in the spi_nor{} ?
> 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.

thanks
Huang Shijie

Re: [PATCH 6/8] mtd: spi-nor: drop replaceable wait-till-ready function pointer

[Brian Norris]

On Tue, Aug 12, 2014 at 09:16:07AM +0800, Huang Shijie wrote:
> On Mon, Aug 11, 2014 at 11:43:11AM -0700, Brian Norris wrote:
> > On Sat, Aug 09, 2014 at 05:53:03PM +0800, Huang Shijie wrote:
> > > On Wed, Aug 06, 2014 at 06:17:00PM -0700, Brian Norris wrote:
> > > > 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.
> > 
> > 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
> 
> but where to put the optional callback? in the spi_nor{} ?

Probably.

> > 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.

Yeah. The point of this series is that without users, the current
callback is both ambiguously defined (it was allegedly serving both a
flash-specific and a controller-specific purpose) and unncecessary. It's
easier to add back once we have a clear purpose.

Brian

Re: [PATCH 6/8] mtd: spi-nor: drop replaceable wait-till-ready function pointer

[Rafał Miłecki]

On 7 August 2014 03:17, Brian Norris <computersforpeace@gmail.com> wrote:
> 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).

Re: [PATCH 6/8] mtd: spi-nor: drop replaceable wait-till-ready function pointer

[Rafał Miłecki]

On 12 August 2014 07:13, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 7 August 2014 03:17, Brian Norris <computersforpeace@gmail.com> wrote:
>> 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).

I meant standard code using command 0x05 and checking for 0x01 to be unset.

-- 
Rafał

[PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[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 <computersforpeace@gmail.com>
---
 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(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 96226ea69f90..116d979ffdb9 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -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);
diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 9c13622a0c7a..07fbfb0a7738 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -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;
-
 	ret = fsl_qspi_runcmd(q, nor->erase_opcode, offs, 0);
 	if (ret)
 		return ret;
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 874e6d9a0b02..d08d9f8bb9bd 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -226,9 +226,6 @@ static int erase_chip(struct spi_nor *nor)
 {
 	dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd->size >> 10));
 
-	/* Send write enable, then erase commands. */
-	write_enable(nor);
-
 	return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0, 0);
 }
 
@@ -281,6 +278,8 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	if (ret)
 		return ret;
 
+	write_enable(nor);
+
 	/* whole-chip erase? */
 	if (len == mtd->size) {
 		if (erase_chip(nor)) {
@@ -314,6 +313,8 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 		}
 	}
 
+	write_disable(nor);
+
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_ERASE);
 
 	instr->state = MTD_ERASE_DONE;
-- 
1.9.1

Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Marek Vasut]

On Thursday, August 07, 2014 at 03:17:01 AM, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>

Makes sense, thanks!

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Huang Shijie]

On Wed, Aug 06, 2014 at 06:17:01PM -0700, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>
> ---
>  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(-)
> 
> diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> index 96226ea69f90..116d979ffdb9 100644
> --- a/drivers/mtd/devices/m25p80.c
> +++ b/drivers/mtd/devices/m25p80.c
> @@ -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);
> diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> index 9c13622a0c7a..07fbfb0a7738 100644
> --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> @@ -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.

thanks
Huang Shijie

Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Brian Norris]

On Sat, Aug 09, 2014 at 06:52:34PM +0800, Huang Shijie wrote:
> On Wed, Aug 06, 2014 at 06:17:01PM -0700, Brian Norris wrote:
> > 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 <computersforpeace@gmail.com>
> > ---
> >  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(-)
> > 
> > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > index 96226ea69f90..116d979ffdb9 100644
> > --- a/drivers/mtd/devices/m25p80.c
> > +++ b/drivers/mtd/devices/m25p80.c
> > @@ -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);
> > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> > index 9c13622a0c7a..07fbfb0a7738 100644
> > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > @@ -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.

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?

Brian

Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Huang Shijie]

On Mon, Aug 11, 2014 at 11:48:18AM -0700, Brian Norris wrote:
> On Sat, Aug 09, 2014 at 06:52:34PM +0800, Huang Shijie wrote:
> > On Wed, Aug 06, 2014 at 06:17:01PM -0700, Brian Norris wrote:
> > > 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 <computersforpeace@gmail.com>
> > > ---
> > >  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(-)
> > > 
> > > diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
> > > index 96226ea69f90..116d979ffdb9 100644
> > > --- a/drivers/mtd/devices/m25p80.c
> > > +++ b/drivers/mtd/devices/m25p80.c
> > > @@ -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);
> > > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> > > index 9c13622a0c7a..07fbfb0a7738 100644
> > > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > > @@ -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.
> 
> 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?
Hi Brian:
   For the spi_nor_erase(), the patch should like this:

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index c130bf7..26c48bc 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -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;
@@ -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);
 			if (nor->erase(nor, addr)) {
 				ret = -EIO;
 				goto erase_err;


thanks
Huang Shijie	

Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Brian Norris]

Hi Huang,

On Tue, Aug 12, 2014 at 08:59:12AM +0800, Huang Shijie wrote:
> On Mon, Aug 11, 2014 at 11:48:18AM -0700, Brian Norris wrote:
> > On Sat, Aug 09, 2014 at 06:52:34PM +0800, Huang Shijie wrote:
> > > On Wed, Aug 06, 2014 at 06:17:01PM -0700, Brian Norris wrote:
> > > > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> > > > index 9c13622a0c7a..07fbfb0a7738 100644
> > > > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > > > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > > > @@ -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.
> > 
> > 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?
> Hi Brian:
>    For the spi_nor_erase(), the patch should like this:
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index c130bf7..26c48bc 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -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;
> @@ -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);
>  			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.

Brian

Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Huang Shijie]

On Wed, Sep 10, 2014 at 12:05:37AM -0700, Brian Norris wrote:
> Hi Huang,
> 
> On Tue, Aug 12, 2014 at 08:59:12AM +0800, Huang Shijie wrote:
> > On Mon, Aug 11, 2014 at 11:48:18AM -0700, Brian Norris wrote:
> > > On Sat, Aug 09, 2014 at 06:52:34PM +0800, Huang Shijie wrote:
> > > > On Wed, Aug 06, 2014 at 06:17:01PM -0700, Brian Norris wrote:
> > > > > diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
> > > > > index 9c13622a0c7a..07fbfb0a7738 100644
> > > > > --- a/drivers/mtd/spi-nor/fsl-quadspi.c
> > > > > +++ b/drivers/mtd/spi-nor/fsl-quadspi.c
> > > > > @@ -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.
> > > 
> > > 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?
> > Hi Brian:
> >    For the spi_nor_erase(), the patch should like this:
> > 
> > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > index c130bf7..26c48bc 100644
> > --- a/drivers/mtd/spi-nor/spi-nor.c
> > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > @@ -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;
> > @@ -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. 

thanks
Huang Shijie
> >  			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.

Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Brian Norris]

On Wed, Sep 10, 2014 at 11:20:21PM +0800, Huang Shijie wrote:
> On Wed, Sep 10, 2014 at 12:05:37AM -0700, Brian Norris wrote:
> > On Tue, Aug 12, 2014 at 08:59:12AM +0800, Huang Shijie wrote:
> > >    For the spi_nor_erase(), the patch should like this:
> > > 
> > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > > index c130bf7..26c48bc 100644
> > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > @@ -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;
> > > @@ -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.

> 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().

> > >  			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.

Brian

Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Huang Shijie]

On Wed, Sep 10, 2014 at 12:47:08AM -0700, Brian Norris wrote:
> On Wed, Sep 10, 2014 at 11:20:21PM +0800, Huang Shijie wrote:
> > On Wed, Sep 10, 2014 at 12:05:37AM -0700, Brian Norris wrote:
> > > On Tue, Aug 12, 2014 at 08:59:12AM +0800, Huang Shijie wrote:
> > > >    For the spi_nor_erase(), the patch should like this:
> > > > 
> > > > diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> > > > index c130bf7..26c48bc 100644
> > > > --- a/drivers/mtd/spi-nor/spi-nor.c
> > > > +++ b/drivers/mtd/spi-nor/spi-nor.c
> > > > @@ -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;
> > > > @@ -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.
> 
> > 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.

> 
> 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.

thanks
Huang Shijie

> 
> 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().

Re: [PATCH 7/8] mtd: spi-nor: factor out write_enable() for erase commands

[Brian Norris]

On Thu, Sep 11, 2014 at 12:12:37AM +0800, Huang Shijie wrote:
> On Wed, Sep 10, 2014 at 12:47:08AM -0700, Brian Norris wrote:
> > On Wed, Sep 10, 2014 at 11:20:21PM +0800, Huang Shijie wrote:
> > > On Wed, Sep 10, 2014 at 12:05:37AM -0700, Brian Norris wrote:
> > > > 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.
> 
> > 
> > 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.

OK, well I guess I can rework this to retain the original behavior. That
was in fact, an oversight (thanks for catching), although I'm not
convinced it's actually necessary.

(Edit: I think you may be right that WREN is necessary before every
erased command. From a Micron N25Q256 datasheet:

  The write enable latch bit must be set before every PROGRAM, ERASE,
  WRITE, ENTER 4-BYTE ADDRESS MODE, and EXIT 4-BYTE ADDRESS MODE
  command.
  
But I don't recall seeing any ill effects last time I tested this on my
Micron SPI flash, so I'm still mildly confused.)

Brian

[PATCH v2] mtd: spi-nor: factor out write_enable() for erase commands

[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 <computersforpeace@gmail.com>
---
 drivers/mtd/devices/m25p80.c      | 6 ------
 drivers/mtd/spi-nor/fsl-quadspi.c | 5 -----
 drivers/mtd/spi-nor/spi-nor.c     | 9 ++++++---
 3 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 35e7e9896b17..3bd12bbb94eb 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -157,16 +157,10 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 static int m25p80_erase(struct spi_nor *nor, loff_t offset)
 {
 	struct m25p *flash = nor->priv;
-	int ret;
 
 	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);
diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 03dcffac8185..7f2ba8d946b5 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -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;
-
 	ret = fsl_qspi_runcmd(q, nor->erase_opcode, offs, 0);
 	if (ret)
 		return ret;
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index bfb67f1e2b7d..5e3a1d363895 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -228,9 +228,6 @@ static int erase_chip(struct spi_nor *nor)
 {
 	dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd->size >> 10));
 
-	/* Send write enable, then erase commands. */
-	write_enable(nor);
-
 	return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0, 0);
 }
 
@@ -285,6 +282,8 @@ 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;
@@ -302,6 +301,8 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	/* "sector"-at-a-time erase */
 	} else {
 		while (len) {
+			write_enable(nor);
+
 			if (nor->erase(nor, addr)) {
 				ret = -EIO;
 				goto erase_err;
@@ -316,6 +317,8 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 		}
 	}
 
+	write_disable(nor);
+
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_ERASE);
 
 	instr->state = MTD_ERASE_DONE;
-- 
1.9.1

Re: [PATCH v2] mtd: spi-nor: factor out write_enable() for erase commands

[Huang Shijie]

On Wed, Nov 05, 2014 at 02:29:03AM -0800, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>
thanks

Acked-by: Huang Shijie <shijie.huang@intel.com>

Re: [PATCH v2] mtd: spi-nor: factor out write_enable() for erase commands

[Brian Norris]

On Wed, Nov 05, 2014 at 02:29:03AM -0800, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>

Applied to l2-mtd.git

[RFC 8/8] debug: mtd: spi-nor: add BUG_ON() prints to check for !ready

[Brian Norris]

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 <computersforpeace@gmail.com>
---
This patch is really just for testing. I don't think we really need it,
although I suppose some form of it couldn't hurt...

 drivers/mtd/spi-nor/spi-nor.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index d08d9f8bb9bd..c2e53e02fe27 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -226,6 +226,8 @@ static int erase_chip(struct spi_nor *nor)
 {
 	dev_dbg(nor->dev, " %lldKiB\n", (long long)(nor->mtd->size >> 10));
 
+	BUG_ON(spi_nor_ready(nor) <= 0);
+
 	return nor->write_reg(nor, SPINOR_OP_CHIP_ERASE, NULL, 0, 0);
 }
 
@@ -278,6 +280,8 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr)
 	if (ret)
 		return ret;
 
+	BUG_ON(spi_nor_ready(nor) <= 0);
+
 	write_enable(nor);
 
 	/* whole-chip erase? */
@@ -339,6 +343,8 @@ static int spi_nor_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	if (ret)
 		return ret;
 
+	BUG_ON(spi_nor_ready(nor) <= 0);
+
 	status_old = read_sr(nor);
 
 	if (offset < mtd->size - (mtd->size / 2))
@@ -381,6 +387,8 @@ static int spi_nor_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	if (ret)
 		return ret;
 
+	BUG_ON(spi_nor_ready(nor) <= 0);
+
 	status_old = read_sr(nor);
 
 	if (offset+len > mtd->size - (mtd->size / 64))
@@ -678,6 +686,8 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len,
 	if (ret)
 		return ret;
 
+	BUG_ON(spi_nor_ready(nor) <= 0);
+
 	ret = nor->read(nor, from, len, retlen, buf);
 
 	spi_nor_unlock_and_unprep(nor, SPI_NOR_OPS_READ);
@@ -697,6 +707,8 @@ static int sst_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (ret)
 		return ret;
 
+	BUG_ON(spi_nor_ready(nor) <= 0);
+
 	write_enable(nor);
 
 	nor->sst_write_second = false;
@@ -768,6 +780,8 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len,
 	if (ret)
 		return ret;
 
+	BUG_ON(spi_nor_ready(nor) <= 0);
+
 	write_enable(nor);
 
 	page_offset = to & (nor->page_size - 1);
-- 
1.9.1

Re: [RFC 8/8] debug: mtd: spi-nor: add BUG_ON() prints to check for !ready

[Marek Vasut]

On Thursday, August 07, 2014 at 03:17:02 AM, Brian Norris wrote:
> 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 <computersforpeace@gmail.com>

I'm all for this, but some kind of a WARN_ON_ONCE would do.

Best regards,
Marek Vasut

Re: [PATCH 0/8] mtd: spi-nor: refactor wait-till-ready

[Brian Norris]

On Wed, Aug 06, 2014 at 06:16:54PM -0700, Brian Norris wrote:
> 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

Pushed the first 6, with some small rebasing. I took v2 for patch 1.

I'll update patch 7 and send it out later.

Brian