Re: [PATCH v4 2/3] mtd: cfi_cmdset_0002: Use chip_ready() for write on S29GL064N

[Tokunori Ikegami <ikegami.t@gmail.com>]

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