[PATCH v3 0/3] mtd: rawnand: denali: add new clocks and improve setup_data_interface

[PATCH v3 0/3] mtd: rawnand: denali: add new clocks and improve setup_data_interface

[Masahiro Yamada]

The ->setup_data_interface() hook needs to know the clock frequency.
In fact, this IP needs three clocks, bus "which clock?" is really
confusing.  (It is not described in the DT-binding at all.)

This series adds more clocks.  In the new binding, three clocks
are required: core clock, bus interface clock, ECC engine clock.

This series also takes care of the backward compatibility by
providing hardcoded values in case the new clocks are missing.
So, existing DT should work.


Changes in v3:
  - Change the patch order so that the bug-fix one comes the first

Changes in v2:
  - Split patches into sensible chunks
  - Split patches into sensible chunks

Masahiro Yamada (3):
  mtd: rawnand: denali_dt: add more clocks based on IP datasheet
  mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
  mtd: rawnand: denali: optimize timing parameters for data interface

 .../devicetree/bindings/mtd/denali-nand.txt        |  5 ++
 drivers/mtd/nand/raw/denali.c                      | 49 ++++++++--------
 drivers/mtd/nand/raw/denali.h                      |  1 +
 drivers/mtd/nand/raw/denali_dt.c                   | 66 ++++++++++++++++++----
 drivers/mtd/nand/raw/denali_pci.c                  |  1 +
 5 files changed, 86 insertions(+), 36 deletions(-)

-- 
2.7.4

[PATCH v3 0/3] mtd: rawnand: denali: add new clocks and improve setup_data_interface

[Masahiro Yamada]

The ->setup_data_interface() hook needs to know the clock frequency.
In fact, this IP needs three clocks, bus "which clock?" is really
confusing.  (It is not described in the DT-binding at all.)

This series adds more clocks.  In the new binding, three clocks
are required: core clock, bus interface clock, ECC engine clock.

This series also takes care of the backward compatibility by
providing hardcoded values in case the new clocks are missing.
So, existing DT should work.


Changes in v3:
  - Change the patch order so that the bug-fix one comes the first

Changes in v2:
  - Split patches into sensible chunks
  - Split patches into sensible chunks

Masahiro Yamada (3):
  mtd: rawnand: denali_dt: add more clocks based on IP datasheet
  mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
  mtd: rawnand: denali: optimize timing parameters for data interface

 .../devicetree/bindings/mtd/denali-nand.txt        |  5 ++
 drivers/mtd/nand/raw/denali.c                      | 49 ++++++++--------
 drivers/mtd/nand/raw/denali.h                      |  1 +
 drivers/mtd/nand/raw/denali_dt.c                   | 66 ++++++++++++++++++----
 drivers/mtd/nand/raw/denali_pci.c                  |  1 +
 5 files changed, 86 insertions(+), 36 deletions(-)

-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Masahiro Yamada]

According to the Denali User's Guide, this IP needs three clocks:

 - clk: controller core clock

 - clk_x: bus interface clock

 - ecc_clk: clock at which ECC circuitry is run

Currently, denali_dt.c requires a single anonymous clock and its
frequency.  However, the driver needs to get the frequency of "clk_x"
not "clk".  This is confusing because people tend to assume the
anonymous clock means the core clock.  In fact, I got a report of
SOCFPGA breakage because the timing parameters are calculated based
on a wrong frequency.

Instead of the cheesy implementation, the clocks in the real hardware
should be represented in the driver and the DT-binding.

However, adding new clocks would break the existing platforms.  For the
backward compatibility, the driver still accepts a single clock just as
before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
This is fine for existing DT of Socionext UniPhier, and also fixes the
issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
the bus interface clock.

Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
Cc: linux-stable <stable@vger.kernel.org> #4.14+
Reported-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v3:
  - Change the patch order so that the bug-fix one comes the first

Changes in v2:
  - Split patches into sensible chunks

 .../devicetree/bindings/mtd/denali-nand.txt        |  5 +++
 drivers/mtd/nand/raw/denali_dt.c                   | 49 ++++++++++++++++++++--
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 0ee8edb..f33da87 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -8,6 +8,9 @@ Required properties:
   - reg : should contain registers location and length for data and reg.
   - reg-names: Should contain the reg names "nand_data" and "denali_reg"
   - interrupts : The interrupt number.
+  - clocks: should contain phandle of the controller core clock, the bus
+    interface clock, and the ECC circuit clock.
+  - clock-names: should contain "nand", "nand_x", "ecc"
 
 Optional properties:
   - nand-ecc-step-size: see nand.txt for details.  If present, the value must be
@@ -31,5 +34,7 @@ nand: nand@ff900000 {
 	compatible = "altr,socfpga-denali-nand";
 	reg = <0xff900000 0x20>, <0xffb80000 0x1000>;
 	reg-names = "nand_data", "denali_reg";
+	clocks = <&nand_clk>, <&nand_x_clk>, <&nand_ecc_clk>;
+	clock-names = "nand", "nand_x", "ecc";
 	interrupts = <0 144 4>;
 };
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index cfd33e6..ce6239d 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -27,7 +27,9 @@
 
 struct denali_dt {
 	struct denali_nand_info	denali;
-	struct clk		*clk;
+	struct clk *clk;	/* core clock */
+	struct clk *clk_x;	/* bus interface clock */
+	struct clk *clk_ecc;	/* ECC circuit clock */
 };
 
 struct denali_dt_data {
@@ -114,24 +116,61 @@ static int denali_dt_probe(struct platform_device *pdev)
 	if (IS_ERR(denali->host))
 		return PTR_ERR(denali->host);
 
-	dt->clk = devm_clk_get(&pdev->dev, NULL);
+	/*
+	 * A single anonymous clock is supported for the backward compatibility.
+	 * New platforms should support all the named clocks.
+	 */
+	dt->clk = devm_clk_get(&pdev->dev, "nand");
+	if (IS_ERR(dt->clk))
+		dt->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dt->clk)) {
 		dev_err(&pdev->dev, "no clk available\n");
 		return PTR_ERR(dt->clk);
 	}
+
+	dt->clk_x = devm_clk_get(&pdev->dev, "nand_x");
+	if (IS_ERR(dt->clk_x))
+		dt->clk_x = NULL;
+
+	dt->clk_ecc = devm_clk_get(&pdev->dev, "ecc");
+	if (IS_ERR(dt->clk_ecc))
+		dt->clk_ecc = NULL;
+
 	ret = clk_prepare_enable(dt->clk);
 	if (ret)
 		return ret;
 
-	denali->clk_x_rate = clk_get_rate(dt->clk);
+	ret = clk_prepare_enable(dt->clk_x);
+	if (ret)
+		goto out_disable_clk;
+
+	ret = clk_prepare_enable(dt->clk_ecc);
+	if (ret)
+		goto out_disable_clk_x;
+
+	if (dt->clk_x) {
+		denali->clk_x_rate = clk_get_rate(dt->clk_x);
+	} else {
+		/*
+		 * Hardcode the clock rates for the backward compatibility.
+		 * This works for both SOCFPGA and UniPhier.
+		 */
+		dev_notice(&pdev->dev,
+			   "necessary clock is missing. default clock rates are used.\n");
+		denali->clk_x_rate = 200000000;
+	}
 
 	ret = denali_init(denali);
 	if (ret)
-		goto out_disable_clk;
+		goto out_disable_clk_ecc;
 
 	platform_set_drvdata(pdev, dt);
 	return 0;
 
+out_disable_clk_ecc:
+	clk_disable_unprepare(dt->clk_ecc);
+out_disable_clk_x:
+	clk_disable_unprepare(dt->clk_x);
 out_disable_clk:
 	clk_disable_unprepare(dt->clk);
 
@@ -143,6 +182,8 @@ static int denali_dt_remove(struct platform_device *pdev)
 	struct denali_dt *dt = platform_get_drvdata(pdev);
 
 	denali_remove(&dt->denali);
+	clk_disable_unprepare(dt->clk_ecc);
+	clk_disable_unprepare(dt->clk_x);
 	clk_disable_unprepare(dt->clk);
 
 	return 0;
-- 
2.7.4

[PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Masahiro Yamada]

According to the Denali User's Guide, this IP needs three clocks:

 - clk: controller core clock

 - clk_x: bus interface clock

 - ecc_clk: clock at which ECC circuitry is run

Currently, denali_dt.c requires a single anonymous clock and its
frequency.  However, the driver needs to get the frequency of "clk_x"
not "clk".  This is confusing because people tend to assume the
anonymous clock means the core clock.  In fact, I got a report of
SOCFPGA breakage because the timing parameters are calculated based
on a wrong frequency.

Instead of the cheesy implementation, the clocks in the real hardware
should be represented in the driver and the DT-binding.

However, adding new clocks would break the existing platforms.  For the
backward compatibility, the driver still accepts a single clock just as
before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
This is fine for existing DT of Socionext UniPhier, and also fixes the
issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
the bus interface clock.

Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
Cc: linux-stable <stable@vger.kernel.org> #4.14+
Reported-by: Richard Weinberger <richard@nod.at>
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v3:
  - Change the patch order so that the bug-fix one comes the first

