Re: [PATCH v2 2/2] mtd: nand: gpmi: Support ->setup_data_interface()

[Miquel Raynal <miquel.raynal@free-electrons.com>]

Hello,

On Fri, 19 Jan 2018 10:25:47 +0100
Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Until now the GPMI driver had its own timings logic while the core
> already handles that and request the NAND controller drivers to support
> the ->setup_data_interface() hook. Implement that hook by reusing the
> already existing function. No real glue is necessary between core timing
> delays and GPMI registers because the driver already translates the
> ONFI timing modes into register values.
> 
> Make use of the core's tREA, tRLOH and tRHOH values that allow computing
> more precise timings for mode [0-3] and get significantly better values
> (+20% with an i.MX6 Sabre Auto board). Otherwise use the existing logic.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 275 +++++++++------------------------
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |  43 ++++--
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  82 +++++-----
>  3 files changed, 149 insertions(+), 251 deletions(-)
> 

[...]

> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index b51db8c85405..a3db75c27348 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -938,11 +938,32 @@ static void gpmi_select_chip(struct mtd_info *mtd, int chipnr)
>  {

[...]

> +	/*
> +	 * This driver currently supports only one NAND chip. Plus, dies share
> +	 * the same configuration. So once timings have been applied on the
> +	 * controller side, they will not change anymore. When the time will
> +	 * come, the check on must_apply_timings will have to be dropped.
> +	 */
> +	if (chipnr > 0 && this->hw.must_apply_timings) {

I realized after sending that performances were actually bad, it is
because this line is wrong twice: the chipnr may be 0, and
hw.must_apply_timings must be reset after a ->setup_data_interface().

> +		this->hw.must_apply_timings = false;
> +		gpmi_nfc_apply_timings(this);
> +	}

I will send a v3, sorry for the noise.

Regards,
Miquèl