Re: [PATCH 1/2] mtd: rawnand: Use the ->init_data_interface() hook

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

Hi Rickard,

Rickard Andersson <rickaran@axis.com> wrote on Thu, 14 May 2020
11:13:41 +0200:

> From: Rickard x Andersson <rickaran@axis.com>
> 
> Check if the NAND vendor provided hook works and check that
> the NAND controller can handle the timings.
> 
> Signed-off-by: Rickard x Andersson <rickaran@axis.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 8744f0033f78..974050a04f23 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -955,6 +955,30 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
>  	return ret;
>  }
>  
> +static int nand_try_init_data_interface(struct nand_chip *chip)
> +{
> +	int ret = chip->ops.init_data_interface(chip);
> +
> +	if (!ret) {
> +		/*
> +		 * 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);

I would like this to happen in Toshiba's code. Imagine you can try
several timings, the vendor code should be able to check by itself the
controller supports the timings.

I think you can drop this "try init data interface" and move the
content to Toshiba's driver.

> +	}
> +
> +	if (ret) {
> +		/* The provided data interface timings did not work */
> +		chip->ops.init_data_interface = NULL;
> +		memset(&chip->data_interface, 0,
> +		       sizeof(struct nand_data_interface));
> +	}
> +
> +	return ret;
> +}
> +
>  /**
>   * nand_choose_data_interface - find the best data interface and timings
>   * @chip: The NAND chip
> @@ -980,8 +1004,11 @@ static int nand_choose_data_interface(struct nand_chip *chip)
>  	 * ->init_data_interface() is expected to update the entire chip's
>  	 * nand_data_interface structure.
>  	 */
> -	if (nand_has_init_data_interface(chip))
> -		return chip->ops.init_data_interface(chip);

I renamed it "choose_data_interface" now, you can pull changes from
Github.

> +	if (nand_has_init_data_interface(chip)) {
> +		ret = nand_try_init_data_interface(chip);
> +		if (!ret)
> +			return 0;

I'm fine with a fallback on the regular ONFI research though, which in
this case would choose timings mode 0 I guess. Can you validate?

On my side I changed the code on Github and now a negative return code
just silently fails so that we fallback on mode 0.

> +	}
>  
>  	/*
>  	 * First try to identify the best timings from ONFI parameters and

Thanks,
Miquèl

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