[PATCH v2 0/5] mtd: nand: gpmi: add i.MX 7 support

[PATCH v2 0/5] mtd: nand: gpmi: add i.MX 7 support

[Stefan Agner]

This patchset adds support for i.MX 7 SoC for the GPMI NAND controller.
There have been similar patchsets already:
https://lkml.org/lkml/2016/2/23/912

However, this patchset does not make use of any of the new features.
The current feature set seems to work fine, I successfully run the MTD
tests on a Colibri iMX7.

--
Stefan

Changes since v1:
- Make clks_count const
- Introduce IS_IMX7D for i.MX 7 SoC's and make it part of GPMI_IS_MX6

Stefan Agner (5):
  mtd: nand: gpmi: unify clock handling
  mtd: nand: gpmi: add i.MX 7 SoC support
  mtd: gpmi: document current clock requirements
  ARM: dts: imx7: add GPMI NAND
  ARM: dts: imx7-colibri: add NAND support

 .../devicetree/bindings/mtd/gpmi-nand.txt          | 14 +++++-
 arch/arm/boot/dts/imx7-colibri.dtsi                |  9 ++++
 arch/arm/boot/dts/imx7s.dtsi                       | 31 ++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c             | 56 +++++++++++++---------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h             |  9 +++-
 5 files changed, 93 insertions(+), 26 deletions(-)

-- 
2.12.2

[PATCH v2 1/5] mtd: nand: gpmi: unify clock handling

[Stefan Agner]

Add device specific list of clocks required, and handle all clocks
in a single for loop. This avoids further code duplication when
adding i.MX 7 support.

Signed-off-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 41 +++++++++++++++-------------------
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  2 ++
 2 files changed, 20 insertions(+), 23 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index d52139635b67..c8bbf5da2ab8 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -82,6 +82,10 @@ static int gpmi_ooblayout_free(struct mtd_info *mtd, int section,
 	return 0;
 }
 
+static const char * const gpmi_clks_for_mx2x[] = {
+	"gpmi_io",
+};
+
 static const struct mtd_ooblayout_ops gpmi_ooblayout_ops = {
 	.ecc = gpmi_ooblayout_ecc,
 	.free = gpmi_ooblayout_free,
@@ -91,24 +95,36 @@ static const struct gpmi_devdata gpmi_devdata_imx23 = {
 	.type = IS_MX23,
 	.bch_max_ecc_strength = 20,
 	.max_chain_delay = 16,
+	.clks = gpmi_clks_for_mx2x,
+	.clks_count = ARRAY_SIZE(gpmi_clks_for_mx2x),
 };
 
 static const struct gpmi_devdata gpmi_devdata_imx28 = {
 	.type = IS_MX28,
 	.bch_max_ecc_strength = 20,
 	.max_chain_delay = 16,
+	.clks = gpmi_clks_for_mx2x,
+	.clks_count = ARRAY_SIZE(gpmi_clks_for_mx2x),
+};
+
+static const char * const gpmi_clks_for_mx6[] = {
+	"gpmi_io", "gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
 };
 
 static const struct gpmi_devdata gpmi_devdata_imx6q = {
 	.type = IS_MX6Q,
 	.bch_max_ecc_strength = 40,
 	.max_chain_delay = 12,
+	.clks = gpmi_clks_for_mx6,
+	.clks_count = ARRAY_SIZE(gpmi_clks_for_mx6),
 };
 
 static const struct gpmi_devdata gpmi_devdata_imx6sx = {
 	.type = IS_MX6SX,
 	.bch_max_ecc_strength = 62,
 	.max_chain_delay = 12,
+	.clks = gpmi_clks_for_mx6,
+	.clks_count = ARRAY_SIZE(gpmi_clks_for_mx6),
 };
 
 static irqreturn_t bch_irq(int irq, void *cookie)
@@ -599,35 +615,14 @@ static int acquire_dma_channels(struct gpmi_nand_data *this)
 	return -EINVAL;
 }
 