Changes in v2:
  - Split patches into sensible chunks

 .../devicetree/bindings/mtd/denali-nand.txt        |  5 +++
 drivers/mtd/nand/raw/denali_dt.c                   | 49 ++++++++++++++++++++--
 2 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index 0ee8edb..f33da87 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -8,6 +8,9 @@ Required properties:
   - reg : should contain registers location and length for data and reg.
   - reg-names: Should contain the reg names "nand_data" and "denali_reg"
   - interrupts : The interrupt number.
+  - clocks: should contain phandle of the controller core clock, the bus
+    interface clock, and the ECC circuit clock.
+  - clock-names: should contain "nand", "nand_x", "ecc"
 
 Optional properties:
   - nand-ecc-step-size: see nand.txt for details.  If present, the value must be
@@ -31,5 +34,7 @@ nand: nand@ff900000 {
 	compatible = "altr,socfpga-denali-nand";
 	reg = <0xff900000 0x20>, <0xffb80000 0x1000>;
 	reg-names = "nand_data", "denali_reg";
+	clocks = <&nand_clk>, <&nand_x_clk>, <&nand_ecc_clk>;
+	clock-names = "nand", "nand_x", "ecc";
 	interrupts = <0 144 4>;
 };
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index cfd33e6..ce6239d 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -27,7 +27,9 @@
 
 struct denali_dt {
 	struct denali_nand_info	denali;
-	struct clk		*clk;
+	struct clk *clk;	/* core clock */
+	struct clk *clk_x;	/* bus interface clock */
+	struct clk *clk_ecc;	/* ECC circuit clock */
 };
 
 struct denali_dt_data {
@@ -114,24 +116,61 @@ static int denali_dt_probe(struct platform_device *pdev)
 	if (IS_ERR(denali->host))
 		return PTR_ERR(denali->host);
 
-	dt->clk = devm_clk_get(&pdev->dev, NULL);
+	/*
+	 * A single anonymous clock is supported for the backward compatibility.
+	 * New platforms should support all the named clocks.
+	 */
+	dt->clk = devm_clk_get(&pdev->dev, "nand");
+	if (IS_ERR(dt->clk))
+		dt->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(dt->clk)) {
 		dev_err(&pdev->dev, "no clk available\n");
 		return PTR_ERR(dt->clk);
 	}
+
+	dt->clk_x = devm_clk_get(&pdev->dev, "nand_x");
+	if (IS_ERR(dt->clk_x))
+		dt->clk_x = NULL;
+
+	dt->clk_ecc = devm_clk_get(&pdev->dev, "ecc");
+	if (IS_ERR(dt->clk_ecc))
+		dt->clk_ecc = NULL;
+
 	ret = clk_prepare_enable(dt->clk);
 	if (ret)
 		return ret;
 
-	denali->clk_x_rate = clk_get_rate(dt->clk);
+	ret = clk_prepare_enable(dt->clk_x);
+	if (ret)
+		goto out_disable_clk;
+
+	ret = clk_prepare_enable(dt->clk_ecc);
+	if (ret)
+		goto out_disable_clk_x;
+
+	if (dt->clk_x) {
+		denali->clk_x_rate = clk_get_rate(dt->clk_x);
+	} else {
+		/*
+		 * Hardcode the clock rates for the backward compatibility.
+		 * This works for both SOCFPGA and UniPhier.
+		 */
+		dev_notice(&pdev->dev,
+			   "necessary clock is missing. default clock rates are used.\n");
+		denali->clk_x_rate = 200000000;
+	}
 
 	ret = denali_init(denali);
 	if (ret)
-		goto out_disable_clk;
+		goto out_disable_clk_ecc;
 
 	platform_set_drvdata(pdev, dt);
 	return 0;
 
+out_disable_clk_ecc:
+	clk_disable_unprepare(dt->clk_ecc);
+out_disable_clk_x:
+	clk_disable_unprepare(dt->clk_x);
 out_disable_clk:
 	clk_disable_unprepare(dt->clk);
 
@@ -143,6 +182,8 @@ static int denali_dt_remove(struct platform_device *pdev)
 	struct denali_dt *dt = platform_get_drvdata(pdev);
 
 	denali_remove(&dt->denali);
+	clk_disable_unprepare(dt->clk_ecc);
+	clk_disable_unprepare(dt->clk_x);
 	clk_disable_unprepare(dt->clk);
 
 	return 0;
-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v3 2/3] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev

[Masahiro Yamada]

The probe function references &pdev->dev many times.  Add 'dev' as
a shorthand.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v3: None
Changes in v2: None

 drivers/mtd/nand/raw/denali_dt.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index ce6239d..afaae37 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -81,38 +81,39 @@ MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
 
 static int denali_dt_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct denali_dt *dt;
 	const struct denali_dt_data *data;
 	struct denali_nand_info *denali;
 	int ret;
 
-	dt = devm_kzalloc(&pdev->dev, sizeof(*dt), GFP_KERNEL);
+	dt = devm_kzalloc(dev, sizeof(*dt), GFP_KERNEL);
 	if (!dt)
 		return -ENOMEM;
 	denali = &dt->denali;
 
-	data = of_device_get_match_data(&pdev->dev);
+	data = of_device_get_match_data(dev);
 	if (data) {
 		denali->revision = data->revision;
 		denali->caps = data->caps;
 		denali->ecc_caps = data->ecc_caps;
 	}
 
-	denali->dev = &pdev->dev;
+	denali->dev = dev;
 	denali->irq = platform_get_irq(pdev, 0);
 	if (denali->irq < 0) {
-		dev_err(&pdev->dev, "no irq defined\n");
+		dev_err(dev, "no irq defined\n");
 		return denali->irq;
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "denali_reg");
-	denali->reg = devm_ioremap_resource(&pdev->dev, res);
+	denali->reg = devm_ioremap_resource(dev, res);
 	if (IS_ERR(denali->reg))
 		return PTR_ERR(denali->reg);
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand_data");
-	denali->host = devm_ioremap_resource(&pdev->dev, res);
+	denali->host = devm_ioremap_resource(dev, res);
 	if (IS_ERR(denali->host))
 		return PTR_ERR(denali->host);
 
@@ -120,19 +121,19 @@ static int denali_dt_probe(struct platform_device *pdev)
 	 * A single anonymous clock is supported for the backward compatibility.
 	 * New platforms should support all the named clocks.
 	 */
-	dt->clk = devm_clk_get(&pdev->dev, "nand");
+	dt->clk = devm_clk_get(dev, "nand");
 	if (IS_ERR(dt->clk))
-		dt->clk = devm_clk_get(&pdev->dev, NULL);
+		dt->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(dt->clk)) {
-		dev_err(&pdev->dev, "no clk available\n");
+		dev_err(dev, "no clk available\n");
 		return PTR_ERR(dt->clk);
 	}
 
-	dt->clk_x = devm_clk_get(&pdev->dev, "nand_x");
+	dt->clk_x = devm_clk_get(dev, "nand_x");
 	if (IS_ERR(dt->clk_x))
 		dt->clk_x = NULL;
 
-	dt->clk_ecc = devm_clk_get(&pdev->dev, "ecc");
+	dt->clk_ecc = devm_clk_get(dev, "ecc");
 	if (IS_ERR(dt->clk_ecc))
 		dt->clk_ecc = NULL;
 
@@ -155,7 +156,7 @@ static int denali_dt_probe(struct platform_device *pdev)
 		 * Hardcode the clock rates for the backward compatibility.
 		 * This works for both SOCFPGA and UniPhier.
 		 */
-		dev_notice(&pdev->dev,
+		dev_notice(dev,
 			   "necessary clock is missing. default clock rates are used.\n");
 		denali->clk_x_rate = 200000000;
 	}
-- 
2.7.4

[PATCH v3 3/3] mtd: rawnand: denali: optimize timing parameters for data interface

[Masahiro Yamada]

This commit improves the ->setup_data_interface() hook.

The denali_setup_data_interface() needs the frequency of clk_x
and the ratio of clk_x / clk.

The latter is currently hardcoded in the driver, like this:

  #define DENALI_CLK_X_MULT       6

The IP datasheet requires that clk_x / clk be 4, 5, or 6.  I just
chose 6 because it is the most defensive value, but it is not optimal.
By getting the clock rate of both "clk" and "clk_x", the driver can
compute the timing values more precisely.

To not break the existing platforms, the fallback value, 50 MHz is
provided.  It is true for all upstreamed platforms.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v3: None
Changes in v2:
  - Split patches into sensible chunks

 drivers/mtd/nand/raw/denali.c     | 49 +++++++++++++++++++--------------------
 drivers/mtd/nand/raw/denali.h     |  1 +
 drivers/mtd/nand/raw/denali_dt.c  |  2 ++
 drivers/mtd/nand/raw/denali_pci.c |  1 +
 4 files changed, 28 insertions(+), 25 deletions(-)

diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
index 2a302a1..2de46d4 100644
--- a/drivers/mtd/nand/raw/denali.c
+++ b/drivers/mtd/nand/raw/denali.c
@@ -51,14 +51,6 @@ MODULE_LICENSE("GPL");
 #define DENALI_INVALID_BANK	-1
 #define DENALI_NR_BANKS		4
 
-/*
- * The bus interface clock, clk_x, is phase aligned with the core clock.  The
- * clk_x is an integral multiple N of the core clk.  The value N is configured
- * at IP delivery time, and its available value is 4, 5, or 6.  We need to align
- * to the largest value to make it work with any possible configuration.
- */
-#define DENALI_CLK_X_MULT	6
-
 static inline struct denali_nand_info *mtd_to_denali(struct mtd_info *mtd)
 {
 	return container_of(mtd_to_nand(mtd), struct denali_nand_info, nand);
@@ -954,7 +946,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 {
 	struct denali_nand_info *denali = mtd_to_denali(mtd);
 	const struct nand_sdr_timings *timings;
-	unsigned long t_clk;
+	unsigned long t_x, mult_x;
 	int acc_clks, re_2_we, re_2_re, we_2_re, addr_2_data;
 	int rdwr_en_lo, rdwr_en_hi, rdwr_en_lo_hi, cs_setup;
 	int addr_2_data_mask;
@@ -965,15 +957,24 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 		return PTR_ERR(timings);
 
 	/* clk_x period in picoseconds */
-	t_clk = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);
-	if (!t_clk)
+	t_x = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);
+	if (!t_x)
+		return -EINVAL;
+
+	/*
+	 * The bus interface clock, clk_x, is phase aligned with the core clock.
+	 * The clk_x is an integral multiple N of the core clk.  The value N is
+	 * configured at IP delivery time, and its available value is 4, 5, 6.
+	 */
+	mult_x = DIV_ROUND_CLOSEST_ULL(denali->clk_x_rate, denali->clk_rate);
+	if (mult_x < 4 || mult_x > 6)
 		return -EINVAL;
 
 	if (chipnr == NAND_DATA_IFACE_CHECK_ONLY)
 		return 0;
 
 	/* tREA -> ACC_CLKS */
-	acc_clks = DIV_ROUND_UP(timings->tREA_max, t_clk);
+	acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x);
 	acc_clks = min_t(int, acc_clks, ACC_CLKS__VALUE);
 
 	tmp = ioread32(denali->reg + ACC_CLKS);
@@ -982,7 +983,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 	iowrite32(tmp, denali->reg + ACC_CLKS);
 
 	/* tRWH -> RE_2_WE */
-	re_2_we = DIV_ROUND_UP(timings->tRHW_min, t_clk);
+	re_2_we = DIV_ROUND_UP(timings->tRHW_min, t_x);
 	re_2_we = min_t(int, re_2_we, RE_2_WE__VALUE);
 
 	tmp = ioread32(denali->reg + RE_2_WE);
@@ -991,7 +992,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 	iowrite32(tmp, denali->reg + RE_2_WE);
 
 	/* tRHZ -> RE_2_RE */
-	re_2_re = DIV_ROUND_UP(timings->tRHZ_max, t_clk);
+	re_2_re = DIV_ROUND_UP(timings->tRHZ_max, t_x);
 	re_2_re = min_t(int, re_2_re, RE_2_RE__VALUE);
 
 	tmp = ioread32(denali->reg + RE_2_RE);
@@ -1005,8 +1006,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 	 * With WE_2_RE properly set, the Denali controller automatically takes
 	 * care of the delay; the driver need not set NAND_WAIT_TCCS.
 	 */
-	we_2_re = DIV_ROUND_UP(max(timings->tCCS_min, timings->tWHR_min),
-			       t_clk);
+	we_2_re = DIV_ROUND_UP(max(timings->tCCS_min, timings->tWHR_min), t_x);
 	we_2_re = min_t(int, we_2_re, TWHR2_AND_WE_2_RE__WE_2_RE);
 
 	tmp = ioread32(denali->reg + TWHR2_AND_WE_2_RE);
@@ -1021,7 +1021,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 	if (denali->revision < 0x0501)
 		addr_2_data_mask >>= 1;
 
-	addr_2_data = DIV_ROUND_UP(timings->tADL_min, t_clk);
+	addr_2_data = DIV_ROUND_UP(timings->tADL_min, t_x);
 	addr_2_data = min_t(int, addr_2_data, addr_2_data_mask);
 
 	tmp = ioread32(denali->reg + TCWAW_AND_ADDR_2_DATA);
@@ -1031,7 +1031,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 
 	/* tREH, tWH -> RDWR_EN_HI_CNT */
 	rdwr_en_hi = DIV_ROUND_UP(max(timings->tREH_min, timings->tWH_min),
-				  t_clk);
+				  t_x);
 	rdwr_en_hi = min_t(int, rdwr_en_hi, RDWR_EN_HI_CNT__VALUE);
 
 	tmp = ioread32(denali->reg + RDWR_EN_HI_CNT);
@@ -1040,11 +1040,10 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 	iowrite32(tmp, denali->reg + RDWR_EN_HI_CNT);
 
 	/* tRP, tWP -> RDWR_EN_LO_CNT */
-	rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min),
-				  t_clk);
+	rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);
 	rdwr_en_lo_hi = DIV_ROUND_UP(max(timings->tRC_min, timings->tWC_min),
-				     t_clk);
-	rdwr_en_lo_hi = max(rdwr_en_lo_hi, DENALI_CLK_X_MULT);
+				     t_x);
+	rdwr_en_lo_hi = max_t(int, rdwr_en_lo_hi, mult_x);
 	rdwr_en_lo = max(rdwr_en_lo, rdwr_en_lo_hi - rdwr_en_hi);
 	rdwr_en_lo = min_t(int, rdwr_en_lo, RDWR_EN_LO_CNT__VALUE);
 
@@ -1054,8 +1053,8 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
 	iowrite32(tmp, denali->reg + RDWR_EN_LO_CNT);
 
 	/* tCS, tCEA -> CS_SETUP_CNT */
-	cs_setup = max3((int)DIV_ROUND_UP(timings->tCS_min, t_clk) - rdwr_en_lo,
-			(int)DIV_ROUND_UP(timings->tCEA_max, t_clk) - acc_clks,
+	cs_setup = max3((int)DIV_ROUND_UP(timings->tCS_min, t_x) - rdwr_en_lo,
+			(int)DIV_ROUND_UP(timings->tCEA_max, t_x) - acc_clks,
 			0);
 	cs_setup = min_t(int, cs_setup, CS_SETUP_CNT__VALUE);
 
@@ -1282,7 +1281,7 @@ int denali_init(struct denali_nand_info *denali)
 	}
 
 	/* clk rate info is needed for setup_data_interface */
-	if (denali->clk_x_rate)
+	if (denali->clk_rate && denali->clk_x_rate)
 		chip->setup_data_interface = denali_setup_data_interface;
 
 	ret = nand_scan_ident(mtd, denali->max_banks, NULL);
diff --git a/drivers/mtd/nand/raw/denali.h b/drivers/mtd/nand/raw/denali.h
index 9ad33d2..1f8feaf 100644
--- a/drivers/mtd/nand/raw/denali.h
+++ b/drivers/mtd/nand/raw/denali.h
@@ -300,6 +300,7 @@
 
 struct denali_nand_info {
 	struct nand_chip nand;
+	unsigned long clk_rate;		/* core clock rate */
 	unsigned long clk_x_rate;	/* bus interface clock rate */
 	int active_bank;		/* currently selected bank */
 	struct device *dev;
diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index afaae37..0faaad0 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -150,6 +150,7 @@ static int denali_dt_probe(struct platform_device *pdev)
 		goto out_disable_clk_x;
 
 	if (dt->clk_x) {
+		denali->clk_rate = clk_get_rate(dt->clk);
 		denali->clk_x_rate = clk_get_rate(dt->clk_x);
 	} else {
 		/*
@@ -158,6 +159,7 @@ static int denali_dt_probe(struct platform_device *pdev)
 		 */
 		dev_notice(dev,
 			   "necessary clock is missing. default clock rates are used.\n");
+		denali->clk_rate = 50000000;
 		denali->clk_x_rate = 200000000;
 	}
 
diff --git a/drivers/mtd/nand/raw/denali_pci.c b/drivers/mtd/nand/raw/denali_pci.c
index 49cb3e1..7c8efc4 100644
--- a/drivers/mtd/nand/raw/denali_pci.c
+++ b/drivers/mtd/nand/raw/denali_pci.c
@@ -73,6 +73,7 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	denali->irq = dev->irq;
 	denali->ecc_caps = &denali_pci_ecc_caps;
 	denali->nand.ecc.options |= NAND_ECC_MAXIMIZE;
+	denali->clk_rate = 50000000;		/* 50 MHz */
 	denali->clk_x_rate = 200000000;		/* 200 MHz */
 
 	ret = pci_request_regions(dev, DENALI_NAND_NAME);
-- 
2.7.4

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Richard Weinberger]

Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> According to the Denali User's Guide, this IP needs three clocks:
> 
>  - clk: controller core clock
> 
>  - clk_x: bus interface clock
> 
>  - ecc_clk: clock at which ECC circuitry is run
> 
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency.  However, the driver needs to get the frequency of "clk_x"
> not "clk".  This is confusing because people tend to assume the
> anonymous clock means the core clock.  In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
> 
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
> 
> However, adding new clocks would break the existing platforms.  For the
> backward compatibility, the driver still accepts a single clock just as
> before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
> 
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
> Cc: linux-stable <stable@vger.kernel.org> #4.14+
> Reported-by: Richard Weinberger <richard@nod.at>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Richard Weinberger <richard@nod.at>
Reported-by: Philipp Rosenberger <p.rosenberger@linutronix.de>

