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