* [PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver @ 2019-10-03 18:29 Sergei Shtylyov 2019-10-03 18:32 ` [PATCH 1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status() Sergei Shtylyov 2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov 0 siblings, 2 replies; 9+ messages in thread From: Sergei Shtylyov @ 2019-10-03 18:29 UTC (permalink / raw) To: David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, linux-mtd Hello! Here's a set of 2 patches against the current Linus' tree (the 'mtd/fixes' branch of the MTD repo is outdated). Linux 5.4-rc1 received initial support for the HyperFlash. However, the patch adding the status register polling to the AMD/Fujitsu/Spansion CFI driver has couple issues which I'm trying to solve here... [1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status() [2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash MBR, Sergei ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status() 2019-10-03 18:29 [PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver Sergei Shtylyov @ 2019-10-03 18:32 ` Sergei Shtylyov 2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov 1 sibling, 0 replies; 9+ messages in thread From: Sergei Shtylyov @ 2019-10-03 18:32 UTC (permalink / raw) To: David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, linux-mtd Cypress S26K{L|S}P{128|256|512}S datasheet says that the error bits in the status register are only valid when the "device ready" bit 7 is set. Add the check for the device ready bit in cfi_check_err_status() as that function isn't always called with this bit set. 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 | 4 ++++ 1 file changed, 4 insertions(+) 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 @@ -136,6 +136,10 @@ static void cfi_check_err_status(struct cfi->device_type, NULL); status = map_read(map, adr); + /* The error bits are invalid while the chip's busy */ + if (!map_word_bitsset(map, status, CMD(CFI_SR_DRB))) + return; + if (map_word_bitsset(map, status, CMD(0x3a))) { unsigned long chipstatus = MERGESTATUS(status); ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash 2019-10-03 18:29 [PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver Sergei Shtylyov 2019-10-03 18:32 ` [PATCH 1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status() Sergei Shtylyov @ 2019-10-03 18:34 ` Sergei Shtylyov 2019-10-06 20:54 ` Tokunori Ikegami 2019-10-16 6:33 ` Vignesh Raghavendra 1 sibling, 2 replies; 9+ messages in thread From: Sergei Shtylyov @ 2019-10-03 18:34 UTC (permalink / raw) To: David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, linux-mtd 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; } + 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 */ ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash 2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov @ 2019-10-06 20:54 ` Tokunori Ikegami 2019-10-11 12:03 ` Sergei Shtylyov 2019-10-16 6:33 ` Vignesh Raghavendra 1 sibling, 1 reply; 9+ messages in thread From: Tokunori Ikegami @ 2019-10-06 20:54 UTC (permalink / raw) To: Sergei Shtylyov, David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, linux-mtd 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/ > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash 2019-10-06 20:54 ` Tokunori Ikegami @ 2019-10-11 12:03 ` Sergei Shtylyov 2019-10-13 3:35 ` Tokunori Ikegami 0 siblings, 1 reply; 9+ messages in thread From: Sergei Shtylyov @ 2019-10-11 12:03 UTC (permalink / raw) To: Tokunori Ikegami, David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash 2019-10-11 12:03 ` Sergei Shtylyov @ 2019-10-13 3:35 ` Tokunori Ikegami 0 siblings, 0 replies; 9+ messages in thread From: Tokunori Ikegami @ 2019-10-13 3:35 UTC (permalink / raw) To: Sergei Shtylyov, David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal, Richard Weinberger, Vignesh Raghavendra, linux-mtd 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash 2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov 2019-10-06 20:54 ` Tokunori Ikegami @ 2019-10-16 6:33 ` Vignesh Raghavendra 2019-10-28 19:15 ` Sergei Shtylyov 1 sibling, 1 reply; 9+ messages in thread From: Vignesh Raghavendra @ 2019-10-16 6:33 UTC (permalink / raw) To: Sergei Shtylyov, David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal, Richard Weinberger, linux-mtd 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! -- Regards Vignesh ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash 2019-10-16 6:33 ` Vignesh Raghavendra @ 2019-10-28 19:15 ` Sergei Shtylyov 2019-10-30 15:35 ` Vignesh Raghavendra 0 siblings, 1 reply; 9+ messages in thread From: Sergei Shtylyov @ 2019-10-28 19:15 UTC (permalink / raw) To: Vignesh Raghavendra, David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal, Richard Weinberger, linux-mtd 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash 2019-10-28 19:15 ` Sergei Shtylyov @ 2019-10-30 15:35 ` Vignesh Raghavendra 0 siblings, 0 replies; 9+ messages in thread From: Vignesh Raghavendra @ 2019-10-30 15:35 UTC (permalink / raw) To: Sergei Shtylyov, David Woodhouse, Brian Norris, Marek Vasut, Miquel Raynal, Richard Weinberger, linux-mtd 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 > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-10-30 15:36 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-10-03 18:29 [PATCH 0/2] Fix the HyperFlash support in the AMD/Fujitsu/Spansion CFI driver Sergei Shtylyov 2019-10-03 18:32 ` [PATCH 1/2] mtd: cfi_cmdset_0002: only check errors when ready in cfi_check_err_status() Sergei Shtylyov 2019-10-03 18:34 ` [PATCH 2/2] mtd: cfi_cmdset_0002: fix delayed error detection on HyperFlash Sergei Shtylyov 2019-10-06 20:54 ` Tokunori Ikegami 2019-10-11 12:03 ` Sergei Shtylyov 2019-10-13 3:35 ` Tokunori Ikegami 2019-10-16 6:33 ` Vignesh Raghavendra 2019-10-28 19:15 ` Sergei Shtylyov 2019-10-30 15:35 ` Vignesh Raghavendra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).