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

[PATCH 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

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             |  2 +
 5 files changed, 88 insertions(+), 24 deletions(-)

-- 
2.12.2

[PATCH 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>
---
 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..2e584e18d980 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;
+	int clks_count;
 };
 
 struct gpmi_nand_data {
-- 
2.12.2

[PATCH 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 6SoloX, but currently
none of this differences are in use so there is no detection needed
and the driver can reuse IS_MX6SX.

Signed-off-by: Stefan Agner <stefan@agner.ch>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index c8bbf5da2ab8..4a45d37ddc80 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_MX6SX,
+	.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);
-- 
2.12.2

[PATCH 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 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 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 1/5] mtd: nand: gpmi: unify clock handling

[Marek Vasut]

On 04/21/2017 03:07 AM, Stefan Agner wrote:
> 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>

[...]

> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> index 4e49a1f5fa27..2e584e18d980 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;
> +	int clks_count;

const int ? :)

Reviewed-by: Marek Vasut <marek.vasut@gmail.com>

-- 
Best regards,
Marek Vasut

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

[Marek Vasut]

On 04/21/2017 03:07 AM, Stefan Agner wrote:
> 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 6SoloX, but currently
> none of this differences are in use so there is no detection needed
> and the driver can reuse IS_MX6SX.
> 
> Signed-off-by: Stefan Agner <stefan@agner.ch>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index c8bbf5da2ab8..4a45d37ddc80 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_MX6SX,

Would it make sense to use IS_MX7 here already to prevent future surprises ?

> +	.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);
> 


-- 
Best regards,
Marek Vasut

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

[Marek Vasut]

On 04/21/2017 03:07 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.
> 
> --

The rest of the patchset is IMO OK too, but you probably want the DT
changes Acked by Shawn or Fabio.

-- 
Best regards,
Marek Vasut

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

[Stefan Agner]

On 2017-04-20 19:03, Marek Vasut wrote:
> On 04/21/2017 03:07 AM, Stefan Agner wrote:
>> 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 6SoloX, but currently
>> none of this differences are in use so there is no detection needed
>> and the driver can reuse IS_MX6SX.
>>
>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>> ---
>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> index c8bbf5da2ab8..4a45d37ddc80 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_MX6SX,
> 
> Would it make sense to use IS_MX7 here already to prevent future surprises ?
> 

Yeah I was thinking we can do it once we have an actual reason to
distinguish.

But then, adding the type would only require 2-3 lines of change if I
add it to the GPMI_IS_MX6 macro...

--
Stefan

>> +	.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);
>>

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

[Marek Vasut]

On 04/21/2017 05:15 AM, Stefan Agner wrote:
> On 2017-04-20 19:03, Marek Vasut wrote:
>> On 04/21/2017 03:07 AM, Stefan Agner wrote:
>>> 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 6SoloX, but currently
>>> none of this differences are in use so there is no detection needed
>>> and the driver can reuse IS_MX6SX.
>>>
>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>> ---
>>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++
>>>  1 file changed, 15 insertions(+)
>>>
>>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>>> index c8bbf5da2ab8..4a45d37ddc80 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_MX6SX,
>>
>> Would it make sense to use IS_MX7 here already to prevent future surprises ?
>>
> 
> Yeah I was thinking we can do it once we have an actual reason to
> distinguish.

So what are the differences anyway ?

> But then, adding the type would only require 2-3 lines of change if I
> add it to the GPMI_IS_MX6 macro...

Then at least add a comment because using type = IMX6SX right under
gpmi_data_mx7d can trigger some head-scratching. And put my R-B on V2.

-- 
Best regards,
Marek Vasut

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

[Stefan Agner]

On 2017-04-21 06:08, Marek Vasut wrote:
> On 04/21/2017 05:15 AM, Stefan Agner wrote:
>> On 2017-04-20 19:03, Marek Vasut wrote:
>>> On 04/21/2017 03:07 AM, Stefan Agner wrote:
>>>> 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 6SoloX, but currently
>>>> none of this differences are in use so there is no detection needed
>>>> and the driver can reuse IS_MX6SX.
>>>>
>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>> ---
>>>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++
>>>>  1 file changed, 15 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>>>> index c8bbf5da2ab8..4a45d37ddc80 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_MX6SX,
>>>
>>> Would it make sense to use IS_MX7 here already to prevent future surprises ?
>>>
>>
>> Yeah I was thinking we can do it once we have an actual reason to
>> distinguish.
> 
> So what are the differences anyway ?
> 

