Re: [PATCH v4 6/6] mtd: rawnand: micron: detect forced on-die ECC

[Boris Brezillon]

On Thu, 21 Jun 2018 22:33:28 +1200
Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx> wrote:

> Some Micron NAND chips have on-die ECC forceably enabled. The detect
> these based on chip ID as there seems to be no other way of
> distinguishing these chips from those that have optional support for
> on-die ECC.
> 
> When a chip with mandatory on-die ECC is detected change the current ECC
> mode to on-die.
> 
> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> ---
> I'm not convinced that just changing chip->ecc.mode is sensible. An
> alternative I considered was only proceeding if the ecc.mode is set to
> ON_DIE.
> 
> Changes in v4:
> - New
> 
>  drivers/mtd/nand/raw/nand_micron.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index f1ecd4986b50..9ba9007a9a06 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -227,6 +227,14 @@ enum {
>  	MICRON_ON_DIE_MANDATORY,
>  };
>  
> +/*
> + * These parts are known to have on-die ECC forceably enabled
> + */
> +static u8 micron_on_die_ecc[] = {
> +	0xd1, /* MT29F1G08ABAFA */
> +	0xa1, /* MT29F1G08ABBFA */
> +};
> +
>  /*
>   * Try to detect if the NAND support on-die ECC. To do this, we enable
>   * the feature, and read back if it has been enabled as expected. We
> @@ -241,6 +249,11 @@ static int micron_supports_on_die_ecc(struct nand_chip *chip)
>  {
>  	u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = { 0, };
>  	int ret;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(micron_on_die_ecc); i++)
> +		if (chip->id.data[1] == micron_on_die_ecc[i])
> +			return MICRON_ON_DIE_MANDATORY;
>  
>  	if (!chip->parameters.onfi.version)
>  		return MICRON_ON_DIE_UNSUPPORTED;
> @@ -289,8 +302,8 @@ static int micron_nand_init(struct nand_chip *chip)
>  	ondie = micron_supports_on_die_ecc(chip);
>  
>  	if (ondie == MICRON_ON_DIE_MANDATORY) {
> -		pr_err("On-die ECC forcefully enabled, not supported\n");
> -		return -EINVAL;
> +		pr_info("On-die ECC forcefully enabled\n");
> +		chip->ecc.mode = NAND_ECC_ON_DIE;

No, you can't do that, because most NAND drivers simply don't support
NAND_ECC_ON_DIE, and will override chip->ecc.mode without even checking
its value. You should just fail if chip->ecc.mode != NAND_ECC_ON_DIE.

		if (chip->ecc.mode != NAND_ECC_ON_DIE) {
			pr_err("On-die ECC can't be disabled\n");
			return -EINVAL;
		}
>  	}
>  
>  	if (chip->ecc.mode == NAND_ECC_ON_DIE) {