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

login
register
mail settings
Submitter Huang Shijie
Date Aug. 12, 2014, 12:59 a.m.
Message ID <20140812005912.GA1850@shldeISGChi005.sh.intel.com>
Download mbox | patch
Permalink /patch/379169/
State New
Headers show

Comments

Huang Shijie - Aug. 12, 2014, 12:59 a.m.
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:



thanks
Huang Shijie
Brian Norris - Sept. 10, 2014, 7:05 a.m.
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
Brian Norris - Sept. 10, 2014, 7:47 a.m.
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
Huang Shijie - Sept. 10, 2014, 3:20 p.m.
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.
Huang Shijie - Sept. 10, 2014, 4:12 p.m.
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().
Brian Norris - Sept. 10, 2014, 11:25 p.m.
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

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;