| Message ID | b146c469-6cc3-885e-3e8e-ff7a5fa8dcd4@cogentembedded.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Vignesh R |
| Headers | show |
| Series | Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver | expand |
Hi, On 2019/10/04 3:34, Sergei Shtylyov wrote: > The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling > status register") added checking for the status register error bits into > chip_good() to only return 1 if these bits are zero. Unfortunately, this > means that polling using chip_good() always reaches a time-out condition > when erase or program failure bits are set. I think the status register > error checking should be fully delegated to cfi_check_err_status() that > should return whether any error bits were set or not... > > Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register") > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 55 +++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 25 deletions(-) > > Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c > =================================================================== > --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c > +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi > (extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG; > } > > -static void cfi_check_err_status(struct map_info *map, struct flchip *chip, > - unsigned long adr) > +static int cfi_check_err_status(struct map_info *map, struct flchip *chip, > + unsigned long adr) > { > struct cfi_private *cfi = map->fldrv_priv; > map_word status; > > if (!cfi_use_status_reg(cfi)) > - return; > + return 0; > > cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, > cfi->device_type, NULL); > @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct > > /* The error bits are invalid while the chip's busy */ > if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB))) > - return; > + return 0; > > if (map_word_bitsset(map, status, CMD(0x3a))) { > unsigned long chipstatus = MERGESTATUS(status); > @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct > if (chipstatus & CFI_SR_SLSB) > pr_err("%s sector write protected, status %lx\n", > map->name, chipstatus); > + return 1; Is it okay to be returned 1 for the errors CFI_SR_WBASB and CFI_SR_SLSB also? Before the change only CFI_SR_ESB and CFI_SR_PSB were checked by chip_good(). > } > + return 0; > } > > /* #define DEBUG_CFI_FEATURES */ > @@ -852,20 +854,16 @@ static int __xipram chip_good(struct map > > if (cfi_use_status_reg(cfi)) { > map_word ready = CMD(CFI_SR_DRB); > - map_word err = CMD(CFI_SR_PSB | CFI_SR_ESB); > + > /* > * For chips that support status register, check device > - * ready bit and Erase/Program status bit to know if > - * operation succeeded. > + * ready bit > */ > cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, > cfi->device_type, NULL); > curd = map_read(map, addr); > > - if (map_word_andequal(map, curd, ready, ready)) > - return !map_word_bitsset(map, curd, err); > - > - return 0; > + return map_word_andequal(map, curd, ready, ready); > } > > oldd = map_read(map, addr); > @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc > break; > } > > - if (chip_good(map, chip, adr, datum)) > + if (chip_good(map, chip, adr, datum)) { > + if (cfi_check_err_status(map, chip, adr)) > + ret = -EIO; > break; > + } > > /* Latency issues. Drop the lock, wait a while and retry */ > UDELAY(map, chip, adr, 1); > @@ -1777,7 +1778,6 @@ static int __xipram do_write_oneword_ret > ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi); > if (ret) { > /* reset on all failures. */ > - cfi_check_err_status(map, chip, adr); > map_write(map, CMD(0xF0), chip->start); > /* FIXME - should have reset delay before continuing */ > > @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait > */ > if (time_after(jiffies, timeo) && > !chip_good(map, chip, adr, datum)) { > + pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n", > + __func__, adr); Is there any reason to change the message to use pr_warn() from pr_err()? In the past it was changed to use pr_err() from printk(KERN_WARNING) as mentioned by the maintainer. Regards, Ikegami > ret = -EIO; > break; > } > > - if (chip_good(map, chip, adr, datum)) > + if (chip_good(map, chip, adr, datum)) { > + if (cfi_check_err_status(map, chip, adr)) > + ret = -EIO; > break; > + } > > /* Latency issues. Drop the lock, wait a while and retry */ > UDELAY(map, chip, adr, 1); > @@ -2075,12 +2080,8 @@ static int __xipram do_write_buffer(stru > chip->word_write_time); > > ret = do_write_buffer_wait(map, chip, adr, datum); > - if (ret) { > - cfi_check_err_status(map, chip, adr); > + if (ret) > do_write_buffer_reset(map, chip, cfi); > - pr_err("MTD %s(): software timeout, address:0x%.8lx.\n", > - __func__, adr); > - } > > xip_enable(map, chip, adr); > > @@ -2275,9 +2276,9 @@ retry: > udelay(1); > } > > - if (!chip_good(map, chip, adr, datum)) { > + if (!chip_good(map, chip, adr, datum) || > + cfi_check_err_status(map, chip, adr)) { > /* reset on all failures. */ > - cfi_check_err_status(map, chip, adr); > map_write(map, CMD(0xF0), chip->start); > /* FIXME - should have reset delay before continuing */ > > @@ -2471,8 +2472,11 @@ static int __xipram do_erase_chip(struct > chip->erase_suspended = 0; > } > > - if (chip_good(map, chip, adr, map_word_ff(map))) > + if (chip_good(map, chip, adr, map_word_ff(map))) { > + if (cfi_check_err_status(map, chip, adr)) > + ret = -EIO; > break; > + } > > if (time_after(jiffies, timeo)) { > printk(KERN_WARNING "MTD %s(): software timeout\n", > @@ -2487,7 +2491,6 @@ static int __xipram do_erase_chip(struct > /* Did we succeed? */ > if (ret) { > /* reset on all failures. */ > - cfi_check_err_status(map, chip, adr); > map_write(map, CMD(0xF0), chip->start); > /* FIXME - should have reset delay before continuing */ > > @@ -2568,8 +2571,11 @@ static int __xipram do_erase_oneblock(st > chip->erase_suspended = 0; > } > > - if (chip_good(map, chip, adr, map_word_ff(map))) > + if (chip_good(map, chip, adr, map_word_ff(map))) { > + if (cfi_check_err_status(map, chip, adr)) > + ret = -EIO; > break; > + } > > if (time_after(jiffies, timeo)) { > printk(KERN_WARNING "MTD %s(): software timeout\n", > @@ -2584,7 +2590,6 @@ static int __xipram do_erase_oneblock(st > /* Did we succeed? */ > if (ret) { > /* reset on all failures. */ > - cfi_check_err_status(map, chip, adr); > map_write(map, CMD(0xF0), chip->start); > /* FIXME - should have reset delay before continuing */ > > > ______________________________________________________ > Linux MTD discussion mailing list > http://lists.infradead.org/mailman/listinfo/linux-mtd/ >
On 10/06/2019 11:54 PM, Tokunori Ikegami wrote: >> The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling >> status register") added checking for the status register error bits into >> chip_good() to only return 1 if these bits are zero. Unfortunately, this >> means that polling using chip_good() always reaches a time-out condition >> when erase or program failure bits are set. I think the status register >> error checking should be fully delegated to cfi_check_err_status() that >> should return whether any error bits were set or not... >> >> Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register") >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> drivers/mtd/chips/cfi_cmdset_0002.c | 55 +++++++++++++++++++----------------- >> 1 file changed, 30 insertions(+), 25 deletions(-) >> >> Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c >> =================================================================== >> --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c >> +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c [...] >> @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct >> if (chipstatus & CFI_SR_SLSB) >> pr_err("%s sector write protected, status %lx\n", >> map->name, chipstatus); >> + return 1; > Is it okay to be returned 1 for the errors CFI_SR_WBASB and CFI_SR_SLSB also? > Before the change only CFI_Well, SR_ESB and CFI_SR_PSB were checked by chip_good(). Well, pr_err() calls above spoke for themselves: all bitmask 0x3a was considered the error bits. But I can change that back to just ESB/PSB if preferred. >> } >> + return 0; >> } >> /* #define DEBUG_CFI_FEATURES */ [...] >> @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait >> */ >> if (time_after(jiffies, timeo) && >> !chip_good(map, chip, adr, datum)) { >> + pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n", >> + __func__, adr); > > Is there any reason to change the message to use pr_warn() from pr_err()? Yes, all the other timeout messages use printk(KERN_WARNING, ...); > In the past it was changed to use pr_err() from printk(KERN_WARNING) as mentioned by the maintainer. Oh, OK, I'll switch back if so... > > Regards, > Ikegami [...] It's better to remove the diff you're not commenting to. MBR, Sergei
Hi, >>> if (chipstatus & CFI_SR_SLSB) >>> pr_err("%s sector write protected, status %lx\n", >>> map->name, chipstatus); >>> + return 1; >> Is it okay to be returned 1 for the errors CFI_SR_WBASB and CFI_SR_SLSB also? >> Before the change only CFI_Well, SR_ESB and CFI_SR_PSB were checked by chip_good(). > Well, pr_err() calls above spoke for themselves: all bitmask 0x3a was considered > the error bits. But I can change that back to just ESB/PSB if preferred. It was mentioned before as it is enough to see if ESB or PSB is set to know if write or erase failed. > It's better to remove the diff you're not commenting to. Noted it. Regards, Ikegami
Hi, On 04/10/19 12:04 AM, Sergei Shtylyov wrote: > The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling > status register") added checking for the status register error bits into > chip_good() to only return 1 if these bits are zero. Unfortunately, this > means that polling using chip_good() always reaches a time-out condition > when erase or program failure bits are set. I think the status register > error checking should be fully delegated to cfi_check_err_status() that > should return whether any error bits were set or not... > Please reword last sentence to drop "I think". Something like: Lets fully delegate the function of determining error condition to cfi_check_err_status() and make chip_good() only look for Device Ready/Busy condition. > Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register") > Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> > > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 55 +++++++++++++++++++----------------- > 1 file changed, 30 insertions(+), 25 deletions(-) > > Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c > =================================================================== > --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c > +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi > (extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG; > } > > -static void cfi_check_err_status(struct map_info *map, struct flchip *chip, > - unsigned long adr) > +static int cfi_check_err_status(struct map_info *map, struct flchip *chip, > + unsigned long adr) > { > struct cfi_private *cfi = map->fldrv_priv; > map_word status; > > if (!cfi_use_status_reg(cfi)) > - return; > + return 0; > > cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, > cfi->device_type, NULL); > @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct > > /* The error bits are invalid while the chip's busy */ > if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB))) > - return; > + return 0; > > if (map_word_bitsset(map, status, CMD(0x3a))) { > unsigned long chipstatus = MERGESTATUS(status); > @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct > if (chipstatus & CFI_SR_SLSB) > pr_err("%s sector write protected, status %lx\n", > map->name, chipstatus); > + return 1; > } > + return 0; > } > > /* #define DEBUG_CFI_FEATURES */ > @@ -852,20 +854,16 @@ static int __xipram chip_good(struct map > > if (cfi_use_status_reg(cfi)) { > map_word ready = CMD(CFI_SR_DRB); > - map_word err = CMD(CFI_SR_PSB | CFI_SR_ESB); > + > /* > * For chips that support status register, check device > - * ready bit and Erase/Program status bit to know if > - * operation succeeded. > + * ready bit > */ > cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, > cfi->device_type, NULL); > curd = map_read(map, addr); > > - if (map_word_andequal(map, curd, ready, ready)) > - return !map_word_bitsset(map, curd, err); > - > - return 0; > + return map_word_andequal(map, curd, ready, ready); > } > > oldd = map_read(map, addr); > @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc Nit: for some reason, your diff has function names truncated abruptly which makes its slightly harder to locate the context. I use git format-patch that produces better readable contexts. > break; > } > > - if (chip_good(map, chip, adr, datum)) > + if (chip_good(map, chip, adr, datum)) { > + if (cfi_check_err_status(map, chip, adr)) > + ret = -EIO; > break; > + } > > /* Latency issues. Drop the lock, wait a while and retry */ > UDELAY(map, chip, adr, 1); > @@ -1777,7 +1778,6 @@ static int __xipram do_write_oneword_ret > ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi); > if (ret) { > /* reset on all failures. */ > - cfi_check_err_status(map, chip, adr); > map_write(map, CMD(0xF0), chip->start); > /* FIXME - should have reset delay before continuing */ > > @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait > */ > if (time_after(jiffies, timeo) && > !chip_good(map, chip, adr, datum)) { > + pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n", > + __func__, adr); Since we are returning an error condition, this should be pr_err() (I know that rest of the file does not follow this convention, but lets make sure new code does) Rest looks fine to me. Thanks for the patch!
Hello! On 10/16/2019 09:33 AM, Vignesh Raghavendra wrote: >> The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling >> status register") added checking for the status register error bits into >> chip_good() to only return 1 if these bits are zero. Unfortunately, this >> means that polling using chip_good() always reaches a time-out condition >> when erase or program failure bits are set. I think the status register >> error checking should be fully delegated to cfi_check_err_status() that >> should return whether any error bits were set or not... >> > > Please reword last sentence to drop "I think". Something like: > > Lets fully delegate the function of determining error condition to > cfi_check_err_status() and make chip_good() only look for Device > Ready/Busy condition. OK. :-) >> Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register") >> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >> >> --- >> drivers/mtd/chips/cfi_cmdset_0002.c | 55 +++++++++++++++++++----------------- >> 1 file changed, 30 insertions(+), 25 deletions(-) >> >> Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c >> =================================================================== >> --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c >> +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c >> @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi >> (extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG; >> } >> >> -static void cfi_check_err_status(struct map_info *map, struct flchip *chip, >> - unsigned long adr) >> +static int cfi_check_err_status(struct map_info *map, struct flchip *chip, >> + unsigned long adr) >> { >> struct cfi_private *cfi = map->fldrv_priv; >> map_word status; >> >> if (!cfi_use_status_reg(cfi)) >> - return; >> + return 0; >> >> cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, >> cfi->device_type, NULL); >> @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct >> >> /* The error bits are invalid while the chip's busy */ >> if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB))) >> - return; >> + return 0; >> >> if (map_word_bitsset(map, status, CMD(0x3a))) { >> unsigned long chipstatus = MERGESTATUS(status); >> @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct >> if (chipstatus & CFI_SR_SLSB) >> pr_err("%s sector write protected, status %lx\n", >> map->name, chipstatus); >> + return 1; So are you OK with extending the set of the error signalling bits I did here, or I should really have accounted only for ESB and PSB bits being error signals? >> } >> + return 0; >> } >> >> /* #define DEBUG_CFI_FEATURES */ [...] >> @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc > > Nit: for some reason, your diff has function names truncated abruptly > which makes its slightly harder to locate the context. I use git > format-patch that produces better readable contexts. I use quilt for development, not a big fan of git in this role. :-) [...] >> @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait >> */ >> if (time_after(jiffies, timeo) && >> !chip_good(map, chip, adr, datum)) { >> + pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n", >> + __func__, adr); > > Since we are returning an error condition, this should be pr_err() (I > know that rest of the file does not follow this convention, but lets > make sure new code does) OK, I was looking at the other timeout code and failed to notice that this printk() was converted to pr_err() by Ikegami-san... > Rest looks fine to me. Thanks for the patch! TY for the review. MBR, Sergei
Hi, On 28/10/19 8:15 PM, Sergei Shtylyov wrote: > Hello! > > On 10/16/2019 09:33 AM, Vignesh Raghavendra wrote: > >>> The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling >>> status register") added checking for the status register error bits into >>> chip_good() to only return 1 if these bits are zero. Unfortunately, this >>> means that polling using chip_good() always reaches a time-out condition >>> when erase or program failure bits are set. I think the status register >>> error checking should be fully delegated to cfi_check_err_status() that >>> should return whether any error bits were set or not... >>> >> >> Please reword last sentence to drop "I think". Something like: >> >> Lets fully delegate the function of determining error condition to >> cfi_check_err_status() and make chip_good() only look for Device >> Ready/Busy condition. > > OK. :-) > >>> Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register") >>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> >>> >>> --- >>> drivers/mtd/chips/cfi_cmdset_0002.c | 55 +++++++++++++++++++----------------- >>> 1 file changed, 30 insertions(+), 25 deletions(-) >>> >>> Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c >>> =================================================================== >>> --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c >>> +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c >>> @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi >>> (extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG; >>> } >>> >>> -static void cfi_check_err_status(struct map_info *map, struct flchip *chip, >>> - unsigned long adr) >>> +static int cfi_check_err_status(struct map_info *map, struct flchip *chip, >>> + unsigned long adr) >>> { >>> struct cfi_private *cfi = map->fldrv_priv; >>> map_word status; >>> >>> if (!cfi_use_status_reg(cfi)) >>> - return; >>> + return 0; >>> >>> cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, >>> cfi->device_type, NULL); >>> @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct >>> >>> /* The error bits are invalid while the chip's busy */ >>> if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB))) >>> - return; >>> + return 0; >>> >>> if (map_word_bitsset(map, status, CMD(0x3a))) { >>> unsigned long chipstatus = MERGESTATUS(status); >>> @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct >>> if (chipstatus & CFI_SR_SLSB) >>> pr_err("%s sector write protected, status %lx\n", >>> map->name, chipstatus); >>> + return 1; > > So are you OK with extending the set of the error signalling bits I > did here, or I should really have accounted only for ESB and PSB bits > being error signals? > I am fine either way. Because as per HyperFlash datasheet, PSB or ESB will always be set if either of WASB or SLSB is set. So it does not really matter if we are checking for just ESB and PSB or otherwise. Regards Vignesh >>> } >>> + return 0; >>> } >>> >>> /* #define DEBUG_CFI_FEATURES */ > [...] >>> @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc >> >> Nit: for some reason, your diff has function names truncated abruptly >> which makes its slightly harder to locate the context. I use git >> format-patch that produces better readable contexts. > > I use quilt for development, not a big fan of git in this role. :-) > > [...] >>> @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait >>> */ >>> if (time_after(jiffies, timeo) && >>> !chip_good(map, chip, adr, datum)) { >>> + pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n", >>> + __func__, adr); >> >> Since we are returning an error condition, this should be pr_err() (I >> know that rest of the file does not follow this convention, but lets >> make sure new code does) > > OK, I was looking at the other timeout code and failed to notice > that this printk() was converted to pr_err() by Ikegami-san... > >> Rest looks fine to me. Thanks for the patch! > > TY for the review. > > MBR, Sergei >
Index: linux/drivers/mtd/chips/cfi_cmdset_0002.c =================================================================== --- linux.orig/drivers/mtd/chips/cfi_cmdset_0002.c +++ linux/drivers/mtd/chips/cfi_cmdset_0002.c @@ -123,14 +123,14 @@ static int cfi_use_status_reg(struct cfi (extp->SoftwareFeatures & poll_mask) == CFI_POLL_STATUS_REG; } -static void cfi_check_err_status(struct map_info *map, struct flchip *chip, - unsigned long adr) +static int cfi_check_err_status(struct map_info *map, struct flchip *chip, + unsigned long adr) { struct cfi_private *cfi = map->fldrv_priv; map_word status; if (!cfi_use_status_reg(cfi)) - return; + return 0; cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); @@ -138,7 +138,7 @@ static void cfi_check_err_status(struct /* The error bits are invalid while the chip's busy */ if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB))) - return; + return 0; if (map_word_bitsset(map, status, CMD(0x3a))) { unsigned long chipstatus = MERGESTATUS(status); @@ -155,7 +155,9 @@ static void cfi_check_err_status(struct if (chipstatus & CFI_SR_SLSB) pr_err("%s sector write protected, status %lx\n", map->name, chipstatus); + return 1; } + return 0; } /* #define DEBUG_CFI_FEATURES */ @@ -852,20 +854,16 @@ static int __xipram chip_good(struct map if (cfi_use_status_reg(cfi)) { map_word ready = CMD(CFI_SR_DRB); - map_word err = CMD(CFI_SR_PSB | CFI_SR_ESB); + /* * For chips that support status register, check device - * ready bit and Erase/Program status bit to know if - * operation succeeded. + * ready bit */ cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); curd = map_read(map, addr); - if (map_word_andequal(map, curd, ready, ready)) - return !map_word_bitsset(map, curd, err); - - return 0; + return map_word_andequal(map, curd, ready, ready); } oldd = map_read(map, addr); @@ -1703,8 +1701,11 @@ static int __xipram do_write_oneword_onc break; } - if (chip_good(map, chip, adr, datum)) + if (chip_good(map, chip, adr, datum)) { + if (cfi_check_err_status(map, chip, adr)) + ret = -EIO; break; + } /* Latency issues. Drop the lock, wait a while and retry */ UDELAY(map, chip, adr, 1); @@ -1777,7 +1778,6 @@ static int __xipram do_write_oneword_ret ret = do_write_oneword_once(map, chip, adr, datum, mode, cfi); if (ret) { /* reset on all failures. */ - cfi_check_err_status(map, chip, adr); map_write(map, CMD(0xF0), chip->start); /* FIXME - should have reset delay before continuing */ @@ -1974,12 +1974,17 @@ static int __xipram do_write_buffer_wait */ if (time_after(jiffies, timeo) && !chip_good(map, chip, adr, datum)) { + pr_warn("MTD %s(): software timeout, address:0x%.8lx.\n", + __func__, adr); ret = -EIO; break; } - if (chip_good(map, chip, adr, datum)) + if (chip_good(map, chip, adr, datum)) { + if (cfi_check_err_status(map, chip, adr)) + ret = -EIO; break; + } /* Latency issues. Drop the lock, wait a while and retry */ UDELAY(map, chip, adr, 1); @@ -2075,12 +2080,8 @@ static int __xipram do_write_buffer(stru chip->word_write_time); ret = do_write_buffer_wait(map, chip, adr, datum); - if (ret) { - cfi_check_err_status(map, chip, adr); + if (ret) do_write_buffer_reset(map, chip, cfi); - pr_err("MTD %s(): software timeout, address:0x%.8lx.\n", - __func__, adr); - } xip_enable(map, chip, adr); @@ -2275,9 +2276,9 @@ retry: udelay(1); } - if (!chip_good(map, chip, adr, datum)) { + if (!chip_good(map, chip, adr, datum) || + cfi_check_err_status(map, chip, adr)) { /* reset on all failures. */ - cfi_check_err_status(map, chip, adr); map_write(map, CMD(0xF0), chip->start); /* FIXME - should have reset delay before continuing */ @@ -2471,8 +2472,11 @@ static int __xipram do_erase_chip(struct chip->erase_suspended = 0; } - if (chip_good(map, chip, adr, map_word_ff(map))) + if (chip_good(map, chip, adr, map_word_ff(map))) { + if (cfi_check_err_status(map, chip, adr)) + ret = -EIO; break; + } if (time_after(jiffies, timeo)) { printk(KERN_WARNING "MTD %s(): software timeout\n", @@ -2487,7 +2491,6 @@ static int __xipram do_erase_chip(struct /* Did we succeed? */ if (ret) { /* reset on all failures. */ - cfi_check_err_status(map, chip, adr); map_write(map, CMD(0xF0), chip->start); /* FIXME - should have reset delay before continuing */ @@ -2568,8 +2571,11 @@ static int __xipram do_erase_oneblock(st chip->erase_suspended = 0; } - if (chip_good(map, chip, adr, map_word_ff(map))) + if (chip_good(map, chip, adr, map_word_ff(map))) { + if (cfi_check_err_status(map, chip, adr)) + ret = -EIO; break; + } if (time_after(jiffies, timeo)) { printk(KERN_WARNING "MTD %s(): software timeout\n", @@ -2584,7 +2590,6 @@ static int __xipram do_erase_oneblock(st /* Did we succeed? */ if (ret) { /* reset on all failures. */ - cfi_check_err_status(map, chip, adr); map_write(map, CMD(0xF0), chip->start); /* FIXME - should have reset delay before continuing */
The commit 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register") added checking for the status register error bits into chip_good() to only return 1 if these bits are zero. Unfortunately, this means that polling using chip_good() always reaches a time-out condition when erase or program failure bits are set. I think the status register error checking should be fully delegated to cfi_check_err_status() that should return whether any error bits were set or not... Fixes: 4844ef80305d ("mtd: cfi_cmdset_0002: Add support for polling status register") Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> --- drivers/mtd/chips/cfi_cmdset_0002.c | 55 +++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 25 deletions(-)