ARM: imx6q: add clocks for gpmi-nand
Subsystems (6)
PATCH 1/3 ARM: imx6q: add clocks for gpmi-nand
Add clocks for gpmi-nand.
--- arch/arm/mach-imx/clk-imx6q.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)
arch/arm/mach-imx/clk-imx6q.c +7 -1
@@ -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" );
Signature · 1 line
1.7.4.4
PATCH 1/3 ARM: imx6q: add clocks for gpmi-nand
Quoted 4 lines · Huang Shijie
Add clocks for gpmi-nand.
Signed-off-by Huang Shijie <[email protected]>Signed-off-by Huang Shijie <[email protected]>
Why double sign off?
Quoted 26 lines
--- arch/arm/mach-imx/clk-imx6q.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)arch/arm/mach-imx/clk-imx6q.c +2
-1@@ -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 );
arch/arm/mach-imx/clk-imx6q.c int __init mx6q_clocks_init(void) · line 401
Quoted 7 lines
arch/arm/mach-imx/clk-imx6q.c +4
-0@@ -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.
Quoted 7 lines
arch/arm/mach-imx/clk-imx6q.c +1
-0+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" );Signature · 1 line
1.7.4.4
Signature · 2 lines
Regards, Shawn
PATCH 2/3 ARM: imx6q: add DT node for gpmi nand
In reply to Huang ShijieAdd 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.
--- arch/arm/boot/dts/imx6q-arm2.dts | 6 ++++++ arch/arm/boot/dts/imx6q.dtsi | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 0 deletions(-)
arch/arm/boot/dts/imx6q-arm2.dts +6 -0
@@ -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";
arch/arm/boot/dts/imx6q.dtsi +36 -0
@@ -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 */
Signature · 1 line
1.7.4.4
PATCH 2/3 ARM: imx6q: add DT node for gpmi nand
Quoted 9 lines · 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 <[email protected]>Signed-off-by Huang Shijie <[email protected]>
Just one minor suggestion, see below.
Otherwise,
Regards
Dong Aisheng
Quoted 10 lines
--- arch/arm/boot/dts/imx6q-arm2.dts | 6 ++++++ arch/arm/boot/dts/imx6q.dtsi | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 0 deletions(-)arch/arm/boot/dts/imx6q-arm2.dts +0
-0
arch/arm/boot/dts/imx6q-arm2.dts line 29
Quoted 8 lines
arch/arm/boot/dts/imx6q-arm2.dts +4
-0@@ -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.
Quoted 5 lines
arch/arm/boot/dts/imx6q-arm2.dts +2
-0+ }; + 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
? 2012?06?29? 11:10, Dong Aisheng ??:
Quoted 2 lines
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 1/3 ARM: imx6q: add clocks for gpmi-nand
In reply to Huang ShijieQuoted 34 lines · Huang Shijie
Add clocks for gpmi-nand.
Signed-off-by Huang Shijie <[email protected]>Signed-off-by Huang Shijie <[email protected]>--- arch/arm/mach-imx/clk-imx6q.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)arch/arm/mach-imx/clk-imx6q.c +3
-1@@ -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 );
...
arch/arm/mach-imx/clk-imx6q.c int __init mx6q_clocks_init(void) · line 401
Quoted 3 lines
arch/arm/mach-imx/clk-imx6q.c +3
-0+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.
arch/arm/mach-imx/clk-imx6q.c int __init mx6q_clocks_init(void) · line 402
Quoted 1 line
arch/arm/mach-imx/clk-imx6q.c +1
-0+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.
Quoted 5 lines
arch/arm/mach-imx/clk-imx6q.c +0
-0clk_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" );Signature · 1 line
1.7.4.4
Regards
Dong Aisheng
PATCH 1/3 ARM: imx6q: add clocks for gpmi-nand
? 2012?06?29? 10:29, Dong Aisheng ??:
Quoted 3 lines
We had several gpmi clocks here, so i guess it would be good if we have the
con_idset 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
Quoted 7 lines · Huang Shijie
? 2012?06?29? 10:29, Dong Aisheng ??:
Quoted 3 lines
We had several gpmi clocks here, so i guess it would be good if we have the
con_idset 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.
Signature · 2 lines
Regards, Shawn
PATCH 3/3 mtd: gpmi: change the code for clocks
In reply to Huang ShijieFrom: Huang Shijie <[email protected]>
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.
--- 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(-)
drivers/mtd/nand/gpmi-nand/gpmi-lib.c +35 -8
@@ -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. */
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +54 -9
84 lines not on this page · raw message
drivers/mtd/nand/gpmi-nand/gpmi-nand.h +2 -1
@@ -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 ]; };/**
Signature · 1 line
1.7.4.4
PATCH 3/3 mtd: gpmi: change the code for clocks
Quoted 151 lines · Huang Shijie
From: Huang Shijie <[email protected]>
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 <[email protected]>Signed-off-by Huang Shijie <[email protected]>--- 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(-)drivers/mtd/nand/gpmi-nand/gpmi-lib.c +35
-8@@ -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. */ drivers/mtd/nand/gpmi-nand/gpmi-nand.c +23
-0@@ -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 ;
drivers/mtd/nand/gpmi-nand/gpmi-nand.c acquire_err: · line 499
Quoted 10 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +10
-0+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.
drivers/mtd/nand/gpmi-nand/gpmi-nand.c acquire_err: · line 504
Quoted 5 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +5
-0+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
Quoted 69 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +16
-9+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);drivers/mtd/nand/gpmi-nand/gpmi-nand.h +2
-1@@ -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 ]; };/** Signature · 1 line
1.7.4.4
PATCH 3/3 mtd: gpmi: change the code for clocks
? 2012?06?29? 10:06, Shawn Guo ??:
Quoted 158 lines · Huang Shijie
Quoted 157 lines
From: Huang Shijie<[email protected]>
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<[email protected]>Signed-off-by Huang Shijie<[email protected]>--- 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(-)drivers/mtd/nand/gpmi-nand/gpmi-lib.c +35
-8@@ -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. */ drivers/mtd/nand/gpmi-nand/gpmi-nand.c +29
-0@@ -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;
Quoted 9 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c acquire_err: · line 499
Quoted 4 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +4
-0+ +/* 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_idand all those 5 clocks can match the same one on imx28.
thanks. got it.
Best Regards
Huang Shijie
Quoted 79 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c acquire_err: · line 504
Quoted 5 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +5
-0+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,
ShawnQuoted 69 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +16
-9+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);drivers/mtd/nand/gpmi-nand/gpmi-nand.h +2
-1@@ -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 ]; };/** Signature · 1 line
1.7.4.4
PATCH 3/3 mtd: gpmi: change the code for clocks
In reply to Shawn Guo? 2012?06?29? 10:06, Shawn Guo ??:
Quoted 59 lines
Quoted 59 lines
gpmi_regs+HW_GPMI_TIMING1);Quoted 58 lines
/* 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__devinitgpmi_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]))
+ gotoerr_clock;
+
+ /* Get extra clocks */
+ if (GPMI_IS_MX6Q(this))
+extra_clks=extra_clks_for_mx6q;
Quoted 5 lines
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_idand 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
In reply to Shawn GuoQuoted 159 lines · Shawn Guo
Quoted 158 lines · Huang Shijie
From: Huang Shijie <[email protected]>
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 <[email protected]>Signed-off-by Huang Shijie <[email protected]>--- 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(-)drivers/mtd/nand/gpmi-nand/gpmi-lib.c +35
-8@@ -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. */ drivers/mtd/nand/gpmi-nand/gpmi-nand.c +30
-0@@ -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; +
Quoted 9 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c acquire_err: · line 499
Quoted 3 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +3
-0+/* 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_idand 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
Hi,
Quoted 13 lines · Dong Aisheng
Quoted 12 lines · Shawn Guo
Quoted 11 lines · Huang Shijie
From: Huang Shijie <[email protected]>
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.
[...]
Quoted 38 lines
Quoted 15 lines
Quoted 15 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +15
-0+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; +Quoted 9 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c acquire_err: · line 499
Quoted 3 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +3
-0+/* 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_idand all those 5 clocks can match the same one on imx28.Will mx28 fail if doing like that?
clk_getwill 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); }
Quoted 6 lines
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
Signature · 9 lines
___________________________________________________________ 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
Quoted 57 lines · Lothar Wa?mann
Hi,
Quoted 13 lines · Dong Aisheng
Quoted 12 lines · Shawn Guo
Quoted 11 lines · Huang Shijie
From: Huang Shijie <[email protected]>
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.
[...]
Quoted 40 lines
Quoted 15 lines
Quoted 15 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +15
-0+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; +Quoted 9 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c acquire_err: · line 499
Quoted 3 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +3
-0+/* 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_idand all those 5 clocks can match the same one on imx28.Will mx28 fail if doing like that?
clk_getwill 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);
Quoted 7 lines
Quoted 4 lines
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.
Quoted 4 lines
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.
Quoted 4 lines
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
Hi,
Quoted 62 lines · Dong Aisheng
Quoted 61 lines · Lothar Wa?mann
Hi,
Quoted 13 lines · Dong Aisheng
Quoted 12 lines · Shawn Guo
Quoted 11 lines · Huang Shijie
From: Huang Shijie <[email protected]>
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.
[...]
Quoted 44 lines
Quoted 15 lines
Quoted 15 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +15
-0+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; +Quoted 9 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c acquire_err: · line 499
Quoted 3 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +3
-0+/* 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_idand all those 5 clocks can match the same one on imx28.Will mx28 fail if doing like that?
clk_getwill 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.
Quoted 6 lines
Quoted 3 lines
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
Signature · 9 lines
___________________________________________________________ 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
Quoted 65 lines · Lothar Wa?mann
Hi,
Quoted 62 lines · Dong Aisheng
Quoted 61 lines · Lothar Wa?mann
Hi,
Quoted 13 lines · Dong Aisheng
Quoted 12 lines · Shawn Guo
Quoted 11 lines · Huang Shijie
From: Huang Shijie <[email protected]>
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.
[...]
Quoted 44 lines
Quoted 15 lines
Quoted 15 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +15
-0+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; +Quoted 9 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c acquire_err: · line 499
Quoted 3 lines
drivers/mtd/nand/gpmi-nand/gpmi-nand.c +3
-0+/* 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_idand all those 5 clocks can match the same one on imx28.Will mx28 fail if doing like that?
clk_getwill 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.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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
Quoted 6 lines · Dong Aisheng
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
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.
Quoted 7 lines
Quoted 2 lines
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.
Signature · 2 lines
Regards, Shawn
PATCH 3/3 mtd: gpmi: change the code for clocks
Quoted 9 lines · Shawn Guo
Quoted 8 lines · Dong Aisheng
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
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!
Quoted 9 lines
Quoted 5 lines
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_idin 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
Quoted 8 lines · Dong Aisheng
Quoted 4 lines
I'm with Lothar on this. At least for imx28 case, we do not even need dummy clock, because NULL
con_idin 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 havecon_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.
Signature · 2 lines
Regards, Shawn
PATCH 3/3 mtd: gpmi: change the code for clocks
Quoted 16 lines · Shawn Guo
Quoted 8 lines · Dong Aisheng
Quoted 4 lines
I'm with Lothar on this. At least for imx28 case, we do not even need dummy clock, because NULL
con_idin 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 havecon_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", }, };
Quoted 5 lines
clk_register_clkdevs(clks[gpmi], gpmi_lookups, ARRAY_SIZE(gpmi_lookups));As you can see,
con_idis 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
Quoted 8 lines · Dong Aisheng
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" likeclk_get(dev, "gpmi-bch"), it may fail for imx28.
What I was saying is it will not fail.
Signature · 2 lines
Regards, Shawn
PATCH 3/3 mtd: gpmi: change the code for clocks
In reply to Lothar Waßmann? 2012?06?29? 17:44, Lothar Wa?mann ??:
Quoted 1 line
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
Quoted 5 lines · Huang Shijie
? 2012?06?29? 17:44, Lothar Wa?mann ??:
Quoted 1 line
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.
Signature · 2 lines
Regards, Shawn
PATCH 3/3 mtd: gpmi: change the code for clocks
In reply to Lothar WaßmannQuoted 3 lines · Lothar Wa??mann
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
In reply to Huang ShijieQuoted 7 lines · Huang Shijie
From: Huang Shijie <[email protected]>
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.
Quoted 6 lines
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 <[email protected]>Signed-off-by Huang Shijie <[email protected]>
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
Quoted 219 lines
--- 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(-)drivers/mtd/nand/gpmi-nand/gpmi-lib.c +35
-8@@ -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. */ drivers/mtd/nand/gpmi-nand/gpmi-nand.c +54
-9@@ -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);drivers/mtd/nand/gpmi-nand/gpmi-nand.h +2
-1@@ -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 ]; };/**