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

[PATCH v2 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 v2:
  - Split patches into sensible chunks

Masahiro Yamada (3):
  mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
  mtd: rawnand: denali_dt: add more clocks based on IP datasheet
  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 v2 2/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.

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.
It is fine because both Altera (Intel) SOCFPGA and Socionext UniPhier
use 200 MHz for the bus interface clock.

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

Changes in v2: split 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 d923cfa..afaae37 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,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(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_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_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 +183,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/

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

[Richard Weinberger]

Masahiro,

Am Donnerstag, 14. Juni 2018, 07:11:04 CEST schrieb 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 v2:
>   - Split patches into sensible chunks
> 
> Masahiro Yamada (3):
>   mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
>   mtd: rawnand: denali_dt: add more clocks based on IP datasheet
>   mtd: rawnand: denali: optimize timing parameters for data interface

This still means that we need to feed at least 2/3 and 3/3 into -stable to
unbreak the driver.
I'd really prefer a single self-hosting fix that can go into -stable and then
patches on top of the fix can prettify the driver.

Thanks,
//richard

-- 
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
ATU66964118 - FN 374287y

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

[Masahiro Yamada]

Hi Richard,

2018-06-14 16:25 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:
> Masahiro,
>
> Am Donnerstag, 14. Juni 2018, 07:11:04 CEST schrieb 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 v2:
>>   - Split patches into sensible chunks
>>
>> Masahiro Yamada (3):
>>   mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
>>   mtd: rawnand: denali_dt: add more clocks based on IP datasheet
>>   mtd: rawnand: denali: optimize timing parameters for data interface
>
> This still means that we need to feed at least 2/3 and 3/3 into -stable to
> unbreak the driver.


3/3 is not mandatory.

You can only back-port 1/3 and 2/3.


3/3 is an optimization.
It will make the device access a little faster,
but you do not have to take it if you do not want to.




> I'd really prefer a single self-hosting fix that can go into -stable and then
> patches on top of the fix can prettify the driver.
>
> Thanks,
> //richard
>
> --
> sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
> ATU66964118 - FN 374287y
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" 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 v2 0/3] mtd: rawnand: denali: add new clocks and improve setup_data_interface

[Boris Brezillon]

On Thu, 14 Jun 2018 16:29:59 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Richard,
> 
> 2018-06-14 16:25 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:
> > Masahiro,
> >
> > Am Donnerstag, 14. Juni 2018, 07:11:04 CEST schrieb 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 v2:
> >>   - Split patches into sensible chunks
> >>
> >> Masahiro Yamada (3):
> >>   mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
> >>   mtd: rawnand: denali_dt: add more clocks based on IP datasheet
> >>   mtd: rawnand: denali: optimize timing parameters for data interface  
> >
> > This still means that we need to feed at least 2/3 and 3/3 into -stable to
> > unbreak the driver.  
> 
> 
> 3/3 is not mandatory.
> 
> You can only back-port 1/3 and 2/3.

Well, patch 1 is not a fix, can we move it after patch 2 so that only
patch 2 is flagged with the Fixes+Cc-stable tags?

Thanks,

Boris

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

[Richard Weinberger]

Am Donnerstag, 14. Juni 2018, 09:38:35 CEST schrieb Boris Brezillon:
> On Thu, 14 Jun 2018 16:29:59 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 
> > Hi Richard,
> > 
> > 2018-06-14 16:25 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:
> > > Masahiro,
> > >
> > > Am Donnerstag, 14. Juni 2018, 07:11:04 CEST schrieb 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 v2:
> > >>   - Split patches into sensible chunks
> > >>
> > >> Masahiro Yamada (3):
> > >>   mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
> > >>   mtd: rawnand: denali_dt: add more clocks based on IP datasheet
> > >>   mtd: rawnand: denali: optimize timing parameters for data interface  
> > >
> > > This still means that we need to feed at least 2/3 and 3/3 into -stable to
> > > unbreak the driver.  
> > 
> > 
> > 3/3 is not mandatory.
> > 
> > You can only back-port 1/3 and 2/3.
> 
> Well, patch 1 is not a fix, can we move it after patch 2 so that only
> patch 2 is flagged with the Fixes+Cc-stable tags?

Another question, shall we fix arch/arm/boot/dts/socfpga.dtsi too?

Thanks,
//richard

-- 
sigma star gmbh - Eduard-Bodem-Gasse 6 - 6020 Innsbruck - Austria
ATU66964118 - FN 374287y

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

[Boris Brezillon]

On Thu, 14 Jun 2018 10:02:12 +0200
Richard Weinberger <richard@sigma-star.at> wrote:

> Am Donnerstag, 14. Juni 2018, 09:38:35 CEST schrieb Boris Brezillon:
> > On Thu, 14 Jun 2018 16:29:59 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >   
> > > Hi Richard,
> > > 
> > > 2018-06-14 16:25 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:  
> > > > Masahiro,
> > > >
> > > > Am Donnerstag, 14. Juni 2018, 07:11:04 CEST schrieb 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 v2:
> > > >>   - Split patches into sensible chunks
> > > >>
> > > >> Masahiro Yamada (3):
> > > >>   mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
> > > >>   mtd: rawnand: denali_dt: add more clocks based on IP datasheet
> > > >>   mtd: rawnand: denali: optimize timing parameters for data interface    
> > > >
> > > > This still means that we need to feed at least 2/3 and 3/3 into -stable to
> > > > unbreak the driver.    
> > > 
> > > 
> > > 3/3 is not mandatory.
> > > 
> > > You can only back-port 1/3 and 2/3.  
> > 
> > Well, patch 1 is not a fix, can we move it after patch 2 so that only
> > patch 2 is flagged with the Fixes+Cc-stable tags?  
> 
> Another question, shall we fix arch/arm/boot/dts/socfpga.dtsi too?

Yep.

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

[Masahiro Yamada]

Hi Boris,

2018-06-14 16:38 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> On Thu, 14 Jun 2018 16:29:59 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Richard,
>>
>> 2018-06-14 16:25 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:
>> > Masahiro,
>> >
>> > Am Donnerstag, 14. Juni 2018, 07:11:04 CEST schrieb 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 v2:
>> >>   - Split patches into sensible chunks
>> >>
>> >> Masahiro Yamada (3):
>> >>   mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
>> >>   mtd: rawnand: denali_dt: add more clocks based on IP datasheet
>> >>   mtd: rawnand: denali: optimize timing parameters for data interface
>> >
>> > This still means that we need to feed at least 2/3 and 3/3 into -stable to
>> > unbreak the driver.
>>
>>
>> 3/3 is not mandatory.
>>
>> You can only back-port 1/3 and 2/3.
>
> Well, patch 1 is not a fix, can we move it after patch 2 so that only
> patch 2 is flagged with the Fixes+Cc-stable tags?


OK, will do that.

If you try to port this back to v4.14.*
you need to fix-up the file path
since the driver did not reside in the raw/ sub-directory at that time.


Except that, I do not see backport issue.




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



-- 
Best Regards
Masahiro Yamada

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

[Boris Brezillon]

On Thu, 14 Jun 2018 20:31:59 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 2018-06-14 16:38 GMT+09:00 Boris Brezillon <boris.brezillon@bootlin.com>:
> > On Thu, 14 Jun 2018 16:29:59 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> Hi Richard,
> >>
> >> 2018-06-14 16:25 GMT+09:00 Richard Weinberger <richard@sigma-star.at>:  
> >> > Masahiro,
> >> >
> >> > Am Donnerstag, 14. Juni 2018, 07:11:04 CEST schrieb 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 v2:
> >> >>   - Split patches into sensible chunks
> >> >>
> >> >> Masahiro Yamada (3):
> >> >>   mtd: rawnand: denali_dt: use dev as a shorthand of &pdev->dev
> >> >>   mtd: rawnand: denali_dt: add more clocks based on IP datasheet
> >> >>   mtd: rawnand: denali: optimize timing parameters for data interface  
> >> >
> >> > This still means that we need to feed at least 2/3 and 3/3 into -stable to
> >> > unbreak the driver.  
> >>
> >>
> >> 3/3 is not mandatory.
> >>
> >> You can only back-port 1/3 and 2/3.  
> >
> > Well, patch 1 is not a fix, can we move it after patch 2 so that only
> > patch 2 is flagged with the Fixes+Cc-stable tags?  
> 
> 
> OK, will do that.
> 
> If you try to port this back to v4.14.*
> you need to fix-up the file path
> since the driver did not reside in the raw/ sub-directory at that time.

Yes I know :-/. We'll do that when we receive GKH's notification
saying that the patch does not apply.