All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-18 10:00 ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-06-18 10:00 UTC (permalink / raw)
  To: linux-arm-kernel; +Cc: linux-kernel, arm, john.stultz, tglx, Jonas Jensen

This patch adds an clocksource driver for the main timer found
on MOXA ART SoCs.

Applies to 3.10-rc1 and arm-soc/for-next (2013-06-15)

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---
 drivers/clocksource/Makefile       |    1 +
 drivers/clocksource/moxart_timer.c |  129 ++++++++++++++++++++++++++++++++++++
 2 files changed, 130 insertions(+), 0 deletions(-)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..c93e1a8 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..ce5a5a2
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,129 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/clocksource.h>
+
+#include <asm/mach/time.h>
+
+#define APB_CLK 48000000
+
+#define TIMER_1_COUNT(base_addr)        (base_addr + 0x00)
+#define TIMER_1_LOAD(base_addr)         (base_addr + 0x04)
+#define TIMER_1_MATCH1(base_addr)       (base_addr + 0x08)
+#define TIMER_1_MATCH2(base_addr)       (base_addr + 0x0C)
+
+#define TIMER_2_COUNT(base_addr)        (base_addr + 0x10)
+#define TIMER_2_LOAD(base_addr)         (base_addr + 0x14)
+#define TIMER_2_MATCH1(base_addr)       (base_addr + 0x18)
+#define TIMER_2_MATCH2(base_addr)       (base_addr + 0x1C)
+
+#define TIMER_3_COUNT(base_addr)        (base_addr + 0x20)
+#define TIMER_3_LOAD(base_addr)         (base_addr + 0x24)
+#define TIMER_3_MATCH1(base_addr)       (base_addr + 0x28)
+#define TIMER_3_MATCH2(base_addr)       (base_addr + 0x2C)
+
+#define TIMER_CR(base_addr)             (base_addr + 0x30)
+
+#define TIMER_1_CR_ENABLE(base_addr)    (base_addr + 0x30)
+#define TIMER_1_CR_EXTCLK(base_addr)    (base_addr + 0x34)
+#define TIMER_1_CR_FLOWIN(base_addr)    (base_addr + 0x38)
+
+#define TIMER_2_CR_ENABLE(base_addr)    (base_addr + 0x42)
+#define TIMER_2_CR_EXTCLK(base_addr)    (base_addr + 0x46)
+#define TIMER_2_CR_FLOWIN(base_addr)    (base_addr + 0x50)
+
+#define TIMER_3_CR_ENABLE(base_addr)    (base_addr + 0x54)
+#define TIMER_3_CR_EXTCLK(base_addr)    (base_addr + 0x58)
+#define TIMER_3_CR_FLOWIN(base_addr)    (base_addr + 0x62)
+
+#define TIMER_INTR_STATE(base_addr)     (base_addr + 0x34)
+
+#define TIMEREG_1_CR_ENABLE         (1 << 0)
+#define TIMEREG_1_CR_CLOCK          (1 << 1)
+#define TIMEREG_1_CR_INT            (1 << 2)
+#define TIMEREG_2_CR_ENABLE         (1 << 3)
+#define TIMEREG_2_CR_CLOCK          (1 << 4)
+#define TIMEREG_2_CR_INT            (1 << 5)
+#define TIMEREG_3_CR_ENABLE         (1 << 6)
+#define TIMEREG_3_CR_CLOCK          (1 << 7)
+#define TIMEREG_3_CR_INT            (1 << 8)
+#define TIMEREG_COUNT_UP            (1 << 9)
+#define TIMEREG_COUNT_DOWN          (0 << 9)
+
+#define MAX_TIMER   2
+#define USED_TIMER  1
+
+#define TIMER1_COUNT                0x0
+#define TIMER1_LOAD                 0x4
+#define TIMER1_MATCH1               0x8
+#define TIMER1_MATCH2               0xC
+#define TIMER2_COUNT                0x10
+#define TIMER2_LOAD                 0x14
+#define TIMER2_MATCH1               0x18
+#define TIMER2_MATCH2               0x1C
+#define TIMER3_COUNT                0x20
+#define TIMER3_LOAD                 0x24
+#define TIMER3_MATCH1               0x28
+#define TIMER3_MATCH2               0x2C
+#define TIMER_INTR_MASK     0x38
+
+static void __iomem *timer_base;
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	timer_tick();
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name = "moxart-timer",
+	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
+	.handler = moxart_timer_interrupt,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+
+	timer_base = of_iomap(node, 0);
+	if (!timer_base)
+		panic("%s: failed to map base\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: can't parse IRQ\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		pr_warn("%s: failed to setup IRQ %d\n", node->full_name, irq);
+
+
+	writel(APB_CLK / HZ, TIMER_1_COUNT(timer_base));
+	writel(APB_CLK / HZ, TIMER_1_LOAD(timer_base));
+
+	writel(1, TIMER_1_CR_ENABLE(timer_base));
+	writel(0, TIMER_1_CR_EXTCLK(timer_base));
+	writel(1, TIMER_1_CR_FLOWIN(timer_base));
+
+	pr_info("%s: count/load (APB_CLK=%d/HZ=%d) IRQ=%d\n",
+		node->full_name, APB_CLK, HZ, irq);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
+
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-18 10:00 ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-06-18 10:00 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds an clocksource driver for the main timer found
on MOXA ART SoCs.

Applies to 3.10-rc1 and arm-soc/for-next (2013-06-15)

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---
 drivers/clocksource/Makefile       |    1 +
 drivers/clocksource/moxart_timer.c |  129 ++++++++++++++++++++++++++++++++++++
 2 files changed, 130 insertions(+), 0 deletions(-)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..c93e1a8 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..ce5a5a2
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,129 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/clocksource.h>
+
+#include <asm/mach/time.h>
+
+#define APB_CLK 48000000
+
+#define TIMER_1_COUNT(base_addr)        (base_addr + 0x00)
+#define TIMER_1_LOAD(base_addr)         (base_addr + 0x04)
+#define TIMER_1_MATCH1(base_addr)       (base_addr + 0x08)
+#define TIMER_1_MATCH2(base_addr)       (base_addr + 0x0C)
+
+#define TIMER_2_COUNT(base_addr)        (base_addr + 0x10)
+#define TIMER_2_LOAD(base_addr)         (base_addr + 0x14)
+#define TIMER_2_MATCH1(base_addr)       (base_addr + 0x18)
+#define TIMER_2_MATCH2(base_addr)       (base_addr + 0x1C)
+
+#define TIMER_3_COUNT(base_addr)        (base_addr + 0x20)
+#define TIMER_3_LOAD(base_addr)         (base_addr + 0x24)
+#define TIMER_3_MATCH1(base_addr)       (base_addr + 0x28)
+#define TIMER_3_MATCH2(base_addr)       (base_addr + 0x2C)
+
+#define TIMER_CR(base_addr)             (base_addr + 0x30)
+
+#define TIMER_1_CR_ENABLE(base_addr)    (base_addr + 0x30)
+#define TIMER_1_CR_EXTCLK(base_addr)    (base_addr + 0x34)
+#define TIMER_1_CR_FLOWIN(base_addr)    (base_addr + 0x38)
+
+#define TIMER_2_CR_ENABLE(base_addr)    (base_addr + 0x42)
+#define TIMER_2_CR_EXTCLK(base_addr)    (base_addr + 0x46)
+#define TIMER_2_CR_FLOWIN(base_addr)    (base_addr + 0x50)
+
+#define TIMER_3_CR_ENABLE(base_addr)    (base_addr + 0x54)
+#define TIMER_3_CR_EXTCLK(base_addr)    (base_addr + 0x58)
+#define TIMER_3_CR_FLOWIN(base_addr)    (base_addr + 0x62)
+
+#define TIMER_INTR_STATE(base_addr)     (base_addr + 0x34)
+
+#define TIMEREG_1_CR_ENABLE         (1 << 0)
+#define TIMEREG_1_CR_CLOCK          (1 << 1)
+#define TIMEREG_1_CR_INT            (1 << 2)
+#define TIMEREG_2_CR_ENABLE         (1 << 3)
+#define TIMEREG_2_CR_CLOCK          (1 << 4)
+#define TIMEREG_2_CR_INT            (1 << 5)
+#define TIMEREG_3_CR_ENABLE         (1 << 6)
+#define TIMEREG_3_CR_CLOCK          (1 << 7)
+#define TIMEREG_3_CR_INT            (1 << 8)
+#define TIMEREG_COUNT_UP            (1 << 9)
+#define TIMEREG_COUNT_DOWN          (0 << 9)
+
+#define MAX_TIMER   2
+#define USED_TIMER  1
+
+#define TIMER1_COUNT                0x0
+#define TIMER1_LOAD                 0x4
+#define TIMER1_MATCH1               0x8
+#define TIMER1_MATCH2               0xC
+#define TIMER2_COUNT                0x10
+#define TIMER2_LOAD                 0x14
+#define TIMER2_MATCH1               0x18
+#define TIMER2_MATCH2               0x1C
+#define TIMER3_COUNT                0x20
+#define TIMER3_LOAD                 0x24
+#define TIMER3_MATCH1               0x28
+#define TIMER3_MATCH2               0x2C
+#define TIMER_INTR_MASK     0x38
+
+static void __iomem *timer_base;
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	timer_tick();
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name = "moxart-timer",
+	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
+	.handler = moxart_timer_interrupt,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+
+	timer_base = of_iomap(node, 0);
+	if (!timer_base)
+		panic("%s: failed to map base\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: can't parse IRQ\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		pr_warn("%s: failed to setup IRQ %d\n", node->full_name, irq);
+
+
+	writel(APB_CLK / HZ, TIMER_1_COUNT(timer_base));
+	writel(APB_CLK / HZ, TIMER_1_LOAD(timer_base));
+
+	writel(1, TIMER_1_CR_ENABLE(timer_base));
+	writel(0, TIMER_1_CR_EXTCLK(timer_base));
+	writel(1, TIMER_1_CR_FLOWIN(timer_base));
+
+	pr_info("%s: count/load (APB_CLK=%d/HZ=%d) IRQ=%d\n",
+		node->full_name, APB_CLK, HZ, irq);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
+
-- 
1.7.2.5

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH] ARM: clocksource: add support for MOXA ART SoCs
  2013-06-18 10:00 ` Jonas Jensen
@ 2013-06-18 15:14   ` Thomas Petazzoni
  -1 siblings, 0 replies; 62+ messages in thread
From: Thomas Petazzoni @ 2013-06-18 15:14 UTC (permalink / raw)
  To: Jonas Jensen; +Cc: linux-arm-kernel, tglx, arm, john.stultz, linux-kernel

Dear Jonas Jensen,

On Tue, 18 Jun 2013 12:00:04 +0200, Jonas Jensen wrote:

> +static void __init moxart_timer_init(struct device_node *node)
> +{
> +	int ret, irq;
> +
> +	timer_base = of_iomap(node, 0);
> +	if (!timer_base)
> +		panic("%s: failed to map base\n", node->full_name);
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0)
> +		panic("%s: can't parse IRQ\n", node->full_name);
> +
> +	ret = setup_irq(irq, &moxart_timer_irq);
> +	if (ret)
> +		pr_warn("%s: failed to setup IRQ %d\n", node->full_name, irq);
> +
> +
> +	writel(APB_CLK / HZ, TIMER_1_COUNT(timer_base));
> +	writel(APB_CLK / HZ, TIMER_1_LOAD(timer_base));
> +
> +	writel(1, TIMER_1_CR_ENABLE(timer_base));
> +	writel(0, TIMER_1_CR_EXTCLK(timer_base));
> +	writel(1, TIMER_1_CR_FLOWIN(timer_base));
> +
> +	pr_info("%s: count/load (APB_CLK=%d/HZ=%d) IRQ=%d\n",
> +		node->full_name, APB_CLK, HZ, irq);
> +}
> +CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);

Any reason to not use the clocksource and clockevents infrastructures?

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-18 15:14   ` Thomas Petazzoni
  0 siblings, 0 replies; 62+ messages in thread
From: Thomas Petazzoni @ 2013-06-18 15:14 UTC (permalink / raw)
  To: linux-arm-kernel

Dear Jonas Jensen,

On Tue, 18 Jun 2013 12:00:04 +0200, Jonas Jensen wrote:

> +static void __init moxart_timer_init(struct device_node *node)
> +{
> +	int ret, irq;
> +
> +	timer_base = of_iomap(node, 0);
> +	if (!timer_base)
> +		panic("%s: failed to map base\n", node->full_name);
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0)
> +		panic("%s: can't parse IRQ\n", node->full_name);
> +
> +	ret = setup_irq(irq, &moxart_timer_irq);
> +	if (ret)
> +		pr_warn("%s: failed to setup IRQ %d\n", node->full_name, irq);
> +
> +
> +	writel(APB_CLK / HZ, TIMER_1_COUNT(timer_base));
> +	writel(APB_CLK / HZ, TIMER_1_LOAD(timer_base));
> +
> +	writel(1, TIMER_1_CR_ENABLE(timer_base));
> +	writel(0, TIMER_1_CR_EXTCLK(timer_base));
> +	writel(1, TIMER_1_CR_FLOWIN(timer_base));
> +
> +	pr_info("%s: count/load (APB_CLK=%d/HZ=%d) IRQ=%d\n",
> +		node->full_name, APB_CLK, HZ, irq);
> +}
> +CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);

Any reason to not use the clocksource and clockevents infrastructures?

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH] ARM: clocksource: add support for MOXA ART SoCs
  2013-06-18 10:00 ` Jonas Jensen
@ 2013-06-18 15:28   ` Arnd Bergmann
  -1 siblings, 0 replies; 62+ messages in thread
From: Arnd Bergmann @ 2013-06-18 15:28 UTC (permalink / raw)
  To: Jonas Jensen, Thomas Petazzoni
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz, tglx

On Tuesday 18 June 2013, Jonas Jensen wrote:
> This patch adds an clocksource driver for the main timer found
> on MOXA ART SoCs.
> 
> Applies to 3.10-rc1 and arm-soc/for-next (2013-06-15)
> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>

I didn't look closely before but I agree with Thomas Petazzoni, this should
be converted to clocksource/clockevent.

A few other things I noticed now:

> +#define APB_CLK 48000000

You are hardcoding the clock rate above, which makes it less portable
than it should be. Ideally you would use clk_get_rate() on
the default clock, but you don't actually implement a clk
driver for your platform and probably don't need one.

I don't know what others think about this, but I'd suggest just
using a "clock-frequency" property in the device node to read
the clock rate.

> +#define TIMER_1_COUNT(base_addr)        (base_addr + 0x00)
> +#define TIMER_1_LOAD(base_addr)         (base_addr + 0x04)
> +#define TIMER_1_MATCH1(base_addr)       (base_addr + 0x08)
> +#define TIMER_1_MATCH2(base_addr)       (base_addr + 0x0C)
> +
> +#define TIMER_2_COUNT(base_addr)        (base_addr + 0x10)
> +#define TIMER_2_LOAD(base_addr)         (base_addr + 0x14)
> +#define TIMER_2_MATCH1(base_addr)       (base_addr + 0x18)
> +#define TIMER_2_MATCH2(base_addr)       (base_addr + 0x1C)
> +
> +#define TIMER_3_COUNT(base_addr)        (base_addr + 0x20)
> +#define TIMER_3_LOAD(base_addr)         (base_addr + 0x24)
> +#define TIMER_3_MATCH1(base_addr)       (base_addr + 0x28)
> +#define TIMER_3_MATCH2(base_addr)       (base_addr + 0x2C)
> +

You actually seem to have three independent timers here, which
means you can use one as the clock source and one for clock
events.

> +#define TIMER1_COUNT                0x0
> +#define TIMER1_LOAD                 0x4
> +#define TIMER1_MATCH1               0x8
> +#define TIMER1_MATCH2               0xC
> +#define TIMER2_COUNT                0x10
> +#define TIMER2_LOAD                 0x14
> +#define TIMER2_MATCH1               0x18
> +#define TIMER2_MATCH2               0x1C
> +#define TIMER3_COUNT                0x20
> +#define TIMER3_LOAD                 0x24
> +#define TIMER3_MATCH1               0x28
> +#define TIMER3_MATCH2               0x2C
> +#define TIMER_INTR_MASK     0x38

These look like duplicates from above. I'd prefer the second syntax, just
do the addition of base_addr where you need it.

	Arnd

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-18 15:28   ` Arnd Bergmann
  0 siblings, 0 replies; 62+ messages in thread
