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

[Boris Brezillon <boris.brezillon@bootlin.com>]

Hi Chris,

On Tue, 19 Jun 2018 01:44:24 +0000
Chris Packham <Chris.Packham@alliedtelesis.co.nz> 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@alliedtelesis.co.nz> 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?).

> Then I run into a problem with the marvell_nand.c which currently 
> doesn't handle NAND_ECC_ON_DIE which is easily fixed.

Yep, adding that to the new driver should be pretty easy.

> 
> But then I have the issue that I need to handle systems with either type 
> of ECC scheme ("on-die" or "hw") which I'm not sure is even possible 
> within the dts.

You mean having the same dts for both setup. Indeed, that's not
supported right now.

> 
> I'll re-base against 4.18-rc1 and send what I have so-far.

Cool. I'm particularly interested by the SET/GET_FEATURE(ECC) fix.

Thanks,

Boris