Message ID | TY1PR01MB0954946451074852ED6D5C8FDC9F0@TY1PR01MB0954.jpnprd01.prod.outlook.com |
---|---|
State | Superseded |
Delegated to: | Boris Brezillon |
Headers | show |
Series | [v3] mtd: cfi_cmdset_0002: Change write buffer to check correct value | expand |
On Fri, 2018-05-11 at 07:58 +0000, IKEGAMI Tokunori wrote: > > From: Tokunori Ikegami <ikegami@allied-telesis.co.jp> > > For the word write it is checked if the chip has the correct value. > But it is not checked for the write buffer as only checked if ready. > To make sure for the write buffer change to check the value. > > It is enough as this patch is only checking the last written word. > Since it is described by data sheets to check the operation status. > > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > Cc: Brian Norris <computersforpeace@gmail.com> > Cc: David Woodhouse <dwmw2@infradead.org> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com> > Cc: Marek Vasut <marek.vasut@gmail.com> > Cc: Richard Weinberger <richard@nod.at> > Cc: Cyrille Pitchen <cyrille.pitchen@wedev4u.fr> > Cc: linux-mtd@lists.infradead.org > --- > No difference for the change itself from the original patch v1. > But just updated the patch subject to add version as v3. > Form the original patch v1 it is also changed to add Brian as Cc and the patch is sent to Boris-san as To. > Note: This was changed from the second patch v2 and this v3 patch is not changed about this. > > This fix is required to resolve the flash write error caused on our products. > > This patch itself is not depended to other related patches to fix for the flash erase error. > The flash erase error is not able to still reproduce at this moment. > So I will do fix the flash erase error separately. > > But the flash write error can be reproduced and this fix can resolve it. > So please review the patch again and if any problem please let me know. > > > drivers/mtd/chips/cfi_cmdset_0002.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index 56aa6b75213d..5e9f2ca0a6c1 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -1879,7 +1879,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > if (time_after(jiffies, timeo) && !chip_ready(map, adr)) > break; > > - if (chip_ready(map, adr)) { > + if (chip_ready(map, adr) && chip_good(map, adr, datum)) { > xip_enable(map, chip, adr); > goto op_done; > } What goes on here, I think, is that the Toggle bit is not reliable and adding chip_good() basically tests DQ7 too. If that is the case, one should only need chip_good(), have you tested that? In general I think toggle bit is awkward and moving to DQ7 would simply things once tests for other status bits are added. Jocke
Hi Jocke-san, Thank you so much for your comments. > What goes on here, I think, is that the Toggle bit is not reliable and > adding chip_good() > basically tests DQ7 too. > If that is the case, one should only need chip_good(), have you tested > that? > > In general I think toggle bit is awkward and moving to DQ7 would simply > things once > tests for other status bits are added. Yes you are right. Since I have just confirmed that the flash write issue can be resolved by checking chip_good() only. So I will do update the patch as so. Regards, Ikegami
On Tue, 2018-05-15 at 09:08 +0000, IKEGAMI Tokunori 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. > > > Hi Jocke-san, > > Thank you so much for your comments. > > > What goes on here, I think, is that the Toggle bit is not reliable and > > adding chip_good() > > basically tests DQ7 too. > > If that is the case, one should only need chip_good(), have you tested > > that? > > > > In general I think toggle bit is awkward and moving to DQ7 would simply > > things once > > tests for other status bits are added. > > Yes you are right. > Since I have just confirmed that the flash write issue can be resolved by checking chip_good() only. > So I will do update the patch as so. Then I got really curious, dos the same work for your erase problem? Replace chip_ready(...) with chip_good(map, adr, map_word_ff(map) ? Don't you have any status bits when erase/write fails? Jocke
Hi Jocke-san, > Then I got really curious, dos the same work for your erase problem? > Replace chip_ready(...) with chip_good(map, adr, map_word_ff(map) ? > Don't you have any status bits when erase/write fails? Yes this also you are right. I have just tested the effect as below. The flash erase error also does not occur by checking chip_good instead of chip_ready. This has been tested by the same reproduction method. It looks that the flash erase error is also able to be resolved by chip_good. So I will do update the patch series to change use chip_good instead of chip_ready. But to make sure I would like remain the changes to retry the error. Also I will combine the changes for the flash write error also as same series. Regards, Ikegami
On Tue, 2018-05-15 at 16:49 +0000, IKEGAMI Tokunori wrote: > > Hi Jocke-san, Hi Ikegami-san, > > > Then I got really curious, dos the same work for your erase problem? > > Replace chip_ready(...) with chip_good(map, adr, map_word_ff(map) ? > > Don't you have any status bits when erase/write fails? > > Yes this also you are right. > I have just tested the effect as below. > The flash erase error also does not occur by checking chip_good instead of chip_ready. > This has been tested by the same reproduction method. > It looks that the flash erase error is also able to be resolved by chip_good. > So I will do update the patch series to change use chip_good instead of chip_ready. > But to make sure I would like remain the changes to retry the error. > Also I will combine the changes for the flash write error also as same series. Nice :) I just had a look at chip_good() and saw that it is actually both toggle and DQ7, belt and suspenders :) I thought chip_good() was only DQ7, anyhow, I guess this makes the test very robust but a bit slow. > > Regards, > Ikegami >
Hi Jocke-san, Thanks for the comments. Noted it. I am happy with it;-) Regards, Ikegami > -----Original Message----- > From: Joakim Tjernlund [mailto:Joakim.Tjernlund@infinera.com] > Sent: Wednesday, May 16, 2018 2:17 AM > To: boris.brezillon@bootlin.com; IKEGAMI Tokunori > Cc: linux-mtd@lists.infradead.org; PACKHAM Chris; > cyrille.pitchen@wedev4u.fr; dwmw2@infradead.org; > computersforpeace@gmail.com; boris.brezillon@free-electrons.com; > richard@nod.at; marek.vasut@gmail.com > Subject: Re: [PATCH v3] mtd: cfi_cmdset_0002: Change write buffer to > check correct value > > On Tue, 2018-05-15 at 16:49 +0000, IKEGAMI Tokunori wrote: > > > > Hi Jocke-san, > > Hi Ikegami-san, > > > > > > Then I got really curious, dos the same work for your erase problem? > > > Replace chip_ready(...) with chip_good(map, adr, map_word_ff(map) ? > > > Don't you have any status bits when erase/write fails? > > > > Yes this also you are right. > > I have just tested the effect as below. > > The flash erase error also does not occur by checking chip_good > instead of chip_ready. > > This has been tested by the same reproduction method. > > It looks that the flash erase error is also able to be resolved by > chip_good. > > So I will do update the patch series to change use chip_good instead > of chip_ready. > > But to make sure I would like remain the changes to retry the error. > > Also I will combine the changes for the flash write error also as > same series. > > Nice :) I just had a look at chip_good() and saw that it is actually both > toggle and DQ7, belt and suspenders :) > I thought chip_good() was only DQ7, anyhow, I guess this makes the test > very robust but a bit slow. > > > > > Regards, > > Ikegami > >
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index 56aa6b75213d..5e9f2ca0a6c1 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -1879,7 +1879,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, if (time_after(jiffies, timeo) && !chip_ready(map, adr)) break; - if (chip_ready(map, adr)) { + if (chip_ready(map, adr) && chip_good(map, adr, datum)) { xip_enable(map, chip, adr); goto op_done; }