-static char *extra_clks_for_mx6q[GPMI_CLK_MAX] = {
-	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
-};
-
 static int gpmi_get_clks(struct gpmi_nand_data *this)
 {
 	struct resources *r = &this->resources;
-	char **extra_clks = NULL;
 	struct clk *clk;
 	int err, i;
 
-	/* The main clock is stored in the first. */
-	r->clock[0] = devm_clk_get(this->dev, "gpmi_io");
-	if (IS_ERR(r->clock[0])) {
-		err = PTR_ERR(r->clock[0]);
-		goto err_clock;
-	}
-
-	/* Get extra clocks */
-	if (GPMI_IS_MX6(this))
-		extra_clks = extra_clks_for_mx6q;
-	if (!extra_clks)
-		return 0;
-
-	for (i = 1; i < GPMI_CLK_MAX; i++) {
-		if (extra_clks[i - 1] == NULL)
-			break;
-
-		clk = devm_clk_get(this->dev, extra_clks[i - 1]);
+	for (i = 0; i < this->devdata->clks_count; i++) {
+		clk = devm_clk_get(this->dev, this->devdata->clks[i]);
 		if (IS_ERR(clk)) {
 			err = PTR_ERR(clk);
 			goto err_clock;
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index 4e49a1f5fa27..8879c4eff25e 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -130,6 +130,8 @@ struct gpmi_devdata {
 	enum gpmi_type type;
 	int bch_max_ecc_strength;
 	int max_chain_delay; /* See the async EDO mode */
+	const char * const *clks;
+	const int clks_count;
 };
 
 struct gpmi_nand_data {
-- 
2.12.2

[PATCH v2 2/5] mtd: nand: gpmi: add i.MX 7 SoC support

[Stefan Agner]

Add support for i.MX 7 SoC. The i.MX 7 has a slightly different
clock architecture requiring only two clocks to be referenced.
The IP is slightly different compared to i.MX 6, but currently none
of this differences are in use, therefore reuse GPMI_IS_MX6.

Signed-off-by: Stefan Agner <stefan@agner.ch>
Reviewed-by: Marek Vasut <marek.vasut@gmail.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  7 +++++--
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index c8bbf5da2ab8..9b777be633a9 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -127,6 +127,18 @@ static const struct gpmi_devdata gpmi_devdata_imx6sx = {
 	.clks_count = ARRAY_SIZE(gpmi_clks_for_mx6),
 };
 
+static const char * const gpmi_clks_for_mx7d[] = {
+	"gpmi_io", "gpmi_bch_apb",
+};
+
+static const struct gpmi_devdata gpmi_devdata_imx7d = {
+	.type = IS_MX7D,
+	.bch_max_ecc_strength = 62,
+	.max_chain_delay = 12,
+	.clks = gpmi_clks_for_mx7d,
+	.clks_count = ARRAY_SIZE(gpmi_clks_for_mx7d),
+};
+
 static irqreturn_t bch_irq(int irq, void *cookie)
 {
 	struct gpmi_nand_data *this = cookie;
@@ -2071,6 +2083,9 @@ static const struct of_device_id gpmi_nand_id_table[] = {
 	}, {
 		.compatible = "fsl,imx6sx-gpmi-nand",
 		.data = &gpmi_devdata_imx6sx,
+	}, {
+		.compatible = "fsl,imx7d-gpmi-nand",
+		.data = &gpmi_devdata_imx7d,
 	}, {}
 };
 MODULE_DEVICE_TABLE(of, gpmi_nand_id_table);
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index 8879c4eff25e..e88a45a62ab6 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -123,7 +123,8 @@ enum gpmi_type {
 	IS_MX23,
 	IS_MX28,
 	IS_MX6Q,
-	IS_MX6SX
+	IS_MX6SX,
+	IS_MX7D,
 };
 
 struct gpmi_devdata {
@@ -307,6 +308,8 @@ void gpmi_copy_bits(u8 *dst, size_t dst_bit_off,
 #define GPMI_IS_MX28(x)		((x)->devdata->type == IS_MX28)
 #define GPMI_IS_MX6Q(x)		((x)->devdata->type == IS_MX6Q)
 #define GPMI_IS_MX6SX(x)	((x)->devdata->type == IS_MX6SX)
+#define GPMI_IS_MX7D(x)		((x)->devdata->type == IS_MX7D)
 
-#define GPMI_IS_MX6(x)		(GPMI_IS_MX6Q(x) || GPMI_IS_MX6SX(x))
+#define GPMI_IS_MX6(x)		(GPMI_IS_MX6Q(x) || GPMI_IS_MX6SX(x) || \
+				 GPMI_IS_MX7D(x))
 #endif
-- 
2.12.2

[PATCH v2 3/5] mtd: gpmi: document current clock requirements

[Stefan Agner]

The clock requirements are completely missing, add the clocks
currently required by the driver.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 Documentation/devicetree/bindings/mtd/gpmi-nand.txt | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
index d02acaff3c35..b289ef3c1b7e 100644
--- a/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/gpmi-nand.txt
@@ -4,7 +4,12 @@ The GPMI nand controller provides an interface to control the
 NAND flash chips.
 
 Required properties:
-  - compatible : should be "fsl,<chip>-gpmi-nand"
+  - compatible : should be "fsl,<chip>-gpmi-nand", chip can be:
+    * imx23
+    * imx28
+    * imx6q
+    * imx6sx
+    * imx7d
   - reg : should contain registers location and length for gpmi and bch.
   - reg-names: Should contain the reg names "gpmi-nand" and "bch"
   - interrupts : BCH interrupt number.
@@ -13,6 +18,13 @@ Required properties:
     and GPMI DMA channel ID.
     Refer to dma.txt and fsl-mxs-dma.txt for details.
   - dma-names: Must be "rx-tx".
+  - clocks : clocks phandle and clock specifier corresponding to each clock
+    specified in clock-names.
+  - clock-names : The "gpmi_io" clock is always required. Which clocks are
+    exactly required depends on chip:
+    * imx23/imx28 : "gpmi_io"
+    * imx6q/sx : "gpmi_io", "gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch"
+    * imx7d : "gpmi_io", "gpmi_bch_apb"
 
 Optional properties:
   - nand-on-flash-bbt: boolean to enable on flash bbt option if not
-- 
2.12.2

[PATCH v2 4/5] ARM: dts: imx7: add GPMI NAND

[Stefan Agner]

Add i.MX 7 GPMI NAND module.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
index 843eb379e1ea..9645257638d4 100644
--- a/arch/arm/boot/dts/imx7s.dtsi
+++ b/arch/arm/boot/dts/imx7s.dtsi
@@ -995,5 +995,36 @@
 				status = "disabled";
 			};
 		};
+
+		dma_apbh: dma-apbh@33000000 {
+			compatible = "fsl,imx7d-dma-apbh", "fsl,imx28-dma-apbh";
+			reg = <0x33000000 0x2000>;
+			interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "gpmi0", "gpmi1", "gpmi2", "gpmi3";
+			#dma-cells = <1>;
+			dma-channels = <4>;
+			clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
+				<&clks IMX7D_NAND_ROOT_CLK>;
+			clock-names = "dma_apbh_bch", "dma_apbh_io";
+		};
+
+		gpmi: gpmi-nand@33002000{
+			compatible = "fsl,imx7d-gpmi-nand";
+			#address-cells = <1>;
+			#size-cells = <1>;
+			reg = <0x33002000 0x2000>, <0x33004000 0x4000>;
+			reg-names = "gpmi-nand", "bch";
+			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "bch";
+			clocks = <&clks IMX7D_NAND_ROOT_CLK>,
+				<&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;
+			clock-names = "gpmi_io", "gpmi_bch_apb";
+			dmas = <&dma_apbh 0>;
+			dma-names = "rx-tx";
+			status = "disabled";
+		};
 	};
 };
-- 
2.12.2

[PATCH v2 5/5] ARM: dts: imx7-colibri: add NAND support

[Stefan Agner]

The Colibri iMX7 modules come with 512MB on-module SLC NAND flash
populated. Make use of it by enabling the GPMI controller.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 arch/arm/boot/dts/imx7-colibri.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/imx7-colibri.dtsi b/arch/arm/boot/dts/imx7-colibri.dtsi
index 2d87489f9105..ad4ce19d455b 100644
--- a/arch/arm/boot/dts/imx7-colibri.dtsi
+++ b/arch/arm/boot/dts/imx7-colibri.dtsi
@@ -106,6 +106,15 @@
 	fsl,magic-packet;
 };
 
+&gpmi {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_gpmi_nand>;
+	fsl,use-minimum-ecc;
+	nand-on-flash-bbt;
+	nand-ecc-mode = "hw";
+	status = "okay";
+};
+
 &i2c1 {
 	clock-frequency = <100000>;
 	pinctrl-names = "default";
-- 
2.12.2

Re: [PATCH v2 0/5] mtd: nand: gpmi: add i.MX 7 support

[Marek Vasut]

On 04/22/2017 03:23 AM, Stefan Agner wrote:
> This patchset adds support for i.MX 7 SoC for the GPMI NAND controller.
> There have been similar patchsets already:
> https://lkml.org/lkml/2016/2/23/912
> 
> However, this patchset does not make use of any of the new features.
> The current feature set seems to work fine, I successfully run the MTD
> tests on a Colibri iMX7.
> 
> --
> Stefan
> 
> Changes since v1:
> - Make clks_count const
> - Introduce IS_IMX7D for i.MX 7 SoC's and make it part of GPMI_IS_MX6

Super, thanks.

-- 
Best regards,
Marek Vasut

Re: [PATCH v2 3/5] mtd: gpmi: document current clock requirements

[Rob Herring]

On Fri, Apr 21, 2017 at 06:23:36PM -0700, Stefan Agner wrote:
> The clock requirements are completely missing, add the clocks
> currently required by the driver.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  Documentation/devicetree/bindings/mtd/gpmi-nand.txt | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH v2 0/5] mtd: nand: gpmi: add i.MX 7 support

[Shawn Guo]

On Fri, Apr 21, 2017 at 06:23:33PM -0700, Stefan Agner wrote:
> This patchset adds support for i.MX 7 SoC for the GPMI NAND controller.
> There have been similar patchsets already:
> https://lkml.org/lkml/2016/2/23/912
> 
> However, this patchset does not make use of any of the new features.
> The current feature set seems to work fine, I successfully run the MTD
> tests on a Colibri iMX7.
> 
> --
> Stefan
> 
> Changes since v1:
> - Make clks_count const
> - Introduce IS_IMX7D for i.MX 7 SoC's and make it part of GPMI_IS_MX6
> 
> Stefan Agner (5):
>   mtd: nand: gpmi: unify clock handling
>   mtd: nand: gpmi: add i.MX 7 SoC support
>   mtd: gpmi: document current clock requirements
>   ARM: dts: imx7: add GPMI NAND
>   ARM: dts: imx7-colibri: add NAND support

The dts patches look good to me.  I will pick them up once the driver
changes are accepted.

Shawn

> 
>  .../devicetree/bindings/mtd/gpmi-nand.txt          | 14 +++++-
>  arch/arm/boot/dts/imx7-colibri.dtsi                |  9 ++++
>  arch/arm/boot/dts/imx7s.dtsi                       | 31 ++++++++++++
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c             | 56 +++++++++++++---------
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h             |  9 +++-
>  5 files changed, 93 insertions(+), 26 deletions(-)
> 
> -- 
> 2.12.2
> 

Re: [PATCH v2 4/5] ARM: dts: imx7: add GPMI NAND

[Han Xu]

On 04/21/2017 08:23 PM, Stefan Agner wrote:
> Add i.MX 7 GPMI NAND module.
>
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>   arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>
> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
> index 843eb379e1ea..9645257638d4 100644
> --- a/arch/arm/boot/dts/imx7s.dtsi
> +++ b/arch/arm/boot/dts/imx7s.dtsi
> @@ -995,5 +995,36 @@
>   				status = "disabled";
>   			};
>   		};
> +
> +		dma_apbh: dma-apbh@33000000 {
> +			compatible = "fsl,imx7d-dma-apbh", "fsl,imx28-dma-apbh";
> +			reg = <0x33000000 0x2000>;
> +			interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "gpmi0", "gpmi1", "gpmi2", "gpmi3";
> +			#dma-cells = <1>;
> +			dma-channels = <4>;
> +			clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
> +				<&clks IMX7D_NAND_ROOT_CLK>;
> +			clock-names = "dma_apbh_bch", "dma_apbh_io";
> +		};
Do you need some patches to enable all clks for APBH DMA? Refer to 
https://patchwork.ozlabs.org/patch/551967/

