[RFC PATCH 0/4] Fix and improve gpmi nand on mx28

[RFC PATCH 0/4] Fix and improve gpmi nand on mx28

[Dario Binacchi]

Starting from [1], the series fixes the timings setting of the gpmi
controller for the mx28 architecture, also adding support for fast
edo mode timings. The whole series has been heavily tested with the
mtd kernel test modules, and with repeated write cycles on nand.

The patches of the series were applied after applying patches [2]
and [3] which at the moment are already in nand-next.

[1] https://lore.kernel.org/r/20210702065350.209646-5-ebiggers@kernel.org
[2] 73b68455f8ac ("mtd: rawnand: gpmi: Remove explicit default gpmi clock setting for i.MX6")
[3] 7944f8346983 ("mtd: rawnand: gpmi: Add ERR007117 protection for nfc_apply_timings")


Dario Binacchi (3):
  mtd: rawnand: gpmi: support fast edo timings for mx28
  mtd: rawnand: gpmi: fix controller timings setting
  mtd: rawnand: gpmi: validate controller clock rate

Michael Trimarchi (1):
  clk: mxs: imx28: Reparent gpmi clk to ref_gpmi

 drivers/clk/mxs/clk-imx28.c                |  3 +
 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 75 +++++++++++++++++-----
 2 files changed, 61 insertions(+), 17 deletions(-)

-- 
2.32.0

[RFC PATCH 1/4] clk: mxs: imx28: Reparent gpmi clk to ref_gpmi

[Dario Binacchi]

From: Michael Trimarchi <michael@amarulasolutions.com>

ref_gpmi is connected that is sourced from pll0. This allow
to get nand clk frequency to handle edo mode 5,4,3

Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---

 drivers/clk/mxs/clk-imx28.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
index 62146ea4d5b8..9e0b9f8e5885 100644
--- a/drivers/clk/mxs/clk-imx28.c
+++ b/drivers/clk/mxs/clk-imx28.c
@@ -243,6 +243,9 @@ static void __init mx28_clocks_init(struct device_node *np)
 
 	clk_register_clkdev(clks[enet_out], NULL, "enet_out");
 
+	/* GPMI set parent to ref_gpmi instead of osc */
+	clk_set_parent(clks[gpmi_sel], clks[ref_gpmi]);
+
 	for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
 		clk_prepare_enable(clks[clks_init_on[i]]);
 }
-- 
2.32.0

[RFC PATCH 2/4] mtd: rawnand: gpmi: support fast edo timings for mx28

[Dario Binacchi]

In the mx28 reference manual there are examples of how to set up the
GPMI controller to support fast NAND EDO timing.

This patch has been tested on a 2048/64 byte NAND (Micron MT29F2G08ABAEAH4).
Kernel mtd tests:
 - mtd_nandbiterrs
 - mtd_nandecctest
 - mtd_oobtest
 - mtd_pagetest
 - mtd_readtest
 - mtd_speedtest
 - mtd_stresstest
 - mtd_subpagetest
 - mtd_torturetest [cycles_count = 10000000]
run without errors.

Before this patch (mode 0):
---------------------------
eraseblock write speed is 2098 KiB/s
eraseblock read speed is 2680 KiB/s
page write speed is 1689 KiB/s
page read speed is 2522 KiB/s
2 page write speed is 1899 KiB/s
2 page read speed is 2579 KiB/s
erase speed is 128000 KiB/s
2x multi-block erase speed is 73142 KiB/s
4x multi-block erase speed is 204800 KiB/s
8x multi-block erase speed is 256000 KiB/s
16x multi-block erase speed is 256000 KiB/s
32x multi-block erase speed is 256000 KiB/s
64x multi-block erase speed is 256000 KiB/s

