[PATCH 0/2] Migrate the GPMI driver to use NAND core timings

[PATCH 0/2] Migrate the GPMI driver to use NAND core timings

[Miquel Raynal]

Hello,

The GPMI NAND controller driver has its own timings logic while the core
also has its own that should be preferred to avoid code duplication.
This series migrates the driver to use the ->setup_data_interface()
hook, preferred way to handle timings negotiation between controllers
and NAND chips.

This driver has stronger requirements in terms of respect of the ONFI
specification than the core has, and last time I post changes to this
section this was raised to me. Hence, the first patch effectively
applies the same requirements to the core. Doing this can slow down
several chips that do not follow entirely the specifications, but
another series is coming to add the possibility for NAND vendors to
flag certain features as "unsupported". See

    "mtd: nand: Check ONFI timings have been acked by the chip"

commit log for more details.

Thank you,
Miquèl


Miquel Raynal (2):
  mtd: nand: Check ONFI timings have been acked by the chip
  mtd: nand: gpmi: Support ->setup_data_interface()

 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 266 ++++++++++-----------------------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |  33 ++--
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 146 +++++++++---------
 drivers/mtd/nand/nand_base.c           |  12 ++
 4 files changed, 180 insertions(+), 277 deletions(-)

-- 
2.11.0

[PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip

[Miquel Raynal]

Choosing ONFI timings when ->onfi_set/get_features() calls are supported
by the NAND chip is a matter of reading the chip's ONFI parameter page
and telling the chip the chosen mode (between all of the supported ones)
with ->onfi_set_feature().

Add a check on whether the chip "acked" the timing mode or not.

This can be a problem for NAND chips that do not follow entirely the
ONFI specification. These chips actually support other modes than
"mode 0", but do not update the parameter page once a timing mode has
been selected. This issue will be addressed in another patch that will
add the feature to overwrite NAND chips features within the parameter
page, from the NAND chip driver.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/nand_base.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 96c97588e1ba..ea862be6a803 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -1236,6 +1236,18 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
 				tmode_param);
 		if (ret)
 			goto err;
+
+		ret = chip->onfi_get_features(mtd, chip,
+					      ONFI_FEATURE_ADDR_TIMING_MODE,
+					      tmode_param);
+		if (ret)
+			goto err;
+
+		if (tmode_param[0] != chip->onfi_timing_mode_default) {
+			pr_warn("timings mode %d not acknowledged by the NAND chip\n",
+				chip->onfi_timing_mode_default);
+			goto err;
+		}
 	}
 
 	ret = chip->setup_data_interface(mtd, chipnr, &chip->data_interface);
-- 
2.11.0

[PATCH 2/2] mtd: nand: gpmi: Support ->setup_data_interface()

[Miquel Raynal]

Until now the GPMI driver had its own timings logic while the core
already handles that and request the NAND controller drivers to support
the ->setup_data_interface() hook. Implement that hook by reusing the
already existing function. No real glue is necessary between core timing
delays and GPMI registers because the driver already translates the
ONFI timing modes into register values.

Make use of the core's tREA, tRLOH and tRHOH values that allow computing
more precise timings for mode [0-3] and get significantly better values
(+20% with an i.MX6 Sabre Auto board). Otherwise use the existing logic.

Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 266 ++++++++++-----------------------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |  33 ++--
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 146 +++++++++---------
 3 files changed, 168 insertions(+), 277 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index 97787246af41..ae47cd8414bf 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -151,8 +151,15 @@ static int __gpmi_enable_clk(struct gpmi_nand_data *this, bool v)
 	return ret;
 }
 
-#define gpmi_enable_clk(x) __gpmi_enable_clk(x, true)
-#define gpmi_disable_clk(x) __gpmi_enable_clk(x, false)
+inline int gpmi_enable_clk(struct gpmi_nand_data *this)
+{
+	return __gpmi_enable_clk(this, true);
+}
+
+inline int gpmi_disable_clk(struct gpmi_nand_data *this)
+{
+	return __gpmi_enable_clk(this, false);
+}
 
 int gpmi_init(struct gpmi_nand_data *this)
 {
@@ -326,11 +333,11 @@ static unsigned int ns_to_cycles(unsigned int time,
 #define DEF_MIN_PROP_DELAY	5
 #define DEF_MAX_PROP_DELAY	9
 /* Apply timing to current hardware conditions. */
-static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
-					struct gpmi_nfc_hardware_timing *hw)
+static void
+gpmi_nfc_compute_hardware_timings(struct gpmi_nand_data *this)
 {
+	struct gpmi_nfc_hardware_timing *hw = &this->hw;
 	struct timing_threshold *nfc = &timing_default_threshold;
-	struct resources *r = &this->resources;
 	struct nand_chip *nand = &this->nand;
 	struct nand_timing target = this->timing;
 	bool improved_timing_is_available;
@@ -349,6 +356,9 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
 	unsigned int min_prop_delay_in_ns = DEF_MIN_PROP_DELAY;
 	unsigned int max_prop_delay_in_ns = DEF_MAX_PROP_DELAY;
 
+	/* Clock rate for non-EDO modes */
+	hw->clk_rate = 22000000;
+
 	/*
 	 * If there are multiple chips, we need to relax the timings to allow
 	 * for signal distortion due to higher capacitance.
@@ -363,14 +373,8 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
 		target.address_setup_in_ns += 5;
 	}
 
-	/* Check if improved timing information is available. */
-	improved_timing_is_available =
-		(target.tREA_in_ns  >= 0) &&
-		(target.tRLOH_in_ns >= 0) &&
-		(target.tRHOH_in_ns >= 0);
-
 	/* Inspect the clock. */
-	nfc->clock_frequency_in_hz = clk_get_rate(r->clock[0]);
+	nfc->clock_frequency_in_hz = hw->clk_rate;
 	clock_frequency_in_hz = nfc->clock_frequency_in_hz;
 	clock_period_in_ns    = NSEC_PER_SEC / clock_frequency_in_hz;
 
@@ -482,65 +486,6 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
 	}
 
 	/*
-	 * Check if improved timing information is available. If not, we have to
-	 * use a less-sophisticated algorithm.
-	 */
-	if (!improved_timing_is_available) {
-		/*
-		 * Fold the read setup time required by the NFC into the ideal
-		 * sample delay.
-		 */
-		ideal_sample_delay_in_ns = target.gpmi_sample_delay_in_ns +
-						nfc->internal_data_setup_in_ns;
-
-		/*
-		 * The ideal sample delay may be greater than the maximum
-		 * allowed by the NFC. If so, we can trade off sample delay time
-		 * for more data setup time.
-		 *
-		 * In each iteration of the following loop, we add a cycle to
-		 * the data setup time and subtract a corresponding amount from
-		 * the sample delay until we've satisified the constraints or
-		 * can't do any better.
-		 */
-		while ((ideal_sample_delay_in_ns > max_sample_delay_in_ns) &&
-			(data_setup_in_cycles < nfc->max_data_setup_cycles)) {
-
-			data_setup_in_cycles++;
-			ideal_sample_delay_in_ns -= clock_period_in_ns;
-
-			if (ideal_sample_delay_in_ns < 0)
-				ideal_sample_delay_in_ns = 0;
-
-		}
-
-		/*
-		 * Compute the sample delay factor that corresponds most closely
-		 * to the ideal sample delay. If the result is too large for the
-		 * NFC, use the maximum value.
-		 *
-		 * Notice that we use the ns_to_cycles function to compute the
-		 * sample delay factor. We do this because the form of the
-		 * computation is the same as that for calculating cycles.
-		 */
-		sample_delay_factor =
-			ns_to_cycles(
-				ideal_sample_delay_in_ns << dll_delay_shift,
-							clock_period_in_ns, 0);
-
-		if (sample_delay_factor > nfc->max_sample_delay_factor)
-			sample_delay_factor = nfc->max_sample_delay_factor;
-
-		/* Skip to the part where we return our results. */
-		goto return_results;
-	}
-
-	/*
-	 * If control arrives here, we have more detailed timing information,
-	 * so we can use a better algorithm.
-	 */
-
-	/*
 	 * Fold the read setup time required by the NFC into the maximum
 	 * propagation delay.
 	 */
@@ -760,8 +705,6 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
 			sample_delay_factor = nfc->max_sample_delay_factor;
 	}
 
-	/* Control arrives here when we're ready to return our results. */
-return_results:
 	hw->data_setup_in_cycles    = data_setup_in_cycles;
 	hw->data_hold_in_cycles     = data_hold_in_cycles;
 	hw->address_setup_in_cycles = address_setup_in_cycles;
@@ -769,9 +712,6 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
 	hw->sample_delay_factor     = sample_delay_factor;
 	hw->device_busy_timeout     = GPMI_DEFAULT_BUSY_TIMEOUT;
 	hw->wrn_dly_sel             = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS;
-
-	/* Return success. */
-	return 0;
 }
 
 /*
@@ -857,12 +797,9 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
  *       So we only support the mode 4 and mode 5. It is no need to
  *       support other modes.
  */
