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.
[1] https://lore.kernel.org/r/[email protected]
Changes in v2:
- Reparent by device tree instead of code (drivers/clk/mxs/clk-imx28.c).
Suggested by Stephen Boyd.
- Improve the commit description.
- give examples of frequencies on my setup.
- Add the patch to the series.
- Fix commit description.
- Add an example to the commit description to better understand the
problem solved by the patch.
- Split the patch.
- Improve the commit message.
- Move the patch to the end of the series.
Dario Binacchi (5):
ARM: dts: imx28: reparent gpmi clock to ref_gpmi
mtd: rawnand: gpmi: fix controller timings setting
mtd: rawnand: gpmi: use a table to get EDO mode setup
mtd: rawnand: gpmi: validate controller clock rate
mtd: rawnand: gpmi: support fast edo timings for mx28
arch/arm/boot/dts/imx28.dtsi | 2 +
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 70 ++++++++++++++++------
2 files changed, 55 insertions(+), 17 deletions(-)
--
2.32.0
Since ref_gpmi is sourced from pll0 (480MHz), It allows the GPMI
controller to manage High-Speed NAND Timing (edo mode 3,4 and 5).
Signed-off-by: Michael Trimarchi <[email protected]>
Signed-off-by: Dario Binacchi <[email protected]>
---
Changes in v2:
- Reparent by device tree instead of code (drivers/clk/mxs/clk-imx28.c).
Suggested by Stephen Boyd.
arch/arm/boot/dts/imx28.dtsi | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index 84d0176d5193..130b4145af82 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -110,6 +110,8 @@ gpmi: nand-controller@8000c000 {
interrupt-names = "bch";
clocks = <&clks 50>;
clock-names = "gpmi_io";
+ assigned-clocks = <&clks 13>;
+ assigned-clock-parents = <&clks 10>;
dmas = <&dma_apbh 4>;
dma-names = "rx-tx";
status = "disabled";
--
2.32.0
Set the controller registers according to the real clock rate. The
controller registers configuration (setup, hold, timeout, ... cycles)
depends on the clock rate of the GPMI. Using the real rate instead of
the ideal one, avoids that this inaccuracy (required_rate - real_rate)
affects the registers setting.
This patch has been tested on two custom boards with i.MX28 and i.MX6
SOCs:
- i.MX28:
required rate 100MHz, real rate 99.3MHz
- i.MX6
required rate 100MHz, real rate 99MHz
Fixes: b1206122069a ("mtd: rawnand: gpmi: use core timings instead of an empirical derivation")
Co-developed-by: Michael Trimarchi <[email protected]>
Signed-off-by: Michael Trimarchi <[email protected]>
Signed-off-by: Dario Binacchi <[email protected]>
---
Changes in v2:
- Improve the commit description.
- give examples of frequencies on my setup.
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 1b64c5a5140d..73c3bf59b55e 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
This is a preparation patch for the upcoming validation of the
GPMI controller clock rate.
Co-developed-by: Michael Trimarchi <[email protected]>
Signed-off-by: Michael Trimarchi <[email protected]>
Signed-off-by: Dario Binacchi <[email protected]>
---
Changes in v2:
- Add the patch to the series.
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 43 +++++++++++++++-------
1 file changed, 30 insertions(+), 13 deletions(-)
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 73c3bf59b55e..4ac695aa5131 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.
@@ -657,22 +678,18 @@ 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;
+ int i, emode = ARRAY_SIZE(edo_modes) - 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;
+ }
}
- hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
+ hw->clk_rate = clk_round_rate(r->clock[0], edo_modes[emode].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);
--
2.32.0
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 lower or equal to the rate of the
previous edo mode. 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.
For example, suppose that gpmi_nfc_compute_timings() is called for edo
mode 5 configuration (100MHz, from the `edo_modes' table) but the rate
returned by clk_round_rate() is 80MHz (edo mode 4 from the `edo_modes'
table). In this case gpmi_nfc_compute_timings() will return error, and
will be called again for edo mode 4 configuration, which this time will
be successful.
[1] https://lore.kernel.org/r/[email protected]
[2] http://www.onfi.org/-/media/client/onfi/specs/onfi_3_0_gold.pdf?la=en
Co-developed-by: Michael Trimarchi <[email protected]>
Signed-off-by: Michael Trimarchi <[email protected]>
Signed-off-by: Dario Binacchi <[email protected]>
---
Changes in v2:
- Fix commit description.
- Add an example to the commit description to better understand the
problem solved by the patch.
- Split the patch.
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 24 ++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 4ac695aa5131..7ae7a37775a3 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -665,8 +665,8 @@ static const struct edo_mode edo_modes[] = {
* 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;
@@ -679,6 +679,7 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
u16 busy_timeout_cycles;
u8 wrn_dly_sel;
int i, emode = ARRAY_SIZE(edo_modes) - 1;
+ long clk_rate;
/* Search the required EDO mode */
for (i = 0; i < ARRAY_SIZE(edo_modes); i++) {
@@ -688,7 +689,18 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
}
}
- hw->clk_rate = clk_round_rate(r->clock[0], edo_modes[emode].clk_rate);
+ 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;
+ }
+
+ 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 */
@@ -731,6 +743,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)
@@ -786,6 +799,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);
@@ -801,7 +815,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
In the i.MX28 manual (MCIMX28RM, Rev. 1, 2010) you can find an example
(15.2.4 High-Speed NAND Timing) of how to configure the GPMI controller
to manage High-Speed NAND devices, so it was wrong to assume that only
i.MX6 can achieve EDO timings.
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
Co-developed-by: Michael Trimarchi <[email protected]>
Signed-off-by: Michael Trimarchi <[email protected]>
Signed-off-by: Dario Binacchi <[email protected]>
---
Changes in v2:
- Improve the commit message.
- Move the patch to the end of the series.
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 7ae7a37775a3..7b9191a70ed1 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -806,8 +806,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
Hi Dario,
On Mon, Jan 17, 2022 at 12:18:27PM +0100, Dario Binacchi wrote:
> +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.
> @@ -657,22 +678,18 @@ 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;
> + int i, emode = ARRAY_SIZE(edo_modes) - 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;
> + }
The first four entries of edo_modes[] all have the same value, so this loop
will never end on the second, third or fourth element. These elements are just
there to match 'emode' with the existing ONFI mode numbers, but then 'emode' is
never used as an ONFI mode number, instead it's only used as an index to the
array. You could equally well remove the second till fourth array entries.
Then with only three entries left in the array I wonder if you're not better
off with the original code and change it to something like:
if (sdr->tRC_min >= 30000) {
/* ONFI non-EDO modes [0-3] */
hw->clk_rate = 22000000;
min_rate = 0;
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;
min_rate = 22000000;
wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
} else {
/* ONFI EDO mode 5 */
hw->clk_rate = 100000000;
min_rate = 80000000;
wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
}
hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
if (hw->clk_rate < min_rate)
return -EINVAL;
I think this would be easier to follow.
Sascha
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Hi Sascha,
On Mon, Jan 17, 2022 at 2:18 PM Sascha Hauer <[email protected]> wrote:
>
> Hi Dario,
>
> On Mon, Jan 17, 2022 at 12:18:27PM +0100, Dario Binacchi wrote:
> > +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.
> > @@ -657,22 +678,18 @@ 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;
> > + int i, emode = ARRAY_SIZE(edo_modes) - 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;
> > + }
>
> The first four entries of edo_modes[] all have the same value, so this loop
> will never end on the second, third or fourth element. These elements are just
> there to match 'emode' with the existing ONFI mode numbers, but then 'emode' is
> never used as an ONFI mode number, instead it's only used as an index to the
> array. You could equally well remove the second till fourth array entries.
>
> Then with only three entries left in the array I wonder if you're not better
> off with the original code and change it to something like:
This implementation is justified by the next patch in the series ([RFC
PATCH v2 4/5] mtd: rawnand: gpmi: validate controller clock rate).
I thought that clock rate validation could be better implemented by
indexing a table. The replication of the second, third and fourth
elements
makes the index also the edo mode (used in the debug printout). Using
a less redundant table (three elements) requires the edo mode
to be stored in it if you want to keep the debug printing or remove
the debug message.
>
> if (sdr->tRC_min >= 30000) {
> /* ONFI non-EDO modes [0-3] */
> hw->clk_rate = 22000000;
> min_rate = 0;
> 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;
> min_rate = 22000000;
> wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> } else {
> /* ONFI EDO mode 5 */
> hw->clk_rate = 100000000;
> min_rate = 80000000;
> wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> }
>
> hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
> if (hw->clk_rate < min_rate)
> return -EINVAL;
>
> I think this would be easier to follow.
I think the key point is to decide if the patch "[RFC PATCH v2 4/5]
mtd: rawnand: gpmi: validate controller clock rate" makes sense.
I thought of this patch to facilitate that implementation, since it
compares the real GPMI clock rate to that of the previous edo mode.
I thought it wasn't elegant to implement another switch case.
Thanks and regards,
Dario
>
> Sascha
>
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
Dario Binacchi
Embedded Linux Developer
[email protected]
__________________________________
Amarula Solutions SRL
Via Le Canevare 30, 31100 Treviso, Veneto, IT
T. +39 042 243 5310
[email protected]
http://www.amarulasolutions.com
On Mon, Jan 17, 2022 at 03:08:30PM +0100, Dario Binacchi wrote:
> Hi Sascha,
>
> On Mon, Jan 17, 2022 at 2:18 PM Sascha Hauer <[email protected]> wrote:
> >
> > Hi Dario,
> >
> > On Mon, Jan 17, 2022 at 12:18:27PM +0100, Dario Binacchi wrote:
> > > +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.
> > > @@ -657,22 +678,18 @@ 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;
> > > + int i, emode = ARRAY_SIZE(edo_modes) - 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;
> > > + }
> >
> > The first four entries of edo_modes[] all have the same value, so this loop
> > will never end on the second, third or fourth element. These eleqments are just
> > there to match 'emode' with the existing ONFI mode numbers, but then 'emode' is
> > never used as an ONFI mode number, instead it's only used as an index to the
> > array. You could equally well remove the second till fourth array entries.
> >
> > Then with only three entries left in the array I wonder if you're not better
> > off with the original code and change it to something like:
>
> This implementation is justified by the next patch in the series ([RFC
> PATCH v2 4/5] mtd: rawnand: gpmi: validate controller clock rate).
> I thought that clock rate validation could be better implemented by
> indexing a table. The replication of the second, third and fourth
> elements
> makes the index also the edo mode (used in the debug printout). Using
> a less redundant table (three elements) requires the edo mode
> to be stored in it if you want to keep the debug printing or remove
> the debug message.
>
> >
> > if (sdr->tRC_min >= 30000) {
> > /* ONFI non-EDO modes [0-3] */
> > hw->clk_rate = 22000000;
> > min_rate = 0;
> > 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;
> > min_rate = 22000000;
> > wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> > } else {
> > /* ONFI EDO mode 5 */
> > hw->clk_rate = 100000000;
> > min_rate = 80000000;
> > wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
> > }
> >
> > hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
> > if (hw->clk_rate < min_rate)
> > return -EINVAL;
> >
> > I think this would be easier to follow.
>
> I think the key point is to decide if the patch "[RFC PATCH v2 4/5]
> mtd: rawnand: gpmi: validate controller clock rate" makes sense.
> I thought of this patch to facilitate that implementation, since it
> compares the real GPMI clock rate to that of the previous edo mode.
> I thought it wasn't elegant to implement another switch case.
You don't have to implement another switch/case. It's already there.
To be more clear my suggestion was to replace 3/5 and 4/5 with the
following.
Sascha
---------------------------8<--------------------------------
From 879fb5daaf19524ae29eda84c75d08668ad06d42 Mon Sep 17 00:00:00 2001
From: Dario Binacchi <[email protected]>
Date: Mon, 17 Jan 2022 12:18:28 +0100
Subject: [PATCH] mtd: rawnand: gpmi: validate controller clock rate
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 lower or equal to the rate of the
previous edo mode. 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.
For example, suppose that gpmi_nfc_compute_timings() is called for edo
mode 5 configuration (100MHz, from the `edo_modes' table) but the rate
returned by clk_round_rate() is 80MHz (edo mode 4 from the `edo_modes'
table). In this case gpmi_nfc_compute_timings() will return error, and
will be called again for edo mode 4 configuration, which this time will
be successful.
[1] https://lore.kernel.org/r/[email protected]
[2] http://www.onfi.org/-/media/client/onfi/specs/onfi_3_0_gold.pdf?la=en
Co-developed-by: Michael Trimarchi <[email protected]>
Signed-off-by: Michael Trimarchi <[email protected]>
Signed-off-by: Dario Binacchi <[email protected]>
Signed-off-by: Sascha Hauer <[email protected]>
---
drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 16 +++++++++++++---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
index 73c3bf59b55e..dcad7410d817 100644
--- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
@@ -644,8 +644,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;
@@ -653,6 +653,7 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
unsigned int period_ps, reference_period_ps;
unsigned int data_setup_cycles, data_hold_cycles, addr_setup_cycles;
unsigned int tRP_ps;
+ unsigned int min_rate;
bool use_half_period;
int sample_delay_ps, sample_delay_factor;
u16 busy_timeout_cycles;
@@ -661,18 +662,23 @@ static void gpmi_nfc_compute_timings(struct gpmi_nand_data *this,
if (sdr->tRC_min >= 30000) {
/* ONFI non-EDO modes [0-3] */
hw->clk_rate = 22000000;
+ min_rate = 0;
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;
+ min_rate = 22000000;
wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
} else {
/* ONFI EDO mode 5 */
hw->clk_rate = 100000000;
+ min_rate = 80000000;
wrn_dly_sel = BV_GPMI_CTRL1_WRN_DLY_SEL_NO_DELAY;
}
hw->clk_rate = clk_round_rate(r->clock[0], hw->clk_rate);
+ if (hw->clk_rate < min_rate)
+ return -ENOTSUPP;
/* SDR core timings are given in picoseconds */
period_ps = div_u64((u64)NSEC_PER_SEC * 1000, hw->clk_rate);
@@ -714,6 +720,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 +776,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 +792,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.30.2
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |