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

Huang Shijie
2014-09-10T08:15:26Z

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