Re: [PATCH v7 21/28] mtd: rawnand: Introduce nand_choose_best_sdr_timings()

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Fri, 29 May 2020 13:13:15 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Extract the logic out of nand_choose_interface() to create a

			   ^nand_choose_interface_config()

> public helper that can be reused by manufacturer drivers. Add the
> possibility to provide a specific set of timings.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>

Reviewed-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>

> ---
>  drivers/mtd/nand/raw/internals.h |  3 +
>  drivers/mtd/nand/raw/nand_base.c | 95 ++++++++++++++++++++------------
>  2 files changed, 62 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> index 63c5af436901..5ebfbb89e572 100644
> --- a/drivers/mtd/nand/raw/internals.h
> +++ b/drivers/mtd/nand/raw/internals.h
> @@ -90,6 +90,9 @@ void onfi_fill_interface_config(struct nand_chip *chip,
>  				unsigned int timing_mode);
>  unsigned int
>  onfi_find_closest_sdr_mode(const struct nand_sdr_timings *spec_timings);
> +int nand_choose_best_sdr_timings(struct nand_chip *chip,
> +				 struct nand_interface_config *iface,
> +				 struct nand_sdr_timings *spec_timings);
>  int nand_get_features(struct nand_chip *chip, int addr, u8 *subfeature_param);
>  int nand_set_features(struct nand_chip *chip, int addr, u8 *subfeature_param);
>  int nand_read_page_raw_notsupp(struct nand_chip *chip, u8 *buf,
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 3bfd71d589cf..956a66fdccb8 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -1005,6 +1005,63 @@ static int nand_setup_interface(struct nand_chip *chip, int chipnr)
>  	return ret;
>  }
>  
> +/**
> + * nand_choose_best_sdr_timings - Pick up the best SDR timings that both the
> + *                                NAND controller and the NAND chip support
> + * @chip: the NAND chip
> + * @iface: the interface configuration (can eventually be updated)
> + * @spec_timings: specific timings, when not fitting the ONFI specification
> + *
> + * If specific timings are provided, use them. Otherwise, try to retrieve
> + * supported timing modes from ONFI information. Finally, if the NAND chip does
> + * not follow the ONFI specification, rely on the ->default_timing_mode
> + * specified in the nand_ids table.
> + */
> +int nand_choose_best_sdr_timings(struct nand_chip *chip,
> +				 struct nand_interface_config *iface,
> +				 struct nand_sdr_timings *spec_timings)
> +{
> +	const struct nand_controller_ops *ops = chip->controller->ops;
> +	int best_mode = 0, mode, ret;
> +
> +	iface->type = NAND_SDR_IFACE;
> +
> +	if (spec_timings) {
> +		iface->timings.sdr = *spec_timings;
> +		iface->timings.mode = onfi_find_closest_sdr_mode(spec_timings);
> +
> +		/* Verify the controller supports the requested interface */
> +		ret = ops->setup_interface(chip, NAND_DATA_IFACE_CHECK_ONLY,
> +					   iface);
> +		if (!ret)
> +			return ret;
> +
> +		/* Fallback to slower modes */
> +		best_mode = iface->timings.mode;
> +	} else {
> +		if (chip->parameters.onfi) {
> +			unsigned int onfi_modes;
> +
> +			onfi_modes = chip->parameters.onfi->async_timing_mode;
> +			best_mode = fls(onfi_modes) - 1;
> +		} else {
> +			best_mode = chip->onfi_timing_mode_default;
> +		}
> +	}
> +
> +
> +	for (mode = best_mode; mode >= 0; mode--) {
> +		onfi_fill_interface_config(chip, iface, NAND_SDR_IFACE, mode);
> +
> +		ret = ops->setup_interface(chip, NAND_DATA_IFACE_CHECK_ONLY,
> +					   iface);
> +		if (!ret)
> +			return 0;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * nand_choose_interface_config - find the best data interface and timings
>   * @chip: The NAND chip
> @@ -1016,48 +1073,14 @@ static int nand_setup_interface(struct nand_chip *chip, int chipnr)
>   * ->onfi_timing_mode_default specified in the nand_ids table. After this
>   * function nand_chip->interface_ is initialized with the best timing mode
>   * available.
> - *
> - * Returns 0 for success or negative error code otherwise.
>   */
>  static int nand_choose_interface_config(struct nand_chip *chip)
>  {
> -	int modes, mode, ret;
> -
>  	if (!nand_controller_can_setup_interface(chip))
>  		return 0;
>  
> -	/*
> -	 * First try to identify the best timings from ONFI parameters and
> -	 * if the NAND does not support ONFI, fallback to the default ONFI
> -	 * timing mode.
> -	 */
> -	if (chip->parameters.onfi) {
> -		modes = chip->parameters.onfi->async_timing_mode;
> -	} else {
> -		if (!chip->onfi_timing_mode_default)
> -			return 0;
> -
> -		modes = GENMASK(chip->onfi_timing_mode_default, 0);
> -	}
> -
> -	for (mode = fls(modes) - 1; mode >= 0; mode--) {
> -		onfi_fill_interface_config(chip, &chip->interface_config,
> -					   NAND_SDR_IFACE, mode);
> -
> -		/*
> -		 * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the
> -		 * controller supports the requested timings.
> -		 */
> -		ret = chip->controller->ops->setup_interface(chip,
> -						 NAND_DATA_IFACE_CHECK_ONLY,
> -						 &chip->interface_config);
> -		if (!ret) {
> -			chip->onfi_timing_mode_default = mode;
> -			break;
> -		}
> -	}
> -
> -	return 0;
> +	return nand_choose_best_sdr_timings(chip, &chip->interface_config,
> +					    NULL);
>  }
>  
>  /**


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/