Hello,
This patch series prepares the transition to common clk framework by
1) replacing all the clk_enable and clk_disable calls by clk_prepare_unable and
clk_disable_unprepare to avoid common clock framework warnings.
2) adding explicit configuration of clk (set_rate) within drivers instead of
default configuration done in arch/arm/mach-at91/clock.c (udc and ohci
drivers)
If the second part of this patch series (the last 2 patches) does not belong
here, please tell me and I'll move it into the "move to common clk framework"
series.
Best Regards,
Boris
Changes since v2:
- add configuraton of usb clock in ohci and udc drivers
- add clk_prepare patch for usba udc driver
- add clk_prepare patch for spi driver
- remove already applied udc clk_prepare patch
Changes since v1:
- remove unneeded clk_unprepare in mci driver
- remove already applied patches (ehci, ohci, dma and serial)
Changes since common clk patch series:
- add clk_prepare_enable return check
- fix a deadlock in atmel-mci (missing spin_unlock)
- remove already applied patches (atmel-ssc and pwm-atmel-tcb)
Boris BREZILLON (7):
ARM: at91/tc/clocksource: replace clk_enable/disable with
clk_prepare_enable/disable_unprepare.
mmc: atmel-mci: prepare clk before calling enable
at91/avr32/atmel_lcdfb: prepare clk before calling enable
USB: gadget: atmel_usba: prepare clk before calling enable
spi: atmel: prepare clk before calling enable
USB: ohci-at91: add usb_clk for transition to common clk framework
usb: gadget: at91_udc: add usb_clk for transition to common clk
framework
drivers/clocksource/tcb_clksrc.c | 10 +++++-----
drivers/mmc/host/atmel-mci.c | 27 ++++++++++++++++++---------
drivers/spi/spi-atmel.c | 12 +++++++-----
drivers/usb/gadget/at91_udc.c | 12 ++++++++++++
drivers/usb/gadget/at91_udc.h | 2 +-
drivers/usb/gadget/atmel_usba_udc.c | 28 +++++++++++++++++++++-------
drivers/usb/host/ohci-at91.c | 18 +++++++++++++++---
drivers/video/atmel_lcdfb.c | 8 ++++----
8 files changed, 83 insertions(+), 34 deletions(-)
--
1.7.9.5
Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
avoid common clk framework warnings.
Signed-off-by: Boris BREZILLON <[email protected]>
Acked-by: Ludovic Desroches <[email protected]>
---
drivers/mmc/host/atmel-mci.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/mmc/host/atmel-mci.c b/drivers/mmc/host/atmel-mci.c
index bdb84da..4636af6 100644
--- a/drivers/mmc/host/atmel-mci.c
+++ b/drivers/mmc/host/atmel-mci.c
@@ -378,6 +378,8 @@ static int atmci_regs_show(struct seq_file *s, void *v)
{
struct atmel_mci *host = s->private;
u32 *buf;
+ int ret = 0;
+
buf = kmalloc(ATMCI_REGS_SIZE, GFP_KERNEL);
if (!buf)
@@ -389,9 +391,13 @@ static int atmci_regs_show(struct seq_file *s, void *v)
* consistent.
*/
spin_lock_bh(&host->lock);
- clk_enable(host->mck);
+ ret = clk_prepare_enable(host->mck);
+ if (ret) {
+ spin_unlock_bh(&host->lock);
+ goto out;
+ }
memcpy_fromio(buf, host->regs, ATMCI_REGS_SIZE);
- clk_disable(host->mck);
+ clk_disable_unprepare(host->mck);
spin_unlock_bh(&host->lock);
seq_printf(s, "MR:\t0x%08x%s%s ",
@@ -442,9 +448,10 @@ static int atmci_regs_show(struct seq_file *s, void *v)
val & ATMCI_CFG_LSYNC ? " LSYNC" : "");
}
+out:
kfree(buf);
- return 0;
+ return ret;
}
static int atmci_regs_open(struct inode *inode, struct file *file)
@@ -1279,7 +1286,7 @@ static void atmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
spin_lock_bh(&host->lock);
if (!host->mode_reg) {
- clk_enable(host->mck);
+ clk_prepare_enable(host->mck);
atmci_writel(host, ATMCI_CR, ATMCI_CR_SWRST);
atmci_writel(host, ATMCI_CR, ATMCI_CR_MCIEN);
if (host->caps.has_cfg_reg)
@@ -1359,7 +1366,7 @@ static void atmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
atmci_writel(host, ATMCI_CR, ATMCI_CR_MCIDIS);
if (host->mode_reg) {
atmci_readl(host, ATMCI_MR);
- clk_disable(host->mck);
+ clk_disable_unprepare(host->mck);
}
host->mode_reg = 0;
}
@@ -2376,10 +2383,12 @@ static int __init atmci_probe(struct platform_device *pdev)
if (!host->regs)
goto err_ioremap;
- clk_enable(host->mck);
+ ret = clk_prepare_enable(host->mck);
+ if (ret)
+ goto err_request_irq;
atmci_writel(host, ATMCI_CR, ATMCI_CR_SWRST);
host->bus_hz = clk_get_rate(host->mck);
- clk_disable(host->mck);
+ clk_disable_unprepare(host->mck);
host->mapbase = regs->start;
@@ -2482,11 +2491,11 @@ static int __exit atmci_remove(struct platform_device *pdev)
atmci_cleanup_slot(host->slot[i], i);
}
- clk_enable(host->mck);
+ clk_prepare_enable(host->mck);
atmci_writel(host, ATMCI_IDR, ~0UL);
atmci_writel(host, ATMCI_CR, ATMCI_CR_MCIDIS);
atmci_readl(host, ATMCI_SR);
- clk_disable(host->mck);
+ clk_disable_unprepare(host->mck);
if (host->dma.chan)
dma_release_channel(host->dma.chan);
--
1.7.9.5
Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
avoid common clk framework warnings.
Signed-off-by: Boris BREZILLON <[email protected]>
---
drivers/usb/gadget/atmel_usba_udc.c | 28 +++++++++++++++++++++-------
1 file changed, 21 insertions(+), 7 deletions(-)
diff --git a/drivers/usb/gadget/atmel_usba_udc.c b/drivers/usb/gadget/atmel_usba_udc.c
index 1d97222..f018017 100644
--- a/drivers/usb/gadget/atmel_usba_udc.c
+++ b/drivers/usb/gadget/atmel_usba_udc.c
@@ -1772,6 +1772,7 @@ out:
static int atmel_usba_start(struct usb_gadget *gadget,
struct usb_gadget_driver *driver)
{
+ int ret = 0;
struct usba_udc *udc = container_of(gadget, struct usba_udc, gadget);
unsigned long flags;
@@ -1781,8 +1782,14 @@ static int atmel_usba_start(struct usb_gadget *gadget,
udc->driver = driver;
spin_unlock_irqrestore(&udc->lock, flags);
- clk_enable(udc->pclk);
- clk_enable(udc->hclk);
+ ret = clk_prepare_enable(udc->pclk);
+ if (ret)
+ goto out;
+ ret = clk_prepare_enable(udc->hclk);
+ if (ret) {
+ clk_disable_unprepare(udc->pclk);
+ goto out;
+ }
DBG(DBG_GADGET, "registered driver `%s'\n", driver->driver.name);
@@ -1797,9 +1804,11 @@ static int atmel_usba_start(struct usb_gadget *gadget,
usba_writel(udc, CTRL, USBA_ENABLE_MASK);
usba_writel(udc, INT_ENB, USBA_END_OF_RESET);
}
+
+out:
spin_unlock_irqrestore(&udc->lock, flags);
- return 0;
+ return ret;
}
static int atmel_usba_stop(struct usb_gadget *gadget,
@@ -1822,8 +1831,8 @@ static int atmel_usba_stop(struct usb_gadget *gadget,
udc->driver = NULL;
- clk_disable(udc->hclk);
- clk_disable(udc->pclk);
+ clk_disable_unprepare(udc->hclk);
+ clk_disable_unprepare(udc->pclk);
DBG(DBG_GADGET, "unregistered driver `%s'\n", driver->driver.name);
@@ -2022,10 +2031,14 @@ static int __init usba_udc_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, udc);
/* Make sure we start from a clean slate */
- clk_enable(pclk);
+ ret = clk_prepare_enable(pclk);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to enable pclk, aborting.\n");
+ goto err_clk_enable;
+ }
toggle_bias(0);
usba_writel(udc, CTRL, USBA_DISABLE_MASK);
- clk_disable(pclk);
+ clk_disable_unprepare(pclk);
if (pdev->dev.of_node)
udc->usba_ep = atmel_udc_of_init(pdev, udc);
@@ -2081,6 +2094,7 @@ err_add_udc:
free_irq(irq, udc);
err_request_irq:
err_alloc_ep:
+err_clk_enable:
iounmap(udc->fifo);
err_map_fifo:
iounmap(udc->regs);
--
1.7.9.5
Dear Boris BREZILLON,
On Tue, 16 Jul 2013 17:06:48 +0200, Boris BREZILLON wrote:
> buf = kmalloc(ATMCI_REGS_SIZE, GFP_KERNEL);
> if (!buf)
> @@ -389,9 +391,13 @@ static int atmci_regs_show(struct seq_file *s, void *v)
> * consistent.
> */
> spin_lock_bh(&host->lock);
> - clk_enable(host->mck);
> + ret = clk_prepare_enable(host->mck);
I am not very familiar with the spin_lock_bh() variant, but are you
sure we are allowed to sleep within a spin_lock_bh()-protected critical
section?
Remember that clk_prepare_enable() calls both ->prepare() and
->enable() for the clock, and ->prepare() is allowed to sleep, while
->enable() is guaranteed not to sleep.
Therefore, clk_prepare() is usually called at probe time, while
clk_enable() is called whenever enabling/disabling the clock is really
needed. So not all clk_enable() can transparently be converted into a
clk_prepare_enable().
Best regards,
Thomas
--
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
avoid common clk framework warnings.
Signed-off-by: Boris BREZILLON <[email protected]>
Acked-by: Nicolas Ferre <[email protected]>
---
drivers/video/atmel_lcdfb.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/video/atmel_lcdfb.c b/drivers/video/atmel_lcdfb.c
index ece49d5..bf9c5d0 100644
--- a/drivers/video/atmel_lcdfb.c
+++ b/drivers/video/atmel_lcdfb.c
@@ -954,14 +954,14 @@ static int __init atmel_lcdfb_init_fbinfo(struct atmel_lcdfb_info *sinfo)
static void atmel_lcdfb_start_clock(struct atmel_lcdfb_info *sinfo)
{
- clk_enable(sinfo->bus_clk);
- clk_enable(sinfo->lcdc_clk);
+ clk_prepare_enable(sinfo->bus_clk);
+ clk_prepare_enable(sinfo->lcdc_clk);
}
static void atmel_lcdfb_stop_clock(struct atmel_lcdfb_info *sinfo)
{
- clk_disable(sinfo->bus_clk);
- clk_disable(sinfo->lcdc_clk);
+ clk_disable_unprepare(sinfo->bus_clk);
+ clk_disable_unprepare(sinfo->lcdc_clk);
}
#ifdef CONFIG_OF
--
1.7.9.5
Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
avoid common clk framework warnings.
Signed-off-by: Boris BREZILLON <[email protected]>
---
drivers/spi/spi-atmel.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index ea1ec00..4c6c455 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -1579,7 +1579,9 @@ static int atmel_spi_probe(struct platform_device *pdev)
goto out_unmap_regs;
/* Initialize the hardware */
- clk_enable(clk);
+ ret = clk_prepare_enable(clk);
+ if (ret)
+ goto out_unmap_regs;
spi_writel(as, CR, SPI_BIT(SWRST));
spi_writel(as, CR, SPI_BIT(SWRST)); /* AT91SAM9263 Rev B workaround */
if (as->caps.has_wdrbt) {
@@ -1609,7 +1611,7 @@ out_free_dma:
spi_writel(as, CR, SPI_BIT(SWRST));
spi_writel(as, CR, SPI_BIT(SWRST)); /* AT91SAM9263 Rev B workaround */
- clk_disable(clk);
+ clk_disable_unprepare(clk);
free_irq(irq, master);
out_unmap_regs:
iounmap(as->regs);
@@ -1661,7 +1663,7 @@ static int atmel_spi_remove(struct platform_device *pdev)
dma_free_coherent(&pdev->dev, BUFFER_SIZE, as->buffer,
as->buffer_dma);
- clk_disable(as->clk);
+ clk_disable_unprepare(as->clk);
clk_put(as->clk);
free_irq(as->irq, master);
iounmap(as->regs);
@@ -1678,7 +1680,7 @@ static int atmel_spi_suspend(struct platform_device *pdev, pm_message_t mesg)
struct spi_master *master = platform_get_drvdata(pdev);
struct atmel_spi *as = spi_master_get_devdata(master);
- clk_disable(as->clk);
+ clk_disable_unprepare(as->clk);
return 0;
}
@@ -1687,7 +1689,7 @@ static int atmel_spi_resume(struct platform_device *pdev)
struct spi_master *master = platform_get_drvdata(pdev);
struct atmel_spi *as = spi_master_get_devdata(master);
- clk_enable(as->clk);
+ return clk_prepare_enable(as->clk);
return 0;
}
--
1.7.9.5
Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
avoid common clk framework warnings.
Signed-off-by: Boris BREZILLON <[email protected]>
---
drivers/clocksource/tcb_clksrc.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
index 8a61872..229c019 100644
--- a/drivers/clocksource/tcb_clksrc.c
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -100,7 +100,7 @@ static void tc_mode(enum clock_event_mode m, struct clock_event_device *d)
|| tcd->clkevt.mode == CLOCK_EVT_MODE_ONESHOT) {
__raw_writel(0xff, regs + ATMEL_TC_REG(2, IDR));
__raw_writel(ATMEL_TC_CLKDIS, regs + ATMEL_TC_REG(2, CCR));
- clk_disable(tcd->clk);
+ clk_disable_unprepare(tcd->clk);
}
switch (m) {
@@ -109,7 +109,7 @@ static void tc_mode(enum clock_event_mode m, struct clock_event_device *d)
* of oneshot, we get lower overhead and improved accuracy.
*/
case CLOCK_EVT_MODE_PERIODIC:
- clk_enable(tcd->clk);
+ clk_prepare_enable(tcd->clk);
/* slow clock, count up to RC, then irq and restart */
__raw_writel(timer_clock
@@ -126,7 +126,7 @@ static void tc_mode(enum clock_event_mode m, struct clock_event_device *d)
break;
case CLOCK_EVT_MODE_ONESHOT:
- clk_enable(tcd->clk);
+ clk_prepare_enable(tcd->clk);
/* slow clock, count up to RC, then irq and stop */
__raw_writel(timer_clock | ATMEL_TC_CPCSTOP
@@ -275,7 +275,7 @@ static int __init tcb_clksrc_init(void)
pdev = tc->pdev;
t0_clk = tc->clk[0];
- clk_enable(t0_clk);
+ clk_prepare_enable(t0_clk);
/* How fast will we be counting? Pick something over 5 MHz. */
rate = (u32) clk_get_rate(t0_clk);
@@ -313,7 +313,7 @@ static int __init tcb_clksrc_init(void)
/* tclib will give us three clocks no matter what the
* underlying platform supports.
*/
- clk_enable(tc->clk[1]);
+ clk_prepare_enable(tc->clk[1]);
/* setup both channel 0 & 1 */
tcb_setup_dual_chan(tc, best_divisor_idx);
}
--
1.7.9.5
The AT91 PMC (Power Management Controller) provides an USB clock used by
USB Full Speed host (ohci) and USB Full Speed device (udc).
The usb drivers (ohci and udc) must configure this clock to 48Mhz.
This configuration was formely done in mach-at91/clock.c, but this
implementation will be removed when moving to common clk framework.
This patch add support for usb clock retrieval and configuration, and is
backward compatible with the current at91 clk implementation (if usb clk
is not found, it does not configure/enable the usb clk).
Signed-off-by: Boris BREZILLON <[email protected]>
---
drivers/usb/host/ohci-at91.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/host/ohci-at91.c b/drivers/usb/host/ohci-at91.c
index 9677f68..426c7d2 100644
--- a/drivers/usb/host/ohci-at91.c
+++ b/drivers/usb/host/ohci-at91.c
@@ -31,8 +31,8 @@
#define at91_for_each_port(index) \
for ((index) = 0; (index) < AT91_MAX_USBH_PORTS; (index)++)
-/* interface and function clocks; sometimes also an AHB clock */
-static struct clk *iclk, *fclk, *hclk;
+/* interface, function and usb clocks; sometimes also an AHB clock */
+static struct clk *iclk, *fclk, *uclk, *hclk;
static int clocked;
extern int usb_disabled(void);
@@ -41,6 +41,10 @@ extern int usb_disabled(void);
static void at91_start_clock(void)
{
+ if (uclk) {
+ clk_set_rate(uclk, 48000000);
+ clk_prepare_enable(uclk);
+ }
clk_prepare_enable(hclk);
clk_prepare_enable(iclk);
clk_prepare_enable(fclk);
@@ -52,6 +56,8 @@ static void at91_stop_clock(void)
clk_disable_unprepare(fclk);
clk_disable_unprepare(iclk);
clk_disable_unprepare(hclk);
+ if (uclk)
+ clk_disable_unprepare(uclk);
clocked = 0;
}
@@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
goto err2;
}
+ uclk = clk_get(&pdev->dev, "usb_clk");
+ if (IS_ERR(uclk)) {
+ uclk = NULL;
+ dev_warn(&pdev->dev, "failed to get usb_clk\n");
+ }
iclk = clk_get(&pdev->dev, "ohci_clk");
if (IS_ERR(iclk)) {
dev_err(&pdev->dev, "failed to get ohci_clk\n");
@@ -212,10 +223,11 @@ static void usb_hcd_at91_remove(struct usb_hcd *hcd,
release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
usb_put_hcd(hcd);
+ clk_put(uclk);
clk_put(hclk);
clk_put(fclk);
clk_put(iclk);
- fclk = iclk = hclk = NULL;
+ fclk = iclk = hclk = uclk = NULL;
dev_set_drvdata(&pdev->dev, NULL);
}
--
1.7.9.5
The AT91 PMC (Power Management Controller) provides an USB clock used by
USB Full Speed host (ohci) and USB Full Speed device (udc).
The usb drivers (ohci and udc) must configure this clock to 48Mhz.
This configuration was formely done in mach-at91/clock.c, but this
implementation will be removed when moving to common clk framework.
This patch add support for usb clock retrieval and configuration, and is
backward compatible with the current at91 clk implementation (if usb clk
is not found, it does not configure/enable the usb clk).
Signed-off-by: Boris BREZILLON <[email protected]>
---
drivers/usb/gadget/at91_udc.c | 12 ++++++++++++
drivers/usb/gadget/at91_udc.h | 2 +-
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/at91_udc.c b/drivers/usb/gadget/at91_udc.c
index fce8e4e..1306a02 100644
--- a/drivers/usb/gadget/at91_udc.c
+++ b/drivers/usb/gadget/at91_udc.c
@@ -870,6 +870,10 @@ static void clk_on(struct at91_udc *udc)
if (udc->clocked)
return;
udc->clocked = 1;
+ if (udc->uclk) {
+ clk_set_rate(udc->uclk, 48000000);
+ clk_prepare_enable(udc->uclk);
+ }
clk_prepare_enable(udc->iclk);
clk_prepare_enable(udc->fclk);
}
@@ -882,6 +886,8 @@ static void clk_off(struct at91_udc *udc)
udc->gadget.speed = USB_SPEED_UNKNOWN;
clk_disable_unprepare(udc->fclk);
clk_disable_unprepare(udc->iclk);
+ if (udc->uclk)
+ clk_disable_unprepare(udc->uclk);
}
/*
@@ -1781,6 +1787,10 @@ static int at91udc_probe(struct platform_device *pdev)
goto fail1;
}
+ udc->uclk = clk_get(dev, "usb_clk");
+ if (IS_ERR(udc->uclk))
+ udc->uclk = NULL;
+
/* don't do anything until we have both gadget driver and VBUS */
retval = clk_prepare_enable(udc->iclk);
if (retval)
@@ -1894,6 +1904,8 @@ static int __exit at91udc_remove(struct platform_device *pdev)
clk_put(udc->iclk);
clk_put(udc->fclk);
+ if (udc->uclk)
+ clk_put(udc->uclk);
return 0;
}
diff --git a/drivers/usb/gadget/at91_udc.h b/drivers/usb/gadget/at91_udc.h
index e647d1c..0175246 100644
--- a/drivers/usb/gadget/at91_udc.h
+++ b/drivers/usb/gadget/at91_udc.h
@@ -126,7 +126,7 @@ struct at91_udc {
unsigned active_suspend:1;
u8 addr;
struct at91_udc_data board;
- struct clk *iclk, *fclk;
+ struct clk *iclk, *fclk, *uclk;
struct platform_device *pdev;
struct proc_dir_entry *pde;
void __iomem *udp_baseaddr;
--
1.7.9.5
On 07/16/2013 05:05 PM, Boris BREZILLON wrote:
> Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
> avoid common clk framework warnings.
>
> Signed-off-by: Boris BREZILLON <[email protected]>
> ---
This patch is part of a series and the recipients are also Thomas and
John (clocksource maintainers).
Is this patch going through Nicolas's tree or Thomas's tree ?
Thanks
-- Daniel
> drivers/clocksource/tcb_clksrc.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/clocksource/tcb_clksrc.c b/drivers/clocksource/tcb_clksrc.c
> index 8a61872..229c019 100644
> --- a/drivers/clocksource/tcb_clksrc.c
> +++ b/drivers/clocksource/tcb_clksrc.c
> @@ -100,7 +100,7 @@ static void tc_mode(enum clock_event_mode m, struct clock_event_device *d)
> || tcd->clkevt.mode == CLOCK_EVT_MODE_ONESHOT) {
> __raw_writel(0xff, regs + ATMEL_TC_REG(2, IDR));
> __raw_writel(ATMEL_TC_CLKDIS, regs + ATMEL_TC_REG(2, CCR));
> - clk_disable(tcd->clk);
> + clk_disable_unprepare(tcd->clk);
> }
>
> switch (m) {
> @@ -109,7 +109,7 @@ static void tc_mode(enum clock_event_mode m, struct clock_event_device *d)
> * of oneshot, we get lower overhead and improved accuracy.
> */
> case CLOCK_EVT_MODE_PERIODIC:
> - clk_enable(tcd->clk);
> + clk_prepare_enable(tcd->clk);
>
> /* slow clock, count up to RC, then irq and restart */
> __raw_writel(timer_clock
> @@ -126,7 +126,7 @@ static void tc_mode(enum clock_event_mode m, struct clock_event_device *d)
> break;
>
> case CLOCK_EVT_MODE_ONESHOT:
> - clk_enable(tcd->clk);
> + clk_prepare_enable(tcd->clk);
>
> /* slow clock, count up to RC, then irq and stop */
> __raw_writel(timer_clock | ATMEL_TC_CPCSTOP
> @@ -275,7 +275,7 @@ static int __init tcb_clksrc_init(void)
> pdev = tc->pdev;
>
> t0_clk = tc->clk[0];
> - clk_enable(t0_clk);
> + clk_prepare_enable(t0_clk);
>
> /* How fast will we be counting? Pick something over 5 MHz. */
> rate = (u32) clk_get_rate(t0_clk);
> @@ -313,7 +313,7 @@ static int __init tcb_clksrc_init(void)
> /* tclib will give us three clocks no matter what the
> * underlying platform supports.
> */
> - clk_enable(tc->clk[1]);
> + clk_prepare_enable(tc->clk[1]);
> /* setup both channel 0 & 1 */
> tcb_setup_dual_chan(tc, best_divisor_idx);
> }
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
Hello Thomas,
On 16/07/2013 17:13, Thomas Petazzoni wrote:
> Dear Boris BREZILLON,
>
> On Tue, 16 Jul 2013 17:06:48 +0200, Boris BREZILLON wrote:
>
>> buf = kmalloc(ATMCI_REGS_SIZE, GFP_KERNEL);
>> if (!buf)
>> @@ -389,9 +391,13 @@ static int atmci_regs_show(struct seq_file *s, void *v)
>> * consistent.
>> */
>> spin_lock_bh(&host->lock);
>> - clk_enable(host->mck);
>> + ret = clk_prepare_enable(host->mck);
> I am not very familiar with the spin_lock_bh() variant, but are you
> sure we are allowed to sleep within a spin_lock_bh()-protected critical
> section?
>
> Remember that clk_prepare_enable() calls both ->prepare() and
> ->enable() for the clock, and ->prepare() is allowed to sleep, while
> ->enable() is guaranteed not to sleep.
>
> Therefore, clk_prepare() is usually called at probe time, while
> clk_enable() is called whenever enabling/disabling the clock is really
> needed. So not all clk_enable() can transparently be converted into a
> clk_prepare_enable().
You're absolutely right. We should not call clk_prepare/unprepare inside
a critical section,
as the prepare/unprepare callback may sleep.
In this particular case it won't hurt as the mci clk is a peripheral clk
which does not
implement the prepare callback (and as a result won't sleep).
Anyway, I will fix it.
What is the best approach to do so ?
1) call clk_prepare/unprepare in the probe/remove functions and call
clk_enable/disable
in resume/suspend functions
2) get clk_prepare_enable/disable_unprepare outside of the critical
sections (I don't think
there is any need for the mci host lock to be held when enabling
the clk,
clk framework already handle concurrent accesses to clks)
I will check other patches of this series to see if they introduce
similar issues.
> Best regards,
>
> Thomas
On Tue, 16 Jul 2013, Boris BREZILLON wrote:
> The AT91 PMC (Power Management Controller) provides an USB clock used by
> USB Full Speed host (ohci) and USB Full Speed device (udc).
> The usb drivers (ohci and udc) must configure this clock to 48Mhz.
> This configuration was formely done in mach-at91/clock.c, but this
> implementation will be removed when moving to common clk framework.
>
> This patch add support for usb clock retrieval and configuration, and is
> backward compatible with the current at91 clk implementation (if usb clk
> is not found, it does not configure/enable the usb clk).
But it does print a warning in the system log, right?
> @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
> goto err2;
> }
>
> + uclk = clk_get(&pdev->dev, "usb_clk");
> + if (IS_ERR(uclk)) {
> + uclk = NULL;
> + dev_warn(&pdev->dev, "failed to get usb_clk\n");
> + }
Is this really what you want for backward compatibility?
Alan Stern
Hello Alan,
On 16/07/2013 18:48, Alan Stern wrote:
> On Tue, 16 Jul 2013, Boris BREZILLON wrote:
>
>> The AT91 PMC (Power Management Controller) provides an USB clock used by
>> USB Full Speed host (ohci) and USB Full Speed device (udc).
>> The usb drivers (ohci and udc) must configure this clock to 48Mhz.
>> This configuration was formely done in mach-at91/clock.c, but this
>> implementation will be removed when moving to common clk framework.
>>
>> This patch add support for usb clock retrieval and configuration, and is
>> backward compatible with the current at91 clk implementation (if usb clk
>> is not found, it does not configure/enable the usb clk).
> But it does print a warning in the system log, right?
Yes it does.
>
>> @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
>> goto err2;
>> }
>>
>> + uclk = clk_get(&pdev->dev, "usb_clk");
>> + if (IS_ERR(uclk)) {
>> + uclk = NULL;
>> + dev_warn(&pdev->dev, "failed to get usb_clk\n");
>> + }
> Is this really what you want for backward compatibility?
Here are some proposition to remove the warning message:
1) replace it with a dev_info and change the message:
dev_info(&pdev->dev, "failed to get usb_clk (most likely using old
at91 clk implementation)\n");
2) drop the log and silently ignore the missing clk (I'm not a big fan
of this solution
as it may lead to some errors if we're using new clk implem and the
clock is really missing)
3) rework the current clk_set_rate function to accept clk_set_rate on
usb clk and add clk_lookup entries
for the usb clk (I'm not a big fan of this solution neither as this
modifications will only be used for a short time
until the transition to common clk framework is completed).
> Alan Stern
>
Thanks for your review.
Best Regards,
Boris
On Tue, Jul 16, 2013 at 05:22:15PM +0200, Boris BREZILLON wrote:
> @@ -41,6 +41,10 @@ extern int usb_disabled(void);
>
> static void at91_start_clock(void)
> {
> + if (uclk) {
if (!IS_ERR(uclk)) {
> + clk_set_rate(uclk, 48000000);
> + clk_prepare_enable(uclk);
> + }
> clk_prepare_enable(hclk);
> clk_prepare_enable(iclk);
> clk_prepare_enable(fclk);
> @@ -52,6 +56,8 @@ static void at91_stop_clock(void)
> clk_disable_unprepare(fclk);
> clk_disable_unprepare(iclk);
> clk_disable_unprepare(hclk);
> + if (uclk)
if (!IS_ERR(uclk))
> + clk_disable_unprepare(uclk);
> clocked = 0;
> }
>
> @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
> goto err2;
> }
>
> + uclk = clk_get(&pdev->dev, "usb_clk");
> + if (IS_ERR(uclk)) {
> + uclk = NULL;
Remove this line.
> + dev_warn(&pdev->dev, "failed to get usb_clk\n");
> + }
> iclk = clk_get(&pdev->dev, "ohci_clk");
> if (IS_ERR(iclk)) {
> dev_err(&pdev->dev, "failed to get ohci_clk\n");
> @@ -212,10 +223,11 @@ static void usb_hcd_at91_remove(struct usb_hcd *hcd,
> release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> usb_put_hcd(hcd);
>
What if uclk is NULL here?
if (!IS_ERR(uclk))
> + clk_put(uclk);
> clk_put(hclk);
> clk_put(fclk);
> clk_put(iclk);
> - fclk = iclk = hclk = NULL;
> + fclk = iclk = hclk = uclk = NULL;
Don't.
Range of invalid struct clk pointers: those which IS_ERR(clk) returns true.
Therefore, the range of valid struct clk pointers is: _____________________
(answer on a post card, stamped and addressed to...)
On Tue, Jul 16, 2013 at 05:06:48PM +0200, Boris BREZILLON wrote:
> @@ -389,9 +391,13 @@ static int atmci_regs_show(struct seq_file *s, void *v)
> * consistent.
> */
> spin_lock_bh(&host->lock);
> - clk_enable(host->mck);
> + ret = clk_prepare_enable(host->mck);
> + if (ret) {
> + spin_unlock_bh(&host->lock);
> + goto out;
> + }
NAK. This is buggy. clk_prepare() can sleep. Calling clk_prepare()
even via clk_prepare_enable() is a blatent bug.
> memcpy_fromio(buf, host->regs, ATMCI_REGS_SIZE);
> - clk_disable(host->mck);
> + clk_disable_unprepare(host->mck);
> spin_unlock_bh(&host->lock);
Now, given that the CLK API counts enables/disables, having the spin lock
around the clk API calls is utterly pointless. This should be:
ret = clk_prepare_enable(host->mck);
if (ret)
goto out;
spin_lock_bh(&host->lock);
memcpy_fromio(buf, host->regs, ATMCI_REGS_SIZE);
spin_unlock_bh(&host->lock);
clk_disable_unprepare(host->mclk);
or, if you really need to have the clock enabled/disabled within the
spinlock (very very very unlikely):
ret = clk_prepare(host->mck);
if (ret)
goto out;
spin_lock_bh(&host->lock);
ret = clk_enable(host->mck);
if (ret == 0) {
memcpy_fromio(buf, host->regs, ATMCI_REGS_SIZE);
clk_disable(host->mck);
}
spin_unlock_bh(&host->lock);
clk_unprepare(host->mclk);
if (ret)
goto out;
> @@ -1279,7 +1286,7 @@ static void atmci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
> spin_lock_bh(&host->lock);
> if (!host->mode_reg) {
> - clk_enable(host->mck);
> + clk_prepare_enable(host->mck);
Again, buggy - calling clk_prepare beneath a spinlock is illegal.
On Tue, 16 Jul 2013, boris brezillon wrote:
> >> + uclk = clk_get(&pdev->dev, "usb_clk");
> >> + if (IS_ERR(uclk)) {
> >> + uclk = NULL;
> >> + dev_warn(&pdev->dev, "failed to get usb_clk\n");
> >> + }
> > Is this really what you want for backward compatibility?
> Here are some proposition to remove the warning message:
>
> 1) replace it with a dev_info and change the message:
> dev_info(&pdev->dev, "failed to get usb_clk (most likely using old
> at91 clk implementation)\n");
> 2) drop the log and silently ignore the missing clk (I'm not a big fan
> of this solution
> as it may lead to some errors if we're using new clk implem and the
> clock is really missing)
> 3) rework the current clk_set_rate function to accept clk_set_rate on
> usb clk and add clk_lookup entries
> for the usb clk (I'm not a big fan of this solution neither as this
> modifications will only be used for a short time
> until the transition to common clk framework is completed).
Another possibility is to combine this change with the clock
implementation update, and do them in a single patch. Then backward
compatibility would not be an issue.
Alan Stern
On 16/07/2013 20:47, Alan Stern wrote:
> On Tue, 16 Jul 2013, boris brezillon wrote:
>
>>>> + uclk = clk_get(&pdev->dev, "usb_clk");
>>>> + if (IS_ERR(uclk)) {
>>>> + uclk = NULL;
>>>> + dev_warn(&pdev->dev, "failed to get usb_clk\n");
>>>> + }
>>> Is this really what you want for backward compatibility?
>> Here are some proposition to remove the warning message:
>>
>> 1) replace it with a dev_info and change the message:
>> dev_info(&pdev->dev, "failed to get usb_clk (most likely using old
>> at91 clk implementation)\n");
>> 2) drop the log and silently ignore the missing clk (I'm not a big fan
>> of this solution
>> as it may lead to some errors if we're using new clk implem and the
>> clock is really missing)
>> 3) rework the current clk_set_rate function to accept clk_set_rate on
>> usb clk and add clk_lookup entries
>> for the usb clk (I'm not a big fan of this solution neither as this
>> modifications will only be used for a short time
>> until the transition to common clk framework is completed).
> Another possibility is to combine this change with the clock
> implementation update, and do them in a single patch. Then backward
> compatibility would not be an issue.
Yes, that was one of the question I asked in the cover-letter.
I think I'll move these patches in the "move to common clk" series.
Thanks
> Alan Stern
>
On Tue, Jul 16, 2013 at 05:16:22PM +0200, Boris BREZILLON wrote:
> Replace clk_enable/disable with clk_prepare_enable/disable_unprepare to
> avoid common clk framework warnings.
Applied, thanks.
On Tue, 16 Jul 2013, Boris BREZILLON wrote:
> The AT91 PMC (Power Management Controller) provides an USB clock used by
> USB Full Speed host (ohci) and USB Full Speed device (udc).
> The usb drivers (ohci and udc) must configure this clock to 48Mhz.
> This configuration was formely done in mach-at91/clock.c, but this
> implementation will be removed when moving to common clk framework.
>
> This patch add support for usb clock retrieval and configuration, and is
> backward compatible with the current at91 clk implementation (if usb clk
> is not found, it does not configure/enable the usb clk).
This does not take into account any of the changes you discussed with
Russell King and me -- it is exactly the same as the previous version.
> @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
> goto err2;
> }
>
> + uclk = clk_get(&pdev->dev, "usb_clk");
> + if (IS_ERR(uclk)) {
> + uclk = NULL;
> + dev_warn(&pdev->dev, "failed to get usb_clk\n");
> + }
> iclk = clk_get(&pdev->dev, "ohci_clk");
> if (IS_ERR(iclk)) {
> dev_err(&pdev->dev, "failed to get ohci_clk\n");
> @@ -212,10 +223,11 @@ static void usb_hcd_at91_remove(struct usb_hcd *hcd,
> release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
> usb_put_hcd(hcd);
>
> + clk_put(uclk);
What will clk_put() do when uclk is NULL?
Alan Stern
On 17/07/2013 17:33, Alan Stern wrote:
> On Tue, 16 Jul 2013, Boris BREZILLON wrote:
>
>> The AT91 PMC (Power Management Controller) provides an USB clock used by
>> USB Full Speed host (ohci) and USB Full Speed device (udc).
>> The usb drivers (ohci and udc) must configure this clock to 48Mhz.
>> This configuration was formely done in mach-at91/clock.c, but this
>> implementation will be removed when moving to common clk framework.
>>
>> This patch add support for usb clock retrieval and configuration, and is
>> backward compatible with the current at91 clk implementation (if usb clk
>> is not found, it does not configure/enable the usb clk).
> This does not take into account any of the changes you discussed with
> Russell King and me -- it is exactly the same as the previous version.
Sorry, I don't understand. I didn't send any new version since yesterday.
I'm sending it right now and it's part of the
"ARM: at91: move to common clk framework" series (as discussed with you).
>> @@ -144,6 +150,11 @@ static int usb_hcd_at91_probe(const struct hc_driver *driver,
>> goto err2;
>> }
>>
>> + uclk = clk_get(&pdev->dev, "usb_clk");
>> + if (IS_ERR(uclk)) {
>> + uclk = NULL;
>> + dev_warn(&pdev->dev, "failed to get usb_clk\n");
>> + }
>> iclk = clk_get(&pdev->dev, "ohci_clk");
>> if (IS_ERR(iclk)) {
>> dev_err(&pdev->dev, "failed to get ohci_clk\n");
>> @@ -212,10 +223,11 @@ static void usb_hcd_at91_remove(struct usb_hcd *hcd,
>> release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
>> usb_put_hcd(hcd);
>>
>> + clk_put(uclk);
> What will clk_put() do when uclk is NULL?
>
> Alan Stern
>
On Wed, 17 Jul 2013, boris brezillon wrote:
> On 17/07/2013 17:33, Alan Stern wrote:
> > On Tue, 16 Jul 2013, Boris BREZILLON wrote:
> >
> >> The AT91 PMC (Power Management Controller) provides an USB clock used by
> >> USB Full Speed host (ohci) and USB Full Speed device (udc).
> >> The usb drivers (ohci and udc) must configure this clock to 48Mhz.
> >> This configuration was formely done in mach-at91/clock.c, but this
> >> implementation will be removed when moving to common clk framework.
> >>
> >> This patch add support for usb clock retrieval and configuration, and is
> >> backward compatible with the current at91 clk implementation (if usb clk
> >> is not found, it does not configure/enable the usb clk).
> > This does not take into account any of the changes you discussed with
> > Russell King and me -- it is exactly the same as the previous version.
> Sorry, I don't understand. I didn't send any new version since yesterday.
Oh. Never mind. That message was the _same_ one that I replied to
yesterday. I got two copies of it, because you sent it both to me
directly and to linux-usb. For some reason one of the copies was
delayed for many hours, so I thought it was a new message.
Alan Stern