| Message ID | 1335935266-25289-2-git-send-email-thomas.abraham@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Wed, May 2, 2012 at 6:07 AM, Thomas Abraham <thomas.abraham@linaro.org> wrote: > Some platforms allow for clock gating and control of bus interface unit clock > and card interface unit clock. Add support for clock lookup of optional biu > and ciu clocks for clock gating and clock speed determination. > > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > --- > drivers/mmc/host/dw_mmc.c | 35 +++++++++++++++++++++++++++++++---- > include/linux/mmc/dw_mmc.h | 4 ++++ > 2 files changed, 35 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 1532357..036846f 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host) > return -ENODEV; > } > > - if (!host->pdata->bus_hz) { > + host->biu_clk = clk_get(&host->dev, "biu"); > + if (IS_ERR(host->biu_clk)) > + dev_info(&host->dev, "biu clock not available\n"); > + else > + clk_enable(host->biu_clk); > + > + host->ciu_clk = clk_get(&host->dev, "ciu"); > + if (IS_ERR(host->ciu_clk)) > + dev_info(&host->dev, "ciu clock not available\n"); Should these printks be at debug level? I'm not 100% sure either way but it could be a little noisy on systems that do not use these clocks. > + else > + clk_enable(host->ciu_clk); > + > + if (IS_ERR(host->ciu_clk)) > + host->bus_hz = host->pdata->bus_hz; > + else > + host->bus_hz = clk_get_rate(host->ciu_clk); > + > + if (!host->bus_hz) { > dev_err(&host->dev, > "Platform data must supply bus speed\n"); > - return -ENODEV; > + ret = -ENODEV; > + goto err_clk; > } > > - host->bus_hz = host->pdata->bus_hz; > host->quirks = host->pdata->quirks; > > spin_lock_init(&host->lock); > INIT_LIST_HEAD(&host->queue); > > - > host->dma_ops = host->pdata->dma_ops; > dw_mci_init_dma(host); > > @@ -2095,6 +2111,13 @@ err_dmaunmap: > regulator_disable(host->vmmc); > regulator_put(host->vmmc); > } > + kfree(host); > + > +err_clk: > + clk_disable(host->ciu_clk); > + clk_disable(host->biu_clk); > + clk_put(host->ciu_clk); > + clk_put(host->biu_clk); Are these calls safe, or do we need to check IS_ERR as above? > return ret; > } > EXPORT_SYMBOL(dw_mci_probe); > @@ -2128,6 +2151,10 @@ void dw_mci_remove(struct dw_mci *host) > regulator_put(host->vmmc); > } > > + clk_disable(host->ciu_clk); > + clk_disable(host->biu_clk); > + clk_put(host->ciu_clk); > + clk_put(host->biu_clk); Likewise. > } > EXPORT_SYMBOL(dw_mci_remove); > > diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h > index 7a7ebd3..fa9a139 100644 > --- a/include/linux/mmc/dw_mmc.h > +++ b/include/linux/mmc/dw_mmc.h > @@ -78,6 +78,8 @@ struct mmc_data; > * @data_offset: Set the offset of DATA register according to VERID. > * @dev: Device associated with the MMC controller. > * @pdata: Platform data associated with the MMC controller. > + * @biu_clk: Pointer to bus interface unit clock instance. > + * @ciu_clk: Pointer to card interface unit clock instance. > * @slot: Slots sharing this MMC controller. > * @fifo_depth: depth of FIFO. > * @data_shift: log2 of FIFO item size. > @@ -158,6 +160,8 @@ struct dw_mci { > u16 data_offset; > struct device dev; > struct dw_mci_board *pdata; > + struct clk *biu_clk; > + struct clk *ciu_clk; > struct dw_mci_slot *slot[MAX_MCI_SLOTS]; > > /* FIFO push and pull */ > -- > 1.7.5.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On Tue, May 01, 2012 at 10:07:40PM -0700, Thomas Abraham wrote: > Some platforms allow for clock gating and control of bus interface unit clock > and card interface unit clock. Add support for clock lookup of optional biu > and ciu clocks for clock gating and clock speed determination. As we're moving progressively towards the common clk API, which requires the new clk_prepare/clk_unprepare calls, new code should be using the dated API. Please update this to do so (eg, by using clk_prepare_enable() instead of clk_enable() for the simple solution.) Better solutions involve decoupling the two and only using clk_enable()/clk_disable() where you really need the clock to be on.
Hi On 2 May 2012 06:07, Thomas Abraham <thomas.abraham@linaro.org> wrote: > Some platforms allow for clock gating and control of bus interface unit clock > and card interface unit clock. Add support for clock lookup of optional biu > and ciu clocks for clock gating and clock speed determination. > > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > --- > drivers/mmc/host/dw_mmc.c | 35 +++++++++++++++++++++++++++++++---- > include/linux/mmc/dw_mmc.h | 4 ++++ > 2 files changed, 35 insertions(+), 4 deletions(-) > > diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c > index 1532357..036846f 100644 > --- a/drivers/mmc/host/dw_mmc.c > +++ b/drivers/mmc/host/dw_mmc.c > @@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host) > return -ENODEV; > } > > - if (!host->pdata->bus_hz) { > + host->biu_clk = clk_get(&host->dev, "biu"); These clock names sound quite platform specific (what if they're called something else on another platform, or another platform has separate ones for different instantiations of the block?). Perhaps the clk names should get passed in through platform data. I haven't looked how other drivers handle that though. > + if (IS_ERR(host->biu_clk)) > + dev_info(&host->dev, "biu clock not available\n"); In this case, should it set host->biu_clk to NULL or are clk_disable and clk_put guaranteed to handle an IS_ERR value? > + else > + clk_enable(host->biu_clk); > + > + host->ciu_clk = clk_get(&host->dev, "ciu"); > + if (IS_ERR(host->ciu_clk)) > + dev_info(&host->dev, "ciu clock not available\n"); same here > + else > + clk_enable(host->ciu_clk); > + > + if (IS_ERR(host->ciu_clk)) > + host->bus_hz = host->pdata->bus_hz; > + else > + host->bus_hz = clk_get_rate(host->ciu_clk); > + > + if (!host->bus_hz) { > dev_err(&host->dev, > "Platform data must supply bus speed\n"); > - return -ENODEV; > + ret = -ENODEV; > + goto err_clk; > } > > - host->bus_hz = host->pdata->bus_hz; > host->quirks = host->pdata->quirks; > > spin_lock_init(&host->lock); > INIT_LIST_HEAD(&host->queue); > > - > host->dma_ops = host->pdata->dma_ops; > dw_mci_init_dma(host); > > @@ -2095,6 +2111,13 @@ err_dmaunmap: > regulator_disable(host->vmmc); > regulator_put(host->vmmc); > } > + kfree(host); what's this about? > + > +err_clk: > + clk_disable(host->ciu_clk); > + clk_disable(host->biu_clk); > + clk_put(host->ciu_clk); > + clk_put(host->biu_clk); > return ret; > } > EXPORT_SYMBOL(dw_mci_probe); > @@ -2128,6 +2151,10 @@ void dw_mci_remove(struct dw_mci *host) > regulator_put(host->vmmc); > } > > + clk_disable(host->ciu_clk); > + clk_disable(host->biu_clk); > + clk_put(host->ciu_clk); > + clk_put(host->biu_clk); > } > EXPORT_SYMBOL(dw_mci_remove); > > diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h > index 7a7ebd3..fa9a139 100644 > --- a/include/linux/mmc/dw_mmc.h > +++ b/include/linux/mmc/dw_mmc.h > @@ -78,6 +78,8 @@ struct mmc_data; > * @data_offset: Set the offset of DATA register according to VERID. > * @dev: Device associated with the MMC controller. > * @pdata: Platform data associated with the MMC controller. > + * @biu_clk: Pointer to bus interface unit clock instance. > + * @ciu_clk: Pointer to card interface unit clock instance. > * @slot: Slots sharing this MMC controller. > * @fifo_depth: depth of FIFO. > * @data_shift: log2 of FIFO item size. > @@ -158,6 +160,8 @@ struct dw_mci { > u16 data_offset; > struct device dev; > struct dw_mci_board *pdata; > + struct clk *biu_clk; > + struct clk *ciu_clk; > struct dw_mci_slot *slot[MAX_MCI_SLOTS]; > > /* FIFO push and pull */ > -- > 1.7.5.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On 2 May 2012 14:47, Will Newton <will.newton@gmail.com> wrote: > On Wed, May 2, 2012 at 6:07 AM, Thomas Abraham > <thomas.abraham@linaro.org> wrote: >> Some platforms allow for clock gating and control of bus interface unit clock >> and card interface unit clock. Add support for clock lookup of optional biu >> and ciu clocks for clock gating and clock speed determination. >> >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> >> --- >> drivers/mmc/host/dw_mmc.c | 35 +++++++++++++++++++++++++++++++---- >> include/linux/mmc/dw_mmc.h | 4 ++++ >> 2 files changed, 35 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 1532357..036846f 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host) >> return -ENODEV; >> } >> >> - if (!host->pdata->bus_hz) { >> + host->biu_clk = clk_get(&host->dev, "biu"); >> + if (IS_ERR(host->biu_clk)) >> + dev_info(&host->dev, "biu clock not available\n"); >> + else >> + clk_enable(host->biu_clk); >> + >> + host->ciu_clk = clk_get(&host->dev, "ciu"); >> + if (IS_ERR(host->ciu_clk)) >> + dev_info(&host->dev, "ciu clock not available\n"); > > Should these printks be at debug level? I'm not 100% sure either way > but it could be a little noisy on systems that do not use these > clocks. Right. I will change dev_info to dev_dbg. > >> + else >> + clk_enable(host->ciu_clk); >> + >> + if (IS_ERR(host->ciu_clk)) >> + host->bus_hz = host->pdata->bus_hz; >> + else >> + host->bus_hz = clk_get_rate(host->ciu_clk); >> + >> + if (!host->bus_hz) { >> dev_err(&host->dev, >> "Platform data must supply bus speed\n"); >> - return -ENODEV; >> + ret = -ENODEV; >> + goto err_clk; >> } >> >> - host->bus_hz = host->pdata->bus_hz; >> host->quirks = host->pdata->quirks; >> >> spin_lock_init(&host->lock); >> INIT_LIST_HEAD(&host->queue); >> >> - >> host->dma_ops = host->pdata->dma_ops; >> dw_mci_init_dma(host); >> >> @@ -2095,6 +2111,13 @@ err_dmaunmap: >> regulator_disable(host->vmmc); >> regulator_put(host->vmmc); >> } >> + kfree(host); >> + >> +err_clk: >> + clk_disable(host->ciu_clk); >> + clk_disable(host->biu_clk); >> + clk_put(host->ciu_clk); >> + clk_put(host->biu_clk); > > Are these calls safe, or do we need to check IS_ERR as above? The clk_disable is safe on Samsung platforms but it cannot be assumed for other platforms. So, the IS_ERR check will be added before clk_disable. clk_put will not need the IS_ERR check. Thanks for pointing this out. > >> return ret; >> } >> EXPORT_SYMBOL(dw_mci_probe); >> @@ -2128,6 +2151,10 @@ void dw_mci_remove(struct dw_mci *host) >> regulator_put(host->vmmc); >> } >> >> + clk_disable(host->ciu_clk); >> + clk_disable(host->biu_clk); >> + clk_put(host->ciu_clk); >> + clk_put(host->biu_clk); > > Likewise. Yes, will be fixed. Thanks, Thomas. [...]
On 2 May 2012 15:23, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, May 01, 2012 at 10:07:40PM -0700, Thomas Abraham wrote: >> Some platforms allow for clock gating and control of bus interface unit clock >> and card interface unit clock. Add support for clock lookup of optional biu >> and ciu clocks for clock gating and clock speed determination. > > As we're moving progressively towards the common clk API, which requires > the new clk_prepare/clk_unprepare calls, new code should be using the > dated API. Please update this to do so (eg, by using clk_prepare_enable() > instead of clk_enable() for the simple solution.) Better solutions > involve decoupling the two and only using clk_enable()/clk_disable() where > you really need the clock to be on. Thanks for your suggestion. I will modify this to use the clk_prepare_enable and clk_disable_unprepare calls for now. Thanks, Thomas.
On 2 May 2012 20:23, James Hogan <james@albanarts.com> wrote: > Hi > > On 2 May 2012 06:07, Thomas Abraham <thomas.abraham@linaro.org> wrote: >> Some platforms allow for clock gating and control of bus interface unit clock >> and card interface unit clock. Add support for clock lookup of optional biu >> and ciu clocks for clock gating and clock speed determination. >> >> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> >> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> >> --- >> drivers/mmc/host/dw_mmc.c | 35 +++++++++++++++++++++++++++++++---- >> include/linux/mmc/dw_mmc.h | 4 ++++ >> 2 files changed, 35 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c >> index 1532357..036846f 100644 >> --- a/drivers/mmc/host/dw_mmc.c >> +++ b/drivers/mmc/host/dw_mmc.c >> @@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host) >> return -ENODEV; >> } >> >> - if (!host->pdata->bus_hz) { >> + host->biu_clk = clk_get(&host->dev, "biu"); > > These clock names sound quite platform specific (what if they're > called something else on another platform, or another platform has > separate ones for different instantiations of the block?). Perhaps the > clk names should get passed in through platform data. I haven't looked > how other drivers handle that though. The clock names 'biu' and 'ciu' are derived from the terminology used by the data sheet of the mshc controller. The 'biu' clock is the source clock for the bus interface unit and 'ciu' clock is the clock source for card interface unit of the controller. So these names are generic and not specific to a platform. Passing clock names from platform data is generally frowned upon. > >> + if (IS_ERR(host->biu_clk)) >> + dev_info(&host->dev, "biu clock not available\n"); > > In this case, should it set host->biu_clk to NULL or are clk_disable > and clk_put guaranteed to handle an IS_ERR value? Yes, the clk_disable will have to be preceded with a IS_ERR check. I will fix this. > >> + else >> + clk_enable(host->biu_clk); >> + >> + host->ciu_clk = clk_get(&host->dev, "ciu"); >> + if (IS_ERR(host->ciu_clk)) >> + dev_info(&host->dev, "ciu clock not available\n"); > > same here Ok, this also will be fixed. > >> + else >> + clk_enable(host->ciu_clk); >> + >> + if (IS_ERR(host->ciu_clk)) >> + host->bus_hz = host->pdata->bus_hz; >> + else >> + host->bus_hz = clk_get_rate(host->ciu_clk); >> + >> + if (!host->bus_hz) { >> dev_err(&host->dev, >> "Platform data must supply bus speed\n"); >> - return -ENODEV; >> + ret = -ENODEV; >> + goto err_clk; >> } >> >> - host->bus_hz = host->pdata->bus_hz; >> host->quirks = host->pdata->quirks; >> >> spin_lock_init(&host->lock); >> INIT_LIST_HEAD(&host->queue); >> >> - >> host->dma_ops = host->pdata->dma_ops; >> dw_mci_init_dma(host); >> >> @@ -2095,6 +2111,13 @@ err_dmaunmap: >> regulator_disable(host->vmmc); >> regulator_put(host->vmmc); >> } >> + kfree(host); > > what's this about? This is wrong. It should not have been here. I will fix this. Thanks for pointing this out. Thanks, Thomas.
diff --git a/drivers/mmc/host/dw_mmc.c b/drivers/mmc/host/dw_mmc.c index 1532357..036846f 100644 --- a/drivers/mmc/host/dw_mmc.c +++ b/drivers/mmc/host/dw_mmc.c @@ -1938,19 +1938,35 @@ int dw_mci_probe(struct dw_mci *host) return -ENODEV; } - if (!host->pdata->bus_hz) { + host->biu_clk = clk_get(&host->dev, "biu"); + if (IS_ERR(host->biu_clk)) + dev_info(&host->dev, "biu clock not available\n"); + else + clk_enable(host->biu_clk); + + host->ciu_clk = clk_get(&host->dev, "ciu"); + if (IS_ERR(host->ciu_clk)) + dev_info(&host->dev, "ciu clock not available\n"); + else + clk_enable(host->ciu_clk); + + if (IS_ERR(host->ciu_clk)) + host->bus_hz = host->pdata->bus_hz; + else + host->bus_hz = clk_get_rate(host->ciu_clk); + + if (!host->bus_hz) { dev_err(&host->dev, "Platform data must supply bus speed\n"); - return -ENODEV; + ret = -ENODEV; + goto err_clk; } - host->bus_hz = host->pdata->bus_hz; host->quirks = host->pdata->quirks; spin_lock_init(&host->lock); INIT_LIST_HEAD(&host->queue); - host->dma_ops = host->pdata->dma_ops; dw_mci_init_dma(host); @@ -2095,6 +2111,13 @@ err_dmaunmap: regulator_disable(host->vmmc); regulator_put(host->vmmc); } + kfree(host); + +err_clk: + clk_disable(host->ciu_clk); + clk_disable(host->biu_clk); + clk_put(host->ciu_clk); + clk_put(host->biu_clk); return ret; } EXPORT_SYMBOL(dw_mci_probe); @@ -2128,6 +2151,10 @@ void dw_mci_remove(struct dw_mci *host) regulator_put(host->vmmc); } + clk_disable(host->ciu_clk); + clk_disable(host->biu_clk); + clk_put(host->ciu_clk); + clk_put(host->biu_clk); } EXPORT_SYMBOL(dw_mci_remove); diff --git a/include/linux/mmc/dw_mmc.h b/include/linux/mmc/dw_mmc.h index 7a7ebd3..fa9a139 100644 --- a/include/linux/mmc/dw_mmc.h +++ b/include/linux/mmc/dw_mmc.h @@ -78,6 +78,8 @@ struct mmc_data; * @data_offset: Set the offset of DATA register according to VERID. * @dev: Device associated with the MMC controller. * @pdata: Platform data associated with the MMC controller. + * @biu_clk: Pointer to bus interface unit clock instance. + * @ciu_clk: Pointer to card interface unit clock instance. * @slot: Slots sharing this MMC controller. * @fifo_depth: depth of FIFO. * @data_shift: log2 of FIFO item size. @@ -158,6 +160,8 @@ struct dw_mci { u16 data_offset; struct device dev; struct dw_mci_board *pdata; + struct clk *biu_clk; + struct clk *ciu_clk; struct dw_mci_slot *slot[MAX_MCI_SLOTS]; /* FIFO push and pull */