Thanks,
//richard

Re: [PATCH v3 2/3] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev

[Richard Weinberger]

Am Freitag, 15. Juni 2018, 03:18:51 CEST schrieb Masahiro Yamada:
> The probe function references &pdev->dev many times.  Add 'dev' as
> a shorthand.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH v3 3/3] mtd: rawnand: denali: optimize timing parameters for data interface

[Richard Weinberger]

Am Freitag, 15. Juni 2018, 03:18:52 CEST schrieb Masahiro Yamada:
> This commit improves the ->setup_data_interface() hook.
> 
> The denali_setup_data_interface() needs the frequency of clk_x
> and the ratio of clk_x / clk.
> 
> The latter is currently hardcoded in the driver, like this:
> 
>   #define DENALI_CLK_X_MULT       6
> 
> The IP datasheet requires that clk_x / clk be 4, 5, or 6.  I just
> chose 6 because it is the most defensive value, but it is not optimal.
> By getting the clock rate of both "clk" and "clk_x", the driver can
> compute the timing values more precisely.

What datasheet do you have, is it public?
Mine clearly states that the factor is 4.
"The frequency of nand_x_clk is four times the frequency of nand_clk."

> To not break the existing platforms, the fallback value, 50 MHz is
> provided.  It is true for all upstreamed platforms.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Boris Brezillon]

On Mon, 18 Jun 2018 09:09:02 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> > According to the Denali User's Guide, this IP needs three clocks:
> > 
> >  - clk: controller core clock
> > 
> >  - clk_x: bus interface clock
> > 
> >  - ecc_clk: clock at which ECC circuitry is run
> > 
> > Currently, denali_dt.c requires a single anonymous clock and its
> > frequency.  However, the driver needs to get the frequency of "clk_x"
> > not "clk".  This is confusing because people tend to assume the
> > anonymous clock means the core clock.  In fact, I got a report of
> > SOCFPGA breakage because the timing parameters are calculated based
> > on a wrong frequency.
> > 
> > Instead of the cheesy implementation, the clocks in the real hardware
> > should be represented in the driver and the DT-binding.
> > 
> > However, adding new clocks would break the existing platforms.  For the
> > backward compatibility, the driver still accepts a single clock just as
> > before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > the bus interface clock.
> > 
> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
> > Cc: linux-stable <stable@vger.kernel.org> #4.14+
> > Reported-by: Richard Weinberger <richard@nod.at>
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>  
> 
> Reviewed-by: Richard Weinberger <richard@nod.at>

Maybe a

Tested-by: Richard Weinberger <richard@nod.at>

?

> Reported-by: Philipp Rosenberger <p.rosenberger@linutronix.de>

Should I replace your Reported-by by this one or simply add it?

Miquel, I'll take this patch in mtd/fixes, and let you take the 2
others in nand/next. That means you'll have to back merge v4.18-rc2
into the nand/next tree, or base your tree on v4.18-rc2 instead of
v4.18-rc1.

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Boris Brezillon]

On Mon, 18 Jun 2018 09:09:02 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> > According to the Denali User's Guide, this IP needs three clocks:
> > 
> >  - clk: controller core clock
> > 
> >  - clk_x: bus interface clock
> > 
> >  - ecc_clk: clock at which ECC circuitry is run
> > 
> > Currently, denali_dt.c requires a single anonymous clock and its
> > frequency.  However, the driver needs to get the frequency of "clk_x"
> > not "clk".  This is confusing because people tend to assume the
> > anonymous clock means the core clock.  In fact, I got a report of
> > SOCFPGA breakage because the timing parameters are calculated based
> > on a wrong frequency.
> > 
> > Instead of the cheesy implementation, the clocks in the real hardware
> > should be represented in the driver and the DT-binding.
> > 
> > However, adding new clocks would break the existing platforms.  For the
> > backward compatibility, the driver still accepts a single clock just as
> > before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > the bus interface clock.
> > 
> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
> > Cc: linux-stable <stable@vger.kernel.org> #4.14+
> > Reported-by: Richard Weinberger <richard@nod.at>
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>  
> 
> Reviewed-by: Richard Weinberger <richard@nod.at>

Maybe a

Tested-by: Richard Weinberger <richard@nod.at>

?

> Reported-by: Philipp Rosenberger <p.rosenberger@linutronix.de>

Should I replace your Reported-by by this one or simply add it?

Miquel, I'll take this patch in mtd/fixes, and let you take the 2
others in nand/next. That means you'll have to back merge v4.18-rc2
into the nand/next tree, or base your tree on v4.18-rc2 instead of
v4.18-rc1.

Re: [PATCH v3 2/3] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev

[Boris Brezillon]

On Fri, 15 Jun 2018 10:18:51 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> The probe function references &pdev->dev many times.  Add 'dev' as
> a shorthand.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>

> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/raw/denali_dt.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> index ce6239d..afaae37 100644
> --- a/drivers/mtd/nand/raw/denali_dt.c
> +++ b/drivers/mtd/nand/raw/denali_dt.c
> @@ -81,38 +81,39 @@ MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
>  
>  static int denali_dt_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct resource *res;
>  	struct denali_dt *dt;
>  	const struct denali_dt_data *data;
>  	struct denali_nand_info *denali;
>  	int ret;
>  
> -	dt = devm_kzalloc(&pdev->dev, sizeof(*dt), GFP_KERNEL);
> +	dt = devm_kzalloc(dev, sizeof(*dt), GFP_KERNEL);
>  	if (!dt)
>  		return -ENOMEM;
>  	denali = &dt->denali;
>  
> -	data = of_device_get_match_data(&pdev->dev);
> +	data = of_device_get_match_data(dev);
>  	if (data) {
>  		denali->revision = data->revision;
>  		denali->caps = data->caps;
>  		denali->ecc_caps = data->ecc_caps;
>  	}
>  
> -	denali->dev = &pdev->dev;
> +	denali->dev = dev;
>  	denali->irq = platform_get_irq(pdev, 0);
>  	if (denali->irq < 0) {
> -		dev_err(&pdev->dev, "no irq defined\n");
> +		dev_err(dev, "no irq defined\n");
>  		return denali->irq;
>  	}
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "denali_reg");
> -	denali->reg = devm_ioremap_resource(&pdev->dev, res);
> +	denali->reg = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(denali->reg))
>  		return PTR_ERR(denali->reg);
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand_data");
> -	denali->host = devm_ioremap_resource(&pdev->dev, res);
> +	denali->host = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(denali->host))
>  		return PTR_ERR(denali->host);
>  
> @@ -120,19 +121,19 @@ static int denali_dt_probe(struct platform_device *pdev)
>  	 * A single anonymous clock is supported for the backward compatibility.
>  	 * New platforms should support all the named clocks.
>  	 */
> -	dt->clk = devm_clk_get(&pdev->dev, "nand");
> +	dt->clk = devm_clk_get(dev, "nand");
>  	if (IS_ERR(dt->clk))
> -		dt->clk = devm_clk_get(&pdev->dev, NULL);
> +		dt->clk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(dt->clk)) {
> -		dev_err(&pdev->dev, "no clk available\n");
> +		dev_err(dev, "no clk available\n");
>  		return PTR_ERR(dt->clk);
>  	}
>  
> -	dt->clk_x = devm_clk_get(&pdev->dev, "nand_x");
> +	dt->clk_x = devm_clk_get(dev, "nand_x");
>  	if (IS_ERR(dt->clk_x))
>  		dt->clk_x = NULL;
>  
> -	dt->clk_ecc = devm_clk_get(&pdev->dev, "ecc");
> +	dt->clk_ecc = devm_clk_get(dev, "ecc");
>  	if (IS_ERR(dt->clk_ecc))
>  		dt->clk_ecc = NULL;
>  
> @@ -155,7 +156,7 @@ static int denali_dt_probe(struct platform_device *pdev)
>  		 * Hardcode the clock rates for the backward compatibility.
>  		 * This works for both SOCFPGA and UniPhier.
>  		 */
> -		dev_notice(&pdev->dev,
> +		dev_notice(dev,
>  			   "necessary clock is missing. default clock rates are used.\n");
>  		denali->clk_x_rate = 200000000;
>  	}

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Miquel Raynal]

