[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