[PATCH 1/3] ARM: imx6q: add clocks for gpmi-nand

[PATCH 1/3] ARM: imx6q: add clocks for gpmi-nand

[Huang Shijie]

Add clocks for gpmi-nand.

Signed-off-by: Huang Shijie <shijie8@gmail.com>
Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 arch/arm/mach-imx/clk-imx6q.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
index 12d9040..f293bcd 100644
--- a/arch/arm/mach-imx/clk-imx6q.c
+++ b/arch/arm/mach-imx/clk-imx6q.c
@@ -151,7 +151,7 @@ enum mx6q_clks {
 	esai, gpt_ipg, gpt_ipg_per, gpu2d_core, gpu3d_core, hdmi_iahb,
 	hdmi_isfr, i2c1, i2c2, i2c3, iim, enfc, ipu1, ipu1_di0, ipu1_di1, ipu2,
 	ipu2_di0, ldb_di0, ldb_di1, ipu2_di1, hsi_tx, mlb, mmdc_ch0_axi,
-	mmdc_ch1_axi, ocram, openvg_axi, pcie_axi, pwm1, pwm2, pwm3, pwm4,
+	mmdc_ch1_axi, ocram, openvg_axi, pcie_axi, pwm1, pwm2, pwm3, pwm4, per1_bch,
 	gpmi_bch_apb, gpmi_bch, gpmi_io, gpmi_apb, sata, sdma, spba, ssi1,
 	ssi2, ssi3, uart_ipg, uart_serial, usboh3, usdhc1, usdhc2, usdhc3,
 	usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
@@ -357,6 +357,7 @@ int __init mx6q_clocks_init(void)
 	clk[ocram]        = imx_clk_gate2("ocram",         "ahb",               base + 0x74, 28);
 	clk[openvg_axi]   = imx_clk_gate2("openvg_axi",    "axi",               base + 0x74, 30);
 	clk[pcie_axi]     = imx_clk_gate2("pcie_axi",      "pcie_axi_sel",      base + 0x78, 0);
+	clk[per1_bch]     = imx_clk_gate2("per1_bch",      "usdhc3",            base + 0x78, 12);
 	clk[pwm1]         = imx_clk_gate2("pwm1",          "ipg_per",           base + 0x78, 16);
 	clk[pwm2]         = imx_clk_gate2("pwm2",          "ipg_per",           base + 0x78, 18);
 	clk[pwm3]         = imx_clk_gate2("pwm3",          "ipg_per",           base + 0x78, 20);
@@ -394,6 +395,11 @@ int __init mx6q_clocks_init(void)
 	clk_register_clkdev(clk[twd], NULL, "smp_twd");
 	clk_register_clkdev(clk[usboh3], NULL, "usboh3");
 	clk_register_clkdev(clk[apbh_dma], NULL, "110000.dma-apbh");
+	clk_register_clkdev(clk[per1_bch], "per1_bch", NULL);
+	clk_register_clkdev(clk[gpmi_bch_apb], "gpmi_bch_apb", NULL);
+	clk_register_clkdev(clk[gpmi_bch], "gpmi_bch", NULL);
+	clk_register_clkdev(clk[gpmi_apb], "gpmi_apb", NULL);
+	clk_register_clkdev(clk[gpmi_io], NULL, "112000.gpmi-nand");
 	clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
 	clk_register_clkdev(clk[uart_ipg], "ipg", "2020000.serial");
 	clk_register_clkdev(clk[uart_serial], "per", "21e8000.serial");
-- 
1.7.4.4

[PATCH 1/3] ARM: imx6q: add clocks for gpmi-nand

[Shawn Guo]

On Thu, Jun 28, 2012 at 11:52:03PM -0400, Huang Shijie wrote:
> Add clocks for gpmi-nand.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>
> Signed-off-by: Huang Shijie <b32955@freescale.com>

Why double sign off?

> ---
>  arch/arm/mach-imx/clk-imx6q.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
> index 12d9040..f293bcd 100644
> --- a/arch/arm/mach-imx/clk-imx6q.c
> +++ b/arch/arm/mach-imx/clk-imx6q.c
> @@ -151,7 +151,7 @@ enum mx6q_clks {
>  	esai, gpt_ipg, gpt_ipg_per, gpu2d_core, gpu3d_core, hdmi_iahb,
>  	hdmi_isfr, i2c1, i2c2, i2c3, iim, enfc, ipu1, ipu1_di0, ipu1_di1, ipu2,
>  	ipu2_di0, ldb_di0, ldb_di1, ipu2_di1, hsi_tx, mlb, mmdc_ch0_axi,
> -	mmdc_ch1_axi, ocram, openvg_axi, pcie_axi, pwm1, pwm2, pwm3, pwm4,
> +	mmdc_ch1_axi, ocram, openvg_axi, pcie_axi, pwm1, pwm2, pwm3, pwm4, per1_bch,
>  	gpmi_bch_apb, gpmi_bch, gpmi_io, gpmi_apb, sata, sdma, spba, ssi1,
>  	ssi2, ssi3, uart_ipg, uart_serial, usboh3, usdhc1, usdhc2, usdhc3,
>  	usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
> @@ -357,6 +357,7 @@ int __init mx6q_clocks_init(void)
>  	clk[ocram]        = imx_clk_gate2("ocram",         "ahb",               base + 0x74, 28);
>  	clk[openvg_axi]   = imx_clk_gate2("openvg_axi",    "axi",               base + 0x74, 30);
>  	clk[pcie_axi]     = imx_clk_gate2("pcie_axi",      "pcie_axi_sel",      base + 0x78, 0);
> +	clk[per1_bch]     = imx_clk_gate2("per1_bch",      "usdhc3",            base + 0x78, 12);
>  	clk[pwm1]         = imx_clk_gate2("pwm1",          "ipg_per",           base + 0x78, 16);
>  	clk[pwm2]         = imx_clk_gate2("pwm2",          "ipg_per",           base + 0x78, 18);
>  	clk[pwm3]         = imx_clk_gate2("pwm3",          "ipg_per",           base + 0x78, 20);
> @@ -394,6 +395,11 @@ int __init mx6q_clocks_init(void)
>  	clk_register_clkdev(clk[twd], NULL, "smp_twd");
>  	clk_register_clkdev(clk[usboh3], NULL, "usboh3");
>  	clk_register_clkdev(clk[apbh_dma], NULL, "110000.dma-apbh");
> +	clk_register_clkdev(clk[per1_bch], "per1_bch", NULL);
> +	clk_register_clkdev(clk[gpmi_bch_apb], "gpmi_bch_apb", NULL);
> +	clk_register_clkdev(clk[gpmi_bch], "gpmi_bch", NULL);
> +	clk_register_clkdev(clk[gpmi_apb], "gpmi_apb", NULL);

These do not look like right.  The dev_id should not be NULL if they
are for device driver to look up.

> +	clk_register_clkdev(clk[gpmi_io], NULL, "112000.gpmi-nand");
>  	clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
>  	clk_register_clkdev(clk[uart_ipg], "ipg", "2020000.serial");
>  	clk_register_clkdev(clk[uart_serial], "per", "21e8000.serial");
> -- 
> 1.7.4.4
> 

-- 
Regards,
Shawn

[PATCH 1/3] ARM: imx6q: add clocks for gpmi-nand

[Dong Aisheng]

On Thu, Jun 28, 2012 at 11:52:03PM -0400, Huang Shijie wrote:
> Add clocks for gpmi-nand.
> 
> Signed-off-by: Huang Shijie <shijie8@gmail.com>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  arch/arm/mach-imx/clk-imx6q.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/mach-imx/clk-imx6q.c b/arch/arm/mach-imx/clk-imx6q.c
> index 12d9040..f293bcd 100644
> --- a/arch/arm/mach-imx/clk-imx6q.c
> +++ b/arch/arm/mach-imx/clk-imx6q.c
> @@ -151,7 +151,7 @@ enum mx6q_clks {
>  	esai, gpt_ipg, gpt_ipg_per, gpu2d_core, gpu3d_core, hdmi_iahb,
>  	hdmi_isfr, i2c1, i2c2, i2c3, iim, enfc, ipu1, ipu1_di0, ipu1_di1, ipu2,
>  	ipu2_di0, ldb_di0, ldb_di1, ipu2_di1, hsi_tx, mlb, mmdc_ch0_axi,
> -	mmdc_ch1_axi, ocram, openvg_axi, pcie_axi, pwm1, pwm2, pwm3, pwm4,
> +	mmdc_ch1_axi, ocram, openvg_axi, pcie_axi, pwm1, pwm2, pwm3, pwm4, per1_bch,
>  	gpmi_bch_apb, gpmi_bch, gpmi_io, gpmi_apb, sata, sdma, spba, ssi1,
>  	ssi2, ssi3, uart_ipg, uart_serial, usboh3, usdhc1, usdhc2, usdhc3,
>  	usdhc4, vdo_axi, vpu_axi, cko1, pll1_sys, pll2_bus, pll3_usb_otg,
> @@ -357,6 +357,7 @@ int __init mx6q_clocks_init(void)
>  	clk[ocram]        = imx_clk_gate2("ocram",         "ahb",               base + 0x74, 28);
>  	clk[openvg_axi]   = imx_clk_gate2("openvg_axi",    "axi",               base + 0x74, 30);
>  	clk[pcie_axi]     = imx_clk_gate2("pcie_axi",      "pcie_axi_sel",      base + 0x78, 0);
> +	clk[per1_bch]     = imx_clk_gate2("per1_bch",      "usdhc3",            base + 0x78, 12);
>  	clk[pwm1]         = imx_clk_gate2("pwm1",          "ipg_per",           base + 0x78, 16);
>  	clk[pwm2]         = imx_clk_gate2("pwm2",          "ipg_per",           base + 0x78, 18);
>  	clk[pwm3]         = imx_clk_gate2("pwm3",          "ipg_per",           base + 0x78, 20);
> @@ -394,6 +395,11 @@ int __init mx6q_clocks_init(void)
>  	clk_register_clkdev(clk[twd], NULL, "smp_twd");
>  	clk_register_clkdev(clk[usboh3], NULL, "usboh3");
>  	clk_register_clkdev(clk[apbh_dma], NULL, "110000.dma-apbh");
> +	clk_register_clkdev(clk[per1_bch], "per1_bch", NULL);
...
> +	clk_register_clkdev(clk[gpmi_bch_apb], "gpmi_bch_apb", NULL);
> +	clk_register_clkdev(clk[gpmi_bch], "gpmi_bch", NULL);
> +	clk_register_clkdev(clk[gpmi_apb], "gpmi_apb", NULL);
Are above three clocks gpmi private?
If yes, you'd better specify a dev_id for it.

> +	clk_register_clkdev(clk[gpmi_io], NULL, "112000.gpmi-nand");
We had several gpmi clocks here, so i guess it would be good if we have
the con_id set here too to distinguish what's this clock for.

>  	clk_register_clkdev(clk[uart_serial], "per", "2020000.serial");
>  	clk_register_clkdev(clk[uart_ipg], "ipg", "2020000.serial");
>  	clk_register_clkdev(clk[uart_serial], "per", "21e8000.serial");
> -- 
> 1.7.4.4

Regards
Dong Aisheng

[PATCH 1/3] ARM: imx6q: add clocks for gpmi-nand

[Huang Shijie]

? 2012?06?29? 10:29, Dong Aisheng ??:
> We had several gpmi clocks here, so i guess it would be good if we have
> the con_id set here too to distinguish what's this clock for.
>
thanks.
But this will cause the mx28 can not find the clock.


Best Regards
Huang Shijie

[PATCH 1/3] ARM: imx6q: add clocks for gpmi-nand

[Shawn Guo]

On Fri, Jun 29, 2012 at 10:57:26AM +0800, Huang Shijie wrote:
> ? 2012?06?29? 10:29, Dong Aisheng ??:
> >We had several gpmi clocks here, so i guess it would be good if we have
> >the con_id set here too to distinguish what's this clock for.
> >
> thanks.
> But this will cause the mx28 can not find the clock.
> 
Hmm, it should not, as long as the con_id in imx28 gpmi-nand lookup
is NULL, which is a wildcard for matching.

-- 
Regards,
Shawn

[PATCH 2/3] ARM: imx6q: add DT node for gpmi nand

[Huang Shijie]

Add the DT node for gpmi nand.
Add the pinmux support for gpmi nand.

The gpmi nand may conflicts with other modules, such as MMC.
So we do not enable the gpmi nand for mx6q-arm2 board, just add the
node for the board.

Signed-off-by: Huang Shijie <b32955@freescale.com>
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 arch/arm/boot/dts/imx6q-arm2.dts |    6 ++++++
 arch/arm/boot/dts/imx6q.dtsi     |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/imx6q-arm2.dts b/arch/arm/boot/dts/imx6q-arm2.dts
index bdab44c..d93a2d9 100644
--- a/arch/arm/boot/dts/imx6q-arm2.dts
+++ b/arch/arm/boot/dts/imx6q-arm2.dts
@@ -22,6 +22,12 @@
 	};
 
 	soc {
+		gpmi-nand at 00112000 {
+			pinctrl-names = "default";
+			pinctrl-0 = <&pinctrl_gpmi_nand_1>;
+			status = "disabled";
+		};
+
 		aips-bus at 02100000 { /* AIPS2 */
 			ethernet at 02188000 {
 				phy-mode = "rgmii";
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi
index 3197744..16a3884 100644
--- a/arch/arm/boot/dts/imx6q.dtsi
+++ b/arch/arm/boot/dts/imx6q.dtsi
@@ -92,6 +92,18 @@
 			reg = <0x00110000 0x2000>;
 		};
 
+		gpmi-nand at 00112000 {
+		       compatible = "fsl,imx6q-gpmi-nand";
+		       #address-cells = <1>;
+		       #size-cells = <1>;
+		       reg = <0x00112000 0x2000>, <0x00114000 0x2000>;
+		       reg-names = "gpmi-nand", "bch";
+		       interrupts = <0 13 0x04>, <0 15 0x04>;
+		       interrupt-names = "gpmi-dma", "bch";
+		       fsl,gpmi-dma-channel = <0>;
+		       status = "disabled";
+		};
+
 		timer at 00a00600 {
 			compatible = "arm,cortex-a9-twd-timer";
 			reg = <0x00a00600 0x20>;
@@ -500,6 +512,30 @@
 					};
 				};
 
+				gpmi-nand {
+					pinctrl_gpmi_nand_1: gpmi-nand-1 {
+						fsl,pins = <1328 0xb0b1		/* MX6Q_PAD_NANDF_CLE__RAWNAND_CLE */
+							    1336 0xb0b1		/* MX6Q_PAD_NANDF_ALE__RAWNAND_ALE */
+							    1344 0xb0b1		/* MX6Q_PAD_NANDF_WP_B__RAWNAND_RESETN */
+							    1352 0xb000		/* MX6Q_PAD_NANDF_RB0__RAWNAND_READY0 */
+							    1360 0xb0b1		/* MX6Q_PAD_NANDF_CS0__RAWNAND_CE0N */
+							    1365 0xb0b1		/* MX6Q_PAD_NANDF_CS1__RAWNAND_CE1N */
+							    1371 0xb0b1		/* MX6Q_PAD_NANDF_CS2__RAWNAND_CE2N */
+							    1378 0xb0b1		/* MX6Q_PAD_NANDF_CS3__RAWNAND_CE3N */
+							    1387 0xb0b1		/* MX6Q_PAD_SD4_CMD__RAWNAND_RDN */
+							    1393 0xb0b1		/* MX6Q_PAD_SD4_CLK__RAWNAND_WRN */
+							    1397 0xb0b1		/* MX6Q_PAD_NANDF_D0__RAWNAND_D0 */
+							    1405 0xb0b1		/* MX6Q_PAD_NANDF_D1__RAWNAND_D1 */
+							    1413 0xb0b1		/* MX6Q_PAD_NANDF_D2__RAWNAND_D2 */
+							    1421 0xb0b1		/* MX6Q_PAD_NANDF_D3__RAWNAND_D3 */
+							    1429 0xb0b1		/* MX6Q_PAD_NANDF_D4__RAWNAND_D4 */
+							    1437 0xb0b1		/* MX6Q_PAD_NANDF_D5__RAWNAND_D5 */
+							    1445 0xb0b1		/* MX6Q_PAD_NANDF_D6__RAWNAND_D6 */
+							    1453 0xb0b1		/* MX6Q_PAD_NANDF_D7__RAWNAND_D7 */
+							    1463 0x00b1>;	/* MX6Q_PAD_SD4_DAT0__RAWNAND_DQS */
+					};
+				};
+
 				i2c1 {
 					pinctrl_i2c1_1: i2c1grp-1 {
 						fsl,pins = <137 0x4001b8b1	/* MX6Q_PAD_EIM_D21__I2C1_SCL */
-- 
1.7.4.4

[PATCH 2/3] ARM: imx6q: add DT node for gpmi nand

[Dong Aisheng]

On Thu, Jun 28, 2012 at 11:52:04PM -0400, Huang Shijie wrote:
> Add the DT node for gpmi nand.
> Add the pinmux support for gpmi nand.
> 
> The gpmi nand may conflicts with other modules, such as MMC.
> So we do not enable the gpmi nand for mx6q-arm2 board, just add the
> node for the board.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> Signed-off-by: Huang Shijie <shijie8@gmail.com>

Just one minor suggestion, see below.
Otherwise,
Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Regards
Dong Aisheng

> ---
>  arch/arm/boot/dts/imx6q-arm2.dts |    6 ++++++
>  arch/arm/boot/dts/imx6q.dtsi     |   36 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/imx6q-arm2.dts b/arch/arm/boot/dts/imx6q-arm2.dts
> index bdab44c..d93a2d9 100644
> --- a/arch/arm/boot/dts/imx6q-arm2.dts
> +++ b/arch/arm/boot/dts/imx6q-arm2.dts
> @@ -22,6 +22,12 @@
>  	};
>  
>  	soc {

> +		gpmi-nand at 00112000 {
> +			pinctrl-names = "default";
> +			pinctrl-0 = <&pinctrl_gpmi_nand_1>;
> +			status = "disabled";
It would be better if we add some comment for gpmi pin conflict here.
Easy for understand.

> +		};
> +
>  		aips-bus at 02100000 { /* AIPS2 */
>  			ethernet at 02188000 {
>  				phy-mode = "rgmii";

Regards
Dong Aisheng

[PATCH 2/3] ARM: imx6q: add DT node for gpmi nand

[Huang Shijie]

? 2012?06?29? 11:10, Dong Aisheng ??:
> It would be better if we add some comment for gpmi pin conflict here.
> Easy for understand.
ok, I will add it in the next version.

thanks
Huang Shijie

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Huang Shijie]

From: Huang Shijie <b32955@freescale.com>

The gpmi nand driver may needs several clocks(MX6Q needs five clocks).

In the old clock framework, all these clocks are chained together,
all you need is to manipulate the first clock.

But the kernel uses the common clk framework now, which forces us to
get the clocks one by one. When we use them, we have to enable them
one by one too.

Signed-off-by: Huang Shijie <b32955@freescale.com>
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |   43 ++++++++++++++++++----
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   63 +++++++++++++++++++++++++++-----
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    3 +-
 3 files changed, 91 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index a1f4332..c3778c0 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -124,12 +124,40 @@ error:
 	return -ETIMEDOUT;
 }
 
+static int __gpmi_enable_clk(struct gpmi_nand_data *this, bool v)
+{
+	struct clk *clk;
+	int ret;
+	int i;
+
+	for (i = 0; i < GPMI_CLK_MAX; i++) {
+		clk = this->resources.clock[i];
+		if (!clk)
+			break;
+
+		if (v) {
+			ret = clk_prepare_enable(clk);
+			if (ret)
+				goto err_clk;
+		} else {
+			clk_disable_unprepare(clk);
+		}
+	}
+	return 0;
+
+err_clk:
+	return ret;
+}
+
+#define gpmi_enable_clk(x) __gpmi_enable_clk(x, true)
+#define gpmi_disable_clk(x) __gpmi_enable_clk(x, false)
+
 int gpmi_init(struct gpmi_nand_data *this)
 {
 	struct resources *r = &this->resources;
 	int ret;
 
-	ret = clk_prepare_enable(r->clock);
+	ret = gpmi_enable_clk(this);
 	if (ret)
 		goto err_out;
 	ret = gpmi_reset_block(r->gpmi_regs, false);
@@ -149,7 +177,7 @@ int gpmi_init(struct gpmi_nand_data *this)
 	/* Select BCH ECC. */
 	writel(BM_GPMI_CTRL1_BCH_MODE, r->gpmi_regs + HW_GPMI_CTRL1_SET);
 
-	clk_disable_unprepare(r->clock);
+	gpmi_disable_clk(this);
 	return 0;
 err_out:
 	return ret;
@@ -205,7 +233,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
 	ecc_strength  = bch_geo->ecc_strength >> 1;
 	page_size     = bch_geo->page_size;
 
-	ret = clk_prepare_enable(r->clock);
+	ret = gpmi_enable_clk(this);
 	if (ret)
 		goto err_out;
 
@@ -240,7 +268,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
 	writel(BM_BCH_CTRL_COMPLETE_IRQ_EN,
 				r->bch_regs + HW_BCH_CTRL_SET);
 
-	clk_disable_unprepare(r->clock);
+	gpmi_disable_clk(this);
 	return 0;
 err_out:
 	return ret;
@@ -716,7 +744,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
 	int ret;
 
 	/* Enable the clock. */
-	ret = clk_prepare_enable(r->clock);
+	ret = gpmi_enable_clk(this);
 	if (ret) {
 		pr_err("We failed in enable the clk\n");
 		goto err_out;
@@ -727,7 +755,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
 		gpmi_regs + HW_GPMI_TIMING1);
 
 	/* Get the timing information we need. */
-	nfc->clock_frequency_in_hz = clk_get_rate(r->clock);
+	nfc->clock_frequency_in_hz = clk_get_rate(r->clock[0]);
 	clock_period_in_ns = 1000000000 / nfc->clock_frequency_in_hz;
 
 	gpmi_nfc_compute_hardware_timing(this, &hw);
@@ -784,8 +812,7 @@ err_out:
 
 void gpmi_end(struct gpmi_nand_data *this)
 {
-	struct resources *r = &this->resources;
-	clk_disable_unprepare(r->clock);
+	gpmi_disable_clk(this);
 }
 
 /* Clears a BCH interrupt. */
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 941cfb7..edda3b1 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -464,9 +464,59 @@ acquire_err:
 	return -EINVAL;
 }
 
+static void gpmi_put_clks(struct gpmi_nand_data *this)
+{
+	struct resources *r = &this->resources;
+	struct clk *clk;
+	int i;
+
+	for (i = 0; i < GPMI_CLK_MAX; i++) {
+		clk = r->clock[i];
+		if (clk) {
+			clk_put(clk);
+			r->clock[i] = NULL;
+		}
+	}
+}
+
+static char *extra_clks_for_mx6q[] = {
+	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
+};
+
+static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
+{
+	struct resources *r = &this->resources;
+	char **extra_clks = NULL;
+	struct clk *clk;
+	int i;
+
+	r->clock[0] = clk_get(&this->pdev->dev, NULL);
+	if (IS_ERR(r->clock[0]))
+		goto err_clock;
+
+	/* Get extra clocks */
+	if (GPMI_IS_MX6Q(this))
+		extra_clks = extra_clks_for_mx6q;
+	if (!extra_clks)
+		return 0;
+
+	for (i = 1; i < GPMI_CLK_MAX; i++) {
+		clk = clk_get(NULL, extra_clks[i - 1]);
+		if (IS_ERR(clk))
+			goto err_clock;
+
+		r->clock[i] = clk;
+	}
+	return 0;
+
+err_clock:
+	pr_err("failed in finding the clocks.\n");
+	gpmi_put_clks(this);
+	return -ENOMEM;
+}
+
 static int __devinit acquire_resources(struct gpmi_nand_data *this)
 {
-	struct resources *res = &this->resources;
 	struct pinctrl *pinctrl;
 	int ret;
 
@@ -492,12 +542,9 @@ static int __devinit acquire_resources(struct gpmi_nand_data *this)
 		goto exit_pin;
 	}
 
-	res->clock = clk_get(&this->pdev->dev, NULL);
-	if (IS_ERR(res->clock)) {
-		pr_err("can not get the clock\n");
-		ret = -ENOENT;
+	ret = gpmi_get_clks(this);
+	if (ret)
 		goto exit_clock;
-	}
 	return 0;
 
 exit_clock:
@@ -512,9 +559,7 @@ exit_regs:
 
 static void release_resources(struct gpmi_nand_data *this)
 {
-	struct resources *r = &this->resources;
-
-	clk_put(r->clock);
+	gpmi_put_clks(this);
 	release_register_block(this);
 	release_bch_irq(this);
 	release_dma_channels(this);
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index ce5daa1..1547a60 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -22,6 +22,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/fsl/mxs-dma.h>
 
+#define GPMI_CLK_MAX 5 /* MX6Q needs five clocks */
 struct resources {
 	void          *gpmi_regs;
 	void          *bch_regs;
@@ -29,7 +30,7 @@ struct resources {
 	unsigned int  bch_high_interrupt;
 	unsigned int  dma_low_channel;
 	unsigned int  dma_high_channel;
-	struct clk    *clock;
+	struct clk    *clock[GPMI_CLK_MAX];
 };
 
 /**
-- 
1.7.4.4

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Shawn Guo]

On Thu, Jun 28, 2012 at 11:52:05PM -0400, Huang Shijie wrote:
> From: Huang Shijie <b32955@freescale.com>
> 
> The gpmi nand driver may needs several clocks(MX6Q needs five clocks).
> 
> In the old clock framework, all these clocks are chained together,
> all you need is to manipulate the first clock.
> 
> But the kernel uses the common clk framework now, which forces us to
> get the clocks one by one. When we use them, we have to enable them
> one by one too.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> Signed-off-by: Huang Shijie <shijie8@gmail.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |   43 ++++++++++++++++++----
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   63 +++++++++++++++++++++++++++-----
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    3 +-
>  3 files changed, 91 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> index a1f4332..c3778c0 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> @@ -124,12 +124,40 @@ error:
>  	return -ETIMEDOUT;
>  }
>  
> +static int __gpmi_enable_clk(struct gpmi_nand_data *this, bool v)
> +{
> +	struct clk *clk;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < GPMI_CLK_MAX; i++) {
> +		clk = this->resources.clock[i];
> +		if (!clk)
> +			break;
> +
> +		if (v) {
> +			ret = clk_prepare_enable(clk);
> +			if (ret)
> +				goto err_clk;
> +		} else {
> +			clk_disable_unprepare(clk);
> +		}
> +	}
> +	return 0;
> +
> +err_clk:
> +	return ret;
> +}
> +
> +#define gpmi_enable_clk(x) __gpmi_enable_clk(x, true)
> +#define gpmi_disable_clk(x) __gpmi_enable_clk(x, false)
> +
>  int gpmi_init(struct gpmi_nand_data *this)
>  {
>  	struct resources *r = &this->resources;
>  	int ret;
>  
> -	ret = clk_prepare_enable(r->clock);
> +	ret = gpmi_enable_clk(this);
>  	if (ret)
>  		goto err_out;
>  	ret = gpmi_reset_block(r->gpmi_regs, false);
> @@ -149,7 +177,7 @@ int gpmi_init(struct gpmi_nand_data *this)
>  	/* Select BCH ECC. */
>  	writel(BM_GPMI_CTRL1_BCH_MODE, r->gpmi_regs + HW_GPMI_CTRL1_SET);
>  
> -	clk_disable_unprepare(r->clock);
> +	gpmi_disable_clk(this);
>  	return 0;
>  err_out:
>  	return ret;
> @@ -205,7 +233,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
>  	ecc_strength  = bch_geo->ecc_strength >> 1;
>  	page_size     = bch_geo->page_size;
>  
> -	ret = clk_prepare_enable(r->clock);
> +	ret = gpmi_enable_clk(this);
>  	if (ret)
>  		goto err_out;
>  
> @@ -240,7 +268,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
>  	writel(BM_BCH_CTRL_COMPLETE_IRQ_EN,
>  				r->bch_regs + HW_BCH_CTRL_SET);
>  
> -	clk_disable_unprepare(r->clock);
> +	gpmi_disable_clk(this);
>  	return 0;
>  err_out:
>  	return ret;
> @@ -716,7 +744,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
>  	int ret;
>  
>  	/* Enable the clock. */
> -	ret = clk_prepare_enable(r->clock);
> +	ret = gpmi_enable_clk(this);
>  	if (ret) {
>  		pr_err("We failed in enable the clk\n");
>  		goto err_out;
> @@ -727,7 +755,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
>  		gpmi_regs + HW_GPMI_TIMING1);
>  
>  	/* Get the timing information we need. */
> -	nfc->clock_frequency_in_hz = clk_get_rate(r->clock);
> +	nfc->clock_frequency_in_hz = clk_get_rate(r->clock[0]);
>  	clock_period_in_ns = 1000000000 / nfc->clock_frequency_in_hz;
>  
>  	gpmi_nfc_compute_hardware_timing(this, &hw);
> @@ -784,8 +812,7 @@ err_out:
>  
>  void gpmi_end(struct gpmi_nand_data *this)
>  {
> -	struct resources *r = &this->resources;
> -	clk_disable_unprepare(r->clock);
> +	gpmi_disable_clk(this);
>  }
>  
>  /* Clears a BCH interrupt. */
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 941cfb7..edda3b1 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -464,9 +464,59 @@ acquire_err:
>  	return -EINVAL;
>  }
>  
> +static void gpmi_put_clks(struct gpmi_nand_data *this)
> +{
> +	struct resources *r = &this->resources;
> +	struct clk *clk;
> +	int i;
> +
> +	for (i = 0; i < GPMI_CLK_MAX; i++) {
> +		clk = r->clock[i];
> +		if (clk) {
> +			clk_put(clk);
> +			r->clock[i] = NULL;
> +		}
> +	}
> +}
> +
> +static char *extra_clks_for_mx6q[] = {
> +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
> +};
> +
> +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
> +{
> +	struct resources *r = &this->resources;
> +	char **extra_clks = NULL;
> +	struct clk *clk;
> +	int i;
> +
> +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
> +	if (IS_ERR(r->clock[0]))
> +		goto err_clock;
> +
> +	/* Get extra clocks */
> +	if (GPMI_IS_MX6Q(this))
> +		extra_clks = extra_clks_for_mx6q;

We probably do not need this tweaking.  We can have the driver always
take all those 5 clocks, and I think the current imx28 clock driver
can just work with it, because the gpmi-nand clkdev lookup has NULL
con_id and all those 5 clocks can match the same one on imx28.

> +	if (!extra_clks)
> +		return 0;
> +
> +	for (i = 1; i < GPMI_CLK_MAX; i++) {
> +		clk = clk_get(NULL, extra_clks[i - 1]);

You should not put NULL but &this->pdev->dev here.

Regards,
Shawn

> +		if (IS_ERR(clk))
> +			goto err_clock;
> +
> +		r->clock[i] = clk;
> +	}
> +	return 0;
> +
> +err_clock:
> +	pr_err("failed in finding the clocks.\n");
> +	gpmi_put_clks(this);
> +	return -ENOMEM;
> +}
> +
>  static int __devinit acquire_resources(struct gpmi_nand_data *this)
>  {
> -	struct resources *res = &this->resources;
>  	struct pinctrl *pinctrl;
>  	int ret;
>  
> @@ -492,12 +542,9 @@ static int __devinit acquire_resources(struct gpmi_nand_data *this)
>  		goto exit_pin;
>  	}
>  
> -	res->clock = clk_get(&this->pdev->dev, NULL);
> -	if (IS_ERR(res->clock)) {
> -		pr_err("can not get the clock\n");
> -		ret = -ENOENT;
> +	ret = gpmi_get_clks(this);
> +	if (ret)
>  		goto exit_clock;
> -	}
>  	return 0;
>  
>  exit_clock:
> @@ -512,9 +559,7 @@ exit_regs:
>  
>  static void release_resources(struct gpmi_nand_data *this)
>  {
> -	struct resources *r = &this->resources;
> -
> -	clk_put(r->clock);
> +	gpmi_put_clks(this);
>  	release_register_block(this);
>  	release_bch_irq(this);
>  	release_dma_channels(this);
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> index ce5daa1..1547a60 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> @@ -22,6 +22,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/fsl/mxs-dma.h>
>  
> +#define GPMI_CLK_MAX 5 /* MX6Q needs five clocks */
>  struct resources {
>  	void          *gpmi_regs;
>  	void          *bch_regs;
> @@ -29,7 +30,7 @@ struct resources {
>  	unsigned int  bch_high_interrupt;
>  	unsigned int  dma_low_channel;
>  	unsigned int  dma_high_channel;
> -	struct clk    *clock;
> +	struct clk    *clock[GPMI_CLK_MAX];
>  };
>  
>  /**
> -- 
> 1.7.4.4
> 

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Huang Shijie]

? 2012?06?29? 10:06, Shawn Guo ??:
> On Thu, Jun 28, 2012 at 11:52:05PM -0400, Huang Shijie wrote:
>> From: Huang Shijie<b32955@freescale.com>
>>
>> The gpmi nand driver may needs several clocks(MX6Q needs five clocks).
>>
>> In the old clock framework, all these clocks are chained together,
>> all you need is to manipulate the first clock.
>>
>> But the kernel uses the common clk framework now, which forces us to
>> get the clocks one by one. When we use them, we have to enable them
>> one by one too.
>>
>> Signed-off-by: Huang Shijie<b32955@freescale.com>
>> Signed-off-by: Huang Shijie<shijie8@gmail.com>
>> ---
>>   drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |   43 ++++++++++++++++++----
>>   drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   63 +++++++++++++++++++++++++++-----
>>   drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    3 +-
>>   3 files changed, 91 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
>> index a1f4332..c3778c0 100644
>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
>> @@ -124,12 +124,40 @@ error:
>>   	return -ETIMEDOUT;
>>   }
>>
>> +static int __gpmi_enable_clk(struct gpmi_nand_data *this, bool v)
>> +{
>> +	struct clk *clk;
>> +	int ret;
>> +	int i;
>> +
>> +	for (i = 0; i<  GPMI_CLK_MAX; i++) {
>> +		clk = this->resources.clock[i];
>> +		if (!clk)
>> +			break;
>> +
>> +		if (v) {
>> +			ret = clk_prepare_enable(clk);
>> +			if (ret)
>> +				goto err_clk;
>> +		} else {
>> +			clk_disable_unprepare(clk);
>> +		}
>> +	}
>> +	return 0;
>> +
>> +err_clk:
>> +	return ret;
>> +}
>> +
>> +#define gpmi_enable_clk(x) __gpmi_enable_clk(x, true)
>> +#define gpmi_disable_clk(x) __gpmi_enable_clk(x, false)
>> +
>>   int gpmi_init(struct gpmi_nand_data *this)
>>   {
>>   	struct resources *r =&this->resources;
>>   	int ret;
>>
>> -	ret = clk_prepare_enable(r->clock);
>> +	ret = gpmi_enable_clk(this);
>>   	if (ret)
>>   		goto err_out;
>>   	ret = gpmi_reset_block(r->gpmi_regs, false);
>> @@ -149,7 +177,7 @@ int gpmi_init(struct gpmi_nand_data *this)
>>   	/* Select BCH ECC. */
>>   	writel(BM_GPMI_CTRL1_BCH_MODE, r->gpmi_regs + HW_GPMI_CTRL1_SET);
>>
>> -	clk_disable_unprepare(r->clock);
>> +	gpmi_disable_clk(this);
>>   	return 0;
>>   err_out:
>>   	return ret;
>> @@ -205,7 +233,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
>>   	ecc_strength  = bch_geo->ecc_strength>>  1;
>>   	page_size     = bch_geo->page_size;
>>
>> -	ret = clk_prepare_enable(r->clock);
>> +	ret = gpmi_enable_clk(this);
>>   	if (ret)
>>   		goto err_out;
>>
>> @@ -240,7 +268,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
>>   	writel(BM_BCH_CTRL_COMPLETE_IRQ_EN,
>>   				r->bch_regs + HW_BCH_CTRL_SET);
>>
>> -	clk_disable_unprepare(r->clock);
>> +	gpmi_disable_clk(this);
>>   	return 0;
>>   err_out:
>>   	return ret;
>> @@ -716,7 +744,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
>>   	int ret;
>>
>>   	/* Enable the clock. */
>> -	ret = clk_prepare_enable(r->clock);
>> +	ret = gpmi_enable_clk(this);
>>   	if (ret) {
>>   		pr_err("We failed in enable the clk\n");
>>   		goto err_out;
>> @@ -727,7 +755,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
>>   		gpmi_regs + HW_GPMI_TIMING1);
>>
>>   	/* Get the timing information we need. */
>> -	nfc->clock_frequency_in_hz = clk_get_rate(r->clock);
>> +	nfc->clock_frequency_in_hz = clk_get_rate(r->clock[0]);
>>   	clock_period_in_ns = 1000000000 / nfc->clock_frequency_in_hz;
>>
>>   	gpmi_nfc_compute_hardware_timing(this,&hw);
>> @@ -784,8 +812,7 @@ err_out:
>>
>>   void gpmi_end(struct gpmi_nand_data *this)
>>   {
>> -	struct resources *r =&this->resources;
>> -	clk_disable_unprepare(r->clock);
>> +	gpmi_disable_clk(this);
>>   }
>>
>>   /* Clears a BCH interrupt. */
>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> index 941cfb7..edda3b1 100644
>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> @@ -464,9 +464,59 @@ acquire_err:
>>   	return -EINVAL;
>>   }
>>
>> +static void gpmi_put_clks(struct gpmi_nand_data *this)
>> +{
>> +	struct resources *r =&this->resources;
>> +	struct clk *clk;
>> +	int i;
>> +
>> +	for (i = 0; i<  GPMI_CLK_MAX; i++) {
>> +		clk = r->clock[i];
>> +		if (clk) {
>> +			clk_put(clk);
>> +			r->clock[i] = NULL;
>> +		}
>> +	}
>> +}
>> +
>> +static char *extra_clks_for_mx6q[] = {
>> +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
>> +};
>> +
>> +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
>> +{
>> +	struct resources *r =&this->resources;
>> +	char **extra_clks = NULL;
>> +	struct clk *clk;
>> +	int i;
>> +
>> +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
>> +	if (IS_ERR(r->clock[0]))
>> +		goto err_clock;
>> +
>> +	/* Get extra clocks */
>> +	if (GPMI_IS_MX6Q(this))
>> +		extra_clks = extra_clks_for_mx6q;
> We probably do not need this tweaking.  We can have the driver always
> take all those 5 clocks, and I think the current imx28 clock driver
> can just work with it, because the gpmi-nand clkdev lookup has NULL
> con_id and all those 5 clocks can match the same one on imx28.
>
thanks. got it.


Best Regards
Huang Shijie
>> +	if (!extra_clks)
>> +		return 0;
>> +
>> +	for (i = 1; i<  GPMI_CLK_MAX; i++) {
>> +		clk = clk_get(NULL, extra_clks[i - 1]);
> You should not put NULL but&this->pdev->dev here.
>
> Regards,
> Shawn
>
>> +		if (IS_ERR(clk))
>> +			goto err_clock;
>> +
>> +		r->clock[i] = clk;
>> +	}
>> +	return 0;
>> +
>> +err_clock:
>> +	pr_err("failed in finding the clocks.\n");
>> +	gpmi_put_clks(this);
>> +	return -ENOMEM;
>> +}
>> +
>>   static int __devinit acquire_resources(struct gpmi_nand_data *this)
>>   {
>> -	struct resources *res =&this->resources;
>>   	struct pinctrl *pinctrl;
>>   	int ret;
>>
>> @@ -492,12 +542,9 @@ static int __devinit acquire_resources(struct gpmi_nand_data *this)
>>   		goto exit_pin;
>>   	}
>>
>> -	res->clock = clk_get(&this->pdev->dev, NULL);
>> -	if (IS_ERR(res->clock)) {
>> -		pr_err("can not get the clock\n");
>> -		ret = -ENOENT;
>> +	ret = gpmi_get_clks(this);
>> +	if (ret)
>>   		goto exit_clock;
>> -	}
>>   	return 0;
>>
>>   exit_clock:
>> @@ -512,9 +559,7 @@ exit_regs:
>>
>>   static void release_resources(struct gpmi_nand_data *this)
>>   {
>> -	struct resources *r =&this->resources;
>> -
>> -	clk_put(r->clock);
>> +	gpmi_put_clks(this);
>>   	release_register_block(this);
>>   	release_bch_irq(this);
>>   	release_dma_channels(this);
>> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
>> index ce5daa1..1547a60 100644
>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
>> @@ -22,6 +22,7 @@
>>   #include<linux/dma-mapping.h>
>>   #include<linux/fsl/mxs-dma.h>
>>
>> +#define GPMI_CLK_MAX 5 /* MX6Q needs five clocks */
>>   struct resources {
>>   	void          *gpmi_regs;
>>   	void          *bch_regs;
>> @@ -29,7 +30,7 @@ struct resources {
>>   	unsigned int  bch_high_interrupt;
>>   	unsigned int  dma_low_channel;
>>   	unsigned int  dma_high_channel;
>> -	struct clk    *clock;
>> +	struct clk    *clock[GPMI_CLK_MAX];
>>   };
>>
>>   /**
>> -- 
>> 1.7.4.4
>>

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Huang Shijie]

? 2012?06?29? 10:06, Shawn Guo ??:
>>   		gpmi_regs + HW_GPMI_TIMING1);
>> >  
>> >    	/* Get the timing information we need. */
>> >  -	nfc->clock_frequency_in_hz = clk_get_rate(r->clock);
>> >  +	nfc->clock_frequency_in_hz = clk_get_rate(r->clock[0]);
>> >    	clock_period_in_ns = 1000000000 / nfc->clock_frequency_in_hz;
>> >  
>> >    	gpmi_nfc_compute_hardware_timing(this,&hw);
>> >  @@ -784,8 +812,7 @@ err_out:
>> >  
>> >    void gpmi_end(struct gpmi_nand_data *this)
>> >    {
>> >  -	struct resources *r =&this->resources;
>> >  -	clk_disable_unprepare(r->clock);
>> >  +	gpmi_disable_clk(this);
>> >    }
>> >  
>> >    /* Clears a BCH interrupt. */
>> >  diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> >  index 941cfb7..edda3b1 100644
>> >  --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> >  +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
>> >  @@ -464,9 +464,59 @@ acquire_err:
>> >    	return -EINVAL;
>> >    }
>> >  
>> >  +static void gpmi_put_clks(struct gpmi_nand_data *this)
>> >  +{
>> >  +	struct resources *r =&this->resources;
>> >  +	struct clk *clk;
>> >  +	int i;
>> >  +
>> >  +	for (i = 0; i<  GPMI_CLK_MAX; i++) {
>> >  +		clk = r->clock[i];
>> >  +		if (clk) {
>> >  +			clk_put(clk);
>> >  +			r->clock[i] = NULL;
>> >  +		}
>> >  +	}
>> >  +}
>> >  +
>> >  +static char *extra_clks_for_mx6q[] = {
>> >  +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
>> >  +};
>> >  +
>> >  +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
>> >  +{
>> >  +	struct resources *r =&this->resources;
>> >  +	char **extra_clks = NULL;
>> >  +	struct clk *clk;
>> >  +	int i;
>> >  +
>> >  +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
>> >  +	if (IS_ERR(r->clock[0]))
>> >  +		goto err_clock;
>> >  +
>> >  +	/* Get extra clocks */
>> >  +	if (GPMI_IS_MX6Q(this))
>> >  +		extra_clks = extra_clks_for_mx6q;
> We probably do not need this tweaking.  We can have the driver always
> take all those 5 clocks, and I think the current imx28 clock driver
> can just work with it, because the gpmi-nand clkdev lookup has NULL
> con_id and all those 5 clocks can match the same one on imx28.
>
I think your method makes the code hard to understand.
My code is more clear in logic.

thanks
Huang Shijie

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Dong Aisheng]

On Fri, Jun 29, 2012 at 10:06:52AM +0800, Shawn Guo wrote:
> On Thu, Jun 28, 2012 at 11:52:05PM -0400, Huang Shijie wrote:
> > From: Huang Shijie <b32955@freescale.com>
> > 
> > The gpmi nand driver may needs several clocks(MX6Q needs five clocks).
> > 
> > In the old clock framework, all these clocks are chained together,
> > all you need is to manipulate the first clock.
> > 
> > But the kernel uses the common clk framework now, which forces us to
> > get the clocks one by one. When we use them, we have to enable them
> > one by one too.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > Signed-off-by: Huang Shijie <shijie8@gmail.com>
> > ---
> >  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |   43 ++++++++++++++++++----
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   63 +++++++++++++++++++++++++++-----
> >  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    3 +-
> >  3 files changed, 91 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > index a1f4332..c3778c0 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> > @@ -124,12 +124,40 @@ error:
> >  	return -ETIMEDOUT;
> >  }
> >  
> > +static int __gpmi_enable_clk(struct gpmi_nand_data *this, bool v)
> > +{
> > +	struct clk *clk;
> > +	int ret;
> > +	int i;
> > +
> > +	for (i = 0; i < GPMI_CLK_MAX; i++) {
> > +		clk = this->resources.clock[i];
> > +		if (!clk)
> > +			break;
> > +
> > +		if (v) {
> > +			ret = clk_prepare_enable(clk);
> > +			if (ret)
> > +				goto err_clk;
> > +		} else {
> > +			clk_disable_unprepare(clk);
> > +		}
> > +	}
> > +	return 0;
> > +
> > +err_clk:
> > +	return ret;
> > +}
> > +
> > +#define gpmi_enable_clk(x) __gpmi_enable_clk(x, true)
> > +#define gpmi_disable_clk(x) __gpmi_enable_clk(x, false)
> > +
> >  int gpmi_init(struct gpmi_nand_data *this)
> >  {
> >  	struct resources *r = &this->resources;
> >  	int ret;
> >  
> > -	ret = clk_prepare_enable(r->clock);
> > +	ret = gpmi_enable_clk(this);
> >  	if (ret)
> >  		goto err_out;
> >  	ret = gpmi_reset_block(r->gpmi_regs, false);
> > @@ -149,7 +177,7 @@ int gpmi_init(struct gpmi_nand_data *this)
> >  	/* Select BCH ECC. */
> >  	writel(BM_GPMI_CTRL1_BCH_MODE, r->gpmi_regs + HW_GPMI_CTRL1_SET);
> >  
> > -	clk_disable_unprepare(r->clock);
> > +	gpmi_disable_clk(this);
> >  	return 0;
> >  err_out:
> >  	return ret;
> > @@ -205,7 +233,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
> >  	ecc_strength  = bch_geo->ecc_strength >> 1;
> >  	page_size     = bch_geo->page_size;
> >  
> > -	ret = clk_prepare_enable(r->clock);
> > +	ret = gpmi_enable_clk(this);
> >  	if (ret)
> >  		goto err_out;
> >  
> > @@ -240,7 +268,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
> >  	writel(BM_BCH_CTRL_COMPLETE_IRQ_EN,
> >  				r->bch_regs + HW_BCH_CTRL_SET);
> >  
> > -	clk_disable_unprepare(r->clock);
> > +	gpmi_disable_clk(this);
> >  	return 0;
> >  err_out:
> >  	return ret;
> > @@ -716,7 +744,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
> >  	int ret;
> >  
> >  	/* Enable the clock. */
> > -	ret = clk_prepare_enable(r->clock);
> > +	ret = gpmi_enable_clk(this);
> >  	if (ret) {
> >  		pr_err("We failed in enable the clk\n");
> >  		goto err_out;
> > @@ -727,7 +755,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
> >  		gpmi_regs + HW_GPMI_TIMING1);
> >  
> >  	/* Get the timing information we need. */
> > -	nfc->clock_frequency_in_hz = clk_get_rate(r->clock);
> > +	nfc->clock_frequency_in_hz = clk_get_rate(r->clock[0]);
> >  	clock_period_in_ns = 1000000000 / nfc->clock_frequency_in_hz;
> >  
> >  	gpmi_nfc_compute_hardware_timing(this, &hw);
> > @@ -784,8 +812,7 @@ err_out:
> >  
> >  void gpmi_end(struct gpmi_nand_data *this)
> >  {
> > -	struct resources *r = &this->resources;
> > -	clk_disable_unprepare(r->clock);
> > +	gpmi_disable_clk(this);
> >  }
> >  
> >  /* Clears a BCH interrupt. */
> > diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > index 941cfb7..edda3b1 100644
> > --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> > @@ -464,9 +464,59 @@ acquire_err:
> >  	return -EINVAL;
> >  }
> >  
> > +static void gpmi_put_clks(struct gpmi_nand_data *this)
> > +{
> > +	struct resources *r = &this->resources;
> > +	struct clk *clk;
> > +	int i;
> > +
> > +	for (i = 0; i < GPMI_CLK_MAX; i++) {
> > +		clk = r->clock[i];
> > +		if (clk) {
> > +			clk_put(clk);
> > +			r->clock[i] = NULL;
> > +		}
> > +	}
> > +}
> > +
> > +static char *extra_clks_for_mx6q[] = {
> > +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
> > +};
> > +
> > +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
> > +{
> > +	struct resources *r = &this->resources;
> > +	char **extra_clks = NULL;
> > +	struct clk *clk;
> > +	int i;
> > +
> > +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
> > +	if (IS_ERR(r->clock[0]))
> > +		goto err_clock;
> > +
> > +	/* Get extra clocks */
> > +	if (GPMI_IS_MX6Q(this))
> > +		extra_clks = extra_clks_for_mx6q;
> 
> We probably do not need this tweaking.  We can have the driver always
> take all those 5 clocks, and I think the current imx28 clock driver
> can just work with it, because the gpmi-nand clkdev lookup has NULL
> con_id and all those 5 clocks can match the same one on imx28.
> 
Will mx28 fail if doing like that?
clk_get will fail if no clock found.
struct clk *clk_get_sys(const char *dev_id, const char *con_id)
{
        struct clk_lookup *cl;

        mutex_lock(&clocks_mutex);
        cl = clk_find(dev_id, con_id);
        if (cl && !__clk_get(cl->clk))
                cl = NULL;
        mutex_unlock(&clocks_mutex);

        return cl ? cl->clk : ERR_PTR(-ENOENT);
}
EXPORT_SYMBOL(clk_get_sys);

Furthermore, find unnecessary clock for mx28 seems not a good choice.
Probably a better way is to define each SoC required clocks and get them
respectively. It's explicit and clear.

Regards
Dong Aisheng

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Lothar Waßmann]

Hi,

Dong Aisheng writes:
> On Fri, Jun 29, 2012 at 10:06:52AM +0800, Shawn Guo wrote:
> > On Thu, Jun 28, 2012 at 11:52:05PM -0400, Huang Shijie wrote:
> > > From: Huang Shijie <b32955@freescale.com>
> > > 
> > > The gpmi nand driver may needs several clocks(MX6Q needs five clocks).
> > > 
> > > In the old clock framework, all these clocks are chained together,
> > > all you need is to manipulate the first clock.
> > > 
> > > But the kernel uses the common clk framework now, which forces us to
> > > get the clocks one by one. When we use them, we have to enable them
> > > one by one too.
> > > 
[...]
> > > +static char *extra_clks_for_mx6q[] = {
> > > +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
> > > +};
> > > +
> > > +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
> > > +{
> > > +	struct resources *r = &this->resources;
> > > +	char **extra_clks = NULL;
> > > +	struct clk *clk;
> > > +	int i;
> > > +
> > > +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
> > > +	if (IS_ERR(r->clock[0]))
> > > +		goto err_clock;
> > > +
> > > +	/* Get extra clocks */
> > > +	if (GPMI_IS_MX6Q(this))
> > > +		extra_clks = extra_clks_for_mx6q;
> > 
> > We probably do not need this tweaking.  We can have the driver always
> > take all those 5 clocks, and I think the current imx28 clock driver
> > can just work with it, because the gpmi-nand clkdev lookup has NULL
> > con_id and all those 5 clocks can match the same one on imx28.
> > 
> Will mx28 fail if doing like that?
> clk_get will fail if no clock found.
> struct clk *clk_get_sys(const char *dev_id, const char *con_id)
> {
>         struct clk_lookup *cl;
> 
>         mutex_lock(&clocks_mutex);
>         cl = clk_find(dev_id, con_id);
>         if (cl && !__clk_get(cl->clk))
>                 cl = NULL;
>         mutex_unlock(&clocks_mutex);
> 
>         return cl ? cl->clk : ERR_PTR(-ENOENT);
> }
> EXPORT_SYMBOL(clk_get_sys);
> 
> Furthermore, find unnecessary clock for mx28 seems not a good choice.
> Probably a better way is to define each SoC required clocks and get them
> respectively. It's explicit and clear.
> 
No, that's silly. You would have to change the driver for each
new platform that the driver can support.

Instead the driver should request the maximum number of clocks that
the existing set of platforms provide and all platforms with fewer
clocks should provide an appropriate number of dummy clocks.

This way new platforms can be supported without any change to the
driver and only if a platform requires even more clocks (like in this
particular case) would some code outside that platform have to be
changed at all.


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Dong Aisheng]

On Fri, Jun 29, 2012 at 05:29:26PM +0800, Lothar Wa?mann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Fri, Jun 29, 2012 at 10:06:52AM +0800, Shawn Guo wrote:
> > > On Thu, Jun 28, 2012 at 11:52:05PM -0400, Huang Shijie wrote:
> > > > From: Huang Shijie <b32955@freescale.com>
> > > > 
> > > > The gpmi nand driver may needs several clocks(MX6Q needs five clocks).
> > > > 
> > > > In the old clock framework, all these clocks are chained together,
> > > > all you need is to manipulate the first clock.
> > > > 
> > > > But the kernel uses the common clk framework now, which forces us to
> > > > get the clocks one by one. When we use them, we have to enable them
> > > > one by one too.
> > > > 
> [...]
> > > > +static char *extra_clks_for_mx6q[] = {
> > > > +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
> > > > +};
> > > > +
> > > > +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
> > > > +{
> > > > +	struct resources *r = &this->resources;
> > > > +	char **extra_clks = NULL;
> > > > +	struct clk *clk;
> > > > +	int i;
> > > > +
> > > > +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
> > > > +	if (IS_ERR(r->clock[0]))
> > > > +		goto err_clock;
> > > > +
> > > > +	/* Get extra clocks */
> > > > +	if (GPMI_IS_MX6Q(this))
> > > > +		extra_clks = extra_clks_for_mx6q;
> > > 
> > > We probably do not need this tweaking.  We can have the driver always
> > > take all those 5 clocks, and I think the current imx28 clock driver
> > > can just work with it, because the gpmi-nand clkdev lookup has NULL
> > > con_id and all those 5 clocks can match the same one on imx28.
> > > 
> > Will mx28 fail if doing like that?
> > clk_get will fail if no clock found.
> > struct clk *clk_get_sys(const char *dev_id, const char *con_id)
> > {
> >         struct clk_lookup *cl;
> > 
> >         mutex_lock(&clocks_mutex);
> >         cl = clk_find(dev_id, con_id);
> >         if (cl && !__clk_get(cl->clk))
> >                 cl = NULL;
> >         mutex_unlock(&clocks_mutex);
> > 
> >         return cl ? cl->clk : ERR_PTR(-ENOENT);
> > }
> > EXPORT_SYMBOL(clk_get_sys);
> > 
> > Furthermore, find unnecessary clock for mx28 seems not a good choice.
> > Probably a better way is to define each SoC required clocks and get them
> > respectively. It's explicit and clear.
> > 
> No, that's silly. You would have to change the driver for each
> new platform that the driver can support.
> 
If the new platform is fully compatible with exist platforms, then no.
If need more clocks, then in either way, we have to add support in driver.

> Instead the driver should request the maximum number of clocks that
> the existing set of platforms provide and all platforms with fewer
> clocks should provide an appropriate number of dummy clocks.
> 
I wish we can not use dummy since it's easy causing misleading until
we have no other better way to go.

> This way new platforms can be supported without any change to the
> driver and only if a platform requires even more clocks (like in this
> particular case) would some code outside that platform have to be
> changed at all.

Regards
Dong Aisheng

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Lothar Waßmann]

Hi,

Dong Aisheng writes:
> On Fri, Jun 29, 2012 at 05:29:26PM +0800, Lothar Wa?mann wrote:
> > Hi,
> > 
> > Dong Aisheng writes:
> > > On Fri, Jun 29, 2012 at 10:06:52AM +0800, Shawn Guo wrote:
> > > > On Thu, Jun 28, 2012 at 11:52:05PM -0400, Huang Shijie wrote:
> > > > > From: Huang Shijie <b32955@freescale.com>
> > > > > 
> > > > > The gpmi nand driver may needs several clocks(MX6Q needs five clocks).
> > > > > 
> > > > > In the old clock framework, all these clocks are chained together,
> > > > > all you need is to manipulate the first clock.
> > > > > 
> > > > > But the kernel uses the common clk framework now, which forces us to
> > > > > get the clocks one by one. When we use them, we have to enable them
> > > > > one by one too.
> > > > > 
> > [...]
> > > > > +static char *extra_clks_for_mx6q[] = {
> > > > > +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
> > > > > +};
> > > > > +
> > > > > +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
> > > > > +{
> > > > > +	struct resources *r = &this->resources;
> > > > > +	char **extra_clks = NULL;
> > > > > +	struct clk *clk;
> > > > > +	int i;
> > > > > +
> > > > > +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
> > > > > +	if (IS_ERR(r->clock[0]))
> > > > > +		goto err_clock;
> > > > > +
> > > > > +	/* Get extra clocks */
> > > > > +	if (GPMI_IS_MX6Q(this))
> > > > > +		extra_clks = extra_clks_for_mx6q;
> > > > 
> > > > We probably do not need this tweaking.  We can have the driver always
> > > > take all those 5 clocks, and I think the current imx28 clock driver
> > > > can just work with it, because the gpmi-nand clkdev lookup has NULL
> > > > con_id and all those 5 clocks can match the same one on imx28.
> > > > 
> > > Will mx28 fail if doing like that?
> > > clk_get will fail if no clock found.
> > > struct clk *clk_get_sys(const char *dev_id, const char *con_id)
> > > {
> > >         struct clk_lookup *cl;
> > > 
> > >         mutex_lock(&clocks_mutex);
> > >         cl = clk_find(dev_id, con_id);
> > >         if (cl && !__clk_get(cl->clk))
> > >                 cl = NULL;
> > >         mutex_unlock(&clocks_mutex);
> > > 
> > >         return cl ? cl->clk : ERR_PTR(-ENOENT);
> > > }
> > > EXPORT_SYMBOL(clk_get_sys);
> > > 
> > > Furthermore, find unnecessary clock for mx28 seems not a good choice.
> > > Probably a better way is to define each SoC required clocks and get them
> > > respectively. It's explicit and clear.
> > > 
> > No, that's silly. You would have to change the driver for each
> > new platform that the driver can support.
> > 
> If the new platform is fully compatible with exist platforms, then no.
> If need more clocks, then in either way, we have to add support in driver.
> 
Even if it had fewer clocks you would have to change the driver!


Lothar Wa?mann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstra?e 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Gesch?ftsf?hrer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info at karo-electronics.de
___________________________________________________________

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Dong Aisheng]

On Fri, Jun 29, 2012 at 05:44:47PM +0800, Lothar Wa?mann wrote:
> Hi,
> 
> Dong Aisheng writes:
> > On Fri, Jun 29, 2012 at 05:29:26PM +0800, Lothar Wa?mann wrote:
> > > Hi,
> > > 
> > > Dong Aisheng writes:
> > > > On Fri, Jun 29, 2012 at 10:06:52AM +0800, Shawn Guo wrote:
> > > > > On Thu, Jun 28, 2012 at 11:52:05PM -0400, Huang Shijie wrote:
> > > > > > From: Huang Shijie <b32955@freescale.com>
> > > > > > 
> > > > > > The gpmi nand driver may needs several clocks(MX6Q needs five clocks).
> > > > > > 
> > > > > > In the old clock framework, all these clocks are chained together,
> > > > > > all you need is to manipulate the first clock.
> > > > > > 
> > > > > > But the kernel uses the common clk framework now, which forces us to
> > > > > > get the clocks one by one. When we use them, we have to enable them
> > > > > > one by one too.
> > > > > > 
> > > [...]
> > > > > > +static char *extra_clks_for_mx6q[] = {
> > > > > > +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
> > > > > > +};
> > > > > > +
> > > > > > +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
> > > > > > +{
> > > > > > +	struct resources *r = &this->resources;
> > > > > > +	char **extra_clks = NULL;
> > > > > > +	struct clk *clk;
> > > > > > +	int i;
> > > > > > +
> > > > > > +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
> > > > > > +	if (IS_ERR(r->clock[0]))
> > > > > > +		goto err_clock;
> > > > > > +
> > > > > > +	/* Get extra clocks */
> > > > > > +	if (GPMI_IS_MX6Q(this))
> > > > > > +		extra_clks = extra_clks_for_mx6q;
> > > > > 
> > > > > We probably do not need this tweaking.  We can have the driver always
> > > > > take all those 5 clocks, and I think the current imx28 clock driver
> > > > > can just work with it, because the gpmi-nand clkdev lookup has NULL
> > > > > con_id and all those 5 clocks can match the same one on imx28.
> > > > > 
> > > > Will mx28 fail if doing like that?
> > > > clk_get will fail if no clock found.
> > > > struct clk *clk_get_sys(const char *dev_id, const char *con_id)
> > > > {
> > > >         struct clk_lookup *cl;
> > > > 
> > > >         mutex_lock(&clocks_mutex);
> > > >         cl = clk_find(dev_id, con_id);
> > > >         if (cl && !__clk_get(cl->clk))
> > > >                 cl = NULL;
> > > >         mutex_unlock(&clocks_mutex);
> > > > 
> > > >         return cl ? cl->clk : ERR_PTR(-ENOENT);
> > > > }
> > > > EXPORT_SYMBOL(clk_get_sys);
> > > > 
> > > > Furthermore, find unnecessary clock for mx28 seems not a good choice.
> > > > Probably a better way is to define each SoC required clocks and get them
> > > > respectively. It's explicit and clear.
> > > > 
> > > No, that's silly. You would have to change the driver for each
> > > new platform that the driver can support.
> > > 
> > If the new platform is fully compatible with exist platforms, then no.
> > If need more clocks, then in either way, we have to add support in driver.
> > 
> Even if it had fewer clocks you would have to change the driver!
> 
Yes.
For your case, you need create some dummy clock, right?
If yes, i would intend to explictly to define it for a new type
of IP.

Regards
Dong Aisheng

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Shawn Guo]

On Fri, Jun 29, 2012 at 05:54:33PM +0800, Dong Aisheng wrote:
> > > > No, that's silly. You would have to change the driver for each
> > > > new platform that the driver can support.
> > > > 
> > > If the new platform is fully compatible with exist platforms, then no.
> > > If need more clocks, then in either way, we have to add support in driver.
> > > 
> > Even if it had fewer clocks you would have to change the driver!
> > 
> Yes.
> For your case, you need create some dummy clock, right?
> If yes, i would intend to explictly to define it for a new type
> of IP.
> 
I'm with Lothar on this.  At least for imx28 case, we do not even need
dummy clock, because NULL con_id in clkdev lookup will have all those
clocks match the only one gpmi-nand clock on imx28, as I already said.

-- 
Regards,
Shawn

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Dong Aisheng]

On Fri, Jun 29, 2012 at 07:13:36PM +0800, Shawn Guo wrote:
> On Fri, Jun 29, 2012 at 05:54:33PM +0800, Dong Aisheng wrote:
> > > > > No, that's silly. You would have to change the driver for each
> > > > > new platform that the driver can support.
> > > > > 
> > > > If the new platform is fully compatible with exist platforms, then no.
> > > > If need more clocks, then in either way, we have to add support in driver.
> > > > 
> > > Even if it had fewer clocks you would have to change the driver!
> > > 
> > Yes.
> > For your case, you need create some dummy clock, right?
> > If yes, i would intend to explictly to define it for a new type
> > of IP.
> > 
> I'm with Lothar on this.  At least for imx28 case, we do not even need
> dummy clock, because NULL con_id in clkdev lookup will have all those
> clocks match the only one gpmi-nand clock on imx28, as I already said.
> 
I'm not quite understand. Can you clarify a bit more?
For extra clocks need by mx6q like "gpmi_bch", they're indeed have con_id.
Getting them will not fail on imx28 without dummy clock?

Regards
Dong Aisheng

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Shawn Guo]

On Fri, Jun 29, 2012 at 08:14:38PM +0800, Dong Aisheng wrote:
> > I'm with Lothar on this.  At least for imx28 case, we do not even need
> > dummy clock, because NULL con_id in clkdev lookup will have all those
> > clocks match the only one gpmi-nand clock on imx28, as I already said.
> > 
> I'm not quite understand. Can you clarify a bit more?
> For extra clocks need by mx6q like "gpmi_bch", they're indeed have con_id.
> Getting them will not fail on imx28 without dummy clock?
> 
You need to look at how imx28 registers clkdev for gpmi-nand.

static struct clk_lookup gpmi_lookups[] __initdata = {
        { .dev_id = "imx28-gpmi-nand", },
        { .dev_id = "8000c000.gpmi", },
};

clk_register_clkdevs(clks[gpmi], gpmi_lookups, ARRAY_SIZE(gpmi_lookups));

As you can see, con_id is NULL for imx28 case.  As NULL is actually
a wildcard for looking up, all the gpmi-nand clocks match it.

-- 
Regards,
Shawn

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Dong Aisheng]

On Fri, Jun 29, 2012 at 08:41:57PM +0800, Shawn Guo wrote:
> On Fri, Jun 29, 2012 at 08:14:38PM +0800, Dong Aisheng wrote:
> > > I'm with Lothar on this.  At least for imx28 case, we do not even need
> > > dummy clock, because NULL con_id in clkdev lookup will have all those
> > > clocks match the only one gpmi-nand clock on imx28, as I already said.
> > > 
> > I'm not quite understand. Can you clarify a bit more?
> > For extra clocks need by mx6q like "gpmi_bch", they're indeed have con_id.
> > Getting them will not fail on imx28 without dummy clock?
> > 
> You need to look at how imx28 registers clkdev for gpmi-nand.
> 
> static struct clk_lookup gpmi_lookups[] __initdata = {
>         { .dev_id = "imx28-gpmi-nand", },
>         { .dev_id = "8000c000.gpmi", },
> };
> 
> clk_register_clkdevs(clks[gpmi], gpmi_lookups, ARRAY_SIZE(gpmi_lookups));
> 
> As you can see, con_id is NULL for imx28 case.  As NULL is actually
> a wildcard for looking up, all the gpmi-nand clocks match it.
> 
I know this.
You may missed my point.
I mean extra clocks introduced for imx6q which is not exist on imx28.
For your proposal, the gpmi driver will acquire all clocks including
imx6q's.
So when get imx6q's gpmi clock like "gpmi-bch" like clk_get(dev, "gpmi-bch"),
it may fail for imx28.

Regards
Dong Aisheng

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Shawn Guo]

On Fri, Jun 29, 2012 at 08:41:59PM +0800, Dong Aisheng wrote:
> I know this.
> You may missed my point.
> I mean extra clocks introduced for imx6q which is not exist on imx28.
> For your proposal, the gpmi driver will acquire all clocks including
> imx6q's.
> So when get imx6q's gpmi clock like "gpmi-bch" like clk_get(dev, "gpmi-bch"),
> it may fail for imx28.
> 
What I was saying is it will not fail.

-- 
Regards,
Shawn

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Huang Shijie]

? 2012?06?29? 17:44, Lothar Wa?mann ??:
> Even if it had fewer clocks you would have to change the driver!
If we do not want to change the driver, we could add the extra clocks in 
the device tree node.

The driver will parse out the extra clocks.

In this way, the driver will not be changed in any platform, no matter 
mx28 or mx6q.


Huang Shijie

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Shawn Guo]

On Fri, Jun 29, 2012 at 06:02:50PM +0800, Huang Shijie wrote:
> ? 2012?06?29? 17:44, Lothar Wa?mann ??:
> >Even if it had fewer clocks you would have to change the driver!
> If we do not want to change the driver, we could add the extra
> clocks in the device tree node.
> 
We need to think about how this would cope with the properties defined
by common clock DT bindings.

-- 
Regards,
Shawn

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Mark Brown]