From: Arnd Bergmann @ 2013-06-18 15:28 UTC (permalink / raw)
  To: linux-arm-kernel

On Tuesday 18 June 2013, Jonas Jensen wrote:
> This patch adds an clocksource driver for the main timer found
> on MOXA ART SoCs.
> 
> Applies to 3.10-rc1 and arm-soc/for-next (2013-06-15)
> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>

I didn't look closely before but I agree with Thomas Petazzoni, this should
be converted to clocksource/clockevent.

A few other things I noticed now:

> +#define APB_CLK 48000000

You are hardcoding the clock rate above, which makes it less portable
than it should be. Ideally you would use clk_get_rate() on
the default clock, but you don't actually implement a clk
driver for your platform and probably don't need one.

I don't know what others think about this, but I'd suggest just
using a "clock-frequency" property in the device node to read
the clock rate.

> +#define TIMER_1_COUNT(base_addr)        (base_addr + 0x00)
> +#define TIMER_1_LOAD(base_addr)         (base_addr + 0x04)
> +#define TIMER_1_MATCH1(base_addr)       (base_addr + 0x08)
> +#define TIMER_1_MATCH2(base_addr)       (base_addr + 0x0C)
> +
> +#define TIMER_2_COUNT(base_addr)        (base_addr + 0x10)
> +#define TIMER_2_LOAD(base_addr)         (base_addr + 0x14)
> +#define TIMER_2_MATCH1(base_addr)       (base_addr + 0x18)
> +#define TIMER_2_MATCH2(base_addr)       (base_addr + 0x1C)
> +
> +#define TIMER_3_COUNT(base_addr)        (base_addr + 0x20)
> +#define TIMER_3_LOAD(base_addr)         (base_addr + 0x24)
> +#define TIMER_3_MATCH1(base_addr)       (base_addr + 0x28)
> +#define TIMER_3_MATCH2(base_addr)       (base_addr + 0x2C)
> +

You actually seem to have three independent timers here, which
means you can use one as the clock source and one for clock
events.

> +#define TIMER1_COUNT                0x0
> +#define TIMER1_LOAD                 0x4
> +#define TIMER1_MATCH1               0x8
> +#define TIMER1_MATCH2               0xC
> +#define TIMER2_COUNT                0x10
> +#define TIMER2_LOAD                 0x14
> +#define TIMER2_MATCH1               0x18
> +#define TIMER2_MATCH2               0x1C
> +#define TIMER3_COUNT                0x20
> +#define TIMER3_LOAD                 0x24
> +#define TIMER3_MATCH1               0x28
> +#define TIMER3_MATCH2               0x2C
> +#define TIMER_INTR_MASK     0x38

These look like duplicates from above. I'd prefer the second syntax, just
do the addition of base_addr where you need it.

	Arnd

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v2] ARM: clocksource: add support for MOXA ART SoCs
  2013-06-18 10:00 ` Jonas Jensen
@ 2013-06-26 14:53   ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-06-26 14:53 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd, Jonas Jensen

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Changes since v2:

1. use clocksource/clockevents infrastructures
2. clock frequency read from system clock

Applies to next-20130619

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---
 drivers/clocksource/Makefile       |    1 +
 drivers/clocksource/moxart_timer.c |  184 ++++++++++++++++++++++++++++++++++++
 2 files changed, 185 insertions(+), 0 deletions(-)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..c93e1a8 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..376df31
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,184 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define MAX_TIMER			2
+#define USED_TIMER			1
+#define APB_CLK				48000000
+
+/* note: timer count is writable */
+
+#define TIMER1_COUNT        0x0
+#define TIMER1_LOAD         0x4
+#define TIMER1_MATCH1       0x8
+#define TIMER1_MATCH2       0xC
+
+#define TIMER2_COUNT        0x10
+#define TIMER2_LOAD         0x14
+#define TIMER2_MATCH1       0x18
+#define TIMER2_MATCH2       0x1C
+
+#define TIMER3_COUNT        0x20
+#define TIMER3_LOAD         0x24
+#define TIMER3_MATCH1       0x28
+#define TIMER3_MATCH2       0x2C
+
+#define TIMER_CR			0x30
+#define TIMER_INTR_STATE    0x34
+#define TIMER_INTR_MASK     0x38
+
+/* TIMER_CR flags:
+   TIMEREG_CR_1_CLOCK  0: PCLK, 1: EXT1CLK
+   TIMEREG_CR_1_INT    over flow interrupt enable bit */
+
+#define TIMEREG_CR_1_ENABLE         (1 << 0)
+#define TIMEREG_CR_1_CLOCK          (1 << 1)
+#define TIMEREG_CR_1_INT            (1 << 2)
+#define TIMEREG_CR_2_ENABLE         (1 << 3)
+#define TIMEREG_CR_2_CLOCK          (1 << 4)
+#define TIMEREG_CR_2_INT            (1 << 5)
+#define TIMEREG_CR_3_ENABLE         (1 << 6)
+#define TIMEREG_CR_3_CLOCK			(1 << 7)
+#define TIMEREG_CR_3_INT			(1 << 8)
+#define TIMEREG_CR_COUNT_UP			(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN		(0 << 9)
+
+static void __iomem *timer_base;
+static unsigned int clock_frequency;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+	struct clock_event_device *clk)
+{
+	u32 u = readl(timer_base + TIMER_CR);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		u |= TIMEREG_CR_1_ENABLE;
+		writel(u, timer_base + TIMER_CR);
+		writel(~0, timer_base + TIMER1_LOAD);
+		pr_debug("%s: CLOCK_EVT_MODE_ONESHOT\n", __func__);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		u |= TIMEREG_CR_1_ENABLE;
+		writel(u, timer_base + TIMER_CR);
+		writel(clock_frequency / HZ, timer_base + TIMER1_LOAD);
+		pr_debug("%s: CLOCK_EVT_MODE_PERIODIC\n", __func__);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, timer_base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+	struct clock_event_device *unused)
+{
+	u32 alarm = readl(timer_base + TIMER1_COUNT) - cycles;
+	writel(alarm, timer_base + TIMER1_MATCH1);
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name = "moxart_timer",
+	.rating = 300,
+	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode = moxart_clkevt_mode,
+	.set_next_event = moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name = "moxart-timer",
+	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
+	.handler = moxart_timer_interrupt,
+	.dev_id = &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	struct clk *clk;
+
+	timer_base = of_iomap(node, 0);
+	if (!timer_base)
+		panic("%s: failed to map base\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: can't parse IRQ\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret) {
+		pr_err("%s: failed to setup IRQ %d\n", node->full_name, irq);
+		return;
+	}
+
+	clock_frequency = APB_CLK;
+	/* it might be a good idea to have a default other than 0 for
+	   clock_frequency, should any attempt at getting a valid
+	   frequency fail, not that i see how it could, it probably could..
+	   having it APB_CLK can certainly be wrong on some hardware,
+	   why it is set again with the DT specific property: */
+
+	ret = of_property_read_u32(node, "clock-frequency", &clock_frequency);
+	if (ret)
+		pr_err("%s: can't read clock-frequency DT property\n",
+			node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		pr_err("%s: of_clk_get failed\n", __func__);
+	else {
+		clock_frequency = clk_get_rate(clk);
+		pr_debug("%s: clk_get_rate=%u success\n", __func__,
+			clock_frequency);
+	}
+
+	writel(~0, timer_base + TIMER2_LOAD);
+
+	writel(TIMEREG_CR_1_ENABLE | TIMEREG_CR_2_ENABLE |
+		TIMEREG_CR_COUNT_DOWN, timer_base + TIMER_CR);
+
+	if (clocksource_mmio_init(timer_base + TIMER2_COUNT, "moxart_timer",
+		clock_frequency, 200, 32, clocksource_mmio_readl_down))
+		pr_err("%s: clocksource_mmio_init failed\n", __func__);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	clockevents_config_and_register(&moxart_clockevent, clock_frequency,
+		0x4, 0xf0000000);
+
+	pr_info("%s: %s finished clock_frequency=%d HZ=%d IRQ=%d\n",
+		node->full_name, __func__, clock_frequency, HZ, irq);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
+
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v2] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-26 14:53   ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-06-26 14:53 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Changes since v2:

1. use clocksource/clockevents infrastructures
2. clock frequency read from system clock

Applies to next-20130619

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---
 drivers/clocksource/Makefile       |    1 +
 drivers/clocksource/moxart_timer.c |  184 ++++++++++++++++++++++++++++++++++++
 2 files changed, 185 insertions(+), 0 deletions(-)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..c93e1a8 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..376df31
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,184 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define MAX_TIMER			2
+#define USED_TIMER			1
+#define APB_CLK				48000000
+
+/* note: timer count is writable */
+
+#define TIMER1_COUNT        0x0
+#define TIMER1_LOAD         0x4
+#define TIMER1_MATCH1       0x8
+#define TIMER1_MATCH2       0xC
+
+#define TIMER2_COUNT        0x10
+#define TIMER2_LOAD         0x14
+#define TIMER2_MATCH1       0x18
+#define TIMER2_MATCH2       0x1C
+
+#define TIMER3_COUNT        0x20
+#define TIMER3_LOAD         0x24
+#define TIMER3_MATCH1       0x28
+#define TIMER3_MATCH2       0x2C
+
+#define TIMER_CR			0x30
+#define TIMER_INTR_STATE    0x34
+#define TIMER_INTR_MASK     0x38
+
+/* TIMER_CR flags:
+   TIMEREG_CR_1_CLOCK  0: PCLK, 1: EXT1CLK
+   TIMEREG_CR_1_INT    over flow interrupt enable bit */
+
+#define TIMEREG_CR_1_ENABLE         (1 << 0)
+#define TIMEREG_CR_1_CLOCK          (1 << 1)
+#define TIMEREG_CR_1_INT            (1 << 2)
+#define TIMEREG_CR_2_ENABLE         (1 << 3)
+#define TIMEREG_CR_2_CLOCK          (1 << 4)
+#define TIMEREG_CR_2_INT            (1 << 5)
+#define TIMEREG_CR_3_ENABLE         (1 << 6)
+#define TIMEREG_CR_3_CLOCK			(1 << 7)
+#define TIMEREG_CR_3_INT			(1 << 8)
+#define TIMEREG_CR_COUNT_UP			(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN		(0 << 9)
+
+static void __iomem *timer_base;
+static unsigned int clock_frequency;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+	struct clock_event_device *clk)
+{
+	u32 u = readl(timer_base + TIMER_CR);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		u |= TIMEREG_CR_1_ENABLE;
+		writel(u, timer_base + TIMER_CR);
+		writel(~0, timer_base + TIMER1_LOAD);
+		pr_debug("%s: CLOCK_EVT_MODE_ONESHOT\n", __func__);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		u |= TIMEREG_CR_1_ENABLE;
+		writel(u, timer_base + TIMER_CR);
+		writel(clock_frequency / HZ, timer_base + TIMER1_LOAD);
+		pr_debug("%s: CLOCK_EVT_MODE_PERIODIC\n", __func__);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, timer_base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+	struct clock_event_device *unused)
+{
+	u32 alarm = readl(timer_base + TIMER1_COUNT) - cycles;
+	writel(alarm, timer_base + TIMER1_MATCH1);
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name = "moxart_timer",
+	.rating = 300,
+	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode = moxart_clkevt_mode,
+	.set_next_event = moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name = "moxart-timer",
+	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
+	.handler = moxart_timer_interrupt,
+	.dev_id = &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	struct clk *clk;
+
+	timer_base = of_iomap(node, 0);
+	if (!timer_base)
+		panic("%s: failed to map base\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: can't parse IRQ\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret) {
+		pr_err("%s: failed to setup IRQ %d\n", node->full_name, irq);
+		return;
+	}
+
+	clock_frequency = APB_CLK;
+	/* it might be a good idea to have a default other than 0 for
+	   clock_frequency, should any attempt at getting a valid
+	   frequency fail, not that i see how it could, it probably could..
+	   having it APB_CLK can certainly be wrong on some hardware,
+	   why it is set again with the DT specific property: */
+
+	ret = of_property_read_u32(node, "clock-frequency", &clock_frequency);
+	if (ret)
+		pr_err("%s: can't read clock-frequency DT property\n",
+			node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		pr_err("%s: of_clk_get failed\n", __func__);
+	else {
+		clock_frequency = clk_get_rate(clk);
+		pr_debug("%s: clk_get_rate=%u success\n", __func__,
+			clock_frequency);
+	}
+
+	writel(~0, timer_base + TIMER2_LOAD);
+
+	writel(TIMEREG_CR_1_ENABLE | TIMEREG_CR_2_ENABLE |
+		TIMEREG_CR_COUNT_DOWN, timer_base + TIMER_CR);
+
+	if (clocksource_mmio_init(timer_base + TIMER2_COUNT, "moxart_timer",
+		clock_frequency, 200, 32, clocksource_mmio_readl_down))
+		pr_err("%s: clocksource_mmio_init failed\n", __func__);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	clockevents_config_and_register(&moxart_clockevent, clock_frequency,
+		0x4, 0xf0000000);
+
+	pr_info("%s: %s finished clock_frequency=%d HZ=%d IRQ=%d\n",
+		node->full_name, __func__, clock_frequency, HZ, irq);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
+
-- 
1.7.2.5

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ARM: clocksource: add support for MOXA ART SoCs
  2013-06-26 14:53   ` Jonas Jensen
@ 2013-06-26 14:59     ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-06-26 14:59 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd, Jonas Jensen

Hi,

I have attempted to rectify with changes from all the feedback I have received.

On 26 June 2013 16:53, Jonas Jensen <jonas.jensen@gmail.com> wrote:
> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
>
> Changes since v2:

Unfortunately the commit message is incorrect, these are changes since v1.

Best regards,
Jonas

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v2] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-26 14:59     ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-06-26 14:59 UTC (permalink / raw)
  To: linux-arm-kernel

Hi,

I have attempted to rectify with changes from all the feedback I have received.

On 26 June 2013 16:53, Jonas Jensen <jonas.jensen@gmail.com> wrote:
> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
>
> Changes since v2:

Unfortunately the commit message is incorrect, these are changes since v1.

Best regards,
Jonas

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ARM: clocksource: add support for MOXA ART SoCs
  2013-06-26 14:53   ` Jonas Jensen
@ 2013-06-26 16:10     ` Uwe Kleine-König
  -1 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-06-26 16:10 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz, tglx,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd

Hello,

On Wed, Jun 26, 2013 at 04:53:03PM +0200, Jonas Jensen wrote:
> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
> 
> Changes since v2:
> 
> 1. use clocksource/clockevents infrastructures
> 2. clock frequency read from system clock
> 
> Applies to next-20130619
Put the changes since vX after the tripple dash please. This way they
don't make it into the git history.

> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> ---
>  drivers/clocksource/Makefile       |    1 +
>  drivers/clocksource/moxart_timer.c |  184 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 185 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/clocksource/moxart_timer.c
> 
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 8d979c7..c93e1a8 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
>  
>  obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
>  obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
> +obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
> diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
> new file mode 100644
> index 0000000..376df31
> --- /dev/null
> +++ b/drivers/clocksource/moxart_timer.c
> @@ -0,0 +1,184 @@
> +/*
> + * MOXA ART SoCs timer handling.
> + *
> + * Copyright (C) 2013 Jonas Jensen
> + *
> + * Jonas Jensen <jonas.jensen@gmail.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqreturn.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +#include <linux/clocksource.h>
> +
> +#define MAX_TIMER			2
> +#define USED_TIMER			1
This define is unused. ..ooOO(UNUSED_TIMER)
> +#define APB_CLK				48000000
> +
> +/* note: timer count is writable */
> +
> +#define TIMER1_COUNT        0x0
> +#define TIMER1_LOAD         0x4
> +#define TIMER1_MATCH1       0x8
> +#define TIMER1_MATCH2       0xC
> +
> +#define TIMER2_COUNT        0x10
> +#define TIMER2_LOAD         0x14
> +#define TIMER2_MATCH1       0x18
> +#define TIMER2_MATCH2       0x1C
> +
> +#define TIMER3_COUNT        0x20
> +#define TIMER3_LOAD         0x24
> +#define TIMER3_MATCH1       0x28
> +#define TIMER3_MATCH2       0x2C
Maybe make this:

	TIMER1_BASE	0x00
	TIMER2_BASE	0x10
	TIMER3_BASE	0x20

	REG_COUNT	0x0
	REG_LOAD	0x4
	REG_MATCH1	0x8
	REG_MATCH2	0xc

?