> +
> +		gpmi: gpmi-nand@33002000{
> +			compatible = "fsl,imx7d-gpmi-nand";
> +			#address-cells = <1>;
> +			#size-cells = <1>;
> +			reg = <0x33002000 0x2000>, <0x33004000 0x4000>;
> +			reg-names = "gpmi-nand", "bch";
> +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
> +			interrupt-names = "bch";
> +			clocks = <&clks IMX7D_NAND_ROOT_CLK>,
> +				<&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;
> +			clock-names = "gpmi_io", "gpmi_bch_apb";
> +			dmas = <&dma_apbh 0>;
> +			dma-names = "rx-tx";
> +			status = "disabled";
> +		};
>   	};
>   };

Re: [PATCH v2 4/5] ARM: dts: imx7: add GPMI NAND

[Stefan Agner]

On 2017-05-04 12:13, Han Xu wrote:
> On 04/21/2017 08:23 PM, Stefan Agner wrote:
>> Add i.MX 7 GPMI NAND module.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>   arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
>> index 843eb379e1ea..9645257638d4 100644
>> --- a/arch/arm/boot/dts/imx7s.dtsi
>> +++ b/arch/arm/boot/dts/imx7s.dtsi
>> @@ -995,5 +995,36 @@
>>   				status = "disabled";
>>   			};
>>   		};
>> +
>> +		dma_apbh: dma-apbh@33000000 {
>> +			compatible = "fsl,imx7d-dma-apbh", "fsl,imx28-dma-apbh";
>> +			reg = <0x33000000 0x2000>;
>> +			interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>> +			interrupt-names = "gpmi0", "gpmi1", "gpmi2", "gpmi3";
>> +			#dma-cells = <1>;
>> +			dma-channels = <4>;
>> +			clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>> +				<&clks IMX7D_NAND_ROOT_CLK>;
>> +			clock-names = "dma_apbh_bch", "dma_apbh_io";
>> +		};
> Do you need some patches to enable all clks for APBH DMA? Refer to 
> https://patchwork.ozlabs.org/patch/551967/
> 

