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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
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
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 <[email protected]>
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
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
Hi,
I have attempted to rectify with changes from all the feedback I have received.
On 26 June 2013 16:53, Jonas Jensen <[email protected]> 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
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 <[email protected]>
> ---
> 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 <[email protected]>
> + *
> + * 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/ |
On Wed, Jun 26, 2013 at 4:53 PM, Jonas Jensen <[email protected]> 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 <[email protected]>
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
This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.
Signed-off-by: Jonas Jensen <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
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
This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.
Signed-off-by: Jonas Jensen <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
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
On Mon, Jul 1, 2013 at 4:02 PM, Jonas Jensen <[email protected]> wrote:
> This patch adds an clocksource driver for the main timer(s)
> found on MOXA ART SoCs.
>
> Signed-off-by: Jonas Jensen <[email protected]>
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 <[email protected]>
Yours,
Linus Walleij
This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.
Signed-off-by: Jonas Jensen <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
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 <[email protected]>
> ---
>
> 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
This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.
Signed-off-by: Jonas Jensen <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
On 4 July 2013 23:42, Thomas Gleixner <[email protected]> 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
On Fri, 5 Jul 2013, Jonas Jensen wrote:
> On 4 July 2013 23:42, Thomas Gleixner <[email protected]> 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
This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.
Signed-off-by: Jonas Jensen <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
Thanks for the replies Thomas.
On 5 July 2013 12:21, Thomas Gleixner <[email protected]> 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
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 <[email protected]>
> ---
>
> 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
This patch adds an clocksource driver for the main timer(s)
found on MOXA ART SoCs.
Signed-off-by: Jonas Jensen <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
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 <[email protected]>
> ---
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
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 <[email protected]>
---
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 <[email protected]>
+ *
+ * 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
On 16 July 2013 17:01, Daniel Lezcano <[email protected]> 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
On 07/17/2013 10:14 AM, Jonas Jensen wrote:
> On 16 July 2013 17:01, Daniel Lezcano <[email protected]> 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
Update device tree bindings document with the correct clock name.
Signed-off-by: Jonas Jensen <[email protected]>
---
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
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
On 19 July 2013 14:16, Daniel Lezcano <[email protected]> wrote:
> Ok, pushed.
Thanks :)
On Wed, Jul 17, 2013 at 10:04 AM, Jonas Jensen <[email protected]> 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 <[email protected]>
Yours,
Linus Walleij
On 07/20/2013 10:45 PM, Linus Walleij wrote:
> On Wed, Jul 17, 2013 at 10:04 AM, Jonas Jensen <[email protected]> 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
bitops.h included implicitly, add #include <linux/bitops.h>
Signed-off-by: Jonas Jensen <[email protected]>
---
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