> +
> +#define TIMER_CR			0x30
> +#define TIMER_INTR_STATE    0x34
> +#define TIMER_INTR_MASK     0x38
All lines above starting with TIMER1_COUNT use spaces to indent, only
TIMER_CR uses tabs.

> +
> +/* TIMER_CR flags:
> +   TIMEREG_CR_1_CLOCK  0: PCLK, 1: EXT1CLK
> +   TIMEREG_CR_1_INT    over flow interrupt enable bit */
> +
> +#define TIMEREG_CR_1_ENABLE         (1 << 0)
> +#define TIMEREG_CR_1_CLOCK          (1 << 1)
> +#define TIMEREG_CR_1_INT            (1 << 2)
> +#define TIMEREG_CR_2_ENABLE         (1 << 3)
> +#define TIMEREG_CR_2_CLOCK          (1 << 4)
> +#define TIMEREG_CR_2_INT            (1 << 5)
> +#define TIMEREG_CR_3_ENABLE         (1 << 6)
> +#define TIMEREG_CR_3_CLOCK			(1 << 7)
> +#define TIMEREG_CR_3_INT			(1 << 8)
> +#define TIMEREG_CR_COUNT_UP			(1 << 9)
> +#define TIMEREG_CR_COUNT_DOWN		(0 << 9)
Same problem here.

> +
> +static void __iomem *timer_base;
> +static unsigned int clock_frequency;
> +
> +static void moxart_clkevt_mode(enum clock_event_mode mode,
> +	struct clock_event_device *clk)
> +{
> +	u32 u = readl(timer_base + TIMER_CR);
> +
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_RESUME:
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		u |= TIMEREG_CR_1_ENABLE;
> +		writel(u, timer_base + TIMER_CR);
> +		writel(~0, timer_base + TIMER1_LOAD);
> +		pr_debug("%s: CLOCK_EVT_MODE_ONESHOT\n", __func__);
This does already start the timer, right? I think the intention is that
after set_mode(ONESHOT) the timer only starts running after a call to
next_event.

> +		break;
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		u |= TIMEREG_CR_1_ENABLE;
> +		writel(u, timer_base + TIMER_CR);
> +		writel(clock_frequency / HZ, timer_base + TIMER1_LOAD);
better precalculate this value to save the division here.

> +		pr_debug("%s: CLOCK_EVT_MODE_PERIODIC\n", __func__);
> +		break;
> +	case CLOCK_EVT_MODE_UNUSED:
> +	case CLOCK_EVT_MODE_SHUTDOWN:
> +	default:
> +		u &= ~TIMEREG_CR_1_ENABLE;
> +		writel(u, timer_base + TIMER_CR);
> +		break;
> +	}
> +}
> +
> +static int moxart_clkevt_next_event(unsigned long cycles,
> +	struct clock_event_device *unused)
> +{
> +	u32 alarm = readl(timer_base + TIMER1_COUNT) - cycles;
> +	writel(alarm, timer_base + TIMER1_MATCH1);
> +	return 0;
> +}
> +
> +static struct clock_event_device moxart_clockevent = {
> +	.name = "moxart_timer",
> +	.rating = 300,
> +	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> +	.set_mode = moxart_clkevt_mode,
> +	.set_next_event = moxart_clkevt_next_event,
> +};
> +
> +static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
This cast is not necessary.

> +	evt->event_handler(evt);
> +	return IRQ_HANDLED;
> +}
> +
> +static struct irqaction moxart_timer_irq = {
> +	.name = "moxart-timer",
> +	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> +	.handler = moxart_timer_interrupt,
> +	.dev_id = &moxart_clockevent,
> +};
> +
> +static void __init moxart_timer_init(struct device_node *node)
> +{
> +	int ret, irq;
> +	struct clk *clk;
> +
> +	timer_base = of_iomap(node, 0);
> +	if (!timer_base)
> +		panic("%s: failed to map base\n", node->full_name);
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0)
> +		panic("%s: can't parse IRQ\n", node->full_name);
> +
> +	ret = setup_irq(irq, &moxart_timer_irq);
> +	if (ret) {
> +		pr_err("%s: failed to setup IRQ %d\n", node->full_name, irq);
> +		return;
> +	}
> +
> +	clock_frequency = APB_CLK;
> +	/* it might be a good idea to have a default other than 0 for
> +	   clock_frequency, should any attempt at getting a valid
> +	   frequency fail, not that i see how it could, it probably could..
> +	   having it APB_CLK can certainly be wrong on some hardware,
> +	   why it is set again with the DT specific property: */
Multi-line comments look as follows in the Kernel:

	/*
	 * ...
	 * ...
	 */

IIUC the comment describes the assignment to clock_frequency. In this
case the comment has to be above the assignment.
> +
> +	ret = of_property_read_u32(node, "clock-frequency", &clock_frequency);
> +	if (ret)
> +		pr_err("%s: can't read clock-frequency DT property\n",
> +			node->full_name);
> +
> +	clk = of_clk_get(node, 0);
> +	if (IS_ERR(clk))
Maybe move the default assignment of clock_frequency to here?

> +		pr_err("%s: of_clk_get failed\n", __func__);
> +	else {
> +		clock_frequency = clk_get_rate(clk);
> +		pr_debug("%s: clk_get_rate=%u success\n", __func__,
> +			clock_frequency);
> +	}
> +
> +	writel(~0, timer_base + TIMER2_LOAD);
> +
> +	writel(TIMEREG_CR_1_ENABLE | TIMEREG_CR_2_ENABLE |
> +		TIMEREG_CR_COUNT_DOWN, timer_base + TIMER_CR);
> +
> +	if (clocksource_mmio_init(timer_base + TIMER2_COUNT, "moxart_timer",
> +		clock_frequency, 200, 32, clocksource_mmio_readl_down))
> +		pr_err("%s: clocksource_mmio_init failed\n", __func__);
> +
> +	moxart_clockevent.cpumask = cpumask_of(0);
> +
> +	clockevents_config_and_register(&moxart_clockevent, clock_frequency,
> +		0x4, 0xf0000000);
tglx recently told me he prefers to align continuation lines to the
matching opening brace.

Best regards
Uwe

> +
> +	pr_info("%s: %s finished clock_frequency=%d HZ=%d IRQ=%d\n",
> +		node->full_name, __func__, clock_frequency, HZ, irq);
> +}
> +CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
> +
> -- 
> 1.7.2.5
> 
> 

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

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v2] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-26 16:10     ` Uwe Kleine-König
  0 siblings, 0 replies; 62+ messages in thread
From: Uwe Kleine-König @ 2013-06-26 16:10 UTC (permalink / raw)
  To: linux-arm-kernel

Hello,

On Wed, Jun 26, 2013 at 04:53:03PM +0200, Jonas Jensen wrote:
> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
> 
> Changes since v2:
> 
> 1. use clocksource/clockevents infrastructures
> 2. clock frequency read from system clock
> 
> Applies to next-20130619
Put the changes since vX after the tripple dash please. This way they
don't make it into the git history.

> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> ---
>  drivers/clocksource/Makefile       |    1 +
>  drivers/clocksource/moxart_timer.c |  184 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 185 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/clocksource/moxart_timer.c
> 
> diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
> index 8d979c7..c93e1a8 100644
> --- a/drivers/clocksource/Makefile
> +++ b/drivers/clocksource/Makefile
> @@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
>  
>  obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
>  obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
> +obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
> diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
> new file mode 100644
> index 0000000..376df31
> --- /dev/null
> +++ b/drivers/clocksource/moxart_timer.c
> @@ -0,0 +1,184 @@
> +/*
> + * MOXA ART SoCs timer handling.
> + *
> + * Copyright (C) 2013 Jonas Jensen
> + *
> + * Jonas Jensen <jonas.jensen@gmail.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2.  This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqreturn.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +#include <linux/clocksource.h>
> +
> +#define MAX_TIMER			2
> +#define USED_TIMER			1
This define is unused. ..ooOO(UNUSED_TIMER)
> +#define APB_CLK				48000000
> +
> +/* note: timer count is writable */
> +
> +#define TIMER1_COUNT        0x0
> +#define TIMER1_LOAD         0x4
> +#define TIMER1_MATCH1       0x8
> +#define TIMER1_MATCH2       0xC
> +
> +#define TIMER2_COUNT        0x10
> +#define TIMER2_LOAD         0x14
> +#define TIMER2_MATCH1       0x18
> +#define TIMER2_MATCH2       0x1C
> +
> +#define TIMER3_COUNT        0x20
> +#define TIMER3_LOAD         0x24
> +#define TIMER3_MATCH1       0x28
> +#define TIMER3_MATCH2       0x2C
Maybe make this:

	TIMER1_BASE	0x00
	TIMER2_BASE	0x10
	TIMER3_BASE	0x20

	REG_COUNT	0x0
	REG_LOAD	0x4
	REG_MATCH1	0x8
	REG_MATCH2	0xc

?

> +
> +#define TIMER_CR			0x30
> +#define TIMER_INTR_STATE    0x34
> +#define TIMER_INTR_MASK     0x38
All lines above starting with TIMER1_COUNT use spaces to indent, only
TIMER_CR uses tabs.

> +
> +/* TIMER_CR flags:
> +   TIMEREG_CR_1_CLOCK  0: PCLK, 1: EXT1CLK
> +   TIMEREG_CR_1_INT    over flow interrupt enable bit */
> +
> +#define TIMEREG_CR_1_ENABLE         (1 << 0)
> +#define TIMEREG_CR_1_CLOCK          (1 << 1)
> +#define TIMEREG_CR_1_INT            (1 << 2)
> +#define TIMEREG_CR_2_ENABLE         (1 << 3)
> +#define TIMEREG_CR_2_CLOCK          (1 << 4)
> +#define TIMEREG_CR_2_INT            (1 << 5)
> +#define TIMEREG_CR_3_ENABLE         (1 << 6)
> +#define TIMEREG_CR_3_CLOCK			(1 << 7)
> +#define TIMEREG_CR_3_INT			(1 << 8)
> +#define TIMEREG_CR_COUNT_UP			(1 << 9)
> +#define TIMEREG_CR_COUNT_DOWN		(0 << 9)
Same problem here.

> +
> +static void __iomem *timer_base;
> +static unsigned int clock_frequency;
> +
> +static void moxart_clkevt_mode(enum clock_event_mode mode,
> +	struct clock_event_device *clk)
> +{
> +	u32 u = readl(timer_base + TIMER_CR);
> +
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_RESUME:
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		u |= TIMEREG_CR_1_ENABLE;
> +		writel(u, timer_base + TIMER_CR);
> +		writel(~0, timer_base + TIMER1_LOAD);
> +		pr_debug("%s: CLOCK_EVT_MODE_ONESHOT\n", __func__);
This does already start the timer, right? I think the intention is that
after set_mode(ONESHOT) the timer only starts running after a call to
next_event.

> +		break;
> +	case CLOCK_EVT_MODE_PERIODIC:
> +		u |= TIMEREG_CR_1_ENABLE;
> +		writel(u, timer_base + TIMER_CR);
> +		writel(clock_frequency / HZ, timer_base + TIMER1_LOAD);
better precalculate this value to save the division here.

> +		pr_debug("%s: CLOCK_EVT_MODE_PERIODIC\n", __func__);
> +		break;
> +	case CLOCK_EVT_MODE_UNUSED:
> +	case CLOCK_EVT_MODE_SHUTDOWN:
> +	default:
> +		u &= ~TIMEREG_CR_1_ENABLE;
> +		writel(u, timer_base + TIMER_CR);
> +		break;
> +	}
> +}
> +
> +static int moxart_clkevt_next_event(unsigned long cycles,
> +	struct clock_event_device *unused)
> +{
> +	u32 alarm = readl(timer_base + TIMER1_COUNT) - cycles;
> +	writel(alarm, timer_base + TIMER1_MATCH1);
> +	return 0;
> +}
> +
> +static struct clock_event_device moxart_clockevent = {
> +	.name = "moxart_timer",
> +	.rating = 300,
> +	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
> +	.set_mode = moxart_clkevt_mode,
> +	.set_next_event = moxart_clkevt_next_event,
> +};
> +
> +static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
> +{
> +	struct clock_event_device *evt = (struct clock_event_device *)dev_id;
This cast is not necessary.

> +	evt->event_handler(evt);
> +	return IRQ_HANDLED;
> +}
> +
> +static struct irqaction moxart_timer_irq = {
> +	.name = "moxart-timer",
> +	.flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,
> +	.handler = moxart_timer_interrupt,
> +	.dev_id = &moxart_clockevent,
> +};
> +
> +static void __init moxart_timer_init(struct device_node *node)
> +{
> +	int ret, irq;
> +	struct clk *clk;
> +
> +	timer_base = of_iomap(node, 0);
> +	if (!timer_base)
> +		panic("%s: failed to map base\n", node->full_name);
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0)
> +		panic("%s: can't parse IRQ\n", node->full_name);
> +
> +	ret = setup_irq(irq, &moxart_timer_irq);
> +	if (ret) {
> +		pr_err("%s: failed to setup IRQ %d\n", node->full_name, irq);
> +		return;
> +	}
> +
> +	clock_frequency = APB_CLK;
> +	/* it might be a good idea to have a default other than 0 for
> +	   clock_frequency, should any attempt at getting a valid
> +	   frequency fail, not that i see how it could, it probably could..
> +	   having it APB_CLK can certainly be wrong on some hardware,
> +	   why it is set again with the DT specific property: */
Multi-line comments look as follows in the Kernel:

	/*
	 * ...
	 * ...
	 */

IIUC the comment describes the assignment to clock_frequency. In this
case the comment has to be above the assignment.
> +
> +	ret = of_property_read_u32(node, "clock-frequency", &clock_frequency);
> +	if (ret)
> +		pr_err("%s: can't read clock-frequency DT property\n",
> +			node->full_name);
> +
> +	clk = of_clk_get(node, 0);
> +	if (IS_ERR(clk))
Maybe move the default assignment of clock_frequency to here?

> +		pr_err("%s: of_clk_get failed\n", __func__);
> +	else {
> +		clock_frequency = clk_get_rate(clk);
> +		pr_debug("%s: clk_get_rate=%u success\n", __func__,
> +			clock_frequency);
> +	}
> +
> +	writel(~0, timer_base + TIMER2_LOAD);
> +
> +	writel(TIMEREG_CR_1_ENABLE | TIMEREG_CR_2_ENABLE |
> +		TIMEREG_CR_COUNT_DOWN, timer_base + TIMER_CR);
> +
> +	if (clocksource_mmio_init(timer_base + TIMER2_COUNT, "moxart_timer",
> +		clock_frequency, 200, 32, clocksource_mmio_readl_down))
> +		pr_err("%s: clocksource_mmio_init failed\n", __func__);
> +
> +	moxart_clockevent.cpumask = cpumask_of(0);
> +
> +	clockevents_config_and_register(&moxart_clockevent, clock_frequency,
> +		0x4, 0xf0000000);
tglx recently told me he prefers to align continuation lines to the
matching opening brace.

Best regards
Uwe

> +
> +	pr_info("%s: %s finished clock_frequency=%d HZ=%d IRQ=%d\n",
> +		node->full_name, __func__, clock_frequency, HZ, irq);
> +}
> +CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
> +
> -- 
> 1.7.2.5
> 
> 

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

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v2] ARM: clocksource: add support for MOXA ART SoCs
  2013-06-26 14:53   ` Jonas Jensen
