Re: [PATCH v4 15/19] 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 v4 15/19] mtd: rawnand: timings: Add a helper to find the closest ONFI mode
Date: Mon, 25 May 2020 21:30:49 +0200	[thread overview]
Message-ID: <20200525213049.2098cd65@collabora.com> (raw)
In-Reply-To: <20200525174239.11349-16-miquel.raynal@bootlin.com>

On Mon, 25 May 2020 19:42:35 +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    |  1 +
>  drivers/mtd/nand/raw/nand_timings.c | 52 +++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> index dc84e3b55d48..ac103d8767be 100644
> --- a/drivers/mtd/nand/raw/internals.h
> +++ b/drivers/mtd/nand/raw/internals.h
> @@ -88,6 +88,7 @@ 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_equivalent_sdr_mode(const struct nand_sdr_timings *vendor_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..7f5caa325fbe 100644
> --- a/drivers/mtd/nand/raw/nand_timings.c
> +++ b/drivers/mtd/nand/raw/nand_timings.c
> @@ -273,6 +273,58 @@ static const struct nand_data_interface onfi_sdr_timings[] = {
>  	},
>  };
>  
> +unsigned int onfi_find_equivalent_sdr_mode(const struct nand_sdr_timings *vendor_timings)

I wouldn't call that vendor timings, but spec timings, and why not
closest instead of equivalent. You also lack a kernel doc, and the
could get things fitting in the 80 chars limit if you declare it
like:

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 (vendor_timings->tCCS_min > onfi_timings->tCCS_min ||
> +		    vendor_timings->tR_max < onfi_timings->tR_max ||

Do we really care about _max values? Also not sure the < condition is
correct.

> +		    vendor_timings->tADL_min > onfi_timings->tADL_min ||
> +		    vendor_timings->tALH_min > onfi_timings->tALH_min ||
> +		    vendor_timings->tALS_min > onfi_timings->tALS_min ||
> +		    vendor_timings->tAR_min > onfi_timings->tAR_min ||
> +		    vendor_timings->tCEA_max < onfi_timings->tCEA_max ||
> +		    vendor_timings->tCEH_min > onfi_timings->tCEH_min ||
> +		    vendor_timings->tCH_min > onfi_timings->tCH_min ||
> +		    vendor_timings->tCHZ_max < onfi_timings->tCHZ_max ||
> +		    vendor_timings->tCLH_min > onfi_timings->tCLH_min ||
> +		    vendor_timings->tCLR_min > onfi_timings->tCLR_min ||
> +		    vendor_timings->tCLS_min > onfi_timings->tCLS_min ||
> +		    vendor_timings->tCOH_min > onfi_timings->tCOH_min ||
> +		    vendor_timings->tCS_min > onfi_timings->tCS_min ||
> +		    vendor_timings->tDH_min > onfi_timings->tDH_min ||
> +		    vendor_timings->tDS_min > onfi_timings->tDS_min ||
> +		    vendor_timings->tFEAT_max < onfi_timings->tFEAT_max ||
> +		    vendor_timings->tIR_min > onfi_timings->tIR_min ||
> +		    vendor_timings->tITC_max < onfi_timings->tITC_max ||
> +		    vendor_timings->tRC_min > onfi_timings->tRC_min ||
> +		    vendor_timings->tREA_max < onfi_timings->tREA_max ||
> +		    vendor_timings->tREH_min > onfi_timings->tREH_min ||
> +		    vendor_timings->tRHOH_min > onfi_timings->tRHOH_min ||
> +		    vendor_timings->tRHW_min > onfi_timings->tRHW_min ||
> +		    vendor_timings->tRHZ_max < onfi_timings->tRHZ_max ||
> +		    vendor_timings->tRLOH_min > onfi_timings->tRLOH_min ||
> +		    vendor_timings->tRP_min > onfi_timings->tRP_min ||
> +		    vendor_timings->tRR_min > onfi_timings->tRR_min ||
> +		    vendor_timings->tRST_max < onfi_timings->tRST_max ||
> +		    vendor_timings->tWB_max < onfi_timings->tWB_max ||
> +		    vendor_timings->tWC_min > onfi_timings->tWC_min ||
> +		    vendor_timings->tWH_min > onfi_timings->tWH_min ||
> +		    vendor_timings->tWHR_min > onfi_timings->tWHR_min ||
> +		    vendor_timings->tWP_min > onfi_timings->tWP_min ||
> +		    vendor_timings->tWW_min > onfi_timings->tWW_min)
> +			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/