Re: [PATCH] mtd: nand: Add support to dedicated function to set timings

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Köry,

kory.maincent@bootlin.com wrote on Tue,  1 Feb 2022 18:45:14 +0100:

Perhaps the title prefix should be "mtd: rawnand:".

I am making a number of reworking proposals below, take what you like.

> With the current code if the board have a NAND ONFI compliant without

                                     has an ONFI compliant NAND

> support to the get and set features, U-boot return an ENOTSUP error and we

                                              returns an ENOTSUP error
when trying to tune the timings which prevents the probe of the device.

> can not talk to the memory. Indeed onfi_set_features return ENOTSUP error

                                              _features() returns ENOTSUP if... 

> if set/get features is not supported. In the case of timings we should not
> return ENOTSUP because we can use the default timings, the modification of
> the NAND timings is optional.

Actually it does not mean that setting the timings is optional but
rather that the NAND is already capable of listening at its highest
supported rate, so we assume in this case that it is fine to skip the
operation. But as far as I remember the ONFI specification does not
clearly state that this is a fully valid behavior.

> Fixed it by adding an intermediate nand_onfi_set_timings function which

Fix it                                                    ()

> does nothing if set/get feature is not supported.

does not error out if...                          

The code looks good otherwise!

> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index f7616985d9..44fb33776f 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -974,6 +974,22 @@ static int nand_reset_data_interface(struct nand_chip *chip, int chipnr)
>  	return ret;
>  }
>  
> +static int nand_onfi_set_timings(struct mtd_info *mtd, struct nand_chip *chip)
> +{
> +	if (!chip->onfi_version ||
> +	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
> +	      & ONFI_OPT_CMD_SET_GET_FEATURES))
> +		return 0;
> +
> +	u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
> +		chip->onfi_timing_mode_default,
> +	};
> +
> +	return chip->onfi_set_features(mtd, chip,
> +				       ONFI_FEATURE_ADDR_TIMING_MODE,
> +				       tmode_param);
> +}
> +
>  /**
>   * nand_setup_data_interface - Setup the best data interface and timings
>   * @chip: The NAND chip
> @@ -999,17 +1015,9 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
>  	 * Ensure the timing mode has been changed on the chip side
>  	 * before changing timings on the controller side.
>  	 */
> -	if (chip->onfi_version) {
> -		u8 tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
> -			chip->onfi_timing_mode_default,
> -		};
> -
> -		ret = chip->onfi_set_features(mtd, chip,
> -				ONFI_FEATURE_ADDR_TIMING_MODE,
> -				tmode_param);
> -		if (ret)
> -			goto err;
> -	}
> +	ret = nand_onfi_set_timings(mtd, chip);
> +	if (ret)
> +		goto err;
>  
>  	ret = chip->setup_data_interface(mtd, chipnr, chip->data_interface);
>  err:


Thanks,
Miquèl