@ 2013-06-26 19:15     ` Linus Walleij
  -1 siblings, 0 replies; 62+ messages in thread
From: Linus Walleij @ 2013-06-26 19:15 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, John Stultz,
	Thomas Gleixner, Uwe Kleine-König, Tomasz Figa,
	Thomas Petazzoni, Arnd Bergmann

On Wed, Jun 26, 2013 at 4:53 PM, Jonas Jensen <jonas.jensen@gmail.com> wrote:

> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
>
> Changes since v2:
>
> 1. use clocksource/clockevents infrastructures
> 2. clock frequency read from system clock
>
> Applies to next-20130619
>
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>

This is starting to look good :-)

> +/* TIMER_CR flags:
> +   TIMEREG_CR_1_CLOCK  0: PCLK, 1: EXT1CLK
> +   TIMEREG_CR_1_INT    over flow interrupt enable bit */

/*
 * Usually we use this comment style, one star at the beginning
 * of every comment line and a closing star-slash sitting lonley
 * on a single line to close it.
 */

> +       case CLOCK_EVT_MODE_RESUME:
> +       case CLOCK_EVT_MODE_ONESHOT:
> +               u |= TIMEREG_CR_1_ENABLE;
> +               writel(u, timer_base + TIMER_CR);
> +               writel(~0, timer_base + TIMER1_LOAD);

Should you not write the load value *before* enabling the timer?

> +               pr_debug("%s: CLOCK_EVT_MODE_ONESHOT\n", __func__);
> +               break;

This looks like you're enabling a tick far ahead in the future.
For oneshot you should just trigger new events in .next_event(),
I would just disable the timer for oneshot and enable it
for each new event in .next_event() instead.

> +       case CLOCK_EVT_MODE_PERIODIC:
> +               u |= TIMEREG_CR_1_ENABLE;
> +               writel(u, timer_base + TIMER_CR);
> +               writel(clock_frequency / HZ, timer_base + TIMER1_LOAD);

Use DIV_ROUND_CLOSEST(clock_frequency, HZ)

> +               pr_debug("%s: CLOCK_EVT_MODE_PERIODIC\n", __func__);
> +               break;
> +       case CLOCK_EVT_MODE_UNUSED:
> +       case CLOCK_EVT_MODE_SHUTDOWN:
> +       default:
> +               u &= ~TIMEREG_CR_1_ENABLE;

I would do this also for Oneshot. Then enable it in
.next_event().

> +               writel(u, timer_base + TIMER_CR);
> +               break;


> +static int moxart_clkevt_next_event(unsigned long cycles,
> +       struct clock_event_device *unused)
> +{
> +       u32 alarm = readl(timer_base + TIMER1_COUNT) - cycles;
> +       writel(alarm, timer_base + TIMER1_MATCH1);

I would write TIMEREG_CR_1_ENABLE *here*. This way the
timer is only strictly triggered when an event is queued.

> +static struct irqaction moxart_timer_irq = {
> +       .name = "moxart-timer",
> +       .flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,

IRQF_DISABLED is pointless, remove it. Nowadays all IRQ handlers
are called with interrupts disabled.

Why do you need IRQF_IRQPOLL? That seems like more
copy/paste luggade.

> +       clock_frequency = APB_CLK;
> +       /* it might be a good idea to have a default other than 0 for
> +          clock_frequency, should any attempt at getting a valid
> +          frequency fail, not that i see how it could, it probably could..
> +          having it APB_CLK can certainly be wrong on some hardware,
> +          why it is set again with the DT specific property: */

Seems overkill.

> +       ret = of_property_read_u32(node, "clock-frequency", &clock_frequency);
> +       if (ret)
> +               pr_err("%s: can't read clock-frequency DT property\n",
> +                       node->full_name);

I don't think it's apropriate to put clock frequencies into the device
tree node as u32:s. Please use the clock framework exclusively.
Now it's like three ways to get this frequency... what if all three
are different :-P

> +       clk = of_clk_get(node, 0);
> +       if (IS_ERR(clk))
> +               pr_err("%s: of_clk_get failed\n", __func__);

bail out here?

> +       else {
> +               clock_frequency = clk_get_rate(clk);
> +               pr_debug("%s: clk_get_rate=%u success\n", __func__,
> +                       clock_frequency);
> +       }

Then you can  remove the else clause and de-indent this.

The rest looks OK...

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v2] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-26 19:15     ` Linus Walleij
  0 siblings, 0 replies; 62+ messages in thread
From: Linus Walleij @ 2013-06-26 19:15 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jun 26, 2013 at 4:53 PM, Jonas Jensen <jonas.jensen@gmail.com> wrote:

> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
>
> Changes since v2:
>
> 1. use clocksource/clockevents infrastructures
> 2. clock frequency read from system clock
>
> Applies to next-20130619
>
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>

This is starting to look good :-)

> +/* TIMER_CR flags:
> +   TIMEREG_CR_1_CLOCK  0: PCLK, 1: EXT1CLK
> +   TIMEREG_CR_1_INT    over flow interrupt enable bit */

/*
 * Usually we use this comment style, one star at the beginning
 * of every comment line and a closing star-slash sitting lonley
 * on a single line to close it.
 */

> +       case CLOCK_EVT_MODE_RESUME:
> +       case CLOCK_EVT_MODE_ONESHOT:
> +               u |= TIMEREG_CR_1_ENABLE;
> +               writel(u, timer_base + TIMER_CR);
> +               writel(~0, timer_base + TIMER1_LOAD);

Should you not write the load value *before* enabling the timer?

> +               pr_debug("%s: CLOCK_EVT_MODE_ONESHOT\n", __func__);
> +               break;

This looks like you're enabling a tick far ahead in the future.
For oneshot you should just trigger new events in .next_event(),
I would just disable the timer for oneshot and enable it
for each new event in .next_event() instead.

> +       case CLOCK_EVT_MODE_PERIODIC:
> +               u |= TIMEREG_CR_1_ENABLE;
> +               writel(u, timer_base + TIMER_CR);
> +               writel(clock_frequency / HZ, timer_base + TIMER1_LOAD);

Use DIV_ROUND_CLOSEST(clock_frequency, HZ)

> +               pr_debug("%s: CLOCK_EVT_MODE_PERIODIC\n", __func__);
> +               break;
> +       case CLOCK_EVT_MODE_UNUSED:
> +       case CLOCK_EVT_MODE_SHUTDOWN:
> +       default:
> +               u &= ~TIMEREG_CR_1_ENABLE;

I would do this also for Oneshot. Then enable it in
.next_event().

> +               writel(u, timer_base + TIMER_CR);
> +               break;


> +static int moxart_clkevt_next_event(unsigned long cycles,
> +       struct clock_event_device *unused)
> +{
> +       u32 alarm = readl(timer_base + TIMER1_COUNT) - cycles;
> +       writel(alarm, timer_base + TIMER1_MATCH1);

I would write TIMEREG_CR_1_ENABLE *here*. This way the
timer is only strictly triggered when an event is queued.

> +static struct irqaction moxart_timer_irq = {
> +       .name = "moxart-timer",
> +       .flags = IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL,

IRQF_DISABLED is pointless, remove it. Nowadays all IRQ handlers
are called with interrupts disabled.

Why do you need IRQF_IRQPOLL? That seems like more
copy/paste luggade.

> +       clock_frequency = APB_CLK;
> +       /* it might be a good idea to have a default other than 0 for
> +          clock_frequency, should any attempt at getting a valid
> +          frequency fail, not that i see how it could, it probably could..
> +          having it APB_CLK can certainly be wrong on some hardware,
> +          why it is set again with the DT specific property: */

Seems overkill.

> +       ret = of_property_read_u32(node, "clock-frequency", &clock_frequency);
> +       if (ret)
> +               pr_err("%s: can't read clock-frequency DT property\n",
> +                       node->full_name);

I don't think it's apropriate to put clock frequencies into the device
tree node as u32:s. Please use the clock framework exclusively.
Now it's like three ways to get this frequency... what if all three
are different :-P

> +       clk = of_clk_get(node, 0);
> +       if (IS_ERR(clk))
> +               pr_err("%s: of_clk_get failed\n", __func__);

bail out here?

> +       else {
> +               clock_frequency = clk_get_rate(clk);
> +               pr_debug("%s: clk_get_rate=%u success\n", __func__,
> +                       clock_frequency);
> +       }

Then you can  remove the else clause and de-indent this.

The rest looks OK...

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v3] ARM: clocksource: add support for MOXA ART SoCs
  2013-06-26 14:53   ` Jonas Jensen
@ 2013-06-27 11:23     ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-06-27 11:23 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd, Jonas Jensen

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Applies to next-20130619
    
    Changes since v2:
    
    1. remove unused defines: MAX_TIMER, USED_TIMER, APB_CLK
    2. split register defines so each timer has a base
    3. use standard multiline comments
    4. replace whitespaces with tabs
    5. rename "timer_base" "base"
    6. rename "clock_frequency" "clock_count_per_tick"
    7. disable TIMER1 in CLOCK_EVT_MODE_ONESHOT
    8. remove pr_debug:s
    9. in CLOCK_EVT_MODE_PERIODIC, switch order, write clock_count_per_tick
       to TIMER1 LOAD and enable it
    10. enable TIMER1 in moxart_clkevt_next_event
    11. remove IRQF_DISABLED, IRQF_IRQPOLL from irqaction flags
    12. set clock_frequency exclusively from system clock
    13. bail if of_clk_get fails
    14. assign clock_count_per_tick to DIV_ROUND_CLOSEST(pclk, HZ)
    15. align continuation lines with matching opening brace

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 166 +++++++++++++++++++++++++++++++++++++
 2 files changed, 167 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..c93e1a8 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..4678b30
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,166 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE			0x00
+#define TIMER2_BASE			0x10
+#define TIMER3_BASE			0x20
+
+#define REG_COUNT			0x0 /* writable */
+#define REG_LOAD			0x4
+#define REG_MATCH1			0x8
+#define REG_MATCH2			0xC
+
+#define TIMER_CR			0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT		overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE		(1 << 0)
+#define TIMEREG_CR_1_CLOCK		(1 << 1)
+#define TIMEREG_CR_1_INT		(1 << 2)
+#define TIMEREG_CR_2_ENABLE		(1 << 3)
+#define TIMEREG_CR_2_CLOCK		(1 << 4)
+#define TIMEREG_CR_2_INT		(1 << 5)
+#define TIMEREG_CR_3_ENABLE		(1 << 6)
+#define TIMEREG_CR_3_CLOCK		(1 << 7)
+#define TIMEREG_CR_3_INT		(1 << 8)
+#define TIMEREG_CR_COUNT_UP		(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+	struct clock_event_device *clk)
+{
+	u32 u = readl(base + TIMER_CR);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		u |= TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+	struct clock_event_device *unused)
+{
+	u32 u;
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+	u = readl(base + TIMER_CR) | TIMEREG_CR_1_ENABLE;
+	writel(u, base + TIMER_CR);
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name = "moxart_timer",
+	.rating = 200,
+	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode = moxart_clkevt_mode,
+	.set_next_event = moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name = "moxart-timer",
+	.flags = IRQF_TIMER,
+	.handler = moxart_timer_interrupt,
+	.dev_id = &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: failed to map base\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: can't parse IRQ\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret) {
+		pr_err("%s: failed to setup IRQ %d\n", node->full_name, irq);
+		return;
+	}
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk)) {
+		pr_err("%s: of_clk_get failed\n", __func__);
+		return;
+	}
+
+	pclk = clk_get_rate(clk);
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+					"moxart_timer", pclk, 200, 32,
+					clocksource_mmio_readl_down)) {
+		pr_err("%s: clocksource_mmio_init failed\n", __func__);
+		return;
+	}
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xf0000000);
+
+	pr_info("%s: %s finished pclk=%lu HZ=%d IRQ=%d\n",
+			node->full_name, __func__, pclk, HZ, irq);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
+
-- 
1.8.2.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v3] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-27 11:23     ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-06-27 11:23 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Applies to next-20130619
    
    Changes since v2:
    
    1. remove unused defines: MAX_TIMER, USED_TIMER, APB_CLK
    2. split register defines so each timer has a base
    3. use standard multiline comments
    4. replace whitespaces with tabs
    5. rename "timer_base" "base"
    6. rename "clock_frequency" "clock_count_per_tick"
    7. disable TIMER1 in CLOCK_EVT_MODE_ONESHOT
    8. remove pr_debug:s
    9. in CLOCK_EVT_MODE_PERIODIC, switch order, write clock_count_per_tick
       to TIMER1 LOAD and enable it
    10. enable TIMER1 in moxart_clkevt_next_event
    11. remove IRQF_DISABLED, IRQF_IRQPOLL from irqaction flags
    12. set clock_frequency exclusively from system clock
    13. bail if of_clk_get fails
    14. assign clock_count_per_tick to DIV_ROUND_CLOSEST(pclk, HZ)
    15. align continuation lines with matching opening brace

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 166 +++++++++++++++++++++++++++++++++++++
 2 files changed, 167 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..c93e1a8 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..4678b30
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,166 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE			0x00
+#define TIMER2_BASE			0x10
+#define TIMER3_BASE			0x20
+
+#define REG_COUNT			0x0 /* writable */
+#define REG_LOAD			0x4
+#define REG_MATCH1			0x8
+#define REG_MATCH2			0xC
+
+#define TIMER_CR			0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT		overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE		(1 << 0)
+#define TIMEREG_CR_1_CLOCK		(1 << 1)
+#define TIMEREG_CR_1_INT		(1 << 2)
+#define TIMEREG_CR_2_ENABLE		(1 << 3)
+#define TIMEREG_CR_2_CLOCK		(1 << 4)
+#define TIMEREG_CR_2_INT		(1 << 5)
+#define TIMEREG_CR_3_ENABLE		(1 << 6)
+#define TIMEREG_CR_3_CLOCK		(1 << 7)
+#define TIMEREG_CR_3_INT		(1 << 8)
+#define TIMEREG_CR_COUNT_UP		(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+	struct clock_event_device *clk)
+{
+	u32 u = readl(base + TIMER_CR);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		u |= TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+	struct clock_event_device *unused)
+{
+	u32 u;
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+	u = readl(base + TIMER_CR) | TIMEREG_CR_1_ENABLE;
+	writel(u, base + TIMER_CR);
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name = "moxart_timer",
+	.rating = 200,
+	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode = moxart_clkevt_mode,
+	.set_next_event = moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name = "moxart-timer",
+	.flags = IRQF_TIMER,
+	.handler = moxart_timer_interrupt,
+	.dev_id = &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: failed to map base\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: can't parse IRQ\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret) {
+		pr_err("%s: failed to setup IRQ %d\n", node->full_name, irq);
+		return;
+	}
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk)) {
+		pr_err("%s: of_clk_get failed\n", __func__);
+		return;
+	}
+
+	pclk = clk_get_rate(clk);
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+					"moxart_timer", pclk, 200, 32,
+					clocksource_mmio_readl_down)) {
+		pr_err("%s: clocksource_mmio_init failed\n", __func__);
+		return;
+	}
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xf0000000);
+
+	pr_info("%s: %s finished pclk=%lu HZ=%d IRQ=%d\n",
+			node->full_name, __func__, pclk, HZ, irq);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
+
-- 
1.8.2.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v3] ARM: clocksource: add support for MOXA ART SoCs
  2013-06-27 11:23     ` Jonas Jensen
@ 2013-06-28 13:34       ` Thomas Gleixner
  -1 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-06-28 13:34 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On Thu, 27 Jun 2013, Jonas Jensen wrote:
> +#define TIMER_CR			0x30
> +#define TIMER_INTR_STATE	0x34
> +#define TIMER_INTR_MASK		0x38

Please use the same indent level for all.

> +
> +static void moxart_clkevt_mode(enum clock_event_mode mode,
> +	struct clock_event_device *clk)

Please align it like this:

static void moxart_clkevt_mode(enum clock_event_mode mode,
       	    		       struct clock_event_device *clk)

Makes the code way simpler to read.

> +{
> +static int moxart_clkevt_next_event(unsigned long cycles,
> +	struct clock_event_device *unused)

Ditto.

> +{
> +	u32 u;

Newline between variable declaration and code please. All over the
place.

> +	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
> +	writel(u, base + TIMER1_BASE + REG_MATCH1);

Is this a real match functionality, i.e. is the trigger on == ?

If yes, how is guaranteed, that for a small cycles value the counter
has not passed the match value already?

> +	u = readl(base + TIMER_CR) | TIMEREG_CR_1_ENABLE;
> +	writel(u, base + TIMER_CR);
> +	return 0;
> +}
> +
> +static struct clock_event_device moxart_clockevent = {
> +	.name = "moxart_timer",

Could you please align the assigned values? i.e.

      .name	 = "moxart_timer",
      .rating	 = 200,

Way better readable than:

> +	.rating = 200,
> +	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,


> +static void __init moxart_timer_init(struct device_node *node)
> +{
> +	int ret, irq;
> +	unsigned long pclk;
> +	struct clk *clk;
> +
> +	base = of_iomap(node, 0);
> +	if (!base)
> +		panic("%s: failed to map base\n", node->full_name);
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0)
> +		panic("%s: can't parse IRQ\n", node->full_name);
> +
> +	ret = setup_irq(irq, &moxart_timer_irq);
> +	if (ret) {
> +		pr_err("%s: failed to setup IRQ %d\n", node->full_name, irq);
> +		return;

This is inconsistent. You panic on the first two checks and then you
simply return.

> +	}
> +
> +	clk = of_clk_get(node, 0);
> +	if (IS_ERR(clk)) {
> +		pr_err("%s: of_clk_get failed\n", __func__);

Your pr_errs are inconsistent. node->full_name in one and __func__ in
the next. __func__ is really not important. The node_name or simply
"moxatimer" is describing for what the clk_get failed.

> +		return;
> +	}
> +
> +	pclk = clk_get_rate(clk);
> +	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
> +
> +	writel(~0, base + TIMER2_BASE + REG_LOAD);
> +
> +	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
> +
> +	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
> +					"moxart_timer", pclk, 200, 32,
> +					clocksource_mmio_readl_down)) {

Please align the arguments consistently.

	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
				  "moxart_timer", pclk, 200, 32,
				  clocksource_mmio_readl_down)) {


> +	clockevents_config_and_register(&moxart_clockevent, pclk,
> +					0x4, 0xf0000000);

How did you come up with 0xf0000000? Random number generator?

> +	pr_info("%s: %s finished pclk=%lu HZ=%d IRQ=%d\n",
> +			node->full_name, __func__, pclk, HZ, irq);

We really do not need to know about the function name and "finished"
is completely pointless information as well.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v3] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-06-28 13:34       ` Thomas Gleixner
  0 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-06-28 13:34 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 27 Jun 2013, Jonas Jensen wrote:
> +#define TIMER_CR			0x30
> +#define TIMER_INTR_STATE	0x34
> +#define TIMER_INTR_MASK		0x38

Please use the same indent level for all.

> +
> +static void moxart_clkevt_mode(enum clock_event_mode mode,
> +	struct clock_event_device *clk)

Please align it like this:

static void moxart_clkevt_mode(enum clock_event_mode mode,
       	    		       struct clock_event_device *clk)

Makes the code way simpler to read.

> +{
> +static int moxart_clkevt_next_event(unsigned long cycles,
> +	struct clock_event_device *unused)

Ditto.

> +{
> +	u32 u;

Newline between variable declaration and code please. All over the
place.

> +	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
> +	writel(u, base + TIMER1_BASE + REG_MATCH1);

Is this a real match functionality, i.e. is the trigger on == ?

If yes, how is guaranteed, that for a small cycles value the counter
has not passed the match value already?

> +	u = readl(base + TIMER_CR) | TIMEREG_CR_1_ENABLE;
> +	writel(u, base + TIMER_CR);
> +	return 0;
> +}
> +
> +static struct clock_event_device moxart_clockevent = {
> +	.name = "moxart_timer",

Could you please align the assigned values? i.e.

      .name	 = "moxart_timer",
      .rating	 = 200,

Way better readable than:

> +	.rating = 200,
> +	.features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,


> +static void __init moxart_timer_init(struct device_node *node)
> +{
> +	int ret, irq;
> +	unsigned long pclk;
> +	struct clk *clk;
> +
> +	base = of_iomap(node, 0);
> +	if (!base)
> +		panic("%s: failed to map base\n", node->full_name);
> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0)
> +		panic("%s: can't parse IRQ\n", node->full_name);
> +
> +	ret = setup_irq(irq, &moxart_timer_irq);
> +	if (ret) {
> +		pr_err("%s: failed to setup IRQ %d\n", node->full_name, irq);
> +		return;

This is inconsistent. You panic on the first two checks and then you
simply return.

> +	}
> +
> +	clk = of_clk_get(node, 0);
> +	if (IS_ERR(clk)) {
> +		pr_err("%s: of_clk_get failed\n", __func__);

Your pr_errs are inconsistent. node->full_name in one and __func__ in
the next. __func__ is really not important. The node_name or simply
"moxatimer" is describing for what the clk_get failed.

> +		return;
> +	}
> +
> +	pclk = clk_get_rate(clk);
> +	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
> +
> +	writel(~0, base + TIMER2_BASE + REG_LOAD);
> +
> +	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
> +
> +	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
> +					"moxart_timer", pclk, 200, 32,
> +					clocksource_mmio_readl_down)) {

Please align the arguments consistently.

	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
				  "moxart_timer", pclk, 200, 32,
				  clocksource_mmio_readl_down)) {


> +	clockevents_config_and_register(&moxart_clockevent, pclk,
> +					0x4, 0xf0000000);

How did you come up with 0xf0000000? Random number generator?

> +	pr_info("%s: %s finished pclk=%lu HZ=%d IRQ=%d\n",
> +			node->full_name, __func__, pclk, HZ, irq);

We really do not need to know about the function name and "finished"
is completely pointless information as well.

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v4] ARM: clocksource: add support for MOXA ART SoCs
  2013-06-27 11:23     ` Jonas Jensen
@ 2013-07-01 14:02       ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-01 14:02 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd, Jonas Jensen

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    This should hopefully address most of the issues pointed out by Thomas.
    
    Preventing the counter from passing match, TIMER1 is now stopped before
    count is read, I leave it up to you to comment.
    
    I think REG_MATCH1 triggers on ==, or rather, I have no reason to believe
    otherwise. Documentation does not seem to be publicly available, at least
    my searches have come up empty.
    
    The old 2.6.9 sources which this is loosely derived from, rely entirely on
    setting load with periodic timer_tick. REG_MATCH1 was never used there.
    
    Applies to next-20130619
    
    Changes since v3:
    
    1. fix indentation
    2. stop TIMER1 before reading count in moxart_clkevt_next_event
    3. use tabs to align assigned values
    4. handle errors and use more consistent messages
    5. change max_delta from 0xf0000000 to 0xfffffffe (and add comment)

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 165 +++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..c93e1a8 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..ae95a11
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,165 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	(1 << 0)
+#define TIMEREG_CR_1_CLOCK	(1 << 1)
+#define TIMEREG_CR_1_INT	(1 << 2)
+#define TIMEREG_CR_2_ENABLE	(1 << 3)
+#define TIMEREG_CR_2_CLOCK	(1 << 4)
+#define TIMEREG_CR_2_INT	(1 << 5)
+#define TIMEREG_CR_3_ENABLE	(1 << 6)
+#define TIMEREG_CR_3_CLOCK	(1 << 7)
+#define TIMEREG_CR_3_INT	(1 << 8)
+#define TIMEREG_CR_COUNT_UP	(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	u32 u = readl(base + TIMER_CR);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		u |= TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	u = readl(base + TIMER_CR) & ~TIMEREG_CR_1_ENABLE;
+	writel(u, base + TIMER_CR);
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+	u = readl(base + TIMER_CR) | TIMEREG_CR_1_ENABLE;
+	writel(u, base + TIMER_CR);
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v4] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-01 14:02       ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-01 14:02 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    This should hopefully address most of the issues pointed out by Thomas.
    
    Preventing the counter from passing match, TIMER1 is now stopped before
    count is read, I leave it up to you to comment.
    
    I think REG_MATCH1 triggers on ==, or rather, I have no reason to believe
    otherwise. Documentation does not seem to be publicly available, at least
    my searches have come up empty.
    
    The old 2.6.9 sources which this is loosely derived from, rely entirely on
    setting load with periodic timer_tick. REG_MATCH1 was never used there.
    
    Applies to next-20130619
    
    Changes since v3:
    
    1. fix indentation
    2. stop TIMER1 before reading count in moxart_clkevt_next_event
    3. use tabs to align assigned values
    4. handle errors and use more consistent messages
    5. change max_delta from 0xf0000000 to 0xfffffffe (and add comment)

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 165 +++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8d979c7..c93e1a8 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -29,3 +29,4 @@ obj-$(CONFIG_CLKSRC_SAMSUNG_PWM)	+= samsung_pwm_timer.o
 
 obj-$(CONFIG_ARM_ARCH_TIMER)		+= arm_arch_timer.o
 obj-$(CONFIG_CLKSRC_METAG_GENERIC)	+= metag_generic.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..ae95a11
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,165 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	(1 << 0)
+#define TIMEREG_CR_1_CLOCK	(1 << 1)
+#define TIMEREG_CR_1_INT	(1 << 2)
+#define TIMEREG_CR_2_ENABLE	(1 << 3)
+#define TIMEREG_CR_2_CLOCK	(1 << 4)
+#define TIMEREG_CR_2_INT	(1 << 5)
+#define TIMEREG_CR_3_ENABLE	(1 << 6)
+#define TIMEREG_CR_3_CLOCK	(1 << 7)
+#define TIMEREG_CR_3_INT	(1 << 8)
+#define TIMEREG_CR_COUNT_UP	(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	u32 u = readl(base + TIMER_CR);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		u |= TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		u &= ~TIMEREG_CR_1_ENABLE;
+		writel(u, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	u = readl(base + TIMER_CR) & ~TIMEREG_CR_1_ENABLE;
+	writel(u, base + TIMER_CR);
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+	u = readl(base + TIMER_CR) | TIMEREG_CR_1_ENABLE;
+	writel(u, base + TIMER_CR);
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v4] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-01 14:02       ` Jonas Jensen
@ 2013-07-01 17:55         ` Thomas Gleixner
  -1 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-07-01 17:55 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On Mon, 1 Jul 2013, Jonas Jensen wrote:
> +static int moxart_clkevt_next_event(unsigned long cycles,
> +				    struct clock_event_device *unused)
> +{
> +	u32 u;
> +
> +	u = readl(base + TIMER_CR) & ~TIMEREG_CR_1_ENABLE;

You should cache that value and avoid another readout below. You could
even cache it in general so you avoid all readouts.

> +	writel(u, base + TIMER_CR);
> +	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
> +	writel(u, base + TIMER1_BASE + REG_MATCH1);
> +	u = readl(base + TIMER_CR) | TIMEREG_CR_1_ENABLE;
> +	writel(u, base + TIMER_CR);
> +	return 0;
> +}

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v4] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-01 17:55         ` Thomas Gleixner
  0 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-07-01 17:55 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, 1 Jul 2013, Jonas Jensen wrote:
