[2/3] mtd: rawnand: micron: Disable ECC earlier in the read path

[Chris.Packham@xxxxxxxxxxxxxxxxxxx (Chris Packham)]

Hi Boris,

On 17/07/18 23:42, Boris Brezillon wrote:
> On Mon, 16 Jul 2018 22:42:54 +0000
> Chris Packham <Chris.Packham at alliedtelesis.co.nz> wrote:
> 
>> On 17/07/18 08:55, Boris Brezillon wrote:
>>> On Mon, 16 Jul 2018 18:10:57 +0200
>>> Boris Brezillon <boris.brezillon at bootlin.com> wrote:
>>>    
>>>> On Mon, 16 Jul 2018 09:00:59 +0000
>>>> Chris Packham <Chris.Packham at alliedtelesis.co.nz> wrote:
>>>>   
>>>>> Hi Boris,
>>>>>
>>>>> On 04/07/18 00:20, Boris Brezillon wrote:
>>>>>> We are about to support extracting the real number of bitflips for
>>>>>> 4-bits ECC when WRITE_RECOMMEND is returned. This requires re-reading
>>>>>> the page in raw mode to compare it to the corrected version, and this
>>>>>> logic will be placed in micron_nand_on_die_ecc_status_4().
>>>>>>
>>>>>> Moving the micron_nand_on_die_ecc_setup() will allow us to disable
>>>>>> ECC only once.
>>>>>>
>>>>>> As a result, we have to rework the exit path and add an error path
>>>>>> where the ECC is disabled.
>>>>>>
>>>>>> Signed-off-by: Boris Brezillon <boris.brezillon at bootlin.com>
>>>>>
>>>>> As I said on the other thread this appears to cause a problem for me on
>>>>> the MT29F1G08ABAFAWP-ITE setup I have. I notice we're not able to find
>>>>> the BBT, not sure if that is symptom or cause.
>>>
>>> It's most likely the symptom, not the cause.
>>>    
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
>>>>>> index 63ac98a36ed7..b9cbaf125a98 100644
>>>>>> --- a/drivers/mtd/nand/raw/nand_micron.c
>>>>>> +++ b/drivers/mtd/nand/raw/nand_micron.c
>>>>>> @@ -197,30 +197,37 @@ micron_nand_read_page_on_die_ecc(struct mtd_info *mtd, struct nand_chip *chip,
>>>>>>     
>>>>>>     	ret = nand_read_page_op(chip, page, 0, NULL, 0);
>>>>>>     	if (ret)
>>>>>> -		goto out;
>>>>>> +		goto err_disable_ecc;
>>>>>>     
>>>>>>     	ret = nand_status_op(chip, &status);
>>>>>>     	if (ret)
>>>>>> -		goto out;
>>>>>> +		goto err_disable_ecc;
>>>>>>     
>>>>>>     	ret = nand_exit_status_op(chip);
>>>>>>     	if (ret)
>>>>>> -		goto out;
>>>>>> +		goto err_disable_ecc;
>>>>>>     
>>>>>> -	if (chip->ecc.strength == 4)
>>>>>> -		max_bitflips = micron_nand_on_die_ecc_status_4(chip, status);
>>>>>> -	else
>>>>>> -		max_bitflips = micron_nand_on_die_ecc_status_8(chip, status);
>>>>>> +	micron_nand_on_die_ecc_setup(chip, false);
>>>>
>>>> Hm, can you try to move the micron_nand_on_die_ecc_setup(chip, false)
>>>> call just before nand_exit_status_op()?
>>>>   
>>>
>>> Just pushed a branch fixing that [1]. Can you test it? If it works,
>>> I'll ask Miquel to drop the initial set of patches and instead pick the
>>> fixed ones so that we don't break bisectibility.
>>>
>>> [1]https://github.com/bbrezillon/linux-0day/commits/nand/next
>>>    
>>
>> Still appears to have the same problem.
>>
>> I'm guessing that since you can't actually disable ecc on this chip
>> calling micron_nand_on_die_ecc_setup(chip, false); before reading the
>> oob data interferes with it somehow (if I call it after there is no
>> problem).
>>
>> We could add code to qualify the attempt to disable ecc early based on
>> it being optional/mandatory or just stick with it being disabled late.
> 
> Okay. I gave up on disabling ECC earlier and instead implemented what
> you suggested. Can you test
> https://github.com/bbrezillon/linux-0day/commits/nand/next again and
> let me know if it works for you?

Yes that works for me. Feel free to add

Tested-by: Chris Packham <chris.packham at alliedtelesis.co.nz>

to

88240e244026 mtd: rawnand: micron: Make ECC activation stateful
6ae7f205cec0 mtd: rawnand: micron: Avoid enabling/disabling ECC when it 
can't be disabled