From: Ahmad Fatoum <a.fatoum@pengutronix.de> To: Vignesh Raghavendra <vigneshr@ti.com>, Miquel Raynal <miquel.raynal@bootlin.com>, Tokunori Ikegami <ikegami.t@gmail.com> Cc: linux-mtd@lists.infradead.org, stable@vger.kernel.org Subject: Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N Date: Thu, 17 Mar 2022 15:16:17 +0100 [thread overview] Message-ID: <201907b1-c43a-8c45-7ab1-4a4606591bef@pengutronix.de> (raw) In-Reply-To: <01fed0aa-8844-1db9-f167-e7e7944bc092@ti.com> Hello Vignesh, On 17.03.22 11:01, Vignesh Raghavendra wrote: > > > On 16/03/22 10:51 pm, Miquel Raynal wrote: >> Hi Tokunori, >> >> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900: >> >>> As pointed out by this bug report [1], buffered writes are now broken on >>> S29GL064N. This issue comes from a rework which switched from using chip_good() >>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error >>> returned by chip_good(). >> >> Vignesh, I believe you understand this issue better than I do, can you >> propose an improved commit log? > > How about: > > Since commit dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to > check correct value") buffered writes fail on S29GL064N. This is > because, on S29GL064N, reads return 0xFF at the end of DQ polling for > write completion, where as, chip_good() check expects actual data > written to the last location to be returned post DQ polling completion. > Fix is to revert to using chip_good() for S29GL064N which only checks > for DQ lines to settle down to determine write completion. Message sounds good to me with one remark: The issue is independent of whether buffered writes are used or not. It's just because buffered writes are the default, that it was broken by dfeae1073583 ("mtd: cfi_cmdset_0002: Change write buffer to check correct value"). The word write case was broken by 37c673ade35c ("mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()"), so the commit message should probably reference both. as this commit indeed fixes both FORCE_WORD_WRITE == 0 and == 1. Thanks, Ahmad > >> >>> One way to solve the issue is to revert the change >>> partially to use chip_ready for S29GL064N. >>> >>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/ >>> >>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") >>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> >>> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >>> Cc: stable@vger.kernel.org >>> --- >>> drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++---- >>> 1 file changed, 21 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c >>> index e68ddf0f7fc0..6c57f85e1b8e 100644 >>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >>> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip, >>> chip_check(map, chip, addr, &datum); \ >>> }) >>> >>> +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map) >> >> At the very least I would call this function: >> cfi_use_chip_ready_for_writes() >> >> Yet, I still don't fully get what chip_ready is versus chip_good. >> >>> +{ >>> + struct cfi_private *cfi = map->fldrv_priv; >>> + >>> + return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01; >>> +} >>> + >>> +static int __xipram chip_good_for_write(struct map_info *map, >>> + struct flchip *chip, unsigned long addr, >>> + map_word expected) >>> +{ >>> + if (cfi_use_chip_ready_for_write(map)) >>> + return chip_ready(map, chip, addr); >> >> If possible and not too invasive I would definitely add a "quirks" flag >> somewhere instead of this cfi_use_chip_ready_for_write() check. >> >> Anyway, I would move this to the chip_good() implementation directly so >> we partially hide the quirks complexity from the core. > > Yeah, unfortunately this driver does not use quirk flags and tends to > hide quirks behind bool functions like above > > Regards > Vignesh > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
WARNING: multiple messages have this Message-ID (diff)
From: Ahmad Fatoum <a.fatoum@pengutronix.de> To: Vignesh Raghavendra <vigneshr@ti.com>, Miquel Raynal <miquel.raynal@bootlin.com>, Tokunori Ikegami <ikegami.t@gmail.com> Cc: linux-mtd@lists.infradead.org, stable@vger.kernel.org Subject: Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N Date: Thu, 17 Mar 2022 15:16:17 +0100 [thread overview] Message-ID: <201907b1-c43a-8c45-7ab1-4a4606591bef@pengutronix.de> (raw) In-Reply-To: <01fed0aa-8844-1db9-f167-e7e7944bc092@ti.com> Hello Vignesh, On 17.03.22 11:01, Vignesh Raghavendra wrote: > > > On 16/03/22 10:51 pm, Miquel Raynal wrote: >> Hi Tokunori, >> >> ikegami.t@gmail.com wrote on Thu, 17 Mar 2022 00:54:54 +0900: >> >>> As pointed out by this bug report [1], buffered writes are now broken on >>> S29GL064N. This issue comes from a rework which switched from using chip_good() >>> to chip_ready(), because DQ true data 0xFF is read on S29GL064N and an error >>> returned by chip_good(). >> >> Vignesh, I believe you understand this issue better than I do, can you >> propose an improved commit log? > > How about: > > Since commit dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to > check correct value") buffered writes fail on S29GL064N. This is > because, on S29GL064N, reads return 0xFF at the end of DQ polling for > write completion, where as, chip_good() check expects actual data > written to the last location to be returned post DQ polling completion. > Fix is to revert to using chip_good() for S29GL064N which only checks > for DQ lines to settle down to determine write completion. Message sounds good to me with one remark: The issue is independent of whether buffered writes are used or not. It's just because buffered writes are the default, that it was broken by dfeae1073583 ("mtd: cfi_cmdset_0002: Change write buffer to check correct value"). The word write case was broken by 37c673ade35c ("mtd: cfi_cmdset_0002: Use chip_good() to retry in do_write_oneword()"), so the commit message should probably reference both. as this commit indeed fixes both FORCE_WORD_WRITE == 0 and == 1. Thanks, Ahmad > >> >>> One way to solve the issue is to revert the change >>> partially to use chip_ready for S29GL064N. >>> >>> [1] https://lore.kernel.org/r/b687c259-6413-26c9-d4c9-b3afa69ea124@pengutronix.de/ >>> >>> Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value") >>> Signed-off-by: Tokunori Ikegami <ikegami.t@gmail.com> >>> Tested-by: Ahmad Fatoum <a.fatoum@pengutronix.de> >>> Cc: stable@vger.kernel.org >>> --- >>> drivers/mtd/chips/cfi_cmdset_0002.c | 25 +++++++++++++++++++++---- >>> 1 file changed, 21 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/mtd/chips/cfi_cmdset_0002.c b/drivers/mtd/chips/cfi_cmdset_0002.c >>> index e68ddf0f7fc0..6c57f85e1b8e 100644 >>> --- a/drivers/mtd/chips/cfi_cmdset_0002.c >>> +++ b/drivers/mtd/chips/cfi_cmdset_0002.c >>> @@ -866,6 +866,23 @@ static int __xipram chip_check(struct map_info *map, struct flchip *chip, >>> chip_check(map, chip, addr, &datum); \ >>> }) >>> >>> +static bool __xipram cfi_use_chip_ready_for_write(struct map_info *map) >> >> At the very least I would call this function: >> cfi_use_chip_ready_for_writes() >> >> Yet, I still don't fully get what chip_ready is versus chip_good. >> >>> +{ >>> + struct cfi_private *cfi = map->fldrv_priv; >>> + >>> + return cfi->mfr == CFI_MFR_AMD && cfi->id == 0x0c01; >>> +} >>> + >>> +static int __xipram chip_good_for_write(struct map_info *map, >>> + struct flchip *chip, unsigned long addr, >>> + map_word expected) >>> +{ >>> + if (cfi_use_chip_ready_for_write(map)) >>> + return chip_ready(map, chip, addr); >> >> If possible and not too invasive I would definitely add a "quirks" flag >> somewhere instead of this cfi_use_chip_ready_for_write() check. >> >> Anyway, I would move this to the chip_good() implementation directly so >> we partially hide the quirks complexity from the core. > > Yeah, unfortunately this driver does not use quirk flags and tends to > hide quirks behind bool functions like above > > Regards > Vignesh > -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2022-03-17 14:16 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 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 [this message] 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=201907b1-c43a-8c45-7ab1-4a4606591bef@pengutronix.de \ --to=a.fatoum@pengutronix.de \ --cc=ikegami.t@gmail.com \ --cc=linux-mtd@lists.infradead.org \ --cc=miquel.raynal@bootlin.com \ --cc=stable@vger.kernel.org \ --cc=vigneshr@ti.com \ /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.