mtd: nand: automate NAND timings selection

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

[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

[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 |

[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

[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 |

[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

[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 |

[PATCH 2/2] mtd: mxc_nand: Set timing for v2 controllers

[Sascha Hauer]

So far we relied on reset default or the bootloader to configure a
suitable clk rate for the Nand controller. This works but we can
optimize the timing for better performance. This sets the clk rate for
v2 controllers (i.MX25/35) based on the timing mode read from the ONFI
parameter page. This may also enable the symmetric mode (aks EDO mode)
if necessary which reads one word per clock cycle.
Tested on an i.MX25 with a Micron MT29F4G08ABBDAHC attached.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 80 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 5173fad..3cd2696 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -152,6 +152,9 @@ struct mxc_nand_devtype_data {
 	void (*select_chip)(struct mtd_info *mtd, int chip);
 	int (*correct_data)(struct mtd_info *mtd, u_char *dat,
 			u_char *read_ecc, u_char *calc_ecc);
+	int (*setup_data_interface)(struct mtd_info *mtd,
+			  const struct nand_data_interface *conf,
+			  bool check_only);
 
 	/*
 	 * On i.MX21 the CONFIG2:INT bit cannot be read if interrupts are masked
@@ -1012,6 +1015,80 @@ static void preset_v1(struct mtd_info *mtd)
 	writew(0x4, NFC_V1_V2_WRPROT);
 }
 
+static int mxc_nand_v2_setup_data_interface(struct mtd_info *mtd,
+					    const struct nand_data_interface *conf,
+					    bool check_only)
+{
+	struct nand_chip *nand_chip = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
+	int tRC_min_ns, tRC_ps, ret;
+	unsigned long rate, rate_round;
+	const struct nand_sdr_timings *timings;
+	uint16_t config1;
+
+	if (conf->type != NAND_SDR_IFACE)
+		return -ENOTSUPP;
+
+	config1 = readw(NFC_V1_V2_CONFIG1);
+
+	timings = &conf->timings.sdr;
+
+	tRC_min_ns = timings->tRC_min / 1000;
+	rate = 1000000000 / tRC_min_ns;
+
+	/*
+	 * For tRC < 30ns we have to use EDO mode. In this case the controller
+	 * does one access per clock cycle. Otherwise the controller does one
+	 * access in two clock cycles, thus we have to double the rate to the
+	 * controller.
+	 */
+	if (tRC_min_ns < 30) {
+		rate_round = clk_round_rate(host->clk, rate);
+		config1 |= NFC_V2_CONFIG1_ONE_CYCLE;
+		tRC_ps = 1000000000 / (rate_round / 1000);
+	} else {
+		rate_round = clk_round_rate(host->clk, rate * 2);
+		config1 &= ~NFC_V2_CONFIG1_ONE_CYCLE;
+		tRC_ps = 1000000000 / (rate_round / 1000 / 2);
+		rate *= 2;
+	}
+
+	/*
+	 * The timing values compared against are from the i.MX25 Automotive
+	 * datasheet, Table 50. NFC Timing Parameters
+	 */
+	if (timings->tCLS_min > tRC_ps - 1000 ||
+	    timings->tCLH_min > tRC_ps - 2000 ||
+	    timings->tCS_min > tRC_ps - 1000 ||
+	    timings->tCH_min > tRC_ps - 2000 ||
+	    timings->tWP_min > tRC_ps - 1500 ||
+	    timings->tALS_min > tRC_ps ||
+	    timings->tALH_min > tRC_ps - 3000 ||
+	    timings->tDS_min > tRC_ps ||
+	    timings->tDH_min > tRC_ps - 5000 ||
+	    timings->tWC_min > 2 * tRC_ps ||
+	    timings->tWH_min > tRC_ps - 2500 ||
+	    timings->tRR_min > 6 * tRC_ps ||
+	    timings->tRP_min > 3 * tRC_ps / 2 ||
+	    timings->tRC_min > 2 * tRC_ps ||
+	    timings->tREH_min > (tRC_ps / 2) - 2500) {
+		dev_dbg(host->dev, "Timing out of bounds\n");
+		return -EINVAL;
+	}
+
+	ret = clk_set_rate(host->clk, rate);
+	if (ret)
+		return ret;
+
+	writew(config1, NFC_V1_V2_CONFIG1);
+
+	dev_dbg(host->dev, "Setting rate to %ldHz, %s mode\n", rate_round,
+		config1 & NFC_V2_CONFIG1_ONE_CYCLE ? "One cycle (EDO)" :
+		"normal");
+
+	return 0;
+}
+
 static void preset_v2(struct mtd_info *mtd)
 {
 	struct nand_chip *nand_chip = mtd_to_nand(mtd);
@@ -1327,6 +1404,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 	.ooblayout = &mxc_v2_ooblayout_ops,
 	.select_chip = mxc_nand_select_chip_v2,
 	.correct_data = mxc_nand_correct_data_v2_v3,
+	.setup_data_interface = mxc_nand_v2_setup_data_interface,
 	.irqpending_quirk = 0,
 	.needs_ip = 0,
 	.regs_offset = 0x1e00,
@@ -1533,6 +1611,8 @@ static int mxcnd_probe(struct platform_device *pdev)
 	if (err < 0)
 		return err;
 
+	this->setup_data_interface = host->devtype_data->setup_data_interface;
+
 	if (host->devtype_data->needs_ip) {
 		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 		host->regs_ip = devm_ioremap_resource(&pdev->dev, res);
-- 
2.8.1

[PATCH 2/2] mtd: mxc_nand: Set timing for v2 controllers

[Lothar Waßmann]

Hi,

On Fri,  2 Sep 2016 14:42:29 +0200 Sascha Hauer wrote:
> So far we relied on reset default or the bootloader to configure a
> suitable clk rate for the Nand controller. This works but we can
> optimize the timing for better performance. This sets the clk rate for
> v2 controllers (i.MX25/35) based on the timing mode read from the ONFI
> parameter page. This may also enable the symmetric mode (aks EDO mode)
> if necessary which reads one word per clock cycle.
> Tested on an i.MX25 with a Micron MT29F4G08ABBDAHC attached.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c | 80 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 5173fad..3cd2696 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -152,6 +152,9 @@ struct mxc_nand_devtype_data {
>  	void (*select_chip)(struct mtd_info *mtd, int chip);
>  	int (*correct_data)(struct mtd_info *mtd, u_char *dat,
>  			u_char *read_ecc, u_char *calc_ecc);
> +	int (*setup_data_interface)(struct mtd_info *mtd,
> +			  const struct nand_data_interface *conf,
> +			  bool check_only);
>  
<nit>
indentation inconsistent with the preceding line.
(<TAB><SPACE><SPACE> vs. <TABs> only)
</nit>

>  	/*
>  	 * On i.MX21 the CONFIG2:INT bit cannot be read if interrupts are masked
> @@ -1012,6 +1015,80 @@ static void preset_v1(struct mtd_info *mtd)
>  	writew(0x4, NFC_V1_V2_WRPROT);
>  }
>  
> +static int mxc_nand_v2_setup_data_interface(struct mtd_info *mtd,
> +					    const struct nand_data_interface *conf,
> +					    bool check_only)
> +{
> +	struct nand_chip *nand_chip = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
> +	int tRC_min_ns, tRC_ps, ret;
> +	unsigned long rate, rate_round;
> +	const struct nand_sdr_timings *timings;
> +	uint16_t config1;
> +
> +	if (conf->type != NAND_SDR_IFACE)
> +		return -ENOTSUPP;
> +
> +	config1 = readw(NFC_V1_V2_CONFIG1);
> +
> +	timings = &conf->timings.sdr;
> +
> +	tRC_min_ns = timings->tRC_min / 1000;
> +	rate = 1000000000 / tRC_min_ns;
> +
> +	/*
> +	 * For tRC < 30ns we have to use EDO mode. In this case the controller
> +	 * does one access per clock cycle. Otherwise the controller does one
> +	 * access in two clock cycles, thus we have to double the rate to the
> +	 * controller.
> +	 */
> +	if (tRC_min_ns < 30) {
> +		rate_round = clk_round_rate(host->clk, rate);
> +		config1 |= NFC_V2_CONFIG1_ONE_CYCLE;
> +		tRC_ps = 1000000000 / (rate_round / 1000);
> +	} else {
> +		rate_round = clk_round_rate(host->clk, rate * 2);
> +		config1 &= ~NFC_V2_CONFIG1_ONE_CYCLE;
> +		tRC_ps = 1000000000 / (rate_round / 1000 / 2);
> +		rate *= 2;
>
You could save an extra lsl by doing the *2 first:
		rate *= 2;
		rate_round = clk_round_rate(host->clk, rate);
[...]

> +	/*
> +	 * The timing values compared against are from the i.MX25 Automotive
> +	 * datasheet, Table 50. NFC Timing Parameters
> +	 */
> +	if (timings->tCLS_min > tRC_ps - 1000 ||
> +	    timings->tCLH_min > tRC_ps - 2000 ||
> +	    timings->tCS_min > tRC_ps - 1000 ||
> +	    timings->tCH_min > tRC_ps - 2000 ||
> +	    timings->tWP_min > tRC_ps - 1500 ||
> +	    timings->tALS_min > tRC_ps ||
> +	    timings->tALH_min > tRC_ps - 3000 ||
> +	    timings->tDS_min > tRC_ps ||
> +	    timings->tDH_min > tRC_ps - 5000 ||
> +	    timings->tWC_min > 2 * tRC_ps ||
> +	    timings->tWH_min > tRC_ps - 2500 ||
> +	    timings->tRR_min > 6 * tRC_ps ||
> +	    timings->tRP_min > 3 * tRC_ps / 2 ||
> +	    timings->tRC_min > 2 * tRC_ps ||
> +	    timings->tREH_min > (tRC_ps / 2) - 2500) {
> +		dev_dbg(host->dev, "Timing out of bounds\n");
>
Since it is an error which prevents the driver from being loaded, the
message deserves to be printed with dev_err(), so that the user can see
why the driver failed (without having to recompile the kernel with
'#define DEBUG' added to the driver source).


Lothar Wa?mann

[PATCH 2/2] mtd: mxc_nand: Set timing for v2 controllers

[Sascha Hauer]

On Fri, Sep 02, 2016 at 04:17:05PM +0200, Lothar Wa?mann wrote:
> Hi,
> 
> On Fri,  2 Sep 2016 14:42:29 +0200 Sascha Hauer wrote:
> > So far we relied on reset default or the bootloader to configure a
> > suitable clk rate for the Nand controller. This works but we can
> > optimize the timing for better performance. This sets the clk rate for
> > v2 controllers (i.MX25/35) based on the timing mode read from the ONFI
> > parameter page. This may also enable the symmetric mode (aks EDO mode)
> > if necessary which reads one word per clock cycle.
> > Tested on an i.MX25 with a Micron MT29F4G08ABBDAHC attached.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/mtd/nand/mxc_nand.c | 80 +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 80 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > index 5173fad..3cd2696 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -152,6 +152,9 @@ struct mxc_nand_devtype_data {
> >  	void (*select_chip)(struct mtd_info *mtd, int chip);
> >  	int (*correct_data)(struct mtd_info *mtd, u_char *dat,
> >  			u_char *read_ecc, u_char *calc_ecc);
> > +	int (*setup_data_interface)(struct mtd_info *mtd,
> > +			  const struct nand_data_interface *conf,
> > +			  bool check_only);
> >  
> <nit>
> indentation inconsistent with the preceding line.
> (<TAB><SPACE><SPACE> vs. <TABs> only)
> </nit>

Okay, will fix in the next version.

> > +	/*
> > +	 * For tRC < 30ns we have to use EDO mode. In this case the controller
> > +	 * does one access per clock cycle. Otherwise the controller does one
> > +	 * access in two clock cycles, thus we have to double the rate to the
> > +	 * controller.
> > +	 */
> > +	if (tRC_min_ns < 30) {
> > +		rate_round = clk_round_rate(host->clk, rate);
> > +		config1 |= NFC_V2_CONFIG1_ONE_CYCLE;
> > +		tRC_ps = 1000000000 / (rate_round / 1000);
> > +	} else {
> > +		rate_round = clk_round_rate(host->clk, rate * 2);
> > +		config1 &= ~NFC_V2_CONFIG1_ONE_CYCLE;
> > +		tRC_ps = 1000000000 / (rate_round / 1000 / 2);
> > +		rate *= 2;
> >
> You could save an extra lsl by doing the *2 first:
> 		rate *= 2;
> 		rate_round = clk_round_rate(host->clk, rate);

Yep, looks better that way.

> [...]
> 
> > +	/*
> > +	 * The timing values compared against are from the i.MX25 Automotive
> > +	 * datasheet, Table 50. NFC Timing Parameters
> > +	 */
> > +	if (timings->tCLS_min > tRC_ps - 1000 ||
> > +	    timings->tCLH_min > tRC_ps - 2000 ||
> > +	    timings->tCS_min > tRC_ps - 1000 ||
> > +	    timings->tCH_min > tRC_ps - 2000 ||
> > +	    timings->tWP_min > tRC_ps - 1500 ||
> > +	    timings->tALS_min > tRC_ps ||
> > +	    timings->tALH_min > tRC_ps - 3000 ||
> > +	    timings->tDS_min > tRC_ps ||
> > +	    timings->tDH_min > tRC_ps - 5000 ||
> > +	    timings->tWC_min > 2 * tRC_ps ||
> > +	    timings->tWH_min > tRC_ps - 2500 ||
> > +	    timings->tRR_min > 6 * tRC_ps ||
> > +	    timings->tRP_min > 3 * tRC_ps / 2 ||
> > +	    timings->tRC_min > 2 * tRC_ps ||
> > +	    timings->tREH_min > (tRC_ps / 2) - 2500) {
> > +		dev_dbg(host->dev, "Timing out of bounds\n");
> >
> Since it is an error which prevents the driver from being loaded, the
> message deserves to be printed with dev_err(), so that the user can see
> why the driver failed (without having to recompile the kernel with
> '#define DEBUG' added to the driver source).

This error won't prevent the driver from loading. The nand core iterates
over the different available timings calling this function for each
until it finds a timing supported by the driver. If we turn this into
dev_err we could end up seeing this message multiple times and still the
driver works. In case the slowest timing isn't supported by the driver
and the nand core bails out it will print:

pr_err("Failed to configure data interface to SDR timing mode 0\n");

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 |