Hi Boris,

On Mon, 18 Jun 2018 09:46:36 +0200, Boris Brezillon
<boris.brezillon@bootlin.com> wrote:

> On Mon, 18 Jun 2018 09:09:02 +0200
> Richard Weinberger <richard@nod.at> wrote:
> 
> > Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:  
> > > According to the Denali User's Guide, this IP needs three clocks:
> > > 
> > >  - clk: controller core clock
> > > 
> > >  - clk_x: bus interface clock
> > > 
> > >  - ecc_clk: clock at which ECC circuitry is run
> > > 
> > > Currently, denali_dt.c requires a single anonymous clock and its
> > > frequency.  However, the driver needs to get the frequency of "clk_x"
> > > not "clk".  This is confusing because people tend to assume the
> > > anonymous clock means the core clock.  In fact, I got a report of
> > > SOCFPGA breakage because the timing parameters are calculated based
> > > on a wrong frequency.
> > > 
> > > Instead of the cheesy implementation, the clocks in the real hardware
> > > should be represented in the driver and the DT-binding.
> > > 
> > > However, adding new clocks would break the existing platforms.  For the
> > > backward compatibility, the driver still accepts a single clock just as
> > > before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > > the bus interface clock.
> > > 
> > > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
> > > Cc: linux-stable <stable@vger.kernel.org> #4.14+
> > > Reported-by: Richard Weinberger <richard@nod.at>
> > > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>    
> > 
> > Reviewed-by: Richard Weinberger <richard@nod.at>  
> 
> Maybe a
> 
> Tested-by: Richard Weinberger <richard@nod.at>
> 
> ?
> 
> > Reported-by: Philipp Rosenberger <p.rosenberger@linutronix.de>  
> 
> Should I replace your Reported-by by this one or simply add it?
> 
> Miquel, I'll take this patch in mtd/fixes, and let you take the 2
> others in nand/next. That means you'll have to back merge v4.18-rc2
> into the nand/next tree, or base your tree on v4.18-rc2 instead of
> v4.18-rc1.

Sure.

Miquèl

Re: [PATCH v3 3/3] mtd: rawnand: denali: optimize timing parameters for data interface

[Masahiro Yamada]

Hi Richard,


2018-06-18 16:22 GMT+09:00 Richard Weinberger <richard@nod.at>:
> Am Freitag, 15. Juni 2018, 03:18:52 CEST schrieb Masahiro Yamada:
>> This commit improves the ->setup_data_interface() hook.
>>
>> The denali_setup_data_interface() needs the frequency of clk_x
>> and the ratio of clk_x / clk.
>>
>> The latter is currently hardcoded in the driver, like this:
>>
>>   #define DENALI_CLK_X_MULT       6
>>
>> The IP datasheet requires that clk_x / clk be 4, 5, or 6.  I just
>> chose 6 because it is the most defensive value, but it is not optimal.
>> By getting the clock rate of both "clk" and "clk_x", the driver can
>> compute the timing values more precisely.
>
> What datasheet do you have, is it public?

No.  Not available in public.

I got the datasheet
because Socionext (formerly, Panasonic) bought this IP.


> Mine clearly states that the factor is 4.
> "The frequency of nand_x_clk is four times the frequency of nand_clk."

I checked
"Denali NAND Flash Memory Controller User's Guide"
released by Denali.

I also get access to a newer version
"Cadence Design IP - NAND Flash Memory Controller User's Guide"
because Cadence acquired Denali.


My datasheet says:
"clk_x - Cadence NAND Flash Memory Controller bus
interface clock. This runs at a configured
multiple of clk as is phase aligned. Configured
multiple can be between 4-6 times."



Various parts of this IP are configurable.
Is yours talking about this IP,
or about a particular configuration for SOCFPGA?



>> To not break the existing platforms, the fallback value, 50 MHz is
>> provided.  It is true for all upstreamed platforms.
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>
> Reviewed-by: Richard Weinberger <richard@nod.at>
>
> Thanks,
> //richard
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Masahiro Yamada]

Hi Boris,


2018-06-18 16:46 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> On Mon, 18 Jun 2018 09:09:02 +0200
> Richard Weinberger <richard@nod.at> wrote:
>
>> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
>> > According to the Denali User's Guide, this IP needs three clocks:
>> >
>> >  - clk: controller core clock
>> >
>> >  - clk_x: bus interface clock
>> >
>> >  - ecc_clk: clock at which ECC circuitry is run
>> >
>> > Currently, denali_dt.c requires a single anonymous clock and its
>> > frequency.  However, the driver needs to get the frequency of "clk_x"
>> > not "clk".  This is confusing because people tend to assume the
>> > anonymous clock means the core clock.  In fact, I got a report of
>> > SOCFPGA breakage because the timing parameters are calculated based
>> > on a wrong frequency.
>> >
>> > Instead of the cheesy implementation, the clocks in the real hardware
>> > should be represented in the driver and the DT-binding.
>> >
>> > However, adding new clocks would break the existing platforms.  For the
>> > backward compatibility, the driver still accepts a single clock just as
>> > before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
>> > This is fine for existing DT of Socionext UniPhier, and also fixes the
>> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
>> > the bus interface clock.
>> >
>> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
>> > Cc: linux-stable <stable@vger.kernel.org> #4.14+
>> > Reported-by: Richard Weinberger <richard@nod.at>
>> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>
>> Reviewed-by: Richard Weinberger <richard@nod.at>
>
> Maybe a
>
> Tested-by: Richard Weinberger <richard@nod.at>
>
> ?
>
>> Reported-by: Philipp Rosenberger <p.rosenberger@linutronix.de>
>
> Should I replace your Reported-by by this one or simply add it?


I think it is good to have Reported-by
both from Philipp and Richard.


Thanks.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Miquel Raynal]

On Fri, 15 Jun 2018 10:18:50 +0900, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:

> According to the Denali User's Guide, this IP needs three clocks:
> 
>  - clk: controller core clock
> 
>  - clk_x: bus interface clock
> 
>  - ecc_clk: clock at which ECC circuitry is run
> 
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency.  However, the driver needs to get the frequency of "clk_x"
> not "clk".  This is confusing because people tend to assume the
> anonymous clock means the core clock.  In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
> 
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
> 
> However, adding new clocks would break the existing platforms.  For the
> backward compatibility, the driver still accepts a single clock just as
> before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
> 
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
> Cc: linux-stable <stable@vger.kernel.org> #4.14+
> Reported-by: Richard Weinberger <richard@nod.at>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Richard Weinberger]

Am Dienstag, 19. Juni 2018, 10:07:26 CEST schrieb Masahiro Yamada:
> Hi Boris,
> 
> 
> 2018-06-18 16:46 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> > On Mon, 18 Jun 2018 09:09:02 +0200
> > Richard Weinberger <richard@nod.at> wrote:
> >
> >> Am Freitag, 15. Juni 2018, 03:18:50 CEST schrieb Masahiro Yamada:
> >> > According to the Denali User's Guide, this IP needs three clocks:
> >> >
> >> >  - clk: controller core clock
> >> >
> >> >  - clk_x: bus interface clock
> >> >
> >> >  - ecc_clk: clock at which ECC circuitry is run
> >> >
> >> > Currently, denali_dt.c requires a single anonymous clock and its
> >> > frequency.  However, the driver needs to get the frequency of "clk_x"
> >> > not "clk".  This is confusing because people tend to assume the
> >> > anonymous clock means the core clock.  In fact, I got a report of
> >> > SOCFPGA breakage because the timing parameters are calculated based
> >> > on a wrong frequency.
> >> >
> >> > Instead of the cheesy implementation, the clocks in the real hardware
> >> > should be represented in the driver and the DT-binding.
> >> >
> >> > However, adding new clocks would break the existing platforms.  For the
> >> > backward compatibility, the driver still accepts a single clock just as
> >> > before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
> >> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> >> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> >> > the bus interface clock.
> >> >
> >> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
> >> > Cc: linux-stable <stable@vger.kernel.org> #4.14+
> >> > Reported-by: Richard Weinberger <richard@nod.at>
> >> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> >>
> >> Reviewed-by: Richard Weinberger <richard@nod.at>
> >
> > Maybe a
> >
> > Tested-by: Richard Weinberger <richard@nod.at>
> >
> > ?
> >
> >> Reported-by: Philipp Rosenberger <p.rosenberger@linutronix.de>
> >
> > Should I replace your Reported-by by this one or simply add it?

Philipp deserves the Reported-by. :)

> 
> I think it is good to have Reported-by
> both from Philipp and Richard.

Patch 1/3 unbreaks v4.14.x on my board.

Tested-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH v3 2/3] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev

[Richard Weinberger]

