[PATCH 0/2] mtd: nand: automate NAND timings selection

[PATCH 0/2] mtd: nand: automate NAND timings selection

[Boris Brezillon]

Hello,

This series aims at automating the NAND timings selection which is
currently supposed to be done in each NAND controller driver, thus
simplifying drivers implementation.

Patch 1 also opens the door to DDR NAND support, though setting DDR
timings is currently not supported.

Note that patch 2 is given here as an example of how patch 1 simplifies
things, but won't apply cleanly on mtd-next (it's based on top of other
changes I haven't submitted yet).

Best Regards,

Boris

Boris Brezillon (2):
  mtd: nand: automate NAND timings selection
  mtd: nand: sunxi: switch from manual to automated timing config

 drivers/mtd/nand/nand_base.c  | 189 +++++++++++++++++++++++++++++++++++++++++-
 drivers/mtd/nand/sunxi_nand.c |  72 ++++------------
 include/linux/mtd/nand.h      | 115 ++++++++++++++-----------
 3 files changed, 270 insertions(+), 106 deletions(-)

-- 
2.1.4

[PATCH 1/2] mtd: nand: automate NAND timings selection

[Boris Brezillon]

The NAND framework provides several helpers to query timing modes supported
by a NAND chip, but this implies that all NAND controller drivers have
to implement the same timings selection dance.

Provide a common logic to select the best timings based on ONFI or
->onfi_timing_mode_default information.
NAND controller willing to support timings adjustment should just
implement the ->setup_data_interface() method.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 189 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/nand.h     | 115 +++++++++++++++-----------
 2 files changed, 254 insertions(+), 50 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index eaf1fcd..52a1f89 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3323,6 +3323,144 @@ static void nand_onfi_detect_micron(struct nand_chip *chip,
 	chip->setup_read_retry = nand_setup_read_retry_micron;
 }
 
+/**
+ * nand_setup_data_interface - Setup the data interface and timings on the
+ *			       controller side
+ * @mtd: MTD device structure
+ * @conf: new configuration to apply
+ *
+ * Try to configure the NAND controller to support the provided data
+ * interface configuration.
+ *
+ * Returns 0 in case of success or -ERROR_CODE.
+ */
+static int nand_setup_data_interface(struct mtd_info *mtd,
+				     const struct nand_data_interface *conf)
+{
+	struct nand_chip *chip = mtd->priv;
+	int ret;
+
+	if (!chip->setup_data_interface)
+		return -ENOTSUPP;
+
+	ret = chip->setup_data_interface(mtd, conf, false);
+	if (ret)
+		return ret;
+
+	*chip->data_iface = *conf;
+
+	return 0;
+}
+
+/**
+ * nand_setup_data_interface - Check if a data interface config is supported
+ *			       by the NAND controller
+ * @mtd: MTD device structure
+ * @conf: new configuration to apply
+ *
+ * Check if the provided data interface configuration is supported by the
+ * NAND controller.
+ *
+ * Returns 0 if it is supported or -ERROR_CODE.
+ */
+static int nand_check_data_interface(struct mtd_info *mtd,
+				     const struct nand_data_interface *conf)
+{
+	struct nand_chip *chip = mtd->priv;
+
+	if (!chip->setup_data_interface)
+		return -ENOTSUPP;
+
+	return chip->setup_data_interface(mtd, conf, true);
+}
+
+/**
+ * nand_configure_data_interface - Configure the data interface and timings
+ * @mtd: MTD device structure
+ *
+ * Try to configure the data interface and NAND timings appropriately.
+ * First tries to retrieve supported timing modes from ONFI information,
+ * and if the NAND chip does not support ONFI, relies on the
+ * ->onfi_timing_mode_default specified in the nand_ids table.
+ *
+ * Returns 0 in case of success or -ERROR_CODE.
+ */
+static int nand_configure_data_interface(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct nand_data_interface *conf;
+	int modes, mode, ret = -EINVAL;
+
+	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
+	if (!conf)
+		return -ENOMEM;
+
+	/* TODO: support DDR interfaces */
+	conf->type = NAND_SDR_IFACE;
+
+	/*
+	 * First try to identify the best timings from ONFI parameters and
+	 * if the NAND does not support ONFI, fallback to the default ONFI
+	 * timing mode.
+	 */
+	modes = onfi_get_async_timing_mode(chip);
+	if (modes != ONFI_TIMING_MODE_UNKNOWN) {
+		for (mode = fls(modes) - 1; mode >= 0; mode--) {
+			conf->timings.sdr =
+				*onfi_async_timing_mode_to_sdr_timings(mode);
+
+			ret = nand_check_data_interface(mtd, conf);
+			if (!ret)
+				break;
+		}
+	} else {
+		mode = chip->onfi_timing_mode_default;
+		conf->timings.sdr =
+				*onfi_async_timing_mode_to_sdr_timings(mode);
+
+		ret = nand_check_data_interface(mtd, conf);
+	}
+
+	if (!ret) {
+		uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { mode };
+		int i;
+
+		/*
+		 * Ensure the timing mode has be changed on the chip side
+		 * before changing timings on the controller side.
+		 */
+		if (modes != ONFI_TIMING_MODE_UNKNOWN) {
+			/*
+			 * FIXME: should we really set the timing mode on all
+			 * dies?
+			 */
+			for (i = 0; i < chip->numchips; i++) {
+				chip->select_chip(mtd, i);
+				ret = chip->onfi_set_features(mtd, chip,
+						ONFI_FEATURE_ADDR_TIMING_MODE,
+						tmode_param);
+			}
+			chip->select_chip(mtd, -1);
+		}
+
+		if (!ret) {
+			ret = nand_setup_data_interface(mtd, conf);
+
+			/*
+			 * Reset the NAND chip if the data interface setup
+			 * failed so that the chip goes back to a known state
+			 * (timing mode 0).
+			 */
+			if (ret)
+				chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
+		}
+	}
+
+	kfree(conf);
+
+	return ret;
+}
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -3906,6 +4044,11 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
 ident_done:
 
+	/*
+	 * Setup the NAND interface (interface type + timings).
+	 */
+	nand_configure_data_interface(mtd);
+
 	/* Try to identify manufacturer */
 	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
 		if (nand_manuf_ids[maf_idx].id == *maf_id)
@@ -4033,6 +4176,41 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	/* Set the default functions */
 	nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
 
+	/*
+	 * Allocate an interface config struct if the controller implements the
+	 * ->apply_interface_conf() method.
+	 */
+	if (chip->setup_data_interface) {
+		chip->data_iface = kzalloc(sizeof(*chip->data_iface),
+					   GFP_KERNEL);
+		if (!chip->data_iface)
+			return -ENOMEM;
+
+		/*
+		 * The ONFI specification says:
+		 * "
+		 * To transition from NV-DDR or NV-DDR2 to the SDR data
+		 * interface, the host shall use the Reset (FFh) command
+		 * using SDR timing mode 0. A device in any timing mode is
+		 * required to recognize Reset (FFh) command issued in SDR
+		 * timing mode 0.
+		 * "
+		 *
+		 * Configure the data interface in SDR mode and set the
+		 * timings to timing mode 0. The Reset command is issued
+		 * in nand_get_flash_type().
+		 */
+
+		chip->data_iface->type = NAND_SDR_IFACE;
+		chip->data_iface->timings.sdr =
+				*onfi_async_timing_mode_to_sdr_timings(0);
+		ret = chip->setup_data_interface(mtd, chip->data_iface, false);
+		if (ret) {
+			pr_err("Failed to configure data interface to SDR timing mode 0\n");
+			goto err;
+		}
+	}
+
 	/* Read the flash type */
 	type = nand_get_flash_type(mtd, chip, &nand_maf_id,
 				   &nand_dev_id, table);
@@ -4041,7 +4219,9 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 		if (!(chip->options & NAND_SCAN_SILENT_NODEV))
 			pr_warn("No NAND device found\n");
 		chip->select_chip(mtd, -1);
-		return PTR_ERR(type);
+		kfree(chip->data_iface);
+		ret = PTR_ERR(type);
+		goto err;
 	}
 
 	chip->select_chip(mtd, -1);
@@ -4069,6 +4249,10 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	mtd->size = i * chip->chipsize;
 
 	return 0;
+
+err:
+	kfree(chip->data_iface);
+	return ret;
 }
 EXPORT_SYMBOL(nand_scan_ident);
 
@@ -4476,6 +4660,9 @@ void nand_release(struct mtd_info *mtd)
 
 	mtd_device_unregister(mtd);
 
