Re: [PATCH v2 1/2] mtd: nand: Check ONFI timings have been acked by the chip

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

On Fri, 19 Jan 2018 10:25:46 +0100
Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Choosing ONFI timings when ->onfi_set/get_features() calls are supported
> by the NAND chip is a matter of reading the chip's ONFI parameter page
> and telling the chip the chosen mode (between all of the supported ones)
> with ->onfi_set_feature().
> 
> Add a check on whether the chip "acked" the timing mode or not.
> 
> This can be a problem for NAND chips that do not follow entirely the
> ONFI specification. These chips actually support other modes than
> "mode 0", but do not update the parameter page once a timing mode has
> been selected. This issue will be addressed in another patch that will
> add the feature to overwrite NAND chips features within the parameter
> page, from the NAND chip driver.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c | 41 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 96c97588e1ba..e209a65a17e0 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1231,15 +1231,47 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
>  			chip->onfi_timing_mode_default,
>  		};
>  
> +		chip->select_chip(mtd, chipnr);
>  		ret = chip->onfi_set_features(mtd, chip,
>  				ONFI_FEATURE_ADDR_TIMING_MODE,
>  				tmode_param);
> +		chip->select_chip(mtd, -1);
>  		if (ret)
> -			goto err;
> +			return ret;
>  	}
>  
>  	ret = chip->setup_data_interface(mtd, chipnr, &chip->data_interface);
> -err:
> +	if (ret)
> +		return ret;
> +
> +	if (chip->onfi_version &&
> +	    (le16_to_cpu(chip->onfi_params.opt_cmd) &
> +	     ONFI_OPT_CMD_SET_GET_FEATURES)) {
> +		u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {};
> +
> +		chip->select_chip(mtd, chipnr);
> +		ret = chip->onfi_get_features(mtd, chip,
> +					      ONFI_FEATURE_ADDR_TIMING_MODE,
> +					      tmode_param);
> +		chip->select_chip(mtd, -1);
> +		if (ret)
> +			goto err_reset_chip;
> +
> +		if (tmode_param[0] != chip->onfi_timing_mode_default) {
> +			pr_warn("timings mode %d not acknowledged by the NAND chip\n",
> +				chip->onfi_timing_mode_default);
> +			goto err_reset_chip;
> +		}
> +	}
> +
> +	return 0;
> +
> +err_reset_chip:
> +	onfi_fill_data_interface(chip, NAND_SDR_IFACE, 0);
> +	chip->select_chip(mtd, chipnr);
> +	nand_reset_op(chip);
> +	chip->select_chip(mtd, -1);
> +
>  	return ret;
>  }
>  
> @@ -2738,10 +2770,8 @@ int nand_reset(struct nand_chip *chip, int chipnr)
>  	if (ret)
>  		return ret;
>  
> -	chip->select_chip(mtd, chipnr);
>  	chip->data_interface = saved_data_intf;
>  	ret = nand_setup_data_interface(chip, chipnr);
> -	chip->select_chip(mtd, -1);
>  	if (ret)
>  		return ret;
>  
> @@ -6571,10 +6601,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>  
>  	/* Enter fastest possible mode on all dies. */
>  	for (i = 0; i < chip->numchips; i++) {
> -		chip->select_chip(mtd, i);
>  		ret = nand_setup_data_interface(chip, i);
> -		chip->select_chip(mtd, -1);

Can you split this patch in 2: one that moves the ->select_chip() calls
to nand_setup_data_interface() and a second one that calls
->get_features() to make sure the new timing mode has been applied.

Thanks,

Boris

> -
>  		if (ret)
>  			goto err_nand_manuf_cleanup;
>  	}



-- 
Boris Brezillon, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com