From: Tokunori Ikegami <ikegami.t@gmail.com> To: Miquel Raynal <miquel.raynal@bootlin.com> Cc: linux-mtd@lists.infradead.org, stable@vger.kernel.org Subject: Re: [PATCH v4 1/3] mtd: cfi_cmdset_0002: Move and rename chip_check/chip_ready/chip_good_for_write Date: Tue, 22 Mar 2022 11:35:30 +0900 [thread overview] Message-ID: <22f5c9f7-2fbd-902b-ca9b-c14d92a9b045@gmail.com> (raw) In-Reply-To: <20220316181519.0ec5bc97@xps13> 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
WARNING: multiple messages have this Message-ID (diff)
From: Tokunori Ikegami <ikegami.t@gmail.com> To: Miquel Raynal <miquel.raynal@bootlin.com> Cc: linux-mtd@lists.infradead.org, stable@vger.kernel.org Subject: Re: [PATCH v4 1/3] mtd: cfi_cmdset_0002: Move and rename chip_check/chip_ready/chip_good_for_write Date: Tue, 22 Mar 2022 11:35:30 +0900 [thread overview] Message-ID: <22f5c9f7-2fbd-902b-ca9b-c14d92a9b045@gmail.com> (raw) In-Reply-To: <20220316181519.0ec5bc97@xps13> 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2022-03-22 2:35 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-03-16 15:54 [PATCH v4 0/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N Tokunori Ikegami 2022-03-16 15:54 ` Tokunori Ikegami 2022-03-16 15:54 ` [PATCH v4 1/3] mtd: cfi_cmdset_0002: Move and rename chip_check/chip_ready/chip_good_for_write Tokunori Ikegami 2022-03-16 15:54 ` Tokunori Ikegami 2022-03-16 17:15 ` Miquel Raynal 2022-03-16 17:15 ` Miquel Raynal 2022-03-22 2:35 ` Tokunori Ikegami [this message] 2022-03-22 2:35 ` Tokunori Ikegami 2022-03-16 15:54 ` [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N Tokunori Ikegami 2022-03-16 15:54 ` Tokunori Ikegami 2022-03-16 17:21 ` Miquel Raynal 2022-03-16 17:21 ` Miquel Raynal 2022-03-17 10:01 ` Vignesh Raghavendra 2022-03-17 10:01 ` Vignesh Raghavendra 2022-03-17 14:16 ` Ahmad Fatoum 2022-03-17 14:16 ` Ahmad Fatoum 2022-03-22 2:49 ` Tokunori Ikegami 2022-03-22 2:49 ` Tokunori Ikegami 2022-03-28 10:49 ` Ahmad Fatoum 2022-03-28 10:49 ` Ahmad Fatoum 2022-03-28 15:27 ` Tokunori Ikegami 2022-03-28 15:27 ` Tokunori Ikegami 2022-03-22 2:42 ` Tokunori Ikegami 2022-03-22 2:42 ` Tokunori Ikegami 2022-03-22 2:39 ` Tokunori Ikegami 2022-03-22 2:39 ` Tokunori Ikegami 2022-03-21 11:48 ` Thorsten Leemhuis 2022-03-21 11:48 ` Thorsten Leemhuis 2022-03-21 12:35 ` Miquel Raynal 2022-03-21 12:35 ` Miquel Raynal 2022-03-21 12:51 ` Thorsten Leemhuis 2022-03-21 12:51 ` Thorsten Leemhuis 2022-03-21 13:41 ` Miquel Raynal 2022-03-21 13:41 ` Miquel Raynal 2022-03-21 14:17 ` Thorsten Leemhuis 2022-03-21 14:17 ` Thorsten Leemhuis 2022-03-21 14:56 ` Miquel Raynal 2022-03-21 14:56 ` Miquel Raynal 2022-03-21 15:16 ` Thorsten Leemhuis 2022-03-21 15:16 ` Thorsten Leemhuis 2022-03-22 2:51 ` Tokunori Ikegami 2022-03-22 2:51 ` Tokunori Ikegami 2022-03-16 15:54 ` [PATCH v4 3/3] mtd: cfi_cmdset_0002: Add S29GL064N ID definition Tokunori Ikegami 2022-03-16 17:27 ` [PATCH v4 0/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N Miquel Raynal 2022-03-16 17:27 ` Miquel Raynal
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=22f5c9f7-2fbd-902b-ca9b-c14d92a9b045@gmail.com \ --to=ikegami.t@gmail.com \ --cc=linux-mtd@lists.infradead.org \ --cc=miquel.raynal@bootlin.com \ --cc=stable@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.