[PATCH v3] mtd: nand: automate NAND timings selection

[PATCH v3] 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.

As suggested by Boris this version of the series introduces a nand_reset()
function which replaces the several open coded NAND_CMD_RESET commands
in the code. This makes sure we can apply the timing each time after
after reset.

Also I have brought back the conversion patch for teh sunxi driver whic
was part of Boris initial posting. It's untested due to the lack of hardware,
so please test before applying.

Sascha

Changes since v2:
- Add accessor function to get the SDR timing from struct nand_data_interface
- Change nand_reset() argument to struct nand_chip
- Drop conversion of nand_timing array to struct nand_data_interface
- Recalculate timing whenever needed instead of storing a pointer in struct
  nand_chip
- some more refactoring

Changes since v1:
- create a nand_reset() function to create a single place to reset NAND
  chips and to apply timings
- Add patch to convert sunxi driver for automated timing setup
- split into more patches

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 chipi
  afterwards.
- Drop Sunxi example, add patch for the mxc_nand controller instead.

----------------------------------------------------------------
Boris Brezillon (1):
      mtd: nand: automate NAND timings selection

Sascha Hauer (6):
      mtd: nand: Create a NAND reset function
      mtd: nand: Introduce nand_data_interface
      mtd: nand: sunxi: switch from manual to automated timing config
      mtd: nand: mxc: implement onfi get/set features
      mtd: nand: mxc: Add timing setup for v2 controllers
      mtd: nand: remove unnecessary 'extern' from function declarations

 drivers/mtd/nand/mxc_nand.c   | 133 ++++++++++++++++++++++++++
 drivers/mtd/nand/nand_base.c  | 157 ++++++++++++++++++++++++++++++-
 drivers/mtd/nand/sunxi_nand.c |  76 +++------------
 include/linux/mtd/nand.h      | 211 ++++++++++++++++++++++++++++--------------
 4 files changed, 442 insertions(+), 135 deletions(-)

[PATCH 1/7] mtd: nand: Create a NAND reset function

[Sascha Hauer]

When NAND devices are resetted some initialization may have to be done,
like for example they have to be configured for the timing mode that
shall be used. To get a common place where this initialization can be
implemented create a nand_reset() function. This currently only issues
a NAND_CMD_RESET to the NAND device. The places issuing this command
manually are replaced with a call to nand_reset().

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/nand_base.c | 23 ++++++++++++++++++-----
 include/linux/mtd/nand.h     |  4 ++++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 77533f7..1f704cc 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -948,6 +948,19 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 }
 
 /**
+ * nand_reset - Reset and initialize a NAND device
+ * @chip: The NAND chip
+ *
+ * Returns 0 for success or negative error code otherwise
+ */
+int nand_reset(struct nand_chip *chip)
+{
+	chip->cmdfunc(&chip->mtd, NAND_CMD_RESET, -1, -1);
+
+	return 0;
+}
+
+/**
  * __nand_unlock - [REPLACEABLE] unlocks specified locked blocks
  * @mtd: mtd info
  * @ofs: offset to start unlock from
@@ -1025,7 +1038,7 @@ int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	 * some operation can also clear the bit 7 of status register
 	 * eg. erase/program a locked block
 	 */