+	/* Free interface config struct */
+	kfree(chip->data_iface);
+
 	/* Free bad block table memory */
 	kfree(chip->bbt);
 	if (!(chip->options & NAND_OWN_BUFFERS))
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 7ebd449..aee4c1e 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -558,6 +558,66 @@ struct nand_buffers {
 	uint8_t *databuf;
 };
 
+/*
+ * struct nand_sdr_timings - SDR NAND chip timings
+ *
+ * This struct defines the timing requirements of a SDR NAND chip.
+ * These information can be found in every NAND datasheets and the timings
+ * meaning are described in the ONFI specifications:
+ * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf (chapter 4.15 Timing
+ * Parameters)
+ *
+ * All these timings are expressed in picoseconds.
+ */
+
+struct nand_sdr_timings {
+	u32 tALH_min;
+	u32 tADL_min;
+	u32 tALS_min;
+	u32 tAR_min;
+	u32 tCEA_max;
+	u32 tCEH_min;
+	u32 tCH_min;
+	u32 tCHZ_max;
+	u32 tCLH_min;
+	u32 tCLR_min;
+	u32 tCLS_min;
+	u32 tCOH_min;
+	u32 tCS_min;
+	u32 tDH_min;
+	u32 tDS_min;
+	u32 tFEAT_max;
+	u32 tIR_min;
+	u32 tITC_max;
+	u32 tRC_min;
+	u32 tREA_max;
+	u32 tREH_min;
+	u32 tRHOH_min;
+	u32 tRHW_min;
+	u32 tRHZ_max;
+	u32 tRLOH_min;
+	u32 tRP_min;
+	u32 tRR_min;
+	u64 tRST_max;
+	u32 tWB_max;
+	u32 tWC_min;
+	u32 tWH_min;
+	u32 tWHR_min;
+	u32 tWP_min;
+	u32 tWW_min;
+};
+
+enum nand_data_interface_type {
+	NAND_SDR_IFACE,
+};
+
+struct nand_data_interface {
+	enum nand_data_interface_type type;
+	union {
+		struct nand_sdr_timings sdr;
+	} timings;
+};
+
 /**
  * struct nand_chip - NAND Private Flash Chip Data
  * @IO_ADDR_R:		[BOARDSPECIFIC] address to read the 8 I/O lines of the
@@ -690,6 +750,10 @@ struct nand_chip {
 	int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
 			int feature_addr, uint8_t *subfeature_para);
 	int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode);
+	int (*setup_data_interface)(struct mtd_info *mtd,
+				    const struct nand_data_interface *conf,
+				    bool check_only);
+
 
 	int chip_delay;
 	unsigned int options;
@@ -719,6 +783,8 @@ struct nand_chip {
 		struct nand_jedec_params jedec_params;
 	};
 
+	struct nand_data_interface *data_iface;
+
 	int read_retries;
 
 	flstate_t state;
@@ -993,55 +1059,6 @@ static inline int jedec_feature(struct nand_chip *chip)
 		: 0;
 }
 
-/*
- * struct nand_sdr_timings - SDR NAND chip timings
- *
- * This struct defines the timing requirements of a SDR NAND chip.
- * These informations can be found in every NAND datasheets and the timings
- * meaning are described in the ONFI specifications:
- * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf (chapter 4.15 Timing
- * Parameters)
- *
- * All these timings are expressed in picoseconds.
- */
-
-struct nand_sdr_timings {
-	u32 tALH_min;
-	u32 tADL_min;
-	u32 tALS_min;
-	u32 tAR_min;
-	u32 tCEA_max;
-	u32 tCEH_min;
-	u32 tCH_min;
-	u32 tCHZ_max;
-	u32 tCLH_min;
-	u32 tCLR_min;
-	u32 tCLS_min;
-	u32 tCOH_min;
-	u32 tCS_min;
-	u32 tDH_min;
-	u32 tDS_min;
-	u32 tFEAT_max;
-	u32 tIR_min;
-	u32 tITC_max;
-	u32 tRC_min;
-	u32 tREA_max;
-	u32 tREH_min;
-	u32 tRHOH_min;
-	u32 tRHW_min;
-	u32 tRHZ_max;
-	u32 tRLOH_min;
-	u32 tRP_min;
-	u32 tRR_min;
-	u64 tRST_max;
-	u32 tWB_max;
-	u32 tWC_min;
-	u32 tWH_min;
-	u32 tWHR_min;
-	u32 tWP_min;
-	u32 tWW_min;
-};
-
 /* get timing characteristics from ONFI timing mode. */
 const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
 
-- 
2.1.4

Re: [PATCH 1/2] mtd: nand: automate NAND timings selection

[Ezequiel Garcia]

Hi Boris,

This looks nice. I gave a try at this patch and it allows to simplify
pxa3xx-nand greatly. A few comments below.

On 23 Oct 01:03 PM, Boris Brezillon wrote:
> The NAND framework provides several helpers to query timing modes supported
> by a NAND chip, but this implies that all NAND controller drivers have
> to implement the same timings selection dance.
> 
> Provide a common logic to select the best timings based on ONFI or
> ->onfi_timing_mode_default information.
> NAND controller willing to support timings adjustment should just
> implement the ->setup_data_interface() method.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c | 189 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/nand.h     | 115 +++++++++++++++-----------
>  2 files changed, 254 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index eaf1fcd..52a1f89 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3323,6 +3323,144 @@ static void nand_onfi_detect_micron(struct nand_chip *chip,
>  	chip->setup_read_retry = nand_setup_read_retry_micron;
>  }
>  
> +/**
> + * nand_setup_data_interface - Setup the data interface and timings on the
> + *			       controller side
> + * @mtd: MTD device structure
> + * @conf: new configuration to apply
> + *
> + * Try to configure the NAND controller to support the provided data
> + * interface configuration.
> + *
> + * Returns 0 in case of success or -ERROR_CODE.
> + */
> +static int nand_setup_data_interface(struct mtd_info *mtd,
> +				     const struct nand_data_interface *conf)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	int ret;
> +
> +	if (!chip->setup_data_interface)
> +		return -ENOTSUPP;
> +
> +	ret = chip->setup_data_interface(mtd, conf, false);
> +	if (ret)
> +		return ret;
> +
> +	*chip->data_iface = *conf;
> +
> +	return 0;
> +}
> +
> +/**
> + * nand_setup_data_interface - Check if a data interface config is supported

s/setup/check

> + *			       by the NAND controller
> + * @mtd: MTD device structure
> + * @conf: new configuration to apply
> + *
> + * Check if the provided data interface configuration is supported by the
> + * NAND controller.
> + *
> + * Returns 0 if it is supported or -ERROR_CODE.
> + */
> +static int nand_check_data_interface(struct mtd_info *mtd,
> +				     const struct nand_data_interface *conf)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +
> +	if (!chip->setup_data_interface)
> +		return -ENOTSUPP;
> +
> +	return chip->setup_data_interface(mtd, conf, true);
> +}
> +
> +/**
> + * nand_configure_data_interface - Configure the data interface and timings
> + * @mtd: MTD device structure
> + *
> + * Try to configure the data interface and NAND timings appropriately.
> + * First tries to retrieve supported timing modes from ONFI information,
> + * and if the NAND chip does not support ONFI, relies on the
> + * ->onfi_timing_mode_default specified in the nand_ids table.
> + *
> + * Returns 0 in case of success or -ERROR_CODE.
> + */
> +static int nand_configure_data_interface(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd->priv;
> +	struct nand_data_interface *conf;
> +	int modes, mode, ret = -EINVAL;
> +
> +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> +	if (!conf)
> +		return -ENOMEM;
> +
> +	/* TODO: support DDR interfaces */
> +	conf->type = NAND_SDR_IFACE;
> +
> +	/*
> +	 * First try to identify the best timings from ONFI parameters and
> +	 * if the NAND does not support ONFI, fallback to the default ONFI
> +	 * timing mode.
> +	 */
> +	modes = onfi_get_async_timing_mode(chip);
> +	if (modes != ONFI_TIMING_MODE_UNKNOWN) {
> +		for (mode = fls(modes) - 1; mode >= 0; mode--) {
> +			conf->timings.sdr =
> +				*onfi_async_timing_mode_to_sdr_timings(mode);
> +
> +			ret = nand_check_data_interface(mtd, conf);
> +			if (!ret)
> +				break;
> +		}
> +	} else {
> +		mode = chip->onfi_timing_mode_default;
> +		conf->timings.sdr =
> +				*onfi_async_timing_mode_to_sdr_timings(mode);
> +
> +		ret = nand_check_data_interface(mtd, conf);
> +	}
> +
> +	if (!ret) {
> +		uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { mode };
> +		int i;
> +
> +		/*
> +		 * Ensure the timing mode has be changed on the chip side
> +		 * before changing timings on the controller side.
> +		 */
> +		if (modes != ONFI_TIMING_MODE_UNKNOWN) {
> +			/*
> +			 * FIXME: should we really set the timing mode on all
> +			 * dies?
> +			 */
> +			for (i = 0; i < chip->numchips; i++) {
> +				chip->select_chip(mtd, i);
> +				ret = chip->onfi_set_features(mtd, chip,
> +						ONFI_FEATURE_ADDR_TIMING_MODE,
> +						tmode_param);
> +			}
> +			chip->select_chip(mtd, -1);
> +		}
> +
> +		if (!ret) {
> +			ret = nand_setup_data_interface(mtd, conf);
> +
> +			/*
> +			 * Reset the NAND chip if the data interface setup
> +			 * failed so that the chip goes back to a known state
> +			 * (timing mode 0).
> +			 */
> +			if (ret)
> +				chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +		}
> +	}
> +
> +	kfree(conf);
> +
> +	return ret;
> +}
> +
>  /*
>   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
>   */
> @@ -3906,6 +4044,11 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
>  		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
>  ident_done:
>  
> +	/*
> +	 * Setup the NAND interface (interface type + timings).
> +	 */
> +	nand_configure_data_interface(mtd);
> +

