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

[daniel.lezcano@linaro.org (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