Re: [RFC PATCH 0/2] mtd: rawnand: support MT29F1G08ABAFAWP-ITE:F

[Chris Packham]

Adding participants from 
http://lists.infradead.org/pipermail/linux-mtd/2017-March/072974.html

On 19/06/18 16:56, Boris Brezillon wrote:

> Hi Chris, >
> On Tue, 19 Jun 2018 01:44:24 +0000
> Chris Packham <Chris.Packham@xxxxxxxxxxxxxxxxxxx> wrote:
> 
>> On 19/06/18 12:35, Chris Packham wrote:
>>> On 19/06/18 01:15, Miquel Raynal wrote:
>>>> Hi Chris,
>>>>
>>>> On Mon, 18 Jun 2018 16:52:53 +1200, Chris Packham
>>>> <chris.packham@xxxxxxxxxxxxxxxxxxx> wrote:
>>>>   
>>>>> Hi,
>>>>>
>>>>> I'm looking at adding support for the Micron MT29F1G08ABAFAWP-ITE:F chip
>>>>> to one of our boards which uses the Marvell NFCv2 controller.
>>>>>
>>>>> This particular chip is a bit odd in that the datasheet states support
>>>>> for ONFI 1.0 but the revision number field is 00 00. It also is marked
>>>>> ABAFA but reports internally as ABAGA. Finally it has internal 8-bit ECC
>>>>> which cannot be disabled.
>>>>
>>>> Boris and I agree that in this case, the chip should not be probed if
>>>> ecc->type != ON_DIE (and eventually NONE).
>>>>
>>>> This should be handled in the Micron driver.
>>>>
>>>> Also, what is the returned value of micron_supports_on_die_ecc() (with
>>>> patch 1/2)?
>>>
>>> micron_supports_on_die_ecc() returns MICRON_ON_DIE_UNSUPPORTED.
>>> Technically this chip should be MICRON_ON_DIE_MANDATORY since it can't
>>> be disabled but that wouldn't be much help since that would still result
>>> in -EINVAL. I'll dig into micron_supports_on_die_ecc() and see if I can
>>> find something in the datasheet to use.
>>>    
>>
>> Some further debugging. Nothing (in 4.17) calls
>> set_bit(ONFI_FEATURE_ON_DIE_ECC) so I don't think
>> micron_supports_on_die_ecc() can return anything other than
>> MICRON_ON_DIE_UNSUPPORTED, unless I'm missing something for how the
>> {get,set}_feature_list is populated.
> 
> Nope you're not. Looks like we broke Micron on-die ECC in 4.17.
> 
>>
>> With the onfi.version fix and the following
>>
>> --- a/drivers/mtd/nand/raw/nand_micron.c
>> +++ b/drivers/mtd/nand/raw/nand_micron.c
>> @@ -66,7 +66,9 @@ static int micron_nand_onfi_init(struct nand_chip *chip)
>>
>>           if (p->supports_set_get_features) {
>>                   set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->set_feature_list);
>> +               set_bit(ONFI_FEATURE_ON_DIE_ECC, p->set_feature_list);
>>                   set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->get_feature_list);
>> +               set_bit(ONFI_FEATURE_ON_DIE_ECC, p->get_feature_list);
>>           }
> 
> Can you send a patch containing only the above changes with the
> Cc-stable and Fixes tags?
> 
>> @@ -240,7 +246,7 @@ static int micron_supports_on_die_ecc(struct
>> nand_chip *chip)
>>            * Some Micron NANDs have an on-die ECC of 4/512, some other
>> -         * 8/512. We only support the former.
>> +         * 8/512.
>>            */
>> -       if (chip->ecc_strength_ds != 4)
>> +       if (chip->ecc_strength_ds != 4 && chip->ecc_strength_ds != 8)
>>                   return MICRON_ON_DIE_UNSUPPORTED;
>>
> 
> This should be done in a separate patch.
>   
>> I can get micron_supports_on_die_ecc() to return MICRON_ON_DIE_SUPPORTED.
>>
> 
> That's weird. You should have MICRON_ON_DIE_MANDATORY here. Could it be
> that the ONFI_FEATURE_ON_DIE_ECC_EN bit does not really reflect the ECC
> engine state? If that's the case, we'll have to change the way we
> detect if on-die ECC is supported/mandatory/not-supported (based on the
> model name stored in the ONFI param page?).
> 

Even though though MT29F1G08ABAFAWP-ITE:F says on-die ECC is enabled and 
cannot be disabled it still seems to respond to 
micron_nand_on_die_ecc_setup(chip, false); by clearing the feature bit 
retrieved by nand_get_features(chip, ONFI_FEATURE_ON_DIE_ECC, feature).

I see in the original thread that the detection of the 70s parts can be 
done by the "Number of bits ECC correctability". Can we assume that all 
70s has MICRON_ON_DIE_MANDATORY or do I need to make it based on 
specific IDs?