Re: [PATCH v5 14/28] mtd: rawnand: timings: Add a helper to find the closest ONFI mode

From: Boris Brezillon <boris.brezillon@collabora.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
Cc: Rickard Andersson <rickaran@axis.com>,
	Richard Weinberger <richard@nod.at>,
	linux-mtd@lists.infradead.org,
	Vignesh Raghavendra <vigneshr@ti.com>,
	Tudor Ambarus <Tudor.Ambarus@microchip.com>
Subject: Re: [PATCH v5 14/28] mtd: rawnand: timings: Add a helper to find the closest ONFI mode
Date: Tue, 26 May 2020 23:42:05 +0200	[thread overview]
Message-ID: <20200526234205.73021fc2@collabora.com> (raw)
In-Reply-To: <20200526191725.7591-15-miquel.raynal@bootlin.com>

On Tue, 26 May 2020 21:17:11 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Vendors are allowed to provide their own set of timings. In this case,
> we provide a way to derive the "closest" timing mode so that, if the
> NAND controller does not support tweaking these parameters, it will be
> able to configure itself anyway.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/internals.h    |  2 ++
>  drivers/mtd/nand/raw/nand_timings.c | 49 +++++++++++++++++++++++++++++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> index dc84e3b55d48..c7357ae86eeb 100644
> --- a/drivers/mtd/nand/raw/internals.h
> +++ b/drivers/mtd/nand/raw/internals.h
> @@ -88,6 +88,8 @@ int onfi_fill_data_interface(struct nand_chip *chip,
>  			     struct nand_data_interface *iface,
>  			     enum nand_data_interface_type type,
>  			     int timing_mode);
> +unsigned int
> +onfi_find_closest_sdr_mode(const 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_timings.c b/drivers/mtd/nand/raw/nand_timings.c
> index ce6bb87db2e8..3c44c4b90536 100644
> --- a/drivers/mtd/nand/raw/nand_timings.c
> +++ b/drivers/mtd/nand/raw/nand_timings.c
> @@ -273,6 +273,55 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
>  	},
>  };
>  
> +/**
> + * onfi_find_closest_sdr_mode - Derive the closest ONFI SDR timing mode given a
> + *                              set of timings
> + * @spec_timings: the timings to challenge
> + */
> +unsigned int
> +onfi_find_closest_sdr_mode(const struct nand_sdr_timings *spec_timings)
> +{
> +	const struct nand_sdr_timings *onfi_timings;
> +	int mode;
> +
> +	for (mode = ARRAY_SIZE(onfi_sdr_timings) - 1; mode > 0; mode--) {
> +		onfi_timings = &onfi_sdr_timings[mode].timings.sdr;
> +
> +		if (spec_timings->tCCS_min > onfi_timings->tCCS_min ||
> +		    spec_timings->tADL_min > onfi_timings->tADL_min ||
> +		    spec_timings->tALH_min > onfi_timings->tALH_min ||
> +		    spec_timings->tALS_min > onfi_timings->tALS_min ||
> +		    spec_timings->tAR_min > onfi_timings->tAR_min ||
> +		    spec_timings->tCEH_min > onfi_timings->tCEH_min ||
> +		    spec_timings->tCH_min > onfi_timings->tCH_min ||
> +		    spec_timings->tCLH_min > onfi_timings->tCLH_min ||
> +		    spec_timings->tCLR_min > onfi_timings->tCLR_min ||
> +		    spec_timings->tCLS_min > onfi_timings->tCLS_min ||
> +		    spec_timings->tCOH_min > onfi_timings->tCOH_min ||
> +		    spec_timings->tCS_min > onfi_timings->tCS_min ||
> +		    spec_timings->tDH_min > onfi_timings->tDH_min ||
> +		    spec_timings->tDS_min > onfi_timings->tDS_min ||
> +		    spec_timings->tIR_min > onfi_timings->tIR_min ||
> +		    spec_timings->tRC_min > onfi_timings->tRC_min ||
> +		    spec_timings->tREH_min > onfi_timings->tREH_min ||
> +		    spec_timings->tRHOH_min > onfi_timings->tRHOH_min ||
> +		    spec_timings->tRHW_min > onfi_timings->tRHW_min ||
> +		    spec_timings->tRLOH_min > onfi_timings->tRLOH_min ||
> +		    spec_timings->tRP_min > onfi_timings->tRP_min ||
> +		    spec_timings->tRR_min > onfi_timings->tRR_min ||
> +		    spec_timings->tWC_min > onfi_timings->tWC_min ||
> +		    spec_timings->tWH_min > onfi_timings->tWH_min ||
> +		    spec_timings->tWHR_min > onfi_timings->tWHR_min ||
> +		    spec_timings->tWP_min > onfi_timings->tWP_min ||
> +		    spec_timings->tWW_min > onfi_timings->tWW_min)

Just nitpicking but you could just do the opposite test ('<=' instead of
'>' and '&&' instead of '||') and return mode directly, so you don't
need this continue.

I didn't check if all the _min timings were tested, but assuming that's
the case, you can add

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

> +			continue;
> +
> +		return mode;
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * onfi_fill_data_interface - Initialize a data interface from a given ONFI mode
>   * @chip: The NAND chip

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