Am Freitag, 15. Juni 2018, 03:18:51 CEST schrieb Masahiro Yamada:
> The probe function references &pdev->dev many times.  Add 'dev' as
> a shorthand.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>  drivers/mtd/nand/raw/denali_dt.c | 25 +++++++++++++------------
>  1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> index ce6239d..afaae37 100644
> --- a/drivers/mtd/nand/raw/denali_dt.c
> +++ b/drivers/mtd/nand/raw/denali_dt.c
> @@ -81,38 +81,39 @@ MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
>  
>  static int denali_dt_probe(struct platform_device *pdev)
>  {
> +	struct device *dev = &pdev->dev;
>  	struct resource *res;
>  	struct denali_dt *dt;
>  	const struct denali_dt_data *data;
>  	struct denali_nand_info *denali;
>  	int ret;
>  
> -	dt = devm_kzalloc(&pdev->dev, sizeof(*dt), GFP_KERNEL);
> +	dt = devm_kzalloc(dev, sizeof(*dt), GFP_KERNEL);
>  	if (!dt)
>  		return -ENOMEM;
>  	denali = &dt->denali;
>  
> -	data = of_device_get_match_data(&pdev->dev);
> +	data = of_device_get_match_data(dev);
>  	if (data) {
>  		denali->revision = data->revision;
>  		denali->caps = data->caps;
>  		denali->ecc_caps = data->ecc_caps;
>  	}
>  
> -	denali->dev = &pdev->dev;
> +	denali->dev = dev;
>  	denali->irq = platform_get_irq(pdev, 0);
>  	if (denali->irq < 0) {
> -		dev_err(&pdev->dev, "no irq defined\n");
> +		dev_err(dev, "no irq defined\n");
>  		return denali->irq;
>  	}
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "denali_reg");
> -	denali->reg = devm_ioremap_resource(&pdev->dev, res);
> +	denali->reg = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(denali->reg))
>  		return PTR_ERR(denali->reg);
>  
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "nand_data");
> -	denali->host = devm_ioremap_resource(&pdev->dev, res);
> +	denali->host = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(denali->host))
>  		return PTR_ERR(denali->host);
>  
> @@ -120,19 +121,19 @@ static int denali_dt_probe(struct platform_device *pdev)
>  	 * A single anonymous clock is supported for the backward compatibility.
>  	 * New platforms should support all the named clocks.
>  	 */
> -	dt->clk = devm_clk_get(&pdev->dev, "nand");
> +	dt->clk = devm_clk_get(dev, "nand");
>  	if (IS_ERR(dt->clk))
> -		dt->clk = devm_clk_get(&pdev->dev, NULL);
> +		dt->clk = devm_clk_get(dev, NULL);
>  	if (IS_ERR(dt->clk)) {
> -		dev_err(&pdev->dev, "no clk available\n");
> +		dev_err(dev, "no clk available\n");
>  		return PTR_ERR(dt->clk);
>  	}
>  
> -	dt->clk_x = devm_clk_get(&pdev->dev, "nand_x");
> +	dt->clk_x = devm_clk_get(dev, "nand_x");
>  	if (IS_ERR(dt->clk_x))
>  		dt->clk_x = NULL;
>  
> -	dt->clk_ecc = devm_clk_get(&pdev->dev, "ecc");
> +	dt->clk_ecc = devm_clk_get(dev, "ecc");
>  	if (IS_ERR(dt->clk_ecc))
>  		dt->clk_ecc = NULL;
>  
> @@ -155,7 +156,7 @@ static int denali_dt_probe(struct platform_device *pdev)
>  		 * Hardcode the clock rates for the backward compatibility.
>  		 * This works for both SOCFPGA and UniPhier.
>  		 */
> -		dev_notice(&pdev->dev,
> +		dev_notice(dev,
>  			   "necessary clock is missing. default clock rates are used.\n");
>  		denali->clk_x_rate = 200000000;
>  	}
> 

Tested-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH v3 3/3] mtd: rawnand: denali: optimize timing parameters for data interface

[Richard Weinberger]

