linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
@ 2018-06-12 10:31 Masahiro Yamada
  2018-06-12 10:31 ` [PATCH 2/2] mtd: rawnand: denali: add more clocks Masahiro Yamada
  2018-06-18 11:54 ` [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev Miquel Raynal
  0 siblings, 2 replies; 8+ messages in thread
From: Masahiro Yamada @ 2018-06-12 10:31 UTC (permalink / raw)
  To: linux-mtd, Boris Brezillon
  Cc: Richard Weinberger, Philipp Rosenberger, Rob Herring,
	Masahiro Yamada, Miquel Raynal, linux-kernel, Marek Vasut,
	Brian Norris, Richard Weinberger, David Woodhouse

Add 'dev' as a shorthand before adding more code that takes a
pointer to struce device.

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

 drivers/mtd/nand/raw/denali_dt.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/nand/raw/denali_dt.c b/drivers/mtd/nand/raw/denali_dt.c
index cfd33e6..d923cfa 100644
--- a/drivers/mtd/nand/raw/denali_dt.c
+++ b/drivers/mtd/nand/raw/denali_dt.c
@@ -79,44 +79,45 @@ 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);
 
-	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);
 	}
 	ret = clk_prepare_enable(dt->clk);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] mtd: rawnand: denali: add more clocks
  2018-06-12 10:31 [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev Masahiro Yamada
@ 2018-06-12 10:31 ` Masahiro Yamada
  2018-06-12 10:36   ` Masahiro Yamada
  2018-06-18 11:54 ` [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev Miquel Raynal
  1 sibling, 1 reply; 8+ messages in thread
From: Masahiro Yamada @ 2018-06-12 10:31 UTC (permalink / raw)
  To: linux-mtd, Boris Brezillon
  Cc: Richard Weinberger, Philipp Rosenberger, Rob Herring,
	Masahiro Yamada, devicetree, Miquel Raynal, linux-kernel,
	Marek Vasut, Brian Norris, Richard Weinberger, David Woodhouse,
	Rob Herring, Mark Rutland

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

 - 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.

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

I improved the ->setup_data_interface() hook as well.

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 specifies that clk_x / clk should be 4, 5, or 6.
I just chose 6 because it is the most defensive value, but it is not
very nice.  By getting the clock rate of both "clk" and "clk_x", the
driver can compute the timing values more precisely.

Adding new clocks would break the existing platforms.  For the backward
compatibility, the driver still accepts the single clock just as before.
Also, ->clk_rate and ->clk_x_rate are set to hardcoded values if clk_x
is missing.

It should work for all upstreamed platforms because Intel Moorestown,
Altera (Intel) SOCFPGA, Socionext UniPhier use the following clock
frequency:

  clk = 50 MHz
  clk_x = ecc_clk = 200 MHz

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

 .../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                   | 51 ++++++++++++++++++++--
 drivers/mtd/nand/raw/denali_pci.c                  |  1 +
 5 files changed, 78 insertions(+), 29 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.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 d923cfa..0faaad0 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 {
@@ -115,24 +117,63 @@ static int denali_dt_probe(struct platform_device *pdev)
 	if (IS_ERR(denali->host))
 		return PTR_ERR(denali->host);
 
-	dt->clk = devm_clk_get(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(dev, "nand");
+	if (IS_ERR(dt->clk))
+		dt->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(dt->clk)) {
 		dev_err(dev, "no clk available\n");
 		return PTR_ERR(dt->clk);
 	}
+
+	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(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_rate = clk_get_rate(dt->clk);
+		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(dev,
+			   "necessary clock is missing. default clock rates are used.\n");
+		denali->clk_rate = 50000000;
+		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);
 
@@ -144,6 +185,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;
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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mtd: rawnand: denali: add more clocks
  2018-06-12 10:31 ` [PATCH 2/2] mtd: rawnand: denali: add more clocks Masahiro Yamada
@ 2018-06-12 10:36   ` Masahiro Yamada
  2018-06-12 11:03     ` Boris Brezillon
  0 siblings, 1 reply; 8+ messages in thread
From: Masahiro Yamada @ 2018-06-12 10:36 UTC (permalink / raw)
  To: linux-mtd, Boris Brezillon
  Cc: Mark Rutland, Rob Herring, Brian Norris, DTML, Richard Weinberger,
	Linux Kernel Mailing List, Masahiro Yamada, Rob Herring,
	Miquel Raynal, Philipp Rosenberger, Richard Weinberger,
	David Woodhouse, Marek Vasut

Maybe, the commit title should be:

mtd: rawnand: denali: add more clocks and improve setup_data_interface



2018-06-12 19:31 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> According to the Denali User's Guide, this IP needs three clock inputs:
>
>  - 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.
>
> Instead of the cheesy implementation, the clocks in the real hardware
> should be represented in the driver code and the DT-binding.
>
> I improved the ->setup_data_interface() hook as well.
>
> 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 specifies that clk_x / clk should be 4, 5, or 6.
> I just chose 6 because it is the most defensive value, but it is not
> very nice.  By getting the clock rate of both "clk" and "clk_x", the
> driver can compute the timing values more precisely.
>
> Adding new clocks would break the existing platforms.  For the backward
> compatibility, the driver still accepts the single clock just as before.
> Also, ->clk_rate and ->clk_x_rate are set to hardcoded values if clk_x
> is missing.
>
> It should work for all upstreamed platforms because Intel Moorestown,
> Altera (Intel) SOCFPGA, Socionext UniPhier use the following clock
> frequency:
>
>   clk = 50 MHz
>   clk_x = ecc_clk = 200 MHz
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---


-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mtd: rawnand: denali: add more clocks
  2018-06-12 10:36   ` Masahiro Yamada
@ 2018-06-12 11:03     ` Boris Brezillon
  2018-06-12 11:05       ` Richard Weinberger
  2018-06-12 11:06       ` Masahiro Yamada
  0 siblings, 2 replies; 8+ messages in thread
From: Boris Brezillon @ 2018-06-12 11:03 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-mtd, Mark Rutland, Rob Herring, Brian Norris, DTML,
	Richard Weinberger, Linux Kernel Mailing List, Rob Herring,
	Miquel Raynal, Philipp Rosenberger, Richard Weinberger,
	David Woodhouse, Marek Vasut

On Tue, 12 Jun 2018 19:36:06 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Maybe, the commit title should be:
> 
> mtd: rawnand: denali: add more clocks and improve setup_data_interface

Or you split that in 2 commits, one retrieving the new clks, and the
other one fixing/improving ->setup_data_interface().

> 
> 
> 
> 2018-06-12 19:31 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> > According to the Denali User's Guide, this IP needs three clock inputs:
> >
> >  - 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.
> >
> > Instead of the cheesy implementation, the clocks in the real hardware
> > should be represented in the driver code and the DT-binding.
> >
> > I improved the ->setup_data_interface() hook as well.
> >
> > 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 specifies that clk_x / clk should be 4, 5, or 6.
> > I just chose 6 because it is the most defensive value, but it is not
> > very nice.  By getting the clock rate of both "clk" and "clk_x", the
> > driver can compute the timing values more precisely.
> >
> > Adding new clocks would break the existing platforms.  For the backward
> > compatibility, the driver still accepts the single clock just as before.
> > Also, ->clk_rate and ->clk_x_rate are set to hardcoded values if clk_x
> > is missing.
> >
> > It should work for all upstreamed platforms because Intel Moorestown,
> > Altera (Intel) SOCFPGA, Socionext UniPhier use the following clock
> > frequency:
> >
> >   clk = 50 MHz
> >   clk_x = ecc_clk = 200 MHz
> >
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> > ---  
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mtd: rawnand: denali: add more clocks
  2018-06-12 11:03     ` Boris Brezillon
@ 2018-06-12 11:05       ` Richard Weinberger
  2018-06-12 11:06       ` Masahiro Yamada
  1 sibling, 0 replies; 8+ messages in thread
From: Richard Weinberger @ 2018-06-12 11:05 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Masahiro Yamada, linux-mtd, Mark Rutland, Rob Herring,
	Brian Norris, DTML, Linux Kernel Mailing List, Rob Herring,
	Miquel Raynal, Philipp Rosenberger, David Woodhouse, Marek Vasut

Am Dienstag, 12. Juni 2018, 13:03:29 CEST schrieb Boris Brezillon:
> On Tue, 12 Jun 2018 19:36:06 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 
> > Maybe, the commit title should be:
> > 
> > mtd: rawnand: denali: add more clocks and improve setup_data_interface
> 
> Or you split that in 2 commits, one retrieving the new clks, and the
> other one fixing/improving ->setup_data_interface().

Yeah, please split the patch. It needs to go into -stable as well.
One patch should just unbreak the driver.

Thanks,
//richard

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 2/2] mtd: rawnand: denali: add more clocks
  2018-06-12 11:03     ` Boris Brezillon
  2018-06-12 11:05       ` Richard Weinberger
@ 2018-06-12 11:06       ` Masahiro Yamada
  1 sibling, 0 replies; 8+ messages in thread
From: Masahiro Yamada @ 2018-06-12 11:06 UTC (permalink / raw)
  To: Boris Brezillon
  Cc: Mark Rutland, Rob Herring, Richard Weinberger, DTML,
	Richard Weinberger, Linux Kernel Mailing List, Marek Vasut,
	Rob Herring, linux-mtd, Miquel Raynal, Philipp Rosenberger,
	Brian Norris, David Woodhouse

2018-06-12 20:03 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> On Tue, 12 Jun 2018 19:36:06 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Maybe, the commit title should be:
>>
>> mtd: rawnand: denali: add more clocks and improve setup_data_interface
>
> Or you split that in 2 commits, one retrieving the new clks, and the
> other one fixing/improving ->setup_data_interface().


Good idea.

I will try that in v2.




-- 
Best Regards
Masahiro Yamada

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
  2018-06-12 10:31 [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev Masahiro Yamada
  2018-06-12 10:31 ` [PATCH 2/2] mtd: rawnand: denali: add more clocks Masahiro Yamada
@ 2018-06-18 11:54 ` Miquel Raynal
  2018-06-18 12:14   ` Miquel Raynal
  1 sibling, 1 reply; 8+ messages in thread
From: Miquel Raynal @ 2018-06-18 11:54 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-mtd, Boris Brezillon, Richard Weinberger,
	Philipp Rosenberger, Rob Herring, linux-kernel, Marek Vasut,
	Brian Norris, Richard Weinberger, David Woodhouse

Hi Masahiro,

On Tue, 12 Jun 2018 19:31:54 +0900, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:

> Add 'dev' as a shorthand before adding more code that takes a
> pointer to struce device.

s/struce/a struct/?

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

With this fixed:

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

Thanks

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
  2018-06-18 11:54 ` [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev Miquel Raynal
@ 2018-06-18 12:14   ` Miquel Raynal
  0 siblings, 0 replies; 8+ messages in thread
From: Miquel Raynal @ 2018-06-18 12:14 UTC (permalink / raw)
  To: Masahiro Yamada
  Cc: linux-mtd, Boris Brezillon, Richard Weinberger,
	Philipp Rosenberger, Rob Herring, linux-kernel, Marek Vasut,
	Brian Norris, Richard Weinberger, David Woodhouse

Hi Masahiro,

On Mon, 18 Jun 2018 13:54:10 +0200, Miquel Raynal
<miquel.raynal@bootlin.com> wrote:

> Hi Masahiro,
> 
> On Tue, 12 Jun 2018 19:31:54 +0900, Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> 
> > Add 'dev' as a shorthand before adding more code that takes a
> > pointer to struce device.  
> 
> s/struce/a struct/?
> 
> > 
> > Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>  
> 
> With this fixed:
> 
> Acked-by: Miquel Raynal <miquel.raynal@bootlin.com>

I missed the two next versions with Boris/Richard reviews, forget about
this one.

Sorry for the inconvenience,
Miquèl

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2018-06-18 12:14 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-12 10:31 [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev Masahiro Yamada
2018-06-12 10:31 ` [PATCH 2/2] mtd: rawnand: denali: add more clocks Masahiro Yamada
2018-06-12 10:36   ` Masahiro Yamada
2018-06-12 11:03     ` Boris Brezillon
2018-06-12 11:05       ` Richard Weinberger
2018-06-12 11:06       ` Masahiro Yamada
2018-06-18 11:54 ` [PATCH 1/2] mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev Miquel Raynal
2018-06-18 12:14   ` Miquel Raynal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).