[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Uwe Kleine-König]

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

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);
-- 
1.8.4.rc3

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Daniel Lezcano]

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);
> 


-- 
 <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

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Daniel Lezcano]

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 ?


-- 
 <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

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[John Stultz]

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

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Uwe Kleine-König]

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 at 40010000 {
                        compatible = "efm32,timer";
                        reg = <0x40010000 0x400>;
                        interrupts = <2>;
                        clocks = <&cmu clk_HFPERCLKTIMER0>;
                };

                timer1: timer at 40010400 {
                        compatible = "efm32,timer";
                        reg = <0x40010400 0x400>;
                        interrupts = <12>;
                        clocks = <&cmu clk_HFPERCLKTIMER1>;
                };

                timer2: timer at 40010800 {
                        compatible = "efm32,timer";
                        reg = <0x40010800 0x400>;
                        interrupts = <13>;
                        clocks = <&cmu clk_HFPERCLKTIMER2>;
                };

                timer3: timer at 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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Daniel Lezcano]

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 at 40010000 {
>                          compatible = "efm32,timer";
>                          reg = <0x40010000 0x400>;
>                          interrupts = <2>;
>                          clocks = <&cmu clk_HFPERCLKTIMER0>;
>                  };
>
>                  timer1: timer at 40010400 {
>                          compatible = "efm32,timer";
>                          reg = <0x40010400 0x400>;
>                          interrupts = <12>;
>                          clocks = <&cmu clk_HFPERCLKTIMER1>;
>                  };
>
>                  timer2: timer at 40010800 {
>                          compatible = "efm32,timer";
>                          reg = <0x40010800 0x400>;
>                          interrupts = <13>;
>                          clocks = <&cmu clk_HFPERCLKTIMER2>;
>                  };
>
>                  timer3: timer at 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 ?



-- 
  <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

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Uwe Kleine-König]

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 at 40010000 {
> >                         compatible = "efm32,timer";
> >                         reg = <0x40010000 0x400>;
> >                         interrupts = <2>;
> >                         clocks = <&cmu clk_HFPERCLKTIMER0>;
> >                 };
> >
> >                 timer1: timer at 40010400 {
> >                         compatible = "efm32,timer";
> >                         reg = <0x40010400 0x400>;
> >                         interrupts = <12>;
> >                         clocks = <&cmu clk_HFPERCLKTIMER1>;
> >                 };
> >
> >                 timer2: timer at 40010800 {
> >                         compatible = "efm32,timer";
> >                         reg = <0x40010800 0x400>;
> >                         interrupts = <13>;
> >                         clocks = <&cmu clk_HFPERCLKTIMER2>;
> >                 };
> >
> >                 timer3: timer at 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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Uwe Kleine-König]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[RFC, PATCH] clocksource: provide timekeeping for efm32 SoCs

[Stephen Warren]

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.