> +static int moxart_clkevt_next_event(unsigned long cycles,
> +				    struct clock_event_device *unused)
> +{
> +	u32 u;
> +
> +	u = readl(base + TIMER_CR) & ~TIMEREG_CR_1_ENABLE;

You should cache that value and avoid another readout below. You could
even cache it in general so you avoid all readouts.

> +	writel(u, base + TIMER_CR);
> +	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
> +	writel(u, base + TIMER1_BASE + REG_MATCH1);
> +	u = readl(base + TIMER_CR) | TIMEREG_CR_1_ENABLE;
> +	writel(u, base + TIMER_CR);
> +	return 0;
> +}

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v4] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-01 14:02       ` Jonas Jensen
@ 2013-07-02 20:19         ` Linus Walleij
  -1 siblings, 0 replies; 62+ messages in thread
From: Linus Walleij @ 2013-07-02 20:19 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, John Stultz,
	Thomas Gleixner, Uwe Kleine-König, Tomasz Figa,
	Thomas Petazzoni, Arnd Bergmann

On Mon, Jul 1, 2013 at 4:02 PM, Jonas Jensen <jonas.jensen@gmail.com> wrote:

> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
>
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>

I'm pretty happy with this version, tglx points out a possible optimization
getting rid of some unnecessary reads in hotpath but anyway:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v4] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-02 20:19         ` Linus Walleij
  0 siblings, 0 replies; 62+ messages in thread
From: Linus Walleij @ 2013-07-02 20:19 UTC (permalink / raw)
  To: linux-arm-kernel

On Mon, Jul 1, 2013 at 4:02 PM, Jonas Jensen <jonas.jensen@gmail.com> wrote:

> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
>
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>

I'm pretty happy with this version, tglx points out a possible optimization
getting rid of some unnecessary reads in hotpath but anyway:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-01 14:02       ` Jonas Jensen
@ 2013-07-04 12:19         ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-04 12:19 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd, Jonas Jensen

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Applies to next-20130703
    
    Changes since v4:
    
    1. add general cache for TIMER_CR register

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 163 +++++++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 9ba8b4d..56257f6 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..61601ef
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,163 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	(1 << 0)
+#define TIMEREG_CR_1_CLOCK	(1 << 1)
+#define TIMEREG_CR_1_INT	(1 << 2)
+#define TIMEREG_CR_2_ENABLE	(1 << 3)
+#define TIMEREG_CR_2_CLOCK	(1 << 4)
+#define TIMEREG_CR_2_INT	(1 << 5)
+#define TIMEREG_CR_3_ENABLE	(1 << 6)
+#define TIMEREG_CR_3_CLOCK	(1 << 7)
+#define TIMEREG_CR_3_INT	(1 << 8)
+#define TIMEREG_CR_COUNT_UP	(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+static u32 timereg_cache;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(timereg_cache & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(timereg_cache | TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(timereg_cache & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	writel(timereg_cache & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(timereg_cache | TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	timereg_cache = readl(base + TIMER_CR) | TIMEREG_CR_2_ENABLE;
+	writel(timereg_cache, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-04 12:19         ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-04 12:19 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Applies to next-20130703
    
    Changes since v4:
    
    1. add general cache for TIMER_CR register

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 163 +++++++++++++++++++++++++++++++++++++
 2 files changed, 164 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 9ba8b4d..56257f6 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..61601ef
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,163 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	(1 << 0)
+#define TIMEREG_CR_1_CLOCK	(1 << 1)
+#define TIMEREG_CR_1_INT	(1 << 2)
+#define TIMEREG_CR_2_ENABLE	(1 << 3)
+#define TIMEREG_CR_2_CLOCK	(1 << 4)
+#define TIMEREG_CR_2_INT	(1 << 5)
+#define TIMEREG_CR_3_ENABLE	(1 << 6)
+#define TIMEREG_CR_3_CLOCK	(1 << 7)
+#define TIMEREG_CR_3_INT	(1 << 8)
+#define TIMEREG_CR_COUNT_UP	(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+static u32 timereg_cache;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(timereg_cache & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(timereg_cache | TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(timereg_cache & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	writel(timereg_cache & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(timereg_cache | TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	timereg_cache = readl(base + TIMER_CR) | TIMEREG_CR_2_ENABLE;
+	writel(timereg_cache, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-04 12:19         ` Jonas Jensen
@ 2013-07-04 21:42           ` Thomas Gleixner
  -1 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-07-04 21:42 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On Thu, 4 Jul 2013, Jonas Jensen wrote:

> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> ---
> 
> Notes:
>     Applies to next-20130703
>     
>     Changes since v4:
>     
>     1. add general cache for TIMER_CR register

What you implemented is not a register cache. A register cache is
caching the current value and not some initial constant.
 
> +static void moxart_clkevt_mode(enum clock_event_mode mode,
> +			       struct clock_event_device *clk)
> +{
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_RESUME:
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		writel(timereg_cache & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);

You just modify bits on the "cache" variable. though you are not
caching it. As it seems to work it looks like this register simply can
be written with constants.

> +	timereg_cache = readl(base + TIMER_CR) | TIMEREG_CR_2_ENABLE;

Why are you reading that back? You know excactly which of the timers
you are using and none of those should be enabled before you reach
that code. If it one of them is enabled by the boot loader you better
disable it in this init function. 

Now if you disable all of those timers and just use a known set, then
you can do without a pseudo cache variable and just write constants
into the control register, right ?

Thanks,

	tglx



^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-04 21:42           ` Thomas Gleixner
  0 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-07-04 21:42 UTC (permalink / raw)
  To: linux-arm-kernel

On Thu, 4 Jul 2013, Jonas Jensen wrote:

> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> ---
> 
> Notes:
>     Applies to next-20130703
>     
>     Changes since v4:
>     
>     1. add general cache for TIMER_CR register

What you implemented is not a register cache. A register cache is
caching the current value and not some initial constant.
 
> +static void moxart_clkevt_mode(enum clock_event_mode mode,
> +			       struct clock_event_device *clk)
> +{
> +	switch (mode) {
> +	case CLOCK_EVT_MODE_RESUME:
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		writel(timereg_cache & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);

You just modify bits on the "cache" variable. though you are not
caching it. As it seems to work it looks like this register simply can
be written with constants.

> +	timereg_cache = readl(base + TIMER_CR) | TIMEREG_CR_2_ENABLE;

Why are you reading that back? You know excactly which of the timers
you are using and none of those should be enabled before you reach
that code. If it one of them is enabled by the boot loader you better
disable it in this init function. 

Now if you disable all of those timers and just use a known set, then
you can do without a pseudo cache variable and just write constants
into the control register, right ?

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v6] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-04 12:19         ` Jonas Jensen
@ 2013-07-05 10:04           ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-05 10:04 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd, Jonas Jensen

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Applies to next-20130703
    
    Changes since v5:
    
    1. remove global timereg_cache variable
    2. use local cache for TIMER_CR register

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 165 +++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 9ba8b4d..56257f6 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..2adecdc
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,165 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	(1 << 0)
+#define TIMEREG_CR_1_CLOCK	(1 << 1)
+#define TIMEREG_CR_1_INT	(1 << 2)
+#define TIMEREG_CR_2_ENABLE	(1 << 3)
+#define TIMEREG_CR_2_CLOCK	(1 << 4)
+#define TIMEREG_CR_2_INT	(1 << 5)
+#define TIMEREG_CR_3_ENABLE	(1 << 6)
+#define TIMEREG_CR_3_CLOCK	(1 << 7)
+#define TIMEREG_CR_3_INT	(1 << 8)
+#define TIMEREG_CR_COUNT_UP	(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	u32 u = readl(base + TIMER_CR);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(u & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(u | TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(u & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u1, u2;
+
+	u1 = readl(base + TIMER_CR);
+
+	writel(u1 & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+
+	u2 = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u2, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(u1 | TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v6] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-05 10:04           ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-05 10:04 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Applies to next-20130703
    
    Changes since v5:
    
    1. remove global timereg_cache variable
    2. use local cache for TIMER_CR register

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 165 +++++++++++++++++++++++++++++++++++++
 2 files changed, 166 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 9ba8b4d..56257f6 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..2adecdc
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,165 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	(1 << 0)
+#define TIMEREG_CR_1_CLOCK	(1 << 1)
+#define TIMEREG_CR_1_INT	(1 << 2)
+#define TIMEREG_CR_2_ENABLE	(1 << 3)
+#define TIMEREG_CR_2_CLOCK	(1 << 4)
+#define TIMEREG_CR_2_INT	(1 << 5)
+#define TIMEREG_CR_3_ENABLE	(1 << 6)
+#define TIMEREG_CR_3_CLOCK	(1 << 7)
+#define TIMEREG_CR_3_INT	(1 << 8)
+#define TIMEREG_CR_COUNT_UP	(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	u32 u = readl(base + TIMER_CR);
+
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(u & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(u | TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(u & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u1, u2;
+
+	u1 = readl(base + TIMER_CR);
+
+	writel(u1 & ~TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+
+	u2 = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u2, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(u1 | TIMEREG_CR_1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-04 21:42           ` Thomas Gleixner
@ 2013-07-05 10:05             ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-05 10:05 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On 4 July 2013 23:42, Thomas Gleixner <tglx@linutronix.de> wrote:
> You just modify bits on the "cache" variable. though you are not
> caching it. As it seems to work it looks like this register simply can
> be written with constants.

I agree, the global "cache" variable wasn't very good. The only good thing, that
it eliminated all TIMER_CR reads in moxart_clkevt_next_event.

Yes it could be written with constants, and it wouldn't be so bad, because in
this case so few need to be set. If more constants were set from init
the benefit
would be more clear.

>> +     timereg_cache = readl(base + TIMER_CR) | TIMEREG_CR_2_ENABLE;
>
> Why are you reading that back? You know excactly which of the timers
> you are using and none of those should be enabled before you reach
> that code. If it one of them is enabled by the boot loader you better
> disable it in this init function.

Removed. All timers except TIMER2 should be disabled in init.

> Now if you disable all of those timers and just use a known set, then
> you can do without a pseudo cache variable and just write constants
> into the control register, right ?

Yes, please take a look at v6.


Best regards,
Jonas

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-05 10:05             ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-05 10:05 UTC (permalink / raw)
  To: linux-arm-kernel

On 4 July 2013 23:42, Thomas Gleixner <tglx@linutronix.de> wrote:
> You just modify bits on the "cache" variable. though you are not
> caching it. As it seems to work it looks like this register simply can
> be written with constants.

I agree, the global "cache" variable wasn't very good. The only good thing, that
it eliminated all TIMER_CR reads in moxart_clkevt_next_event.

Yes it could be written with constants, and it wouldn't be so bad, because in
this case so few need to be set. If more constants were set from init
the benefit
would be more clear.

>> +     timereg_cache = readl(base + TIMER_CR) | TIMEREG_CR_2_ENABLE;
>
> Why are you reading that back? You know excactly which of the timers
> you are using and none of those should be enabled before you reach
> that code. If it one of them is enabled by the boot loader you better
> disable it in this init function.

Removed. All timers except TIMER2 should be disabled in init.

> Now if you disable all of those timers and just use a known set, then
> you can do without a pseudo cache variable and just write constants
> into the control register, right ?

Yes, please take a look at v6.


Best regards,
Jonas

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-05 10:05             ` Jonas Jensen
@ 2013-07-05 10:21               ` Thomas Gleixner
  -1 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-07-05 10:21 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On Fri, 5 Jul 2013, Jonas Jensen wrote:

> On 4 July 2013 23:42, Thomas Gleixner <tglx@linutronix.de> wrote:
> > You just modify bits on the "cache" variable. though you are not
> > caching it. As it seems to work it looks like this register simply can
> > be written with constants.
> 
> I agree, the global "cache" variable wasn't very good. The only good thing, that
> it eliminated all TIMER_CR reads in moxart_clkevt_next_event.

Well, you can use a global cache variable. But that wants to be
implemented as a real cache, i.e. it always contains the current state
of the register.

   cache = 0;

   cache |= T2_ENABLE;
   write(cache, CR);
   ....
 
> Yes it could be written with constants, and it wouldn't be so bad, because in
> this case so few need to be set. If more constants were set from init
> the benefit
> would be more clear.
>
> >> +     timereg_cache = readl(base + TIMER_CR) | TIMEREG_CR_2_ENABLE;
> >
> > Why are you reading that back? You know excactly which of the timers
> > you are using and none of those should be enabled before you reach
> > that code. If it one of them is enabled by the boot loader you better
> > disable it in this init function.
> 
> Removed. All timers except TIMER2 should be disabled in init.
> 
> > Now if you disable all of those timers and just use a known set, then
> > you can do without a pseudo cache variable and just write constants
> > into the control register, right ?
> 
> Yes, please take a look at v6.

You are still reading from the control register.

What's wrong with:

#define T1_ENABLE	(TIMEREG_CR_2_ENABLE | TIMEREG_CR_1_ENABLE)
#define T1_DISABLE	(TIMEREG_CR_2_ENABLE)

Because you need to preserve the CR2 enable bit so your clocksource
does not get switched off.

Then the set_mode/next_event functions simply do:

     write(T1_DISABLE)
     write(data)
     write(T1_ENABLE)

Hmm?

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-05 10:21               ` Thomas Gleixner
  0 siblings, 0 replies; 62+ messages in thread
From: Thomas Gleixner @ 2013-07-05 10:21 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, 5 Jul 2013, Jonas Jensen wrote:

> On 4 July 2013 23:42, Thomas Gleixner <tglx@linutronix.de> wrote:
> > You just modify bits on the "cache" variable. though you are not
> > caching it. As it seems to work it looks like this register simply can
> > be written with constants.
> 
> I agree, the global "cache" variable wasn't very good. The only good thing, that
> it eliminated all TIMER_CR reads in moxart_clkevt_next_event.

Well, you can use a global cache variable. But that wants to be
implemented as a real cache, i.e. it always contains the current state
of the register.

   cache = 0;

   cache |= T2_ENABLE;
   write(cache, CR);
   ....
 
> Yes it could be written with constants, and it wouldn't be so bad, because in
> this case so few need to be set. If more constants were set from init
> the benefit
> would be more clear.
>
> >> +     timereg_cache = readl(base + TIMER_CR) | TIMEREG_CR_2_ENABLE;
> >
> > Why are you reading that back? You know excactly which of the timers
> > you are using and none of those should be enabled before you reach
> > that code. If it one of them is enabled by the boot loader you better
> > disable it in this init function.
> 
> Removed. All timers except TIMER2 should be disabled in init.
> 
> > Now if you disable all of those timers and just use a known set, then
> > you can do without a pseudo cache variable and just write constants
> > into the control register, right ?
> 
> Yes, please take a look at v6.

You are still reading from the control register.

What's wrong with:

#define T1_ENABLE	(TIMEREG_CR_2_ENABLE | TIMEREG_CR_1_ENABLE)
#define T1_DISABLE	(TIMEREG_CR_2_ENABLE)

Because you need to preserve the CR2 enable bit so your clocksource
does not get switched off.

Then the set_mode/next_event functions simply do:

     write(T1_DISABLE)
     write(data)
     write(T1_ENABLE)

Hmm?

Thanks,

	tglx

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v7] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-05 10:04           ` Jonas Jensen
@ 2013-07-05 11:46             ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-05 11:46 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd, Jonas Jensen

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Applies to next-20130703
    
    Changes since v6:
    
    1. remove TIMER_CR read back
    2. set TIMER_CR from constants

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 164 +++++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 9ba8b4d..56257f6 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..5743853
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,164 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	(1 << 0)
+#define TIMEREG_CR_1_CLOCK	(1 << 1)
+#define TIMEREG_CR_1_INT	(1 << 2)
+#define TIMEREG_CR_2_ENABLE	(1 << 3)
+#define TIMEREG_CR_2_CLOCK	(1 << 4)
+#define TIMEREG_CR_2_INT	(1 << 5)
+#define TIMEREG_CR_3_ENABLE	(1 << 6)
+#define TIMEREG_CR_3_CLOCK	(1 << 7)
+#define TIMEREG_CR_3_INT	(1 << 8)
+#define TIMEREG_CR_COUNT_UP	(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+#define TIMER1_ENABLE		(TIMEREG_CR_2_ENABLE | TIMEREG_CR_1_ENABLE)
+#define TIMER1_DISABLE		(TIMEREG_CR_2_ENABLE)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(TIMER1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	writel(TIMER1_DISABLE, base + TIMER_CR);
+
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(TIMER1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v7] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-05 11:46             ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-05 11:46 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Applies to next-20130703
    
    Changes since v6:
    
    1. remove TIMER_CR read back
    2. set TIMER_CR from constants

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 164 +++++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 9ba8b4d..56257f6 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_CLKSRC_DBX500_PRCMU)	+= clksrc-dbx500-prcmu.o
 obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..5743853
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,164 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	(1 << 0)
+#define TIMEREG_CR_1_CLOCK	(1 << 1)
+#define TIMEREG_CR_1_INT	(1 << 2)
+#define TIMEREG_CR_2_ENABLE	(1 << 3)
+#define TIMEREG_CR_2_CLOCK	(1 << 4)
+#define TIMEREG_CR_2_INT	(1 << 5)
+#define TIMEREG_CR_3_ENABLE	(1 << 6)
+#define TIMEREG_CR_3_CLOCK	(1 << 7)
+#define TIMEREG_CR_3_INT	(1 << 8)
+#define TIMEREG_CR_COUNT_UP	(1 << 9)
+#define TIMEREG_CR_COUNT_DOWN	(0 << 9)
+
+#define TIMER1_ENABLE		(TIMEREG_CR_2_ENABLE | TIMEREG_CR_1_ENABLE)
+#define TIMER1_DISABLE		(TIMEREG_CR_2_ENABLE)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(TIMER1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	writel(TIMER1_DISABLE, base + TIMER_CR);
+
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(TIMER1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-05 10:21               ` Thomas Gleixner
@ 2013-07-05 11:48                 ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-05 11:48 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

Thanks for the replies Thomas.

On 5 July 2013 12:21, Thomas Gleixner <tglx@linutronix.de> wrote:
> Because you need to preserve the CR2 enable bit so your clocksource
> does not get switched off.

Yes, that was my main concern. The possibility of more flags being added.
I was experimenting with TIMEREG_CR_COUNT_UP but could never get REG_MATCH1
to work right for oneshot mode.

Please take a look at v7.

Best regards,
Jonas

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v5] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-05 11:48                 ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-05 11:48 UTC (permalink / raw)
  To: linux-arm-kernel

Thanks for the replies Thomas.

On 5 July 2013 12:21, Thomas Gleixner <tglx@linutronix.de> wrote:
> Because you need to preserve the CR2 enable bit so your clocksource
> does not get switched off.

Yes, that was my main concern. The possibility of more flags being added.
I was experimenting with TIMEREG_CR_COUNT_UP but could never get REG_MATCH1
to work right for oneshot mode.

Please take a look at v7.

Best regards,
Jonas

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v7] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-05 11:46             ` Jonas Jensen
@ 2013-07-16 13:52               ` Daniel Lezcano
  -1 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-16 13:52 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz, tglx,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On 07/05/2013 01:46 PM, Jonas Jensen wrote:
> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> ---
> 
> Notes:
>     Applies to next-20130703
>     
>     Changes since v6:
>     
>     1. remove TIMER_CR read back
>     2. set TIMER_CR from constants
> 

Except for the few comments below, it looks ok for me.

[ ... ]

> + */
> +#define TIMEREG_CR_1_ENABLE	(1 << 0)
> +#define TIMEREG_CR_1_CLOCK	(1 << 1)
> +#define TIMEREG_CR_1_INT	(1 << 2)
> +#define TIMEREG_CR_2_ENABLE	(1 << 3)
> +#define TIMEREG_CR_2_CLOCK	(1 << 4)
> +#define TIMEREG_CR_2_INT	(1 << 5)
> +#define TIMEREG_CR_3_ENABLE	(1 << 6)
> +#define TIMEREG_CR_3_CLOCK	(1 << 7)
> +#define TIMEREG_CR_3_INT	(1 << 8)
> +#define TIMEREG_CR_COUNT_UP	(1 << 9)
> +#define TIMEREG_CR_COUNT_DOWN	(0 << 9)

Could you replace this by the BIT macro ?

[ ... ]

> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0)
> +		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
> +
> +	ret = setup_irq(irq, &moxart_timer_irq);
> +	if (ret)
> +		panic("%s: setup_irq failed\n", node->full_name);
> +
> +	clk = of_clk_get(node, 0);
> +	if (IS_ERR(clk))
> +		panic("%s: of_clk_get failed\n", node->full_name);
> +
> +	pclk = clk_get_rate(clk);
> +
> +	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
> +				  "moxart_timer", pclk, 200, 32,
> +				  clocksource_mmio_readl_down))
> +		panic("%s: clocksource_mmio_init failed\n", node->full_name);
> +
> +	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
> +
> +	writel(~0, base + TIMER2_BASE + REG_LOAD);
> +	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
> +
> +	moxart_clockevent.cpumask = cpumask_of(0);

	moxart_clockevent.irq = irq;

> +
> +	/*
> +	 * documentation is not publicly available:
> +	 * min_delta / max_delta obtained by trial-and-error,
> +	 * max_delta 0xfffffffe should be ok because count
> +	 * register size is u32
> +	 */
> +	clockevents_config_and_register(&moxart_clockevent, pclk,
> +					0x4, 0xfffffffe);
> +}
> +CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);


Thanks
  -- Daniel


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


^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v7] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-16 13:52               ` Daniel Lezcano
  0 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-16 13:52 UTC (permalink / raw)
  To: linux-arm-kernel

On 07/05/2013 01:46 PM, Jonas Jensen wrote:
> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> ---
> 
> Notes:
>     Applies to next-20130703
>     
>     Changes since v6:
>     
>     1. remove TIMER_CR read back
>     2. set TIMER_CR from constants
> 

Except for the few comments below, it looks ok for me.

[ ... ]

> + */
> +#define TIMEREG_CR_1_ENABLE	(1 << 0)
> +#define TIMEREG_CR_1_CLOCK	(1 << 1)
> +#define TIMEREG_CR_1_INT	(1 << 2)
> +#define TIMEREG_CR_2_ENABLE	(1 << 3)
> +#define TIMEREG_CR_2_CLOCK	(1 << 4)
> +#define TIMEREG_CR_2_INT	(1 << 5)
> +#define TIMEREG_CR_3_ENABLE	(1 << 6)
> +#define TIMEREG_CR_3_CLOCK	(1 << 7)
> +#define TIMEREG_CR_3_INT	(1 << 8)
> +#define TIMEREG_CR_COUNT_UP	(1 << 9)
> +#define TIMEREG_CR_COUNT_DOWN	(0 << 9)

Could you replace this by the BIT macro ?

[ ... ]

> +
> +	irq = irq_of_parse_and_map(node, 0);
> +	if (irq <= 0)
> +		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
> +
> +	ret = setup_irq(irq, &moxart_timer_irq);
> +	if (ret)
> +		panic("%s: setup_irq failed\n", node->full_name);
> +
> +	clk = of_clk_get(node, 0);
> +	if (IS_ERR(clk))
> +		panic("%s: of_clk_get failed\n", node->full_name);
> +
> +	pclk = clk_get_rate(clk);
> +
> +	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
> +				  "moxart_timer", pclk, 200, 32,
> +				  clocksource_mmio_readl_down))
> +		panic("%s: clocksource_mmio_init failed\n", node->full_name);
> +
> +	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
> +
> +	writel(~0, base + TIMER2_BASE + REG_LOAD);
> +	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
> +
> +	moxart_clockevent.cpumask = cpumask_of(0);

	moxart_clockevent.irq = irq;

> +
> +	/*
> +	 * documentation is not publicly available:
> +	 * min_delta / max_delta obtained by trial-and-error,
> +	 * max_delta 0xfffffffe should be ok because count
> +	 * register size is u32
> +	 */
> +	clockevents_config_and_register(&moxart_clockevent, pclk,
> +					0x4, 0xfffffffe);
> +}
> +CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);


Thanks
  -- Daniel


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

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v8] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-05 11:46             ` Jonas Jensen
@ 2013-07-16 14:44               ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-16 14:44 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd,
	daniel.lezcano, Jonas Jensen

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Changes since v7:
    
    1. use BIT macro
    2. set moxart_clockevent.irq
    
    Applies to next-20130716

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 164 +++++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8b00c5c..704d6d3 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ORION_TIMER)	+= time-orion.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..08a5943
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,164 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	BIT(0)
+#define TIMEREG_CR_1_CLOCK	BIT(1)
+#define TIMEREG_CR_1_INT	BIT(2)
+#define TIMEREG_CR_2_ENABLE	BIT(3)
+#define TIMEREG_CR_2_CLOCK	BIT(4)
+#define TIMEREG_CR_2_INT	BIT(5)
+#define TIMEREG_CR_3_ENABLE	BIT(6)
+#define TIMEREG_CR_3_CLOCK	BIT(7)
+#define TIMEREG_CR_3_INT	BIT(8)
+#define TIMEREG_CR_COUNT_UP	BIT(9)
+
+#define TIMER1_ENABLE		(TIMEREG_CR_2_ENABLE | TIMEREG_CR_1_ENABLE)
+#define TIMER1_DISABLE		(TIMEREG_CR_2_ENABLE)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(TIMER1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	writel(TIMER1_DISABLE, base + TIMER_CR);
+
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(TIMER1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+	moxart_clockevent.irq = irq;
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v8] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-16 14:44               ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-16 14:44 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Changes since v7:
    
    1. use BIT macro
    2. set moxart_clockevent.irq
    
    Applies to next-20130716

 drivers/clocksource/Makefile       |   1 +
 drivers/clocksource/moxart_timer.c | 164 +++++++++++++++++++++++++++++++++++++
 2 files changed, 165 insertions(+)
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8b00c5c..704d6d3 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ORION_TIMER)	+= time-orion.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..08a5943
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,164 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	BIT(0)
+#define TIMEREG_CR_1_CLOCK	BIT(1)
+#define TIMEREG_CR_1_INT	BIT(2)
+#define TIMEREG_CR_2_ENABLE	BIT(3)
+#define TIMEREG_CR_2_CLOCK	BIT(4)
+#define TIMEREG_CR_2_INT	BIT(5)
+#define TIMEREG_CR_3_ENABLE	BIT(6)
+#define TIMEREG_CR_3_CLOCK	BIT(7)
+#define TIMEREG_CR_3_INT	BIT(8)
+#define TIMEREG_CR_COUNT_UP	BIT(9)
+
+#define TIMER1_ENABLE		(TIMEREG_CR_2_ENABLE | TIMEREG_CR_1_ENABLE)
+#define TIMER1_DISABLE		(TIMEREG_CR_2_ENABLE)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(TIMER1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	writel(TIMER1_DISABLE, base + TIMER_CR);
+
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(TIMER1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+	moxart_clockevent.irq = irq;
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v8] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-16 14:44               ` Jonas Jensen
@ 2013-07-16 15:01                 ` Daniel Lezcano
  -1 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-16 15:01 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz, tglx,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On 07/16/2013 04:44 PM, Jonas Jensen wrote:
> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> ---

Ok, I will ask you two more things before taking your patch. I missed
them at the previous review, sorry for asking a new version.

1. could you elaborate the patch log: as it is a new driver, a nice
description of the timer hardware would be valuable for people willing
to understand the code (eg. some specificities of the driver).

2. fix the alignment below

Thanks
  -- Daniel

[ ... ]

> +
> +#define TIMER_CR		0x30
> +#define TIMER_INTR_STATE	0x34
> +#define TIMER_INTR_MASK		0x38


[ ... ]

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


^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v8] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-16 15:01                 ` Daniel Lezcano
  0 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-16 15:01 UTC (permalink / raw)
  To: linux-arm-kernel

On 07/16/2013 04:44 PM, Jonas Jensen wrote:
> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
> 
> Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
> ---

Ok, I will ask you two more things before taking your patch. I missed
them at the previous review, sorry for asking a new version.

1. could you elaborate the patch log: as it is a new driver, a nice
description of the timer hardware would be valuable for people willing
to understand the code (eg. some specificities of the driver).

2. fix the alignment below

Thanks
  -- Daniel

[ ... ]

> +
> +#define TIMER_CR		0x30
> +#define TIMER_INTR_STATE	0x34
> +#define TIMER_INTR_MASK		0x38


[ ... ]

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

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v9] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-16 14:44               ` Jonas Jensen
@ 2013-07-17  8:04                 ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-17  8:04 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd,
	daniel.lezcano, Jonas Jensen

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