-	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
+	nand_reset(chip);
 
 	/* Check, if it is write protected */
 	if (nand_check_wp(mtd)) {
@@ -1084,7 +1097,7 @@ int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
 	 * some operation can also clear the bit 7 of status register
 	 * eg. erase/program a locked block
 	 */
-	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
+	nand_reset(chip);
 
 	/* Check, if it is write protected */
 	if (nand_check_wp(mtd)) {
@@ -2788,7 +2801,7 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
 	 * if we don't do this. I have no clue why, but I seem to have 'fixed'
 	 * it in the doc2000 driver in August 1999.  dwmw2.
 	 */
-	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
+	nand_reset(chip);
 
 	/* Check, if it is write protected */
 	if (nand_check_wp(mtd)) {
@@ -3829,7 +3842,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
 	 * Reset the chip, required by some chips (e.g. Micron MT29FxGxxxxx)
 	 * after power-up.
 	 */
-	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
+	nand_reset(chip);
 
 	/* Send the command for reading device ID */
 	chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
@@ -4161,7 +4174,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
 	for (i = 1; i < maxchips; i++) {
 		chip->select_chip(mtd, i);
 		/* See comment in nand_get_flash_type for reset */
-		chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
+		nand_reset(chip);
 		/* Send the command for reading device ID */
 		chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
 		/* Read manufacturer and device IDs */
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 8dd6e01..9890df2 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -1093,4 +1093,8 @@ int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
 /* Default read_oob syndrome implementation */
 int nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
 			   int page);
+
+/* Reset and initialize a NAND device */
+int nand_reset(struct nand_chip *chip);
+
 #endif /* __LINUX_MTD_NAND_H */
-- 
2.8.1

[PATCH 2/7] mtd: nand: Introduce nand_data_interface

[Sascha Hauer]

Currently we have no data structure to fully describe a NAND timing.
We only have struct nand_sdr_timings for NAND timings in SDR mode,
but nothing for DDR mode and also no container to store both types
of timing.
This patch adds struct nand_data_interface which stores the timing
type and a union of different timings. This can be used to pass to
drivers in order to configure the timing.
Add kerneldoc for struct nand_sdr_timings while touching it anyway.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/linux/mtd/nand.h | 165 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 116 insertions(+), 49 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9890df2..f305bed 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -566,6 +566,122 @@ struct nand_buffers {
 };
 
 /**
+ * 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.
+ *
+ * @tALH_min: ALE hold time
+ * @tADL_min: ALE to data loading time
+ * @tALS_min: ALE setup time
+ * @tAR_min: ALE to RE# delay
+ * @tCEA_max: CE# access time
+ * @tCEH_min:
+ * @tCH_min:  CE# hold time
+ * @tCHZ_max: CE# high to output hi-Z
+ * @tCLH_min: CLE hold time
+ * @tCLR_min: CLE to RE# delay
+ * @tCLS_min: CLE setup time
+ * @tCOH_min: CE# high to output hold
+ * @tCS_min: CE# setup time
+ * @tDH_min: Data hold time
+ * @tDS_min: Data setup time
+ * @tFEAT_max: Busy time for Set Features and Get Features
+ * @tIR_min: Output hi-Z to RE# low
+ * @tITC_max: Interface and Timing Mode Change time
+ * @tRC_min: RE# cycle time
+ * @tREA_max: RE# access time
+ * @tREH_min: RE# high hold time
+ * @tRHOH_min: RE# high to output hold
+ * @tRHW_min: RE# high to WE# low
+ * @tRHZ_max: RE# high to output hi-Z
+ * @tRLOH_min: RE# low to output hold
+ * @tRP_min: RE# pulse width
+ * @tRR_min: Ready to RE# low (data only)
+ * @tRST_max: Device reset time, measured from the falling edge of R/B# to the rising edge of R/B#.
+ * @tWB_max: WE# high to SR[6] low
+ * @tWC_min: WE# cycle time
+ * @tWH_min: WE# high hold time
+ * @tWHR_min: WE# high to RE# low
+ * @tWP_min: WE# pulse width
+ * @tWW_min: WP# transition to WE# low
+ */
+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 interface timing type
+ * @NAND_SDR_IFACE:	Single Data Rate interface
+ */
+enum nand_data_interface_type {
+	NAND_SDR_IFACE,
+};
+
+/**
+ * struct nand_data_interface - NAND interface timing
+ * @type:	type of the timing
+ * @timings:	The timing, type according to @type
+ */
+struct nand_data_interface {
+	enum nand_data_interface_type type;
+	union {
+		struct nand_sdr_timings sdr;
+	} timings;
+};
+
+/**
+ * nand_get_sdr_timings - get SDR timing from data interface
+ * @conf:	The data interface
+ */
+static inline const struct nand_sdr_timings *
+nand_get_sdr_timings(const struct nand_data_interface *conf)
+{
+	if (conf->type != NAND_SDR_IFACE)
+		return ERR_PTR(-EINVAL);
+
+	return &conf->timings.sdr;
+}
+
+/**
  * struct nand_chip - NAND Private Flash Chip Data
  * @mtd:		MTD device registered to the MTD framework
  * @IO_ADDR_R:		[BOARDSPECIFIC] address to read the 8 I/O lines of the
@@ -1023,55 +1139,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 3/7] 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. Also currently NAND
devices can be resetted at arbitrary places which also resets the timing
for ONFI chips to timing mode 0.

Provide a common logic to select the best timings based on ONFI or
->onfi_timing_mode_default information. Hook this into nand_reset()
to make sure the new timing is applied each time during a reset.

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 | 134 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/nand.h     |  12 ++--
 2 files changed, 142 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 1f704cc..c9bc988 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -948,6 +948,130 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
 }
 
 /**
+ * nand_reset_data_interface - Reset data interface and timings
+ * @chip: The NAND chip
+ *
+ * Reset the Data interface and timings to ONFI mode 0.
+ *
+ * Returns 0 for success or negative error code otherwise.
+ */
+static int nand_reset_data_interface(struct nand_chip *chip)
+{
+	struct mtd_info *mtd = &chip->mtd;
+	struct nand_data_interface *conf;
+	int ret;
+
+	if (!chip->setup_data_interface)
+		return 0;
+
+	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
+	if (!conf)
+		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.
+	 */
+
+	conf->type = NAND_SDR_IFACE,
+	conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(0);
+
+	ret = chip->setup_data_interface(mtd, conf, false);
+	if (ret)
+		pr_err("Failed to configure data interface to SDR timing mode 0\n");
+
+	kfree(conf);
+
+	return ret;
+}
+
+/**
+ * nand_setup_data_interface - Setup the best data interface and timings
+ * @chip: The NAND chip
+ *
+ * Find and configure the best data interface and NAND timings supported by
+ * the chip and the driver.
+ * 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 for success or negative error code otherwise.
+ */
+static int nand_setup_data_interface(struct nand_chip *chip)
+{
+	struct mtd_info *mtd = &chip->mtd;
+	struct nand_data_interface *conf;
+	int modes, mode, ret;
+
+	if (!chip->setup_data_interface)
+		return 0;
+
+	/*
+	 * 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) {
+		if (!chip->onfi_timing_mode_default)
+			return 0;
+
+		modes = GENMASK(chip->onfi_timing_mode_default, 0);
+	}
+
+	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
+	if (!conf)
+		return -ENOMEM;
+
+	ret = -EINVAL;
+	for (mode = fls(modes) - 1; mode >= 0; mode--) {
+		conf->type = NAND_SDR_IFACE,
+		conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(mode);
+
+		ret = chip->setup_data_interface(mtd, conf, true);
+		if (!ret) {
+			chip->onfi_timing_mode_default = mode;
+			break;
+		}
+	}
+
+	if (ret)
+		goto err;
+
+	/*
+	 * Ensure the timing mode has been changed on the chip side
+	 * before changing timings on the controller side.
+	 */
+	if (chip->onfi_version) {
+		uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
+			chip->onfi_timing_mode_default,
+		};
+
+		ret = chip->onfi_set_features(mtd, chip,
+				ONFI_FEATURE_ADDR_TIMING_MODE,
+				tmode_param);
+		if (ret)
+			goto err;
+	}
+
+	ret = chip->setup_data_interface(mtd, conf, false);
+
+err:
+	kfree(conf);
+
+	return ret;
+}
+
+/**
  * nand_reset - Reset and initialize a NAND device
  * @chip: The NAND chip
  *
@@ -955,8 +1079,18 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
  */
 int nand_reset(struct nand_chip *chip)
 {
+	int ret;
+
+	ret = nand_reset_data_interface(chip);
+	if (ret)
+		return ret;
+
 	chip->cmdfunc(&chip->mtd, NAND_CMD_RESET, -1, -1);
 
+	ret = nand_setup_data_interface(chip);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index f305bed..9f3d7be 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -743,10 +743,9 @@ nand_get_sdr_timings(const struct nand_data_interface *conf)
  *                      also from the datasheet. It is the recommended ECC step
  *			size, if known; if unknown, set to zero.
  * @onfi_timing_mode_default: [INTERN] default ONFI timing mode. This field is
- *			      either deduced from the datasheet if the NAND
- *			      chip is not ONFI compliant or set to 0 if it is
- *			      (an ONFI chip is always configured in mode 0
- *			      after a NAND reset)
+ * 			      set to the actually used ONFI mode if the chip is
+ * 			      ONFI compliant or deduced from the datasheet if
+ * 			      the NAND chip is not ONFI compliant.
  * @numchips:		[INTERN] number of physical chips
  * @chipsize:		[INTERN] the size of one chip for multichip arrays
  * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1
@@ -766,6 +765,7 @@ nand_get_sdr_timings(const struct nand_data_interface *conf)
  * @read_retries:	[INTERN] the number of read retry modes supported
  * @onfi_set_features:	[REPLACEABLE] set the features for ONFI nand
  * @onfi_get_features:	[REPLACEABLE] get the features for ONFI nand
+ * @setup_data_interface: [OPTIONAL] setup the data interface and timing
  * @bbt:		[INTERN] bad block table pointer
  * @bbt_td:		[REPLACEABLE] bad block table descriptor for flash
  *			lookup.
@@ -812,6 +812,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;
-- 
2.8.1

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

[Sascha Hauer]

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: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/sunxi_nand.c | 76 ++++++++-----------------------------------
 1 file changed, 14 insertions(+), 62 deletions(-)

diff --git a/drivers/mtd/nand/sunxi_nand.c b/drivers/mtd/nand/sunxi_nand.c
index e414b31..8c59a10 100644
--- a/drivers/mtd/nand/sunxi_nand.c
+++ b/drivers/mtd/nand/sunxi_nand.c
@@ -1572,14 +1572,22 @@ static int _sunxi_nand_lookup_timing(const s32 *lut, int lut_size, u32 duration,
 #define sunxi_nand_lookup_timing(l, p, c) \
 			_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)
+static int sunxi_nfc_setup_data_interface(struct mtd_info *mtd,
+					    const struct nand_data_interface *conf,
+					    bool check_only)
 {
+	struct nand_chip *nand = mtd_to_nand(mtd);
+	struct sunxi_nand_chip *chip = to_sunxi_nand(nand);
 	struct sunxi_nfc *nfc = to_sunxi_nfc(chip->nand.controller);
+	const struct nand_sdr_timings *timings;
 	u32 min_clk_period = 0;
 	s32 tWB, tADL, tWHR, tRHW, tCAD;
 	long real_clk_rate;
 
+	timings = nand_get_sdr_timings(conf);
+	if (IS_ERR(timings))
+		return -ENOTSUPP;
+
 	/* T1 <=> tCLS */
 	if (timings->tCLS_min > min_clk_period)
 		min_clk_period = timings->tCLS_min;
@@ -1679,6 +1687,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.
@@ -1712,44 +1723,6 @@ 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)
-{
-	struct mtd_info *mtd = nand_to_mtd(&chip->nand);
-	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(mtd, i);
-			ret = chip->nand.onfi_set_features(mtd,	&chip->nand,
-						ONFI_FEATURE_ADDR_TIMING_MODE,
-						feature);
-			chip->nand.select_chip(mtd, -1);
-			if (ret)
-				return ret;
-		}
-	}
-
-	timings = onfi_async_timing_mode_to_sdr_timings(mode);
-	if (IS_ERR(timings))
-		return PTR_ERR(timings);
-
-	return sunxi_nand_chip_set_timings(chip, timings);
-}
-
 static int sunxi_nand_ooblayout_ecc(struct mtd_info *mtd, int section,
 				    struct mtd_oob_region *oobregion)
 {
@@ -1975,7 +1948,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_info *mtd;
 	struct nand_chip *nand;
@@ -2065,25 +2037,11 @@ 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_nfc_setup_data_interface;
 
 	mtd = nand_to_mtd(nand);
 	mtd->dev.parent = dev;
 
-	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;
-	}
-
 	ret = nand_scan_ident(mtd, nsels, NULL);
 	if (ret)
 		return ret;
@@ -2096,12 +2054,6 @@ static int sunxi_nand_chip_init(struct device *dev, struct sunxi_nfc *nfc,
 
 	nand->options |= NAND_SUBPAGE_READ;
 
-	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.8.1

[PATCH 5/7] mtd: nand: mxc: implement onfi get/set features

[Sascha Hauer]

To be able to support different ONFI timing modes we have to implement
the onfi_set_features and onfi_get_features. Tested on an i.MX25 SoC.

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

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 5173fad..1db8299 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -1239,6 +1239,57 @@ static void mxc_nand_command(struct mtd_info *mtd, unsigned command,
 	}
 }
 
+static int mxc_nand_onfi_set_features(struct mtd_info *mtd, struct nand_chip *chip,
+			int addr, uint8_t *subfeature_param)
+{
+	struct nand_chip *nand_chip = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
+	int i;
+
+	if (!chip->onfi_version ||
+	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
+	      & ONFI_OPT_CMD_SET_GET_FEATURES))
+		return -EINVAL;
+
+	host->buf_start = 0;
+
+	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
+		chip->write_byte(mtd, subfeature_param[i]);
+
+	memcpy32_toio(host->main_area0, host->data_buf, mtd->writesize);
+	host->devtype_data->send_cmd(host, NAND_CMD_SET_FEATURES, false);
+	mxc_do_addr_cycle(mtd, addr, -1);
+	host->devtype_data->send_page(mtd, NFC_INPUT);
+
+	return 0;
+}
+
+static int mxc_nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *chip,
+			int addr, uint8_t *subfeature_param)
+{
+	struct nand_chip *nand_chip = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
+	int i;
+
+	if (!chip->onfi_version ||
+	    !(le16_to_cpu(chip->onfi_params.opt_cmd)
+	      & ONFI_OPT_CMD_SET_GET_FEATURES))
+		return -EINVAL;
+
+	*(uint32_t *)host->main_area0 = 0xdeadbeef;
+
+	host->devtype_data->send_cmd(host, NAND_CMD_GET_FEATURES, false);
+	mxc_do_addr_cycle(mtd, addr, -1);
+	host->devtype_data->send_page(mtd, NFC_OUTPUT);
+	memcpy32_fromio(host->data_buf, host->main_area0, 512);
+	host->buf_start = 0;
+
+	for (i = 0; i < ONFI_SUBFEATURE_PARAM_LEN; ++i)
+		*subfeature_param++ = chip->read_byte(mtd);
+
+	return 0;
+}
+
 /*
  * The generic flash bbt decriptors overlap with our ecc
  * hardware, so define some i.MX specific ones.
@@ -1513,6 +1564,8 @@ static int mxcnd_probe(struct platform_device *pdev)
 	this->read_word = mxc_nand_read_word;
 	this->write_buf = mxc_nand_write_buf;
 	this->read_buf = mxc_nand_read_buf;
+	this->onfi_set_features = mxc_nand_onfi_set_features;
+	this->onfi_get_features = mxc_nand_onfi_get_features;
 
 	host->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(host->clk))
-- 
2.8.1

[PATCH 6/7] mtd: nand: mxc: Add timing setup 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 | 84 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 82 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 1db8299..0d34d72 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,82 @@ 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;
+
+	timings = nand_get_sdr_timings(conf);
+	if (IS_ERR(timings))
+		return -ENOTSUPP;
+
+	config1 = readw(NFC_V1_V2_CONFIG1);
+
+	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 *= 2;
+		rate_round = clk_round_rate(host->clk, rate);
+		config1 &= ~NFC_V2_CONFIG1_ONE_CYCLE;
+		tRC_ps = 1000000000 / (rate_round / 1000 / 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;
+	}
+
+	if (check_only)
+		return 0;
+
+	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);
@@ -1276,8 +1355,6 @@ static int mxc_nand_onfi_get_features(struct mtd_info *mtd, struct nand_chip *ch
 	      & ONFI_OPT_CMD_SET_GET_FEATURES))
 		return -EINVAL;
 
-	*(uint32_t *)host->main_area0 = 0xdeadbeef;
-
 	host->devtype_data->send_cmd(host, NAND_CMD_GET_FEATURES, false);
 	mxc_do_addr_cycle(mtd, addr, -1);
 	host->devtype_data->send_page(mtd, NFC_OUTPUT);
@@ -1378,6 +1455,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,
@@ -1586,6 +1664,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 7/7] mtd: nand: remove unnecessary 'extern' from function declarations

[Sascha Hauer]

'extern' is not necessary for function declarations. To prevent
people from adding the keyword to new declarations remove the
existing ones.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 include/linux/mtd/nand.h | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 9f3d7be..7989178 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -29,26 +29,26 @@ struct nand_flash_dev;
 struct device_node;
 
 /* Scan and identify a NAND device */
