Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change do_write_oneword() to use chip_good()

[Boris Brezillon <boris.brezillon@bootlin.com>]

On Mon, 5 Nov 2018 12:03:04 +0000
Joakim Tjernlund <Joakim.Tjernlund@infinera.com> wrote:

> On Mon, 2018-11-05 at 11:15 +0100, Boris Brezillon wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
> > 
> > 
> > On Fri, 26 Oct 2018 01:32:09 +0900
> > Tokunori Ikegami <ikegami@allied-telesis.co.jp> wrote:
> >   
> > > In OpenWrt Project the flash write error caused on some products.  
> > 
> > It's okay to mention that the issue was discovered by the OpenWRT team,
> > but I'd rephrase it differently.
> > 
> > "As reported by the OpenWRT team, write requests sometimes fail on some
> > platforms".
> >   
> > > Also the issue can be fixed by using chip_good() instead of chip_ready().
> > > The chip_ready() just checks the value from flash memory twice.
> > > And the chip_good() checks the value with the expected value.
> > > Probably the issue can be fixed as checked correctly by the chip_good().
> > > So change to use chip_good() instead of chip_ready().  
> > 
> > Well, that's not really explaining why you think chip_good() should be
> > used instead of chip_ready(). So I went on and looked at the
> > chip_good(), chip_ready() and do_write_oneword() implementation, and
> > also looked at users of do_write_oneword(). It seems this function is
> > used to write data to the flash, and apparently the "one bit should
> > toggle to reflect a busy state" does not apply when writing things to
> > the memory array (it's probably working for other CFI commands, but I
> > guess it takes more time to actually change the level of a NOR cell,
> > hence the result of 2 identical reads does not mean that the write is
> > done).
> > 
> > Also, it seems that cmdset_0001 is not implementing chip_ready() the
> > same way, and I wonder if cmdset_0002 implementation is correct to
> > start with. Or maybe I don't get what chip_ready() is for.
> >   
> The 0001 cmd set is quite different to 0002 and 0001 is the superior one.
> If you look at recent 0002 cmd sets they offer an alternative cmd
> set to replace the all the "toggle" ones with something that is
> same/similar to what 0001 offers.

Okay. Do you know when chip_ready() (the one that checks if something
changes between 2 reads) should be used and when it shouldn't?