Re: [PATCH v3 1/2] mtd: rawnand: Add and use helper for testing data interface

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hi Rickard,

Rickard Andersson <rickaran@xxxxxxxx> wrote on Mon, 25 May 2020
16:51:32 +0200:

> From: Rickard x Andersson <rickaran@xxxxxxxx>
> 
> This helper checks if the controller supports the current
> data interface timings. If the timings are not supported
> the vendor provided corresponding onfi mode will be tried,
> and if that fails we will fall back to onfi mode 0.
> 
> The helper can also be used by NAND vendor driver for
> testing different timings.
> 
> Signed-off-by: Rickard x Andersson <rickaran@xxxxxxxx>
> ---
>  drivers/mtd/nand/raw/internals.h | 24 ++++++++++++++++++++++++
>  drivers/mtd/nand/raw/nand_base.c | 28 ++++++++++++++++++----------
>  2 files changed, 42 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> index 615677820338..6dacf110b559 100644
> --- a/drivers/mtd/nand/raw/internals.h
> +++ b/drivers/mtd/nand/raw/internals.h
> @@ -148,6 +148,30 @@ static inline bool nand_can_choose_data_interface(struct nand_chip *chip)
>  	return chip->ops.choose_data_interface;
>  }
>  
> +/**
> + * nand_controller_supports_data_interface - Check if controller can handle
> + * the current timings.
> + *
> + * @chip: The NAND chip
> + */
> +static inline int
> +nand_controller_supports_data_interface(struct nand_chip *chip)
> +{
> +	int ret;
> +	const struct nand_controller_ops *ops = chip->controller->ops;
> +
> +	/*
> +	 * Pass NAND_DATA_IFACE_CHECK_ONLY to only check if the
> +	 * controller supports the requested timings.
> +	 */
> +	ret = ops->setup_data_interface(chip,
> +					NAND_DATA_IFACE_CHECK_ONLY,
> +					&chip->data_interface);
> +
> +	return ret;
> +}
> +
> +
>  /* BBT functions */
>  int nand_markbad_bbt(struct nand_chip *chip, loff_t offs);
>  int nand_isreserved_bbt(struct nand_chip *chip, loff_t offs);
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index c42cbeb7e446..e3c57649380b 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -982,8 +982,24 @@ static int nand_choose_data_interface(struct nand_chip *chip)
>  	 */
>  	if (nand_can_choose_data_interface(chip)) {
>  		ret = chip->ops.choose_data_interface(chip);
> +		if (ret)
> +			return ret;
> +
> +		/*
> +		 * Check if the controller can handle the NAND vendor specific
> +		 * best timings.
> +		 */
> +		ret = nand_controller_supports_data_interface(chip);
>  		if (!ret)
>  			return ret;
> +
> +		/*
> +		 * Specific timings did not work, try using corresponding
> +		 * onfi mode provided by vendor.
> +		 */
> +		if (chip->data_interface.timings.mode)
> +			chip->default_timing_mode =
> +				chip->data_interface.timings.mode;
>  	}
>  
>  	/*
> @@ -994,9 +1010,6 @@ static int nand_choose_data_interface(struct nand_chip *chip)
>  	if (chip->parameters.onfi) {
>  		modes = chip->parameters.onfi->async_timing_mode;
>  	} else {
> -		if (!chip->default_timing_mode)
> -			return 0;
> -
>  		modes = GENMASK(chip->default_timing_mode, 0);
>  	}
>  
> @@ -1005,13 +1018,8 @@ static int nand_choose_data_interface(struct nand_chip *chip)
>  		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);
> +		/* Check if the controller supports the requested timings. */
> +		ret = nand_controller_supports_data_interface(chip);
>  		if (!ret) {
>  			chip->default_timing_mode = mode;
>  			break;

Actually I think what Boris had in mind was a little bit different. As
the "preparation series" has not been sent over the mailing list yet,
if you don't mind, I will drop patch 1/2 which is not needed after my
latest changes and take your second patch, adapted to the latest
changes too. I am not taking the ownership of your series, just
proposing the big picture and provide a more usual way to review the
patches than a Github branch. Then, I'll might ask you to respin the
entire series as well if needed.


Thanks,
Miquèl

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