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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

On Fri, 22 Dec 2017 18:28:52 +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 | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 96c97588e1ba..ea862be6a803 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1236,6 +1236,18 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
>  				tmode_param);
>  		if (ret)
>  			goto err;
> +
> +		ret = chip->onfi_get_features(mtd, chip,
> +					      ONFI_FEATURE_ADDR_TIMING_MODE,
> +					      tmode_param);
> +		if (ret)
> +			goto err;
> +
> +		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;
> +		}

Hm, I'm not sure this is safe. The spec says that new ONFI timing mode
is applied as soon the CS line is released after a
SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no
guarantee that the CS will be kept low by the controller after
->onfi_set_features() returns we must assume the new mode has been
applied and call ->setup_data_interface() to instruct the controller
to apply new timings.

If you want to check if the mode has really been applied, you should
release the CS (->select_chip(-1)), re-acquire it
(->select_chip(X)), and call
->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears that
the mode has not been applied, you should restore timing mode 0 and
issue a RESET.

>  	}
>  
>  	ret = chip->setup_data_interface(mtd, chipnr, &chip->data_interface);