| Message ID | 20220316155455.162362-2-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 Thu, 17 Mar 2022 00:54:53 +0900: > This is a preparation patch for the functional change in preparation to a change > expected to fix the buffered writes on S29GL064N. This is a preparation patch for the S29GL064N buffer writes fix. There is no functional change. > > Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") > Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> > Cc: stable@vger.kernel.org > --- > drivers/mtd/chips/cfi_cmdset_0002.c | 77 ++++++++++++----------------- > 1 file changed, 32 insertions(+), 45 deletions(-) > > diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c > index a761134fd3be..e68ddf0f7fc0 100644 > --- a/drivers/mtd/chips/cfi_cmdset_0002.c > +++ b/drivers/mtd/chips/cfi_cmdset_0002.c > @@ -801,22 +801,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); > @@ -826,17 +816,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); A lot of the diff is just a rename. I am not against a rename if you feel it's better, but in this order: 1: prepare the fix 2: fix 3: rename/define id's, whatever > } > > - 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) I don't see the point of such a define. Just get rid of it. > + > /* > * Return true if the chip is ready and has the correct value. > * > @@ -852,32 +860,11 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip, > * as each chip must be checked independently of the others). > * > */ > -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 map_word_equal(map, oldd, curd) && > - map_word_equal(map, curd, expected); > -} > +#define chip_good(map, chip, addr, expected) \ > + ({ \ > + map_word datum = expected; \ > + chip_check(map, chip, addr, &datum); \ > + }) What is this for? Same here, I don't see the point. Please get rid of all these unnecessary helpers which do nothing, besides complicating user's life. > static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode) > { Thanks, Miquèl
Hi Miquèl-san, On 2022/03/17 2:15, Miquel Raynal wrote: > Hi Tokunori, > > ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:53 +0900: > >> This is a preparation patch for the functional change in preparation to a change >> expected to fix the buffered writes on S29GL064N. > This is a preparation patch for the S29GL064N buffer writes fix. There > is no functional change. Fixed this by the version 5 patch. > >> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") >> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> >> Cc: stable@vger.kernel.org >> --- >> drivers/mtd/chips/cfi_cmdset_0002.c | 77 ++++++++++++----------------- >> 1 file changed, 32 insertions(+), 45 deletions(-) >> >> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c >> index a761134fd3be..e68ddf0f7fc0 100644 >> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >> @@ -801,22 +801,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); >> @@ -826,17 +816,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); > A lot of the diff is just a rename. I am not against a rename if you > feel it's better, but in this order: > 1: prepare the fix > 2: fix > 3: rename/define id's, whatever This is also fixed as same. > >> } >> >> - 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) > I don't see the point of such a define. Just get rid of it. Yes deleted the macro as changed the name chip_check to chip_ready and to use only chip_ready without the macro. > >> + >> /* >> * Return true if the chip is ready and has the correct value. >> * >> @@ -852,32 +860,11 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip, >> * as each chip must be checked independently of the others). >> * >> */ >> -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 map_word_equal(map, oldd, curd) && >> - map_word_equal(map, curd, expected); >> -} >> +#define chip_good(map, chip, addr, expected) \ >> + ({ \ >> + map_word datum = expected; \ >> + chip_check(map, chip, addr, &datum); \ >> + }) > What is this for? Same here, I don't see the point. Please get rid of > all these unnecessary helpers which do nothing, besides complicating > user's life. Fixed as same. Regards, Ikegami > >> static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode) >> { > > Thanks, > Miquèl
diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c index a761134fd3be..e68ddf0f7fc0 100644 --- a/drivers/mtd/chips/cfi_cmdset_0002.c +++ b/drivers/mtd/chips/cfi_cmdset_0002.c @@ -801,22 +801,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); @@ -826,17 +816,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. * @@ -852,32 +860,11 @@ static int __xipram chip_ready(struct map_info *map, struct flchip *chip, * as each chip must be checked independently of the others). * */ -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 map_word_equal(map, oldd, curd) && - map_word_equal(map, curd, expected); -} +#define chip_good(map, chip, addr, expected) \ + ({ \ + map_word datum = expected; \ + chip_check(map, chip, addr, &datum); \ + }) static int get_chip(struct map_info *map, struct flchip *chip, unsigned long adr, int mode) {
This is a preparation patch for the functional change in preparation to a change expected to fix the buffered writes on S29GL064N. Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> Cc: stable@vger.kernel.org --- drivers/mtd/chips/cfi_cmdset_0002.c | 77 ++++++++++++----------------- 1 file changed, 32 insertions(+), 45 deletions(-)