The MOXA ART SoC provides three separate timers with individual
count/load/match registers, two are used here:

TIMER1: clockevents, used to support oneshot and periodic events
TIMER2: set up as a free running counter, used as clocksource

Timers are preconfigured by bootloader to count down and interrupt
on match or zero. Count increments every APB clock cycle and is
automatically reloaded when it reaches zero.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Changes since v8:
    
    1. add devicetree bindings document
    
    Applies to next-20130716

 .../bindings/timer/moxa,moxart-timer.txt           |  17 +++
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/moxart_timer.c                 | 164 +++++++++++++++++++++
 3 files changed, 182 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
new file mode 100644
index 0000000..77c4cfa
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
@@ -0,0 +1,17 @@
+MOXA ART timer
+
+Required properties:
+
+- compatible : Should be "moxa,moxart-timer"
+- reg : Should contain registers location and length
+- interrupts : Should contain the timer interrupt number
+- clocks : Should contain phandle for APB clock "clkapb"
+
+Example:
+
+	timer: timer@98400000 {
+		compatible = "moxa,moxart-timer";
+		reg = <0x98400000 0x42>;
+		interrupts = <19 1>;
+		clocks = <&clkapb>;
+	};
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8b00c5c..704d6d3 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ORION_TIMER)	+= time-orion.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..08a5943
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,164 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	BIT(0)
+#define TIMEREG_CR_1_CLOCK	BIT(1)
+#define TIMEREG_CR_1_INT	BIT(2)
+#define TIMEREG_CR_2_ENABLE	BIT(3)
+#define TIMEREG_CR_2_CLOCK	BIT(4)
+#define TIMEREG_CR_2_INT	BIT(5)
+#define TIMEREG_CR_3_ENABLE	BIT(6)
+#define TIMEREG_CR_3_CLOCK	BIT(7)
+#define TIMEREG_CR_3_INT	BIT(8)
+#define TIMEREG_CR_COUNT_UP	BIT(9)
+
+#define TIMER1_ENABLE		(TIMEREG_CR_2_ENABLE | TIMEREG_CR_1_ENABLE)
+#define TIMER1_DISABLE		(TIMEREG_CR_2_ENABLE)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(TIMER1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	writel(TIMER1_DISABLE, base + TIMER_CR);
+
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(TIMER1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+	moxart_clockevent.irq = irq;
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH v9] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-17  8:04                 ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-17  8:04 UTC (permalink / raw)
  To: linux-arm-kernel

This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.

The MOXA ART SoC provides three separate timers with individual
count/load/match registers, two are used here:

TIMER1: clockevents, used to support oneshot and periodic events
TIMER2: set up as a free running counter, used as clocksource

Timers are preconfigured by bootloader to count down and interrupt
on match or zero. Count increments every APB clock cycle and is
automatically reloaded when it reaches zero.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Changes since v8:
    
    1. add devicetree bindings document
    
    Applies to next-20130716

 .../bindings/timer/moxa,moxart-timer.txt           |  17 +++
 drivers/clocksource/Makefile                       |   1 +
 drivers/clocksource/moxart_timer.c                 | 164 +++++++++++++++++++++
 3 files changed, 182 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
 create mode 100644 drivers/clocksource/moxart_timer.c

diff --git a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
new file mode 100644
index 0000000..77c4cfa
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
@@ -0,0 +1,17 @@
+MOXA ART timer
+
+Required properties:
+
+- compatible : Should be "moxa,moxart-timer"
+- reg : Should contain registers location and length
+- interrupts : Should contain the timer interrupt number
+- clocks : Should contain phandle for APB clock "clkapb"
+
+Example:
+
+	timer: timer at 98400000 {
+		compatible = "moxa,moxart-timer";
+		reg = <0x98400000 0x42>;
+		interrupts = <19 1>;
+		clocks = <&clkapb>;
+	};
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile
index 8b00c5c..704d6d3 100644
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_ARMADA_370_XP_TIMER)	+= time-armada-370-xp.o
 obj-$(CONFIG_ORION_TIMER)	+= time-orion.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835_timer.o
 obj-$(CONFIG_ARCH_MARCO)	+= timer-marco.o
+obj-$(CONFIG_ARCH_MOXART)	+= moxart_timer.o
 obj-$(CONFIG_ARCH_MXS)		+= mxs_timer.o
 obj-$(CONFIG_ARCH_PRIMA2)	+= timer-prima2.o
 obj-$(CONFIG_SUN4I_TIMER)	+= sun4i_timer.o
diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
new file mode 100644
index 0000000..08a5943
--- /dev/null
+++ b/drivers/clocksource/moxart_timer.c
@@ -0,0 +1,164 @@
+/*
+ * MOXA ART SoCs timer handling.
+ *
+ * Copyright (C) 2013 Jonas Jensen
+ *
+ * Jonas Jensen <jonas.jensen@gmail.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/clk.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+#include <linux/irqreturn.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/io.h>
+#include <linux/clocksource.h>
+
+#define TIMER1_BASE		0x00
+#define TIMER2_BASE		0x10
+#define TIMER3_BASE		0x20
+
+#define REG_COUNT		0x0 /* writable */
+#define REG_LOAD		0x4
+#define REG_MATCH1		0x8
+#define REG_MATCH2		0xC
+
+#define TIMER_CR		0x30
+#define TIMER_INTR_STATE	0x34
+#define TIMER_INTR_MASK		0x38
+
+/*
+ * TIMER_CR flags:
+ *
+ * TIMEREG_CR_*_CLOCK	0: PCLK, 1: EXT1CLK
+ * TIMEREG_CR_*_INT	overflow interrupt enable bit
+ */
+#define TIMEREG_CR_1_ENABLE	BIT(0)
+#define TIMEREG_CR_1_CLOCK	BIT(1)
+#define TIMEREG_CR_1_INT	BIT(2)
+#define TIMEREG_CR_2_ENABLE	BIT(3)
+#define TIMEREG_CR_2_CLOCK	BIT(4)
+#define TIMEREG_CR_2_INT	BIT(5)
+#define TIMEREG_CR_3_ENABLE	BIT(6)
+#define TIMEREG_CR_3_CLOCK	BIT(7)
+#define TIMEREG_CR_3_INT	BIT(8)
+#define TIMEREG_CR_COUNT_UP	BIT(9)
+
+#define TIMER1_ENABLE		(TIMEREG_CR_2_ENABLE | TIMEREG_CR_1_ENABLE)
+#define TIMER1_DISABLE		(TIMEREG_CR_2_ENABLE)
+
+static void __iomem *base;
+static unsigned int clock_count_per_tick;
+
+static void moxart_clkevt_mode(enum clock_event_mode mode,
+			       struct clock_event_device *clk)
+{
+	switch (mode) {
+	case CLOCK_EVT_MODE_RESUME:
+	case CLOCK_EVT_MODE_ONESHOT:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		writel(~0, base + TIMER1_BASE + REG_LOAD);
+		break;
+	case CLOCK_EVT_MODE_PERIODIC:
+		writel(clock_count_per_tick, base + TIMER1_BASE + REG_LOAD);
+		writel(TIMER1_ENABLE, base + TIMER_CR);
+		break;
+	case CLOCK_EVT_MODE_UNUSED:
+	case CLOCK_EVT_MODE_SHUTDOWN:
+	default:
+		writel(TIMER1_DISABLE, base + TIMER_CR);
+		break;
+	}
+}
+
+static int moxart_clkevt_next_event(unsigned long cycles,
+				    struct clock_event_device *unused)
+{
+	u32 u;
+
+	writel(TIMER1_DISABLE, base + TIMER_CR);
+
+	u = readl(base + TIMER1_BASE + REG_COUNT) - cycles;
+	writel(u, base + TIMER1_BASE + REG_MATCH1);
+
+	writel(TIMER1_ENABLE, base + TIMER_CR);
+
+	return 0;
+}
+
+static struct clock_event_device moxart_clockevent = {
+	.name		= "moxart_timer",
+	.rating		= 200,
+	.features	= CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+	.set_mode	= moxart_clkevt_mode,
+	.set_next_event	= moxart_clkevt_next_event,
+};
+
+static irqreturn_t moxart_timer_interrupt(int irq, void *dev_id)
+{
+	struct clock_event_device *evt = dev_id;
+	evt->event_handler(evt);
+	return IRQ_HANDLED;
+}
+
+static struct irqaction moxart_timer_irq = {
+	.name		= "moxart-timer",
+	.flags		= IRQF_TIMER,
+	.handler	= moxart_timer_interrupt,
+	.dev_id		= &moxart_clockevent,
+};
+
+static void __init moxart_timer_init(struct device_node *node)
+{
+	int ret, irq;
+	unsigned long pclk;
+	struct clk *clk;
+
+	base = of_iomap(node, 0);
+	if (!base)
+		panic("%s: of_iomap failed\n", node->full_name);
+
+	irq = irq_of_parse_and_map(node, 0);
+	if (irq <= 0)
+		panic("%s: irq_of_parse_and_map failed\n", node->full_name);
+
+	ret = setup_irq(irq, &moxart_timer_irq);
+	if (ret)
+		panic("%s: setup_irq failed\n", node->full_name);
+
+	clk = of_clk_get(node, 0);
+	if (IS_ERR(clk))
+		panic("%s: of_clk_get failed\n", node->full_name);
+
+	pclk = clk_get_rate(clk);
+
+	if (clocksource_mmio_init(base + TIMER2_BASE + REG_COUNT,
+				  "moxart_timer", pclk, 200, 32,
+				  clocksource_mmio_readl_down))
+		panic("%s: clocksource_mmio_init failed\n", node->full_name);
+
+	clock_count_per_tick = DIV_ROUND_CLOSEST(pclk, HZ);
+
+	writel(~0, base + TIMER2_BASE + REG_LOAD);
+	writel(TIMEREG_CR_2_ENABLE, base + TIMER_CR);
+
+	moxart_clockevent.cpumask = cpumask_of(0);
+	moxart_clockevent.irq = irq;
+
+	/*
+	 * documentation is not publicly available:
+	 * min_delta / max_delta obtained by trial-and-error,
+	 * max_delta 0xfffffffe should be ok because count
+	 * register size is u32
+	 */
+	clockevents_config_and_register(&moxart_clockevent, pclk,
+					0x4, 0xfffffffe);
+}
+CLOCKSOURCE_OF_DECLARE(moxart, "moxa,moxart-timer", moxart_timer_init);
-- 
1.8.2.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH v8] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-16 15:01                 ` Daniel Lezcano
@ 2013-07-17  8:14                   ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-17  8:14 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz, tglx,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On 16 July 2013 17:01, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> 1. could you elaborate the patch log: as it is a new driver, a nice
> description of the timer hardware would be valuable for people willing
> to understand the code (eg. some specificities of the driver).

A short description is now added to the commit message, I hope this is
what you wanted, I can reformat the text if needed.

> 2. fix the alignment below

I don't see the alignment error, I think this is because git adds the
one '+' character, it should be correct once applied?


Best regards,
Jonas

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v8] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-17  8:14                   ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-17  8:14 UTC (permalink / raw)
  To: linux-arm-kernel

On 16 July 2013 17:01, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> 1. could you elaborate the patch log: as it is a new driver, a nice
> description of the timer hardware would be valuable for people willing
> to understand the code (eg. some specificities of the driver).

A short description is now added to the commit message, I hope this is
what you wanted, I can reformat the text if needed.

> 2. fix the alignment below

I don't see the alignment error, I think this is because git adds the
one '+' character, it should be correct once applied?


Best regards,
Jonas

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v8] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-17  8:14                   ` Jonas Jensen
@ 2013-07-17 12:13                     ` Daniel Lezcano
  -1 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-17 12:13 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz, tglx,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On 07/17/2013 10:14 AM, Jonas Jensen wrote:
> On 16 July 2013 17:01, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> 1. could you elaborate the patch log: as it is a new driver, a nice
>> description of the timer hardware would be valuable for people willing
>> to understand the code (eg. some specificities of the driver).
> 
> A short description is now added to the commit message, I hope this is
> what you wanted, I can reformat the text if needed.

It is ok. Thanks for the update.


>> 2. fix the alignment below
> 
> I don't see the alignment error, I think this is because git adds the
> one '+' character, it should be correct once applied?

Ok. I will check it.

Thanks
  -- Daniel

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


^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v8] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-17 12:13                     ` Daniel Lezcano
  0 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-17 12:13 UTC (permalink / raw)
  To: linux-arm-kernel

