| Message ID | 1379324644-20934-1-git-send-email-u.kleine-koenig@pengutronix.de |
|---|---|
| State | New |
| Headers | show |
On 09/16/2013 11:44 AM, Uwe Kleine-König wrote: > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@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@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); >
Hello Daniel, On Wed, Sep 25, 2013 at 04:33:24PM +0200, Daniel Lezcano wrote: > On 09/16/2013 11:44 AM, Uwe Kleine-König wrote: > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > 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. in the binding document or the commit log? > 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. It is. If ARCH_EFM32=y there is no prompt and the "default ARCH_EFM32" makes it true. > > 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@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. The BIT macro only can define values that have a single bit set. So it doesn't help for the definition of the TIMERn_CTRL_PRESC macro and most others. And then I prefer to have a common way to define the register descriptions. > > +#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. Maybe that's because the patch has one char more (the +) before the tabs, that make things look wrong sometimes. In the file it looks ok. > > + > > +#define TIMERn_TOP 0x1c > > +#define TIMERn_CNT 0x24 > > + > > +#define TIMER_CLOCKSOURCE 0 > > +#define TIMER_CLOCKEVENT 1 > > I don't see these macro used anywhere. Right, pre-dt cruft. > > +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 ? This isn't a hot path, is it? IIRC this function is called exactly twice. And I think it still needs stopping at least in some cases. Then it's more complicated and so not worth the effort. > > + 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. I don't know what you want here? A #define for the common bits? I like being explicit here to not have to jump around to verify the settings with the hardware reference manual. > > + 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. Which function? I prefer having a unique prefix for the driver + an accurate description. All lines affected by "efm32_clock_event_handler" don't even need to be broken, so IMO it's OK like that. > > +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); oh, here is an indention problem. Will fixup for the next version. > > + > > + 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 ? An efm32 SoC has four timer blocks. A single block can only be used for one of clocksource or clockevent device and having more than one clocksource or clockevent device doesn't make sense. So this routine asserts that the first timer is used as clocksource and the second as clockevent device. The others are unused. Best regards Uwe
On 09/25/2013 05:32 PM, Uwe Kleine-König wrote: > Hello Daniel, > > On Wed, Sep 25, 2013 at 04:33:24PM +0200, Daniel Lezcano wrote: >> On 09/16/2013 11:44 AM, Uwe Kleine-König wrote: >>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> >>> --- >>> 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. > in the binding document or the commit log? commit log. >> 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. > It is. If ARCH_EFM32=y there is no prompt and the "default ARCH_EFM32" > makes it true. ok, with that but if ARCH_EFM32=no, you can enable it manually. AFAIK, we want to prevent this and let the correct arch to enable it. John ? >>> 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@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. > The BIT macro only can define values that have a single bit set. So it > doesn't help for the definition of the TIMERn_CTRL_PRESC macro and most > others. And then I prefer to have a common way to define the register > descriptions. Ok. >>> +#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. > Maybe that's because the patch has one char more (the +) before the > tabs, that make things look wrong sometimes. In the file it looks ok. Ok. >>> + >>> +#define TIMERn_TOP 0x1c >>> +#define TIMERn_CNT 0x24 >>> + >>> +#define TIMER_CLOCKSOURCE 0 >>> +#define TIMER_CLOCKEVENT 1 >> >> I don't see these macro used anywhere. > Right, pre-dt cruft. > >>> +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 ? > This isn't a hot path, is it? IIRC this function is called exactly > twice. And I think it still needs stopping at least in some cases. Then > it's more complicated and so not worth the effort. Ok. >>> + 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. > I don't know what you want here? A #define for the common bits? I like > being explicit here to not have to jump around to verify the settings > with the hardware reference manual. Never mind, I missed one line when reading the CLOCK_EVT_MODE_PERIODIC and the CLOCK_EVT_MODE_ONESHOT blocks. At the first glance, they looked very similar and I thought they can be factored out into a 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. > Which function? I prefer having a unique prefix for the driver + an > accurate description. All lines affected by "efm32_clock_event_handler" > don't even need to be broken, so IMO it's OK like that. Well, replacing clock_event/clockevent by clkevt and clocksource by clksrc would have made the functions name shorter. But it is a personal preference, feel free to ignore it if you prefer your naming convention. [ ... ] >>> + >>> +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 ? > An efm32 SoC has four timer blocks. A single block can only be used for > one of clocksource or clockevent device and having more than one > clocksource or clockevent device doesn't make sense. So this routine > asserts that the first timer is used as clocksource and the second as > clockevent device. The others are unused. Shouldn't be up to the dt to give the timers you want ?
On 09/25/2013 04:49 PM, Daniel Lezcano wrote: > On 09/25/2013 05:32 PM, Uwe Kleine-König wrote: >> Hello Daniel, >> >> On Wed, Sep 25, 2013 at 04:33:24PM +0200, Daniel Lezcano wrote: >>> On 09/16/2013 11:44 AM, Uwe Kleine-König wrote: >>>> 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. >> It is. If ARCH_EFM32=y there is no prompt and the "default ARCH_EFM32" >> makes it true. > ok, with that but if ARCH_EFM32=no, you can enable it manually. AFAIK, > we want to prevent this and let the correct arch to enable it. > > John ? Right until there's really a compelling reason (which I've still not heard), I don't want to introduce independent clocksource options. Any such options should be something like a platform or board config option. thanks -john
On Wed, Sep 25, 2013 at 04:55:15PM -0700, John Stultz wrote: > On 09/25/2013 04:49 PM, Daniel Lezcano wrote: > > On 09/25/2013 05:32 PM, Uwe Kleine-König wrote: > >> Hello Daniel, > >> > >> On Wed, Sep 25, 2013 at 04:33:24PM +0200, Daniel Lezcano wrote: > >>> On 09/16/2013 11:44 AM, Uwe Kleine-König wrote: > >>>> 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. > >> It is. If ARCH_EFM32=y there is no prompt and the "default ARCH_EFM32" > >> makes it true. > > ok, with that but if ARCH_EFM32=no, you can enable it manually. AFAIK, > > we want to prevent this and let the correct arch to enable it. > > > > John ? > > Right until there's really a compelling reason (which I've still not > heard), I don't want to introduce independent clocksource options. Any > such options should be something like a platform or board config option. You can only manually enable it if you also enabled COMPILE_TEST. Then the intention is clear, isn't it? Best regards Uwe
Hello Daniel, On Thu, Sep 26, 2013 at 01:49:52AM +0200, Daniel Lezcano wrote: > On 09/25/2013 05:32 PM, Uwe Kleine-König wrote: > >>> +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 ? > > An efm32 SoC has four timer blocks. A single block can only be used for > > one of clocksource or clockevent device and having more than one > > clocksource or clockevent device doesn't make sense. So this routine > > asserts that the first timer is used as clocksource and the second as > > clockevent device. The others are unused. > > Shouldn't be up to the dt to give the timers you want ? The dt looks as follows: timer0: timer@40010000 { compatible = "efm32,timer"; reg = <0x40010000 0x400>; interrupts = <2>; clocks = <&cmu clk_HFPERCLKTIMER0>; }; timer1: timer@40010400 { compatible = "efm32,timer"; reg = <0x40010400 0x400>; interrupts = <12>; clocks = <&cmu clk_HFPERCLKTIMER1>; }; timer2: timer@40010800 { compatible = "efm32,timer"; reg = <0x40010800 0x400>; interrupts = <13>; clocks = <&cmu clk_HFPERCLKTIMER2>; }; timer3: timer@40010c00 { compatible = "efm32,timer"; reg = <0x40010c00 0x400>; interrupts = <14>; clocks = <&cmu clk_HFPERCLKTIMER3>; }; What is your suggestion now? Add a property that specifies if the block should be used as clocksource or clockevent_device? That isn't a hardware description and so shouldn't go into the device tree. Provide two drivers that match on "efm32,timer", one for clocksource and another for clockevent_device? That wouldn't work, too, as the first driver to be loaded would grab all four timers and the second would get none. Best regards Uwe
On 09/26/2013 10:20 AM, Uwe Kleine-König wrote: > Hello Daniel, > > On Thu, Sep 26, 2013 at 01:49:52AM +0200, Daniel Lezcano wrote: >> On 09/25/2013 05:32 PM, Uwe Kleine-König wrote: >>>>> +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 ? >>> An efm32 SoC has four timer blocks. A single block can only be used for >>> one of clocksource or clockevent device and having more than one >>> clocksource or clockevent device doesn't make sense. So this routine >>> asserts that the first timer is used as clocksource and the second as >>> clockevent device. The others are unused. >> >> Shouldn't be up to the dt to give the timers you want ? > The dt looks as follows: > > timer0: timer@40010000 { > compatible = "efm32,timer"; > reg = <0x40010000 0x400>; > interrupts = <2>; > clocks = <&cmu clk_HFPERCLKTIMER0>; > }; > > timer1: timer@40010400 { > compatible = "efm32,timer"; > reg = <0x40010400 0x400>; > interrupts = <12>; > clocks = <&cmu clk_HFPERCLKTIMER1>; > }; > > timer2: timer@40010800 { > compatible = "efm32,timer"; > reg = <0x40010800 0x400>; > interrupts = <13>; > clocks = <&cmu clk_HFPERCLKTIMER2>; > }; > > timer3: timer@40010c00 { > compatible = "efm32,timer"; > reg = <0x40010c00 0x400>; > interrupts = <14>; > clocks = <&cmu clk_HFPERCLKTIMER3>; > }; > > What is your suggestion now? > Add a property that specifies if the block > should be used as clocksource or clockevent_device? That isn't a > hardware description and so shouldn't go into the device tree. At this point, I just asked a question and did not make any suggestion. > Provide two drivers that match on "efm32,timer", one for clocksource and > another for clockevent_device? That wouldn't work, too, as the first > driver to be loaded would grab all four timers and the second would get > none. Thanks, now I understand the purpose of this routine, it is very similar than: http://www.spinics.net/lists/arm-kernel/msg273984.html right ?
Hello Daniel, On Thu, Sep 26, 2013 at 10:52:29AM +0200, Daniel Lezcano wrote: > On 09/26/2013 10:20 AM, Uwe Kleine-König wrote: > >Hello Daniel, > > > >On Thu, Sep 26, 2013 at 01:49:52AM +0200, Daniel Lezcano wrote: > >>On 09/25/2013 05:32 PM, Uwe Kleine-König wrote: > >>>>>+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 ? > >>>An efm32 SoC has four timer blocks. A single block can only be used for > >>>one of clocksource or clockevent device and having more than one > >>>clocksource or clockevent device doesn't make sense. So this routine > >>>asserts that the first timer is used as clocksource and the second as > >>>clockevent device. The others are unused. > >> > >>Shouldn't be up to the dt to give the timers you want ? > >The dt looks as follows: > > > > timer0: timer@40010000 { > > compatible = "efm32,timer"; > > reg = <0x40010000 0x400>; > > interrupts = <2>; > > clocks = <&cmu clk_HFPERCLKTIMER0>; > > }; > > > > timer1: timer@40010400 { > > compatible = "efm32,timer"; > > reg = <0x40010400 0x400>; > > interrupts = <12>; > > clocks = <&cmu clk_HFPERCLKTIMER1>; > > }; > > > > timer2: timer@40010800 { > > compatible = "efm32,timer"; > > reg = <0x40010800 0x400>; > > interrupts = <13>; > > clocks = <&cmu clk_HFPERCLKTIMER2>; > > }; > > > > timer3: timer@40010c00 { > > compatible = "efm32,timer"; > > reg = <0x40010c00 0x400>; > > interrupts = <14>; > > clocks = <&cmu clk_HFPERCLKTIMER3>; > > }; > > > >What is your suggestion now? > >Add a property that specifies if the block > >should be used as clocksource or clockevent_device? That isn't a > >hardware description and so shouldn't go into the device tree. > > At this point, I just asked a question and did not make any suggestion. I thought your question implied knowing a better way. I'd be happy if it did. > >Provide two drivers that match on "efm32,timer", one for clocksource and > >another for clockevent_device? That wouldn't work, too, as the first > >driver to be loaded would grab all four timers and the second would get > >none. > > Thanks, now I understand the purpose of this routine, it is very > similar than: > > http://www.spinics.net/lists/arm-kernel/msg273984.html > > right ? Right. And as tglx points out in that post, it's not pretty, but I don't have an idea how to do it nicer. (BTW, I wonder if the of_node_put in that snipplet is correct, also the three static functions being called could be marked __init.) At least my implementation is a bit more robust as it handles the case that the timer intended to be used as clockevent device doesn't have an irq while the dw_apb_timer driver simply BUGs then. Best regards Uwe
On Wed, Sep 25, 2013 at 04:33:24PM +0200, Daniel Lezcano wrote: > On 09/16/2013 11:44 AM, Uwe Kleine-König wrote: > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@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. Where is the location to put a device tree binding document for a clocksource/clock event device? I found arm,armv7-timer-mem | arm/arch_timer.txt fsl,timrot | N/A nvidia,tegra20-rtc | rtc/nvidia,tegra20-rtc.txt Should I introduce a "clocksource" directory below Documentation/devicetree/bindings? Other than that if there are no further comments I'll sent a v2 soon. Best regards Uwe
On 10/01/2013 02:08 AM, Uwe Kleine-König wrote: > On Wed, Sep 25, 2013 at 04:33:24PM +0200, Daniel Lezcano wrote: >> On 09/16/2013 11:44 AM, Uwe Kleine-König wrote: >>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@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. > Where is the location to put a device tree binding document for a > clocksource/clock event device? I found > > arm,armv7-timer-mem | arm/arch_timer.txt > fsl,timrot | N/A > nvidia,tegra20-rtc | rtc/nvidia,tegra20-rtc.txt The Tegra example isn't a good one here, since the Tegra HW block is primarily an RTC (hence the location of the binding file), which also happens to be able to act as the clocksource. > Should I introduce a "clocksource" directory below > Documentation/devicetree/bindings? That seems reasonable for any HW block that is truly purely a clocksource. However, you'd want to also check with all the other DT bindings maintainers.
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. + 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@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) +#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 + +#define TIMERn_TOP 0x1c +#define TIMERn_CNT 0x24 + +#define TIMER_CLOCKSOURCE 0 +#define TIMER_CLOCKEVENT 1 + +struct efm32_clock_event_ddata { + 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); + + 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; + + 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, +}; + +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; + } + } +} +CLOCKSOURCE_OF_DECLARE(efm32, "efm32,timer", efm32_timer_init);
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@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. 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