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. > 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@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 >>> >> > -- 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 |