Thank you so much for your reviewing. > -----Original Message----- > From: Boris Brezillon [mailto:boris.brezillon@xxxxxxxxxxx] > Sent: Monday, November 05, 2018 7:16 PM > To: IKEGAMI Tokunori > Cc: boris.brezillon@xxxxxxxxxxxxxxxxxx; Hauke Mehrtens; > stable@xxxxxxxxxxxxxxx; Joakim Tjernlund; PACKHAM Chris; > linux-mtd@xxxxxxxxxxxxxxxxxxx; Koen Vandeputte; Fabio Bettoni > Subject: Re: [PATCH v3 01/11] mtd: cfi_cmdset_0002: Change > do_write_oneword() to use chip_good() > > On Fri, 26 Oct 2018 01:32:09 +0900 > Tokunori Ikegami <ikegami@xxxxxxxxxxxxxxxxxxxx> 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". Yes I will do fix as this. > > > 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. 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. It is depended on the actual flash chip behavior so the root cause is unknown. 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. Yes so I am thinking to change as below. Signed-off-by: Tokunori Ikegami <ikegami@xxxxxxxxxxxxxxxxxxxx> Signed-off-by: Felix Fietkau <nbd@xxxxxxxxxxx> Tested-by: Fabio Bettoni <fbettoni@xxxxxxxxx> Reported-by: Fabio Bettoni <fbettoni@xxxxxxxxx> Cc: Hauke Mehrtens <hauke@xxxxxxxxxx> Cc: Koen Vandeputte <koen.vandeputte@xxxxxxxxxxxx> If any problem let me know. By the way the patch has been tested by Fabio-san then there was still the failure behavior. And it was not followed the original patch changes correctly. So I will update the patch change a little by the next version v4 patch. Note: It has been retested by the Fabio-san as okay. > > > Reported-by: Fabio Bettoni <fbettoni@xxxxxxxxx> > > Cc: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx> > > Cc: Joakim Tjernlund <Joakim.Tjernlund@xxxxxxxxxxxx> > > Cc: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx> > > Cc: linux-mtd@xxxxxxxxxxxxxxxxxxx > > Cc: stable@xxxxxxxxxxxxxxx > > --- > > Changes since v2: > > - Just update the commit message for the comment. > > > > Changes since v1: > > - Just update the commit message. > > > > Background: > > This is required for OpenWrt Project to result the flash write issue as > > below patche. > > > <https://git.openwrt.org/?p=openwrt/openwrt.git;a=commitdiff;h=ddc11c3 > 932c7b7b7df7d5fbd48f207e77619eaa7> > > > > Also the original patch in OpenWRT is below. > > > <https://github.com/openwrt/openwrt/blob/v18.06.0/target/linux/ar71xx/ > patches-4.9/403-mtd_fix_cfi_cmdset_0002_status_check.patch> > > > > The reason to use chip_good() is that just actually fix the issue. > > And also in the past I had fixed the erase function also as same way by > the > > patch below. > > <https://patchwork.ozlabs.org/patch/922656/> > > Note: The reason for the patch for erase is same. > > > > In my understanding the chip_ready() is just checked the value twice from > > flash. > > So I think that sometimes incorrect value is read twice and it is depended > > on the flash device behavior but not sure.. > > > > So change to use chip_good() instead of chip_ready(). > > > > drivers/mtd/chips/cfi_cmdset_0002.c | 18 ++++++++++++------ > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c > b/drivers/mtd/chips/cfi_cmdset_0002.c > > index 72428b6bfc47..251c9e1675bd 100644 > > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > > @@ -1627,31 +1627,37 @@ static int __xipram do_write_oneword(struct > map_info *map, struct flchip *chip, > > continue; > > } > > > > - if (time_after(jiffies, timeo) && !chip_ready(map, adr)){ > > + if (chip_good(map, adr, datum)) > > + break; > > + > > + if (time_after(jiffies, timeo)){ > > xip_enable(map, chip, adr); > > printk(KERN_WARNING "MTD %s(): software > timeout\n", __func__); > > xip_disable(map, chip, adr); > > + ret = -EIO; > > break; > > } > > > > - if (chip_ready(map, adr)) > > - break; > > - > > /* Latency issues. Drop the lock, wait a while and retry > */ > > UDELAY(map, chip, adr, 1); > > } > > + > > /* Did we succeed? */ > > - if (!chip_good(map, adr, datum)) { > > + if (ret) { > > /* reset on all failures. */ > > map_write(map, CMD(0xF0), chip->start); > > /* FIXME - should have reset delay before continuing */ > > > > - if (++retry_cnt <= MAX_RETRIES) > > + if (++retry_cnt <= MAX_RETRIES) { > > + ret = 0; > > goto retry; > > + } > > > > ret = -EIO; > > } > > + > > xip_enable(map, chip, adr); > > + > > Not a big deal, but I'd prefer to not have coding style changes mixed > with functional changes (in other words, you can drop the addition of > blanks lines around xip_enable()). Yes I will do fix this. Regards, Ikegami > > > op_done: > > if (mode == FL_OTP_WRITE) > > otp_exit(map, chip, adr, map_bankwidth(map));
