| Message ID | 20220315165607.390070-3-ikegami.t@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Vignesh R |
| Headers | show |
| Series | mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N | expand |
Hi Tokunori, ikegami.t@gmail.com wrote on Wed, 16 Mar 2022 01:56:06 +0900: > This is a preparation patch for the functinal change to fix the issue. functional Commits are independent "to fix the issue" does not make any sense here, please give more information like "in preparation to a change expected to fix the buffered writes on S29GL..." > > Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> > Cc: Miquel Raynal <miquel.raynal@bootlin.com> > Cc: Richard Weinberger <richard@nod.at> > Cc: Vignesh Raghavendra <vigneshr@ti.com> > Cc: linux-mtd@lists.infradead.org > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 82 +++++++++++++---------------- > 1 file changed, 38 insertions(+), 44 deletions(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index 0125658a1d30..8f3f0309dc03 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -802,22 +802,12 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd) > return NULL; > } > > -/* > - * Return true if the chip is ready. > - * > - * Ready is one of: read mode, query mode, erase-suspend-read mode (in any > - * non-suspended sector) and is indicated by no toggle bits toggling. > - * > - * Note that anything more complicated than checking if no bits are toggling > - * (including checking DQ5 for an error status) is tricky to get working > - * correctly and is therefore not done (particularly with interleaved chips > - * as each chip must be checked independently of the others). > - */ > -static int __xipram chip_ready(struct map_info *map, struct flchip *chip, > - unsigned long addr) > +static int __xipram chip_check(struct map_info *map, struct flchip *chip, > + unsigned long addr, map_word *expected) > { > struct cfi_private *cfi = map->fldrv_priv; > - map_word d, t; > + map_word oldd, curd; > + int ret; > > if (cfi_use_status_reg(cfi)) { > map_word ready = CMD(CFI_SR_DRB); > @@ -827,17 +817,35 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip, > */ > cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, > cfi->device_type, NULL); > - d = map_read(map, addr); > + curd = map_read(map, addr); > > - return map_word_andequal(map, d, ready, ready); > + return map_word_andequal(map, curd, ready, ready); > } > > - d = map_read(map, addr); > - t = map_read(map, addr); > + oldd = map_read(map, addr); > + curd = map_read(map, addr); > + > + ret = map_word_equal(map, oldd, curd); > > - return map_word_equal(map, d, t); > + if (!ret || !expected) > + return ret; > + > + return map_word_equal(map, curd, *expected); > } > > +/* > + * Return true if the chip is ready. > + * > + * Ready is one of: read mode, query mode, erase-suspend-read mode (in any > + * non-suspended sector) and is indicated by no toggle bits toggling. > + * > + * Note that anything more complicated than checking if no bits are toggling > + * (including checking DQ5 for an error status) is tricky to get working > + * correctly and is therefore not done (particularly with interleaved chips > + * as each chip must be checked independently of the others). > + */ > +#define chip_ready(map, chip, addr) chip_check(map, chip, addr, NULL) > + > /* > * Return true if the chip is ready and has the correct value. > * > @@ -856,28 +864,14 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip, > static int __xipram chip_good(struct map_info *map, struct flchip *chip, > unsigned long addr, map_word expected) > { > - struct cfi_private *cfi = map->fldrv_priv; > - map_word oldd, curd; > - > - if (cfi_use_status_reg(cfi)) { > - map_word ready = CMD(CFI_SR_DRB); > - > - /* > - * For chips that support status register, check device > - * ready bit > - */ > - cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, > - cfi->device_type, NULL); > - curd = map_read(map, addr); > - > - return map_word_andequal(map, curd, ready, ready); > - } > - > - oldd = map_read(map, addr); > - curd = map_read(map, addr); > + return chip_check(map, chip, addr, &expected); I must admit that chip_check, chip_good(), chip_ready() are rather poor names, at least they don't bring a lot of information. Is this part of a revert? If yes maybe it would be better to actually revert the patch if it not too complicated? Otherwise I don't really see the point of having chip_good() calling just chip_check()? And creating a macro for chip_ready()? > +} > > - return map_word_equal(map, oldd, curd) && > - map_word_equal(map, curd, expected); > +static int __xipram chip_good_for_write(struct map_info *map, > + struct flchip *chip, unsigned long addr, > + map_word expected) > +{ > + return chip_good(map, chip, addr, expected); > } > > static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode) > @@ -1700,7 +1694,7 @@ static int __xipram do_write_oneword_once(struct map_info *map, > * "chip_good" to avoid the failure due to scheduling. > */ > if (time_after(jiffies, timeo) && > - !chip_good(map, chip, adr, datum)) { > + !chip_good_for_write(map, chip, adr, datum)) { > xip_enable(map, chip, adr); > printk(KERN_WARNING "MTD %s(): software timeout\n", __func__); > xip_disable(map, chip, adr); > @@ -1708,7 +1702,7 @@ static int __xipram do_write_oneword_once(struct map_info *map, > break; > } > > - if (chip_good(map, chip, adr, datum)) { > + if (chip_good_for_write(map, chip, adr, datum)) { > if (cfi_check_err_status(map, chip, adr)) > ret = -EIO; > break; > @@ -1980,14 +1974,14 @@ static int __xipram do_write_buffer_wait(struct map_info *map, > * "chip_good" to avoid the failure due to scheduling. > */ > if (time_after(jiffies, timeo) && > - !chip_good(map, chip, adr, datum)) { > + !chip_good_for_write(map, chip, adr, datum)) { > pr_err("MTD %s(): software timeout, address:0x%.8lx.\n", > __func__, adr); > ret = -EIO; > break; > } > > - if (chip_good(map, chip, adr, datum)) { > + if (chip_good_for_write(map, chip, adr, datum)) { > if (cfi_check_err_status(map, chip, adr)) > ret = -EIO; > break; Thanks, Miquèl
Hi, On 2022/03/16 3:44, Miquel Raynal wrote: > Hi Tokunori, > > ikegami.t@gmail.com wrote on Wed, 16 Mar 2022 01:56:06 +0900: > >> This is a preparation patch for the functinal change to fix the issue. > functional > > Commits are independent "to fix the issue" does not make any sense > here, please give more information like "in preparation to a change > expected to fix the buffered writes on S29GL..." Fixed by the version 4 patches. > >> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> >> Cc: Miquel Raynal <miquel.raynal@bootlin.com> >> Cc: Richard Weinberger <richard@nod.at> >> Cc: Vignesh Raghavendra <vigneshr@ti.com> >> Cc: linux-mtd@lists.infradead.org >> --- >> drivers/mtd/chips/cfi_cmdset_0002.c | 82 +++++++++++++---------------- >> 1 file changed, 38 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c >> index 0125658a1d30..8f3f0309dc03 100644 >> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >> @@ -802,22 +802,12 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd) >> return NULL; >> } >> >> -/* >> - * Return true if the chip is ready. >> - * >> - * Ready is one of: read mode, query mode, erase-suspend-read mode (in any >> - * non-suspended sector) and is indicated by no toggle bits toggling. >> - * >> - * Note that anything more complicated than checking if no bits are toggling >> - * (including checking DQ5 for an error status) is tricky to get working >> - * correctly and is therefore not done (particularly with interleaved chips >> - * as each chip must be checked independently of the others). >> - */ >> -static int __xipram chip_ready(struct map_info *map, struct flchip *chip, >> - unsigned long addr) >> +static int __xipram chip_check(struct map_info *map, struct flchip *chip, >> + unsigned long addr, map_word *expected) >> { >> struct cfi_private *cfi = map->fldrv_priv; >> - map_word d, t; >> + map_word oldd, curd; >> + int ret; >> >> if (cfi_use_status_reg(cfi)) { >> map_word ready = CMD(CFI_SR_DRB); >> @@ -827,17 +817,35 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip, >> */ >> cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, >> cfi->device_type, NULL); >> - d = map_read(map, addr); >> + curd = map_read(map, addr); >> >> - return map_word_andequal(map, d, ready, ready); >> + return map_word_andequal(map, curd, ready, ready); >> } >> >> - d = map_read(map, addr); >> - t = map_read(map, addr); >> + oldd = map_read(map, addr); >> + curd = map_read(map, addr); >> + >> + ret = map_word_equal(map, oldd, curd); >> >> - return map_word_equal(map, d, t); >> + if (!ret || !expected) >> + return ret; >> + >> + return map_word_equal(map, curd, *expected); >> } >> >> +/* >> + * Return true if the chip is ready. >> + * >> + * Ready is one of: read mode, query mode, erase-suspend-read mode (in any >> + * non-suspended sector) and is indicated by no toggle bits toggling. >> + * >> + * Note that anything more complicated than checking if no bits are toggling >> + * (including checking DQ5 for an error status) is tricky to get working >> + * correctly and is therefore not done (particularly with interleaved chips >> + * as each chip must be checked independently of the others). >> + */ >> +#define chip_ready(map, chip, addr) chip_check(map, chip, addr, NULL) >> + >> /* >> * Return true if the chip is ready and has the correct value. >> * >> @@ -856,28 +864,14 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip, >> static int __xipram chip_good(struct map_info *map, struct flchip *chip, >> unsigned long addr, map_word expected) >> { >> - struct cfi_private *cfi = map->fldrv_priv; >> - map_word oldd, curd; >> - >> - if (cfi_use_status_reg(cfi)) { >> - map_word ready = CMD(CFI_SR_DRB); >> - >> - /* >> - * For chips that support status register, check device >> - * ready bit >> - */ >> - cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, >> - cfi->device_type, NULL); >> - curd = map_read(map, addr); >> - >> - return map_word_andequal(map, curd, ready, ready); >> - } >> - >> - oldd = map_read(map, addr); >> - curd = map_read(map, addr); >> + return chip_check(map, chip, addr, &expected); > I must admit that chip_check, chip_good(), chip_ready() are rather poor > names, at least they don't bring a lot of information. Is this part of a > revert? If yes maybe it would be better to actually revert the patch if > it not too complicated? Otherwise I don't really see the point of No this is not for the revert. > having chip_good() calling just chip_check()? And creating a macro for > chip_ready()? Changed chip_good by the version 4 patches as a macro but not a function as same with the chip_ready. > >> +} >> >> - return map_word_equal(map, oldd, curd) && >> - map_word_equal(map, curd, expected); >> +static int __xipram chip_good_for_write(struct map_info *map, >> + struct flchip *chip, unsigned long addr, >> + map_word expected) >> +{ >> + return chip_good(map, chip, addr, expected); >> } >> >> static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode) >> @@ -1700,7 +1694,7 @@ static int __xipram do_write_oneword_once(struct map_info *map, >> * "chip_good" to avoid the failure due to scheduling. >> */ >> if (time_after(jiffies, timeo) && >> - !chip_good(map, chip, adr, datum)) { >> + !chip_good_for_write(map, chip, adr, datum)) { >> xip_enable(map, chip, adr); >> printk(KERN_WARNING "MTD %s(): software timeout\n", __func__); >> xip_disable(map, chip, adr); >> @@ -1708,7 +1702,7 @@ static int __xipram do_write_oneword_once(struct map_info *map, >> break; >> } >> >> - if (chip_good(map, chip, adr, datum)) { >> + if (chip_good_for_write(map, chip, adr, datum)) { >> if (cfi_check_err_status(map, chip, adr)) >> ret = -EIO; >> break; >> @@ -1980,14 +1974,14 @@ static int __xipram do_write_buffer_wait(struct map_info *map, >> * "chip_good" to avoid the failure due to scheduling. >> */ >> if (time_after(jiffies, timeo) && >> - !chip_good(map, chip, adr, datum)) { >> + !chip_good_for_write(map, chip, adr, datum)) { >> pr_err("MTD %s(): software timeout, address:0x%.8lx.\n", >> __func__, adr); >> ret = -EIO; >> break; >> } >> >> - if (chip_good(map, chip, adr, datum)) { >> + if (chip_good_for_write(map, chip, adr, datum)) { >> if (cfi_check_err_status(map, chip, adr)) >> ret = -EIO; >> break; > > Thanks, > Miquèl
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index 0125658a1d30..8f3f0309dc03 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -802,22 +802,12 @@ static struct mtd_info *cfi_amdstd_setup(struct mtd_info *mtd) return NULL; } -/* - * Return true if the chip is ready. - * - * Ready is one of: read mode, query mode, erase-suspend-read mode (in any - * non-suspended sector) and is indicated by no toggle bits toggling. - * - * Note that anything more complicated than checking if no bits are toggling - * (including checking DQ5 for an error status) is tricky to get working - * correctly and is therefore not done (particularly with interleaved chips - * as each chip must be checked independently of the others). - */ -static int __xipram chip_ready(struct map_info *map, struct flchip *chip, - unsigned long addr) +static int __xipram chip_check(struct map_info *map, struct flchip *chip, + unsigned long addr, map_word *expected) { struct cfi_private *cfi = map->fldrv_priv; - map_word d, t; + map_word oldd, curd; + int ret; if (cfi_use_status_reg(cfi)) { map_word ready = CMD(CFI_SR_DRB); @@ -827,17 +817,35 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip, */ cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, cfi->device_type, NULL); - d = map_read(map, addr); + curd = map_read(map, addr); - return map_word_andequal(map, d, ready, ready); + return map_word_andequal(map, curd, ready, ready); } - d = map_read(map, addr); - t = map_read(map, addr); + oldd = map_read(map, addr); + curd = map_read(map, addr); + + ret = map_word_equal(map, oldd, curd); - return map_word_equal(map, d, t); + if (!ret || !expected) + return ret; + + return map_word_equal(map, curd, *expected); } +/* + * Return true if the chip is ready. + * + * Ready is one of: read mode, query mode, erase-suspend-read mode (in any + * non-suspended sector) and is indicated by no toggle bits toggling. + * + * Note that anything more complicated than checking if no bits are toggling + * (including checking DQ5 for an error status) is tricky to get working + * correctly and is therefore not done (particularly with interleaved chips + * as each chip must be checked independently of the others). + */ +#define chip_ready(map, chip, addr) chip_check(map, chip, addr, NULL) + /* * Return true if the chip is ready and has the correct value. * @@ -856,28 +864,14 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip, static int __xipram chip_good(struct map_info *map, struct flchip *chip, unsigned long addr, map_word expected) { - struct cfi_private *cfi = map->fldrv_priv; - map_word oldd, curd; - - if (cfi_use_status_reg(cfi)) { - map_word ready = CMD(CFI_SR_DRB); - - /* - * For chips that support status register, check device - * ready bit - */ - cfi_send_gen_cmd(0x70, cfi->addr_unlock1, chip->start, map, cfi, - cfi->device_type, NULL); - curd = map_read(map, addr); - - return map_word_andequal(map, curd, ready, ready); - } - - oldd = map_read(map, addr); - curd = map_read(map, addr); + return chip_check(map, chip, addr, &expected); +} - return map_word_equal(map, oldd, curd) && - map_word_equal(map, curd, expected); +static int __xipram chip_good_for_write(struct map_info *map, + struct flchip *chip, unsigned long addr, + map_word expected) +{ + return chip_good(map, chip, addr, expected); } static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode) @@ -1700,7 +1694,7 @@ static int __xipram do_write_oneword_once(struct map_info *map, * "chip_good" to avoid the failure due to scheduling. */ if (time_after(jiffies, timeo) && - !chip_good(map, chip, adr, datum)) { + !chip_good_for_write(map, chip, adr, datum)) { xip_enable(map, chip, adr); printk(KERN_WARNING "MTD %s(): software timeout\n", __func__); xip_disable(map, chip, adr); @@ -1708,7 +1702,7 @@ static int __xipram do_write_oneword_once(struct map_info *map, break; } - if (chip_good(map, chip, adr, datum)) { + if (chip_good_for_write(map, chip, adr, datum)) { if (cfi_check_err_status(map, chip, adr)) ret = -EIO; break; @@ -1980,14 +1974,14 @@ static int __xipram do_write_buffer_wait(struct map_info *map, * "chip_good" to avoid the failure due to scheduling. */ if (time_after(jiffies, timeo) && - !chip_good(map, chip, adr, datum)) { + !chip_good_for_write(map, chip, adr, datum)) { pr_err("MTD %s(): software timeout, address:0x%.8lx.\n", __func__, adr); ret = -EIO; break; } - if (chip_good(map, chip, adr, datum)) { + if (chip_good_for_write(map, chip, adr, datum)) { if (cfi_check_err_status(map, chip, adr)) ret = -EIO; break;
This is a preparation patch for the functinal change to fix the issue. Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> Cc: Miquel Raynal <miquel.raynal@bootlin.com> Cc: Richard Weinberger <richard@nod.at> Cc: Vignesh Raghavendra <vigneshr@ti.com> Cc: linux-mtd@lists.infradead.org --- drivers/mtd/chips/cfi_cmdset_0002.c | 82 +++++++++++++---------------- 1 file changed, 38 insertions(+), 44 deletions(-)