Re: [PATCH v4 1/5] mtd: nand: Detect Micron flash with on-die ECC (aka "internal ECC") enabled.

[Brian Norris <computersforpeace@gmail.com>]

On Mon, Mar 31, 2014 at 05:28:53PM -0600, David Mosberger wrote:
> Signed-off-by: David Mosberger <davidm@egauge.net>
> ---
>  drivers/mtd/nand/nand_base.c |   27 ++++++++++++++++++++++++---
>  include/linux/mtd/nand.h     |    4 ++++
>  2 files changed, 28 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 09fe1b1..f145f00 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3054,11 +3054,30 @@ static int nand_setup_read_retry_micron(struct mtd_info *mtd, int retry_mode)
>  			feature);
>  }
>  
> +static void

Does this really need to be on its own line? It doesn't match the style
of anything else.

> +nand_onfi_detect_micron_on_die_ecc(struct mtd_info *mtd,
> +				   struct nand_chip *chip)

I'm really not sure why the inconsistent style throughout nand_base on
using both an 'mtd' and a 'chip' parameter (we often assume that
'mtd->priv == chip'). If you need the mtd parameter, let's just pass it
instead of 'chip'.

> +{
> +	u8 features[ONFI_SUBFEATURE_PARAM_LEN];
> +
> +	if (chip->onfi_get_features(mtd, chip, ONFI_FEATURE_ADDR_ARRAY_OP_MODE,
> +				    features) < 0)
> +		return;
> +
> +	if (features[0] & ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC) {
> +		/*
> +		 * If the chip has on-die ECC enabled, we kind of have
> +		 * to do the same...
> +		 */

That's not really true at all, is it? We can simply disable the on-die
ECC and use the provided spare area with a different ECC scheme, can't
we? (e.g., software BCH; or an SoC's HW ECC) In fact, I think disabling
the on-die ECC is a more reasonable default; nand_base is used by many
drivers, most of which provide their own implementations, and many of
which would not be compatible with the implementations you provide. A
system should have to "opt in" somehow to enable this, I think.

Additionally, you need a way to inform the hardware driver that you're
using on-die ECC, so they can make the appropriate choices (to disable
their HW ECC, for instance).

BTW, what driver/controller/SoC are you running this with?

> +		pr_info("Using on-die ECC\n");
> +	}
> +}
> +
>  /*
>   * Configure chip properties from Micron vendor-specific ONFI table
>   */
> -static void nand_onfi_detect_micron(struct nand_chip *chip,
> -		struct nand_onfi_params *p)
> +static void nand_onfi_detect_micron(struct mtd_info *mtd,
> +		struct nand_chip *chip, struct nand_onfi_params *p)

Ditto on mtd + chip.

>  {
>  	struct nand_onfi_vendor_micron *micron = (void *)p->vendor;
>  
> @@ -3067,6 +3086,8 @@ static void nand_onfi_detect_micron(struct nand_chip *chip,
>  
>  	chip->read_retries = micron->read_retry_options;
>  	chip->setup_read_retry = nand_setup_read_retry_micron;
> +
> +	nand_onfi_detect_micron_on_die_ecc(mtd, chip);
>  }
>  
>  /*
> @@ -3168,7 +3189,7 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
>  	}
>  
>  	if (p->jedec_id == NAND_MFR_MICRON)
> -		nand_onfi_detect_micron(chip, p);
> +		nand_onfi_detect_micron(mtd, chip, p);
>  
>  	return 1;
>  }
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 2bd6e4e..780ab58 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -225,6 +225,10 @@ struct nand_chip;
>  /* Vendor-specific feature address (Micron) */
>  #define ONFI_FEATURE_ADDR_READ_RETRY	0x89
>  
> +/* Vendor-specific array operation mode (Micron) */
> +#define ONFI_FEATURE_ADDR_ARRAY_OP_MODE	0x90
> +#define ONFI_FEATURE_ARRAY_OP_MODE_ENABLE_ON_DIE_ECC		0x08
> +
>  /* ONFI subfeature parameters length */
>  #define ONFI_SUBFEATURE_PARAM_LEN	4
>  

Brian