| Message ID | TY1PR01MB0954474CBDBF98D0E3D3E648DC9A0@TY1PR01MB0954.jpnprd01.prod.outlook.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mtd: cfi_cmdset_0002: Change erase functions to retry for error | expand |
Hello, On Tue, 8 May 2018 10:20:29 +0000 IKEGAMI Tokunori <ikegami@allied-telesis.co.jp> wrote: > From: Tokunori Ikegami <ikegami@allied-telesis.co.jp> > > For the word write functions it is retried for error. > But it is not implemented to retry for the erase functions. > To make sure for the erase functions change to retry as same. Can you give a bit more context, like why this is needed, where did you see the problem (which chip/controller), ... > > Signed-off-by: Tokunori Ikegami <ikegami@allied-telesis.co.jp> > Cc: Chris Packham <chris.packham@alliedtelesis.co.nz> > 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 > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 36 +++++++++++++++++++++++------------- > 1 file changed, 23 insertions(+), 13 deletions(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index 56aa6b75213d..e2dc020cd102 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -42,7 +42,7 @@ > #define AMD_BOOTLOC_BUG > #define FORCE_WORD_WRITE 0 > > -#define MAX_WORD_RETRIES 3 > +#define MAX_WRITE_RETRIES 3 Not sure why you rename this macro, but if it's needed can you do that in a separate patch? > > #define SST49LF004B 0x0060 > #define SST49LF040B 0x0050 > @@ -207,7 +207,7 @@ static void fixup_use_write_buffers(struct mtd_info *mtd) > struct map_info *map = mtd->priv; > struct cfi_private *cfi = map->fldrv_priv; > if (cfi->cfiq->BufWriteTimeoutTyp) { > - pr_debug("Using buffer write method\n" ); > + pr_debug("Using buffer write method\n"); Same for coding style issues, they should be fixed in a separate commit. > mtd->_write = cfi_amdstd_write_buffers; > } > } > @@ -1577,7 +1577,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip, > } > > pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", > - __func__, adr, datum.x[0] ); > + __func__, adr, datum.x[0]); > > if (mode == FL_OTP_WRITE) > otp_enter(map, chip, adr, map_bankwidth(map)); > @@ -1643,10 +1643,10 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip, > /* Did we succeed? */ > if (!chip_good(map, adr, datum)) { > /* reset on all failures. */ > - map_write( map, CMD(0xF0), chip->start ); > + map_write(map, CMD(0xF0), chip->start); > /* FIXME - should have reset delay before continuing */ > > - if (++retry_cnt <= MAX_WORD_RETRIES) > + if (++retry_cnt <= MAX_WRITE_RETRIES) > goto retry; > > ret = -EIO; > @@ -1821,7 +1821,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, > datum = map_word_load(map, buf); > > pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", > - __func__, adr, datum.x[0] ); > + __func__, adr, datum.x[0]); > > XIP_INVAL_CACHED_RANGE(map, adr, len); > ENABLE_VPP(map); > @@ -2105,7 +2105,7 @@ static int do_panic_write_oneword(struct map_info *map, struct flchip *chip, > map_write(map, CMD(0xF0), chip->start); > /* FIXME - should have reset delay before continuing */ > > - if (++retry_cnt <= MAX_WORD_RETRIES) > + if (++retry_cnt <= MAX_WRITE_RETRIES) > goto retry; > > ret = -EIO; > @@ -2240,6 +2240,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip) > unsigned long int adr; > DECLARE_WAITQUEUE(wait, current); > int ret = 0; > + int retry_cnt = 0; > > adr = cfi->addr_unlock1; > > @@ -2251,12 +2252,13 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip) > } > > pr_debug("MTD %s(): ERASE 0x%.8lx\n", > - __func__, chip->start ); > + __func__, chip->start); > > XIP_INVAL_CACHED_RANGE(map, adr, map->size); > ENABLE_VPP(map); > xip_disable(map, chip, adr); > > + retry: > cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); > cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL); > cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); > @@ -2297,7 +2299,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip) > > if (time_after(jiffies, timeo)) { > printk(KERN_WARNING "MTD %s(): software timeout\n", > - __func__ ); > + __func__); > break; > } > > @@ -2307,9 +2309,12 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip) > /* Did we succeed? */ > if (!chip_good(map, adr, map_word_ff(map))) { > /* reset on all failures. */ > - map_write( map, CMD(0xF0), chip->start ); > + map_write(map, CMD(0xF0), chip->start); > /* FIXME - should have reset delay before continuing */ > > + if (++retry_cnt <= MAX_WRITE_RETRIES) > + goto retry; Shouldn't we have a MAX_ERASE_RETRY for that? Also, what makes you think the following erase operations will work? Thanks, Boris
Hi Boris-san, Thank you so much for your review comments. > > For the word write functions it is retried for error. > > But it is not implemented to retry for the erase functions. > > To make sure for the erase functions change to retry as same. > > Can you give a bit more context, like why this is needed, where did > you see the problem (which chip/controller), ... Okay I will do update the commit message to add more context. > > -#define MAX_WORD_RETRIES 3 > > +#define MAX_WRITE_RETRIES 3 > > Not sure why you rename this macro, but if it's needed can you do that > in a separate patch? The reason is just to share the definition for both the word write and erase functions. Okay I will consider to do this in a separate patch. > > - pr_debug("Using buffer write method\n" ); > > + pr_debug("Using buffer write method\n"); > > Same for coding style issues, they should be fixed in a separate commit. Okay I will do that. > > + if (++retry_cnt <= MAX_WRITE_RETRIES) > > + goto retry; > > Shouldn't we have a MAX_ERASE_RETRY for that? It is okay also and I also thought this at first. But the similar definitions are defined so I thought that it is better to be shared as MAX_WRITE_RETRIES. > Also, what makes you think the following erase operations will work? I think that it is possible to be recovered by the retry and it is same for the word write operations. Regards, Ikegami
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index 56aa6b75213d..e2dc020cd102 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -42,7 +42,7 @@ #define AMD_BOOTLOC_BUG #define FORCE_WORD_WRITE 0 -#define MAX_WORD_RETRIES 3 +#define MAX_WRITE_RETRIES 3 #define SST49LF004B 0x0060 #define SST49LF040B 0x0050 @@ -207,7 +207,7 @@ static void fixup_use_write_buffers(struct mtd_info *mtd) struct map_info *map = mtd->priv; struct cfi_private *cfi = map->fldrv_priv; if (cfi->cfiq->BufWriteTimeoutTyp) { - pr_debug("Using buffer write method\n" ); + pr_debug("Using buffer write method\n"); mtd->_write = cfi_amdstd_write_buffers; } } @@ -1577,7 +1577,7 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip, } pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", - __func__, adr, datum.x[0] ); + __func__, adr, datum.x[0]); if (mode == FL_OTP_WRITE) otp_enter(map, chip, adr, map_bankwidth(map)); @@ -1643,10 +1643,10 @@ static int __xipram do_write_oneword(struct map_info *map, struct flchip *chip, /* Did we succeed? */ if (!chip_good(map, adr, datum)) { /* reset on all failures. */ - map_write( map, CMD(0xF0), chip->start ); + map_write(map, CMD(0xF0), chip->start); /* FIXME - should have reset delay before continuing */ - if (++retry_cnt <= MAX_WORD_RETRIES) + if (++retry_cnt <= MAX_WRITE_RETRIES) goto retry; ret = -EIO; @@ -1821,7 +1821,7 @@ static int __xipram do_write_buffer(struct map_info *map, struct flchip *chip, datum = map_word_load(map, buf); pr_debug("MTD %s(): WRITE 0x%.8lx(0x%.8lx)\n", - __func__, adr, datum.x[0] ); + __func__, adr, datum.x[0]); XIP_INVAL_CACHED_RANGE(map, adr, len); ENABLE_VPP(map); @@ -2105,7 +2105,7 @@ static int do_panic_write_oneword(struct map_info *map, struct flchip *chip, map_write(map, CMD(0xF0), chip->start); /* FIXME - should have reset delay before continuing */ - if (++retry_cnt <= MAX_WORD_RETRIES) + if (++retry_cnt <= MAX_WRITE_RETRIES) goto retry; ret = -EIO; @@ -2240,6 +2240,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip) unsigned long int adr; DECLARE_WAITQUEUE(wait, current); int ret = 0; + int retry_cnt = 0; adr = cfi->addr_unlock1; @@ -2251,12 +2252,13 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip) } pr_debug("MTD %s(): ERASE 0x%.8lx\n", - __func__, chip->start ); + __func__, chip->start); XIP_INVAL_CACHED_RANGE(map, adr, map->size); ENABLE_VPP(map); xip_disable(map, chip, adr); + retry: cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL); cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); @@ -2297,7 +2299,7 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip) if (time_after(jiffies, timeo)) { printk(KERN_WARNING "MTD %s(): software timeout\n", - __func__ ); + __func__); break; } @@ -2307,9 +2309,12 @@ static int __xipram do_erase_chip(struct map_info *map, struct flchip *chip) /* Did we succeed? */ if (!chip_good(map, adr, map_word_ff(map))) { /* reset on all failures. */ - map_write( map, CMD(0xF0), chip->start ); + map_write(map, CMD(0xF0), chip->start); /* FIXME - should have reset delay before continuing */ + if (++retry_cnt <= MAX_WRITE_RETRIES) + goto retry; + ret = -EIO; } @@ -2329,6 +2334,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip, unsigned long timeo = jiffies + HZ; DECLARE_WAITQUEUE(wait, current); int ret = 0; + int retry_cnt = 0; adr += chip->start; @@ -2340,12 +2346,13 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip, } pr_debug("MTD %s(): ERASE 0x%.8lx\n", - __func__, adr ); + __func__, adr); XIP_INVAL_CACHED_RANGE(map, adr, len); ENABLE_VPP(map); xip_disable(map, chip, adr); + retry: cfi_send_gen_cmd(0xAA, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); cfi_send_gen_cmd(0x55, cfi->addr_unlock2, chip->start, map, cfi, cfi->device_type, NULL); cfi_send_gen_cmd(0x80, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); @@ -2389,7 +2396,7 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip, if (time_after(jiffies, timeo)) { xip_enable(map, chip, adr); printk(KERN_WARNING "MTD %s(): software timeout\n", - __func__ ); + __func__); break; } @@ -2399,9 +2406,12 @@ static int __xipram do_erase_oneblock(struct map_info *map, struct flchip *chip, /* Did we succeed? */ if (!chip_good(map, adr, map_word_ff(map))) { /* reset on all failures. */ - map_write( map, CMD(0xF0), chip->start ); + map_write(map, CMD(0xF0), chip->start); /* FIXME - should have reset delay before continuing */ + if (++retry_cnt <= MAX_WRITE_RETRIES) + goto retry; + ret = -EIO; }