After this patch (mode 5):
-------------------------
eraseblock write speed is 3390 KiB/s
eraseblock read speed is 5688 KiB/s
page write speed is 2680 KiB/s
page read speed is 4876 KiB/s
2 page write speed is 2909 KiB/s
2 page read speed is 5224 KiB/s
erase speed is 170666 KiB/s
2x multi-block erase speed is 204800 KiB/s
4x multi-block erase speed is 256000 KiB/s
8x multi-block erase speed is 256000 KiB/s
16x multi-block erase speed is 256000 KiB/s
32x multi-block erase speed is 256000 KiB/s
64x multi-block erase speed is 256000 KiB/s

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
---

 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 65bcd1c548d2..fd935e893daf 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -772,8 +772,8 @@ static int gpmi_setup_interface(struct nand_chip *chip, int chipnr,
 	if (IS_ERR(sdr))
 		return PTR_ERR(sdr);
 
-	/* Only MX6 GPMI controller can reach EDO timings */
-	if (sdr->tRC_min <= 25000 && !GPMI_IS_MX6(this))
+	/* Only MX28/MX6 GPMI controller can reach EDO timings */
+	if (sdr->tRC_min <= 25000 && !GPMI_IS_MX28(this) && !GPMI_IS_MX6(this))
 		return -ENOTSUPP;
 
 	/* Stop here if this call was just a check */
-- 
2.32.0

[RFC PATCH 3/4] mtd: rawnand: gpmi: fix controller timings setting

[Dario Binacchi]

The controller registers are now set accordling to the real clock rate.

Fixes: b1206122069a ("mtd: rawnand: gpmi: use core timings instead of an empirical derivation")
Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
---

 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index fd935e893daf..0517b81bb24c 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -648,6 +648,7 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
 				     const struct nand_sdr_timings *sdr)
 {
 	struct gpmi_nfc_hardware_timing *hw = &this->hw;
+	struct resources *r = &this->resources;
 	unsigned int dll_threshold_ps = this->devdata->max_chain_delay;
 	unsigned int period_ps, reference_period_ps;
 	unsigned int data_setup_cycles, data_hold_cycles, addr_setup_cycles;
@@ -671,6 +672,8 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
 		wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
 	}
 
+	hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
+
 	/* SDR core timings are given in picoseconds */
 	period_ps = div_u64((u64)NSEC_PER_SEC * 1000, hw->clk_rate);
 
-- 
2.32.0

[RFC PATCH 4/4] mtd: rawnand: gpmi: validate controller clock rate

[Dario Binacchi]

What to do when the real rate of the gpmi clock is not equal to the
required one? The solutions proposed in [1] did not lead to a conclusion
on how to validate the clock rate, so, inspired by the document [2], I
consider the rate correct only if not greater than the rate of the
previous edo. In fact, in chapter 4.16.2 (NV-DDR) of the document [2],
it is written that "If the host selects timing mode n, then its clock
period shall be faster than the clock period of timing mode n-1 and
slower than or equal to the clock period of timing mode n.". I thought
that it could therefore also be used in this case, without therefore
having to define the valid rate ranges empirically.

[1] https://lore.kernel.org/r/20210702065350.209646-5-ebiggers@kernel.org
[2] http://www.onfi.org/-/media/client/onfi/specs/onfi_3_0_gold.pdf?la=en

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>

---

 drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 70 +++++++++++++++++-----
 1 file changed, 54 insertions(+), 16 deletions(-)

diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 0517b81bb24c..3d37cd49abd5 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -570,6 +570,27 @@ static int bch_set_geometry(struct gpmi_nand_data *this)
 	return ret;
 }
 