Need to check the returned value.

Also, it doesn't feel right to configure the timings in nand_get_flash_type.
How about something like this:

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 947e74d24ee8..5fae81101c32 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4009,11 +4009,6 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
 ident_done:
 
-	/*
-	 * Setup the NAND interface (interface type + timings).
-	 */
-	nand_configure_data_interface(mtd);
-
 	/* Try to identify manufacturer */
 	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
 		if (nand_manuf_ids[maf_idx].id == *maf_id)
@@ -4189,6 +4184,13 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 		goto err;
 	}
 
+	/*
+	 * Setup the NAND interface (interface type + timings).
+	 */
+	ret = nand_configure_data_interface(mtd);
+	if (ret)
+		return ret;
+
 	chip->select_chip(mtd, -1);
[..]
 

>  	/* Try to identify manufacturer */
>  	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
>  		if (nand_manuf_ids[maf_idx].id == *maf_id)
> @@ -4033,6 +4176,41 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	/* Set the default functions */
>  	nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
>  
> +	/*
> +	 * Allocate an interface config struct if the controller implements the
> +	 * ->apply_interface_conf() method.
> +	 */
> +	if (chip->setup_data_interface) {
> +		chip->data_iface = kzalloc(sizeof(*chip->data_iface),
> +					   GFP_KERNEL);
> +		if (!chip->data_iface)
> +			return -ENOMEM;
> +
> +		/*
> +		 * The ONFI specification says:
> +		 * "
> +		 * To transition from NV-DDR or NV-DDR2 to the SDR data
> +		 * interface, the host shall use the Reset (FFh) command
> +		 * using SDR timing mode 0. A device in any timing mode is
> +		 * required to recognize Reset (FFh) command issued in SDR
> +		 * timing mode 0.
> +		 * "
> +		 *
> +		 * Configure the data interface in SDR mode and set the
> +		 * timings to timing mode 0. The Reset command is issued
> +		 * in nand_get_flash_type().
> +		 */
> +
> +		chip->data_iface->type = NAND_SDR_IFACE;
> +		chip->data_iface->timings.sdr =
> +				*onfi_async_timing_mode_to_sdr_timings(0);
> +		ret = chip->setup_data_interface(mtd, chip->data_iface, false);
> +		if (ret) {
> +			pr_err("Failed to configure data interface to SDR timing mode 0\n");
> +			goto err;
> +		}
> +	}
> +
>  	/* Read the flash type */
>  	type = nand_get_flash_type(mtd, chip, &nand_maf_id,
>  				   &nand_dev_id, table);
> @@ -4041,7 +4219,9 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  		if (!(chip->options & NAND_SCAN_SILENT_NODEV))
>  			pr_warn("No NAND device found\n");
>  		chip->select_chip(mtd, -1);
> -		return PTR_ERR(type);
> +		kfree(chip->data_iface);

You free data_iface here...

> +		ret = PTR_ERR(type);
> +		goto err;
>  	}
>  
>  	chip->select_chip(mtd, -1);
> @@ -4069,6 +4249,10 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	mtd->size = i * chip->chipsize;
>  
>  	return 0;
> +
> +err:
> +	kfree(chip->data_iface);

...and again here.

