| Message ID | 1340941925-14591-1-git-send-email-shijie8@gmail.com |
|---|---|
| State | New |
| Headers | show |
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 >
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 >
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
于 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 >>
于 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
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.
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@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@02100000 { /* AIPS2 */ > ethernet@02188000 { > phy-mode = "rgmii"; Regards Dong Aisheng
于 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
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]; > }; > > /**
于 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
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
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
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
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
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
于 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
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.
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.
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
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.
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
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.
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.
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");