+struct edo_mode {
+	u32 tRC_min;
+	long clk_rate;
+	u8 wrn_dly_sel;
+};
+
+static const struct edo_mode edo_modes[] = {
+	{.tRC_min = 30000, .clk_rate = 22000000,
+	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
+	{.tRC_min = 30000, .clk_rate = 22000000,
+	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
+	{.tRC_min = 30000, .clk_rate = 22000000,
+	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
+	{.tRC_min = 30000, .clk_rate = 22000000,
+	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
+	{.tRC_min = 25000, .clk_rate = 80000000,
+	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY},
+	{.tRC_min = 20000, .clk_rate = 100000000,
+	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY},
+};
+
 /*
  * <1> Firstly, we should know what's the GPMI-clock means.
  *     The GPMI-clock is the internal clock in the gpmi nand controller.
@@ -644,8 +665,8 @@ static int bch_set_geometry(struct gpmi_nand_data *this)
  *         RDN_DELAY = -----------------------     {3}
  *                           RP
  */
-static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
-				     const struct nand_sdr_timings *sdr)
+static int gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
+				    const struct nand_sdr_timings *sdr)
 {
 	struct gpmi_nfc_hardware_timing *hw = &this->hw;
 	struct resources *r = &this->resources;
@@ -657,22 +678,35 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
 	int sample_delay_ps, sample_delay_factor;
 	u16 busy_timeout_cycles;
 	u8 wrn_dly_sel;
+	long clk_rate;
+	int i, emode = -1;
 
-	if (sdr->tRC_min >= 30000) {
-		/* ONFI non-EDO modes [0-3] */
-		hw->clk_rate = 22000000;
-		wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS;
-	} else if (sdr->tRC_min >= 25000) {
-		/* ONFI EDO mode 4 */
-		hw->clk_rate = 80000000;
-		wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
-	} else {
-		/* ONFI EDO mode 5 */
-		hw->clk_rate = 100000000;
-		wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
+	/* Search the required EDO mode */
+	for (i = 0; i < ARRAY_SIZE(edo_modes); i++) {
+		if (sdr->tRC_min >= edo_modes[i].tRC_min) {
+			emode = i;
+			break;
+		}
+	}
+
+	if (emode < 0) {
+		dev_err(this->dev, "tRC_min %d not supported\n", sdr->tRC_min);
+		return -ENOTSUPP;
+	}
+
+	clk_rate = clk_round_rate(r->clock[0], edo_modes[emode].clk_rate);
+	if (emode > 0 && !(clk_rate <= edo_modes[emode].clk_rate &&
+			   clk_rate > edo_modes[emode - 1].clk_rate)) {
+		dev_err(this->dev,
+			"edo mode %d clock setting: expected %ld, got %ld\n",
+			emode, edo_modes[emode].clk_rate, clk_rate);
+		return -ENOTSUPP;
 	}
 
-	hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
+	dev_dbg(this->dev, "edo mode %d @ %ld Hz\n", emode, clk_rate);
+
+	hw->clk_rate = clk_rate;
+	wrn_dly_sel = edo_modes[emode].wrn_dly_sel;
 
 	/* SDR core timings are given in picoseconds */
 	period_ps = div_u64((u64)NSEC_PER_SEC * 1000, hw->clk_rate);
@@ -714,6 +748,7 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
 		hw->ctrl1n |= BF_GPMI_CTRL1_RDN_DELAY(sample_delay_factor) |
 			      BM_GPMI_CTRL1_DLL_ENABLE |
 			      (use_half_period ? BM_GPMI_CTRL1_HALF_PERIOD : 0);
+	return 0;
 }
 
 static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this)
@@ -769,6 +804,7 @@ static int gpmi_setup_interface(struct nand_chip *chip, int chipnr,
 {
 	struct gpmi_nand_data *this = nand_get_controller_data(chip);
 	const struct nand_sdr_timings *sdr;
+	int ret;
 
 	/* Retrieve required NAND timings */
 	sdr = nand_get_sdr_timings(conf);
@@ -784,7 +820,9 @@ static int gpmi_setup_interface(struct nand_chip *chip, int chipnr,
 		return 0;
 
 	/* Do the actual derivation of the controller timings */
-	gpmi_nfc_compute_timings(this, sdr);
+	ret = gpmi_nfc_compute_timings(this, sdr);
+	if (ret)
+		return ret;
 
 	this->hw.must_apply_timings = true;
 
-- 
2.32.0

Re: [RFC PATCH 2/4] mtd: rawnand: gpmi: support fast edo timings for mx28

[Miquel Raynal]

Hi Dario,

dario.binacchi@amarulasolutions.com wrote on Fri, 17 Dec 2021 16:55:10
+0100:

> In the mx28 reference manual there are examples of how to set up the
> GPMI controller to support fast NAND EDO timing.

... which are? I am not sure to understand what you mean here.

And what you change below is just "not refusing EDO timings with on
MX28". There is a mismatch between the the two.

Plus, it seems that this patch should come last given the fact that
you're fixing the EDO calculations.

> This patch has been tested on a 2048/64 byte NAND (Micron MT29F2G08ABAEAH4).
> Kernel mtd tests:
>  - mtd_nandbiterrs
>  - mtd_nandecctest
>  - mtd_oobtest
>  - mtd_pagetest
>  - mtd_readtest
>  - mtd_speedtest
>  - mtd_stresstest
>  - mtd_subpagetest
>  - mtd_torturetest [cycles_count = 10000000]
> run without errors.
> 
> Before this patch (mode 0):
> ---------------------------
> eraseblock write speed is 2098 KiB/s
> eraseblock read speed is 2680 KiB/s
> page write speed is 1689 KiB/s
> page read speed is 2522 KiB/s
> 2 page write speed is 1899 KiB/s
> 2 page read speed is 2579 KiB/s
> erase speed is 128000 KiB/s
> 2x multi-block erase speed is 73142 KiB/s
> 4x multi-block erase speed is 204800 KiB/s
> 8x multi-block erase speed is 256000 KiB/s
> 16x multi-block erase speed is 256000 KiB/s
> 32x multi-block erase speed is 256000 KiB/s
> 64x multi-block erase speed is 256000 KiB/s
> 
> After this patch (mode 5):
> -------------------------
> eraseblock write speed is 3390 KiB/s
> eraseblock read speed is 5688 KiB/s
> page write speed is 2680 KiB/s
> page read speed is 4876 KiB/s
> 2 page write speed is 2909 KiB/s
> 2 page read speed is 5224 KiB/s
> erase speed is 170666 KiB/s
> 2x multi-block erase speed is 204800 KiB/s
> 4x multi-block erase speed is 256000 KiB/s
> 8x multi-block erase speed is 256000 KiB/s
> 16x multi-block erase speed is 256000 KiB/s
> 32x multi-block erase speed is 256000 KiB/s
> 64x multi-block erase speed is 256000 KiB/s
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
> 
>  drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index 65bcd1c548d2..fd935e893daf 100644
> --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> @@ -772,8 +772,8 @@ static int gpmi_setup_interface(struct nand_chip *chip, int chipnr,
>  	if (IS_ERR(sdr))
>  		return PTR_ERR(sdr);
>  
> -	/* Only MX6 GPMI controller can reach EDO timings */
> -	if (sdr->tRC_min <= 25000 && !GPMI_IS_MX6(this))
> +	/* Only MX28/MX6 GPMI controller can reach EDO timings */
> +	if (sdr->tRC_min <= 25000 && !GPMI_IS_MX28(this) && !GPMI_IS_MX6(this))
>  		return -ENOTSUPP;
>  
>  	/* Stop here if this call was just a check */


Thanks,
Miquèl

Re: [RFC PATCH 3/4] mtd: rawnand: gpmi: fix controller timings setting

[Miquel Raynal]

Hi Dario,

dario.binacchi@amarulasolutions.com wrote on Fri, 17 Dec 2021 16:55:11
+0100:

> The controller registers are now set accordling to the real clock rate.

You should use another tense (which I forgot the name) such as:

Set the controller registers according to the real clock rate.

But most importantly, you should explain why and perhaps give examples
of frequencies on your setup.

> Fixes: b1206122069a ("mtd: rawnand: gpmi: use core timings instead of an empirical derivation")
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>
> ---
> 
>  drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index fd935e893daf..0517b81bb24c 100644
> --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> @@ -648,6 +648,7 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
>  				     const struct nand_sdr_timings *sdr)
>  {
>  	struct gpmi_nfc_hardware_timing *hw = &this->hw;
> +	struct resources *r = &this->resources;
>  	unsigned int dll_threshold_ps = this->devdata->max_chain_delay;
>  	unsigned int period_ps, reference_period_ps;
>  	unsigned int data_setup_cycles, data_hold_cycles, addr_setup_cycles;
> @@ -671,6 +672,8 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
>  		wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
>  	}
>  
> +	hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
> +
>  	/* SDR core timings are given in picoseconds */
>  	period_ps = div_u64((u64)NSEC_PER_SEC * 1000, hw->clk_rate);
>  


Thanks,
Miquèl

Re: [RFC PATCH 4/4] mtd: rawnand: gpmi: validate controller clock rate

[Miquel Raynal]

Hi Dario,

dario.binacchi@amarulasolutions.com wrote on Fri, 17 Dec 2021 16:55:12
+0100:

> What to do when the real rate of the gpmi clock is not equal to the
> required one? The solutions proposed in [1] did not lead to a conclusion
> on how to validate the clock rate, so, inspired by the document [2], I
> consider the rate correct only if not greater than the rate of the
> previous edo.

Not greater? what are you talking about here, if it's a rate, are you
sure "not greater" is what you mean?

> In fact, in chapter 4.16.2 (NV-DDR) of the document [2],
> it is written that "If the host selects timing mode n, then its clock
> period shall be faster than the clock period of timing mode n-1 and

faster? is that the real wording in the document? seems inaccurate when
referring to a clock period.

> slower than or equal to the clock period of timing mode n.". I thought
> that it could therefore also be used in this case, without therefore
> having to define the valid rate ranges empirically.

Can you give empirical values in your case so that we understand better
the problem that you are trying to solve and how you solve it?

Also, I don't know if the NV-DDR logic applies to SDR EDO modes, but if
it works and if Han acknowledges it, it's fine for me.

> [1] https://lore.kernel.org/r/20210702065350.209646-5-ebiggers@kernel.org
> [2] http://www.onfi.org/-/media/client/onfi/specs/onfi_3_0_gold.pdf?la=en
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> Co-developed-by: Michael Trimarchi <michael@amarulasolutions.com>

You need Michael's Signed-off-by.

> 
> ---
> 
>  drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 70 +++++++++++++++++-----
>  1 file changed, 54 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index 0517b81bb24c..3d37cd49abd5 100644
> --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> @@ -570,6 +570,27 @@ static int bch_set_geometry(struct gpmi_nand_data *this)
>  	return ret;
>  }
>  
> +struct edo_mode {
> +	u32 tRC_min;
> +	long clk_rate;
> +	u8 wrn_dly_sel;
> +};
> +
> +static const struct edo_mode edo_modes[] = {
> +	{.tRC_min = 30000, .clk_rate = 22000000,

Do you really need to provide a tRC_min here? It is already part of the
nand_timings structure.

> +	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
> +	{.tRC_min = 30000, .clk_rate = 22000000,
> +	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
> +	{.tRC_min = 30000, .clk_rate = 22000000,
> +	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
> +	{.tRC_min = 30000, .clk_rate = 22000000,

Not sure to get the difference between these three first modes.

> +	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS},
> +	{.tRC_min = 25000, .clk_rate = 80000000,
> +	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY},
> +	{.tRC_min = 20000, .clk_rate = 100000000,
> +	 .wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY},

I am also tempted to say that I don't really understand what this is
all about, maybe an explanation would be good in a comment.
> +};
> +
>  /*
>   * <1> Firstly, we should know what's the GPMI-clock means.
>   *     The GPMI-clock is the internal clock in the gpmi nand controller.
> @@ -644,8 +665,8 @@ static int bch_set_geometry(struct gpmi_nand_data *this)
>   *         RDN_DELAY = -----------------------     {3}
>   *                           RP
>   */
> -static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
> -				     const struct nand_sdr_timings *sdr)
> +static int gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
> +				    const struct nand_sdr_timings *sdr)
>  {
>  	struct gpmi_nfc_hardware_timing *hw = &this->hw;
>  	struct resources *r = &this->resources;
> @@ -657,22 +678,35 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
>  	int sample_delay_ps, sample_delay_factor;
>  	u16 busy_timeout_cycles;
>  	u8 wrn_dly_sel;
> +	long clk_rate;
> +	int i, emode = -1;
>  
> -	if (sdr->tRC_min >= 30000) {
> -		/* ONFI non-EDO modes [0-3] */
> -		hw->clk_rate = 22000000;
> -		wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_4_TO_8NS;
> -	} else if (sdr->tRC_min >= 25000) {
> -		/* ONFI EDO mode 4 */
> -		hw->clk_rate = 80000000;
> -		wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> -	} else {
> -		/* ONFI EDO mode 5 */
> -		hw->clk_rate = 100000000;
> -		wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;

I would rather prefer a preparation patch which changes nothing in the
behavior, but prepares the following change where you actually do
something different so that we don't mi the wrn_dly_sel change with the
clock rate approximation.

Also, please consider using the ONFI modes now provided in the timings
structure if it helps.

> +	/* Search the required EDO mode */
> +	for (i = 0; i < ARRAY_SIZE(edo_modes); i++) {
> +		if (sdr->tRC_min >= edo_modes[i].tRC_min) {
> +			emode = i;
> +			break;
> +		}
> +	}
> +
> +	if (emode < 0) {
> +		dev_err(this->dev, "tRC_min %d not supported\n", sdr->tRC_min);
> +		return -ENOTSUPP;
> +	}
> +
> +	clk_rate = clk_round_rate(r->clock[0], edo_modes[emode].clk_rate);
> +	if (emode > 0 && !(clk_rate <= edo_modes[emode].clk_rate &&
> +			   clk_rate > edo_modes[emode - 1].clk_rate)) {
> +		dev_err(this->dev,
> +			"edo mode %d clock setting: expected %ld, got %ld\n",
> +			emode, edo_modes[emode].clk_rate, clk_rate);
> +		return -ENOTSUPP;
>  	}
>  
> -	hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
> +	dev_dbg(this->dev, "edo mode %d @ %ld Hz\n", emode, clk_rate);
> +
> +	hw->clk_rate = clk_rate;
> +	wrn_dly_sel = edo_modes[emode].wrn_dly_sel;
>  
>  	/* SDR core timings are given in picoseconds */
>  	period_ps = div_u64((u64)NSEC_PER_SEC * 1000, hw->clk_rate);
> @@ -714,6 +748,7 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
>  		hw->ctrl1n |= BF_GPMI_CTRL1_RDN_DELAY(sample_delay_factor) |
>  			      BM_GPMI_CTRL1_DLL_ENABLE |
>  			      (use_half_period ? BM_GPMI_CTRL1_HALF_PERIOD : 0);

Space

> +	return 0;
>  }
>  
>  static int gpmi_nfc_apply_timings(struct gpmi_nand_data *this)
> @@ -769,6 +804,7 @@ static int gpmi_setup_interface(struct nand_chip *chip, int chipnr,
>  {
>  	struct gpmi_nand_data *this = nand_get_controller_data(chip);
>  	const struct nand_sdr_timings *sdr;
> +	int ret;
>  
>  	/* Retrieve required NAND timings */
>  	sdr = nand_get_sdr_timings(conf);
> @@ -784,7 +820,9 @@ static int gpmi_setup_interface(struct nand_chip *chip, int chipnr,
>  		return 0;
>  
>  	/* Do the actual derivation of the controller timings */
> -	gpmi_nfc_compute_timings(this, sdr);
> +	ret = gpmi_nfc_compute_timings(this, sdr);
> +	if (ret)
> +		return ret;
>  
>  	this->hw.must_apply_timings = true;
>  


Thanks,
Miquèl

Re: [RFC PATCH 1/4] clk: mxs: imx28: Reparent gpmi clk to ref_gpmi

[Stephen Boyd]

Quoting Dario Binacchi (2021-12-17 07:55:09)
> From: Michael Trimarchi <michael@amarulasolutions.com>
> 
> ref_gpmi is connected that is sourced from pll0. This allow
> to get nand clk frequency to handle edo mode 5,4,3
> 
> Signed-off-by: Michael Trimarchi <michael@amarulasolutions.com>
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
> 
>  drivers/clk/mxs/clk-imx28.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/clk/mxs/clk-imx28.c b/drivers/clk/mxs/clk-imx28.c
> index 62146ea4d5b8..9e0b9f8e5885 100644
> --- a/drivers/clk/mxs/clk-imx28.c
> +++ b/drivers/clk/mxs/clk-imx28.c
> @@ -243,6 +243,9 @@ static void __init mx28_clocks_init(struct device_node *np)
>  
>         clk_register_clkdev(clks[enet_out], NULL, "enet_out");
>  
> +       /* GPMI set parent to ref_gpmi instead of osc */
> +       clk_set_parent(clks[gpmi_sel], clks[ref_gpmi]);

Please check the return value and print a warning or something if it
fails. Also, can it be done through assigned-clock-parents instead?

> +
>         for (i = 0; i < ARRAY_SIZE(clks_init_on); i++)
>                 clk_prepare_enable(clks[clks_init_on[i]]);
>  }