On 22.03.22 03:49, Tokunori Ikegami wrote:
Hi Ahmad-san,
On 2022/03/17 23:16, Ahmad Fatoum wrote:
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.
Is this really caused the error on do_write_oneword by the changed? Actually it was changed to use chip_good instead of chip_ready. But before the change still do_write_oneword uses both chip_ready and chip_good. So it seems that it is possible to be caused the error before the change also.
Oh, I think you're right. Disregard my suggestion for the other Fixes: entry then.
By the way could you please try to test the version 5 patches again?
Just did so for v7. Sorry for the delay.
Cheers, Ahmad
Regards, Ikegami
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.d...
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