> +	return ret;
>  }
>  EXPORT_SYMBOL(nand_scan_ident);
>  
> @@ -4476,6 +4660,9 @@ void nand_release(struct mtd_info *mtd)
>  
>  	mtd_device_unregister(mtd);
>  
> +	/* Free interface config struct */
> +	kfree(chip->data_iface);
> +
>  	/* Free bad block table memory */
>  	kfree(chip->bbt);
>  	if (!(chip->options & NAND_OWN_BUFFERS))
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
[..]
>  /**
>   * struct nand_chip - NAND Private Flash Chip Data
>   * @IO_ADDR_R:		[BOARDSPECIFIC] address to read the 8 I/O lines of the
> @@ -690,6 +750,10 @@ struct nand_chip {
>  	int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
>  			int feature_addr, uint8_t *subfeature_para);
>  	int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode);
> +	int (*setup_data_interface)(struct mtd_info *mtd,
> +				    const struct nand_data_interface *conf,
> +				    bool check_only);

The function lacks its proper documentation in the comment above
the struct.

-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 1/2] mtd: nand: automate NAND timings selection

[Boris Brezillon]

Hi Ezequiel,

On Sun, 1 Nov 2015 21:37:33 -0300
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> wrote:

> Hi Boris,
> 
> This looks nice. I gave a try at this patch and it allows to simplify
> pxa3xx-nand greatly. A few comments below.
> 
> On 23 Oct 01:03 PM, Boris Brezillon wrote:
> > The NAND framework provides several helpers to query timing modes supported
> > by a NAND chip, but this implies that all NAND controller drivers have
> > to implement the same timings selection dance.
> > 
> > Provide a common logic to select the best timings based on ONFI or
> > ->onfi_timing_mode_default information.
> > NAND controller willing to support timings adjustment should just
> > implement the ->setup_data_interface() method.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> > ---
> >  drivers/mtd/nand/nand_base.c | 189 ++++++++++++++++++++++++++++++++++++++++++-
> >  include/linux/mtd/nand.h     | 115 +++++++++++++++-----------
> >  2 files changed, 254 insertions(+), 50 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index eaf1fcd..52a1f89 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -3323,6 +3323,144 @@ static void nand_onfi_detect_micron(struct nand_chip *chip,
> >  	chip->setup_read_retry = nand_setup_read_retry_micron;
> >  }
> >  
> > +/**
> > + * nand_setup_data_interface - Setup the data interface and timings on the
> > + *			       controller side
> > + * @mtd: MTD device structure
> > + * @conf: new configuration to apply
> > + *
> > + * Try to configure the NAND controller to support the provided data
> > + * interface configuration.
> > + *
> > + * Returns 0 in case of success or -ERROR_CODE.
> > + */
> > +static int nand_setup_data_interface(struct mtd_info *mtd,
> > +				     const struct nand_data_interface *conf)
> > +{
> > +	struct nand_chip *chip = mtd->priv;
> > +	int ret;
> > +
> > +	if (!chip->setup_data_interface)
> > +		return -ENOTSUPP;
> > +
> > +	ret = chip->setup_data_interface(mtd, conf, false);
> > +	if (ret)
> > +		return ret;
> > +
> > +	*chip->data_iface = *conf;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * nand_setup_data_interface - Check if a data interface config is supported
> 
> s/setup/check

Yes, I'll fix it.

> 
> > + *			       by the NAND controller
> > + * @mtd: MTD device structure
> > + * @conf: new configuration to apply
> > + *
> > + * Check if the provided data interface configuration is supported by the
> > + * NAND controller.
> > + *
> > + * Returns 0 if it is supported or -ERROR_CODE.
> > + */
> > +static int nand_check_data_interface(struct mtd_info *mtd,
> > +				     const struct nand_data_interface *conf)
> > +{
> > +	struct nand_chip *chip = mtd->priv;
> > +
> > +	if (!chip->setup_data_interface)
> > +		return -ENOTSUPP;
> > +
> > +	return chip->setup_data_interface(mtd, conf, true);
> > +}
> > +
> > +/**
> > + * nand_configure_data_interface - Configure the data interface and timings
> > + * @mtd: MTD device structure
> > + *
> > + * Try to configure the data interface and NAND timings appropriately.
> > + * First tries to retrieve supported timing modes from ONFI information,
> > + * and if the NAND chip does not support ONFI, relies on the
> > + * ->onfi_timing_mode_default specified in the nand_ids table.
> > + *
> > + * Returns 0 in case of success or -ERROR_CODE.
> > + */
> > +static int nand_configure_data_interface(struct mtd_info *mtd)
> > +{
> > +	struct nand_chip *chip = mtd->priv;
> > +	struct nand_data_interface *conf;
> > +	int modes, mode, ret = -EINVAL;
> > +
> > +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > +	if (!conf)
> > +		return -ENOMEM;
> > +
> > +	/* TODO: support DDR interfaces */
> > +	conf->type = NAND_SDR_IFACE;
> > +
> > +	/*
> > +	 * First try to identify the best timings from ONFI parameters and
> > +	 * if the NAND does not support ONFI, fallback to the default ONFI
> > +	 * timing mode.
> > +	 */
> > +	modes = onfi_get_async_timing_mode(chip);
> > +	if (modes != ONFI_TIMING_MODE_UNKNOWN) {
> > +		for (mode = fls(modes) - 1; mode >= 0; mode--) {
> > +			conf->timings.sdr =
> > +				*onfi_async_timing_mode_to_sdr_timings(mode);
> > +
> > +			ret = nand_check_data_interface(mtd, conf);
> > +			if (!ret)
> > +				break;
> > +		}
> > +	} else {
> > +		mode = chip->onfi_timing_mode_default;
> > +		conf->timings.sdr =
> > +				*onfi_async_timing_mode_to_sdr_timings(mode);
> > +
> > +		ret = nand_check_data_interface(mtd, conf);
> > +	}
> > +
> > +	if (!ret) {
> > +		uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { mode };
> > +		int i;
> > +
> > +		/*
> > +		 * Ensure the timing mode has be changed on the chip side
> > +		 * before changing timings on the controller side.
> > +		 */
> > +		if (modes != ONFI_TIMING_MODE_UNKNOWN) {
> > +			/*
> > +			 * FIXME: should we really set the timing mode on all
> > +			 * dies?
> > +			 */
> > +			for (i = 0; i < chip->numchips; i++) {
> > +				chip->select_chip(mtd, i);
> > +				ret = chip->onfi_set_features(mtd, chip,
> > +						ONFI_FEATURE_ADDR_TIMING_MODE,
> > +						tmode_param);
> > +			}
> > +			chip->select_chip(mtd, -1);
> > +		}
> > +
> > +		if (!ret) {
> > +			ret = nand_setup_data_interface(mtd, conf);
> > +
> > +			/*
> > +			 * Reset the NAND chip if the data interface setup
> > +			 * failed so that the chip goes back to a known state
> > +			 * (timing mode 0).
> > +			 */
> > +			if (ret)
> > +				chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> > +		}
> > +	}
> > +
> > +	kfree(conf);
> > +
> > +	return ret;
> > +}
> > +
> >  /*
> >   * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
> >   */
> > @@ -3906,6 +4044,11 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
> >  		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
> >  ident_done:
> >  
> > +	/*
> > +	 * Setup the NAND interface (interface type + timings).
> > +	 */
> > +	nand_configure_data_interface(mtd);
> > +
> 
> Need to check the returned value.

The reason I don't check the return value here is because the
->setup_data_interface() function is not mandatory, and
nand_configure_data_interface() will return -ENOTSUPP if it's not
implemented. But maybe we should check the return value and avoid
calling this function if chip->setup_data_interface() is not
implemented.

> 
> Also, it doesn't feel right to configure the timings in nand_get_flash_type.
> How about something like this:

Yes, it's probably a better place.

> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 947e74d24ee8..5fae81101c32 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4009,11 +4009,6 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
>  		chip->options &= ~NAND_SAMSUNG_LP_OPTIONS;
>  ident_done:
>  
> -	/*
> -	 * Setup the NAND interface (interface type + timings).
> -	 */
> -	nand_configure_data_interface(mtd);
> -
>  	/* Try to identify manufacturer */
>  	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
>  		if (nand_manuf_ids[maf_idx].id == *maf_id)
> @@ -4189,6 +4184,13 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  		goto err;
>  	}
>  
> +	/*
> +	 * Setup the NAND interface (interface type + timings).
> +	 */
> +	ret = nand_configure_data_interface(mtd);
> +	if (ret)
> +		return ret;
> +
>  	chip->select_chip(mtd, -1);
> [..]
>  
> 
> >  	/* Try to identify manufacturer */
> >  	for (maf_idx = 0; nand_manuf_ids[maf_idx].id != 0x0; maf_idx++) {
> >  		if (nand_manuf_ids[maf_idx].id == *maf_id)
> > @@ -4033,6 +4176,41 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> >  	/* Set the default functions */
> >  	nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
> >  
> > +	/*
> > +	 * Allocate an interface config struct if the controller implements the
> > +	 * ->apply_interface_conf() method.
> > +	 */
> > +	if (chip->setup_data_interface) {
> > +		chip->data_iface = kzalloc(sizeof(*chip->data_iface),
> > +					   GFP_KERNEL);
> > +		if (!chip->data_iface)
> > +			return -ENOMEM;
> > +
> > +		/*
> > +		 * The ONFI specification says:
> > +		 * "
> > +		 * To transition from NV-DDR or NV-DDR2 to the SDR data
> > +		 * interface, the host shall use the Reset (FFh) command
> > +		 * using SDR timing mode 0. A device in any timing mode is
> > +		 * required to recognize Reset (FFh) command issued in SDR
> > +		 * timing mode 0.
> > +		 * "
> > +		 *
> > +		 * Configure the data interface in SDR mode and set the
> > +		 * timings to timing mode 0. The Reset command is issued
> > +		 * in nand_get_flash_type().
> > +		 */
> > +
> > +		chip->data_iface->type = NAND_SDR_IFACE;
> > +		chip->data_iface->timings.sdr =
> > +				*onfi_async_timing_mode_to_sdr_timings(0);
> > +		ret = chip->setup_data_interface(mtd, chip->data_iface, false);
> > +		if (ret) {
> > +			pr_err("Failed to configure data interface to SDR timing mode 0\n");
> > +			goto err;
> > +		}
> > +	}
> > +
> >  	/* Read the flash type */
> >  	type = nand_get_flash_type(mtd, chip, &nand_maf_id,
> >  				   &nand_dev_id, table);
> > @@ -4041,7 +4219,9 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> >  		if (!(chip->options & NAND_SCAN_SILENT_NODEV))
> >  			pr_warn("No NAND device found\n");
> >  		chip->select_chip(mtd, -1);
> > -		return PTR_ERR(type);
> > +		kfree(chip->data_iface);
> 
> You free data_iface here...

Oops, probably a leftover of a previous version where I was not freeing
chip->data_iface in the error path.
I'll fix that.

> 
> > +		ret = PTR_ERR(type);
> > +		goto err;
> >  	}
> >  
> >  	chip->select_chip(mtd, -1);
> > @@ -4069,6 +4249,10 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
> >  	mtd->size = i * chip->chipsize;
> >  
> >  	return 0;
> > +
> > +err:
> > +	kfree(chip->data_iface);
> 
> ...and again here.
> 
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL(nand_scan_ident);
> >  
> > @@ -4476,6 +4660,9 @@ void nand_release(struct mtd_info *mtd)
> >  
> >  	mtd_device_unregister(mtd);
> >  
> > +	/* Free interface config struct */
> > +	kfree(chip->data_iface);
> > +
> >  	/* Free bad block table memory */
> >  	kfree(chip->bbt);
> >  	if (!(chip->options & NAND_OWN_BUFFERS))
> > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> [..]
> >  /**
> >   * struct nand_chip - NAND Private Flash Chip Data
> >   * @IO_ADDR_R:		[BOARDSPECIFIC] address to read the 8 I/O lines of the
> > @@ -690,6 +750,10 @@ struct nand_chip {
> >  	int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
> >  			int feature_addr, uint8_t *subfeature_para);
> >  	int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode);
> > +	int (*setup_data_interface)(struct mtd_info *mtd,
> > +				    const struct nand_data_interface *conf,
> > +				    bool check_only);
> 
> The function lacks its proper documentation in the comment above
> the struct.
> 

Yep, I'll document this new function.

Thanks for your review.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH 2/2] mtd: nand: sunxi: switch from manual to automated timing config

[Boris Brezillon]

The NAND framework is now able to select the best NAND timings for us.
All we have to do is implement a ->setup_data_interface() function to
apply those timings and remove the timing selection code from the sunxi
driver.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/sunxi_nand.c | 72 ++++++++++---------------------------------
 1 file changed, 16 insertions(+), 56 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
index b657235..b6f21f4 100644
--- a/drivers/mtd/nand/sunxi_nand.c
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -1131,7 +1131,8 @@ static int _sunxi_nand_lookup_timing(const s32 *lut, int lut_size, u32 duration,
 			_sunxi_nand_lookup_timing(l, ARRAY_SIZE(l), p, c)
 
 static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
-				       const struct nand_sdr_timings *timings)
+				       const struct nand_sdr_timings *timings,
+				       bool check_only)
 {
 	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->nand.controller);
 	u32 min_clk_period = 0;
@@ -1224,6 +1225,9 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
 		return tRHW;
 	}
 
+	if (check_only)
+		return 0;
+
 	/*
 	 * TODO: according to ONFI specs this value only applies for DDR NAND,
 	 * but Allwinner seems to set this to 0x7. Mimic them for now.
@@ -1254,42 +1258,19 @@ static int sunxi_nand_chip_set_timings(struct sunxi_nand_chip *chip,
 	return 0;
 }
 
-static int sunxi_nand_chip_init_timings(struct sunxi_nand_chip *chip,
-					struct device_node *np)
+static int sunxi_nand_setup_data_interface(struct mtd_info *mtd,
+					const struct nand_data_interface *conf,
+					bool check_only)
 {
-	const struct nand_sdr_timings *timings;
-	int ret;
-	int mode;
-
-	mode = onfi_get_async_timing_mode(&chip->nand);
-	if (mode == ONFI_TIMING_MODE_UNKNOWN) {
-		mode = chip->nand.onfi_timing_mode_default;
-	} else {
-		uint8_t feature[ONFI_SUBFEATURE_PARAM_LEN] = {};
-		int i;
-
-		mode = fls(mode) - 1;
-		if (mode < 0)
-			mode = 0;
-
-		feature[0] = mode;
-		for (i = 0; i < chip->nsels; i++) {
-			chip->nand.select_chip(&chip->mtd, i);
-			ret = chip->nand.onfi_set_features(&chip->mtd,
-						&chip->nand,
-						ONFI_FEATURE_ADDR_TIMING_MODE,
-						feature);
-			chip->nand.select_chip(&chip->mtd, -1);
-			if (ret)
-				return ret;
-		}
-	}
+	struct nand_chip *nand = mtd->priv;
+	struct sunxi_nand_chip *sunxi_nand = to_sunxi_nand(nand);
 
-	timings = onfi_async_timing_mode_to_sdr_timings(mode);
-	if (IS_ERR(timings))
-		return PTR_ERR(timings);
+	/* TODO: add support for DDR NANDs */
+	if (conf->type != NAND_SDR_IFACE)
+		return -ENOTSUPP;
 