Oh I see, with the current code and this device tree we actually only
enable the first clock... But it seems to work in practice...

What is interesting, the chapter 5.2.5 System Clocks only describes
APBHDMA, and no GPMI module.

Chapter 9.4 about APBH-Bridge-DMA bridge only specifies "hclk" as clock
sources.

When looking at the table in chapter 5.2.5, hclk is
NAND_USDHC_BUS_CLK_ROOT

Module    Syste Module Clock   Clock Root
APBHDMA   apbhdma.hclk         NAND_USDHC_BUS_CLK_ROOT
 
So my guess is that the actual DMA controller should work fine with
IMX7D_NAND_USDHC_BUS_ROOT_CLK only. It is only the GPMI module which
needs the NAND_ROOT_CLK. My guess is since the APBHDMA controller is
only used for GPMI these days, it has been combined in one module in the
table in chapter 5.2.5.

So we could actually only specify:

clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;

Tested with this and NAND seems to work fine.

Also tested accessing APBHDMA register in U-Boot with
IMX7D_NAND_ROOT_CLK disabled, it worked fine.

What do you think? Should we go with this?

This probably even saves power since GPMI's runtime clock management
disables IMX7D_NAND_ROOT_CLK when NAND is idle (according to the
nand_root_clk counter in clk_summary and the root clock register at
0x3038AA00).

