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

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

Hi IKEGAMI,

On Tue, 6 Nov 2018 00:25:43 +0000
IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote:

> > > 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.
> > 
> > Anyway, this is the sort of clarification I'd like to have.  
> 
> I am thinking to update the commit message as below.
> 
>     mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()
> 
>     As reported by the OpenWRT team, write requests sometimes fail on some
>     platforms.
>     Currently to check the state chip_ready() is used correctly as described by
>     the flash memory S29GL256P11TFI01 datasheet.

I had a look at the S29GL256P datasheet here [1], and if I'm correct,
it's using cmdset 0001.

>     Also chip_good() is used to check if the write is succeeded and it was
>     implemented by the commit fb4a90bfcd6d8 ("[MTD] CFI-0002 - Improve error
>     checking").
>     But actually the write failure is caused on some platforms and also it can
>     be fixed by using chip_good() to check the state and retry instead.

Do you know on which NOR chips this happens? Do you have access to the
datasheet?

>     It is depended on the actual flash chip behavior so the root cause is
>     unknown.

Yes, and that's what I'd like you to figure out, or at least have a
good idea why this doesn't work on some chips but works on others.

> 
> If any comment please let me know.
> 
> >   
> > >
> > > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp>
> > > Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> > > Signed-off-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> > > Signed-off-by: Fabio Bettoni <fbettoni@gmail.com>  
> > 
> > Has the patch really gone through all those people? SoB is used when you
> > apply a patch in your tree or when you're the original author.  
> 
> I have just checked the OpenWRT git log again and it looks that it was originally
> implemented by Felix Fietkau <nbd@openwrt.org> by the patch below so I will update the Signed-off-by tag as so.
>   <https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=2530640f07cd2b3b14fe9ec03fa63a586452cc5f>
> 
> >   
> > > Co-Developed-by: Hauke Mehrtens <hauke@hauke-m.de>
> > > Co-Developed-by: Koen Vandeputte <koen.vandeputte@ncentric.com>
> > > Co-Developed-by: Fabio Bettoni <fbettoni@gmail.com>  
> > 
> > Not sure we want to add new undocumented tags, but you can mention
> > that all those people helped you find/debug the issue. They can also
> > add their Reviewed-by/Tested-by if they like.

My bad, I just noticed these are valid flags [2], so you can keep them,
and according to the doc, you should also keep the SoB.

Regards,

Boris

[1]http://www.cypress.com/file/219926/download
[2]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v4.20-rc1#n546