On 07/17/2013 10:14 AM, Jonas Jensen wrote:
> On 16 July 2013 17:01, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>> 1. could you elaborate the patch log: as it is a new driver, a nice
>> description of the timer hardware would be valuable for people willing
>> to understand the code (eg. some specificities of the driver).
> 
> A short description is now added to the commit message, I hope this is
> what you wanted, I can reformat the text if needed.

It is ok. Thanks for the update.


>> 2. fix the alignment below
> 
> I don't see the alignment error, I think this is because git adds the
> one '+' character, it should be correct once applied?

Ok. I will check it.

Thanks
  -- Daniel

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

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH] ARM: clocksource: moxart: documentation: update device tree bindings document
  2013-07-17  8:04                 ` Jonas Jensen
@ 2013-07-19 11:12                   ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-19 11:12 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd,
	daniel.lezcano, Jonas Jensen

Update device tree bindings document with the correct clock name.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Hi Daniel,
    
    I apologize in advance if this causes trouble..
    
    One of the device tree bindings files you merged
    git://git.linaro.org/people/dlezcano/clockevents.git timers/clockevents-next
    now needs an update.
    
    This patch can be applied directly to timers/clockevents-next.
    
    Best regards,
    Jonas

 Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
index 77c4cfa..ad0bf7f 100644
--- a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
+++ b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
@@ -5,7 +5,7 @@ Required properties:
 - compatible : Should be "moxa,moxart-timer"
 - reg : Should contain registers location and length
 - interrupts : Should contain the timer interrupt number
-- clocks : Should contain phandle for APB clock "clkapb"
+- clocks : Should contain phandle for the MOXA ART core clock "coreclk"
 
 Example:
 
@@ -13,5 +13,5 @@ Example:
 		compatible = "moxa,moxart-timer";
 		reg = <0x98400000 0x42>;
 		interrupts = <19 1>;
-		clocks = <&clkapb>;
+		clocks = <&coreclk>;
 	};
-- 
1.8.2.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH] ARM: clocksource: moxart: documentation: update device tree bindings document
@ 2013-07-19 11:12                   ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-19 11:12 UTC (permalink / raw)
  To: linux-arm-kernel

Update device tree bindings document with the correct clock name.

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Hi Daniel,
    
    I apologize in advance if this causes trouble..
    
    One of the device tree bindings files you merged
    git://git.linaro.org/people/dlezcano/clockevents.git timers/clockevents-next
    now needs an update.
    
    This patch can be applied directly to timers/clockevents-next.
    
    Best regards,
    Jonas

 Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
index 77c4cfa..ad0bf7f 100644
--- a/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
+++ b/Documentation/devicetree/bindings/timer/moxa,moxart-timer.txt
@@ -5,7 +5,7 @@ Required properties:
 - compatible : Should be "moxa,moxart-timer"
 - reg : Should contain registers location and length
 - interrupts : Should contain the timer interrupt number
-- clocks : Should contain phandle for APB clock "clkapb"
+- clocks : Should contain phandle for the MOXA ART core clock "coreclk"
 
 Example:
 
@@ -13,5 +13,5 @@ Example:
 		compatible = "moxa,moxart-timer";
 		reg = <0x98400000 0x42>;
 		interrupts = <19 1>;
-		clocks = <&clkapb>;
+		clocks = <&coreclk>;
 	};
-- 
1.8.2.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread

* Re: [PATCH] ARM: clocksource: moxart: documentation: update device tree bindings document
  2013-07-19 11:12                   ` Jonas Jensen
@ 2013-07-19 12:16                     ` Daniel Lezcano
  -1 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-19 12:16 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz, tglx,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On 07/19/2013 01:12 PM, Jonas Jensen wrote:
> Update device tree bindings document with the correct clock name.

Ok, pushed.


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


^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH] ARM: clocksource: moxart: documentation: update device tree bindings document
@ 2013-07-19 12:16                     ` Daniel Lezcano
  0 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-19 12:16 UTC (permalink / raw)
  To: linux-arm-kernel

On 07/19/2013 01:12 PM, Jonas Jensen wrote:
> Update device tree bindings document with the correct clock name.

Ok, pushed.


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

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH] ARM: clocksource: moxart: documentation: update device tree bindings document
  2013-07-19 12:16                     ` Daniel Lezcano
@ 2013-07-19 12:50                       ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-19 12:50 UTC (permalink / raw)
  To: Daniel Lezcano
  Cc: linux-arm-kernel, linux-kernel, arm, john.stultz, tglx,
	u.kleine-koenig, tomasz.figa, linus.walleij, thomas.petazzoni,
	arnd

On 19 July 2013 14:16, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Ok, pushed.

Thanks :)

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH] ARM: clocksource: moxart: documentation: update device tree bindings document
@ 2013-07-19 12:50                       ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-19 12:50 UTC (permalink / raw)
  To: linux-arm-kernel

On 19 July 2013 14:16, Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> Ok, pushed.

Thanks :)

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v9] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-17  8:04                 ` Jonas Jensen
@ 2013-07-20 20:45                   ` Linus Walleij
  -1 siblings, 0 replies; 62+ messages in thread
From: Linus Walleij @ 2013-07-20 20:45 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: linux-arm-kernel, linux-kernel, arm, John Stultz,
	Thomas Gleixner, Uwe Kleine-König, Tomasz Figa,
	Thomas Petazzoni, Arnd Bergmann, Daniel Lezcano

On Wed, Jul 17, 2013 at 10:04 AM, Jonas Jensen <jonas.jensen@gmail.com> wrote:

> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqreturn.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +#include <linux/clocksource.h>

#include <linux/bitops.h>

> +#define TIMEREG_CR_1_ENABLE    BIT(0)

Because BIT() comes from there. And we shall not rely on
implicit #inclusion.

Apart from that:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v9] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-20 20:45                   ` Linus Walleij
  0 siblings, 0 replies; 62+ messages in thread
From: Linus Walleij @ 2013-07-20 20:45 UTC (permalink / raw)
  To: linux-arm-kernel

On Wed, Jul 17, 2013 at 10:04 AM, Jonas Jensen <jonas.jensen@gmail.com> wrote:

> +#include <linux/clk.h>
> +#include <linux/clockchips.h>
> +#include <linux/interrupt.h>
> +#include <linux/irq.h>
> +#include <linux/irqreturn.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/io.h>
> +#include <linux/clocksource.h>

#include <linux/bitops.h>

> +#define TIMEREG_CR_1_ENABLE    BIT(0)

Because BIT() comes from there. And we shall not rely on
implicit #inclusion.

Apart from that:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 62+ messages in thread

* Re: [PATCH v9] ARM: clocksource: add support for MOXA ART SoCs
  2013-07-20 20:45                   ` Linus Walleij
@ 2013-07-20 21:34                     ` Daniel Lezcano
  -1 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-20 21:34 UTC (permalink / raw)
  To: Jonas Jensen
  Cc: Linus Walleij, linux-arm-kernel, linux-kernel, arm, John Stultz,
	Thomas Gleixner, Uwe Kleine-König, Tomasz Figa,
	Thomas Petazzoni, Arnd Bergmann

On 07/20/2013 10:45 PM, Linus Walleij wrote:
> On Wed, Jul 17, 2013 at 10:04 AM, Jonas Jensen <jonas.jensen@gmail.com> wrote:
> 
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqreturn.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/io.h>
>> +#include <linux/clocksource.h>
> 
> #include <linux/bitops.h>
> 
>> +#define TIMEREG_CR_1_ENABLE    BIT(0)
> 
> Because BIT() comes from there. And we shall not rely on
> implicit #inclusion.

Hi Jonas,

as the patchset is already merged could you fix it with a separate patch ?

Thanks
  -- Daniel

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


^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH v9] ARM: clocksource: add support for MOXA ART SoCs
@ 2013-07-20 21:34                     ` Daniel Lezcano
  0 siblings, 0 replies; 62+ messages in thread
From: Daniel Lezcano @ 2013-07-20 21:34 UTC (permalink / raw)
  To: linux-arm-kernel

On 07/20/2013 10:45 PM, Linus Walleij wrote:
> On Wed, Jul 17, 2013 at 10:04 AM, Jonas Jensen <jonas.jensen@gmail.com> wrote:
> 
>> +#include <linux/clk.h>
>> +#include <linux/clockchips.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/irq.h>
>> +#include <linux/irqreturn.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/io.h>
>> +#include <linux/clocksource.h>
> 
> #include <linux/bitops.h>
> 
>> +#define TIMEREG_CR_1_ENABLE    BIT(0)
> 
> Because BIT() comes from there. And we shall not rely on
> implicit #inclusion.

Hi Jonas,

as the patchset is already merged could you fix it with a separate patch ?

Thanks
  -- Daniel

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

^ permalink raw reply	[flat|nested] 62+ messages in thread

* [PATCH] ARM: clocksource: moxart: add bitops.h include
  2013-07-20 21:34                     ` Daniel Lezcano
@ 2013-07-26 14:03                       ` Jonas Jensen
  -1 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-26 14:03 UTC (permalink / raw)
  To: linux-arm-kernel
  Cc: linux-kernel, arm, john.stultz, tglx, u.kleine-koenig,
	tomasz.figa, linus.walleij, thomas.petazzoni, arnd,
	daniel.lezcano, Jonas Jensen

bitops.h included implicitly, add #include <linux/bitops.h>

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Hi Daniel,
    
    A separate patch for moxart clocksource (already merged), as you requested.
    
    Can be applied directly to timers/clockevents-next.
    
    Best regards,
    Jonas

 drivers/clocksource/moxart_timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
index 08a5943..5eb2c35 100644
--- a/drivers/clocksource/moxart_timer.c
+++ b/drivers/clocksource/moxart_timer.c
@@ -20,6 +20,7 @@
 #include <linux/of_irq.h>
 #include <linux/io.h>
 #include <linux/clocksource.h>
+#include <linux/bitops.h>
 
 #define TIMER1_BASE		0x00
 #define TIMER2_BASE		0x10
-- 
1.8.2.1


^ permalink raw reply related	[flat|nested] 62+ messages in thread

* [PATCH] ARM: clocksource: moxart: add bitops.h include
@ 2013-07-26 14:03                       ` Jonas Jensen
  0 siblings, 0 replies; 62+ messages in thread
From: Jonas Jensen @ 2013-07-26 14:03 UTC (permalink / raw)
  To: linux-arm-kernel

bitops.h included implicitly, add #include <linux/bitops.h>

Signed-off-by: Jonas Jensen <jonas.jensen@gmail.com>
---

Notes:
    Hi Daniel,
    
    A separate patch for moxart clocksource (already merged), as you requested.
    
    Can be applied directly to timers/clockevents-next.
    
    Best regards,
    Jonas

 drivers/clocksource/moxart_timer.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clocksource/moxart_timer.c b/drivers/clocksource/moxart_timer.c
index 08a5943..5eb2c35 100644
--- a/drivers/clocksource/moxart_timer.c
+++ b/drivers/clocksource/moxart_timer.c
@@ -20,6 +20,7 @@
 #include <linux/of_irq.h>
 #include <linux/io.h>
 #include <linux/clocksource.h>
+#include <linux/bitops.h>
 
 #define TIMER1_BASE		0x00
 #define TIMER2_BASE		0x10
-- 
1.8.2.1

^ permalink raw reply related	[flat|nested] 62+ messages in thread

end of thread, other threads:[~2013-07-26 14:03 UTC | newest]

Thread overview: 62+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-18 10:00 [PATCH] ARM: clocksource: add support for MOXA ART SoCs Jonas Jensen
2013-06-18 10:00 ` Jonas Jensen
2013-06-18 15:14 ` Thomas Petazzoni
2013-06-18 15:14   ` Thomas Petazzoni
2013-06-18 15:28 ` Arnd Bergmann
2013-06-18 15:28   ` Arnd Bergmann
2013-06-26 14:53 ` [PATCH v2] " Jonas Jensen
2013-06-26 14:53   ` Jonas Jensen
2013-06-26 14:59   ` Jonas Jensen
2013-06-26 14:59     ` Jonas Jensen
2013-06-26 16:10   ` Uwe Kleine-König
2013-06-26 16:10     ` Uwe Kleine-König
2013-06-26 19:15   ` Linus Walleij
2013-06-26 19:15     ` Linus Walleij
2013-06-27 11:23   ` [PATCH v3] " Jonas Jensen
2013-06-27 11:23     ` Jonas Jensen
2013-06-28 13:34     ` Thomas Gleixner
2013-06-28 13:34       ` Thomas Gleixner
2013-07-01 14:02     ` [PATCH v4] " Jonas Jensen
2013-07-01 14:02       ` Jonas Jensen
2013-07-01 17:55       ` Thomas Gleixner
2013-07-01 17:55         ` Thomas Gleixner
2013-07-02 20:19       ` Linus Walleij
2013-07-02 20:19         ` Linus Walleij
2013-07-04 12:19       ` [PATCH v5] " Jonas Jensen
2013-07-04 12:19         ` Jonas Jensen
2013-07-04 21:42         ` Thomas Gleixner
2013-07-04 21:42           ` Thomas Gleixner
2013-07-05 10:05           ` Jonas Jensen
2013-07-05 10:05             ` Jonas Jensen
2013-07-05 10:21             ` Thomas Gleixner
2013-07-05 10:21               ` Thomas Gleixner
2013-07-05 11:48               ` Jonas Jensen
2013-07-05 11:48                 ` Jonas Jensen
2013-07-05 10:04         ` [PATCH v6] " Jonas Jensen
2013-07-05 10:04           ` Jonas Jensen
2013-07-05 11:46           ` [PATCH v7] " Jonas Jensen
2013-07-05 11:46             ` Jonas Jensen
2013-07-16 13:52             ` Daniel Lezcano
2013-07-16 13:52               ` Daniel Lezcano
2013-07-16 14:44             ` [PATCH v8] " Jonas Jensen
2013-07-16 14:44               ` Jonas Jensen
2013-07-16 15:01               ` Daniel Lezcano
2013-07-16 15:01                 ` Daniel Lezcano
2013-07-17  8:14                 ` Jonas Jensen
2013-07-17  8:14                   ` Jonas Jensen
2013-07-17 12:13                   ` Daniel Lezcano
2013-07-17 12:13                     ` Daniel Lezcano
2013-07-17  8:04               ` [PATCH v9] " Jonas Jensen
2013-07-17  8:04                 ` Jonas Jensen
2013-07-19 11:12                 ` [PATCH] ARM: clocksource: moxart: documentation: update device tree bindings document Jonas Jensen
2013-07-19 11:12                   ` Jonas Jensen
2013-07-19 12:16                   ` Daniel Lezcano
2013-07-19 12:16                     ` Daniel Lezcano
2013-07-19 12:50                     ` Jonas Jensen
2013-07-19 12:50                       ` Jonas Jensen
2013-07-20 20:45                 ` [PATCH v9] ARM: clocksource: add support for MOXA ART SoCs Linus Walleij
2013-07-20 20:45                   ` Linus Walleij
2013-07-20 21:34                   ` Daniel Lezcano
2013-07-20 21:34                     ` Daniel Lezcano
2013-07-26 14:03                     ` [PATCH] ARM: clocksource: moxart: add bitops.h include Jonas Jensen
2013-07-26 14:03                       ` Jonas Jensen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.