-extern int nand_scan(struct mtd_info *mtd, int max_chips);
+int nand_scan(struct mtd_info *mtd, int max_chips);
 /*
  * Separate phases of nand_scan(), allowing board driver to intervene
  * and override command or ECC setup according to flash type.
  */
-extern int nand_scan_ident(struct mtd_info *mtd, int max_chips,
+int nand_scan_ident(struct mtd_info *mtd, int max_chips,
 			   struct nand_flash_dev *table);
-extern int nand_scan_tail(struct mtd_info *mtd);
+int nand_scan_tail(struct mtd_info *mtd);
 
 /* Free resources held by the NAND device */
-extern void nand_release(struct mtd_info *mtd);
+void nand_release(struct mtd_info *mtd);
 
 /* Internal helper for board drivers which need to override command function */
-extern void nand_wait_ready(struct mtd_info *mtd);
+void nand_wait_ready(struct mtd_info *mtd);
 
 /* locks all blocks present in the device */
-extern int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
+int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 
 /* unlocks specified locked blocks */
-extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
+int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
 
 /* The maximum number of NAND chips in an array */
 #define NAND_MAX_CHIPS		8
@@ -1013,14 +1013,14 @@ struct nand_manufacturers {
 extern struct nand_flash_dev nand_flash_ids[];
 extern struct nand_manufacturers nand_manuf_ids[];
 
-extern int nand_default_bbt(struct mtd_info *mtd);
-extern int nand_markbad_bbt(struct mtd_info *mtd, loff_t offs);
-extern int nand_isreserved_bbt(struct mtd_info *mtd, loff_t offs);
-extern int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt);
-extern int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
-			   int allowbbt);
-extern int nand_do_read(struct mtd_info *mtd, loff_t from, size_t len,
-			size_t *retlen, uint8_t *buf);
+int nand_default_bbt(struct mtd_info *mtd);
+int nand_markbad_bbt(struct mtd_info *mtd, loff_t offs);
+int nand_isreserved_bbt(struct mtd_info *mtd, loff_t offs);
+int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt);
+int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
+		    int allowbbt);
+int nand_do_read(struct mtd_info *mtd, loff_t from, size_t len,
+		 size_t *retlen, uint8_t *buf);
 
 /**
  * struct platform_nand_chip - chip level device structure
-- 
2.8.1

[PATCH 1/7] mtd: nand: Create a NAND reset function

[Boris Brezillon]

On Wed,  7 Sep 2016 14:21:36 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> When NAND devices are resetted some initialization may have to be done,
> like for example they have to be configured for the timing mode that
> shall be used. To get a common place where this initialization can be
> implemented create a nand_reset() function. This currently only issues
> a NAND_CMD_RESET to the NAND device. The places issuing this command
> manually are replaced with a call to nand_reset().
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/nand_base.c | 23 ++++++++++++++++++-----
>  include/linux/mtd/nand.h     |  4 ++++
>  2 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 77533f7..1f704cc 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -948,6 +948,19 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
>  }
>  
>  /**
> + * nand_reset - Reset and initialize a NAND device
> + * @chip: The NAND chip
> + *
> + * Returns 0 for success or negative error code otherwise
> + */
> +int nand_reset(struct nand_chip *chip)
> +{
> +	chip->cmdfunc(&chip->mtd, NAND_CMD_RESET, -1, -1);

		      ^ nand_to_mtd(chip)

Otherwise, the patch looks good.

> +
> +	return 0;
> +}
> +
> +/**
>   * __nand_unlock - [REPLACEABLE] unlocks specified locked blocks
>   * @mtd: mtd info
>   * @ofs: offset to start unlock from
> @@ -1025,7 +1038,7 @@ int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	 * some operation can also clear the bit 7 of status register
>  	 * eg. erase/program a locked block
>  	 */
> -	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +	nand_reset(chip);
>  
>  	/* Check, if it is write protected */
>  	if (nand_check_wp(mtd)) {
> @@ -1084,7 +1097,7 @@ int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  	 * some operation can also clear the bit 7 of status register
>  	 * eg. erase/program a locked block
>  	 */
> -	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +	nand_reset(chip);
>  
>  	/* Check, if it is write protected */
>  	if (nand_check_wp(mtd)) {
> @@ -2788,7 +2801,7 @@ static int nand_do_write_oob(struct mtd_info *mtd, loff_t to,
>  	 * if we don't do this. I have no clue why, but I seem to have 'fixed'
>  	 * it in the doc2000 driver in August 1999.  dwmw2.
>  	 */
> -	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +	nand_reset(chip);
>  
>  	/* Check, if it is write protected */
>  	if (nand_check_wp(mtd)) {
> @@ -3829,7 +3842,7 @@ static struct nand_flash_dev *nand_get_flash_type(struct mtd_info *mtd,
>  	 * Reset the chip, required by some chips (e.g. Micron MT29FxGxxxxx)
>  	 * after power-up.
>  	 */
> -	chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +	nand_reset(chip);
>  
>  	/* Send the command for reading device ID */
>  	chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
> @@ -4161,7 +4174,7 @@ int nand_scan_ident(struct mtd_info *mtd, int maxchips,
>  	for (i = 1; i < maxchips; i++) {
>  		chip->select_chip(mtd, i);
>  		/* See comment in nand_get_flash_type for reset */
> -		chip->cmdfunc(mtd, NAND_CMD_RESET, -1, -1);
> +		nand_reset(chip);
>  		/* Send the command for reading device ID */
>  		chip->cmdfunc(mtd, NAND_CMD_READID, 0x00, -1);
>  		/* Read manufacturer and device IDs */
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 8dd6e01..9890df2 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -1093,4 +1093,8 @@ int nand_read_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
>  /* Default read_oob syndrome implementation */
>  int nand_read_oob_syndrome(struct mtd_info *mtd, struct nand_chip *chip,
>  			   int page);
> +
> +/* Reset and initialize a NAND device */
> +int nand_reset(struct nand_chip *chip);
> +
>  #endif /* __LINUX_MTD_NAND_H */

[PATCH 3/7] mtd: nand: automate NAND timings selection

[Boris Brezillon]

On Wed,  7 Sep 2016 14:21:38 +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. Also currently NAND
> devices can be resetted at arbitrary places which also resets the timing
> for ONFI chips to timing mode 0.
> 
> Provide a common logic to select the best timings based on ONFI or
> ->onfi_timing_mode_default information. Hook this into nand_reset()  
> to make sure the new timing is applied each time during a reset.
> 
> 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 | 134 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/nand.h     |  12 ++--
>  2 files changed, 142 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 1f704cc..c9bc988 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -948,6 +948,130 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
>  }
>  
>  /**
> + * nand_reset_data_interface - Reset data interface and timings
> + * @chip: The NAND chip
> + *
> + * Reset the Data interface and timings to ONFI mode 0.
> + *
> + * Returns 0 for success or negative error code otherwise.
> + */
> +static int nand_reset_data_interface(struct nand_chip *chip)
> +{
> +	struct mtd_info *mtd = &chip->mtd;
> +	struct nand_data_interface *conf;
> +	int ret;
> +
> +	if (!chip->setup_data_interface)
> +		return 0;
> +
> +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> +	if (!conf)
> +		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.
> +	 */
> +
> +	conf->type = NAND_SDR_IFACE,
> +	conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(0);
> +
> +	ret = chip->setup_data_interface(mtd, conf, false);
> +	if (ret)
> +		pr_err("Failed to configure data interface to SDR timing mode 0\n");

I just realized that going back to timing mode 0 is not needed if your
chip is not ONFI or JEDEC compliant. But that's not really an issue.

> +
> +	kfree(conf);
> +
> +	return ret;
> +}
> +
> +/**
> + * nand_setup_data_interface - Setup the best data interface and timings
> + * @chip: The NAND chip
> + *
> + * Find and configure the best data interface and NAND timings supported by
> + * the chip and the driver.
> + * 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 for success or negative error code otherwise.
> + */
> +static int nand_setup_data_interface(struct nand_chip *chip)
> +{
> +	struct mtd_info *mtd = &chip->mtd;
> +	struct nand_data_interface *conf;
> +	int modes, mode, ret;
> +
> +	if (!chip->setup_data_interface)
> +		return 0;
> +
> +	/*
> +	 * 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) {
> +		if (!chip->onfi_timing_mode_default)
> +			return 0;
> +
> +		modes = GENMASK(chip->onfi_timing_mode_default, 0);
> +	}

This implementation is assuming the chip is either ONFI compliant or
not compliant with JEDEC and ONFI, but JEDEC chips also provides
'timing modes', except it's called 'speed grades'. Not sure how they
match to the ONFI timing modes, and I'm definitely not asking you to
support that right now, but that would be good to split the different
cases (ONFI compliant, JEDEC compliant and not compliant with JEDEC or
ONFI) in different functions.

Those functions would just fill in the nand_data_interface object, and 
nand_setup_data_interface() (or nand_select_data_interface(), if we
decide to split the logic in 2 different functions as suggested below)
would call one of them depending on the ->{jedec,onfo}_version values.

> +
> +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> +	if (!conf)
> +		return -ENOMEM;
> +
> +	ret = -EINVAL;
> +	for (mode = fls(modes) - 1; mode >= 0; mode--) {
> +		conf->type = NAND_SDR_IFACE,
> +		conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(mode);
> +
> +		ret = chip->setup_data_interface(mtd, conf, true);
> +		if (!ret) {
> +			chip->onfi_timing_mode_default = mode;
> +			break;
> +		}
> +	}
> +
> +	if (ret)
> +		goto err;

The data interface config selection only has to be done once: when you
discover/init the chip...

> +
> +	/*
> +	 * Ensure the timing mode has been changed on the chip side
> +	 * before changing timings on the controller side.
> +	 */
> +	if (chip->onfi_version) {
> +		uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
> +			chip->onfi_timing_mode_default,
> +		};
> +
> +		ret = chip->onfi_set_features(mtd, chip,
> +				ONFI_FEATURE_ADDR_TIMING_MODE,
> +				tmode_param);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	ret = chip->setup_data_interface(mtd, conf, false);

... but the setup will be done each time you reset the chip.

Maybe that's what you were trying to explain me yesterday.

Anyway, I really think we should keep the default/best data interface
config in nand_chip so that we can later re-use it without have to go
through the supported timing detection logic.


> +
> +err:
> +	kfree(conf);
> +
> +	return ret;
> +}
> +
> +/**
>   * nand_reset - Reset and initialize a NAND device
>   * @chip: The NAND chip
>   *
> @@ -955,8 +1079,18 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
>   */
>  int nand_reset(struct nand_chip *chip)
>  {
> +	int ret;
> +
> +	ret = nand_reset_data_interface(chip);
> +	if (ret)
> +		return ret;
> +
>  	chip->cmdfunc(&chip->mtd, NAND_CMD_RESET, -1, -1);
>  
> +	ret = nand_setup_data_interface(chip);
> +	if (ret)
> +		return ret;
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index f305bed..9f3d7be 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -743,10 +743,9 @@ nand_get_sdr_timings(const struct nand_data_interface *conf)
>   *                      also from the datasheet. It is the recommended ECC step
>   *			size, if known; if unknown, set to zero.
>   * @onfi_timing_mode_default: [INTERN] default ONFI timing mode. This field is
> - *			      either deduced from the datasheet if the NAND
> - *			      chip is not ONFI compliant or set to 0 if it is
> - *			      (an ONFI chip is always configured in mode 0
> - *			      after a NAND reset)
> + * 			      set to the actually used ONFI mode if the chip is
> + * 			      ONFI compliant or deduced from the datasheet if
> + * 			      the NAND chip is not ONFI compliant.
>   * @numchips:		[INTERN] number of physical chips
>   * @chipsize:		[INTERN] the size of one chip for multichip arrays
>   * @pagemask:		[INTERN] page number mask = number of (pages / chip) - 1
> @@ -766,6 +765,7 @@ nand_get_sdr_timings(const struct nand_data_interface *conf)
>   * @read_retries:	[INTERN] the number of read retry modes supported
>   * @onfi_set_features:	[REPLACEABLE] set the features for ONFI nand
>   * @onfi_get_features:	[REPLACEABLE] get the features for ONFI nand
> + * @setup_data_interface: [OPTIONAL] setup the data interface and timing
>   * @bbt:		[INTERN] bad block table pointer
>   * @bbt_td:		[REPLACEABLE] bad block table descriptor for flash
>   *			lookup.
> @@ -812,6 +812,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);
> +

I'd like to keep the current data_iface config in nand_chip. Just in
case we need to expose this information through debugfs.

>  
>  	int chip_delay;
>  	unsigned int options;

[PATCH 3/7] mtd: nand: automate NAND timings selection

[Sascha Hauer]

On Wed, Sep 07, 2016 at 03:41:14PM +0200, Boris Brezillon wrote:
> On Wed,  7 Sep 2016 14:21:38 +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. Also currently NAND
> > devices can be resetted at arbitrary places which also resets the timing
> > for ONFI chips to timing mode 0.
> > 
> > Provide a common logic to select the best timings based on ONFI or
> > ->onfi_timing_mode_default information. Hook this into nand_reset()  
> > to make sure the new timing is applied each time during a reset.
> > 
> > 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 | 134 +++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/mtd/nand.h     |  12 ++--
> >  2 files changed, 142 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > index 1f704cc..c9bc988 100644
> > --- a/drivers/mtd/nand/nand_base.c
> > +++ b/drivers/mtd/nand/nand_base.c
> > @@ -948,6 +948,130 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> >  }
> >  
> >  /**
> > + * nand_reset_data_interface - Reset data interface and timings
> > + * @chip: The NAND chip
> > + *
> > + * Reset the Data interface and timings to ONFI mode 0.
> > + *
> > + * Returns 0 for success or negative error code otherwise.
> > + */
> > +static int nand_reset_data_interface(struct nand_chip *chip)
> > +{
> > +	struct mtd_info *mtd = &chip->mtd;
> > +	struct nand_data_interface *conf;
> > +	int ret;
> > +
> > +	if (!chip->setup_data_interface)
> > +		return 0;
> > +
> > +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > +	if (!conf)
> > +		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.
> > +	 */
> > +
> > +	conf->type = NAND_SDR_IFACE,
> > +	conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(0);
> > +
> > +	ret = chip->setup_data_interface(mtd, conf, false);
> > +	if (ret)
> > +		pr_err("Failed to configure data interface to SDR timing mode 0\n");
> 
> I just realized that going back to timing mode 0 is not needed if your
> chip is not ONFI or JEDEC compliant. But that's not really an issue.
> 
> > +
> > +	kfree(conf);
> > +
> > +	return ret;
> > +}
> > +
> > +/**
> > + * nand_setup_data_interface - Setup the best data interface and timings
> > + * @chip: The NAND chip
> > + *
> > + * Find and configure the best data interface and NAND timings supported by
> > + * the chip and the driver.
> > + * 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 for success or negative error code otherwise.
> > + */
> > +static int nand_setup_data_interface(struct nand_chip *chip)
> > +{
> > +	struct mtd_info *mtd = &chip->mtd;
> > +	struct nand_data_interface *conf;
> > +	int modes, mode, ret;
> > +
> > +	if (!chip->setup_data_interface)
> > +		return 0;
> > +
> > +	/*
> > +	 * 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) {
> > +		if (!chip->onfi_timing_mode_default)
> > +			return 0;
> > +
> > +		modes = GENMASK(chip->onfi_timing_mode_default, 0);
> > +	}
> 
> This implementation is assuming the chip is either ONFI compliant or
> not compliant with JEDEC and ONFI, but JEDEC chips also provides
> 'timing modes', except it's called 'speed grades'. Not sure how they
> match to the ONFI timing modes, and I'm definitely not asking you to
> support that right now, but that would be good to split the different
> cases (ONFI compliant, JEDEC compliant and not compliant with JEDEC or
> ONFI) in different functions.
> 
> Those functions would just fill in the nand_data_interface object, and 
> nand_setup_data_interface() (or nand_select_data_interface(), if we
> decide to split the logic in 2 different functions as suggested below)
> would call one of them depending on the ->{jedec,onfo}_version values.
> 
> > +
> > +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > +	if (!conf)
> > +		return -ENOMEM;
> > +
> > +	ret = -EINVAL;
> > +	for (mode = fls(modes) - 1; mode >= 0; mode--) {
> > +		conf->type = NAND_SDR_IFACE,
> > +		conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(mode);
> > +
> > +		ret = chip->setup_data_interface(mtd, conf, true);
> > +		if (!ret) {
> > +			chip->onfi_timing_mode_default = mode;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (ret)
> > +		goto err;
> 
> The data interface config selection only has to be done once: when you
> discover/init the chip...
> 
> > +
> > +	/*
> > +	 * Ensure the timing mode has been changed on the chip side
> > +	 * before changing timings on the controller side.
> > +	 */
> > +	if (chip->onfi_version) {
> > +		uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
> > +			chip->onfi_timing_mode_default,
> > +		};
> > +
> > +		ret = chip->onfi_set_features(mtd, chip,
> > +				ONFI_FEATURE_ADDR_TIMING_MODE,
> > +				tmode_param);
> > +		if (ret)
> > +			goto err;
> > +	}
> > +
> > +	ret = chip->setup_data_interface(mtd, conf, false);
> 
> ... but the setup will be done each time you reset the chip.
> 
> Maybe that's what you were trying to explain me yesterday.

Indeed.

> 
> Anyway, I really think we should keep the default/best data interface
> config in nand_chip so that we can later re-use it without have to go
> through the supported timing detection logic.

Ok, I think now we are understanding each other. So I keep two timing
instances in struct nand_chip, one for initialization and one optimized
timing. They both get initialized once during chip detection and can be
reused when needed.

Note that the default timing is the same for all chips, so I decided to
turn the onfi_sdr_timings table into struct nand_data_interface so that
I can access this default timing without having to allocate an instance
for each chip.

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 3/7] mtd: nand: automate NAND timings selection

[Boris Brezillon]

On Wed, 7 Sep 2016 16:36:15 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Wed, Sep 07, 2016 at 03:41:14PM +0200, Boris Brezillon wrote:
> > On Wed,  7 Sep 2016 14:21:38 +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. Also currently NAND
> > > devices can be resetted at arbitrary places which also resets the timing
> > > for ONFI chips to timing mode 0.
> > > 
> > > Provide a common logic to select the best timings based on ONFI or  
> > > ->onfi_timing_mode_default information. Hook this into nand_reset()    
> > > to make sure the new timing is applied each time during a reset.
> > > 
> > > 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 | 134 +++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mtd/nand.h     |  12 ++--
> > >  2 files changed, 142 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > index 1f704cc..c9bc988 100644
> > > --- a/drivers/mtd/nand/nand_base.c
> > > +++ b/drivers/mtd/nand/nand_base.c
> > > @@ -948,6 +948,130 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> > >  }
> > >  
> > >  /**
> > > + * nand_reset_data_interface - Reset data interface and timings
> > > + * @chip: The NAND chip
> > > + *
> > > + * Reset the Data interface and timings to ONFI mode 0.
> > > + *
> > > + * Returns 0 for success or negative error code otherwise.
> > > + */
> > > +static int nand_reset_data_interface(struct nand_chip *chip)
> > > +{
> > > +	struct mtd_info *mtd = &chip->mtd;
> > > +	struct nand_data_interface *conf;
> > > +	int ret;
> > > +
> > > +	if (!chip->setup_data_interface)
> > > +		return 0;
> > > +
> > > +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > > +	if (!conf)
> > > +		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.
> > > +	 */
> > > +
> > > +	conf->type = NAND_SDR_IFACE,
> > > +	conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(0);
> > > +
> > > +	ret = chip->setup_data_interface(mtd, conf, false);
> > > +	if (ret)
> > > +		pr_err("Failed to configure data interface to SDR timing mode 0\n");  
> > 
> > I just realized that going back to timing mode 0 is not needed if your
> > chip is not ONFI or JEDEC compliant. But that's not really an issue.
> >   
> > > +
> > > +	kfree(conf);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/**
> > > + * nand_setup_data_interface - Setup the best data interface and timings
> > > + * @chip: The NAND chip
> > > + *
> > > + * Find and configure the best data interface and NAND timings supported by
> > > + * the chip and the driver.
> > > + * 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 for success or negative error code otherwise.
> > > + */
> > > +static int nand_setup_data_interface(struct nand_chip *chip)
> > > +{
> > > +	struct mtd_info *mtd = &chip->mtd;
> > > +	struct nand_data_interface *conf;
> > > +	int modes, mode, ret;
> > > +
> > > +	if (!chip->setup_data_interface)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * 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) {
> > > +		if (!chip->onfi_timing_mode_default)
> > > +			return 0;
> > > +
> > > +		modes = GENMASK(chip->onfi_timing_mode_default, 0);
> > > +	}  
> > 
> > This implementation is assuming the chip is either ONFI compliant or
> > not compliant with JEDEC and ONFI, but JEDEC chips also provides
> > 'timing modes', except it's called 'speed grades'. Not sure how they
> > match to the ONFI timing modes, and I'm definitely not asking you to
> > support that right now, but that would be good to split the different
> > cases (ONFI compliant, JEDEC compliant and not compliant with JEDEC or
> > ONFI) in different functions.
> > 
> > Those functions would just fill in the nand_data_interface object, and 
> > nand_setup_data_interface() (or nand_select_data_interface(), if we
> > decide to split the logic in 2 different functions as suggested below)
> > would call one of them depending on the ->{jedec,onfo}_version values.
> >   
> > > +
> > > +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > > +	if (!conf)
> > > +		return -ENOMEM;
> > > +
> > > +	ret = -EINVAL;
> > > +	for (mode = fls(modes) - 1; mode >= 0; mode--) {
> > > +		conf->type = NAND_SDR_IFACE,
> > > +		conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(mode);
> > > +
> > > +		ret = chip->setup_data_interface(mtd, conf, true);
> > > +		if (!ret) {
> > > +			chip->onfi_timing_mode_default = mode;
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	if (ret)
> > > +		goto err;  
> > 
> > The data interface config selection only has to be done once: when you
> > discover/init the chip...
> >   
> > > +
> > > +	/*
> > > +	 * Ensure the timing mode has been changed on the chip side
> > > +	 * before changing timings on the controller side.
> > > +	 */
> > > +	if (chip->onfi_version) {
> > > +		uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
> > > +			chip->onfi_timing_mode_default,
> > > +		};
> > > +
> > > +		ret = chip->onfi_set_features(mtd, chip,
> > > +				ONFI_FEATURE_ADDR_TIMING_MODE,
> > > +				tmode_param);
> > > +		if (ret)
> > > +			goto err;
> > > +	}
> > > +
> > > +	ret = chip->setup_data_interface(mtd, conf, false);  
> > 
> > ... but the setup will be done each time you reset the chip.
> > 
> > Maybe that's what you were trying to explain me yesterday.  
> 
> Indeed.
> 
> > 
> > Anyway, I really think we should keep the default/best data interface
> > config in nand_chip so that we can later re-use it without have to go
> > through the supported timing detection logic.  
> 
> Ok, I think now we are understanding each other. So I keep two timing
> instances in struct nand_chip, one for initialization and one optimized
> timing. They both get initialized once during chip detection and can be
> reused when needed.

Hm, not sure we need to keep 2 instances around, all we need to save is
the 'best_onfi_timing_mode', or we can just update
->default_onfi_timing_mode based on the result of the timing mode
detection, so that, when nand_setup_data_interface() is called, all we
have to do is:

	conf = chip->data_iface_conf;
	conf->type = NAND_SDR_IFACE,
	conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(chip->default_onfi_timing_mode);
	chip->setup_data_interface(mtd, conf, false);

> 
> Note that the default timing is the same for all chips, so I decided to
> turn the onfi_sdr_timings table into struct nand_data_interface so that
> I can access this default timing without having to allocate an instance
> for each chip.

As I said earlier, I'd like to avoid exposing the static sdr timings
definitions, and certainly not let nand_chip point to one of these.
I know the existing implementation does not follow this rule, since
onfi_async_timing_mode_to_sdr_timings() returns a
const struct nand_sdr_timings pointer, but that's something I'd like.
See my previous proposal:

int onfi_init_data_interface(struct nand_chip *chip,
			     struct nand_data_interface *iface,
			     enum nand_data_interface_type type,
			     int timing_mode)
{
	if (type != NAND_SDR_IFACE)
		return -EINVAL;

	if (timing_mode < 0 ||
	    timing_mode >= ARRAY_SIZE(onfi_sdr_timings))
		return -EINVAL;

	iface->type = NAND_SDR_IFACE;
	iface->timings.sdr = onfi_sdr_timings[timing_mode];

	/*
	 * TODO: initialize timings that cannot be deduced from timing mode:
	 * tR, tPROG, tCCS, ...
	 * These information are part of the ONFI parameter page.
	 */

	return 0;
}

[PATCH 3/7] mtd: nand: automate NAND timings selection

[Boris Brezillon]

On Wed, 7 Sep 2016 16:59:51 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Wed, 7 Sep 2016 16:36:15 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > On Wed, Sep 07, 2016 at 03:41:14PM +0200, Boris Brezillon wrote:  
> > > On Wed,  7 Sep 2016 14:21:38 +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. Also currently NAND
> > > > devices can be resetted at arbitrary places which also resets the timing
> > > > for ONFI chips to timing mode 0.
> > > > 
> > > > Provide a common logic to select the best timings based on ONFI or    
> > > > ->onfi_timing_mode_default information. Hook this into nand_reset()      
> > > > to make sure the new timing is applied each time during a reset.
> > > > 
> > > > 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 | 134 +++++++++++++++++++++++++++++++++++++++++++
> > > >  include/linux/mtd/nand.h     |  12 ++--
> > > >  2 files changed, 142 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> > > > index 1f704cc..c9bc988 100644
> > > > --- a/drivers/mtd/nand/nand_base.c
> > > > +++ b/drivers/mtd/nand/nand_base.c
> > > > @@ -948,6 +948,130 @@ static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> > > >  }
> > > >  
> > > >  /**
> > > > + * nand_reset_data_interface - Reset data interface and timings
> > > > + * @chip: The NAND chip
> > > > + *
> > > > + * Reset the Data interface and timings to ONFI mode 0.
> > > > + *
> > > > + * Returns 0 for success or negative error code otherwise.
> > > > + */
> > > > +static int nand_reset_data_interface(struct nand_chip *chip)
> > > > +{
> > > > +	struct mtd_info *mtd = &chip->mtd;
> > > > +	struct nand_data_interface *conf;
> > > > +	int ret;
> > > > +
> > > > +	if (!chip->setup_data_interface)
> > > > +		return 0;
> > > > +
> > > > +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > > > +	if (!conf)
> > > > +		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.
> > > > +	 */
> > > > +
> > > > +	conf->type = NAND_SDR_IFACE,
> > > > +	conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(0);
> > > > +
> > > > +	ret = chip->setup_data_interface(mtd, conf, false);
> > > > +	if (ret)
> > > > +		pr_err("Failed to configure data interface to SDR timing mode 0\n");    
> > > 
> > > I just realized that going back to timing mode 0 is not needed if your
> > > chip is not ONFI or JEDEC compliant. But that's not really an issue.
> > >     
> > > > +
> > > > +	kfree(conf);
> > > > +
> > > > +	return ret;
> > > > +}
> > > > +
> > > > +/**
> > > > + * nand_setup_data_interface - Setup the best data interface and timings
> > > > + * @chip: The NAND chip
> > > > + *
> > > > + * Find and configure the best data interface and NAND timings supported by
> > > > + * the chip and the driver.
> > > > + * 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 for success or negative error code otherwise.
> > > > + */
> > > > +static int nand_setup_data_interface(struct nand_chip *chip)
> > > > +{
> > > > +	struct mtd_info *mtd = &chip->mtd;
> > > > +	struct nand_data_interface *conf;
> > > > +	int modes, mode, ret;
> > > > +
> > > > +	if (!chip->setup_data_interface)
> > > > +		return 0;
> > > > +
> > > > +	/*
> > > > +	 * 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) {
> > > > +		if (!chip->onfi_timing_mode_default)
> > > > +			return 0;
> > > > +
> > > > +		modes = GENMASK(chip->onfi_timing_mode_default, 0);
> > > > +	}    
> > > 
> > > This implementation is assuming the chip is either ONFI compliant or
> > > not compliant with JEDEC and ONFI, but JEDEC chips also provides
> > > 'timing modes', except it's called 'speed grades'. Not sure how they
> > > match to the ONFI timing modes, and I'm definitely not asking you to
> > > support that right now, but that would be good to split the different
> > > cases (ONFI compliant, JEDEC compliant and not compliant with JEDEC or
> > > ONFI) in different functions.
> > > 
> > > Those functions would just fill in the nand_data_interface object, and 
> > > nand_setup_data_interface() (or nand_select_data_interface(), if we
> > > decide to split the logic in 2 different functions as suggested below)
> > > would call one of them depending on the ->{jedec,onfo}_version values.
> > >     
> > > > +
> > > > +	conf = kzalloc(sizeof(*conf), GFP_KERNEL);
> > > > +	if (!conf)
> > > > +		return -ENOMEM;
> > > > +
> > > > +	ret = -EINVAL;
> > > > +	for (mode = fls(modes) - 1; mode >= 0; mode--) {
> > > > +		conf->type = NAND_SDR_IFACE,
> > > > +		conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(mode);
> > > > +
> > > > +		ret = chip->setup_data_interface(mtd, conf, true);
> > > > +		if (!ret) {
> > > > +			chip->onfi_timing_mode_default = mode;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (ret)
> > > > +		goto err;    
> > > 
> > > The data interface config selection only has to be done once: when you
> > > discover/init the chip...
> > >     
> > > > +
> > > > +	/*
> > > > +	 * Ensure the timing mode has been changed on the chip side
> > > > +	 * before changing timings on the controller side.
> > > > +	 */
> > > > +	if (chip->onfi_version) {
> > > > +		uint8_t tmode_param[ONFI_SUBFEATURE_PARAM_LEN] = {
> > > > +			chip->onfi_timing_mode_default,
> > > > +		};
> > > > +
> > > > +		ret = chip->onfi_set_features(mtd, chip,
> > > > +				ONFI_FEATURE_ADDR_TIMING_MODE,
> > > > +				tmode_param);
> > > > +		if (ret)
> > > > +			goto err;
> > > > +	}
> > > > +
> > > > +	ret = chip->setup_data_interface(mtd, conf, false);    
> > > 
> > > ... but the setup will be done each time you reset the chip.
> > > 
> > > Maybe that's what you were trying to explain me yesterday.    
> > 
> > Indeed.
> >   
> > > 
> > > Anyway, I really think we should keep the default/best data interface
> > > config in nand_chip so that we can later re-use it without have to go
> > > through the supported timing detection logic.    
> > 
> > Ok, I think now we are understanding each other. So I keep two timing
> > instances in struct nand_chip, one for initialization and one optimized
> > timing. They both get initialized once during chip detection and can be
> > reused when needed.  
> 
> Hm, not sure we need to keep 2 instances around, all we need to save is
> the 'best_onfi_timing_mode', or we can just update
> ->default_onfi_timing_mode based on the result of the timing mode  
> detection, so that, when nand_setup_data_interface() is called, all we
> have to do is:
> 
> 	conf = chip->data_iface_conf;
> 	conf->type = NAND_SDR_IFACE,
> 	conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(chip->default_onfi_timing_mode);
> 	chip->setup_data_interface(mtd, conf, false);
> 

After the discussion we had on IRC, I want to reconsider what I said.
How about having a global nand_default_data_iface_config that would
work will all chips (probably exposing mode 0 timings and an SDR
interface).
This one will be used even for DDR NANDs, because after a reset they
return back to SDR mode, timing mode 0.

Now, I keep thinking that other timing modes should not be directly
exposed.

Doing that should solve the problem you mentioned: when interacting with
multi-dies chips, chip->data_iface content is changed N times from mode
0 to mode X during a reset (where N is the number of dies in your chip).

Let me know what you think.

[PATCH 7/7] mtd: nand: remove unnecessary 'extern' from function declarations

[Boris Brezillon]

On Wed,  7 Sep 2016 14:21:42 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> 'extern' is not necessary for function declarations. To prevent
> people from adding the keyword to new declarations remove the
> existing ones.
> 

Applied.

Thanks,

Boris

> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  include/linux/mtd/nand.h | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 9f3d7be..7989178 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -29,26 +29,26 @@ struct nand_flash_dev;
>  struct device_node;
>  
>  /* Scan and identify a NAND device */
> -extern int nand_scan(struct mtd_info *mtd, int max_chips);
> +int nand_scan(struct mtd_info *mtd, int max_chips);
>  /*
>   * Separate phases of nand_scan(), allowing board driver to intervene
>   * and override command or ECC setup according to flash type.
>   */
> -extern int nand_scan_ident(struct mtd_info *mtd, int max_chips,
> +int nand_scan_ident(struct mtd_info *mtd, int max_chips,
>  			   struct nand_flash_dev *table);
> -extern int nand_scan_tail(struct mtd_info *mtd);
> +int nand_scan_tail(struct mtd_info *mtd);
>  
>  /* Free resources held by the NAND device */
> -extern void nand_release(struct mtd_info *mtd);
> +void nand_release(struct mtd_info *mtd);
>  
>  /* Internal helper for board drivers which need to override command function */
> -extern void nand_wait_ready(struct mtd_info *mtd);
> +void nand_wait_ready(struct mtd_info *mtd);
>  
>  /* locks all blocks present in the device */
> -extern int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
> +int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  
>  /* unlocks specified locked blocks */
> -extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
> +int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  
>  /* The maximum number of NAND chips in an array */
>  #define NAND_MAX_CHIPS		8
> @@ -1013,14 +1013,14 @@ struct nand_manufacturers {
>  extern struct nand_flash_dev nand_flash_ids[];
>  extern struct nand_manufacturers nand_manuf_ids[];
>  
> -extern int nand_default_bbt(struct mtd_info *mtd);
> -extern int nand_markbad_bbt(struct mtd_info *mtd, loff_t offs);
> -extern int nand_isreserved_bbt(struct mtd_info *mtd, loff_t offs);
> -extern int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt);
> -extern int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
> -			   int allowbbt);
> -extern int nand_do_read(struct mtd_info *mtd, loff_t from, size_t len,
> -			size_t *retlen, uint8_t *buf);
> +int nand_default_bbt(struct mtd_info *mtd);
> +int nand_markbad_bbt(struct mtd_info *mtd, loff_t offs);
> +int nand_isreserved_bbt(struct mtd_info *mtd, loff_t offs);
> +int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt);
> +int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
> +		    int allowbbt);
> +int nand_do_read(struct mtd_info *mtd, loff_t from, size_t len,
> +		 size_t *retlen, uint8_t *buf);
>  
>  /**
>   * struct platform_nand_chip - chip level device structure

[PATCH 3/7] mtd: nand: automate NAND timings selection

[Sascha Hauer]

On Wed, Sep 07, 2016 at 05:59:17PM +0200, Boris Brezillon wrote:
> On Wed, 7 Sep 2016 16:59:51 +0200
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Wed, 7 Sep 2016 16:36:15 +0200
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > 
> > > Ok, I think now we are understanding each other. So I keep two timing
> > > instances in struct nand_chip, one for initialization and one optimized
> > > timing. They both get initialized once during chip detection and can be
> > > reused when needed.  
> > 
> > Hm, not sure we need to keep 2 instances around, all we need to save is
> > the 'best_onfi_timing_mode', or we can just update
> > ->default_onfi_timing_mode based on the result of the timing mode  
> > detection, so that, when nand_setup_data_interface() is called, all we
> > have to do is:
> > 
> > 	conf = chip->data_iface_conf;
> > 	conf->type = NAND_SDR_IFACE,
> > 	conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(chip->default_onfi_timing_mode);
> > 	chip->setup_data_interface(mtd, conf, false);
> > 
> 
> After the discussion we had on IRC, I want to reconsider what I said.
> How about having a global nand_default_data_iface_config that would
> work will all chips (probably exposing mode 0 timings and an SDR
> interface).
> This one will be used even for DDR NANDs, because after a reset they
> return back to SDR mode, timing mode 0.
> 
> Now, I keep thinking that other timing modes should not be directly
> exposed.

sounds good. How do you think the default iface_config should be
exposed? Should I turn the onfi_sdr_timings array to struct
nand_data_interface like done before and add a accessor function for the
first element, something like:

const struct nand_data_interface *nand_default_data_interface(void);

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 3/7] mtd: nand: automate NAND timings selection

[Boris Brezillon]

On Thu, 8 Sep 2016 09:55:30 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Wed, Sep 07, 2016 at 05:59:17PM +0200, Boris Brezillon wrote:
> > On Wed, 7 Sep 2016 16:59:51 +0200
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >   
> > > On Wed, 7 Sep 2016 16:36:15 +0200
> > > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > >   
> > > > Ok, I think now we are understanding each other. So I keep two timing
> > > > instances in struct nand_chip, one for initialization and one optimized
> > > > timing. They both get initialized once during chip detection and can be
> > > > reused when needed.    
> > > 
> > > Hm, not sure we need to keep 2 instances around, all we need to save is
> > > the 'best_onfi_timing_mode', or we can just update  
> > > ->default_onfi_timing_mode based on the result of the timing mode    
> > > detection, so that, when nand_setup_data_interface() is called, all we
> > > have to do is:
> > > 
> > > 	conf = chip->data_iface_conf;
> > > 	conf->type = NAND_SDR_IFACE,
> > > 	conf->timings.sdr = *onfi_async_timing_mode_to_sdr_timings(chip->default_onfi_timing_mode);
> > > 	chip->setup_data_interface(mtd, conf, false);
> > >   
> > 
> > After the discussion we had on IRC, I want to reconsider what I said.
> > How about having a global nand_default_data_iface_config that would
> > work will all chips (probably exposing mode 0 timings and an SDR
> > interface).
> > This one will be used even for DDR NANDs, because after a reset they
> > return back to SDR mode, timing mode 0.
> > 
> > Now, I keep thinking that other timing modes should not be directly
> > exposed.  
> 
> sounds good. How do you think the default iface_config should be
> exposed? Should I turn the onfi_sdr_timings array to struct
> nand_data_interface like done before and add a accessor function for the
> first element, something like:
> 
> const struct nand_data_interface *nand_default_data_interface(void);

Sounds goods. Sorry if I changed my mind to finally get back to
something close to your initial proposal, but I must admit I didn't
know what I wanted exactly, and only realized when seeing your
implementation.

Thanks for your patience ;-).