-	return sunxi_nand_chip_set_timings(chip, timings);
+	return sunxi_nand_chip_set_timings(sunxi_nand, &conf->timings.sdr,
+					   check_only);
 }
 
 static int sunxi_nand_hw_common_ecc_ctrl_init(struct mtd_info *mtd,
@@ -1502,7 +1483,6 @@ static int sunxi_nand_ecc_init(struct mtd_info *mtd, struct nand_ecc_ctrl *ecc,
 static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
 				struct device_node *np)
 {
-	const struct nand_sdr_timings *timings;
 	struct sunxi_nand_chip *chip;
 	struct mtd_part_parser_data ppdata;
 	struct mtd_info *mtd;
@@ -1578,21 +1558,6 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
 		}
 	}
 
-	timings = onfi_async_timing_mode_to_sdr_timings(0);
-	if (IS_ERR(timings)) {
-		ret = PTR_ERR(timings);
-		dev_err(dev,
-			"could not retrieve timings for ONFI mode 0: %d\n",
-			ret);
-		return ret;
-	}
-
-	ret = sunxi_nand_chip_set_timings(chip, timings);
-	if (ret) {
-		dev_err(dev, "could not configure chip timings: %d\n", ret);
-		return ret;
-	}
-
 	nand = &chip->nand;
 	/* Default tR value specified in the ONFI spec (chapter 4.15.1) */
 	nand->chip_delay = 200;
@@ -1608,6 +1573,7 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
 	nand->read_buf = sunxi_nfc_read_buf;
 	nand->write_buf = sunxi_nfc_write_buf;
 	nand->read_byte = sunxi_nfc_read_byte;
+	nand->setup_data_interface = sunxi_nand_setup_data_interface;
 
 	mtd = &chip->mtd;
 	mtd->dev.parent = dev;
@@ -1625,12 +1591,6 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
 		nand->bbt_options |= NAND_BBT_SCANRAWMODE;
 	}
 
-	ret = sunxi_nand_chip_init_timings(chip, np);
-	if (ret) {
-		dev_err(dev, "could not configure chip timings: %d\n", ret);
-		return ret;
-	}
-
 	ret = sunxi_nand_ecc_init(mtd, &nand->ecc, np);
 	if (ret) {
 		dev_err(dev, "ECC init failed: %d\n", ret);
-- 
2.1.4

mtd: nand: automate NAND timings selection

[Sascha Hauer]

This series aims at automating the NAND timings selection which is
currently supposed to be done in each NAND controller driver, thus
simplifying drivers implementation.

Patch 1 also opens the door to DDR NAND support, though setting DDR
timings is currently not supported.

I picked this up from earlier work from Boris Brezillon
(https://lkml.org/lkml/2015/10/23/179). Changes since the initial
posting from Boris:

- Integrate Feedback from Ezequiel Garcia
- When iterating over the chips calling onfi_set_features() for each
  bail out when any of the calls fail, not only the last one.
- When one of the onfi_set_features() calls fail then reset the chip
  afterwards.
- Drop Sunxi example, add patch for the mxc_nand controller instead.

Sascha

[PATCH 1/2] mtd: nand: automate NAND timings selection

[Sascha Hauer]

From: Boris Brezillon <boris.brezillon@free-electrons.com>

The NAND framework provides several helpers to query timing modes supported
by a NAND chip, but this implies that all NAND controller drivers have
to implement the same timings selection dance.

Provide a common logic to select the best timings based on ONFI or
->onfi_timing_mode_default information.
NAND controller willing to support timings adjustment should just
implement the ->setup_data_interface() method.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/nand_base.c | 196 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/nand.h     | 115 ++++++++++++++-----------
 2 files changed, 261 insertions(+), 50 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 77533f7..018fd56 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3322,6 +3322,148 @@ static void nand_onfi_detect_micron(struct nand_chip *chip,
 	chip->setup_read_retry = nand_setup_read_retry_micron;
 }
 
+/**
+ * nand_setup_data_interface - Setup the data interface and timings on the
+ *			       controller side
+ * @mtd: MTD device structure
+ * @conf: new configuration to apply
+ *
+ * Try to configure the NAND controller to support the provided data
+ * interface configuration.
+ *
+ * Returns 0 in case of success or -ERROR_CODE.
+ */
+static int nand_setup_data_interface(struct mtd_info *mtd,
+				     const struct nand_data_interface *conf)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	int ret;
+
+	if (!chip->setup_data_interface)
+		return -ENOTSUPP;
+
+	ret = chip->setup_data_interface(mtd, conf, false);
+	if (ret)
+		return ret;
+
+	*chip->data_iface = *conf;
+
+	return 0;
+}
+
+/**
+ * nand_check_data_interface - Check if a data interface config is supported
+ *			       by the NAND controller
+ * @mtd: MTD device structure
+ * @conf: new configuration to apply
+ *
+ * Check if the provided data interface configuration is supported by the
+ * NAND controller.
+ *
+ * Returns 0 if it is supported or -ERROR_CODE.
+ */
+static int nand_check_data_interface(struct mtd_info *mtd,
+				     const struct nand_data_interface *conf)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+
+	if (!chip->setup_data_interface)
+		return -ENOTSUPP;
+
+	return chip->setup_data_interface(mtd, conf, true);
+}
+
+/**
+ * nand_configure_data_interface - Configure the data interface and timings
+ * @mtd: MTD device structure
+ *
+ * Try to configure the data interface and NAND timings appropriately.
+ * First tries to retrieve supported timing modes from ONFI information,
+ * and if the NAND chip does not support ONFI, relies on the
+ * ->onfi_timing_mode_default specified in the nand_ids table.
+ *
+ * Returns 0 in case of success or -ERROR_CODE.
+ */
+static int nand_configure_data_interface(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct nand_data_interface *conf;
+	int modes, mode, ret = -EINVAL;
+	uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { };
+	int i;
+
+	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
+	if (!conf)
+		return -ENOMEM;
+
+	/* TODO: support DDR interfaces */
+	conf->type = NAND_SDR_IFACE;
+
+	/*
+	 * First try to identify the best timings from ONFI parameters and
+	 * if the NAND does not support ONFI, fallback to the default ONFI
+	 * timing mode.
+	 */
+	modes = onfi_get_async_timing_mode(chip);
+	if (modes == ONFI_TIMING_MODE_UNKNOWN) {
+		mode = chip->onfi_timing_mode_default;
+		conf->timings.sdr =
+				*onfi_async_timing_mode_to_sdr_timings(mode);
+
+		ret = nand_check_data_interface(mtd, conf);
+	} else {
+		for (mode = fls(modes) - 1; mode >= 0; mode--) {
+			conf->timings.sdr =
+				*onfi_async_timing_mode_to_sdr_timings(mode);
+
+			ret = nand_check_data_interface(mtd, conf);
+			if (!ret)
+				break;
+		}
+	}
+
+	if (ret)
+		goto out;
+
+	tmode_param[0] = mode;
+
+	/*
+	 * Ensure the timing mode has be changed on the chip side
+	 * before changing timings on the controller side.
+	 */
+	if (modes != ONFI_TIMING_MODE_UNKNOWN) {
+		/*
+		 * FIXME: should we really set the timing mode on all
+		 * dies?
+		 */
+		for (i = 0; i < chip->numchips; i++) {
+			chip->select_chip(mtd, i);
+			ret = chip->onfi_set_features(mtd, chip,
+					ONFI_FEATURE_ADDR_TIMING_MODE,
+					tmode_param);
+			if (ret)
+				goto out_reset;
+		}
+		chip->select_chip(mtd, -1);
+	}
+
+	ret = nand_setup_data_interface(mtd, conf);
+
+out_reset:
+	/*
+	 * Reset the NAND chip if the data interface setup
+	 * failed so that the chip goes back to a known state
+	 * (timing mode 0).
+	 */
+	if (ret)
+		chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
+
+out:
+	kfree(conf);
+
+	return ret;
+}
+
 /*
  * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise.
  */
