Hi Ahmad-san,
On 2022/03/28 19:49, Ahmad Fatoum wrote:
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@xxxxxxxxx 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.
Thanks for the confirmation.
By the way could you please try to test the version 5 patches again?
Just did so for v7. Sorry for the delay.
Thank you so much for your help. Sorry I missed to cc on version 5 to
version 7 patches.
Regards,
Ikegami
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@xxxxxxxxxxxxxx/
Fixes: dfeae1073583("mtd: cfi_cmdset_0002: Change write buffer to check correct value")
Signed-off-by: Tokunori Ikegami <ikegami.t@xxxxxxxxx>
Tested-by: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
---
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