Am Freitag, 15. Juni 2018, 03:18:52 CEST schrieb Masahiro Yamada:
> This commit improves the ->setup_data_interface() hook.
> 
> The denali_setup_data_interface() needs the frequency of clk_x
> and the ratio of clk_x / clk.
> 
> The latter is currently hardcoded in the driver, like this:
> 
>   #define DENALI_CLK_X_MULT       6
> 
> The IP datasheet requires that clk_x / clk be 4, 5, or 6.  I just
> chose 6 because it is the most defensive value, but it is not optimal.
> By getting the clock rate of both "clk" and "clk_x", the driver can
> compute the timing values more precisely.
> 
> To not break the existing platforms, the fallback value, 50 MHz is
> provided.  It is true for all upstreamed platforms.
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v3: None
> Changes in v2:
>   - Split patches into sensible chunks
> 
>  drivers/mtd/nand/raw/denali.c     | 49 +++++++++++++++++++--------------------
>  drivers/mtd/nand/raw/denali.h     |  1 +
>  drivers/mtd/nand/raw/denali_dt.c  |  2 ++
>  drivers/mtd/nand/raw/denali_pci.c |  1 +
>  4 files changed, 28 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/denali.c b/drivers/mtd/nand/raw/denali.c
> index 2a302a1..2de46d4 100644
> --- a/drivers/mtd/nand/raw/denali.c
> +++ b/drivers/mtd/nand/raw/denali.c
> @@ -51,14 +51,6 @@ MODULE_LICENSE("GPL");
>  #define DENALI_INVALID_BANK	-1
>  #define DENALI_NR_BANKS		4
>  
> -/*
> - * The bus interface clock, clk_x, is phase aligned with the core clock.  The
> - * clk_x is an integral multiple N of the core clk.  The value N is configured
> - * at IP delivery time, and its available value is 4, 5, or 6.  We need to align
> - * to the largest value to make it work with any possible configuration.
> - */
> -#define DENALI_CLK_X_MULT	6
> -
>  static inline struct denali_nand_info *mtd_to_denali(struct mtd_info *mtd)
>  {
>  	return container_of(mtd_to_nand(mtd), struct denali_nand_info, nand);
> @@ -954,7 +946,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
>  {
>  	struct denali_nand_info *denali = mtd_to_denali(mtd);
>  	const struct nand_sdr_timings *timings;
> -	unsigned long t_clk;
> +	unsigned long t_x, mult_x;
>  	int acc_clks, re_2_we, re_2_re, we_2_re, addr_2_data;
>  	int rdwr_en_lo, rdwr_en_hi, rdwr_en_lo_hi, cs_setup;
>  	int addr_2_data_mask;
> @@ -965,15 +957,24 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
>  		return PTR_ERR(timings);
>  
>  	/* clk_x period in picoseconds */
> -	t_clk = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);
> -	if (!t_clk)
> +	t_x = DIV_ROUND_DOWN_ULL(1000000000000ULL, denali->clk_x_rate);
> +	if (!t_x)
> +		return -EINVAL;
> +
> +	/*
> +	 * The bus interface clock, clk_x, is phase aligned with the core clock.
> +	 * The clk_x is an integral multiple N of the core clk.  The value N is
> +	 * configured at IP delivery time, and its available value is 4, 5, 6.
> +	 */
> +	mult_x = DIV_ROUND_CLOSEST_ULL(denali->clk_x_rate, denali->clk_rate);
> +	if (mult_x < 4 || mult_x > 6)
>  		return -EINVAL;
>  
>  	if (chipnr == NAND_DATA_IFACE_CHECK_ONLY)
>  		return 0;
>  
>  	/* tREA -> ACC_CLKS */
> -	acc_clks = DIV_ROUND_UP(timings->tREA_max, t_clk);
> +	acc_clks = DIV_ROUND_UP(timings->tREA_max, t_x);
>  	acc_clks = min_t(int, acc_clks, ACC_CLKS__VALUE);
>  
>  	tmp = ioread32(denali->reg + ACC_CLKS);
> @@ -982,7 +983,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
>  	iowrite32(tmp, denali->reg + ACC_CLKS);
>  
>  	/* tRWH -> RE_2_WE */
> -	re_2_we = DIV_ROUND_UP(timings->tRHW_min, t_clk);
> +	re_2_we = DIV_ROUND_UP(timings->tRHW_min, t_x);
>  	re_2_we = min_t(int, re_2_we, RE_2_WE__VALUE);
>  
>  	tmp = ioread32(denali->reg + RE_2_WE);
> @@ -991,7 +992,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
>  	iowrite32(tmp, denali->reg + RE_2_WE);
>  
>  	/* tRHZ -> RE_2_RE */
> -	re_2_re = DIV_ROUND_UP(timings->tRHZ_max, t_clk);
> +	re_2_re = DIV_ROUND_UP(timings->tRHZ_max, t_x);
>  	re_2_re = min_t(int, re_2_re, RE_2_RE__VALUE);
>  
>  	tmp = ioread32(denali->reg + RE_2_RE);
> @@ -1005,8 +1006,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
>  	 * With WE_2_RE properly set, the Denali controller automatically takes
>  	 * care of the delay; the driver need not set NAND_WAIT_TCCS.
>  	 */
> -	we_2_re = DIV_ROUND_UP(max(timings->tCCS_min, timings->tWHR_min),
> -			       t_clk);
> +	we_2_re = DIV_ROUND_UP(max(timings->tCCS_min, timings->tWHR_min), t_x);
>  	we_2_re = min_t(int, we_2_re, TWHR2_AND_WE_2_RE__WE_2_RE);
>  
>  	tmp = ioread32(denali->reg + TWHR2_AND_WE_2_RE);
> @@ -1021,7 +1021,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
>  	if (denali->revision < 0x0501)
>  		addr_2_data_mask >>= 1;
>  
> -	addr_2_data = DIV_ROUND_UP(timings->tADL_min, t_clk);
> +	addr_2_data = DIV_ROUND_UP(timings->tADL_min, t_x);
>  	addr_2_data = min_t(int, addr_2_data, addr_2_data_mask);
>  
>  	tmp = ioread32(denali->reg + TCWAW_AND_ADDR_2_DATA);
> @@ -1031,7 +1031,7 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
>  
>  	/* tREH, tWH -> RDWR_EN_HI_CNT */
>  	rdwr_en_hi = DIV_ROUND_UP(max(timings->tREH_min, timings->tWH_min),
> -				  t_clk);
> +				  t_x);
>  	rdwr_en_hi = min_t(int, rdwr_en_hi, RDWR_EN_HI_CNT__VALUE);
>  
>  	tmp = ioread32(denali->reg + RDWR_EN_HI_CNT);
> @@ -1040,11 +1040,10 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
>  	iowrite32(tmp, denali->reg + RDWR_EN_HI_CNT);
>  
>  	/* tRP, tWP -> RDWR_EN_LO_CNT */
> -	rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min),
> -				  t_clk);
> +	rdwr_en_lo = DIV_ROUND_UP(max(timings->tRP_min, timings->tWP_min), t_x);
>  	rdwr_en_lo_hi = DIV_ROUND_UP(max(timings->tRC_min, timings->tWC_min),
> -				     t_clk);
> -	rdwr_en_lo_hi = max(rdwr_en_lo_hi, DENALI_CLK_X_MULT);
> +				     t_x);
> +	rdwr_en_lo_hi = max_t(int, rdwr_en_lo_hi, mult_x);
>  	rdwr_en_lo = max(rdwr_en_lo, rdwr_en_lo_hi - rdwr_en_hi);
>  	rdwr_en_lo = min_t(int, rdwr_en_lo, RDWR_EN_LO_CNT__VALUE);
>  
> @@ -1054,8 +1053,8 @@ static int denali_setup_data_interface(struct mtd_info *mtd, int chipnr,
>  	iowrite32(tmp, denali->reg + RDWR_EN_LO_CNT);
>  
>  	/* tCS, tCEA -> CS_SETUP_CNT */
> -	cs_setup = max3((int)DIV_ROUND_UP(timings->tCS_min, t_clk) - rdwr_en_lo,
> -			(int)DIV_ROUND_UP(timings->tCEA_max, t_clk) - acc_clks,
> +	cs_setup = max3((int)DIV_ROUND_UP(timings->tCS_min, t_x) - rdwr_en_lo,
> +			(int)DIV_ROUND_UP(timings->tCEA_max, t_x) - acc_clks,
>  			0);
>  	cs_setup = min_t(int, cs_setup, CS_SETUP_CNT__VALUE);
>  
> @@ -1282,7 +1281,7 @@ int denali_init(struct denali_nand_info *denali)
>  	}
>  
>  	/* clk rate info is needed for setup_data_interface */
> -	if (denali->clk_x_rate)
> +	if (denali->clk_rate && denali->clk_x_rate)
>  		chip->setup_data_interface = denali_setup_data_interface;
>  
>  	ret = nand_scan_ident(mtd, denali->max_banks, NULL);
> diff --git a/drivers/mtd/nand/raw/denali.h b/drivers/mtd/nand/raw/denali.h
> index 9ad33d2..1f8feaf 100644
> --- a/drivers/mtd/nand/raw/denali.h
> +++ b/drivers/mtd/nand/raw/denali.h
> @@ -300,6 +300,7 @@
>  
>  struct denali_nand_info {
>  	struct nand_chip nand;
> +	unsigned long clk_rate;		/* core clock rate */
>  	unsigned long clk_x_rate;	/* bus interface clock rate */
>  	int active_bank;		/* currently selected bank */
>  	struct device *dev;
> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> index afaae37..0faaad0 100644
> --- a/drivers/mtd/nand/raw/denali_dt.c
> +++ b/drivers/mtd/nand/raw/denali_dt.c
> @@ -150,6 +150,7 @@ static int denali_dt_probe(struct platform_device *pdev)
>  		goto out_disable_clk_x;
>  
>  	if (dt->clk_x) {
> +		denali->clk_rate = clk_get_rate(dt->clk);
>  		denali->clk_x_rate = clk_get_rate(dt->clk_x);
>  	} else {
>  		/*
> @@ -158,6 +159,7 @@ static int denali_dt_probe(struct platform_device *pdev)
>  		 */
>  		dev_notice(dev,
>  			   "necessary clock is missing. default clock rates are used.\n");
> +		denali->clk_rate = 50000000;
>  		denali->clk_x_rate = 200000000;
>  	}
>  
> diff --git a/drivers/mtd/nand/raw/denali_pci.c b/drivers/mtd/nand/raw/denali_pci.c
> index 49cb3e1..7c8efc4 100644
> --- a/drivers/mtd/nand/raw/denali_pci.c
> +++ b/drivers/mtd/nand/raw/denali_pci.c
> @@ -73,6 +73,7 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  	denali->irq = dev->irq;
>  	denali->ecc_caps = &denali_pci_ecc_caps;
>  	denali->nand.ecc.options |= NAND_ECC_MAXIMIZE;
> +	denali->clk_rate = 50000000;		/* 50 MHz */
>  	denali->clk_x_rate = 200000000;		/* 200 MHz */
>  
>  	ret = pci_request_regions(dev, DENALI_NAND_NAME);
> 

Tested-by: Richard Weinberger <richard@nod.at>

Thanks,
//richard

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Boris Brezillon]

Hi Masahiro,

On Fri, 15 Jun 2018 10:18:50 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> According to the Denali User's Guide, this IP needs three clocks:
> 
>  - clk: controller core clock
> 
>  - clk_x: bus interface clock
> 
>  - ecc_clk: clock at which ECC circuitry is run
> 
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency.  However, the driver needs to get the frequency of "clk_x"
> not "clk".  This is confusing because people tend to assume the
> anonymous clock means the core clock.  In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
> 
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
> 
> However, adding new clocks would break the existing platforms.  For the
> backward compatibility, the driver still accepts a single clock just as
> before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
> 
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")

Sorry for changing my mind, but I think this patch is doing more than
just fixing a bug. The bugfix should IMO be limited to unconditionally
setting ->clk_x_rate to 200000000 since this is what was done before
commit 1bb88666775e. And the remaining changes should go in nand/next.

Regards,

Boris

> Cc: linux-stable <stable@vger.kernel.org> #4.14+
> Reported-by: Richard Weinberger <richard@nod.at>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v3:
>   - Change the patch order so that the bug-fix one comes the first
> 
> Changes in v2:
>   - Split patches into sensible chunks
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |  5 +++
>  drivers/mtd/nand/raw/denali_dt.c                   | 49 ++++++++++++++++++++--
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index 0ee8edb..f33da87 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -8,6 +8,9 @@ Required properties:
>    - reg : should contain registers location and length for data and reg.
>    - reg-names: Should contain the reg names "nand_data" and "denali_reg"
>    - interrupts : The interrupt number.
> +  - clocks: should contain phandle of the controller core clock, the bus
> +    interface clock, and the ECC circuit clock.
> +  - clock-names: should contain "nand", "nand_x", "ecc"
>  
>  Optional properties:
>    - nand-ecc-step-size: see nand.txt for details.  If present, the value must be
> @@ -31,5 +34,7 @@ nand: nand@ff900000 {
>  	compatible = "altr,socfpga-denali-nand";
>  	reg = <0xff900000 0x20>, <0xffb80000 0x1000>;
>  	reg-names = "nand_data", "denali_reg";
> +	clocks = <&nand_clk>, <&nand_x_clk>, <&nand_ecc_clk>;
> +	clock-names = "nand", "nand_x", "ecc";
>  	interrupts = <0 144 4>;
>  };
> diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
> index cfd33e6..ce6239d 100644
> --- a/drivers/mtd/nand/raw/denali_dt.c
> +++ b/drivers/mtd/nand/raw/denali_dt.c
> @@ -27,7 +27,9 @@
>  
>  struct denali_dt {
>  	struct denali_nand_info	denali;
> -	struct clk		*clk;
> +	struct clk *clk;	/* core clock */
> +	struct clk *clk_x;	/* bus interface clock */
> +	struct clk *clk_ecc;	/* ECC circuit clock */
>  };
>  
>  struct denali_dt_data {
> @@ -114,24 +116,61 @@ static int denali_dt_probe(struct platform_device *pdev)
>  	if (IS_ERR(denali->host))
>  		return PTR_ERR(denali->host);
>  
> -	dt->clk = devm_clk_get(&pdev->dev, NULL);
> +	/*
> +	 * A single anonymous clock is supported for the backward compatibility.
> +	 * New platforms should support all the named clocks.
> +	 */
> +	dt->clk = devm_clk_get(&pdev->dev, "nand");
> +	if (IS_ERR(dt->clk))
> +		dt->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(dt->clk)) {
>  		dev_err(&pdev->dev, "no clk available\n");
>  		return PTR_ERR(dt->clk);
>  	}
> +
> +	dt->clk_x = devm_clk_get(&pdev->dev, "nand_x");
> +	if (IS_ERR(dt->clk_x))
> +		dt->clk_x = NULL;
> +
> +	dt->clk_ecc = devm_clk_get(&pdev->dev, "ecc");
> +	if (IS_ERR(dt->clk_ecc))
> +		dt->clk_ecc = NULL;
> +
>  	ret = clk_prepare_enable(dt->clk);
>  	if (ret)
>  		return ret;
>  
> -	denali->clk_x_rate = clk_get_rate(dt->clk);
> +	ret = clk_prepare_enable(dt->clk_x);
> +	if (ret)
> +		goto out_disable_clk;
> +
> +	ret = clk_prepare_enable(dt->clk_ecc);
> +	if (ret)
> +		goto out_disable_clk_x;
> +
> +	if (dt->clk_x) {
> +		denali->clk_x_rate = clk_get_rate(dt->clk_x);
> +	} else {
> +		/*
> +		 * Hardcode the clock rates for the backward compatibility.
> +		 * This works for both SOCFPGA and UniPhier.
> +		 */
> +		dev_notice(&pdev->dev,
> +			   "necessary clock is missing. default clock rates are used.\n");
> +		denali->clk_x_rate = 200000000;
> +	}
>  
>  	ret = denali_init(denali);
>  	if (ret)
> -		goto out_disable_clk;
> +		goto out_disable_clk_ecc;
>  
>  	platform_set_drvdata(pdev, dt);
>  	return 0;
>  
> +out_disable_clk_ecc:
> +	clk_disable_unprepare(dt->clk_ecc);
> +out_disable_clk_x:
> +	clk_disable_unprepare(dt->clk_x);
>  out_disable_clk:
>  	clk_disable_unprepare(dt->clk);
>  
> @@ -143,6 +182,8 @@ static int denali_dt_remove(struct platform_device *pdev)
>  	struct denali_dt *dt = platform_get_drvdata(pdev);
>  
>  	denali_remove(&dt->denali);
> +	clk_disable_unprepare(dt->clk_ecc);
> +	clk_disable_unprepare(dt->clk_x);
>  	clk_disable_unprepare(dt->clk);
>  
>  	return 0;

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Rob Herring]

