[PATCH 0/3] Introduce clocksource driver for Keystone platform

[PATCH 0/3] Introduce clocksource driver for Keystone platform

[Ivan Khoronzhuk]

Add a backup timer64 based clockevent driver for keystone arch.
This driver uses timer in 64-bit general purpose mode as clock event
device.

Ivan Khoronzhuk (3):
  clocksource: timer-keystone: introduce clocksource driver for
    Keystone
  clocksource: keystone: add bindings for keystone timer
  arm: dts: keystone: add keystone timer entry

 .../bindings/timer/ti,keystone-timer.txt           |   29 +++
 arch/arm/boot/dts/keystone-clocks.dtsi             |   10 +
 arch/arm/boot/dts/keystone.dtsi                    |    7 +
 drivers/clocksource/Makefile                       |    1 +
 drivers/clocksource/timer-keystone.c               |  223 ++++++++++++++++++++
 5 files changed, 270 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/ti,keystone-timer.txt
 create mode 100644 drivers/clocksource/timer-keystone.c

-- 
1.7.9.5

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[Ivan Khoronzhuk]

Add broadcast clock-event device for the Keystone arch.

The timer can be configured as a general-purpose 64-bit timer,
dual general-purpose 32-bit timers. When configured as dual 32-bit
timers, each half can operate in conjunction (chain mode) or
independently (unchained mode) of each other.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/clocksource/Makefile         |    1 +
 drivers/clocksource/timer-keystone.c |  223 ++++++++++++++++++++++++++++++++++
 2 files changed, 224 insertions(+)
 create mode 100644 drivers/clocksource/timer-keystone.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 33621ef..2acf3fc 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -36,3 +36,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_ARM_GLOBAL_TIMER)		+= arm_global_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
 obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)	+= dummy_timer.o
+obj-$(CONFIG_ARCH_KEYSTONE)		+= timer-keystone.o
diff --git a/drivers/clocksource/timer-keystone.c b/drivers/clocksource/timer-keystone.c
new file mode 100644
index 0000000..4a8083a
--- /dev/null
+++ b/drivers/clocksource/timer-keystone.c
@@ -0,0 +1,223 @@
+/*
+ * Keystone broadcast clock-event
+ *
+ * Copyright 2013 Texas Instruments, Inc.
+ *
+ * Author: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
+ *
+ * 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.
+ *
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/clocksource.h>
+#include <linux/interrupt.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+
+#define TIMER_NAME			"timer64-event"
+
+/* Timer register offsets */
+#define TIM12				0x10
+#define TIM34				0x14
+#define PRD12				0x18
+#define PRD34				0x1c
+#define TCR				0x20
+#define TGCR				0x24
+#define INTCTLSTAT			0x44
+
+/* Timer register bitfields */
+#define TCR_ENAMODE_MASK		0xC0
+#define TCR_ENAMODE_ONESHOT_MASK	0x40
+#define TCR_ENAMODE_PERIODIC_MASK	0x80
+
+#define TGCR_TIM_UNRESET_MASK		0x03
+#define INTCTLSTAT_ENINT_MASK		0x01
+
+/**
+ * struct keystone_timer: holds timer's data
+ * @base: timer memory base address
+ * @clk_rate: source clock rate
+ * @irqacrion: interrupt action descriptor
+ * @event_dev: event device based on timer
+ */
+static struct keystone_timer {
+	void __iomem *base;
+	unsigned long clk_rate;
+	struct irqaction irqaction;
+	struct clock_event_device event_dev;
+} timer;
+
+static inline u32 keystone_timer_readl(unsigned long rg)
+{
+	return readl(timer.base + rg);
+}
+
+static inline void keystone_timer_writel(u32 val, unsigned long rg)
+{
+	writel(val, timer.base + rg);
+}
+
+/**
+ * keystone_timer_config: configures timer to work in oneshot/periodic modes, in
+ * other cases disables timer.
+ * @ mode: mode to configure
+ * @ period: cycles number to configure for
+ */
+static int keystone_timer_config(u64 period, enum clock_event_mode mode)
+{
+	u32 tcr;
+
+	tcr = keystone_timer_readl(TCR);
+
+	/* disable timer */
+	tcr &= ~(TCR_ENAMODE_MASK);
+	keystone_timer_writel(tcr, TCR);
+
+	/* set enable mode or leave disabled timer */
+	switch (mode) {
+	case CLOCK_EVT_MODE_ONESHOT:
+		tcr |= TCR_ENAMODE_ONESHOT_MASK;
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		tcr |= TCR_ENAMODE_PERIODIC_MASK;
+		break;
+	default:
+		return 0;
+	}
+
+	/* reset counter to zero, set new period */
+	keystone_timer_writel(0, TIM12);
+	keystone_timer_writel(0, TIM34);
+	keystone_timer_writel(period & 0xffffffff, PRD12);
+	keystone_timer_writel(period >> 32, PRD34);
+
+	/* enable timer */
+	keystone_timer_writel(tcr, TCR);
+	return 0;
+}
+
+static irqreturn_t keystone_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = &timer.event_dev;
+
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static int keystone_set_next_event(unsigned long cycles,
+				  struct clock_event_device *evt)
+{
+	return keystone_timer_config(cycles, evt->mode);
+}
+
+static void keystone_set_mode(enum clock_event_mode mode,
+			     struct clock_event_device *evt)
+{
+	u64 period;
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_PERIODIC:
+		period = timer.clk_rate / (HZ);
+		keystone_timer_config(period, CLOCK_EVT_MODE_PERIODIC);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	case CLOCK_EVT_MODE_ONESHOT:
+		keystone_timer_config(0, CLOCK_EVT_MODE_SHUTDOWN);
+	default:
+		break;
+	}
+}
+
+static void __init keystone_timer_init(struct device_node *np)
+{
+	struct clock_event_device *event_dev = &timer.event_dev;
+	struct irqaction *irqaction = &timer.irqaction;
+	unsigned long rate;
+	struct clk *clk;
+	int irq, error;
+	u32 tgcr;
+
+	irq  = irq_of_parse_and_map(np, 0);
+	if (irq == NO_IRQ) {
+		pr_err("%s: failed to map interrupts\n", __func__);
+		return;
+	}
+
+	timer.base = of_iomap(np, 0);
+	if (!timer.base) {
+		pr_err("%s: failed to map registers\n", __func__);
+		return;
+	}
+
+	clk = of_clk_get(np, 0);
+	if (!clk) {
+		pr_err("%s: failed to get clock\n", __func__);
+		iounmap(timer.base);
+		return;
+	}
+
+	error = clk_prepare_enable(clk);
+	if (error) {
+		pr_err("%s: failed to enable clock\n", __func__);
+		goto err;
+	}
+
+	rate = clk_get_rate(clk);
+
+	/* disable, use internal clock source */
+	keystone_timer_writel(0, TCR);
+
+	/* reset timer as 64-bit, no pre-scaler, plus features are disabled */
+	tgcr = 0;
+	keystone_timer_writel(0, TGCR);
+
+	/* unreset timer */
+	tgcr |= TGCR_TIM_UNRESET_MASK;
+	keystone_timer_writel(tgcr, TGCR);
+
+	/* init counter to zero */
+	keystone_timer_writel(0, TIM12);
+	keystone_timer_writel(0, TIM34);
+
+	timer.clk_rate = rate;
+
+	/* enable timer interrupts */
+	keystone_timer_writel(INTCTLSTAT_ENINT_MASK, INTCTLSTAT);
+
+	keystone_timer_config(0, CLOCK_EVT_MODE_UNUSED);
+
+	/* init irqaction */
+	irqaction->flags = IRQF_TIMER;
+	irqaction->handler = keystone_timer_interrupt;
+	irqaction->name = TIMER_NAME;
+	irqaction->dev_id = (void *)&timer;
+	error = setup_irq(irq, irqaction);
+	if (error) {
+		pr_err("%s: failed to setup irq\n", __func__);
+		goto err;
+	}
+
+	/* setup clockevent */
+	event_dev->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
+	event_dev->set_next_event = keystone_set_next_event;
+	event_dev->set_mode = keystone_set_mode;
+	event_dev->cpumask = cpu_all_mask;
+	event_dev->owner = THIS_MODULE;
+	event_dev->name = TIMER_NAME;
+
+	clockevents_config_and_register(event_dev, rate, 1, ULONG_MAX);
+
+	pr_info("keystone timer clock @%lu Hz\n", rate);
+	return;
+err:
+	clk_put(clk);
+	iounmap(timer.base);
+}
+
+CLOCKSOURCE_OF_DECLARE(keystone_timer, "ti,keystone-timer",
+					keystone_timer_init);
-- 
1.7.9.5

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[Daniel Lezcano]