--
Stefan

>> +
>> +		gpmi: gpmi-nand@33002000{
>> +			compatible = "fsl,imx7d-gpmi-nand";
>> +			#address-cells = <1>;
>> +			#size-cells = <1>;
>> +			reg = <0x33002000 0x2000>, <0x33004000 0x4000>;
>> +			reg-names = "gpmi-nand", "bch";
>> +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>> +			interrupt-names = "bch";
>> +			clocks = <&clks IMX7D_NAND_ROOT_CLK>,
>> +				<&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;
>> +			clock-names = "gpmi_io", "gpmi_bch_apb";
>> +			dmas = <&dma_apbh 0>;
>> +			dma-names = "rx-tx";
>> +			status = "disabled";
>> +		};
>>   	};
>>   };

Re: [PATCH v2 4/5] ARM: dts: imx7: add GPMI NAND

[Han Xu]

On 05/04/2017 04:50 PM, Stefan Agner wrote:
> On 2017-05-04 12:13, Han Xu wrote:
>> On 04/21/2017 08:23 PM, Stefan Agner wrote:
>>> Add i.MX 7 GPMI NAND module.
>>>
>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>> ---
>>>    arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
>>>    1 file changed, 31 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
>>> index 843eb379e1ea..9645257638d4 100644
>>> --- a/arch/arm/boot/dts/imx7s.dtsi
>>> +++ b/arch/arm/boot/dts/imx7s.dtsi
>>> @@ -995,5 +995,36 @@
>>>    				status = "disabled";
>>>    			};
>>>    		};
>>> +
>>> +		dma_apbh: dma-apbh@33000000 {
>>> +			compatible = "fsl,imx7d-dma-apbh", "fsl,imx28-dma-apbh";
>>> +			reg = <0x33000000 0x2000>;
>>> +			interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>>> +			interrupt-names = "gpmi0", "gpmi1", "gpmi2", "gpmi3";
>>> +			#dma-cells = <1>;
>>> +			dma-channels = <4>;
>>> +			clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>>> +				<&clks IMX7D_NAND_ROOT_CLK>;
>>> +			clock-names = "dma_apbh_bch", "dma_apbh_io";
>>> +		};
>> Do you need some patches to enable all clks for APBH DMA? Refer to
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F551967%2F&data=01%7C01%7Chan.xu%40nxp.com%7C98493f7e475341dbf67008d49337b441%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=FH9k%2FC4VcVCeyidkRgUJ5Ahu4%2B%2FlDAjXVwO0DY7XMhA%3D&reserved=0
>>
> Oh I see, with the current code and this device tree we actually only
> enable the first clock... But it seems to work in practice...
>
> What is interesting, the chapter 5.2.5 System Clocks only describes
> APBHDMA, and no GPMI module.
>
> Chapter 9.4 about APBH-Bridge-DMA bridge only specifies "hclk" as clock
> sources.
>
> When looking at the table in chapter 5.2.5, hclk is
> NAND_USDHC_BUS_CLK_ROOT
>
> Module    Syste Module Clock   Clock Root
> APBHDMA   apbhdma.hclk         NAND_USDHC_BUS_CLK_ROOT
>   
> So my guess is that the actual DMA controller should work fine with
> IMX7D_NAND_USDHC_BUS_ROOT_CLK only. It is only the GPMI module which
> needs the NAND_ROOT_CLK. My guess is since the APBHDMA controller is
> only used for GPMI these days, it has been combined in one module in the
> table in chapter 5.2.5.
>
> So we could actually only specify:
>
> clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;
>
> Tested with this and NAND seems to work fine.
>
> Also tested accessing APBHDMA register in U-Boot with
> IMX7D_NAND_ROOT_CLK disabled, it worked fine.
>
> What do you think? Should we go with this?
>
> This probably even saves power since GPMI's runtime clock management
> disables IMX7D_NAND_ROOT_CLK when NAND is idle (according to the
> nand_root_clk counter in clk_summary and the root clock register at
> 0x3038AA00).