-static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
-			struct gpmi_nfc_hardware_timing *hw)
+static void gpmi_nfc_compute_edo_timings(struct gpmi_nand_data *this)
 {
-	struct resources *r = &this->resources;
-	unsigned long rate = clk_get_rate(r->clock[0]);
-	int mode = this->timing_mode;
+	struct gpmi_nfc_hardware_timing *hw = &this->hw;
 	int dll_threshold = this->devdata->max_chain_delay;
 	unsigned long delay;
 	unsigned long clk_period;
@@ -871,6 +808,9 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
 	int t_rp;
 	int rp;
 
+	/* Set the main clock to: 100MHz (mode 5) or 80MHz (mode 4) */
+	hw->clk_rate = (hw->timing_mode == 5) ? 100000000 : 80000000;
+
 	/*
 	 * [1] for GPMI_HW_GPMI_TIMING0:
 	 *     The async mode requires 40MHz for mode 4, 50MHz for mode 5.
@@ -880,7 +820,7 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
 	 */
 	hw->data_setup_in_cycles = 1;
 	hw->data_hold_in_cycles = 1;
-	hw->address_setup_in_cycles = ((mode == 5) ? 1 : 0);
+	hw->address_setup_in_cycles = (hw->timing_mode == 5) ? 1 : 0;
 
 	/* [2] for GPMI_HW_GPMI_TIMING1 */
 	hw->device_busy_timeout = 0x9000;
@@ -892,9 +832,9 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
 	 * Enlarge 10 times for the numerator and denominator in {3}.
 	 * This make us to get more accurate result.
 	 */
-	clk_period = NSEC_PER_SEC / (rate / 10);
+	clk_period = NSEC_PER_SEC / (hw->clk_rate / 10);
 	dll_threshold *= 10;
-	t_rea = ((mode == 5) ? 16 : 20) * 10;
+	t_rea = (hw->timing_mode == 5 ? 16 : 20) * 10;
 	c *= 10;
 
 	t_rp = clk_period * 1; /* DATA_SETUP is 1 */
@@ -917,123 +857,36 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
 	hw->sample_delay_factor = delay;
 }
 
-static int enable_edo_mode(struct gpmi_nand_data *this, int mode)
-{
-	struct resources  *r = &this->resources;
-	struct nand_chip *nand = &this->nand;
-	struct mtd_info	 *mtd = nand_to_mtd(nand);
-	uint8_t *feature;
-	unsigned long rate;
-	int ret;
-
-	feature = kzalloc(ONFI_SUBFEATURE_PARAM_LEN, GFP_KERNEL);
-	if (!feature)
-		return -ENOMEM;
-
-	nand->select_chip(mtd, 0);
-
-	/* [1] send SET FEATURE command to NAND */
-	feature[0] = mode;
-	ret = nand->onfi_set_features(mtd, nand,
-				ONFI_FEATURE_ADDR_TIMING_MODE, feature);
-	if (ret)
-		goto err_out;
-
-	/* [2] send GET FEATURE command to double-check the timing mode */
-	memset(feature, 0, ONFI_SUBFEATURE_PARAM_LEN);
-	ret = nand->onfi_get_features(mtd, nand,
-				ONFI_FEATURE_ADDR_TIMING_MODE, feature);
-	if (ret || feature[0] != mode)
-		goto err_out;
-
-	nand->select_chip(mtd, -1);
-
-	/* [3] set the main IO clock, 100MHz for mode 5, 80MHz for mode 4. */
-	rate = (mode == 5) ? 100000000 : 80000000;
-	clk_set_rate(r->clock[0], rate);
-
-	/* Let the gpmi_begin() re-compute the timing again. */
-	this->flags &= ~GPMI_TIMING_INIT_OK;
-
-	this->flags |= GPMI_ASYNC_EDO_ENABLED;
-	this->timing_mode = mode;
-	kfree(feature);
-	dev_info(this->dev, "enable the asynchronous EDO mode %d\n", mode);
-	return 0;
-
-err_out:
-	nand->select_chip(mtd, -1);
-	kfree(feature);
-	dev_err(this->dev, "mode:%d ,failed in set feature.\n", mode);
-	return -EINVAL;
-}
-
-int gpmi_extra_init(struct gpmi_nand_data *this)
-{
-	struct nand_chip *chip = &this->nand;
-
-	/* Enable the asynchronous EDO feature. */
-	if (GPMI_IS_MX6(this) && chip->onfi_version) {
-		int mode = onfi_get_async_timing_mode(chip);
-
-		/* We only support the timing mode 4 and mode 5. */
-		if (mode & ONFI_TIMING_MODE_5)
-			mode = 5;
-		else if (mode & ONFI_TIMING_MODE_4)
-			mode = 4;
-		else
-			return 0;
-
-		return enable_edo_mode(this, mode);
-	}
-	return 0;
-}
-
 /* Begin the I/O */
-void gpmi_begin(struct gpmi_nand_data *this)
+void gpmi_nfc_apply_timings(struct gpmi_nand_data *this)
 {
+	struct gpmi_nfc_hardware_timing *hw = &this->hw;
 	struct resources *r = &this->resources;
 	void __iomem *gpmi_regs = r->gpmi_regs;
 	unsigned int   clock_period_in_ns;
 	uint32_t       reg;
 	unsigned int   dll_wait_time_in_us;
-	struct gpmi_nfc_hardware_timing  hw;
-	int ret;
-
-	/* Enable the clock. */
-	ret = gpmi_enable_clk(this);
-	if (ret) {
-		dev_err(this->dev, "We failed in enable the clk\n");
-		goto err_out;
-	}
 
-	/* Only initialize the timing once */
-	if (this->flags & GPMI_TIMING_INIT_OK)
-		return;
-	this->flags |= GPMI_TIMING_INIT_OK;
-
-	if (this->flags & GPMI_ASYNC_EDO_ENABLED)
-		gpmi_compute_edo_timing(this, &hw);
-	else
-		gpmi_nfc_compute_hardware_timing(this, &hw);
+	/* [0] Set the main clock rate */
+	clk_set_rate(r->clock[0], hw->clk_rate);
 
 	/* [1] Set HW_GPMI_TIMING0 */
-	reg = BF_GPMI_TIMING0_ADDRESS_SETUP(hw.address_setup_in_cycles) |
-		BF_GPMI_TIMING0_DATA_HOLD(hw.data_hold_in_cycles)         |
-		BF_GPMI_TIMING0_DATA_SETUP(hw.data_setup_in_cycles);
+	reg = BF_GPMI_TIMING0_ADDRESS_SETUP(hw->address_setup_in_cycles) |
+		BF_GPMI_TIMING0_DATA_HOLD(hw->data_hold_in_cycles) |
+		BF_GPMI_TIMING0_DATA_SETUP(hw->data_setup_in_cycles);
 
 	writel(reg, gpmi_regs + HW_GPMI_TIMING0);
 
 	/* [2] Set HW_GPMI_TIMING1 */
-	writel(BF_GPMI_TIMING1_BUSY_TIMEOUT(hw.device_busy_timeout),
-		gpmi_regs + HW_GPMI_TIMING1);
+	writel(BF_GPMI_TIMING1_BUSY_TIMEOUT(hw->device_busy_timeout),
+	       gpmi_regs + HW_GPMI_TIMING1);
 
 	/* [3] The following code is to set the HW_GPMI_CTRL1. */
 
 	/* Set the WRN_DLY_SEL */
 	writel(BM_GPMI_CTRL1_WRN_DLY_SEL, gpmi_regs + HW_GPMI_CTRL1_CLR);
-	writel(BF_GPMI_CTRL1_WRN_DLY_SEL(hw.wrn_dly_sel),
-					gpmi_regs + HW_GPMI_CTRL1_SET);
+	writel(BF_GPMI_CTRL1_WRN_DLY_SEL(hw->wrn_dly_sel),
+	       gpmi_regs + HW_GPMI_CTRL1_SET);
 
 	/* DLL_ENABLE must be set to 0 when setting RDN_DELAY or HALF_PERIOD. */
 	writel(BM_GPMI_CTRL1_DLL_ENABLE, gpmi_regs + HW_GPMI_CTRL1_CLR);
@@ -1043,12 +896,12 @@ void gpmi_begin(struct gpmi_nand_data *this)
 	writel(reg, gpmi_regs + HW_GPMI_CTRL1_CLR);
 
 	/* If no sample delay is called for, return immediately. */
-	if (!hw.sample_delay_factor)
+	if (!hw->sample_delay_factor)
 		return;
 
 	/* Set RDN_DELAY or HALF_PERIOD. */
-	reg = ((hw.use_half_periods) ? BM_GPMI_CTRL1_HALF_PERIOD : 0)
-		| BF_GPMI_CTRL1_RDN_DELAY(hw.sample_delay_factor);
+	reg = ((hw->use_half_periods) ? BM_GPMI_CTRL1_HALF_PERIOD : 0)
+		| BF_GPMI_CTRL1_RDN_DELAY(hw->sample_delay_factor);
 
 	writel(reg, gpmi_regs + HW_GPMI_CTRL1_SET);
 
@@ -1060,7 +913,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
 	 * we can use the GPMI. Calculate the amount of time we need to wait,
 	 * in microseconds.
 	 */
-	clock_period_in_ns = NSEC_PER_SEC / clk_get_rate(r->clock[0]);
+	clock_period_in_ns = NSEC_PER_SEC / hw->clk_rate;
 	dll_wait_time_in_us = (clock_period_in_ns * 64) / 1000;
 
 	if (!dll_wait_time_in_us)
@@ -1068,14 +921,45 @@ void gpmi_begin(struct gpmi_nand_data *this)
 
 	/* Wait for the DLL to settle. */
 	udelay(dll_wait_time_in_us);
-
-err_out:
-	return;
 }
 
