[PATCH] mtd: nand: Wait for PAGEPROG to finish in drivers setting NAND_ECC_CUSTOM_PAGE_ACCESS

[Marc Gonzalez]

On 17/05/2017 12:51, Boris Brezillon wrote:

> On Wed, 17 May 2017 12:41:01 +0200
> Marc Gonzalez <marc_gonzalez at sigmadesigns.com> wrote:
> 
>> On 16/05/2017 18:27, Boris Brezillon wrote:
>>
>>> Drivers setting NAND_ECC_CUSTOM_PAGE_ACCESS are supposed to handle the
>>> full read/write page sequence, and waiting for a page to actually be
>>> programmed is part of this write-page sequence.
>>> This is also what is done in ->write_oob_xxx() hooks, so let's do that in  
>>> ->write_page_xxx() as well to make it consistent.  
>>>
>>> Signed-off-by: Boris Brezillon <boris.brezillon at free-electrons.com>
>>> ---
>>>  drivers/mtd/nand/atmel/nand-controller.c |  6 +++++-
>>>  drivers/mtd/nand/nand_base.c             | 10 ++++++----
>>>  drivers/mtd/nand/nand_micron.c           | 10 ++++++++--
>>>  drivers/mtd/nand/tango_nand.c            | 13 ++++++++++++-
>>>  4 files changed, 31 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>>> index 8dafd2a54e11..08ff98c47e1f 100644
>>> --- a/drivers/mtd/nand/nand_base.c
>>> +++ b/drivers/mtd/nand/nand_base.c
>>> @@ -2574,11 +2574,13 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>>>  	if (status < 0)
>>>  		return status;
>>>  
>>> -	if (nand_standard_page_accessors(&chip->ecc))
>>> +	if (nand_standard_page_accessors(&chip->ecc)) {
>>>  		chip->cmdfunc(mtd, NAND_CMD_PAGEPROG, -1, -1);
>>> -	status = chip->waitfunc(mtd, chip);
>>> -	if (status & NAND_STATUS_FAIL)
>>> -		return -EIO;
>>> +
>>> +		status = chip->waitfunc(mtd, chip);
>>> +		if (status & NAND_STATUS_FAIL)
>>> +			return -EIO;
>>> +	}  
>>
>> AFAIU, the wait operation used to be unconditional;
>> but it is now skipped for CUSTOM accessors. OK.
> 
> Yep.
> 
>>
>>
>>> diff --git a/drivers/mtd/nand/tango_nand.c b/drivers/mtd/nand/tango_nand.c
>>> index a2150b15d4c1..8498fa36e533 100644
>>> --- a/drivers/mtd/nand/tango_nand.c
>>> +++ b/drivers/mtd/nand/tango_nand.c
>>> @@ -295,7 +295,7 @@ static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>>>  			    const u8 *buf, int oob_required, int page)
>>>  {
>>>  	struct tango_nfc *nfc = to_tango_nfc(chip->controller);
>>> -	int err, len = mtd->writesize;
>>> +	int err, status, len = mtd->writesize;
>>>  
>>>  	/* Calling tango_write_oob() would send PAGEPROG twice */
>>>  	if (oob_required)
>>> @@ -306,6 +306,10 @@ static int tango_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>>>  	if (err)
>>>  		return err;
>>>  
>>> +	status = chip->waitfunc(mtd, chip);
>>> +	if (status & NAND_STATUS_FAIL)
>>> +		return -EIO;
>>> +
>>>  	return 0;
>>>  }  
>>
>> When I introduced the custom_accessors flag, I missed the removal
>> of this wait operation. The tango NFC is supposed to take care of
>> everything, from start to finish.
>>
>> I applied the following patch:
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -2675,6 +2675,9 @@ static int nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
>>         if (status < 0)
>>                 return status;
>>  
>> +       if (!nand_standard_page_accessors(&chip->ecc))
>> +               return 0;
>> +
>>
>> And ran mtd_speedtest. No measurable change.
>> mtd_stresstest did not detect any issues.
> 
> Okay. I think I'll keep the patch as is to avoid introducing extra
> functional changes.
> You can then send a patch removing the extra chip->waitfunc() from
> tango_write_page() and explain why this is unneeded in your commit
> message. Would that work for you?

Could you ping me when the patch hits linux-next?
Then I'll send a fixup.

Regards.