On 12/11/2013 07:00 PM, Ivan Khoronzhuk wrote:
> Add broadcast clock-event device for the Keystone arch.
>
> The timer can be configured as a general-purpose 64-bit timer,
> dual general-purpose 32-bit timers. When configured as dual 32-bit
> timers, each half can operate in conjunction (chain mode) or
> independently (unchained mode) of each other.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>   drivers/clocksource/Makefile         |    1 +
>   drivers/clocksource/timer-keystone.c |  223 ++++++++++++++++++++++++++++++++++
>   2 files changed, 224 insertions(+)
>   create mode 100644 drivers/clocksource/timer-keystone.c
>
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 33621ef..2acf3fc 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -36,3 +36,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
>   obj-$(CONFIG_ARM_GLOBAL_TIMER)		+= arm_global_timer.o
>   obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
>   obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)	+= dummy_timer.o
> +obj-$(CONFIG_ARCH_KEYSTONE)		+= timer-keystone.o
> diff --git a/drivers/clocksource/timer-keystone.c b/drivers/clocksource/timer-keystone.c
> new file mode 100644
> index 0000000..4a8083a
> --- /dev/null
> +++ b/drivers/clocksource/timer-keystone.c
> @@ -0,0 +1,223 @@
> +/*
> + * Keystone broadcast clock-event
> + *
> + * Copyright 2013 Texas Instruments, Inc.
> + *
> + * Author: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/clocksource.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#define TIMER_NAME			"timer64-event"
> +
> +/* Timer register offsets */
> +#define TIM12				0x10
> +#define TIM34				0x14
> +#define PRD12				0x18
> +#define PRD34				0x1c
> +#define TCR				0x20
> +#define TGCR				0x24
> +#define INTCTLSTAT			0x44
> +
> +/* Timer register bitfields */
> +#define TCR_ENAMODE_MASK		0xC0
> +#define TCR_ENAMODE_ONESHOT_MASK	0x40
> +#define TCR_ENAMODE_PERIODIC_MASK	0x80
> +
> +#define TGCR_TIM_UNRESET_MASK		0x03
> +#define INTCTLSTAT_ENINT_MASK		0x01
> +
> +/**
> + * struct keystone_timer: holds timer's data
> + * @base: timer memory base address
> + * @clk_rate: source clock rate
> + * @irqacrion: interrupt action descriptor
> + * @event_dev: event device based on timer
> + */
> +static struct keystone_timer {
> +	void __iomem *base;
> +	unsigned long clk_rate;
> +	struct irqaction irqaction;
> +	struct clock_event_device event_dev;
> +} timer;
> +
> +static inline u32 keystone_timer_readl(unsigned long rg)
> +{
> +	return readl(timer.base + rg);
> +}
> +
> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
> +{
> +	writel(val, timer.base + rg);
> +}
> +
> +/**
> + * keystone_timer_config: configures timer to work in oneshot/periodic modes, in
> + * other cases disables timer.
> + * @ mode: mode to configure
> + * @ period: cycles number to configure for
> + */
> +static int keystone_timer_config(u64 period, enum clock_event_mode mode)
> +{
> +	u32 tcr;
> +
> +	tcr = keystone_timer_readl(TCR);
> +
> +	/* disable timer */
> +	tcr &= ~(TCR_ENAMODE_MASK);
> +	keystone_timer_writel(tcr, TCR);

Please move this write after the switch (cf. linked comment below)

> +	/* set enable mode or leave disabled timer */
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		tcr |= TCR_ENAMODE_ONESHOT_MASK;
> +		break;
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		tcr |= TCR_ENAMODE_PERIODIC_MASK;
> +		break;
> +	default:
> +		return 0;

Shouldn't you return an error ?

> +	}
> +
> +	/* reset counter to zero, set new period */
> +	keystone_timer_writel(0, TIM12);
> +	keystone_timer_writel(0, TIM34);
> +	keystone_timer_writel(period & 0xffffffff, PRD12);
> +	keystone_timer_writel(period >> 32, PRD34);
> +
> +	/* enable timer */
> +	keystone_timer_writel(tcr, TCR);
> +	return 0;
> +}
> +
> +static irqreturn_t keystone_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = &timer.event_dev;
> +
> +	evt->event_handler(evt);
> +	return IRQ_HANDLED;
> +}
> +
> +static int keystone_set_next_event(unsigned long cycles,
> +				  struct clock_event_device *evt)
> +{
> +	return keystone_timer_config(cycles, evt->mode);
> +}
> +
> +static void keystone_set_mode(enum clock_event_mode mode,
> +			     struct clock_event_device *evt)
> +{
> +	u64 period;
> +
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		period = timer.clk_rate / (HZ);
> +		keystone_timer_config(period, CLOCK_EVT_MODE_PERIODIC);
> +		break;
> +	case CLOCK_EVT_MODE_UNUSED:
> +	case CLOCK_EVT_MODE_SHUTDOWN:
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		keystone_timer_config(0, CLOCK_EVT_MODE_SHUTDOWN);

I don't really this code assuming the side effect of specifying a 
unsupported mode will disable the timer.

> +	default:
> +		break;
> +	}
> +}
> +
> +static void __init keystone_timer_init(struct device_node *np)
> +{
> +	struct clock_event_device *event_dev = &timer.event_dev;
> +	struct irqaction *irqaction = &timer.irqaction;
> +	unsigned long rate;
> +	struct clk *clk;
> +	int irq, error;
> +	u32 tgcr;
> +
> +	irq  = irq_of_parse_and_map(np, 0);
> +	if (irq == NO_IRQ) {
> +		pr_err("%s: failed to map interrupts\n", __func__);
> +		return;
> +	}
> +
> +	timer.base = of_iomap(np, 0);
> +	if (!timer.base) {
> +		pr_err("%s: failed to map registers\n", __func__);
> +		return;
> +	}
> +
> +	clk = of_clk_get(np, 0);
> +	if (!clk) {
> +		pr_err("%s: failed to get clock\n", __func__);
> +		iounmap(timer.base);
> +		return;
> +	}
> +
> +	error = clk_prepare_enable(clk);
> +	if (error) {
> +		pr_err("%s: failed to enable clock\n", __func__);
> +		goto err;
> +	}
> +
> +	rate = clk_get_rate(clk);
> +
> +	/* disable, use internal clock source */
> +	keystone_timer_writel(0, TCR);
> +
> +	/* reset timer as 64-bit, no pre-scaler, plus features are disabled */
> +	tgcr = 0;
> +	keystone_timer_writel(0, TGCR);
> +
> +	/* unreset timer */
> +	tgcr |= TGCR_TIM_UNRESET_MASK;
> +	keystone_timer_writel(tgcr, TGCR);
> +
> +	/* init counter to zero */
> +	keystone_timer_writel(0, TIM12);
> +	keystone_timer_writel(0, TIM34);
> +
> +	timer.clk_rate = rate;
> +
> +	/* enable timer interrupts */
> +	keystone_timer_writel(INTCTLSTAT_ENINT_MASK, INTCTLSTAT);
> +
> +	keystone_timer_config(0, CLOCK_EVT_MODE_UNUSED);

Same comment here.

> +	/* init irqaction */
> +	irqaction->flags = IRQF_TIMER;
> +	irqaction->handler = keystone_timer_interrupt;
> +	irqaction->name = TIMER_NAME;
> +	irqaction->dev_id = (void *)&timer;
> +	error = setup_irq(irq, irqaction);
> +	if (error) {
> +		pr_err("%s: failed to setup irq\n", __func__);
> +		goto err;
> +	}
> +
> +	/* setup clockevent */
> +	event_dev->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
> +	event_dev->set_next_event = keystone_set_next_event;
> +	event_dev->set_mode = keystone_set_mode;
> +	event_dev->cpumask = cpu_all_mask;
> +	event_dev->owner = THIS_MODULE;
> +	event_dev->name = TIMER_NAME;
> +
> +	clockevents_config_and_register(event_dev, rate, 1, ULONG_MAX);
> +
> +	pr_info("keystone timer clock @%lu Hz\n", rate);
> +	return;
> +err:
> +	clk_put(clk);
> +	iounmap(timer.base);
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(keystone_timer, "ti,keystone-timer",
> +					keystone_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

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[ivan.khoronzhuk]

On 12/12/2013 05:51 PM, Daniel Lezcano wrote:
> On 12/11/2013 07:00 PM, Ivan Khoronzhuk wrote:
>> Add broadcast clock-event device for the Keystone arch.
>>
>> The timer can be configured as a general-purpose 64-bit timer,
>> dual general-purpose 32-bit timers. When configured as dual 32-bit
>> timers, each half can operate in conjunction (chain mode) or
>> independently (unchained mode) of each other.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   drivers/clocksource/Makefile         |    1 +
>>   drivers/clocksource/timer-keystone.c |  223 
>> ++++++++++++++++++++++++++++++++++
>>   2 files changed, 224 insertions(+)
>>   create mode 100644 drivers/clocksource/timer-keystone.c
>>
>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>> index 33621ef..2acf3fc 100644
>> --- a/drivers/clocksource/Makefile
>> +++ b/drivers/clocksource/Makefile
>> @@ -36,3 +36,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER)        += arm_arch_timer.o
>>   obj-$(CONFIG_ARM_GLOBAL_TIMER)        += arm_global_timer.o
>>   obj-$(CONFIG_CLKSRC_METAG_GENERIC)    += metag_generic.o
>>   obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)    += dummy_timer.o
>> +obj-$(CONFIG_ARCH_KEYSTONE)        += timer-keystone.o
>> diff --git a/drivers/clocksource/timer-keystone.c 
>> b/drivers/clocksource/timer-keystone.c
>> new file mode 100644
>> index 0000000..4a8083a
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-keystone.c
>> @@ -0,0 +1,223 @@
>> +/*
>> + * Keystone broadcast clock-event
>> + *
>> + * Copyright 2013 Texas Instruments, Inc.
>> + *
>> + * Author: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> + *
>> + * 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.
>> + *
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/clocksource.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +
>> +#define TIMER_NAME            "timer64-event"
>> +
>> +/* Timer register offsets */
>> +#define TIM12                0x10
>> +#define TIM34                0x14
>> +#define PRD12                0x18
>> +#define PRD34                0x1c
>> +#define TCR                0x20
>> +#define TGCR                0x24
>> +#define INTCTLSTAT            0x44
>> +
>> +/* Timer register bitfields */
>> +#define TCR_ENAMODE_MASK        0xC0
>> +#define TCR_ENAMODE_ONESHOT_MASK    0x40
>> +#define TCR_ENAMODE_PERIODIC_MASK    0x80
>> +
>> +#define TGCR_TIM_UNRESET_MASK        0x03
>> +#define INTCTLSTAT_ENINT_MASK        0x01
>> +
>> +/**
>> + * struct keystone_timer: holds timer's data
>> + * @base: timer memory base address
>> + * @clk_rate: source clock rate
>> + * @irqacrion: interrupt action descriptor
>> + * @event_dev: event device based on timer
>> + */
>> +static struct keystone_timer {
>> +    void __iomem *base;
>> +    unsigned long clk_rate;
>> +    struct irqaction irqaction;
>> +    struct clock_event_device event_dev;
>> +} timer;
>> +
>> +static inline u32 keystone_timer_readl(unsigned long rg)
>> +{
>> +    return readl(timer.base + rg);
>> +}
>> +
>> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
>> +{
>> +    writel(val, timer.base + rg);
>> +}
>> +
>> +/**
>> + * keystone_timer_config: configures timer to work in 
>> oneshot/periodic modes, in
>> + * other cases disables timer.
>> + * @ mode: mode to configure
>> + * @ period: cycles number to configure for
>> + */
>> +static int keystone_timer_config(u64 period, enum clock_event_mode mode)
>> +{
>> +    u32 tcr;
>> +
>> +    tcr = keystone_timer_readl(TCR);
>> +
>> +    /* disable timer */
>> +    tcr &= ~(TCR_ENAMODE_MASK);
>> +    keystone_timer_writel(tcr, TCR);
> 
> Please move this write after the switch (cf. linked comment below)
> 

See below

>> +    /* set enable mode or leave disabled timer */
>> +    switch (mode) {
>> +    case CLOCK_EVT_MODE_ONESHOT:
>> +        tcr |= TCR_ENAMODE_ONESHOT_MASK;
>> +        break;
>> +    case CLOCK_EVT_MODE_PERIODIC:
>> +        tcr |= TCR_ENAMODE_PERIODIC_MASK;
>> +        break;
>> +    default:
>> +        return 0;
> 
> Shouldn't you return an error ?
> 

It returns value only for convenient usage in keystone_set_next_event(),
see [1].

In general, this function is used to configure the timer.
The procedure is that we need to stop the timer then set it.
If timer needs to be disabled (shutdown mode), function disables it and returns.

>> +    }
>> +
>> +    /* reset counter to zero, set new period */
>> +    keystone_timer_writel(0, TIM12);
>> +    keystone_timer_writel(0, TIM34);
>> +    keystone_timer_writel(period & 0xffffffff, PRD12);
>> +    keystone_timer_writel(period >> 32, PRD34);
>> +
>> +    /* enable timer */
>> +    keystone_timer_writel(tcr, TCR);
>> +    return 0;
>> +}
>> +
>> +static irqreturn_t keystone_timer_interrupt(int irq, void *dev_id)
>> +{
>> +    struct clock_event_device *evt = &timer.event_dev;
>> +
>> +    evt->event_handler(evt);
>> +    return IRQ_HANDLED;
>> +}
>> +
>> +static int keystone_set_next_event(unsigned long cycles,
>> +                  struct clock_event_device *evt)
>> +{
>> +    return keystone_timer_config(cycles, evt->mode);

[1]

>> +}
>> +
>> +static void keystone_set_mode(enum clock_event_mode mode,
>> +                 struct clock_event_device *evt)
>> +{
>> +    u64 period;
>> +
>> +    switch (mode) {
>> +    case CLOCK_EVT_MODE_PERIODIC:
>> +        period = timer.clk_rate / (HZ);
>> +        keystone_timer_config(period, CLOCK_EVT_MODE_PERIODIC);
>> +        break;
>> +    case CLOCK_EVT_MODE_UNUSED:
>> +    case CLOCK_EVT_MODE_SHUTDOWN:
>> +    case CLOCK_EVT_MODE_ONESHOT:
>> +        keystone_timer_config(0, CLOCK_EVT_MODE_SHUTDOWN);
> 
> I don't really this code assuming the side effect of specifying a 
> unsupported mode will disable the timer.
> 

In case of an unsupported mode the function does nothing.
Is it not correct?

It disables the timer only in 3 cases.
CLOCK_EVT_MODE_UNUSED
CLOCK_EVT_MODE_SHUTDOWN
CLOCK_EVT_MODE_ONESHOT

For CLOCK_EVT_MODE_ONESHOT,
It is supposed that then keystone_set_next_even() will re-enable the timer.

>> +    default:
>> +        break;
>> +    }
>> +}
>> +
>> +static void __init keystone_timer_init(struct device_node *np)
>> +{
>> +    struct clock_event_device *event_dev = &timer.event_dev;
>> +    struct irqaction *irqaction = &timer.irqaction;
>> +    unsigned long rate;
>> +    struct clk *clk;
>> +    int irq, error;
>> +    u32 tgcr;
>> +
>> +    irq  = irq_of_parse_and_map(np, 0);
>> +    if (irq == NO_IRQ) {
>> +        pr_err("%s: failed to map interrupts\n", __func__);
>> +        return;
>> +    }
>> +
>> +    timer.base = of_iomap(np, 0);
>> +    if (!timer.base) {
>> +        pr_err("%s: failed to map registers\n", __func__);
>> +        return;
>> +    }
>> +
>> +    clk = of_clk_get(np, 0);
>> +    if (!clk) {
>> +        pr_err("%s: failed to get clock\n", __func__);
>> +        iounmap(timer.base);
>> +        return;
>> +    }
>> +
>> +    error = clk_prepare_enable(clk);
>> +    if (error) {
>> +        pr_err("%s: failed to enable clock\n", __func__);
>> +        goto err;
>> +    }
>> +
>> +    rate = clk_get_rate(clk);
>> +
>> +    /* disable, use internal clock source */
>> +    keystone_timer_writel(0, TCR);
>> +
>> +    /* reset timer as 64-bit, no pre-scaler, plus features are 
>> disabled */
>> +    tgcr = 0;
>> +    keystone_timer_writel(0, TGCR);
>> +
>> +    /* unreset timer */
>> +    tgcr |= TGCR_TIM_UNRESET_MASK;
>> +    keystone_timer_writel(tgcr, TGCR);
>> +
>> +    /* init counter to zero */
>> +    keystone_timer_writel(0, TIM12);
>> +    keystone_timer_writel(0, TIM34);
>> +
>> +    timer.clk_rate = rate;
>> +
>> +    /* enable timer interrupts */
>> +    keystone_timer_writel(INTCTLSTAT_ENINT_MASK, INTCTLSTAT);
>> +
>> +    keystone_timer_config(0, CLOCK_EVT_MODE_UNUSED);
> 
> Same comment here.
> 

you are right, I disabled the timer above.., so
I may delete "keystone_timer_config(0, CLOCK_EVT_MODE_UNUSED);"

>> +    /* init irqaction */
>> +    irqaction->flags = IRQF_TIMER;
>> +    irqaction->handler = keystone_timer_interrupt;
>> +    irqaction->name = TIMER_NAME;
>> +    irqaction->dev_id = (void *)&timer;
>> +    error = setup_irq(irq, irqaction);
>> +    if (error) {
>> +        pr_err("%s: failed to setup irq\n", __func__);
>> +        goto err;
>> +    }
>> +
>> +    /* setup clockevent */
>> +    event_dev->features = CLOCK_EVT_FEAT_PERIODIC | 
>> CLOCK_EVT_FEAT_ONESHOT;
>> +    event_dev->set_next_event = keystone_set_next_event;
>> +    event_dev->set_mode = keystone_set_mode;
>> +    event_dev->cpumask = cpu_all_mask;
>> +    event_dev->owner = THIS_MODULE;
>> +    event_dev->name = TIMER_NAME;
>> +
>> +    clockevents_config_and_register(event_dev, rate, 1, ULONG_MAX);
>> +
>> +    pr_info("keystone timer clock @%lu Hz\n", rate);
>> +    return;
>> +err:
>> +    clk_put(clk);
>> +    iounmap(timer.base);
>> +}
>> +
>> +CLOCKSOURCE_OF_DECLARE(keystone_timer, "ti,keystone-timer",
>> +                    keystone_timer_init);
>>
> 
> 