-void gpmi_end(struct gpmi_nand_data *this)
+int gpmi_setup_data_interface(struct mtd_info *mtd, int chipnr,
+			      const struct nand_data_interface *conf)
 {
-	gpmi_disable_clk(this);
+	struct nand_chip *chip = mtd_to_nand(mtd);
+	struct gpmi_nand_data *this = nand_get_controller_data(chip);
+	const struct nand_sdr_timings *sdr;
+
+	if (chip->onfi_timing_mode_default < 0 ||
+	    chip->onfi_timing_mode_default > 5)
+		return -ENOTSUPP;
+
+	/* Only MX6 GPMI controller can reach EDO timings */
+	if (chip->onfi_timing_mode_default >= 4 && !GPMI_IS_MX6(this))
+		return -ENOTSUPP;
+
+	if (chipnr < 0)
+		return 0;
+
+	this->hw.timing_mode = chip->onfi_timing_mode_default;
+
+	sdr = nand_get_sdr_timings(conf);
+	if (IS_ERR(sdr))
+		return PTR_ERR(sdr);
+
+	this->timing.tREA_in_ns = sdr->tREA_max / 1000;
+	this->timing.tRLOH_in_ns = sdr->tRLOH_min / 1000;
+	this->timing.tRHOH_in_ns = sdr->tRHOH_min / 1000;
+
+	/* Compute GPMI parameters depending on the mode */
+	if (this->hw.timing_mode >= 4)
+		gpmi_nfc_compute_edo_timings(this);
+	else
+		gpmi_nfc_compute_hardware_timings(this);
+
+	gpmi_nfc_apply_timings(this);
+
+	return 0;
 }
 
 /* Clears a BCH interrupt. */
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index b51db8c85405..04986cd89c99 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -938,11 +938,23 @@ static void gpmi_select_chip(struct mtd_info *mtd, int chipnr)
 {
 	struct nand_chip *chip = mtd_to_nand(mtd);
 	struct gpmi_nand_data *this = nand_get_controller_data(chip);
+	int ret;
 
-	if ((this->current_chip < 0) && (chipnr >= 0))
-		gpmi_begin(this);
-	else if ((this->current_chip >= 0) && (chipnr < 0))
-		gpmi_end(this);
+	/*
+	 * This driver currently supports only one NAND chip. So once timings
+	 * have been decided, they will not change. Furthermore, dies share the
+	 * same configuration, so for power consumption matters, we only
+	 * disable/enable the clock.
+	 */
+	if (this->current_chip < 0 && chipnr >= 0) {
+		ret = gpmi_enable_clk(this);
+		if (ret)
+			dev_err(this->dev, "Failed to enable the clock\n");
+	} else if (this->current_chip >= 0 && chipnr < 0) {
+		ret = gpmi_disable_clk(this);
+		if (ret)
+			dev_err(this->dev, "Failed to disable the clock\n");
+	}
 
 	this->current_chip = chipnr;
 }
@@ -1947,14 +1959,6 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
 		chip->options |= NAND_SUBPAGE_READ;
 	}
 
-	/*
-	 * Can we enable the extra features? such as EDO or Sync mode.
-	 *
-	 * We do not check the return value now. That's means if we fail in
-	 * enable the extra features, we still can run in the normal way.
-	 */
-	gpmi_extra_init(this);
-
 	return 0;
 }
 
@@ -1975,6 +1979,7 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
 	nand_set_controller_data(chip, this);
 	nand_set_flash_node(chip, this->pdev->dev.of_node);
 	chip->select_chip	= gpmi_select_chip;
+	chip->setup_data_interface = gpmi_setup_data_interface;
 	chip->cmd_ctrl		= gpmi_cmd_ctrl;
 	chip->dev_ready		= gpmi_dev_ready;
 	chip->read_byte		= gpmi_read_byte;
@@ -2133,7 +2138,6 @@ static int gpmi_pm_resume(struct device *dev)
 		return ret;
 
 	/* re-init the GPMI registers */
-	this->flags &= ~GPMI_TIMING_INIT_OK;
 	ret = gpmi_init(this);
 	if (ret) {
 		dev_err(this->dev, "Error setting GPMI : %d\n", ret);
@@ -2147,9 +2151,6 @@ static int gpmi_pm_resume(struct device *dev)
 		return ret;
 	}
 
-	/* re-init others */
-	gpmi_extra_init(this);
-
 	return 0;
 }
 #endif /* CONFIG_PM_SLEEP */
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index 06c1f993912c..4890e1378475 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -135,11 +135,77 @@ struct gpmi_devdata {
 	const int clks_count;
 };
 