@@ -4144,6 +4286,41 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	/* Set the default functions */
 	nand_set_defaults(chip, chip->options & NAND_BUSWIDTH_16);
 
+	/*
+	 * Allocate an interface config struct if the controller implements the
+	 * ->apply_interface_conf() method.
+	 */
+	if (chip->setup_data_interface) {
+		chip->data_iface = kzalloc(sizeof(*chip->data_iface),
+					   GFP_KERNEL);
+		if (!chip->data_iface)
+			return -ENOMEM;
+
+		/*
+		 * The ONFI specification says:
+		 * "
+		 * To transition from NV-DDR or NV-DDR2 to the SDR data
+		 * interface, the host shall use the Reset (FFh) command
+		 * using SDR timing mode 0. A device in any timing mode is
+		 * required to recognize Reset (FFh) command issued in SDR
+		 * timing mode 0.
+		 * "
+		 *
+		 * Configure the data interface in SDR mode and set the
+		 * timings to timing mode 0. The Reset command is issued
+		 * in nand_get_flash_type().
+		 */
+
+		chip->data_iface->type = NAND_SDR_IFACE;
+		chip->data_iface->timings.sdr =
+				*onfi_async_timing_mode_to_sdr_timings(0);
+		ret = chip->setup_data_interface(mtd, chip->data_iface, false);
+		if (ret) {
+			pr_err("Failed to configure data interface to SDR timing mode 0\n");
+			goto err;
+		}
+	}
+
 	/* Read the flash type */
 	type = nand_get_flash_type(mtd, chip, &nand_maf_id,
 				   &nand_dev_id, table);
@@ -4152,7 +4329,17 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 		if (!(chip->options & NAND_SCAN_SILENT_NODEV))
 			pr_warn("No NAND device found\n");
 		chip->select_chip(mtd, -1);
-		return PTR_ERR(type);
+		ret = PTR_ERR(type);
+		goto err;
+	}
+
+	/*
+	 * Setup the NAND interface (interface type + timings).
+	 */
+	if (chip->setup_data_interface) {
+		ret = nand_configure_data_interface(mtd);
+		if (ret)
+			goto err;
 	}
 
 	chip->select_chip(mtd, -1);
@@ -4180,6 +4367,10 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	mtd->size = i * chip->chipsize;
 
 	return 0;
+
+err:
+	kfree(chip->data_iface);
+	return ret;
 }
 EXPORT_SYMBOL(nand_scan_ident);
 
@@ -4614,6 +4805,9 @@ void nand_release(struct mtd_info *mtd)
 
 	mtd_device_unregister(mtd);
 
+	/* Free interface config struct */
+	kfree(chip->data_iface);
+
 	/* Free bad block table memory */
 	kfree(chip->bbt);
 	if (!(chip->options & NAND_OWN_BUFFERS))
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 8dd6e01..7493215 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -565,6 +565,66 @@ struct nand_buffers {
 	uint8_t *databuf;
 };
 
+/*
+ * struct nand_sdr_timings - SDR NAND chip timings
+ *
+ * This struct defines the timing requirements of a SDR NAND chip.
+ * These information can be found in every NAND datasheets and the timings
+ * meaning are described in the ONFI specifications:
+ * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf (chapter 4.15 Timing
+ * Parameters)
+ *
+ * All these timings are expressed in picoseconds.
+ */
+
+struct nand_sdr_timings {
+	u32 tALH_min;
+	u32 tADL_min;
+	u32 tALS_min;
+	u32 tAR_min;
+	u32 tCEA_max;
+	u32 tCEH_min;
+	u32 tCH_min;
+	u32 tCHZ_max;
+	u32 tCLH_min;
+	u32 tCLR_min;
+	u32 tCLS_min;
+	u32 tCOH_min;
+	u32 tCS_min;
+	u32 tDH_min;
+	u32 tDS_min;
+	u32 tFEAT_max;
+	u32 tIR_min;
+	u32 tITC_max;
+	u32 tRC_min;
+	u32 tREA_max;
+	u32 tREH_min;
+	u32 tRHOH_min;
+	u32 tRHW_min;
+	u32 tRHZ_max;
+	u32 tRLOH_min;
+	u32 tRP_min;
+	u32 tRR_min;
+	u64 tRST_max;
+	u32 tWB_max;
+	u32 tWC_min;
+	u32 tWH_min;
+	u32 tWHR_min;
+	u32 tWP_min;
+	u32 tWW_min;
+};
+
+enum nand_data_interface_type {
+	NAND_SDR_IFACE,
+};
+
+struct nand_data_interface {
+	enum nand_data_interface_type type;
+	union {
+		struct nand_sdr_timings sdr;
+	} timings;
+};
+
 /**
  * struct nand_chip - NAND Private Flash Chip Data
  * @mtd:		MTD device registered to the MTD framework
@@ -696,6 +756,10 @@ struct nand_chip {
 	int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip *chip,
 			int feature_addr, uint8_t *subfeature_para);
 	int (*setup_read_retry)(struct mtd_info *mtd, int retry_mode);
+	int (*setup_data_interface)(struct mtd_info *mtd,
+				    const struct nand_data_interface *conf,
+				    bool check_only);
+
 
 	int chip_delay;
 	unsigned int options;
@@ -725,6 +789,8 @@ struct nand_chip {
 		struct nand_jedec_params jedec_params;
 	};
 
+	struct nand_data_interface *data_iface;
+
 	int read_retries;
 
 	flstate_t state;
@@ -1023,55 +1089,6 @@ static inline int jedec_feature(struct nand_chip *chip)
 		: 0;
 }
 
-/*
- * struct nand_sdr_timings - SDR NAND chip timings
- *
- * This struct defines the timing requirements of a SDR NAND chip.
- * These informations can be found in every NAND datasheets and the timings
- * meaning are described in the ONFI specifications:
- * www.onfi.org/~/media/ONFI/specs/onfi_3_1_spec.pdf (chapter 4.15 Timing
- * Parameters)
- *
- * All these timings are expressed in picoseconds.
- */
-
-struct nand_sdr_timings {
-	u32 tALH_min;
-	u32 tADL_min;
-	u32 tALS_min;
-	u32 tAR_min;
-	u32 tCEA_max;
-	u32 tCEH_min;
-	u32 tCH_min;
-	u32 tCHZ_max;
-	u32 tCLH_min;
-	u32 tCLR_min;
-	u32 tCLS_min;
-	u32 tCOH_min;
-	u32 tCS_min;
-	u32 tDH_min;
-	u32 tDS_min;
-	u32 tFEAT_max;
-	u32 tIR_min;
-	u32 tITC_max;
-	u32 tRC_min;
-	u32 tREA_max;
-	u32 tREH_min;
-	u32 tRHOH_min;
-	u32 tRHW_min;
-	u32 tRHZ_max;
-	u32 tRLOH_min;
-	u32 tRP_min;
-	u32 tRR_min;
-	u64 tRST_max;
-	u32 tWB_max;
-	u32 tWC_min;
-	u32 tWH_min;
-	u32 tWHR_min;
-	u32 tWP_min;
-	u32 tWW_min;
-};
-
 /* get timing characteristics from ONFI timing mode. */
 const struct nand_sdr_timings *onfi_async_timing_mode_to_sdr_timings(int mode);
 
