[Patch 1/1] don't suspend erase for erase in cfi_cmdset_0002

[Joakim Tjernlund]

Peter Wippich <pewi at gw-instruments.de> wrote on 04/08/2009 14:21:25:
>
>
> And Hi again,
>
> On Tue, 4 Aug 2009, Joakim Tjernlund wrote:
>
> > Peter Wippich <pewi at gw-instruments.de> wrote on 03/08/2009 23:24:01:
> > >
> > >
> > > Hi Jocke,
> > >
> > > On Mon, 3 Aug 2009, Joakim Tjernlund wrote:
> > >
> > > > Hi again :)
> > > >
> > > > I think mine is better because:
> > > > 1) It matches cmdset_0001 and the code in general better.
> > > > 2) It can handle FL_WRITING too.
> > > >
> > > > But I am not the maintainer and I don't use 0002 myself, just trying to
> > > > help out.
> > > >
> > > > I hope you will constuct a cfip too for your JEDEC chips. It is no
> > > > fun to be without erase suspend :(
> > > >
> > >
> > > It's fine to have some discussion about that. From that at least we agree
> > > that something is wrong here. The main difference between the suggested
> > > patches is the change in behaviour from the current implementation.
> > > Currently the default behaviour is that erase suspend will work with jedec
> > > and cfi probed chips. I can not tell if this is by intention or by chance,
> > > but I think if we change something it should not break existing
> > > implementations.
> > > So at least a final fix shall:
> > > - use erase suspend for jedec chips for "read only" suspends were
> > >   cfip == NULL
> >
> > Are you sure all JEDEC chips support RO suspends? If not,
> > I think this is a 3 step/pathes procedure:
> > 1) Just fix the if stmt as I outlined. Then JEDEC chips wont bug, but won't have erase
> >    suspend either unless:
> >
> > 2) Add a fixup for your JEDEC flash that fills in the erase suspend bits you
> need. If all
> >    JEDEC chips support RO suspends you can do a JEDEC generic fixup that
> adds this capability
> >    to cfip.
> >
> > 3) remove the FIXME and test if it works for you. Send it to the
> >    list too and ask for some testing.
> >
> > > - use erase suspend for all cfi probed chips for "read only" suspends
> > >   if supported (as indicated by cfip->EraseSuspend)
> > >
> > > And for the write case, I can not tell why the erase suspend is viewed to
> > > be broken here. If we change this,  as in your patch, severe testing is
> > > needed to make sure it is realy working. Currently I don't have the free
> > > resources to do this testing. Maybe we'll find some volunteer ?!.......
> > >
> > > I'll try to find a fix which will not break current implementations and
> > > allows updates to implement erase suspend for writes easily. Appriciate
> > > your comments.
> > >
> > > BTW: who's the maintainer and what are her/his  thoughts.
> >
> > Don't konw.
>
> Partly disagree. For Step 1 we shall leave erase suspend active for Jedec
> Chips because this is the current behaviour. If this behaviour is wrong
> we would have seen a lot of complains on the list (which aren't there).
> If we change the default behaviour then we have to add fxups for each and
> every Jedec chip which is used out there and supports erase suspend. And
> from cherry picking in the long list most chips which use
> cfi_cmdset_0002 support erase suspend.

Well, I figured the fixups was already there, in jedec_probe.c, but your chip isn't
listed I guess. Try adding your chip there and se what happens. I suspect
that only chips that are in this list works or they are all broken if the
do not set cfip.

>
> And BTW: I realy hate this "cfip->EraseSuspend & 0x1" stuff. Nobody knows
> what this is unless going through the CFI specification. It would make
> the code much better readable if we introduce some symbolic constants
> here.

Possibly, but that is another issue that will have to be addressed with a
separate cleanup patch. The style in both 0001 and 0002 is the "cfip->EraseSuspend & 0x1"
style so you better follow it with your fixes, you got a much better chance getting
you stuff in the tree with as little changes as possible while keeping the current style.

Anyhow, style should be discussed with David Woodhouse, the maintainer of MTD.

I am done with this now and you need to find someone else that actually
cares/uses this driver with JEDEC chips.

      Jocke