+/**
+ * struct gpmi_nfc_hardware_timing - GPMI hardware timing parameters.
+ * @timing_mode:               The timing mode to comply with.
+ * @clk_rate:                  The clock rate that must be used to derive the
+ *                             following parameters.
+ * @data_setup_in_cycles:      The data setup time, in cycles.
+ * @data_hold_in_cycles:       The data hold time, in cycles.
+ * @address_setup_in_cycles:   The address setup time, in cycles.
+ * @device_busy_timeout:       The timeout waiting for NAND Ready/Busy,
+ *                             this value is the number of cycles multiplied
+ *                             by 4096.
+ * @use_half_periods:          Indicates the clock is running slowly, so the
+ *                             NFC DLL should use half-periods.
+ * @sample_delay_factor:       The sample delay factor.
+ * @wrn_dly_sel:               The delay on the GPMI write strobe.
+ */
+struct gpmi_nfc_hardware_timing {
+	unsigned int timing_mode;
+	unsigned long int clk_rate;
+
+	/* for HW_GPMI_TIMING0 */
+	u8 data_setup_in_cycles;
+	u8 data_hold_in_cycles;
+	u8 address_setup_in_cycles;
+
+	/* for HW_GPMI_TIMING1 */
+	u16 device_busy_timeout;
+#define GPMI_DEFAULT_BUSY_TIMEOUT	0x500 /* default busy timeout value.*/
+
+	/* for HW_GPMI_CTRL1 */
+	bool use_half_periods;
+	u8 sample_delay_factor;
+	u8 wrn_dly_sel;
+};
+
+/**
+ * struct timing_threshold - Timing threshold
+ * @max_data_setup_cycles:       The maximum number of data setup cycles that
+ *                               can be expressed in the hardware.
+ * @internal_data_setup_in_ns:   The time, in ns, that the NFC hardware requires
+ *                               for data read internal setup. In the Reference
+ *                               Manual, see the chapter "High-Speed NAND
+ *                               Timing" for more details.
+ * @max_sample_delay_factor:     The maximum sample delay factor that can be
+ *                               expressed in the hardware.
+ * @max_dll_clock_period_in_ns:  The maximum period of the GPMI clock that the
+ *                               sample delay DLL hardware can possibly work
+ *                               with (the DLL is unusable with longer periods).
+ *                               If the full-cycle period is greater than HALF
+ *                               this value, the DLL must be configured to use
+ *                               half-periods.
+ * @max_dll_delay_in_ns:         The maximum amount of delay, in ns, that the
+ *                               DLL can implement.
+ * @clock_frequency_in_hz:       The clock frequency, in Hz, during the current
+ *                               I/O transaction. If no I/O transaction is in
+ *                               progress, this is the clock frequency during
+ *                               the most recent I/O transaction.
+ */
+struct timing_threshold {
+	const unsigned int      max_chip_count;
+	const unsigned int      max_data_setup_cycles;
+	const unsigned int      internal_data_setup_in_ns;
+	const unsigned int      max_sample_delay_factor;
+	const unsigned int      max_dll_clock_period_in_ns;
+	const unsigned int      max_dll_delay_in_ns;
+	unsigned long           clock_frequency_in_hz;
+
+};
+
 struct gpmi_nand_data {
-	/* flags */
-#define GPMI_ASYNC_EDO_ENABLED	(1 << 0)
-#define GPMI_TIMING_INIT_OK	(1 << 1)
-	int			flags;
+	/* Devdata */
 	const struct gpmi_devdata *devdata;
 
 	/* System Interface */
@@ -152,6 +218,7 @@ struct gpmi_nand_data {
 	/* Flash Hardware */
 	struct nand_timing	timing;
 	int			timing_mode;
+	struct gpmi_nfc_hardware_timing hw;
 
 	/* BCH */
 	struct bch_geometry	bch_geometry;
@@ -204,69 +271,6 @@ struct gpmi_nand_data {
 	void			*private;
 };
 
-/**
- * struct gpmi_nfc_hardware_timing - GPMI hardware timing parameters.
- * @data_setup_in_cycles:      The data setup time, in cycles.
- * @data_hold_in_cycles:       The data hold time, in cycles.
- * @address_setup_in_cycles:   The address setup time, in cycles.
- * @device_busy_timeout:       The timeout waiting for NAND Ready/Busy,
- *                             this value is the number of cycles multiplied
- *                             by 4096.
- * @use_half_periods:          Indicates the clock is running slowly, so the
- *                             NFC DLL should use half-periods.
- * @sample_delay_factor:       The sample delay factor.
- * @wrn_dly_sel:               The delay on the GPMI write strobe.
- */
-struct gpmi_nfc_hardware_timing {
-	/* for HW_GPMI_TIMING0 */
-	uint8_t  data_setup_in_cycles;
-	uint8_t  data_hold_in_cycles;
-	uint8_t  address_setup_in_cycles;
-
-	/* for HW_GPMI_TIMING1 */
-	uint16_t device_busy_timeout;
-#define GPMI_DEFAULT_BUSY_TIMEOUT	0x500 /* default busy timeout value.*/
-
-	/* for HW_GPMI_CTRL1 */
-	bool     use_half_periods;
-	uint8_t  sample_delay_factor;
-	uint8_t  wrn_dly_sel;
-};
-
-/**
- * struct timing_threshold - Timing threshold
- * @max_data_setup_cycles:       The maximum number of data setup cycles that
- *                               can be expressed in the hardware.
- * @internal_data_setup_in_ns:   The time, in ns, that the NFC hardware requires
- *                               for data read internal setup. In the Reference
- *                               Manual, see the chapter "High-Speed NAND
- *                               Timing" for more details.
- * @max_sample_delay_factor:     The maximum sample delay factor that can be
- *                               expressed in the hardware.
- * @max_dll_clock_period_in_ns:  The maximum period of the GPMI clock that the
- *                               sample delay DLL hardware can possibly work
- *                               with (the DLL is unusable with longer periods).
- *                               If the full-cycle period is greater than HALF
- *                               this value, the DLL must be configured to use
- *                               half-periods.
- * @max_dll_delay_in_ns:         The maximum amount of delay, in ns, that the
- *                               DLL can implement.
- * @clock_frequency_in_hz:       The clock frequency, in Hz, during the current
- *                               I/O transaction. If no I/O transaction is in
- *                               progress, this is the clock frequency during
- *                               the most recent I/O transaction.
- */
-struct timing_threshold {
-	const unsigned int      max_chip_count;
-	const unsigned int      max_data_setup_cycles;
-	const unsigned int      internal_data_setup_in_ns;
-	const unsigned int      max_sample_delay_factor;
-	const unsigned int      max_dll_clock_period_in_ns;
-	const unsigned int      max_dll_delay_in_ns;
-	unsigned long           clock_frequency_in_hz;
-
-};
-
 /* Common Services */
 int common_nfc_set_geometry(struct gpmi_nand_data *);
 struct dma_chan *get_dma_chan(struct gpmi_nand_data *);
@@ -279,14 +283,16 @@ int start_dma_with_bch_irq(struct gpmi_nand_data *,
 
 /* GPMI-NAND helper function library */
 int gpmi_init(struct gpmi_nand_data *);
-int gpmi_extra_init(struct gpmi_nand_data *);
 void gpmi_clear_bch(struct gpmi_nand_data *);
 void gpmi_dump_info(struct gpmi_nand_data *);
 int bch_set_geometry(struct gpmi_nand_data *);
 int gpmi_is_ready(struct gpmi_nand_data *, unsigned chip);
 int gpmi_send_command(struct gpmi_nand_data *);
-void gpmi_begin(struct gpmi_nand_data *);
-void gpmi_end(struct gpmi_nand_data *);
+int gpmi_enable_clk(struct gpmi_nand_data *this);
+int gpmi_disable_clk(struct gpmi_nand_data *this);
+int gpmi_setup_data_interface(struct mtd_info *mtd, int chipnr,
+			      const struct nand_data_interface *conf);
+void gpmi_nfc_apply_timings(struct gpmi_nand_data *this);
 int gpmi_read_data(struct gpmi_nand_data *);
 int gpmi_send_data(struct gpmi_nand_data *);
 int gpmi_send_page(struct gpmi_nand_data *,
-- 
2.11.0

Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip

[Boris Brezillon]

On Fri, 22 Dec 2017 18:28:52 +0100
Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Choosing ONFI timings when ->onfi_set/get_features() calls are supported
> by the NAND chip is a matter of reading the chip's ONFI parameter page
> and telling the chip the chosen mode (between all of the supported ones)
> with ->onfi_set_feature().
> 
> Add a check on whether the chip "acked" the timing mode or not.
> 
> This can be a problem for NAND chips that do not follow entirely the
> ONFI specification. These chips actually support other modes than
> "mode 0", but do not update the parameter page once a timing mode has
> been selected. This issue will be addressed in another patch that will
> add the feature to overwrite NAND chips features within the parameter
> page, from the NAND chip driver.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/nand_base.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 96c97588e1ba..ea862be6a803 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -1236,6 +1236,18 @@ static int nand_setup_data_interface(struct nand_chip *chip, int chipnr)
>  				tmode_param);
>  		if (ret)
>  			goto err;
> +
> +		ret = chip->onfi_get_features(mtd, chip,
> +					      ONFI_FEATURE_ADDR_TIMING_MODE,
> +					      tmode_param);
> +		if (ret)
> +			goto err;
> +
> +		if (tmode_param[0] != chip->onfi_timing_mode_default) {
> +			pr_warn("timings mode %d not acknowledged by the NAND chip\n",
> +				chip->onfi_timing_mode_default);
> +			goto err;
> +		}

Hm, I'm not sure this is safe. The spec says that new ONFI timing mode
is applied as soon the CS line is released after a
SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no
guarantee that the CS will be kept low by the controller after
->onfi_set_features() returns we must assume the new mode has been
applied and call ->setup_data_interface() to instruct the controller
to apply new timings.

If you want to check if the mode has really been applied, you should
release the CS (->select_chip(-1)), re-acquire it
(->select_chip(X)), and call
->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears that
the mode has not been applied, you should restore timing mode 0 and
issue a RESET.

>  	}
>  
>  	ret = chip->setup_data_interface(mtd, chipnr, &chip->data_interface);

Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip

[Miquel RAYNAL]

Hello,

> Hm, I'm not sure this is safe. The spec says that new ONFI timing mode
> is applied as soon the CS line is released after a
> SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no
> guarantee that the CS will be kept low by the controller after
> ->onfi_set_features() returns we must assume the new mode has been  
> applied and call ->setup_data_interface() to instruct the controller
> to apply new timings.
> 
> If you want to check if the mode has really been applied, you should
> release the CS (->select_chip(-1)), re-acquire it
> (->select_chip(X)), and call
> ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears
> that the mode has not been applied, you should restore timing mode 0
> and issue a RESET.

Boris, thanks for the comment, I will fix that.

Han, could I have your input on this series? Aside Boris' comment of
course.

Thank you very much,
Miquèl

Re: [PATCH 2/2] mtd: nand: gpmi: Support ->setup_data_interface()

[Boris Brezillon]

On Fri, 22 Dec 2017 18:28:53 +0100
Miquel Raynal <miquel.raynal@free-electrons.com> wrote:

> Until now the GPMI driver had its own timings logic while the core
> already handles that and request the NAND controller drivers to support
> the ->setup_data_interface() hook. Implement that hook by reusing the
> already existing function. No real glue is necessary between core timing
> delays and GPMI registers because the driver already translates the
> ONFI timing modes into register values.
> 
> Make use of the core's tREA, tRLOH and tRHOH values that allow computing
> more precise timings for mode [0-3] and get significantly better values
> (+20% with an i.MX6 Sabre Auto board). Otherwise use the existing logic.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@free-electrons.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 266 ++++++++++-----------------------
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |  33 ++--
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h | 146 +++++++++---------
>  3 files changed, 168 insertions(+), 277 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> index 97787246af41..ae47cd8414bf 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> @@ -151,8 +151,15 @@ static int __gpmi_enable_clk(struct gpmi_nand_data *this, bool v)
>  	return ret;
>  }
>  
> -#define gpmi_enable_clk(x) __gpmi_enable_clk(x, true)
> -#define gpmi_disable_clk(x) __gpmi_enable_clk(x, false)
> +inline int gpmi_enable_clk(struct gpmi_nand_data *this)

Drop the inline here.

> +{
> +	return __gpmi_enable_clk(this, true);
> +}
> +
> +inline int gpmi_disable_clk(struct gpmi_nand_data *this)

Ditto.

> +{
> +	return __gpmi_enable_clk(this, false);
> +}
>  
>  int gpmi_init(struct gpmi_nand_data *this)
>  {
> @@ -326,11 +333,11 @@ static unsigned int ns_to_cycles(unsigned int time,
>  #define DEF_MIN_PROP_DELAY	5
>  #define DEF_MAX_PROP_DELAY	9
>  /* Apply timing to current hardware conditions. */
> -static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
> -					struct gpmi_nfc_hardware_timing *hw)
> +static void
> +gpmi_nfc_compute_hardware_timings(struct gpmi_nand_data *this)
>  {
> +	struct gpmi_nfc_hardware_timing *hw = &this->hw;
>  	struct timing_threshold *nfc = &timing_default_threshold;
> -	struct resources *r = &this->resources;
>  	struct nand_chip *nand = &this->nand;
>  	struct nand_timing target = this->timing;
>  	bool improved_timing_is_available;
> @@ -349,6 +356,9 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
>  	unsigned int min_prop_delay_in_ns = DEF_MIN_PROP_DELAY;
>  	unsigned int max_prop_delay_in_ns = DEF_MAX_PROP_DELAY;
>  
> +	/* Clock rate for non-EDO modes */
> +	hw->clk_rate = 22000000;
> +
>  	/*
>  	 * If there are multiple chips, we need to relax the timings to allow
>  	 * for signal distortion due to higher capacitance.
> @@ -363,14 +373,8 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
>  		target.address_setup_in_ns += 5;
>  	}
>  
> -	/* Check if improved timing information is available. */
> -	improved_timing_is_available =
> -		(target.tREA_in_ns  >= 0) &&
> -		(target.tRLOH_in_ns >= 0) &&
> -		(target.tRHOH_in_ns >= 0);
> -
>  	/* Inspect the clock. */
> -	nfc->clock_frequency_in_hz = clk_get_rate(r->clock[0]);
> +	nfc->clock_frequency_in_hz = hw->clk_rate;
>  	clock_frequency_in_hz = nfc->clock_frequency_in_hz;
>  	clock_period_in_ns    = NSEC_PER_SEC / clock_frequency_in_hz;
>  
> @@ -482,65 +486,6 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
>  	}
>  
>  	/*
> -	 * Check if improved timing information is available. If not, we have to
> -	 * use a less-sophisticated algorithm.
> -	 */
> -	if (!improved_timing_is_available) {
> -		/*
> -		 * Fold the read setup time required by the NFC into the ideal
> -		 * sample delay.
> -		 */
> -		ideal_sample_delay_in_ns = target.gpmi_sample_delay_in_ns +
> -						nfc->internal_data_setup_in_ns;
> -
> -		/*
> -		 * The ideal sample delay may be greater than the maximum
> -		 * allowed by the NFC. If so, we can trade off sample delay time
> -		 * for more data setup time.
> -		 *
> -		 * In each iteration of the following loop, we add a cycle to
> -		 * the data setup time and subtract a corresponding amount from
> -		 * the sample delay until we've satisified the constraints or
> -		 * can't do any better.
> -		 */
> -		while ((ideal_sample_delay_in_ns > max_sample_delay_in_ns) &&
> -			(data_setup_in_cycles < nfc->max_data_setup_cycles)) {
> -
> -			data_setup_in_cycles++;
> -			ideal_sample_delay_in_ns -= clock_period_in_ns;
> -
> -			if (ideal_sample_delay_in_ns < 0)
> -				ideal_sample_delay_in_ns = 0;
> -
> -		}
> -
> -		/*
> -		 * Compute the sample delay factor that corresponds most closely
> -		 * to the ideal sample delay. If the result is too large for the
> -		 * NFC, use the maximum value.
> -		 *
> -		 * Notice that we use the ns_to_cycles function to compute the
> -		 * sample delay factor. We do this because the form of the
> -		 * computation is the same as that for calculating cycles.
> -		 */
> -		sample_delay_factor =
> -			ns_to_cycles(
> -				ideal_sample_delay_in_ns << dll_delay_shift,
> -							clock_period_in_ns, 0);
> -
> -		if (sample_delay_factor > nfc->max_sample_delay_factor)
> -			sample_delay_factor = nfc->max_sample_delay_factor;
> -
> -		/* Skip to the part where we return our results. */
> -		goto return_results;
> -	}
> -
> -	/*
> -	 * If control arrives here, we have more detailed timing information,
> -	 * so we can use a better algorithm.
> -	 */
> -
> -	/*
>  	 * Fold the read setup time required by the NFC into the maximum
>  	 * propagation delay.
>  	 */
> @@ -760,8 +705,6 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
>  			sample_delay_factor = nfc->max_sample_delay_factor;
>  	}
>  
> -	/* Control arrives here when we're ready to return our results. */
> -return_results:
>  	hw->data_setup_in_cycles    = data_setup_in_cycles;
>  	hw->data_hold_in_cycles     = data_hold_in_cycles;
>  	hw->address_setup_in_cycles = address_setup_in_cycles;
> @@ -769,9 +712,6 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
>  	hw->sample_delay_factor     = sample_delay_factor;
>  	hw->device_busy_timeout     = GPMI_DEFAULT_BUSY_TIMEOUT;
>  	hw->wrn_dly_sel             = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS;
> -
> -	/* Return success. */
> -	return 0;
>  }
>  
>  /*
> @@ -857,12 +797,9 @@ static int gpmi_nfc_compute_hardware_timing(struct gpmi_nand_data *this,
>   *       So we only support the mode 4 and mode 5. It is no need to
>   *       support other modes.
>   */
> -static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
> -			struct gpmi_nfc_hardware_timing *hw)
> +static void gpmi_nfc_compute_edo_timings(struct gpmi_nand_data *this)
>  {
> -	struct resources *r = &this->resources;
> -	unsigned long rate = clk_get_rate(r->clock[0]);
> -	int mode = this->timing_mode;
> +	struct gpmi_nfc_hardware_timing *hw = &this->hw;
>  	int dll_threshold = this->devdata->max_chain_delay;
>  	unsigned long delay;
>  	unsigned long clk_period;
> @@ -871,6 +808,9 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
>  	int t_rp;
>  	int rp;
>  
> +	/* Set the main clock to: 100MHz (mode 5) or 80MHz (mode 4) */
> +	hw->clk_rate = (hw->timing_mode == 5) ? 100000000 : 80000000;
> +
>  	/*
>  	 * [1] for GPMI_HW_GPMI_TIMING0:
>  	 *     The async mode requires 40MHz for mode 4, 50MHz for mode 5.
> @@ -880,7 +820,7 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
>  	 */
>  	hw->data_setup_in_cycles = 1;
>  	hw->data_hold_in_cycles = 1;
> -	hw->address_setup_in_cycles = ((mode == 5) ? 1 : 0);
> +	hw->address_setup_in_cycles = (hw->timing_mode == 5) ? 1 : 0;
>  
>  	/* [2] for GPMI_HW_GPMI_TIMING1 */
>  	hw->device_busy_timeout = 0x9000;
> @@ -892,9 +832,9 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
>  	 * Enlarge 10 times for the numerator and denominator in {3}.
>  	 * This make us to get more accurate result.
>  	 */
> -	clk_period = NSEC_PER_SEC / (rate / 10);
> +	clk_period = NSEC_PER_SEC / (hw->clk_rate / 10);
>  	dll_threshold *= 10;
> -	t_rea = ((mode == 5) ? 16 : 20) * 10;
> +	t_rea = (hw->timing_mode == 5 ? 16 : 20) * 10;

Why don't you use the tREA_max value provided in nand_sdr_timings?

>  	c *= 10;
>  
>  	t_rp = clk_period * 1; /* DATA_SETUP is 1 */
> @@ -917,123 +857,36 @@ static void gpmi_compute_edo_timing(struct gpmi_nand_data *this,
>  	hw->sample_delay_factor = delay;
>  }
>  
> -static int enable_edo_mode(struct gpmi_nand_data *this, int mode)
> -{
> -	struct resources  *r = &this->resources;
> -	struct nand_chip *nand = &this->nand;
> -	struct mtd_info	 *mtd = nand_to_mtd(nand);
> -	uint8_t *feature;
> -	unsigned long rate;
> -	int ret;
> -
> -	feature = kzalloc(ONFI_SUBFEATURE_PARAM_LEN, GFP_KERNEL);
> -	if (!feature)
> -		return -ENOMEM;
> -
> -	nand->select_chip(mtd, 0);
> -
> -	/* [1] send SET FEATURE command to NAND */
> -	feature[0] = mode;
> -	ret = nand->onfi_set_features(mtd, nand,
> -				ONFI_FEATURE_ADDR_TIMING_MODE, feature);
> -	if (ret)
> -		goto err_out;
> -
> -	/* [2] send GET FEATURE command to double-check the timing mode */
> -	memset(feature, 0, ONFI_SUBFEATURE_PARAM_LEN);
> -	ret = nand->onfi_get_features(mtd, nand,
> -				ONFI_FEATURE_ADDR_TIMING_MODE, feature);
> -	if (ret || feature[0] != mode)
> -		goto err_out;
> -
> -	nand->select_chip(mtd, -1);
> -
> -	/* [3] set the main IO clock, 100MHz for mode 5, 80MHz for mode 4. */
> -	rate = (mode == 5) ? 100000000 : 80000000;
> -	clk_set_rate(r->clock[0], rate);
> -
> -	/* Let the gpmi_begin() re-compute the timing again. */
> -	this->flags &= ~GPMI_TIMING_INIT_OK;
> -
> -	this->flags |= GPMI_ASYNC_EDO_ENABLED;
> -	this->timing_mode = mode;
> -	kfree(feature);
> -	dev_info(this->dev, "enable the asynchronous EDO mode %d\n", mode);
> -	return 0;
> -
> -err_out:
> -	nand->select_chip(mtd, -1);
> -	kfree(feature);
> -	dev_err(this->dev, "mode:%d ,failed in set feature.\n", mode);
> -	return -EINVAL;
> -}
> -
> -int gpmi_extra_init(struct gpmi_nand_data *this)
> -{
> -	struct nand_chip *chip = &this->nand;
> -
> -	/* Enable the asynchronous EDO feature. */
> -	if (GPMI_IS_MX6(this) && chip->onfi_version) {
> -		int mode = onfi_get_async_timing_mode(chip);
> -
> -		/* We only support the timing mode 4 and mode 5. */
> -		if (mode & ONFI_TIMING_MODE_5)
> -			mode = 5;
> -		else if (mode & ONFI_TIMING_MODE_4)
> -			mode = 4;
> -		else
> -			return 0;
> -
> -		return enable_edo_mode(this, mode);
> -	}
> -	return 0;
> -}
> -
>  /* Begin the I/O */
> -void gpmi_begin(struct gpmi_nand_data *this)
> +void gpmi_nfc_apply_timings(struct gpmi_nand_data *this)
>  {
> +	struct gpmi_nfc_hardware_timing *hw = &this->hw;
>  	struct resources *r = &this->resources;
>  	void __iomem *gpmi_regs = r->gpmi_regs;
>  	unsigned int   clock_period_in_ns;
>  	uint32_t       reg;
>  	unsigned int   dll_wait_time_in_us;
> -	struct gpmi_nfc_hardware_timing  hw;
> -	int ret;
> -
> -	/* Enable the clock. */
> -	ret = gpmi_enable_clk(this);
> -	if (ret) {
> -		dev_err(this->dev, "We failed in enable the clk\n");
> -		goto err_out;
> -	}
>  
> -	/* Only initialize the timing once */
> -	if (this->flags & GPMI_TIMING_INIT_OK)
> -		return;
> -	this->flags |= GPMI_TIMING_INIT_OK;
> -
> -	if (this->flags & GPMI_ASYNC_EDO_ENABLED)
> -		gpmi_compute_edo_timing(this, &hw);
> -	else
> -		gpmi_nfc_compute_hardware_timing(this, &hw);
> +	/* [0] Set the main clock rate */
> +	clk_set_rate(r->clock[0], hw->clk_rate);
>  
>  	/* [1] Set HW_GPMI_TIMING0 */
> -	reg = BF_GPMI_TIMING0_ADDRESS_SETUP(hw.address_setup_in_cycles) |
> -		BF_GPMI_TIMING0_DATA_HOLD(hw.data_hold_in_cycles)         |
> -		BF_GPMI_TIMING0_DATA_SETUP(hw.data_setup_in_cycles);
> +	reg = BF_GPMI_TIMING0_ADDRESS_SETUP(hw->address_setup_in_cycles) |
> +		BF_GPMI_TIMING0_DATA_HOLD(hw->data_hold_in_cycles) |
> +		BF_GPMI_TIMING0_DATA_SETUP(hw->data_setup_in_cycles);
>  
>  	writel(reg, gpmi_regs + HW_GPMI_TIMING0);
>  
>  	/* [2] Set HW_GPMI_TIMING1 */
> -	writel(BF_GPMI_TIMING1_BUSY_TIMEOUT(hw.device_busy_timeout),
> -		gpmi_regs + HW_GPMI_TIMING1);
> +	writel(BF_GPMI_TIMING1_BUSY_TIMEOUT(hw->device_busy_timeout),
> +	       gpmi_regs + HW_GPMI_TIMING1);
>  
>  	/* [3] The following code is to set the HW_GPMI_CTRL1. */
>  
>  	/* Set the WRN_DLY_SEL */
>  	writel(BM_GPMI_CTRL1_WRN_DLY_SEL, gpmi_regs + HW_GPMI_CTRL1_CLR);
> -	writel(BF_GPMI_CTRL1_WRN_DLY_SEL(hw.wrn_dly_sel),
> -					gpmi_regs + HW_GPMI_CTRL1_SET);
> +	writel(BF_GPMI_CTRL1_WRN_DLY_SEL(hw->wrn_dly_sel),
> +	       gpmi_regs + HW_GPMI_CTRL1_SET);
>  
>  	/* DLL_ENABLE must be set to 0 when setting RDN_DELAY or HALF_PERIOD. */
>  	writel(BM_GPMI_CTRL1_DLL_ENABLE, gpmi_regs + HW_GPMI_CTRL1_CLR);
> @@ -1043,12 +896,12 @@ void gpmi_begin(struct gpmi_nand_data *this)
>  	writel(reg, gpmi_regs + HW_GPMI_CTRL1_CLR);
>  
>  	/* If no sample delay is called for, return immediately. */
> -	if (!hw.sample_delay_factor)
> +	if (!hw->sample_delay_factor)
>  		return;
>  
>  	/* Set RDN_DELAY or HALF_PERIOD. */
> -	reg = ((hw.use_half_periods) ? BM_GPMI_CTRL1_HALF_PERIOD : 0)
> -		| BF_GPMI_CTRL1_RDN_DELAY(hw.sample_delay_factor);
> +	reg = ((hw->use_half_periods) ? BM_GPMI_CTRL1_HALF_PERIOD : 0)
> +		| BF_GPMI_CTRL1_RDN_DELAY(hw->sample_delay_factor);
>  
>  	writel(reg, gpmi_regs + HW_GPMI_CTRL1_SET);
>  
> @@ -1060,7 +913,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
>  	 * we can use the GPMI. Calculate the amount of time we need to wait,
>  	 * in microseconds.
>  	 */
> -	clock_period_in_ns = NSEC_PER_SEC / clk_get_rate(r->clock[0]);
> +	clock_period_in_ns = NSEC_PER_SEC / hw->clk_rate;
>  	dll_wait_time_in_us = (clock_period_in_ns * 64) / 1000;
>  
>  	if (!dll_wait_time_in_us)
> @@ -1068,14 +921,45 @@ void gpmi_begin(struct gpmi_nand_data *this)
>  
>  	/* Wait for the DLL to settle. */
>  	udelay(dll_wait_time_in_us);
> -
> -err_out:
> -	return;
>  }
>  
> -void gpmi_end(struct gpmi_nand_data *this)
> +int gpmi_setup_data_interface(struct mtd_info *mtd, int chipnr,
> +			      const struct nand_data_interface *conf)
>  {
> -	gpmi_disable_clk(this);
> +	struct nand_chip *chip = mtd_to_nand(mtd);
> +	struct gpmi_nand_data *this = nand_get_controller_data(chip);
> +	const struct nand_sdr_timings *sdr;
> +
> +	if (chip->onfi_timing_mode_default < 0 ||
> +	    chip->onfi_timing_mode_default > 5)
> +		return -ENOTSUPP;

I'd prefer if you could check conf instead of
->onfi_timing_mode_default. I still hope that one day we'll support
per-chip timings for those chips that are not ONFI compliant, and that
means ->onfi_timing_mode_default will be invalid in this case.

> +
> +	/* Only MX6 GPMI controller can reach EDO timings */
> +	if (chip->onfi_timing_mode_default >= 4 && !GPMI_IS_MX6(this))
> +		return -ENOTSUPP;

Ditto: base your check on tRC/tWC.

> +
> +	if (chipnr < 0)
> +		return 0;
> +
> +	this->hw.timing_mode = chip->onfi_timing_mode_default;

Do you really need to keep a copy of the ONFI timing mode?

> +
> +	sdr = nand_get_sdr_timings(conf);
> +	if (IS_ERR(sdr))
> +		return PTR_ERR(sdr);
> +
> +	this->timing.tREA_in_ns = sdr->tREA_max / 1000;
> +	this->timing.tRLOH_in_ns = sdr->tRLOH_min / 1000;
> +	this->timing.tRHOH_in_ns = sdr->tRHOH_min / 1000;
> +
> +	/* Compute GPMI parameters depending on the mode */
> +	if (this->hw.timing_mode >= 4)
> +		gpmi_nfc_compute_edo_timings(this);
> +	else
> +		gpmi_nfc_compute_hardware_timings(this);
> +
> +	gpmi_nfc_apply_timings(this);

Please add a comment saying that this gpmi_nfc_apply_timings()
call should be moved to ->select_chip() if we want to support multiple
NAND chips with different timings. Actually, if that's not to much
effort, I'd prefer to have this moved in gpmi_select_chip() now.

> +
> +	return 0;
>  }
>  
>  /* Clears a BCH interrupt. */
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index b51db8c85405..04986cd89c99 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -938,11 +938,23 @@ static void gpmi_select_chip(struct mtd_info *mtd, int chipnr)
>  {
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  	struct gpmi_nand_data *this = nand_get_controller_data(chip);
> +	int ret;
>  
> -	if ((this->current_chip < 0) && (chipnr >= 0))
> -		gpmi_begin(this);
> -	else if ((this->current_chip >= 0) && (chipnr < 0))
> -		gpmi_end(this);
> +	/*
> +	 * This driver currently supports only one NAND chip. So once timings
> +	 * have been decided, they will not change. Furthermore, dies share the
> +	 * same configuration, so for power consumption matters, we only
> +	 * disable/enable the clock.
> +	 */
> +	if (this->current_chip < 0 && chipnr >= 0) {
> +		ret = gpmi_enable_clk(this);
> +		if (ret)
> +			dev_err(this->dev, "Failed to enable the clock\n");
> +	} else if (this->current_chip >= 0 && chipnr < 0) {
> +		ret = gpmi_disable_clk(this);
> +		if (ret)
> +			dev_err(this->dev, "Failed to disable the clock\n");
> +	}
>  
>  	this->current_chip = chipnr;
>  }
> @@ -1947,14 +1959,6 @@ static int gpmi_init_last(struct gpmi_nand_data *this)
>  		chip->options |= NAND_SUBPAGE_READ;
>  	}
>  
> -	/*
> -	 * Can we enable the extra features? such as EDO or Sync mode.
> -	 *
> -	 * We do not check the return value now. That's means if we fail in
> -	 * enable the extra features, we still can run in the normal way.
> -	 */
> -	gpmi_extra_init(this);
> -
>  	return 0;
>  }
>  
> @@ -1975,6 +1979,7 @@ static int gpmi_nand_init(struct gpmi_nand_data *this)
>  	nand_set_controller_data(chip, this);
>  	nand_set_flash_node(chip, this->pdev->dev.of_node);
>  	chip->select_chip	= gpmi_select_chip;
> +	chip->setup_data_interface = gpmi_setup_data_interface;
>  	chip->cmd_ctrl		= gpmi_cmd_ctrl;
>  	chip->dev_ready		= gpmi_dev_ready;
>  	chip->read_byte		= gpmi_read_byte;
> @@ -2133,7 +2138,6 @@ static int gpmi_pm_resume(struct device *dev)
>  		return ret;
>  
>  	/* re-init the GPMI registers */
> -	this->flags &= ~GPMI_TIMING_INIT_OK;
>  	ret = gpmi_init(this);
>  	if (ret) {
>  		dev_err(this->dev, "Error setting GPMI : %d\n", ret);
> @@ -2147,9 +2151,6 @@ static int gpmi_pm_resume(struct device *dev)
>  		return ret;
>  	}
>  
> -	/* re-init others */
> -	gpmi_extra_init(this);
> -
>  	return 0;
>  }
>  #endif /* CONFIG_PM_SLEEP */
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> index 06c1f993912c..4890e1378475 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> @@ -135,11 +135,77 @@ struct gpmi_devdata {
>  	const int clks_count;
>  };
>  
> +/**
> + * struct gpmi_nfc_hardware_timing - GPMI hardware timing parameters.
> + * @timing_mode:               The timing mode to comply with.
> + * @clk_rate:                  The clock rate that must be used to derive the
> + *                             following parameters.
> + * @data_setup_in_cycles:      The data setup time, in cycles.
> + * @data_hold_in_cycles:       The data hold time, in cycles.
> + * @address_setup_in_cycles:   The address setup time, in cycles.
> + * @device_busy_timeout:       The timeout waiting for NAND Ready/Busy,
> + *                             this value is the number of cycles multiplied
> + *                             by 4096.
> + * @use_half_periods:          Indicates the clock is running slowly, so the
> + *                             NFC DLL should use half-periods.
> + * @sample_delay_factor:       The sample delay factor.
> + * @wrn_dly_sel:               The delay on the GPMI write strobe.
> + */
> +struct gpmi_nfc_hardware_timing {
> +	unsigned int timing_mode;
> +	unsigned long int clk_rate;
> +
> +	/* for HW_GPMI_TIMING0 */
> +	u8 data_setup_in_cycles;
> +	u8 data_hold_in_cycles;
> +	u8 address_setup_in_cycles;
> +
> +	/* for HW_GPMI_TIMING1 */
> +	u16 device_busy_timeout;
> +#define GPMI_DEFAULT_BUSY_TIMEOUT	0x500 /* default busy timeout value.*/
> +
> +	/* for HW_GPMI_CTRL1 */
> +	bool use_half_periods;
> +	u8 sample_delay_factor;
> +	u8 wrn_dly_sel;

Hm, why not having 3 u32 fields containing the values to program in
TIMING0, TIMING1 and CTRL1?

> +};
> +
> +/**
> + * struct timing_threshold - Timing threshold
> + * @max_data_setup_cycles:       The maximum number of data setup cycles that
> + *                               can be expressed in the hardware.
> + * @internal_data_setup_in_ns:   The time, in ns, that the NFC hardware requires
> + *                               for data read internal setup. In the Reference
> + *                               Manual, see the chapter "High-Speed NAND
> + *                               Timing" for more details.
> + * @max_sample_delay_factor:     The maximum sample delay factor that can be
> + *                               expressed in the hardware.
> + * @max_dll_clock_period_in_ns:  The maximum period of the GPMI clock that the
> + *                               sample delay DLL hardware can possibly work
> + *                               with (the DLL is unusable with longer periods).
> + *                               If the full-cycle period is greater than HALF
> + *                               this value, the DLL must be configured to use
> + *                               half-periods.
> + * @max_dll_delay_in_ns:         The maximum amount of delay, in ns, that the
> + *                               DLL can implement.
> + * @clock_frequency_in_hz:       The clock frequency, in Hz, during the current
> + *                               I/O transaction. If no I/O transaction is in
> + *                               progress, this is the clock frequency during
> + *                               the most recent I/O transaction.
> + */
> +struct timing_threshold {
> +	const unsigned int      max_chip_count;
> +	const unsigned int      max_data_setup_cycles;
> +	const unsigned int      internal_data_setup_in_ns;
> +	const unsigned int      max_sample_delay_factor;
> +	const unsigned int      max_dll_clock_period_in_ns;
> +	const unsigned int      max_dll_delay_in_ns;
> +	unsigned long           clock_frequency_in_hz;
> +
> +};