-- 
2.8.1

Re: [PATCH 1/2] mtd: nand: automate NAND timings selection

[Boris Brezillon]

Hi Sascha,

It feels weird to review his own patch, but I have a few comments. :)

On Fri,  2 Sep 2016 14:42:28 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> From: Boris Brezillon <boris.brezillon@free-electrons.com>
> 
> The NAND framework provides several helpers to query timing modes supported
> by a NAND chip, but this implies that all NAND controller drivers have
> to implement the same timings selection dance.
> 
> Provide a common logic to select the best timings based on ONFI or
> ->onfi_timing_mode_default information.  
> NAND controller willing to support timings adjustment should just
> implement the ->setup_data_interface() method.

Now I remember one of the reason I did not post a v2 (apart from not
having the time).

If understand the ONFI spec correctly, when you reset the NAND chip,
you get back to SDR+timing-mode0. In the core we do not control when
the reset command (0xff) is issued, and this prevents us from
re-applying the correct timing mode after a reset.

Maybe we should provide a nand_reset() helper to hide this complexity,
and patch all ->cmdfunc(NAND_CMD_RESET) callers to call nand_reset()
instead.

Note that the interface+timing-mode config is not necessarily the only
thing we'll have to re-apply after a reset (especially on MLC NANDs), so
having place where we can put all operations that should be done after
a reset is a good thing.

> 
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/nand_base.c | 196 ++++++++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/nand.h     | 115 ++++++++++++++-----------
>  2 files changed, 261 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 77533f7..018fd56 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3322,6 +3322,148 @@ static void nand_onfi_detect_micron(struct nand_chip *chip,
>  	chip->setup_read_retry = nand_setup_read_retry_micron;
>  }
>  
> +/**
> + * nand_setup_data_interface - Setup the data interface and timings on the
> + *			       controller side
> + * @mtd: MTD device structure
> + * @conf: new configuration to apply
> + *
> + * Try to configure the NAND controller to support the provided data
> + * interface configuration.
> + *
> + * Returns 0 in case of success or -ERROR_CODE.
> + */
> +static int nand_setup_data_interface(struct mtd_info *mtd,
> +				     const struct nand_data_interface *conf)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	int ret;
> +
> +	if (!chip->setup_data_interface)
> +		return -ENOTSUPP;
> +
> +	ret = chip->setup_data_interface(mtd, conf, false);
> +	if (ret)
> +		return ret;
> +
> +	*chip->data_iface = *conf;

Maybe we can just switch the pointers here instead of copying its
content.
This would require turning the chip->data_iface into const, or passing
non-const parameter to nand_setup_data_interface(), but I don't see any
good reason to do this extra copy.

> +
> +	return 0;
> +}
> +

Thanks,

Boris

Re: [PATCH 1/2] mtd: nand: automate NAND timings selection

[Sascha Hauer]

On Mon, Sep 05, 2016 at 08:51:46AM +0200, Boris Brezillon wrote:
> Hi Sascha,
> 
> It feels weird to review his own patch, but I have a few comments. :)

I know this feeling. Suddenly you have to criticise code you previously
hoped to get through with ;)

> 
> On Fri,  2 Sep 2016 14:42:28 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > From: Boris Brezillon <boris.brezillon@free-electrons.com>
> > 
> > The NAND framework provides several helpers to query timing modes supported
> > by a NAND chip, but this implies that all NAND controller drivers have
> > to implement the same timings selection dance.
> > 
> > Provide a common logic to select the best timings based on ONFI or
> > ->onfi_timing_mode_default information.  
> > NAND controller willing to support timings adjustment should just
> > implement the ->setup_data_interface() method.
> 
> Now I remember one of the reason I did not post a v2 (apart from not
> having the time).
> 
> If understand the ONFI spec correctly, when you reset the NAND chip,
> you get back to SDR+timing-mode0. In the core we do not control when
> the reset command (0xff) is issued, and this prevents us from
> re-applying the correct timing mode after a reset.
> 
> Maybe we should provide a nand_reset() helper to hide this complexity,
> and patch all ->cmdfunc(NAND_CMD_RESET) callers to call nand_reset()
> instead.
> 
> Note that the interface+timing-mode config is not necessarily the only
> thing we'll have to re-apply after a reset (especially on MLC NANDs), so
> having place where we can put all operations that should be done after
> a reset is a good thing.

Ouch, there are indeed some things wrong in this patch. We iterate over
all chips and set the timing mode for each:

+		if (modes != ONFI_TIMING_MODE_UNKNOWN) {
+			/*
+			 * FIXME: should we really set the timing mode on all
+			 * dies?
+			 */
+			for (i = 0; i < chip->numchips; i++) {
+				chip->select_chip(mtd, i);
+				ret = chip->onfi_set_features(mtd, chip,
+						ONFI_FEATURE_ADDR_TIMING_MODE,
+						tmode_param);
+			}
+			chip->select_chip(mtd, -1);
+		}
+

Afterwards the code in nand_scan_ident() resets all chips while checking
for a chip array, reverting the effect of the above code. Looking closer
at it the above code has no effect anyway since it's executed when
chip->numchips is not yet initialized and still 0.

I think providing a nand_reset() function is a good idea. I'll implement
one and see what I end up with.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] mtd: nand: automate NAND timings selection

[Boris Brezillon]

On Mon, 5 Sep 2016 13:09:32 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Mon, Sep 05, 2016 at 08:51:46AM +0200, Boris Brezillon wrote:
> > Hi Sascha,
> > 
> > It feels weird to review his own patch, but I have a few comments. :)  
> 
> I know this feeling. Suddenly you have to criticise code you previously
> hoped to get through with ;)
> 
> > 
> > On Fri,  2 Sep 2016 14:42:28 +0200
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >   
> > > From: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > 
> > > The NAND framework provides several helpers to query timing modes supported
> > > by a NAND chip, but this implies that all NAND controller drivers have
> > > to implement the same timings selection dance.
> > > 
> > > Provide a common logic to select the best timings based on ONFI or  
> > > ->onfi_timing_mode_default information.    
> > > NAND controller willing to support timings adjustment should just
> > > implement the ->setup_data_interface() method.  
> > 
> > Now I remember one of the reason I did not post a v2 (apart from not
> > having the time).
> > 
> > If understand the ONFI spec correctly, when you reset the NAND chip,
> > you get back to SDR+timing-mode0. In the core we do not control when
> > the reset command (0xff) is issued, and this prevents us from
> > re-applying the correct timing mode after a reset.
> > 
> > Maybe we should provide a nand_reset() helper to hide this complexity,
> > and patch all ->cmdfunc(NAND_CMD_RESET) callers to call nand_reset()
> > instead.
> > 
> > Note that the interface+timing-mode config is not necessarily the only
> > thing we'll have to re-apply after a reset (especially on MLC NANDs), so
> > having place where we can put all operations that should be done after
> > a reset is a good thing.  
> 
> Ouch, there are indeed some things wrong in this patch. We iterate over
> all chips and set the timing mode for each:
> 
> +		if (modes != ONFI_TIMING_MODE_UNKNOWN) {
> +			/*
> +			 * FIXME: should we really set the timing mode on all
> +			 * dies?
> +			 */
> +			for (i = 0; i < chip->numchips; i++) {
> +				chip->select_chip(mtd, i);
> +				ret = chip->onfi_set_features(mtd, chip,
> +						ONFI_FEATURE_ADDR_TIMING_MODE,
> +						tmode_param);
> +			}
> +			chip->select_chip(mtd, -1);
> +		}
> +
> 
> Afterwards the code in nand_scan_ident() resets all chips while checking
> for a chip array, reverting the effect of the above code. Looking closer
> at it the above code has no effect anyway since it's executed when
> chip->numchips is not yet initialized and still 0.

