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

[Vignesh Raghavendra <vigneshr@ti.com>]

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.

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