It also surprised me how it can work on your side, I remember we made 
the code change for some purpose, please give me some time I can check 
if the doc is wrong.

> --
> Stefan
>
>>> +
>>> +		gpmi: gpmi-nand@33002000{
>>> +			compatible = "fsl,imx7d-gpmi-nand";
>>> +			#address-cells = <1>;
>>> +			#size-cells = <1>;
>>> +			reg = <0x33002000 0x2000>, <0x33004000 0x4000>;
>>> +			reg-names = "gpmi-nand", "bch";
>>> +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>>> +			interrupt-names = "bch";
>>> +			clocks = <&clks IMX7D_NAND_ROOT_CLK>,
>>> +				<&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;
>>> +			clock-names = "gpmi_io", "gpmi_bch_apb";
>>> +			dmas = <&dma_apbh 0>;
>>> +			dma-names = "rx-tx";
>>> +			status = "disabled";
>>> +		};
>>>    	};
>>>    };

Re: [PATCH v2 0/5] mtd: nand: gpmi: add i.MX 7 support

[Boris Brezillon]

On Fri, 21 Apr 2017 18:23:33 -0700
Stefan Agner <stefan@agner.ch> wrote:

> This patchset adds support for i.MX 7 SoC for the GPMI NAND controller.
> There have been similar patchsets already:
> https://lkml.org/lkml/2016/2/23/912
> 
> However, this patchset does not make use of any of the new features.
> The current feature set seems to work fine, I successfully run the MTD
> tests on a Colibri iMX7.
> 
> --
> Stefan
> 
> Changes since v1:
> - Make clks_count const
> - Introduce IS_IMX7D for i.MX 7 SoC's and make it part of GPMI_IS_MX6
> 
> Stefan Agner (5):
>   mtd: nand: gpmi: unify clock handling
>   mtd: nand: gpmi: add i.MX 7 SoC support
>   mtd: gpmi: document current clock requirements