I did not check the details, but Han's patchset (link in cover letter)
mentions:
"add the HW bitflip detection and correction for i.MX6QP and i.MX7D."...

>> But then, adding the type would only require 2-3 lines of change if I
>> add it to the GPMI_IS_MX6 macro...
> 
> Then at least add a comment because using type = IMX6SX right under
> gpmi_data_mx7d can trigger some head-scratching. And put my R-B on V2.

FWIW, I mentioned it in the commit message.

I think rather then adding a comment it is cleaner to just add IS_IMX7D
and add it to the GPMI_IS_MX6 macro. That does not need a comment since
it implicitly says we have a i.MX 7 but treat it like i.MX 6 and it is a
rather small change. Does that sound acceptable?

--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -132,7 +132,7 @@ static const char * const gpmi_clks_for_mx7d[] = {
 };

 static const struct gpmi_devdata gpmi_devdata_imx7d = {
-       .type = IS_MX6SX,
+       .type = IS_MX7D,
        .bch_max_ecc_strength = 62,
        .max_chain_delay = 12,
        .clks = gpmi_clks_for_mx7d,
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index 2e584e18d980..f2cc13abc896 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

--
Stefan

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

[Marek Vasut]

On 04/21/2017 06:19 PM, Stefan Agner wrote:
> On 2017-04-21 06:08, Marek Vasut wrote:
>> On 04/21/2017 05:15 AM, Stefan Agner wrote:
>>> On 2017-04-20 19:03, Marek Vasut wrote:
>>>> On 04/21/2017 03:07 AM, Stefan Agner wrote:
>>>>> 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 6SoloX, but currently
>>>>> none of this differences are in use so there is no detection needed
>>>>> and the driver can reuse IS_MX6SX.
>>>>>
>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>> ---
>>>>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++
>>>>>  1 file changed, 15 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>>>>> index c8bbf5da2ab8..4a45d37ddc80 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_MX6SX,
>>>>
>>>> Would it make sense to use IS_MX7 here already to prevent future surprises ?
>>>>
>>>
>>> Yeah I was thinking we can do it once we have an actual reason to
>>> distinguish.
>>
>> So what are the differences anyway ?
>>
> 
> I did not check the details, but Han's patchset (link in cover letter)
> mentions:
> "add the HW bitflip detection and correction for i.MX6QP and i.MX7D."...

Oh, interesting.

>>> But then, adding the type would only require 2-3 lines of change if I
>>> add it to the GPMI_IS_MX6 macro...
>>
>> Then at least add a comment because using type = IMX6SX right under
>> gpmi_data_mx7d can trigger some head-scratching. And put my R-B on V2.
> 
> FWIW, I mentioned it in the commit message.
> 
> I think rather then adding a comment it is cleaner to just add IS_IMX7D
> and add it to the GPMI_IS_MX6 macro. That does not need a comment since
> it implicitly says we have a i.MX 7 but treat it like i.MX 6 and it is a
> rather small change. Does that sound acceptable?

Sure, that's even better, thanks.

btw isn't there some single-core mx7 (mx7s ?) , maybe we should just go
with mx7 (without the d suffix). I dunno if it has GPMI NAND though, so
maybe mx7d is the right thing to do here ...

> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -132,7 +132,7 @@ static const char * const gpmi_clks_for_mx7d[] = {
>  };
> 
>  static const struct gpmi_devdata gpmi_devdata_imx7d = {
> -       .type = IS_MX6SX,
> +       .type = IS_MX7D,
>         .bch_max_ecc_strength = 62,
>         .max_chain_delay = 12,
>         .clks = gpmi_clks_for_mx7d,
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> index 2e584e18d980..f2cc13abc896 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
> 
> --
> Stefan
> 


-- 
Best regards,
Marek Vasut

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

[Stefan Agner]

On 2017-04-21 10:22, Marek Vasut wrote:
> On 04/21/2017 06:19 PM, Stefan Agner wrote:
>> On 2017-04-21 06:08, Marek Vasut wrote:
>>> On 04/21/2017 05:15 AM, Stefan Agner wrote:
>>>> On 2017-04-20 19:03, Marek Vasut wrote:
>>>>> On 04/21/2017 03:07 AM, Stefan Agner wrote:
>>>>>> 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 6SoloX, but currently
>>>>>> none of this differences are in use so there is no detection needed
>>>>>> and the driver can reuse IS_MX6SX.
>>>>>>
>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>> ---
>>>>>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++
>>>>>>  1 file changed, 15 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>>>>>> index c8bbf5da2ab8..4a45d37ddc80 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_MX6SX,
>>>>>
>>>>> Would it make sense to use IS_MX7 here already to prevent future surprises ?
>>>>>
>>>>
>>>> Yeah I was thinking we can do it once we have an actual reason to
>>>> distinguish.
>>>
>>> So what are the differences anyway ?
>>>
>>
>> I did not check the details, but Han's patchset (link in cover letter)
>> mentions:
>> "add the HW bitflip detection and correction for i.MX6QP and i.MX7D."...
> 
> Oh, interesting.
> 
>>>> But then, adding the type would only require 2-3 lines of change if I
>>>> add it to the GPMI_IS_MX6 macro...
>>>
>>> Then at least add a comment because using type = IMX6SX right under
>>> gpmi_data_mx7d can trigger some head-scratching. And put my R-B on V2.
>>
>> FWIW, I mentioned it in the commit message.
>>
>> I think rather then adding a comment it is cleaner to just add IS_IMX7D
>> and add it to the GPMI_IS_MX6 macro. That does not need a comment since
>> it implicitly says we have a i.MX 7 but treat it like i.MX 6 and it is a
>> rather small change. Does that sound acceptable?
> 
> Sure, that's even better, thanks.
> 
> btw isn't there some single-core mx7 (mx7s ?) , maybe we should just go
> with mx7 (without the d suffix). I dunno if it has GPMI NAND though, so
> maybe mx7d is the right thing to do here ...
> 

There is a Solo version yes, and it has GPMI NAND too. However, almost
all i.MX 7 IPs have been named imx7d by NXP for some reason (including
compatible strings, see grep -r -e imx7 Documentation/), so I thought I
stay consistent here...

--
Stefan

>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> @@ -132,7 +132,7 @@ static const char * const gpmi_clks_for_mx7d[] = {
>>  };
>>
>>  static const struct gpmi_devdata gpmi_devdata_imx7d = {
>> -       .type = IS_MX6SX,
>> +       .type = IS_MX7D,
>>         .bch_max_ecc_strength = 62,
>>         .max_chain_delay = 12,
>>         .clks = gpmi_clks_for_mx7d,
>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
>> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
>> index 2e584e18d980..f2cc13abc896 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
>>
>> --
>> Stefan
>>

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

[Han Xu]

On Fri, Apr 21, 2017 at 12:38 PM, Stefan Agner <stefan@agner.ch> wrote:
> On 2017-04-21 10:22, Marek Vasut wrote:
>> On 04/21/2017 06:19 PM, Stefan Agner wrote:
>>> On 2017-04-21 06:08, Marek Vasut wrote:
>>>> On 04/21/2017 05:15 AM, Stefan Agner wrote:
>>>>> On 2017-04-20 19:03, Marek Vasut wrote:
>>>>>> On 04/21/2017 03:07 AM, Stefan Agner wrote:
>>>>>>> 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 6SoloX, but currently
>>>>>>> none of this differences are in use so there is no detection needed
>>>>>>> and the driver can reuse IS_MX6SX.
>>>>>>>
>>>>>>> Signed-off-by: Stefan Agner <stefan@agner.ch>
>>>>>>> ---
>>>>>>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 15 +++++++++++++++
>>>>>>>  1 file changed, 15 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>>>>>>> index c8bbf5da2ab8..4a45d37ddc80 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_MX6SX,
>>>>>>
>>>>>> Would it make sense to use IS_MX7 here already to prevent future surprises ?
>>>>>>
>>>>>
>>>>> Yeah I was thinking we can do it once we have an actual reason to
>>>>> distinguish.
>>>>
>>>> So what are the differences anyway ?
>>>>
>>>
>>> I did not check the details, but Han's patchset (link in cover letter)
>>> mentions:
>>> "add the HW bitflip detection and correction for i.MX6QP and i.MX7D."...
>>
>> Oh, interesting.
>>
>>>>> But then, adding the type would only require 2-3 lines of change if I
>>>>> add it to the GPMI_IS_MX6 macro...
>>>>
>>>> Then at least add a comment because using type = IMX6SX right under
>>>> gpmi_data_mx7d can trigger some head-scratching. And put my R-B on V2.
>>>
>>> FWIW, I mentioned it in the commit message.
>>>
>>> I think rather then adding a comment it is cleaner to just add IS_IMX7D
>>> and add it to the GPMI_IS_MX6 macro. That does not need a comment since
>>> it implicitly says we have a i.MX 7 but treat it like i.MX 6 and it is a
>>> rather small change. Does that sound acceptable?
>>
>> Sure, that's even better, thanks.
>>
>> btw isn't there some single-core mx7 (mx7s ?) , maybe we should just go
>> with mx7 (without the d suffix). I dunno if it has GPMI NAND though, so
>> maybe mx7d is the right thing to do here ...
>>
>
> There is a Solo version yes, and it has GPMI NAND too. However, almost
> all i.MX 7 IPs have been named imx7d by NXP for some reason (including
> compatible strings, see grep -r -e imx7 Documentation/), so I thought I
> stay consistent here...

Hi Guys,

Yes, there should be a i.MX7 Solo version with one core fused out. IMO, can
we use QUIRK to distinguish them rather than SoC name. I know I also sent
some patch set with SoC Name but I prefer to use QUIRK now.

>
> --
> Stefan
>
>>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>>> @@ -132,7 +132,7 @@ static const char * const gpmi_clks_for_mx7d[] = {
>>>  };
>>>
>>>  static const struct gpmi_devdata gpmi_devdata_imx7d = {
>>> -       .type = IS_MX6SX,
>>> +       .type = IS_MX7D,
>>>         .bch_max_ecc_strength = 62,
>>>         .max_chain_delay = 12,
>>>         .clks = gpmi_clks_for_mx7d,
>>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
>>> b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
>>> index 2e584e18d980..f2cc13abc896 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
>>>
>>> --
>>> Stefan
>>>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Sincerely,

Han XU

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

[Boris Brezillon]

Hi Han,

On Fri, 21 Apr 2017 13:29:16 -0500
Han Xu <xhnjupt@gmail.com> wrote:

> >>  
> >>>>> But then, adding the type would only require 2-3 lines of change if I
> >>>>> add it to the GPMI_IS_MX6 macro...  
> >>>>
> >>>> Then at least add a comment because using type = IMX6SX right under
> >>>> gpmi_data_mx7d can trigger some head-scratching. And put my R-B on V2.  
> >>>
> >>> FWIW, I mentioned it in the commit message.
> >>>
> >>> I think rather then adding a comment it is cleaner to just add IS_IMX7D
> >>> and add it to the GPMI_IS_MX6 macro. That does not need a comment since
> >>> it implicitly says we have a i.MX 7 but treat it like i.MX 6 and it is a
> >>> rather small change. Does that sound acceptable?  
> >>
> >> Sure, that's even better, thanks.
> >>
> >> btw isn't there some single-core mx7 (mx7s ?) , maybe we should just go
> >> with mx7 (without the d suffix). I dunno if it has GPMI NAND though, so
> >> maybe mx7d is the right thing to do here ...
> >>  
> >
> > There is a Solo version yes, and it has GPMI NAND too. However, almost
> > all i.MX 7 IPs have been named imx7d by NXP for some reason (including
> > compatible strings, see grep -r -e imx7 Documentation/), so I thought I
> > stay consistent here...  
> 
> Hi Guys,
> 
> Yes, there should be a i.MX7 Solo version with one core fused out. IMO, can
> we use QUIRK to distinguish them rather than SoC name. I know I also sent
> some patch set with SoC Name but I prefer to use QUIRK now.

Not sure what this means. Are you okay with Stefan's v2?

Regards,

Boris

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

[Marek Vasut]

On 05/02/2017 11:17 AM, Boris Brezillon wrote:
> Hi Han,
> 
> On Fri, 21 Apr 2017 13:29:16 -0500
> Han Xu <xhnjupt@gmail.com> wrote:
> 
>>>>  
>>>>>>> But then, adding the type would only require 2-3 lines of change if I
>>>>>>> add it to the GPMI_IS_MX6 macro...  
>>>>>>
>>>>>> Then at least add a comment because using type = IMX6SX right under
>>>>>> gpmi_data_mx7d can trigger some head-scratching. And put my R-B on V2.  
>>>>>
>>>>> FWIW, I mentioned it in the commit message.
>>>>>
>>>>> I think rather then adding a comment it is cleaner to just add IS_IMX7D
>>>>> and add it to the GPMI_IS_MX6 macro. That does not need a comment since
>>>>> it implicitly says we have a i.MX 7 but treat it like i.MX 6 and it is a
>>>>> rather small change. Does that sound acceptable?  
>>>>
>>>> Sure, that's even better, thanks.
>>>>
>>>> btw isn't there some single-core mx7 (mx7s ?) , maybe we should just go
>>>> with mx7 (without the d suffix). I dunno if it has GPMI NAND though, so
>>>> maybe mx7d is the right thing to do here ...
>>>>  
>>>
>>> There is a Solo version yes, and it has GPMI NAND too. However, almost
>>> all i.MX 7 IPs have been named imx7d by NXP for some reason (including
>>> compatible strings, see grep -r -e imx7 Documentation/), so I thought I
>>> stay consistent here...  

I missed the DT bit, sorry. the DT bindings say:
  - compatible : should be "fsl,<chip>-gpmi-nand"
so if FSL invented their own buggy bindings, they need to get them
through Rob :) IMO for MX7, this should be "imx7-gpmi-nand" , unless
there's some incentive to discern the solo/dual chips and/or there
is a future imx7 coming up with different GPMI NAND block version.

>> Hi Guys,
>>
>> Yes, there should be a i.MX7 Solo version with one core fused out. IMO, can
>> we use QUIRK to distinguish them rather than SoC name. I know I also sent
>> some patch set with SoC Name but I prefer to use QUIRK now.
> 
> Not sure what this means. Are you okay with Stefan's v2?

IMO the GPMI controller in solo and dual should be the same, so there's
no need to have quirks for it.

-- 
Best regards,
Marek Vasut

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

[Stefan Agner]

On 2017-05-02 04:18, Marek Vasut wrote:
> On 05/02/2017 11:17 AM, Boris Brezillon wrote:
>> Hi Han,
>>
>> On Fri, 21 Apr 2017 13:29:16 -0500
>> Han Xu <xhnjupt@gmail.com> wrote:
>>
>>>>>
>>>>>>>> But then, adding the type would only require 2-3 lines of change if I
>>>>>>>> add it to the GPMI_IS_MX6 macro...
>>>>>>>
>>>>>>> Then at least add a comment because using type = IMX6SX right under
>>>>>>> gpmi_data_mx7d can trigger some head-scratching. And put my R-B on V2.
>>>>>>
>>>>>> FWIW, I mentioned it in the commit message.
>>>>>>
>>>>>> I think rather then adding a comment it is cleaner to just add IS_IMX7D
>>>>>> and add it to the GPMI_IS_MX6 macro. That does not need a comment since
>>>>>> it implicitly says we have a i.MX 7 but treat it like i.MX 6 and it is a
>>>>>> rather small change. Does that sound acceptable?
>>>>>
>>>>> Sure, that's even better, thanks.
>>>>>
>>>>> btw isn't there some single-core mx7 (mx7s ?) , maybe we should just go
>>>>> with mx7 (without the d suffix). I dunno if it has GPMI NAND though, so
>>>>> maybe mx7d is the right thing to do here ...
>>>>>
>>>>
>>>> There is a Solo version yes, and it has GPMI NAND too. However, almost
>>>> all i.MX 7 IPs have been named imx7d by NXP for some reason (including
>>>> compatible strings, see grep -r -e imx7 Documentation/), so I thought I
>>>> stay consistent here...
> 
> I missed the DT bit, sorry. the DT bindings say:
>   - compatible : should be "fsl,<chip>-gpmi-nand"
> so if FSL invented their own buggy bindings, they need to get them
> through Rob :) IMO for MX7, this should be "imx7-gpmi-nand" , unless
> there's some incentive to discern the solo/dual chips and/or there
> is a future imx7 coming up with different GPMI NAND block version.
> 

There is no incentive to discern solo/dual, afaict this are the same
chips, only some IP's "fused away"... From GPMI NAND perspective, they
are definitely the same.

So that leaves us with "imx7-gpmi-nand" vs. "imx7d-gpmi-nand".

All device tree compatible strings as well as Kconfig symbol,
preprocessor defines and function prefixes have been named "imx7d" or
IMX7D so far... Although they work just fine for i.MX7 Solo too... Not
sure why that exactly ended up to be that way, but I guess the reason
was that NXP started to upstream with the dual...

For the sake of continuity, I would prefer if we stick to that (as my
current patchsets do)... It is like imx6q, which also works for dual...

$ grep -r -w compatible.*imx7d  arch/arm/boot/dts/*.dts* | wc -l
68
$ grep -r -w compatible.*imx7^d  arch/arm/boot/dts/*.dts* | wc -l
0


>>> Hi Guys,
>>>
>>> Yes, there should be a i.MX7 Solo version with one core fused out. IMO, can
>>> we use QUIRK to distinguish them rather than SoC name. I know I also sent
>>> some patch set with SoC Name but I prefer to use QUIRK now.
>>
>> Not sure what this means. Are you okay with Stefan's v2?
> 
> IMO the GPMI controller in solo and dual should be the same, so there's
> no need to have quirks for it.

Agreed.

--
Stefan