On Fri, Jun 29, 2012 at 11:29:26AM +0200, Lothar Wa??mann wrote:

> Instead the driver should request the maximum number of clocks that
> the existing set of platforms provide and all platforms with fewer
> clocks should provide an appropriate number of dummy clocks.

Or the driver could handle missing clocks, that can make more sense in
some cases.

[PATCH 3/3] mtd: gpmi: change the code for clocks

[Subodh Nijsure]

On 06/28/2012 08:52 PM, Huang Shijie wrote:
> From: Huang Shijie <b32955@freescale.com>
>
> The gpmi nand driver may needs several clocks(MX6Q needs five clocks).
>
> In the old clock framework, all these clocks are chained together,
> all you need is to manipulate the first clock.
>
> But the kernel uses the common clk framework now, which forces us to
> get the clocks one by one. When we use them, we have to enable them
> one by one too.
>
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> Signed-off-by: Huang Shijie <shijie8@gmail.com>
I will test this on MX28 platform, given that some of these things may 
span multiple platform should we include platforms the patch was tested 
on or request that someone with access to specific platform test this?

-Subodh
> ---
>   drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |   43 ++++++++++++++++++----
>   drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   63 +++++++++++++++++++++++++++-----
>   drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    3 +-
>   3 files changed, 91 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> index a1f4332..c3778c0 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
> @@ -124,12 +124,40 @@ error:
>   	return -ETIMEDOUT;
>   }
>   
> +static int __gpmi_enable_clk(struct gpmi_nand_data *this, bool v)
> +{
> +	struct clk *clk;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < GPMI_CLK_MAX; i++) {
> +		clk = this->resources.clock[i];
> +		if (!clk)
> +			break;
> +
> +		if (v) {
> +			ret = clk_prepare_enable(clk);
> +			if (ret)
> +				goto err_clk;
> +		} else {
> +			clk_disable_unprepare(clk);
> +		}
> +	}
> +	return 0;
> +
> +err_clk:
> +	return ret;
> +}
> +
> +#define gpmi_enable_clk(x) __gpmi_enable_clk(x, true)
> +#define gpmi_disable_clk(x) __gpmi_enable_clk(x, false)
> +
>   int gpmi_init(struct gpmi_nand_data *this)
>   {
>   	struct resources *r = &this->resources;
>   	int ret;
>   
> -	ret = clk_prepare_enable(r->clock);
> +	ret = gpmi_enable_clk(this);
>   	if (ret)
>   		goto err_out;
>   	ret = gpmi_reset_block(r->gpmi_regs, false);
> @@ -149,7 +177,7 @@ int gpmi_init(struct gpmi_nand_data *this)
>   	/* Select BCH ECC. */
>   	writel(BM_GPMI_CTRL1_BCH_MODE, r->gpmi_regs + HW_GPMI_CTRL1_SET);
>   
> -	clk_disable_unprepare(r->clock);
> +	gpmi_disable_clk(this);
>   	return 0;
>   err_out:
>   	return ret;
> @@ -205,7 +233,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
>   	ecc_strength  = bch_geo->ecc_strength >> 1;
>   	page_size     = bch_geo->page_size;
>   
> -	ret = clk_prepare_enable(r->clock);
> +	ret = gpmi_enable_clk(this);
>   	if (ret)
>   		goto err_out;
>   
> @@ -240,7 +268,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
>   	writel(BM_BCH_CTRL_COMPLETE_IRQ_EN,
>   				r->bch_regs + HW_BCH_CTRL_SET);
>   
> -	clk_disable_unprepare(r->clock);
> +	gpmi_disable_clk(this);
>   	return 0;
>   err_out:
>   	return ret;
> @@ -716,7 +744,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
>   	int ret;
>   
>   	/* Enable the clock. */
> -	ret = clk_prepare_enable(r->clock);
> +	ret = gpmi_enable_clk(this);
>   	if (ret) {
>   		pr_err("We failed in enable the clk\n");
>   		goto err_out;
> @@ -727,7 +755,7 @@ void gpmi_begin(struct gpmi_nand_data *this)
>   		gpmi_regs + HW_GPMI_TIMING1);
>   
>   	/* Get the timing information we need. */
> -	nfc->clock_frequency_in_hz = clk_get_rate(r->clock);
> +	nfc->clock_frequency_in_hz = clk_get_rate(r->clock[0]);
>   	clock_period_in_ns = 1000000000 / nfc->clock_frequency_in_hz;
>   
>   	gpmi_nfc_compute_hardware_timing(this, &hw);
> @@ -784,8 +812,7 @@ err_out:
>   
>   void gpmi_end(struct gpmi_nand_data *this)
>   {
> -	struct resources *r = &this->resources;
> -	clk_disable_unprepare(r->clock);
> +	gpmi_disable_clk(this);
>   }
>   
>   /* Clears a BCH interrupt. */
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 941cfb7..edda3b1 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -464,9 +464,59 @@ acquire_err:
>   	return -EINVAL;
>   }
>   
> +static void gpmi_put_clks(struct gpmi_nand_data *this)
> +{
> +	struct resources *r = &this->resources;
> +	struct clk *clk;
> +	int i;
> +
> +	for (i = 0; i < GPMI_CLK_MAX; i++) {
> +		clk = r->clock[i];
> +		if (clk) {
> +			clk_put(clk);
> +			r->clock[i] = NULL;
> +		}
> +	}
> +}
> +
> +static char *extra_clks_for_mx6q[] = {
> +	"gpmi_apb", "gpmi_bch", "gpmi_bch_apb", "per1_bch",
> +};
> +
> +static int __devinit gpmi_get_clks(struct gpmi_nand_data *this)
> +{
> +	struct resources *r = &this->resources;
> +	char **extra_clks = NULL;
> +	struct clk *clk;
> +	int i;
> +
> +	r->clock[0] = clk_get(&this->pdev->dev, NULL);
> +	if (IS_ERR(r->clock[0]))
> +		goto err_clock;
> +
> +	/* Get extra clocks */
> +	if (GPMI_IS_MX6Q(this))
> +		extra_clks = extra_clks_for_mx6q;
> +	if (!extra_clks)
> +		return 0;
> +
> +	for (i = 1; i < GPMI_CLK_MAX; i++) {
> +		clk = clk_get(NULL, extra_clks[i - 1]);
> +		if (IS_ERR(clk))
> +			goto err_clock;
> +
> +		r->clock[i] = clk;
> +	}
> +	return 0;
> +
> +err_clock:
> +	pr_err("failed in finding the clocks.\n");
> +	gpmi_put_clks(this);
> +	return -ENOMEM;
> +}
> +
>   static int __devinit acquire_resources(struct gpmi_nand_data *this)
>   {
> -	struct resources *res = &this->resources;
>   	struct pinctrl *pinctrl;
>   	int ret;
>   
> @@ -492,12 +542,9 @@ static int __devinit acquire_resources(struct gpmi_nand_data *this)
>   		goto exit_pin;
>   	}
>   
> -	res->clock = clk_get(&this->pdev->dev, NULL);
> -	if (IS_ERR(res->clock)) {
> -		pr_err("can not get the clock\n");
> -		ret = -ENOENT;
> +	ret = gpmi_get_clks(this);
> +	if (ret)
>   		goto exit_clock;
> -	}
>   	return 0;
>   
>   exit_clock:
> @@ -512,9 +559,7 @@ exit_regs:
>   
>   static void release_resources(struct gpmi_nand_data *this)
>   {
> -	struct resources *r = &this->resources;
> -
> -	clk_put(r->clock);
> +	gpmi_put_clks(this);
>   	release_register_block(this);
>   	release_bch_irq(this);
>   	release_dma_channels(this);
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> index ce5daa1..1547a60 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
> @@ -22,6 +22,7 @@
>   #include <linux/dma-mapping.h>
>   #include <linux/fsl/mxs-dma.h>
>   
> +#define GPMI_CLK_MAX 5 /* MX6Q needs five clocks */
>   struct resources {
>   	void          *gpmi_regs;
>   	void          *bch_regs;
> @@ -29,7 +30,7 @@ struct resources {
>   	unsigned int  bch_high_interrupt;
>   	unsigned int  dma_low_channel;
>   	unsigned int  dma_high_channel;
> -	struct clk    *clock;
> +	struct clk    *clock[GPMI_CLK_MAX];
>   };
>   
>   /**