Hi IKEGAMI,
On Tue, 6 Nov 2018 00:25:43 +0000
IKEGAMI Tokunori <ikegami@xxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
> > > Signed-off-by: Hauke Mehrtens <hauke@xxxxxxxxxx>
> > > Signed-off-by: Koen Vandeputte <koen.vandeputte@xxxxxxxxxxxx>
> > > Signed-off-by: Fabio Bettoni <fbettoni@xxxxxxxxx>
> >
> > 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@xxxxxxxxxxx> 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@xxxxxxxxxx>
> > > Co-Developed-by: Koen Vandeputte <koen.vandeputte@xxxxxxxxxxxx>
> > > Co-Developed-by: Fabio Bettoni <fbettoni@xxxxxxxxx>
> >
> > 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
