[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs
Daniel Lezcano
daniel.lezcano at linaro.org
Wed Sep 25 10:33:24 EDT 2013
On 09/16/2013 11:44 AM, Uwe Kleine-König wrote:
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig at pengutronix.de>
> ---
> Hello,
>
> I'm not sure that the way I implemented if a given timer is used as
> clock_source or clock_event_device is robust. Does it need locking?
> The reason to create a timer device for each timer instead of a single
> device of all of them is that it makes it cleaner to specify irqs and
> clks which each timer has one of each respectively. I didn't find an
> example, but while looking I wondered if in zevio-timer.c a single timer
> can really support both clock_event and clocksource.
>
> I guess for inclusion I need to write a document describing the
> of-binding. I will include that in the next iteration.
Right and a nice description of the timer would be valuable.
The first letter of the description should be in capital "Provide".
> checkpatch wails that the description of CLKSRC_EFM32 is too short. I
> think it's OK though.
>
> Best regards
> Uwe
>
> drivers/clocksource/Kconfig | 8 ++
> drivers/clocksource/Makefile | 1 +
> drivers/clocksource/time-efm32.c | 274 +++++++++++++++++++++++++++++++++++++++
> 3 files changed, 283 insertions(+)
> create mode 100644 drivers/clocksource/time-efm32.c
>
> diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> index 41c6946..410b152 100644
> --- a/drivers/clocksource/Kconfig
> +++ b/drivers/clocksource/Kconfig
> @@ -70,6 +70,14 @@ config CLKSRC_DBX500_PRCMU_SCHED_CLOCK
> help
> Use the always on PRCMU Timer as sched_clock
>
> +config CLKSRC_EFM32
> + bool "Clocksource for Energy Micro's EFM32 SoCs" if !ARCH_EFM32
> + depends on OF && ARM && (ARCH_EFM32 || COMPILE_TEST)
> + default ARCH_EFM32
> + help
> + Support to use the timers of EFM32 SoCs as clock source and clock
> + event device.
> +
No option for the timer. It must be selected by the platform.
> config ARM_ARCH_TIMER
> bool
> select CLKSRC_OF if OF
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 704d6d3..33621ef 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_VT8500_TIMER) += vt8500_timer.o
> obj-$(CONFIG_ARCH_NSPIRE) += zevio-timer.o
> obj-$(CONFIG_ARCH_BCM) += bcm_kona_timer.o
> obj-$(CONFIG_CADENCE_TTC_TIMER) += cadence_ttc_timer.o
> +obj-$(CONFIG_CLKSRC_EFM32) += time-efm32.o
> obj-$(CONFIG_CLKSRC_EXYNOS_MCT) += exynos_mct.o
> obj-$(CONFIG_CLKSRC_SAMSUNG_PWM) += samsung_pwm_timer.o
> obj-$(CONFIG_VF_PIT_TIMER) += vf_pit_timer.o
> diff --git a/drivers/clocksource/time-efm32.c b/drivers/clocksource/time-efm32.c
> new file mode 100644
> index 0000000..6ead8d7
> --- /dev/null
> +++ b/drivers/clocksource/time-efm32.c
> @@ -0,0 +1,274 @@
> +/*
> + * Copyright (C) 2013 Pengutronix
> + * Uwe Kleine-Koenig <u.kleine-koenig at pengutronix.de>
> + *
> + * This program is free software; you can redistribute it and/or modify it under
> + * the terms of the GNU General Public License version 2 as published by the
> + * Free Software Foundation.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/clocksource.h>
> +#include <linux/clockchips.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/clk.h>
> +
> +#define TIMERn_CTRL 0x00
> +#define TIMERn_CTRL_PRESC(val) (((val) & 0xf) << 24)
You can replace all binary shift with the BIT macro.
> +#define TIMERn_CTRL_PRESC_1024 TIMERn_CTRL_PRESC(10)
> +#define TIMERn_CTRL_CLKSEL(val) (((val) & 0x3) << 16)
> +#define TIMERn_CTRL_CLKSEL_PRESCHFPERCLK TIMERn_CTRL_CLKSEL(0)
> +#define TIMERn_CTRL_OSMEN 0x00000010
> +#define TIMERn_CTRL_MODE(val) (((val) & 0x3) << 0)
> +#define TIMERn_CTRL_MODE_UP TIMERn_CTRL_MODE(0)
> +#define TIMERn_CTRL_MODE_DOWN TIMERn_CTRL_MODE(1)
> +
> +#define TIMERn_CMD 0x04
> +#define TIMERn_CMD_START 0x1
> +#define TIMERn_CMD_STOP 0x2
> +
> +#define TIMERn_IEN 0x0c
> +#define TIMERn_IF 0x10
> +#define TIMERn_IFS 0x14
> +#define TIMERn_IFC 0x18
> +#define TIMERn_IRQ_UF 0x2
> +#define TIMERn_IRQ_OF 0x1
I see a wrong alignment with the values. May be it is due to my mailer.
> +
> +#define TIMERn_TOP 0x1c
> +#define TIMERn_CNT 0x24
> +
> +#define TIMER_CLOCKSOURCE 0
> +#define TIMER_CLOCKEVENT 1
I don't see these macro used anywhere.
> +struct efm32_clock_event_ddata {
The structure name looks a bit long to me.
> + struct clock_event_device evtdev;
> + void __iomem *base;
> + unsigned periodic_top;
> +};
> +static void efm32_clock_event_set_mode(enum clock_event_mode mode,
> + struct clock_event_device *evtdev)
> +{
> + struct efm32_clock_event_ddata *ddata =
> + container_of(evtdev, struct efm32_clock_event_ddata, evtdev);
Wouldn't be worth to check the previous mode of the timer before
switching to a mode where it is already ?
> + switch (mode) {
> + case CLOCK_EVT_MODE_PERIODIC:
> + writel_relaxed(TIMERn_CMD_STOP, ddata->base + TIMERn_CMD);
> + writel_relaxed(ddata->periodic_top, ddata->base + TIMERn_TOP);
> + writel_relaxed(TIMERn_CTRL_PRESC_1024 |
> + TIMERn_CTRL_CLKSEL_PRESCHFPERCLK |
> + TIMERn_CTRL_MODE_DOWN,
> + ddata->base + TIMERn_CTRL);
> + writel_relaxed(TIMERn_CMD_START, ddata->base + TIMERn_CMD);
> + break;
> +
> + case CLOCK_EVT_MODE_ONESHOT:
> + writel_relaxed(TIMERn_CMD_STOP, ddata->base + TIMERn_CMD);
> + writel_relaxed(TIMERn_CTRL_PRESC_1024 |
> + TIMERn_CTRL_CLKSEL_PRESCHFPERCLK |
> + TIMERn_CTRL_OSMEN |
> + TIMERn_CTRL_MODE_DOWN,
> + ddata->base + TIMERn_CTRL);
> + break;
IMO, these two routines are similar enough to factor them out in a
single function.
> +
> + case CLOCK_EVT_MODE_UNUSED:
> + case CLOCK_EVT_MODE_SHUTDOWN:
> + writel_relaxed(TIMERn_CMD_STOP, ddata->base + TIMERn_CMD);
> + break;
> +
> + case CLOCK_EVT_MODE_RESUME:
> + break;
> + }
> +}
> +
> +static int efm32_clock_event_set_next_event(unsigned long evt,
> + struct clock_event_device *evtdev)
> +{
> + struct efm32_clock_event_ddata *ddata =
> + container_of(evtdev, struct efm32_clock_event_ddata, evtdev);
> +
> + writel_relaxed(TIMERn_CMD_STOP, ddata->base + TIMERn_CMD);
> + writel_relaxed(evt, ddata->base + TIMERn_CNT);
> + writel_relaxed(TIMERn_CMD_START, ddata->base + TIMERn_CMD);
> +
> + return 0;
> +}
> +
> +static irqreturn_t efm32_clock_event_handler(int irq, void *dev_id)
> +{
> + struct efm32_clock_event_ddata *ddata = dev_id;
> +
> + writel_relaxed(TIMERn_IRQ_UF, ddata->base + TIMERn_IFC);
> +
> + ddata->evtdev.event_handler(&ddata->evtdev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static struct efm32_clock_event_ddata clock_event_ddata = {
> + .evtdev = {
> + .name = "efm32 clockevent",
> + .features = CLOCK_EVT_FEAT_ONESHOT | CLOCK_EVT_MODE_PERIODIC,
> + .set_mode = efm32_clock_event_set_mode,
> + .set_next_event = efm32_clock_event_set_next_event,
> + .rating = 200,
> + },
> +};
> +
> +static struct irqaction efm32_clock_event_irq = {
> + .name = "efm32 clockevent",
> + .flags = IRQF_TIMER,
> + .handler = efm32_clock_event_handler,
> + .dev_id = &clock_event_ddata,
> +};
Function name looks a bit long.
> +static int efm32_clocksource_init(struct device_node *np)
> +{
> + struct clk *clk;
> + void __iomem *base;
> + unsigned long rate;
> + int ret;
> +
> + clk = of_clk_get(np, 0);
> + if (IS_ERR(clk)) {
> + ret = PTR_ERR(clk);
> + pr_err("failed to get clock for clocksource (%d)\n", ret);
> + goto err_clk_get;
> + }
> +
> + ret = clk_prepare_enable(clk);
> + if (ret) {
> + pr_err("failed to enable timer clock for clocksource (%d)\n",
> + ret);
> + goto err_clk_enable;
> + }
> + rate = clk_get_rate(clk);
> +
> + base = of_iomap(np, 0);
> + if (!base) {
> + ret = -EADDRNOTAVAIL;
> + pr_err("failed to map registers for clocksource\n");
> + goto err_iomap;
> + }
> +
> + writel_relaxed(TIMERn_CTRL_PRESC_1024 |
> + TIMERn_CTRL_CLKSEL_PRESCHFPERCLK |
> + TIMERn_CTRL_MODE_UP, base + TIMERn_CTRL);
> + writel_relaxed(TIMERn_CMD_START, base + TIMERn_CMD);
> +
> + ret = clocksource_mmio_init(base + TIMERn_CNT, "efm32 timer",
> + DIV_ROUND_CLOSEST(rate, 1024), 200, 16,
> + clocksource_mmio_readl_up);
> + if (ret) {
> + pr_err("failed to init clocksource (%d)\n", ret);
> + goto err_clocksource_init;
> + }
> +
> + return 0;
> +
> +err_clocksource_init:
> +
> + iounmap(base);
> +err_iomap:
> +
> + clk_disable_unprepare(clk);
> +err_clk_enable:
> +
> + clk_put(clk);
> +err_clk_get:
> +
> + return ret;
> +}
> +
> +static int __init efm32_clockevent_init(struct device_node *np)
> +{
> + struct clk *clk;
> + void __iomem *base;
> + unsigned long rate;
> + int irq;
> + int ret;
> +
> + clk = of_clk_get(np, 0);
> + if (IS_ERR(clk)) {
> + ret = PTR_ERR(clk);
> + pr_err("failed to get clock for clockevent (%d)\n", ret);
> + goto err_clk_get;
> + }
> +
> + ret = clk_prepare_enable(clk);
> + if (ret) {
> + pr_err("failed to enable timer clock for clockevent (%d)\n",
> + ret);
> + goto err_clk_enable;
> + }
> + rate = clk_get_rate(clk);
> +
> + base = of_iomap(np, 0);
> + if (!base) {
> + ret = -EADDRNOTAVAIL;
> + pr_err("failed to map registers for clockevent\n");
> + goto err_iomap;
> + }
> +
> + irq = irq_of_parse_and_map(np, 0);
> + if (!irq) {
> + ret = -ENOENT;
> + pr_err("failed to get irq for clockevent\n");
> + goto err_get_irq;
> + }
> +
> + writel_relaxed(TIMERn_IRQ_UF, base + TIMERn_IEN);
> +
> + clock_event_ddata.base = base;
> + clock_event_ddata.periodic_top = DIV_ROUND_CLOSEST(rate, 1024 * HZ);
> +
> + setup_irq(irq, &efm32_clock_event_irq);
> +
> + clockevents_config_and_register(&clock_event_ddata.evtdev,
> + DIV_ROUND_CLOSEST(rate, 1024), 0xf, 0xffff);
> +
> + return 0;
> +
> +err_get_irq:
> +
> + iounmap(base);
> +err_iomap:
> +
> + clk_disable_unprepare(clk);
> +err_clk_enable:
> +
> + clk_put(clk);
> +err_clk_get:
> +
> + return ret;
> +}
> +
> +static void __init efm32_timer_init(struct device_node *np)
> +{
> + static int has_clocksource, has_clockevent;
> + int ret;
> +
> + if (!has_clocksource) {
> + ret = efm32_clocksource_init(np);
> + if (!ret) {
> + has_clocksource = 1;
> + return;
> + }
> + }
> +
> + if (!has_clockevent) {
> + ret = efm32_clockevent_init(np);
> + if (!ret) {
> + has_clockevent = 1;
> + return;
> + }
> + }
> +}
I don't get the purpose of this initialization, can you explain ?
> +CLOCKSOURCE_OF_DECLARE(efm32, "efm32,timer", efm32_timer_init);
>
--
<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
More information about the linux-arm-kernel
mailing list