Applied patch 1-3 to nand/next.

Thanks,

Boris

>   ARM: dts: imx7: add GPMI NAND
>   ARM: dts: imx7-colibri: add NAND support
> 
>  .../devicetree/bindings/mtd/gpmi-nand.txt          | 14 +++++-
>  arch/arm/boot/dts/imx7-colibri.dtsi                |  9 ++++
>  arch/arm/boot/dts/imx7s.dtsi                       | 31 ++++++++++++
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c             | 56 +++++++++++++---------
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h             |  9 +++-
>  5 files changed, 93 insertions(+), 26 deletions(-)
> 

Re: [PATCH v2 4/5] ARM: dts: imx7: add GPMI NAND

[Stefan Agner]

On 2017-05-04 15:59, Han Xu wrote:
> On 05/04/2017 04:50 PM, Stefan Agner wrote:
>> On 2017-05-04 12:13, Han Xu wrote:
>>> On 04/21/2017 08:23 PM, Stefan Agner wrote:
>>>> Add i.MX 7 GPMI NAND module.
>>>>
>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>> ---
>>>>    arch/arm/boot/dts/imx7s.dtsi | 31 +++++++++++++++++++++++++++++++
>>>>    1 file changed, 31 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
>>>> index 843eb379e1ea..9645257638d4 100644
>>>> --- a/arch/arm/boot/dts/imx7s.dtsi
>>>> +++ b/arch/arm/boot/dts/imx7s.dtsi
>>>> @@ -995,5 +995,36 @@
>>>>    				status = "disabled";
>>>>    			};
>>>>    		};
>>>> +
>>>> +		dma_apbh: dma-apbh@33000000 {
>>>> +			compatible = "fsl,imx7d-dma-apbh", "fsl,imx28-dma-apbh";
>>>> +			reg = <0x33000000 0x2000>;
>>>> +			interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>>>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>>>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
>>>> +				     <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>;
>>>> +			interrupt-names = "gpmi0", "gpmi1", "gpmi2", "gpmi3";
>>>> +			#dma-cells = <1>;
>>>> +			dma-channels = <4>;
>>>> +			clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>>>> +				<&clks IMX7D_NAND_ROOT_CLK>;
>>>> +			clock-names = "dma_apbh_bch", "dma_apbh_io";
>>>> +		};
>>> Do you need some patches to enable all clks for APBH DMA? Refer to
>>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F551967%2F&data=01%7C01%7Chan.xu%40nxp.com%7C98493f7e475341dbf67008d49337b441%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0&sdata=FH9k%2FC4VcVCeyidkRgUJ5Ahu4%2B%2FlDAjXVwO0DY7XMhA%3D&reserved=0
>>>
>> Oh I see, with the current code and this device tree we actually only
>> enable the first clock... But it seems to work in practice...
>>
>> What is interesting, the chapter 5.2.5 System Clocks only describes
>> APBHDMA, and no GPMI module.
>>
>> Chapter 9.4 about APBH-Bridge-DMA bridge only specifies "hclk" as clock
>> sources.
>>
>> When looking at the table in chapter 5.2.5, hclk is
>> NAND_USDHC_BUS_CLK_ROOT
>>
>> Module    Syste Module Clock   Clock Root
>> APBHDMA   apbhdma.hclk         NAND_USDHC_BUS_CLK_ROOT
>>
>> So my guess is that the actual DMA controller should work fine with
>> IMX7D_NAND_USDHC_BUS_ROOT_CLK only. It is only the GPMI module which
>> needs the NAND_ROOT_CLK. My guess is since the APBHDMA controller is
>> only used for GPMI these days, it has been combined in one module in the
>> table in chapter 5.2.5.
>>
>> So we could actually only specify:
>>
>> clocks = <&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;
>>
>> Tested with this and NAND seems to work fine.
>>
>> Also tested accessing APBHDMA register in U-Boot with
>> IMX7D_NAND_ROOT_CLK disabled, it worked fine.
>>
>> What do you think? Should we go with this?
>>
>> This probably even saves power since GPMI's runtime clock management
>> disables IMX7D_NAND_ROOT_CLK when NAND is idle (according to the
>> nand_root_clk counter in clk_summary and the root clock register at
>> 0x3038AA00).
> 
> It also surprised me how it can work on your side, I remember we made 
> the code change for some purpose, please give me some time I can check 
> if the doc is wrong.
> 

Hi Han,

Do you have an update to this?

Otherwise, I will send out a version which only uses
IMX7D_NAND_USDHC_BUS_ROOT_CLK for now. This binding is already defined
and needs no driver change. We can still extend the binding (and
ddriver) when it turns out it is actually required.

--
Stefan

>> --
>> Stefan
>>
>>>> +
>>>> +		gpmi: gpmi-nand@33002000{
>>>> +			compatible = "fsl,imx7d-gpmi-nand";
>>>> +			#address-cells = <1>;
>>>> +			#size-cells = <1>;
>>>> +			reg = <0x33002000 0x2000>, <0x33004000 0x4000>;
>>>> +			reg-names = "gpmi-nand", "bch";
>>>> +			interrupts = <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
>>>> +			interrupt-names = "bch";
>>>> +			clocks = <&clks IMX7D_NAND_ROOT_CLK>,
>>>> +				<&clks IMX7D_NAND_USDHC_BUS_ROOT_CLK>;
>>>> +			clock-names = "gpmi_io", "gpmi_bch_apb";
>>>> +			dmas = <&dma_apbh 0>;
>>>> +			dma-names = "rx-tx";
>>>> +			status = "disabled";
>>>> +		};
>>>>    	};
>>>>    };