| Message ID | 1372940383-5957-1-git-send-email-jonas.jensen@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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);
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