Yep :-(.

> 
> I think providing a nand_reset() function is a good idea. I'll implement
> one and see what I end up with.

Thanks for looking into that, that's truly appreciated (I have so much
things on my plate lately that the NAND framework rework I planned are
constantly delayed).

Boris

Re: [PATCH 1/2] mtd: nand: automate NAND timings selection

[Sascha Hauer]

Hi Boris,

On Fri, Sep 02, 2016 at 02:42:28PM +0200, Sascha Hauer wrote:
> +static int nand_configure_data_interface(struct mtd_info *mtd)
> +{
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct nand_data_interface *conf;
> +	int modes, mode, ret = -EINVAL;
> +	uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { };
> +	int i;
> +
> +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> +	if (!conf)
> +		return -ENOMEM;
> +
> +	/* TODO: support DDR interfaces */
> +	conf->type = NAND_SDR_IFACE;
> +
> +	/*
> +	 * First try to identify the best timings from ONFI parameters and
> +	 * if the NAND does not support ONFI, fallback to the default ONFI
> +	 * timing mode.
> +	 */
> +	modes = onfi_get_async_timing_mode(chip);
> +	if (modes == ONFI_TIMING_MODE_UNKNOWN) {
> +		mode = chip->onfi_timing_mode_default;
> +		conf->timings.sdr =
> +				*onfi_async_timing_mode_to_sdr_timings(mode);
> +
> +		ret = nand_check_data_interface(mtd, conf);
> +	} else {
> +		for (mode = fls(modes) - 1; mode >= 0; mode--) {
> +			conf->timings.sdr =
> +				*onfi_async_timing_mode_to_sdr_timings(mode);
> +
> +			ret = nand_check_data_interface(mtd, conf);
> +			if (!ret)
> +				break;
> +		}
> +	}

I wonder if this works good for non ONFI NANDs. In the ONFI case we
iterate over all modes supported by the NAND until we find one that also
suits the driver. By doing so we inherently assume that not all drivers
support all modes. In the non ONFI case instead we hardcode a single
timing into the nand_id table, what if the driver does not support this
timing? It will fail in this case without ever trying slower timings.
Shouldn't we encode the mode bitmask into the nand_id table rather than
a single timing?
I cannot find a chip in the nand_id table which actually sets
onfi_timing_mode_default, but since you introduced the field in the
nand_id table with commit 57a94e24bc92 you can probably tell me more.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] mtd: nand: automate NAND timings selection

[Boris Brezillon]

On Tue, 6 Sep 2016 10:23:02 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Hi Boris,
> 
> On Fri, Sep 02, 2016 at 02:42:28PM +0200, Sascha Hauer wrote:
> > +static int nand_configure_data_interface(struct mtd_info *mtd)
> > +{
> > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > +	struct nand_data_interface *conf;
> > +	int modes, mode, ret = -EINVAL;
> > +	uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { };
> > +	int i;
> > +
> > +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > +	if (!conf)
> > +		return -ENOMEM;
> > +
> > +	/* TODO: support DDR interfaces */
> > +	conf->type = NAND_SDR_IFACE;
> > +
> > +	/*
> > +	 * First try to identify the best timings from ONFI parameters and
> > +	 * if the NAND does not support ONFI, fallback to the default ONFI
> > +	 * timing mode.
> > +	 */
> > +	modes = onfi_get_async_timing_mode(chip);
> > +	if (modes == ONFI_TIMING_MODE_UNKNOWN) {
> > +		mode = chip->onfi_timing_mode_default;
> > +		conf->timings.sdr =
> > +				*onfi_async_timing_mode_to_sdr_timings(mode);
> > +
> > +		ret = nand_check_data_interface(mtd, conf);
> > +	} else {
> > +		for (mode = fls(modes) - 1; mode >= 0; mode--) {
> > +			conf->timings.sdr =
> > +				*onfi_async_timing_mode_to_sdr_timings(mode);
> > +
> > +			ret = nand_check_data_interface(mtd, conf);
> > +			if (!ret)
> > +				break;
> > +		}
> > +	}  
> 
> I wonder if this works good for non ONFI NANDs. In the ONFI case we
> iterate over all modes supported by the NAND until we find one that also
> suits the driver. By doing so we inherently assume that not all drivers
> support all modes. In the non ONFI case instead we hardcode a single
> timing into the nand_id table, what if the driver does not support this
> timing? It will fail in this case without ever trying slower timings.
> Shouldn't we encode the mode bitmask into the nand_id table rather than
> a single timing?

IIRC, this is what I proposed in my first patch (having a bitmask
encoding supported modes), but Brian suggested to directly put the
highest supported mode.

Anyway, I don't think a NAND can support higher modes without
supporting lower ones, so extracting the supported modes info from the
default_onfi_timing_mode should be pretty easy:

	supported_modes = GENMASK(default_onfi_timing_mode, 0);

> I cannot find a chip in the nand_id table which actually sets
> onfi_timing_mode_default, but since you introduced the field in the
> nand_id table with commit 57a94e24bc92 you can probably tell me more.

Hm, this one [1] is defining timing mode 4.

[1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_ids.c#L51

Re: [PATCH 1/2] mtd: nand: automate NAND timings selection

[Sascha Hauer]

On Tue, Sep 06, 2016 at 10:41:30AM +0200, Boris Brezillon wrote:
> On Tue, 6 Sep 2016 10:23:02 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > Hi Boris,
> > 
> > On Fri, Sep 02, 2016 at 02:42:28PM +0200, Sascha Hauer wrote:
> > > +static int nand_configure_data_interface(struct mtd_info *mtd)
> > > +{
> > > +	struct nand_chip *chip = mtd_to_nand(mtd);
> > > +	struct nand_data_interface *conf;
> > > +	int modes, mode, ret = -EINVAL;
> > > +	uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = { };
> > > +	int i;
> > > +
> > > +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > > +	if (!conf)
> > > +		return -ENOMEM;
> > > +
> > > +	/* TODO: support DDR interfaces */
> > > +	conf->type = NAND_SDR_IFACE;
> > > +
> > > +	/*
> > > +	 * First try to identify the best timings from ONFI parameters and
> > > +	 * if the NAND does not support ONFI, fallback to the default ONFI
> > > +	 * timing mode.
> > > +	 */
> > > +	modes = onfi_get_async_timing_mode(chip);
> > > +	if (modes == ONFI_TIMING_MODE_UNKNOWN) {
> > > +		mode = chip->onfi_timing_mode_default;
> > > +		conf->timings.sdr =
> > > +				*onfi_async_timing_mode_to_sdr_timings(mode);
> > > +
> > > +		ret = nand_check_data_interface(mtd, conf);
> > > +	} else {
> > > +		for (mode = fls(modes) - 1; mode >= 0; mode--) {
> > > +			conf->timings.sdr =
> > > +				*onfi_async_timing_mode_to_sdr_timings(mode);
> > > +
> > > +			ret = nand_check_data_interface(mtd, conf);
> > > +			if (!ret)
> > > +				break;
> > > +		}
> > > +	}  
> > 
> > I wonder if this works good for non ONFI NANDs. In the ONFI case we
> > iterate over all modes supported by the NAND until we find one that also
> > suits the driver. By doing so we inherently assume that not all drivers
> > support all modes. In the non ONFI case instead we hardcode a single
> > timing into the nand_id table, what if the driver does not support this
> > timing? It will fail in this case without ever trying slower timings.
> > Shouldn't we encode the mode bitmask into the nand_id table rather than
> > a single timing?
> 
> IIRC, this is what I proposed in my first patch (having a bitmask
> encoding supported modes), but Brian suggested to directly put the
> highest supported mode.
> 
> Anyway, I don't think a NAND can support higher modes without
> supporting lower ones, so extracting the supported modes info from the
> default_onfi_timing_mode should be pretty easy:
> 
> 	supported_modes = GENMASK(default_onfi_timing_mode, 0);

Ok, if you think we can assume this then I'll do it like that.

> 
> > I cannot find a chip in the nand_id table which actually sets
> > onfi_timing_mode_default, but since you introduced the field in the
> > nand_id table with commit 57a94e24bc92 you can probably tell me more.
> 
> Hm, this one [1] is defining timing mode 4.
> 
> [1]http://lxr.free-electrons.com/source/drivers/mtd/nand/nand_ids.c#L51

Uh, yes. I'm so glad we have C99 initializers mostly nowadays :)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |