Re: [PATCH v4 16/19] mtd: rawnand: Introduce nand_choose_best_sdr_iface()

[Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>]

On Mon, 25 May 2020 19:42:36 +0200
Miquel Raynal <miquel.raynal@xxxxxxxxxxx> wrote:

> Add a new helper: given a data interface with a specific set of
> timings, check with the controller if this interface can be
> supported. If not, fallback to the closest slower ONFI mode.
> 
> Extract this logic from nand_choose_data_interface() and use the new
> helper instead, so that this code can be reused later.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@xxxxxxxxxxx>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 74 ++++++++++++++++++++------------
>  1 file changed, 46 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 776f2d119bad..15e10f045c9f 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -1004,6 +1004,42 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
>  	return ret;
>  }
>  
> +/**
> + * nand_choose_best_sdr_iface - given a data interface, find the closest
> + *                              mode/timings set for this interface supported
> + *                              by both the NAND controller and the NAND chip
> + * @chip: the NAND chip
> + * @best_iface: the best data interface (can eventually be updated)
> + */
> +static int nand_choose_best_sdr_iface(struct nand_chip *chip,
> +				      struct nand_data_interface *best_iface)

You're just choosing the timings here, not the interface
configuration (SDR/DDR+timings), so I'd recommend renaming this function
nand_choose_best_sdr_timings() and passing a nand_sdr_timings object.

> +{
> +	const struct nand_controller_ops *ops = chip->controller->ops;
> +	int mode, ret;
> +
> +	/* Verify the controller supports the requested interface */
> +	ret = ops->setup_data_interface(chip, NAND_DATA_IFACE_CHECK_ONLY,
> +					best_iface);
> +	if (!ret)
> +		return ret;
> +
> +	/* Fallback to slower modes */
> +	for (mode = best_iface->timings.mode - 1; mode >= 0; mode--) {
> +		ret = onfi_fill_data_interface(chip, best_iface,
> +					       NAND_SDR_IFACE, mode);
> +		if (ret)
> +			continue;
> +
> +		ret = ops->setup_data_interface(chip,
> +						NAND_DATA_IFACE_CHECK_ONLY,
> +						best_iface);
> +		if (!ret)
> +			break;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * nand_choose_data_interface - find the best data interface and timings
>   * @chip: The NAND chip
> @@ -1019,7 +1055,7 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
>   */
>  static int nand_choose_data_interface(struct nand_chip *chip)
>  {
> -	int modes, mode, ret;
> +	int best_mode, ret;
>  
>  	if (!nand_controller_has_setup_data_iface(chip))
>  		return 0;
> @@ -1029,35 +1065,17 @@ static int nand_choose_data_interface(struct nand_chip *chip)
>  	 * 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->default_timing_mode)
> -			return 0;
> +	if (chip->parameters.onfi)
> +		best_mode = fls(chip->parameters.onfi->async_timing_mode) - 1;
> +	else
> +		best_mode = chip->default_timing_mode;
>  
> -		modes = GENMASK(chip->default_timing_mode, 0);
> -	}
> +	ret = onfi_fill_data_interface(chip, &chip->data_interface,
> +				       NAND_SDR_IFACE, best_mode);
> +	if (ret)
> +		return ret;
>  
> -	for (mode = fls(modes) - 1; mode >= 0; mode--) {
> -		ret = onfi_fill_data_interface(chip, &chip->data_interface,
> -					       NAND_SDR_IFACE, mode);
> -		if (ret)
> -			continue;
> -
> -		/*
> -		 * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the
> -		 * controller supports the requested timings.
> -		 */
> -		ret = chip->controller->ops->setup_data_interface(chip,
> -						 NAND_DATA_IFACE_CHECK_ONLY,
> -						 &chip->data_interface);
> -		if (!ret) {
> -			chip->default_timing_mode = mode;
> -			break;
> -		}
> -	}
> -
> -	return 0;
> +	return nand_choose_best_sdr_iface(chip, &chip->data_interface);
>  }
>  
>  /**


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