-- 
Regards,
Ivan Khoronzhuk

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[Daniel Lezcano]

On 12/12/2013 06:36 PM, ivan.khoronzhuk wrote:
> On 12/12/2013 05:51 PM, Daniel Lezcano wrote:
>> On 12/11/2013 07:00 PM, Ivan Khoronzhuk wrote:
>>> Add broadcast clock-event device for the Keystone arch.
>>>
>>> The timer can be configured as a general-purpose 64-bit timer,
>>> dual general-purpose 32-bit timers. When configured as dual 32-bit
>>> timers, each half can operate in conjunction (chain mode) or
>>> independently (unchained mode) of each other.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> ---
>>>    drivers/clocksource/Makefile         |    1 +
>>>    drivers/clocksource/timer-keystone.c |  223
>>> ++++++++++++++++++++++++++++++++++
>>>    2 files changed, 224 insertions(+)
>>>    create mode 100644 drivers/clocksource/timer-keystone.c
>>>
>>> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
>>> index 33621ef..2acf3fc 100644
>>> --- a/drivers/clocksource/Makefile
>>> +++ b/drivers/clocksource/Makefile
>>> @@ -36,3 +36,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER)        += arm_arch_timer.o
>>>    obj-$(CONFIG_ARM_GLOBAL_TIMER)        += arm_global_timer.o
>>>    obj-$(CONFIG_CLKSRC_METAG_GENERIC)    += metag_generic.o
>>>    obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)    += dummy_timer.o
>>> +obj-$(CONFIG_ARCH_KEYSTONE)        += timer-keystone.o
>>> diff --git a/drivers/clocksource/timer-keystone.c
>>> b/drivers/clocksource/timer-keystone.c
>>> new file mode 100644
>>> index 0000000..4a8083a
>>> --- /dev/null
>>> +++ b/drivers/clocksource/timer-keystone.c
>>> @@ -0,0 +1,223 @@
>>> +/*
>>> + * Keystone broadcast clock-event
>>> + *
>>> + * Copyright 2013 Texas Instruments, Inc.
>>> + *
>>> + * Author: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> + *
>>> + * 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.
>>> + *
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/clockchips.h>
>>> +#include <linux/clocksource.h>
>>> +#include <linux/interrupt.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_irq.h>
>>> +
>>> +#define TIMER_NAME            "timer64-event"
>>> +
>>> +/* Timer register offsets */
>>> +#define TIM12                0x10
>>> +#define TIM34                0x14
>>> +#define PRD12                0x18
>>> +#define PRD34                0x1c
>>> +#define TCR                0x20
>>> +#define TGCR                0x24
>>> +#define INTCTLSTAT            0x44
>>> +
>>> +/* Timer register bitfields */
>>> +#define TCR_ENAMODE_MASK        0xC0
>>> +#define TCR_ENAMODE_ONESHOT_MASK    0x40
>>> +#define TCR_ENAMODE_PERIODIC_MASK    0x80
>>> +
>>> +#define TGCR_TIM_UNRESET_MASK        0x03
>>> +#define INTCTLSTAT_ENINT_MASK        0x01
>>> +
>>> +/**
>>> + * struct keystone_timer: holds timer's data
>>> + * @base: timer memory base address
>>> + * @clk_rate: source clock rate
>>> + * @irqacrion: interrupt action descriptor
>>> + * @event_dev: event device based on timer
>>> + */
>>> +static struct keystone_timer {
>>> +    void __iomem *base;
>>> +    unsigned long clk_rate;
>>> +    struct irqaction irqaction;
>>> +    struct clock_event_device event_dev;
>>> +} timer;
>>> +
>>> +static inline u32 keystone_timer_readl(unsigned long rg)
>>> +{
>>> +    return readl(timer.base + rg);
>>> +}
>>> +
>>> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
>>> +{
>>> +    writel(val, timer.base + rg);
>>> +}
>>> +
>>> +/**
>>> + * keystone_timer_config: configures timer to work in
>>> oneshot/periodic modes, in
>>> + * other cases disables timer.
>>> + * @ mode: mode to configure
>>> + * @ period: cycles number to configure for
>>> + */
>>> +static int keystone_timer_config(u64 period, enum clock_event_mode mode)
>>> +{
>>> +    u32 tcr;
>>> +
>>> +    tcr = keystone_timer_readl(TCR);
>>> +
>>> +    /* disable timer */
>>> +    tcr &= ~(TCR_ENAMODE_MASK);
>>> +    keystone_timer_writel(tcr, TCR);
>>
>> Please move this write after the switch (cf. linked comment below)
>>
>
> See below
>
>>> +    /* set enable mode or leave disabled timer */
>>> +    switch (mode) {
>>> +    case CLOCK_EVT_MODE_ONESHOT:
>>> +        tcr |= TCR_ENAMODE_ONESHOT_MASK;
>>> +        break;
>>> +    case CLOCK_EVT_MODE_PERIODIC:
>>> +        tcr |= TCR_ENAMODE_PERIODIC_MASK;
>>> +        break;
>>> +    default:
>>> +        return 0;
>>
>> Shouldn't you return an error ?
>>
>
> It returns value only for convenient usage in keystone_set_next_event(),
> see [1].
>
> In general, this function is used to configure the timer.
> The procedure is that we need to stop the timer then set it.
> If timer needs to be disabled (shutdown mode), function disables it and returns.
>
>>> +    }
>>> +
>>> +    /* reset counter to zero, set new period */
>>> +    keystone_timer_writel(0, TIM12);
>>> +    keystone_timer_writel(0, TIM34);
>>> +    keystone_timer_writel(period & 0xffffffff, PRD12);
>>> +    keystone_timer_writel(period >> 32, PRD34);
>>> +
>>> +    /* enable timer */
>>> +    keystone_timer_writel(tcr, TCR);
>>> +    return 0;
>>> +}
>>> +
>>> +static irqreturn_t keystone_timer_interrupt(int irq, void *dev_id)
>>> +{
>>> +    struct clock_event_device *evt = &timer.event_dev;
>>> +
>>> +    evt->event_handler(evt);
>>> +    return IRQ_HANDLED;
>>> +}
>>> +
>>> +static int keystone_set_next_event(unsigned long cycles,
>>> +                  struct clock_event_device *evt)
>>> +{
>>> +    return keystone_timer_config(cycles, evt->mode);
>
> [1]
>
>>> +}
>>> +
>>> +static void keystone_set_mode(enum clock_event_mode mode,
>>> +                 struct clock_event_device *evt)
>>> +{
>>> +    u64 period;
>>> +
>>> +    switch (mode) {
>>> +    case CLOCK_EVT_MODE_PERIODIC:
>>> +        period = timer.clk_rate / (HZ);
>>> +        keystone_timer_config(period, CLOCK_EVT_MODE_PERIODIC);
>>> +        break;
>>> +    case CLOCK_EVT_MODE_UNUSED:
>>> +    case CLOCK_EVT_MODE_SHUTDOWN:
>>> +    case CLOCK_EVT_MODE_ONESHOT:
>>> +        keystone_timer_config(0, CLOCK_EVT_MODE_SHUTDOWN);
>>
>> I don't really this code assuming the side effect of specifying a
>> unsupported mode will disable the timer.
>>
>
> In case of an unsupported mode the function does nothing.
> Is it not correct?
>
> It disables the timer only in 3 cases.
> CLOCK_EVT_MODE_UNUSED
> CLOCK_EVT_MODE_SHUTDOWN
> CLOCK_EVT_MODE_ONESHOT
>
> For CLOCK_EVT_MODE_ONESHOT,
> It is supposed that then keystone_set_next_even() will re-enable the timer.

The code looks like is doing what is expected. It is not about a bug but 
a coding style issue. I am not in favor of passing the 
CLOCK_EVT_MODE_SHUTDOWN which default in keystone_timer_config on 
returning no error. The caller assumes the function will exit without 
re-enabling the timer. It is correct from an implementation POV but IMO 
it is a bit subtle and prone to errors for the future modifications.

If keystone_timer_config(period, __A_VALUE_FROM_SPACE__) is called, it 
will return no error and disables the timer.

IMO, for the sake of clarity, defining the different modes in the switch 
and returning an error on default would be more convenient.

For this reason, you shouldn't disable the timer before checking the 
correctness of the mode, thus the comment about moving the timer 
disabling after the switch.

>>> +    default:
>>> +        break;
>>> +    }
>>> +}
>>> +
>>> +static void __init keystone_timer_init(struct device_node *np)
>>> +{
>>> +    struct clock_event_device *event_dev = &timer.event_dev;
>>> +    struct irqaction *irqaction = &timer.irqaction;
>>> +    unsigned long rate;
>>> +    struct clk *clk;
>>> +    int irq, error;
>>> +    u32 tgcr;
>>> +
>>> +    irq  = irq_of_parse_and_map(np, 0);
>>> +    if (irq == NO_IRQ) {
>>> +        pr_err("%s: failed to map interrupts\n", __func__);
>>> +        return;
>>> +    }
>>> +
>>> +    timer.base = of_iomap(np, 0);
>>> +    if (!timer.base) {
>>> +        pr_err("%s: failed to map registers\n", __func__);
>>> +        return;
>>> +    }
>>> +
>>> +    clk = of_clk_get(np, 0);
>>> +    if (!clk) {
>>> +        pr_err("%s: failed to get clock\n", __func__);
>>> +        iounmap(timer.base);
>>> +        return;
>>> +    }
>>> +
>>> +    error = clk_prepare_enable(clk);
>>> +    if (error) {
>>> +        pr_err("%s: failed to enable clock\n", __func__);
>>> +        goto err;
>>> +    }
>>> +
>>> +    rate = clk_get_rate(clk);
>>> +
>>> +    /* disable, use internal clock source */
>>> +    keystone_timer_writel(0, TCR);
>>> +
>>> +    /* reset timer as 64-bit, no pre-scaler, plus features are
>>> disabled */
>>> +    tgcr = 0;
>>> +    keystone_timer_writel(0, TGCR);
>>> +
>>> +    /* unreset timer */
>>> +    tgcr |= TGCR_TIM_UNRESET_MASK;
>>> +    keystone_timer_writel(tgcr, TGCR);
>>> +
>>> +    /* init counter to zero */
>>> +    keystone_timer_writel(0, TIM12);
>>> +    keystone_timer_writel(0, TIM34);
>>> +
>>> +    timer.clk_rate = rate;
>>> +
>>> +    /* enable timer interrupts */
>>> +    keystone_timer_writel(INTCTLSTAT_ENINT_MASK, INTCTLSTAT);
>>> +
>>> +    keystone_timer_config(0, CLOCK_EVT_MODE_UNUSED);
>>
>> Same comment here.
>>
>
> you are right, I disabled the timer above.., so
> I may delete "keystone_timer_config(0, CLOCK_EVT_MODE_UNUSED);"
>
>>> +    /* init irqaction */
>>> +    irqaction->flags = IRQF_TIMER;
>>> +    irqaction->handler = keystone_timer_interrupt;
>>> +    irqaction->name = TIMER_NAME;
>>> +    irqaction->dev_id = (void *)&timer;
>>> +    error = setup_irq(irq, irqaction);
>>> +    if (error) {
>>> +        pr_err("%s: failed to setup irq\n", __func__);
>>> +        goto err;
>>> +    }
>>> +
>>> +    /* setup clockevent */
>>> +    event_dev->features = CLOCK_EVT_FEAT_PERIODIC |
>>> CLOCK_EVT_FEAT_ONESHOT;
>>> +    event_dev->set_next_event = keystone_set_next_event;
>>> +    event_dev->set_mode = keystone_set_mode;
>>> +    event_dev->cpumask = cpu_all_mask;
>>> +    event_dev->owner = THIS_MODULE;
>>> +    event_dev->name = TIMER_NAME;
>>> +
>>> +    clockevents_config_and_register(event_dev, rate, 1, ULONG_MAX);
>>> +
>>> +    pr_info("keystone timer clock @%lu Hz\n", rate);
>>> +    return;
>>> +err:
>>> +    clk_put(clk);
>>> +    iounmap(timer.base);
>>> +}
>>> +
>>> +CLOCKSOURCE_OF_DECLARE(keystone_timer, "ti,keystone-timer",
>>> +                    keystone_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

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[ivan.khoronzhuk]

On 12/13/2013 08:55 AM, Daniel Lezcano wrote:
> On 12/12/2013 06:36 PM, ivan.khoronzhuk wrote:
>> On 12/12/2013 05:51 PM, Daniel Lezcano wrote:
>>> On 12/11/2013 07:00 PM, Ivan Khoronzhuk wrote:
>>>> Add broadcast clock-event device for the Keystone arch.
>>>>
>>>> The timer can be configured as a general-purpose 64-bit timer,
>>>> dual general-purpose 32-bit timers. When configured as dual 32-bit
>>>> timers, each half can operate in conjunction (chain mode) or
>>>> independently (unchained mode) of each other.
>>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>> ---
>>>>    drivers/clocksource/Makefile         |    1 +
>>>>    drivers/clocksource/timer-keystone.c |  223
>>>> ++++++++++++++++++++++++++++++++++
>>>>    2 files changed, 224 insertions(+)
>>>>    create mode 100644 drivers/clocksource/timer-keystone.c
>>>>
>>>> diff --git a/drivers/clocksource/Makefile 
>>>> b/drivers/clocksource/Makefile
>>>> index 33621ef..2acf3fc 100644
>>>> --- a/drivers/clocksource/Makefile
>>>> +++ b/drivers/clocksource/Makefile
>>>> @@ -36,3 +36,4 @@ obj-$(CONFIG_ARM_ARCH_TIMER)        += 
>>>> arm_arch_timer.o
>>>>    obj-$(CONFIG_ARM_GLOBAL_TIMER)        += arm_global_timer.o
>>>>    obj-$(CONFIG_CLKSRC_METAG_GENERIC)    += metag_generic.o
>>>>    obj-$(CONFIG_ARCH_HAS_TICK_BROADCAST)    += dummy_timer.o
>>>> +obj-$(CONFIG_ARCH_KEYSTONE)        += timer-keystone.o
>>>> diff --git a/drivers/clocksource/timer-keystone.c
>>>> b/drivers/clocksource/timer-keystone.c
>>>> new file mode 100644
>>>> index 0000000..4a8083a
>>>> --- /dev/null
>>>> +++ b/drivers/clocksource/timer-keystone.c
>>>> @@ -0,0 +1,223 @@
>>>> +/*
>>>> + * Keystone broadcast clock-event
>>>> + *
>>>> + * Copyright 2013 Texas Instruments, Inc.
>>>> + *
>>>> + * Author: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>> + *
>>>> + * 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.
>>>> + *
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/clockchips.h>
>>>> +#include <linux/clocksource.h>
>>>> +#include <linux/interrupt.h>
>>>> +#include <linux/of_address.h>
>>>> +#include <linux/of_irq.h>
>>>> +
>>>> +#define TIMER_NAME            "timer64-event"
>>>> +
>>>> +/* Timer register offsets */
>>>> +#define TIM12                0x10
>>>> +#define TIM34                0x14
>>>> +#define PRD12                0x18
>>>> +#define PRD34                0x1c
>>>> +#define TCR                0x20
>>>> +#define TGCR                0x24
>>>> +#define INTCTLSTAT            0x44
>>>> +
>>>> +/* Timer register bitfields */
>>>> +#define TCR_ENAMODE_MASK        0xC0
>>>> +#define TCR_ENAMODE_ONESHOT_MASK    0x40
>>>> +#define TCR_ENAMODE_PERIODIC_MASK    0x80
>>>> +
>>>> +#define TGCR_TIM_UNRESET_MASK        0x03
>>>> +#define INTCTLSTAT_ENINT_MASK        0x01
>>>> +
>>>> +/**
>>>> + * struct keystone_timer: holds timer's data
>>>> + * @base: timer memory base address
>>>> + * @clk_rate: source clock rate
>>>> + * @irqacrion: interrupt action descriptor
>>>> + * @event_dev: event device based on timer
>>>> + */
>>>> +static struct keystone_timer {
>>>> +    void __iomem *base;
>>>> +    unsigned long clk_rate;
>>>> +    struct irqaction irqaction;
>>>> +    struct clock_event_device event_dev;
>>>> +} timer;
>>>> +
>>>> +static inline u32 keystone_timer_readl(unsigned long rg)
>>>> +{
>>>> +    return readl(timer.base + rg);
>>>> +}
>>>> +
>>>> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
>>>> +{
>>>> +    writel(val, timer.base + rg);
>>>> +}
>>>> +
>>>> +/**
>>>> + * keystone_timer_config: configures timer to work in
>>>> oneshot/periodic modes, in
>>>> + * other cases disables timer.
>>>> + * @ mode: mode to configure
>>>> + * @ period: cycles number to configure for
>>>> + */
>>>> +static int keystone_timer_config(u64 period, enum clock_event_mode 
>>>> mode)
>>>> +{
>>>> +    u32 tcr;
>>>> +
>>>> +    tcr = keystone_timer_readl(TCR);
>>>> +
>>>> +    /* disable timer */
>>>> +    tcr &= ~(TCR_ENAMODE_MASK);
>>>> +    keystone_timer_writel(tcr, TCR);
>>>
>>> Please move this write after the switch (cf. linked comment below)
>>>
>>
>> See below
>>
>>>> +    /* set enable mode or leave disabled timer */
>>>> +    switch (mode) {
>>>> +    case CLOCK_EVT_MODE_ONESHOT:
>>>> +        tcr |= TCR_ENAMODE_ONESHOT_MASK;
>>>> +        break;
>>>> +    case CLOCK_EVT_MODE_PERIODIC:
>>>> +        tcr |= TCR_ENAMODE_PERIODIC_MASK;
>>>> +        break;
>>>> +    default:
>>>> +        return 0;
>>>
>>> Shouldn't you return an error ?
>>>
>>
>> It returns value only for convenient usage in keystone_set_next_event(),
>> see [1].
>>
>> In general, this function is used to configure the timer.
>> The procedure is that we need to stop the timer then set it.
>> If timer needs to be disabled (shutdown mode), function disables it 
>> and returns.
>>
>>>> +    }
>>>> +
>>>> +    /* reset counter to zero, set new period */
>>>> +    keystone_timer_writel(0, TIM12);
>>>> +    keystone_timer_writel(0, TIM34);
>>>> +    keystone_timer_writel(period & 0xffffffff, PRD12);
>>>> +    keystone_timer_writel(period >> 32, PRD34);
>>>> +
>>>> +    /* enable timer */
>>>> +    keystone_timer_writel(tcr, TCR);
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static irqreturn_t keystone_timer_interrupt(int irq, void *dev_id)
>>>> +{
>>>> +    struct clock_event_device *evt = &timer.event_dev;
>>>> +
>>>> +    evt->event_handler(evt);
>>>> +    return IRQ_HANDLED;
>>>> +}
>>>> +
>>>> +static int keystone_set_next_event(unsigned long cycles,
>>>> +                  struct clock_event_device *evt)
>>>> +{
>>>> +    return keystone_timer_config(cycles, evt->mode);
>>
>> [1]
>>
>>>> +}
>>>> +
>>>> +static void keystone_set_mode(enum clock_event_mode mode,
>>>> +                 struct clock_event_device *evt)
>>>> +{
>>>> +    u64 period;
>>>> +
>>>> +    switch (mode) {
>>>> +    case CLOCK_EVT_MODE_PERIODIC:
>>>> +        period = timer.clk_rate / (HZ);
>>>> +        keystone_timer_config(period, CLOCK_EVT_MODE_PERIODIC);
>>>> +        break;
>>>> +    case CLOCK_EVT_MODE_UNUSED:
>>>> +    case CLOCK_EVT_MODE_SHUTDOWN:
>>>> +    case CLOCK_EVT_MODE_ONESHOT:
>>>> +        keystone_timer_config(0, CLOCK_EVT_MODE_SHUTDOWN);
>>>
>>> I don't really this code assuming the side effect of specifying a
>>> unsupported mode will disable the timer.
>>>
>>
>> In case of an unsupported mode the function does nothing.
>> Is it not correct?
>>
>> It disables the timer only in 3 cases.
>> CLOCK_EVT_MODE_UNUSED
>> CLOCK_EVT_MODE_SHUTDOWN
>> CLOCK_EVT_MODE_ONESHOT
>>
>> For CLOCK_EVT_MODE_ONESHOT,
>> It is supposed that then keystone_set_next_even() will re-enable the 
>> timer.
> 
> The code looks like is doing what is expected. It is not about a bug but 
> a coding style issue. I am not in favor of passing the 
> CLOCK_EVT_MODE_SHUTDOWN which default in keystone_timer_config on 
> returning no error. The caller assumes the function will exit without 
> re-enabling the timer. It is correct from an implementation POV but IMO 
> it is a bit subtle and prone to errors for the future modifications.
> 
> If keystone_timer_config(period, __A_VALUE_FROM_SPACE__) is called, it 
> will return no error and disables the timer.
> 
> IMO, for the sake of clarity, defining the different modes in the switch 
> and returning an error on default would be more convenient.
> 
> For this reason, you shouldn't disable the timer before checking the 
> correctness of the mode, thus the comment about moving the timer 
> disabling after the switch.
> 

Ok,
I'll add separate function "keystone_timer_disable()" for using in keystone_set_mode().
The keystone_timer_config() will not be used for disabling the timer any more.
In case of an unsupported mode the keystone_timer_config() will return -1.


-- 
Regards,
Ivan Khoronzhuk

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[Stephen Boyd]

On 12/11/13 10:00, Ivan Khoronzhuk wrote:
> diff --git a/drivers/clocksource/timer-keystone.c b/drivers/clocksource/timer-keystone.c
> new file mode 100644
> index 0000000..4a8083a
> --- /dev/null
> +++ b/drivers/clocksource/timer-keystone.c
[...]
> +#include <linux/interrupt.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +
> +#define TIMER_NAME			"timer64-event"

Why not have keystone in the name? Or should this file be renamed?

> +
> +static inline u32 keystone_timer_readl(unsigned long rg)
> +{
> +	return readl(timer.base + rg);
> +}
> +
> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
> +{
> +	writel(val, timer.base + rg);
> +}

It's probably better to use the relaxed variants here to avoid any
memory barriers that aren't necessary.

> +
> +static irqreturn_t keystone_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = &timer.event_dev;
> +
> +	evt->event_handler(evt);
> +	return IRQ_HANDLED;
> +}

Why not just pass the evt pointer via dev_id? That would be faster and
we want this function to be as fast as possible.

> +
> +static int keystone_set_next_event(unsigned long cycles,
> +				  struct clock_event_device *evt)
> +{
> +	return keystone_timer_config(cycles, evt->mode);
> +}
> +
> +static void keystone_set_mode(enum clock_event_mode mode,
> +			     struct clock_event_device *evt)
> +{
> +	u64 period;
> +
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		period = timer.clk_rate / (HZ);

Why not just store the period instead of calculating it here?

> +		keystone_timer_config(period, CLOCK_EVT_MODE_PERIODIC);
> +		break;
> +	case CLOCK_EVT_MODE_UNUSED:
> +	case CLOCK_EVT_MODE_SHUTDOWN:
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		keystone_timer_config(0, CLOCK_EVT_MODE_SHUTDOWN);
> +	default:
> +		break;
> +	}
> +}
> +
> +static void __init keystone_timer_init(struct device_node *np)
> +{
[...]
> +
> +	/* enable timer interrupts */
> +	keystone_timer_writel(INTCTLSTAT_ENINT_MASK, INTCTLSTAT);
> +
> +	keystone_timer_config(0, CLOCK_EVT_MODE_UNUSED);
> +
> +	/* init irqaction */
> +	irqaction->flags = IRQF_TIMER;
> +	irqaction->handler = keystone_timer_interrupt;
> +	irqaction->name = TIMER_NAME;
> +	irqaction->dev_id = (void *)&timer;
> +	error = setup_irq(irq, irqaction);
> +	if (error) {
> +		pr_err("%s: failed to setup irq\n", __func__);
> +		goto err;
> +	}

Why not use request_irq() here? This isn't called really early, is it?

> +
> +	/* setup clockevent */
> +	event_dev->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
> +	event_dev->set_next_event = keystone_set_next_event;
> +	event_dev->set_mode = keystone_set_mode;
> +	event_dev->cpumask = cpu_all_mask;
> +	event_dev->owner = THIS_MODULE;
> +	event_dev->name = TIMER_NAME;

Please assign the irq here too.

> +
> +	clockevents_config_and_register(event_dev, rate, 1, ULONG_MAX);
> +
> +	pr_info("keystone timer clock @%lu Hz\n", rate);
> +	return;
> +err:
> +	clk_put(clk);
> +	iounmap(timer.base);
> +}
> +
> +CLOCKSOURCE_OF_DECLARE(keystone_timer, "ti,keystone-timer",
> +					keystone_timer_init);


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[ivan.khoronzhuk]

On 12/13/2013 03:42 AM, Stephen Boyd wrote:
> On 12/11/13 10:00, Ivan Khoronzhuk wrote:
>> diff --git a/drivers/clocksource/timer-keystone.c b/drivers/clocksource/timer-keystone.c
>> new file mode 100644
>> index 0000000..4a8083a
>> --- /dev/null
>> +++ b/drivers/clocksource/timer-keystone.c
> [...]
>> +#include <linux/interrupt.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +
>> +#define TIMER_NAME			"timer64-event"
> 
> Why not have keystone in the name? Or should this file be renamed?
> 

Ok, I'll rename on "timer-keystone"

>> +
>> +static inline u32 keystone_timer_readl(unsigned long rg)
>> +{
>> +	return readl(timer.base + rg);
>> +}
>> +
>> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
>> +{
>> +	writel(val, timer.base + rg);
>> +}
> 
> It's probably better to use the relaxed variants here to avoid any
> memory barriers that aren't necessary.
>

Yes, but the code has places where I cannot use relaxed variants.

>From timer user guide:
"Writes from the configuration bus to the timer registers are not allowed
when the timer is active, except for stopping or resetting the timers.
Registers that are protected by hardware include CNTLO, CNTHI, PRDLO,
PRDHI, TGCR (except the TIMLORS and TIMHIRS bits), and TCR (except the
ENAMODE bits)."

According to this I have to add keystone readl/write relaxed functions
and use mixed calls of writel/writel_relaxed functions.

For instance, for keystone_timer_config() which is used by
keystone_set_next_event(), I will do following:
-----
tcr = keystone_timer_readl_relaxed(TCR);

/* disable timer */
tcr &= ~(TCR_ENAMODE_MASK);
keystone_timer_writel_relaxed(tcr, TCR);
...
/* reset counter to zero, set new period */
*** here I have to be sure the timer is disabled ***
keystone_timer_writel(0, TIM12);
keystone_timer_writel_relaxed(0, TIM34);
keystone_timer_writel_relaxed(period & 0xffffffff, PRD12);
keystone_timer_writel_relaxed(period >> 32, PRD34);

/* enable timer */
*** here I have to be sure that TIM and PRD registers are written ***
keystone_timer_writel(tcr, TCR);
---

The same for keystone_timer_init().

Will it be better?

>> +
>> +static irqreturn_t keystone_timer_interrupt(int irq, void *dev_id)
>> +{
>> +	struct clock_event_device *evt = &timer.event_dev;
>> +
>> +	evt->event_handler(evt);
>> +	return IRQ_HANDLED;
>> +}
> 
> Why not just pass the evt pointer via dev_id? That would be faster and
> we want this function to be as fast as possible.
> 

Ok, I will pass clock_event_device via dev_id.

>> +
>> +static int keystone_set_next_event(unsigned long cycles,
>> +				  struct clock_event_device *evt)
>> +{
>> +	return keystone_timer_config(cycles, evt->mode);
>> +}
>> +
>> +static void keystone_set_mode(enum clock_event_mode mode,
>> +			     struct clock_event_device *evt)
>> +{
>> +	u64 period;
>> +
>> +	switch (mode) {
>> +	case CLOCK_EVT_MODE_PERIODIC:
>> +		period = timer.clk_rate / (HZ);
> 
> Why not just store the period instead of calculating it here?
> 

Ok, I'll add timer.hz_period and delete timer.clk_rate.


>> +		keystone_timer_config(period, CLOCK_EVT_MODE_PERIODIC);
>> +		break;
>> +	case CLOCK_EVT_MODE_UNUSED:
>> +	case CLOCK_EVT_MODE_SHUTDOWN:
>> +	case CLOCK_EVT_MODE_ONESHOT:
>> +		keystone_timer_config(0, CLOCti,keystone-timerK_EVT_MODE_SHUTDOWN);
>> +	default:
>> +		break;
>> +	}
>> +}
>> +
>> +static void __init keystone_timer_init(struct device_node *np)
>> +{
> [...]
>> +
>> +	/* enable timer interrupts */
>> +	keystone_timer_writel(INTCTLSTAT_ENINT_MASK, INTCTLSTAT);
>> +
>> +	keystone_timer_config(0, CLOCK_EVT_MODE_UNUSED);
>> +
>> +	/* init irqaction */
>> +	irqaction->flags = IRQF_TIMER;
>> +	irqaction->handler = keystone_timer_interrupt;
>> +	irqaction->name = TIMER_NAME;
>> +	irqaction->dev_id = (void *)&timer;
>> +	error = setup_irq(irq, irqaction);
>> +	if (error) {
>> +		pr_err("%s: failed to setup irq\n", __func__);
>> +		goto err;
>> +	}
> 
> Why not use request_irq() here? This isn't called really early, is it?
> 

It is called at time_init(), that is after init_IRQ().
Additionally I've checked it. I'll replace.


>> +
>> +	/* setup clockevent */
>> +	event_dev->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT;
>> +	event_dev->set_next_event = keystone_set_next_event;
>> +	event_dev->set_mode = keystone_set_mode;
>> +	event_dev->cpumask = cpu_all_mask;
>> +	event_dev->owner = THIS_MODULE;
>> +	event_dev->name = TIMER_NAME;
> 
> Please assign the irq here too.
> 

Ok, event_dev->irq = irq;

>> +
>> +	clockevents_config_and_register(event_dev, rate, 1, ULONG_MAX);
>> +
>> +	pr_info("keystone timer clock @%lu Hz\n", rate);
>> +	return;
>> +err:
>> +	clk_put(clk);
>> +	iounmap(timer.base);
>> +}
>> +
>> +CLOCKSOURCE_OF_DECLARE(keystone_timer, "ti,keystone-timer",
>> +					keystone_timer_init);
> 
> 


-- 
Regards,
Ivan Khoronzhuk

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[Stephen Boyd]

On 12/16, ivan.khoronzhuk wrote:
> On 12/13/2013 03:42 AM, Stephen Boyd wrote:
> > On 12/11/13 10:00, Ivan Khoronzhuk wrote:
> >> +
> >> +static inline u32 keystone_timer_readl(unsigned long rg)
> >> +{
> >> +	return readl(timer.base + rg);
> >> +}
> >> +
> >> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
> >> +{
> >> +	writel(val, timer.base + rg);
> >> +}
> > 
> > It's probably better to use the relaxed variants here to avoid any
> > memory barriers that aren't necessary.
> >
> 
> Yes, but the code has places where I cannot use relaxed variants.
> 
> From timer user guide:
> "Writes from the configuration bus to the timer registers are not allowed
> when the timer is active, except for stopping or resetting the timers.
> Registers that are protected by hardware include CNTLO, CNTHI, PRDLO,
> PRDHI, TGCR (except the TIMLORS and TIMHIRS bits), and TCR (except the
> ENAMODE bits)."
> 
> According to this I have to add keystone readl/write relaxed functions
> and use mixed calls of writel/writel_relaxed functions.
> 
> For instance, for keystone_timer_config() which is used by
> keystone_set_next_event(), I will do following:
> -----
> tcr = keystone_timer_readl_relaxed(TCR);
> 
> /* disable timer */
> tcr &= ~(TCR_ENAMODE_MASK);
> keystone_timer_writel_relaxed(tcr, TCR);
> ...
> /* reset counter to zero, set new period */
> *** here I have to be sure the timer is disabled ***
> keystone_timer_writel(0, TIM12);
> keystone_timer_writel_relaxed(0, TIM34);
> keystone_timer_writel_relaxed(period & 0xffffffff, PRD12);
> keystone_timer_writel_relaxed(period >> 32, PRD34);
> 
> /* enable timer */
> *** here I have to be sure that TIM and PRD registers are written ***
> keystone_timer_writel(tcr, TCR);
> ---
> 
> The same for keystone_timer_init().
> 
> Will it be better?

Why not just put the explicit memory barriers where you need them
and always use the relaxed variants in the mmio wrappers? That
way you're always sure the memory barriers are there and that
they're properly documented.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[Santosh Shilimkar]

On Monday 16 December 2013 03:40 PM, Stephen Boyd wrote:
> On 12/16, ivan.khoronzhuk wrote:
>> On 12/13/2013 03:42 AM, Stephen Boyd wrote:
>>> On 12/11/13 10:00, Ivan Khoronzhuk wrote:
>>>> +
>>>> +static inline u32 keystone_timer_readl(unsigned long rg)
>>>> +{
>>>> +	return readl(timer.base + rg);
>>>> +}
>>>> +
>>>> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
>>>> +{
>>>> +	writel(val, timer.base + rg);
>>>> +}
>>>
>>> It's probably better to use the relaxed variants here to avoid any
>>> memory barriers that aren't necessary.
>>>
>>
>> Yes, but the code has places where I cannot use relaxed variants.
>>
>> From timer user guide:
>> "Writes from the configuration bus to the timer registers are not allowed
>> when the timer is active, except for stopping or resetting the timers.
>> Registers that are protected by hardware include CNTLO, CNTHI, PRDLO,
>> PRDHI, TGCR (except the TIMLORS and TIMHIRS bits), and TCR (except the
>> ENAMODE bits)."
>>
>> According to this I have to add keystone readl/write relaxed functions
>> and use mixed calls of writel/writel_relaxed functions.
>>
>> For instance, for keystone_timer_config() which is used by
>> keystone_set_next_event(), I will do following:
>> -----
>> tcr = keystone_timer_readl_relaxed(TCR);
>>
>> /* disable timer */
>> tcr &= ~(TCR_ENAMODE_MASK);
>> keystone_timer_writel_relaxed(tcr, TCR);
>> ...
>> /* reset counter to zero, set new period */
>> *** here I have to be sure the timer is disabled ***
>> keystone_timer_writel(0, TIM12);
>> keystone_timer_writel_relaxed(0, TIM34);
>> keystone_timer_writel_relaxed(period & 0xffffffff, PRD12);
>> keystone_timer_writel_relaxed(period >> 32, PRD34);
>>
>> /* enable timer */
>> *** here I have to be sure that TIM and PRD registers are written ***
>> keystone_timer_writel(tcr, TCR);
>> ---
>>
>> The same for keystone_timer_init().
>>
>> Will it be better?
> 
> Why not just put the explicit memory barriers where you need them
> and always use the relaxed variants in the mmio wrappers? That
> way you're always sure the memory barriers are there and that
> they're properly documented.
> 
I agree with Stephen.

Regards,
Santosh

[PATCH 1/3] clocksource: timer-keystone: introduce clocksource driver for Keystone

[ivan.khoronzhuk]

On 12/16/2013 10:55 PM, Santosh Shilimkar wrote:
> On Monday 16 December 2013 03:40 PM, Stephen Boyd wrote:
>> On 12/16, ivan.khoronzhuk wrote:
>>> On 12/13/2013 03:42 AM, Stephen Boyd wrote:
>>>> On 12/11/13 10:00, Ivan Khoronzhuk wrote:
>>>>> +
>>>>> +static inline u32 keystone_timer_readl(unsigned long rg)
>>>>> +{
>>>>> +	return readl(timer.base + rg);
>>>>> +}
>>>>> +
>>>>> +static inline void keystone_timer_writel(u32 val, unsigned long rg)
>>>>> +{
>>>>> +	writel(val, timer.base + rg);
>>>>> +}
>>>>
>>>> It's probably better to use the relaxed variants here to avoid any
>>>> memory barriers that aren't necessary.
>>>>
>>>
>>> Yes, but the code has places where I cannot use relaxed variants.
>>>
>>>  From timer user guide:
>>> "Writes from the configuration bus to the timer registers are not allowed
>>> when the timer is active, except for stopping or resetting the timers.
>>> Registers that are protected by hardware include CNTLO, CNTHI, PRDLO,
>>> PRDHI, TGCR (except the TIMLORS and TIMHIRS bits), and TCR (except the
>>> ENAMODE bits)."
>>>
>>> According to this I have to add keystone readl/write relaxed functions
>>> and use mixed calls of writel/writel_relaxed functions.
>>>
>>> For instance, for keystone_timer_config() which is used by
>>> keystone_set_next_event(), I will do following:
>>> -----
>>> tcr = keystone_timer_readl_relaxed(TCR);
>>>
>>> /* disable timer */
>>> tcr &= ~(TCR_ENAMODE_MASK);
>>> keystone_timer_writel_relaxed(tcr, TCR);
>>> ...
>>> /* reset counter to zero, set new period */
>>> *** here I have to be sure the timer is disabled ***
>>> keystone_timer_writel(0, TIM12);
>>> keystone_timer_writel_relaxed(0, TIM34);
>>> keystone_timer_writel_relaxed(period & 0xffffffff, PRD12);
>>> keystone_timer_writel_relaxed(period >> 32, PRD34);
>>>
>>> /* enable timer */
>>> *** here I have to be sure that TIM and PRD registers are written ***
>>> keystone_timer_writel(tcr, TCR);
>>> ---
>>>
>>> The same for keystone_timer_init().
>>>
>>> Will it be better?
>>
>> Why not just put the explicit memory barriers where you need them
>> and always use the relaxed variants in the mmio wrappers? That
>> way you're always sure the memory barriers are there and that
>> they're properly documented.
>>
> I agree with Stephen.
>
> Regards,
> Santosh
>

Yes, it will be better

-- 
Regards,
Ivan Khoronzhuk

[PATCH 2/3] clocksource: keystone: add bindings for keystone timer

[Ivan Khoronzhuk]

This patch provides bindings for the 64-bit timer in the KeyStone
architecture devices. The timer can be configured as a general-purpose 64-bit
timer, dual general-purpose 32-bit timers. When configured as dual 32-bit
timers, each half can operate in conjunction (chain mode) or independently
(unchained mode) of each other.

It is global timer is a free running up-counter and can generate interrupt
when the counter reaches preset counter values.

Documentation:
http://www.ti.com/lit/ug/sprugv5a/sprugv5a.pdf

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 .../bindings/timer/ti,keystone-timer.txt           |   29 ++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/ti,keystone-timer.txt

diff --git a/Documentation/devicetree/bindings/timer/ti,keystone-timer.txt b/Documentation/devicetree/bindings/timer/ti,keystone-timer.txt
new file mode 100644
index 0000000..5fbe361
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/ti,keystone-timer.txt
@@ -0,0 +1,29 @@
+* Device tree bindings for Texas instruments Keystone timer
+
+This document provides bindings for the 64-bit timer in the KeyStone
+architecture devices. The timer can be configured as a general-purpose 64-bit
+timer, dual general-purpose 32-bit timers. When configured as dual 32-bit
+timers, each half can operate in conjunction (chain mode) or independently
+(unchained mode) of each other.
+
+It is global timer is a free running up-counter and can generate interrupt
+when the counter reaches preset counter values.
+
+Documentation:
+http://www.ti.com/lit/ug/sprugv5a/sprugv5a.pdf
+
+Required properties:
+
+- compatible : should be "ti,keystone-timer".
+- reg : specifies base physical address and count of the registers.
+- interrupts : interrupt generated by the timer.
+- clocks : the clock feeding the timer clock.
+
+Example:
+
+timer at 22f0000 {
+	compatible = "ti,keystone-timer";
+	reg = <0x022f0000 0x80>;
+	interrupts = <GIC_SPI 110 IRQ_TYPE_EDGE_RISING>;
+	clocks = <&clktimer15>;
+};
-- 
1.7.9.5

[PATCH 3/3] arm: dts: keystone: add keystone timer entry

[Ivan Khoronzhuk]

Add keystone timer entry to keystone device tree.
This 64-bit timer is used as backup clock event device.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 arch/arm/boot/dts/keystone-clocks.dtsi |   10 ++++++++++
 arch/arm/boot/dts/keystone.dtsi        |    7 +++++++
 2 files changed, 17 insertions(+)

diff --git a/arch/arm/boot/dts/keystone-clocks.dtsi b/arch/arm/boot/dts/keystone-clocks.dtsi
index 2363593..16d2aba 100644
--- a/arch/arm/boot/dts/keystone-clocks.dtsi
+++ b/arch/arm/boot/dts/keystone-clocks.dtsi
@@ -737,6 +737,16 @@ clocks {
 		domain-id = <0>;
 	};
 
+	clktimer15: clktimer15 {
+		#clock-cells = <0>;
+		compatible = "ti,keystone,psc-clock";
+		clocks = <&clkmodrst0>;
+		clock-output-names = "timer15";
+		reg = <0x02350000 0xb00>, <0x02350000 0x400>;
+		reg-names = "control", "domain";
+		domain-id = <0>;
+	};
+
 	clkuart0: clkuart0 {
 		#clock-cells = <0>;
 		compatible = "ti,keystone,psc-clock";
diff --git a/arch/arm/boot/dts/keystone.dtsi b/arch/arm/boot/dts/keystone.dtsi
index f6d6d9e..3f05c07 100644
--- a/arch/arm/boot/dts/keystone.dtsi
+++ b/arch/arm/boot/dts/keystone.dtsi
@@ -181,5 +181,12 @@
 			interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;
 			clocks = <&clkspi>;
 		};
+
+		clock_event: timer at 22f0000 {
+			compatible = "ti,keystone-timer";
+			reg = <0x022f0000 0x80>;
+			interrupts = <GIC_SPI 110 IRQ_TYPE_EDGE_RISING>;
+			clocks = <&clktimer15>;
+		};
 	};
 };
-- 
1.7.9.5