Not sure why you're moving this definition here.

> +
>  struct gpmi_nand_data {
> -	/* flags */
> -#define GPMI_ASYNC_EDO_ENABLED	(1 << 0)
> -#define GPMI_TIMING_INIT_OK	(1 << 1)
> -	int			flags;
> +	/* Devdata */
>  	const struct gpmi_devdata *devdata;
>  
>  	/* System Interface */
> @@ -152,6 +218,7 @@ struct gpmi_nand_data {
>  	/* Flash Hardware */
>  	struct nand_timing	timing;
>  	int			timing_mode;
> +	struct gpmi_nfc_hardware_timing hw;
>  
>  	/* BCH */
>  	struct bch_geometry	bch_geometry;
> @@ -204,69 +271,6 @@ struct gpmi_nand_data {
>  	void			*private;
>  };
>  
> -/**
> - * struct gpmi_nfc_hardware_timing - GPMI hardware timing parameters.
> - * @data_setup_in_cycles:      The data setup time, in cycles.
> - * @data_hold_in_cycles:       The data hold time, in cycles.
> - * @address_setup_in_cycles:   The address setup time, in cycles.
> - * @device_busy_timeout:       The timeout waiting for NAND Ready/Busy,
> - *                             this value is the number of cycles multiplied
> - *                             by 4096.
> - * @use_half_periods:          Indicates the clock is running slowly, so the
> - *                             NFC DLL should use half-periods.
> - * @sample_delay_factor:       The sample delay factor.
> - * @wrn_dly_sel:               The delay on the GPMI write strobe.
> - */
> -struct gpmi_nfc_hardware_timing {
> -	/* for HW_GPMI_TIMING0 */
> -	uint8_t  data_setup_in_cycles;
> -	uint8_t  data_hold_in_cycles;
> -	uint8_t  address_setup_in_cycles;
> -
> -	/* for HW_GPMI_TIMING1 */
> -	uint16_t device_busy_timeout;
> -#define GPMI_DEFAULT_BUSY_TIMEOUT	0x500 /* default busy timeout value.*/
> -
> -	/* for HW_GPMI_CTRL1 */
> -	bool     use_half_periods;
> -	uint8_t  sample_delay_factor;
> -	uint8_t  wrn_dly_sel;
> -};
> -
> -/**
> - * struct timing_threshold - Timing threshold
> - * @max_data_setup_cycles:       The maximum number of data setup cycles that
> - *                               can be expressed in the hardware.
> - * @internal_data_setup_in_ns:   The time, in ns, that the NFC hardware requires
> - *                               for data read internal setup. In the Reference
> - *                               Manual, see the chapter "High-Speed NAND
> - *                               Timing" for more details.
> - * @max_sample_delay_factor:     The maximum sample delay factor that can be
> - *                               expressed in the hardware.
> - * @max_dll_clock_period_in_ns:  The maximum period of the GPMI clock that the
> - *                               sample delay DLL hardware can possibly work
> - *                               with (the DLL is unusable with longer periods).
> - *                               If the full-cycle period is greater than HALF
> - *                               this value, the DLL must be configured to use
> - *                               half-periods.
> - * @max_dll_delay_in_ns:         The maximum amount of delay, in ns, that the
> - *                               DLL can implement.
> - * @clock_frequency_in_hz:       The clock frequency, in Hz, during the current
> - *                               I/O transaction. If no I/O transaction is in
> - *                               progress, this is the clock frequency during
> - *                               the most recent I/O transaction.
> - */
> -struct timing_threshold {
> -	const unsigned int      max_chip_count;
> -	const unsigned int      max_data_setup_cycles;
> -	const unsigned int      internal_data_setup_in_ns;
> -	const unsigned int      max_sample_delay_factor;
> -	const unsigned int      max_dll_clock_period_in_ns;
> -	const unsigned int      max_dll_delay_in_ns;
> -	unsigned long           clock_frequency_in_hz;
> -
> -};
> -
>  /* Common Services */
>  int common_nfc_set_geometry(struct gpmi_nand_data *);
>  struct dma_chan *get_dma_chan(struct gpmi_nand_data *);
> @@ -279,14 +283,16 @@ int start_dma_with_bch_irq(struct gpmi_nand_data *,
>  
>  /* GPMI-NAND helper function library */
>  int gpmi_init(struct gpmi_nand_data *);
> -int gpmi_extra_init(struct gpmi_nand_data *);
>  void gpmi_clear_bch(struct gpmi_nand_data *);
>  void gpmi_dump_info(struct gpmi_nand_data *);
>  int bch_set_geometry(struct gpmi_nand_data *);
>  int gpmi_is_ready(struct gpmi_nand_data *, unsigned chip);
>  int gpmi_send_command(struct gpmi_nand_data *);
> -void gpmi_begin(struct gpmi_nand_data *);
> -void gpmi_end(struct gpmi_nand_data *);
> +int gpmi_enable_clk(struct gpmi_nand_data *this);
> +int gpmi_disable_clk(struct gpmi_nand_data *this);
> +int gpmi_setup_data_interface(struct mtd_info *mtd, int chipnr,
> +			      const struct nand_data_interface *conf);
> +void gpmi_nfc_apply_timings(struct gpmi_nand_data *this);
>  int gpmi_read_data(struct gpmi_nand_data *);
>  int gpmi_send_data(struct gpmi_nand_data *);
>  int gpmi_send_page(struct gpmi_nand_data *,

Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip

[Boris Brezillon]

On Fri, 5 Jan 2018 16:42:39 +0100
Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:

> Hello,
> 
> > Hm, I'm not sure this is safe. The spec says that new ONFI timing mode
> > is applied as soon the CS line is released after a
> > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no
> > guarantee that the CS will be kept low by the controller after  
> > ->onfi_set_features() returns we must assume the new mode has been    
> > applied and call ->setup_data_interface() to instruct the controller
> > to apply new timings.
> > 
> > If you want to check if the mode has really been applied, you should
> > release the CS (->select_chip(-1)), re-acquire it
> > (->select_chip(X)), and call  
> > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears  
> > that the mode has not been applied, you should restore timing mode 0
> > and issue a RESET.  
> 
> Boris, thanks for the comment, I will fix that.
> 
> Han, could I have your input on this series? Aside Boris' comment of
> course.

Han, we really need your feedback on this series since you were the one
complaining that ONFI mode should be checked back after applying a new
mode. Miquel is reworking the framework to mimic what the GPMI driver,
but we need to be sure that you'll accept to transition to the generic
->setup_data_interface() solution.

Thanks,

Boris

Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip

[Boris Brezillon]

Hi Han,

On Mon, 8 Jan 2018 14:04:29 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Fri, 5 Jan 2018 16:42:39 +0100
> Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:
> 
> > Hello,
> >   
> > > Hm, I'm not sure this is safe. The spec says that new ONFI timing mode
> > > is applied as soon the CS line is released after a
> > > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no
> > > guarantee that the CS will be kept low by the controller after    
> > > ->onfi_set_features() returns we must assume the new mode has been      
> > > applied and call ->setup_data_interface() to instruct the controller
> > > to apply new timings.
> > > 
> > > If you want to check if the mode has really been applied, you should
> > > release the CS (->select_chip(-1)), re-acquire it
> > > (->select_chip(X)), and call    
> > > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears    
> > > that the mode has not been applied, you should restore timing mode 0
> > > and issue a RESET.    
> > 
> > Boris, thanks for the comment, I will fix that.
> > 
> > Han, could I have your input on this series? Aside Boris' comment of
> > course.  
> 
> Han, we really need your feedback on this series since you were the one
> complaining that ONFI mode should be checked back after applying a new
> mode. Miquel is reworking the framework to mimic what the GPMI driver,
> but we need to be sure that you'll accept to transition to the generic
> ->setup_data_interface() solution.

Gentle ping.

Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip

[Han Xu]

________________________________________
From: Boris Brezillon <boris.brezillon@free-electrons.com>
Sent: Monday, January 15, 2018 7:19 AM
To: Han Xu
Cc: Miquel RAYNAL; Richard Weinberger; David Woodhouse; Brian Norris; Marek Vasut; Cyrille Pitchen; linux-mtd@lists.infradead.org
Subject: Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip

Hi Han,

On Mon, 8 Jan 2018 14:04:29 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Fri, 5 Jan 2018 16:42:39 +0100
> Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:
>
> > Hello,
> >
> > > Hm, I'm not sure this is safe. The spec says that new ONFI timing mode
> > > is applied as soon the CS line is released after a
> > > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no
> > > guarantee that the CS will be kept low by the controller after
> > > ->onfi_set_features() returns we must assume the new mode has been
> > > applied and call ->setup_data_interface() to instruct the controller
> > > to apply new timings.
> > >
> > > If you want to check if the mode has really been applied, you should
> > > release the CS (->select_chip(-1)), re-acquire it
> > > (->select_chip(X)), and call
> > > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears
> > > that the mode has not been applied, you should restore timing mode 0
> > > and issue a RESET.
> >
> > Boris, thanks for the comment, I will fix that.
> >
> > Han, could I have your input on this series? Aside Boris' comment of
> > course.
>
> Han, we really need your feedback on this series since you were the one
> complaining that ONFI mode should be checked back after applying a new
> mode. Miquel is reworking the framework to mimic what the GPMI driver,
> but we need to be sure that you'll accept to transition to the generic
> ->setup_data_interface() solution.

Thanks Miquel, I do accept to transit to setup_data_interface solution and actually
I am also working on the similar patches. I reviewed the patch set and don't have
more comments, this one improved more than mine.

Gentle ping.

Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip

[Boris Brezillon]

On Mon, 15 Jan 2018 17:57:08 +0000
Han Xu <han.xu@nxp.com> wrote:

> ________________________________________
> From: Boris Brezillon <boris.brezillon@free-electrons.com>
> Sent: Monday, January 15, 2018 7:19 AM
> To: Han Xu
> Cc: Miquel RAYNAL; Richard Weinberger; David Woodhouse; Brian Norris; Marek Vasut; Cyrille Pitchen; linux-mtd@lists.infradead.org
> Subject: Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip
> 
> Hi Han,
> 
> On Mon, 8 Jan 2018 14:04:29 +0100
> Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> 
> > On Fri, 5 Jan 2018 16:42:39 +0100
> > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:
> >  
> > > Hello,
> > >  
> > > > Hm, I'm not sure this is safe. The spec says that new ONFI timing mode
> > > > is applied as soon the CS line is released after a
> > > > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we have no
> > > > guarantee that the CS will be kept low by the controller after  
> > > > ->onfi_set_features() returns we must assume the new mode has been  
> > > > applied and call ->setup_data_interface() to instruct the controller
> > > > to apply new timings.
> > > >
> > > > If you want to check if the mode has really been applied, you should
> > > > release the CS (->select_chip(-1)), re-acquire it
> > > > (->select_chip(X)), and call  
> > > > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it appears  
> > > > that the mode has not been applied, you should restore timing mode 0
> > > > and issue a RESET.  
> > >
> > > Boris, thanks for the comment, I will fix that.
> > >
> > > Han, could I have your input on this series? Aside Boris' comment of
> > > course.  
> >
> > Han, we really need your feedback on this series since you were the one
> > complaining that ONFI mode should be checked back after applying a new
> > mode. Miquel is reworking the framework to mimic what the GPMI driver,
> > but we need to be sure that you'll accept to transition to the generic  
> > ->setup_data_interface() solution.  
> 
> Thanks Miquel, I do accept to transit to setup_data_interface solution and actually
> I am also working on the similar patches. I reviewed the patch set and don't have
> more comments, this one improved more than mine.

Glad to hear you're not opposed to the idea.

Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been acked by the chip

[Miquel Raynal]

Hi Han,

On Mon, 15 Jan 2018 19:41:08 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Mon, 15 Jan 2018 17:57:08 +0000
> Han Xu <han.xu@nxp.com> wrote:
> 
> > ________________________________________
> > From: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Sent: Monday, January 15, 2018 7:19 AM
> > To: Han Xu
> > Cc: Miquel RAYNAL; Richard Weinberger; David Woodhouse; Brian
> > Norris; Marek Vasut; Cyrille Pitchen; linux-mtd@lists.infradead.org
> > Subject: Re: [PATCH 1/2] mtd: nand: Check ONFI timings have been
> > acked by the chip
> > 
> > Hi Han,
> > 
> > On Mon, 8 Jan 2018 14:04:29 +0100
> > Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
> >   
> > > On Fri, 5 Jan 2018 16:42:39 +0100
> > > Miquel RAYNAL <miquel.raynal@free-electrons.com> wrote:
> > >    
> > > > Hello,
> > > >    
> > > > > Hm, I'm not sure this is safe. The spec says that new ONFI
> > > > > timing mode is applied as soon the CS line is released after a
> > > > > SET_FEATURES(ONFI_FEATURE_ADDR_TIMING_MODE), and since we
> > > > > have no guarantee that the CS will be kept low by the
> > > > > controller after ->onfi_set_features() returns we must assume
> > > > > the new mode has been applied and call
> > > > > ->setup_data_interface() to instruct the controller to apply
> > > > > new timings.
> > > > >
> > > > > If you want to check if the mode has really been applied, you
> > > > > should release the CS (->select_chip(-1)), re-acquire it
> > > > > (->select_chip(X)), and call    
> > > > > ->onfi_get_features(ONFI_FEATURE_ADDR_TIMING_MODE). If it
> > > > > appears that the mode has not been applied, you should
> > > > > restore timing mode 0 and issue a RESET.    
> > > >
> > > > Boris, thanks for the comment, I will fix that.
> > > >
> > > > Han, could I have your input on this series? Aside Boris'
> > > > comment of course.    
> > >
> > > Han, we really need your feedback on this series since you were
> > > the one complaining that ONFI mode should be checked back after
> > > applying a new mode. Miquel is reworking the framework to mimic
> > > what the GPMI driver, but we need to be sure that you'll accept
> > > to transition to the generic ->setup_data_interface()
> > > solution.    
> > 
> > Thanks Miquel, I do accept to transit to setup_data_interface
> > solution and actually I am also working on the similar patches. I
> > reviewed the patch set and don't have more comments, this one
> > improved more than mine.  
> 
> Glad to hear you're not opposed to the idea.

I'm happy to read this, I'll send a v2 tomorrow that addresses Boris'
comments.

Thanks,
Miquèl