On Fri, Jun 15, 2018 at 10:18:50AM +0900, Masahiro Yamada wrote:
> According to the Denali User's Guide, this IP needs three clocks:
> 
>  - clk: controller core clock
> 
>  - clk_x: bus interface clock
> 
>  - ecc_clk: clock at which ECC circuitry is run
> 
> Currently, denali_dt.c requires a single anonymous clock and its
> frequency.  However, the driver needs to get the frequency of "clk_x"
> not "clk".  This is confusing because people tend to assume the
> anonymous clock means the core clock.  In fact, I got a report of
> SOCFPGA breakage because the timing parameters are calculated based
> on a wrong frequency.
> 
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver and the DT-binding.
> 
> However, adding new clocks would break the existing platforms.  For the
> backward compatibility, the driver still accepts a single clock just as
> before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
> This is fine for existing DT of Socionext UniPhier, and also fixes the
> issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> the bus interface clock.
> 
> Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
> Cc: linux-stable <stable@vger.kernel.org> #4.14+
> Reported-by: Richard Weinberger <richard@nod.at>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
> Changes in v3:
>   - Change the patch order so that the bug-fix one comes the first
> 
> Changes in v2:
>   - Split patches into sensible chunks
> 
>  .../devicetree/bindings/mtd/denali-nand.txt        |  5 +++
>  drivers/mtd/nand/raw/denali_dt.c                   | 49 ++++++++++++++++++++--
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index 0ee8edb..f33da87 100644
> --- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> @@ -8,6 +8,9 @@ Required properties:
>    - reg : should contain registers location and length for data and reg.
>    - reg-names: Should contain the reg names "nand_data" and "denali_reg"
>    - interrupts : The interrupt number.
> +  - clocks: should contain phandle of the controller core clock, the bus
> +    interface clock, and the ECC circuit clock.
> +  - clock-names: should contain "nand", "nand_x", "ecc"

The subject says "add more clocks", but it doesn't look like any clocks 
were supported before. Or documentation was missing?

Please split bindings to a separate patch.

Rob

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Boris Brezillon]

Hi Masahiro,

On Tue, 19 Jun 2018 13:28:11 +0200
Boris Brezillon <boris.brezillon@bootlin.com> wrote:

> Hi Masahiro,
> 
> On Fri, 15 Jun 2018 10:18:50 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 
> > According to the Denali User's Guide, this IP needs three clocks:
> > 
> >  - clk: controller core clock
> > 
> >  - clk_x: bus interface clock
> > 
> >  - ecc_clk: clock at which ECC circuitry is run
> > 
> > Currently, denali_dt.c requires a single anonymous clock and its
> > frequency.  However, the driver needs to get the frequency of "clk_x"
> > not "clk".  This is confusing because people tend to assume the
> > anonymous clock means the core clock.  In fact, I got a report of
> > SOCFPGA breakage because the timing parameters are calculated based
> > on a wrong frequency.
> > 
> > Instead of the cheesy implementation, the clocks in the real hardware
> > should be represented in the driver and the DT-binding.
> > 
> > However, adding new clocks would break the existing platforms.  For the
> > backward compatibility, the driver still accepts a single clock just as
> > before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
> > This is fine for existing DT of Socionext UniPhier, and also fixes the
> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
> > the bus interface clock.
> > 
> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")  
> 
> Sorry for changing my mind, but I think this patch is doing more than
> just fixing a bug. The bugfix should IMO be limited to unconditionally
> setting ->clk_x_rate to 200000000 since this is what was done before
> commit 1bb88666775e. And the remaining changes should go in nand/next.

Do you want me to write this patch? Note that I can't really take this
patch since Rob asked for a new version with DT bindings changes placed
in a separate patch.

Regards,

Boris

Re: [PATCH v3 1/3] mtd: rawnand: denali_dt: add more clocks based on IP datasheet

[Masahiro Yamada]

2018-06-22 20:42 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> Hi Masahiro,
>
> On Tue, 19 Jun 2018 13:28:11 +0200
> Boris Brezillon <boris.brezillon@bootlin.com> wrote:
>
>> Hi Masahiro,
>>
>> On Fri, 15 Jun 2018 10:18:50 +0900
>> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>>
>> > According to the Denali User's Guide, this IP needs three clocks:
>> >
>> >  - clk: controller core clock
>> >
>> >  - clk_x: bus interface clock
>> >
>> >  - ecc_clk: clock at which ECC circuitry is run
>> >
>> > Currently, denali_dt.c requires a single anonymous clock and its
>> > frequency.  However, the driver needs to get the frequency of "clk_x"
>> > not "clk".  This is confusing because people tend to assume the
>> > anonymous clock means the core clock.  In fact, I got a report of
>> > SOCFPGA breakage because the timing parameters are calculated based
>> > on a wrong frequency.
>> >
>> > Instead of the cheesy implementation, the clocks in the real hardware
>> > should be represented in the driver and the DT-binding.
>> >
>> > However, adding new clocks would break the existing platforms.  For the
>> > backward compatibility, the driver still accepts a single clock just as
>> > before.  If clk_x is missing, clk_x_rate is set to a hardcoded value.
>> > This is fine for existing DT of Socionext UniPhier, and also fixes the
>> > issue of Altera (Intel) SOCFPGA because both platforms use 200 MHz for
>> > the bus interface clock.
>> >
>> > Fixes: 1bb88666775e ("mtd: nand: denali: handle timing parameters by setup_data_interface()")
>>
>> Sorry for changing my mind, but I think this patch is doing more than
>> just fixing a bug. The bugfix should IMO be limited to unconditionally
>> setting ->clk_x_rate to 200000000 since this is what was done before
>> commit 1bb88666775e. And the remaining changes should go in nand/next.
>
> Do you want me to write this patch? Note that I can't really take this
> patch since Rob asked for a new version with DT bindings changes placed
> in a separate patch.


Sorry for the delay.  I was traveling.

Now I am back home, and started to flush my TODO queue.

V4 will be soon.




-- 
Best Regards
Masahiro Yamada