| Message ID | 1368332581-94691-5-git-send-email-dt.tangr@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sun, May 12, 2013 at 6:22 AM, Daniel Tang <dt.tangr@gmail.com> wrote: > Signed-off-by: Daniel Tang <dt.tangr@gmail.com> A commit message does not hurt. > drivers/clocksource/Makefile | 1 + > drivers/clocksource/nspire-classic-timer.c | 199 +++++++++++++++++++++++++++++ This subsystem is managed by Thomas Gleixner and John Stultz so you should include them on the To: line in reviewes. > +++ b/drivers/clocksource/nspire-classic-timer.c (...) > +struct nspire_timer { > + void __iomem *base; > + void __iomem *timer1, *timer2; > + void __iomem *interrupt_regs; > + > + int irqnr; > + > + struct clk *clk; > + struct clock_event_device clkevt; > + struct irqaction clkevt_irq; > + > + char clocksource_name[64]; > + char clockevent_name[64]; > +}; > + > +static int nspire_timer_set_event(unsigned long delta, > + struct clock_event_device *dev) > +{ > + unsigned long flags; > + struct nspire_timer *timer = container_of(dev, > + struct nspire_timer, > + clkevt); > + > + local_irq_save(flags); > + > + writel(delta, timer->timer1 + IO_CURRENT_VAL); > + writel(CNTL_RUN_TIMER | CNTL_DEC | CNTL_MATCH1, > + timer->timer1 + IO_CONTROL); > + > + local_irq_restore(flags); > + > + return 0; > +} > +static void nspire_timer_set_mode(enum clock_event_mode mode, > + struct clock_event_device *evt) > +{ > + evt->mode = mode; So you support any mode? Atleast handle CLOCK_EVT_MODE_SHUTDOWN and CLOCK_EVT_UNUSED like any well-behaved clock event driver. > +} > + > +static irqreturn_t nspire_timer_interrupt(int irq, void *dev_id) > +{ > + struct nspire_timer *timer = dev_id; > + > + writel((1<<0), timer->interrupt_regs + IO_INTR_ACK); write(0x1, timer->interrupt_regs + IO_INTR_ACK); is no less intelligible, if you really want to point out that you're setting bit 0, include <linux/bitops.h> and use BIT(0). If there is a status bit you're clearing, maybe you should check it first to watch for spurious IRQs? > + writel(CNTL_STOP_TIMER, timer->timer1 + IO_CONTROL); > + > + if (timer->clkevt.event_handler) > + timer->clkevt.event_handler(&timer->clkevt); > + > + return IRQ_HANDLED; > +} > + > +static int __init nspire_timer_add(struct device_node *node) > +{ > + struct nspire_timer *timer; > + struct resource res; > + int ret; > + > + timer = kzalloc(sizeof(*timer), GFP_ATOMIC); > + if (!timer) > + return -ENOMEM; > + > + timer->base = of_iomap(node, 0); > + if (!timer->base) { > + ret = -EINVAL; > + goto error_free; > + } > + timer->timer1 = timer->base + IO_TIMER1; > + timer->timer2 = timer->base + IO_TIMER2; > + > + timer->clk = of_clk_get(node, 0); > + if (IS_ERR(timer->clk)) { > + ret = PTR_ERR(timer->clk); > + pr_err("Timer clock not found! (error %d)\n", ret); > + goto error_unmap; > + } > + > + timer->interrupt_regs = of_iomap(node, 1); Check for errors. > + timer->irqnr = irq_of_parse_and_map(node, 0); Check for errors. > + > + of_address_to_resource(node, 0, &res); > + scnprintf(timer->clocksource_name, sizeof(timer->clocksource_name), > + "%llx.%s_clocksource", > + (unsigned long long)res.start, node->name); > + > + scnprintf(timer->clockevent_name, sizeof(timer->clockevent_name), > + "%llx.%s_clockevent", > + (unsigned long long)res.start, node->name); > + > + if (timer->interrupt_regs && timer->irqnr) { > + timer->clkevt.name = timer->clockevent_name; > + timer->clkevt.set_next_event = nspire_timer_set_event; > + timer->clkevt.set_mode = nspire_timer_set_mode; > + timer->clkevt.rating = 200; > + timer->clkevt.shift = 32; Delete this shift. It will be set by clockevents_config_and_register(). > + timer->clkevt.cpumask = cpu_all_mask; > + timer->clkevt.features = > + CLOCK_EVT_FEAT_ONESHOT; > + > + writel(CNTL_STOP_TIMER, timer->timer1 + IO_CONTROL); > + writel(0, timer->timer1 + IO_DIVIDER); For a 16-bit counter I don't think it's wise to set the divider to 0 unless the clock frequency is very low. See further comments below. (Assumes this is some prescaler.) > + /* Mask out interrupts except the one we're using */ > + writel((1<<0), timer->interrupt_regs + IO_INTR_MSK); Just 0x1 or BIT(0). > + writel(0x3F, timer->interrupt_regs + IO_INTR_ACK); Hm magic number, I guess it's clearing all IRQ lines... > + > + /* Interrupt to occur when timer value matches 0 */ > + writel(0, timer->base + IO_MATCH1); > + > + timer->clkevt_irq.name = timer->clockevent_name; > + timer->clkevt_irq.handler = nspire_timer_interrupt; > + timer->clkevt_irq.dev_id = timer; > + timer->clkevt_irq.flags = > + IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL; > + > + setup_irq(timer->irqnr, &timer->clkevt_irq); > + > + clockevents_config_and_register(&timer->clkevt, > + clk_get_rate(timer->clk), 0x0001, 0xfffe); > + pr_info("Added %s as clockevent\n", timer->clockevent_name); > + } > + > + writel(CNTL_RUN_TIMER | CNTL_FOREVER | CNTL_INC, > + timer->timer2 + IO_CONTROL); > + > + clocksource_mmio_init(timer->timer2 + IO_CURRENT_VAL, > + timer->clocksource_name, > + clk_get_rate(timer->clk), > + 200, 16, > + clocksource_mmio_readw_up); If this timer is really just 16 bits, it's a *pretty* good idea to use the prescaler (I guess this is what IO_DIVIDER is) beacuse else you will get short sleep times with CONFIG_NO_HZ_IDLE on this system, and wake up unnecessarily often. The same goes for the clock event. See how we calculate the prescaler in arch/arm/mach-integrator/integrator_ap.c for example. Yours, Linus Walleij
On 14/05/2013, at 6:03 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >> + >> + timer->interrupt_regs = of_iomap(node, 1); > > Check for errors. > >> + timer->irqnr = irq_of_parse_and_map(node, 0); > > Check for errors. >> + if (timer->interrupt_regs && timer->irqnr) { >> The idea with this is to make these properties optional. Each timer has two sub-timers but only one can generate interrupts. One will be added as a clockevent device and the other as a clocksource. If an IRQ number or interrupt acknowledge address is not passed through the DT, then the clockevents device is not added but the clocksource still is. Should I comment it in the code to make it apparent what's happening or is there a better way to express this? > >> + writel(0x3F, timer->interrupt_regs + IO_INTR_ACK); > > Hm magic number, I guess it's clearing all IRQ lines… It's a bitmask of bits 0-5. > >> + >> + /* Interrupt to occur when timer value matches 0 */ >> + writel(0, timer->base + IO_MATCH1); >> + >> + timer->clkevt_irq.name = timer->clockevent_name; >> + timer->clkevt_irq.handler = nspire_timer_interrupt; >> + timer->clkevt_irq.dev_id = timer; >> + timer->clkevt_irq.flags = >> + IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL; >> + >> + setup_irq(timer->irqnr, &timer->clkevt_irq); >> + >> + clockevents_config_and_register(&timer->clkevt, >> + clk_get_rate(timer->clk), 0x0001, 0xfffe); >> + pr_info("Added %s as clockevent\n", timer->clockevent_name); >> + } >> + >> + writel(CNTL_RUN_TIMER | CNTL_FOREVER | CNTL_INC, >> + timer->timer2 + IO_CONTROL); >> + >> + clocksource_mmio_init(timer->timer2 + IO_CURRENT_VAL, >> + timer->clocksource_name, >> + clk_get_rate(timer->clk), >> + 200, 16, >> + clocksource_mmio_readw_up); > > If this timer is really just 16 bits, it's a *pretty* good idea to use > the prescaler (I guess this is what IO_DIVIDER is) beacuse else you > will get short sleep times with CONFIG_NO_HZ_IDLE on this system, > and wake up unnecessarily often. > > The same goes for the clock event. The clock frequency is 32768Hz. Should I be scaling it down at that frequency? > > See how we calculate the prescaler in arch/arm/mach-integrator/integrator_ap.c > for example. > > Yours, > Linus Walleij Cheers, Daniel Tang
On Tue, May 14, 2013 at 1:51 PM, Daniel Tang <dt.tangr@gmail.com> wrote: > On 14/05/2013, at 6:03 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > >>> + >>> + timer->interrupt_regs = of_iomap(node, 1); >> >> Check for errors. >> >>> + timer->irqnr = irq_of_parse_and_map(node, 0); >> >> Check for errors. >>> + if (timer->interrupt_regs && timer->irqnr) { >>> > > The idea with this is to make these properties optional. Each timer has two sub-timers > but only one can generate interrupts. I see now. Sorry, I was just being dumb, don't worry! > Should I comment it in the code to make it apparent what's happening or is there a > better way to express this? Maybe. Maybe it's obvious to everyone else but me :-/ >>> + writel(0x3F, timer->interrupt_regs + IO_INTR_ACK); >> >> Hm magic number, I guess it's clearing all IRQ lines… > > It's a bitmask of bits 0-5. Yeah I figured so much... So 6 IRQ lines that get cleared. >> If this timer is really just 16 bits, it's a *pretty* good idea to use >> the prescaler (I guess this is what IO_DIVIDER is) beacuse else you >> will get short sleep times with CONFIG_NO_HZ_IDLE on this system, >> and wake up unnecessarily often. >> >> The same goes for the clock event. > > The clock frequency is 32768Hz. Should I be scaling it down at that frequency? No. But that was *pretty* slow. The thing is that the driver is getting "some clockfrequency" from the clk subsystem and does not know what it is. Usually (and I've seen this hardware design pattern a lot) when you have a clock like this, and go talk to the clk implementation specialist s/he will say "oh, yeah, I can mux in this 34 MHz clock to the timer as well", and if that is possible then that is often what you want, because it's good for things like HRtimers, and then it's useful to have a prescaler och the clocksource/clockevent. You can use that 32768 Hz for clock events and clocksource (but I suspect that HRtimers will never be useful on this platform due to this). Yours, Linus Walleij
On 17/05/2013, at 11:17 PM, Linus Walleij <linus.walleij@linaro.org> wrote: >>> If this timer is really just 16 bits, it's a *pretty* good idea to use >>> the prescaler (I guess this is what IO_DIVIDER is) beacuse else you >>> will get short sleep times with CONFIG_NO_HZ_IDLE on this system, >>> and wake up unnecessarily often. >>> >>> The same goes for the clock event. >> >> The clock frequency is 32768Hz. Should I be scaling it down at that frequency? > > No. But that was *pretty* slow. > > The thing is that the driver is getting "some clockfrequency" from > the clk subsystem and does not know what it is. > > Usually (and I've seen this hardware design pattern a lot) when you > have a clock like this, and go talk to the clk implementation specialist > s/he will say "oh, yeah, I can mux in this 34 MHz clock to the timer > as well", and if that is possible then that is often what you want, > because it's good for things like HRtimers, and then it's useful to have > a prescaler och the clocksource/clockevent. > > You can use that 32768 Hz for clock events and clocksource (but I > suspect that HRtimers will never be useful on this platform due to this). > I forgot to mention before that there is also another timer that runs at the same freq as the APB (which is usually around 30MHz). Is this what you were alluding to? I haven't tested the driver on that timer yet. As I understand it, you're saying to use a prescaler on faster clock rates so the kernel can have a longer maximum delay time (since a 16 bit register can only hold only so much). In that case, what kind of range should I be scaling to? I suspect it needs to be large enough to keep the kernel from constantly waking but small enough for HR timers to be useful. > Yours, > Linus Walleij
On Sat, May 18, 2013 at 8:40 AM, Daniel Tang <dt.tangr@gmail.com> wrote: > On 17/05/2013, at 11:17 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > >>>> If this timer is really just 16 bits, it's a *pretty* good idea to use >>>> the prescaler (I guess this is what IO_DIVIDER is) beacuse else you >>>> will get short sleep times with CONFIG_NO_HZ_IDLE on this system, >>>> and wake up unnecessarily often. >>>> >>>> The same goes for the clock event. >>> >>> The clock frequency is 32768Hz. Should I be scaling it down at that frequency? >> >> No. But that was *pretty* slow. >> >> The thing is that the driver is getting "some clockfrequency" from >> the clk subsystem and does not know what it is. >> >> Usually (and I've seen this hardware design pattern a lot) when you >> have a clock like this, and go talk to the clk implementation specialist >> s/he will say "oh, yeah, I can mux in this 34 MHz clock to the timer >> as well", and if that is possible then that is often what you want, >> because it's good for things like HRtimers, and then it's useful to have >> a prescaler och the clocksource/clockevent. >> >> You can use that 32768 Hz for clock events and clocksource (but I >> suspect that HRtimers will never be useful on this platform due to this). >> > > I forgot to mention before that there is also another timer that runs > at the same freq as the APB (which is usually around 30MHz). > Is this what you were alluding to? I haven't tested the driver on that timer yet. OK then that is probably the timer you will use for clock event and also sched_clock() in the end. (You'll be setting a better rating on it.) I'm not alluding to it as I didn't know of it, but it makes HW-sense that it exists :-) > As I understand it, you're saying to use a prescaler on faster clock rates > so the kernel can have a longer maximum delay time (since a 16 bit register > can only hold only so much). In that case, what kind of range should I be > scaling to? I suspect it needs to be large enough to keep the kernel from > constantly waking but small enough for HR timers to be useful. Yeah :-/ This is one of the cases where one thing excludes the other. But if it's hard-wired to 32kHz you probably want to leave it as it is. Yours, Linus Walleij
diff --git a/drivers/clocksource/Makefile b/drivers/clocksource/Makefile index 8d979c7..b9b56cb 100644 --- a/drivers/clocksource/Makefile +++ b/drivers/clocksource/Makefile @@ -22,6 +22,7 @@ obj-$(CONFIG_ARCH_PRIMA2) += timer-prima2.o obj-$(CONFIG_SUN4I_TIMER) += sun4i_timer.o obj-$(CONFIG_ARCH_TEGRA) += tegra20_timer.o obj-$(CONFIG_VT8500_TIMER) += vt8500_timer.o +obj-$(CONFIG_ARCH_NSPIRE) += nspire-classic-timer.o obj-$(CONFIG_ARCH_BCM) += bcm_kona_timer.o obj-$(CONFIG_CADENCE_TTC_TIMER) += cadence_ttc_timer.o obj-$(CONFIG_CLKSRC_EXYNOS_MCT) += exynos_mct.o diff --git a/drivers/clocksource/nspire-classic-timer.c b/drivers/clocksource/nspire-classic-timer.c new file mode 100644 index 0000000..4b92b61 --- /dev/null +++ b/drivers/clocksource/nspire-classic-timer.c @@ -0,0 +1,199 @@ +/* + * linux/drivers/clocksource/nspire-classic-timer.c + * + * Copyright (C) 2013 Daniel Tang <tangrs@tangrs.id.au> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2, as + * published by the Free Software Foundation. + * + */ + +#include <linux/io.h> +#include <linux/irq.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/clk.h> +#include <linux/clockchips.h> +#include <linux/cpumask.h> +#include <linux/interrupt.h> +#include <linux/slab.h> + +#define DT_COMPAT "nspire-classic-timer" + +#define IO_CURRENT_VAL 0x00 +#define IO_DIVIDER 0x04 +#define IO_CONTROL 0x08 + +#define IO_TIMER1 0x00 +#define IO_TIMER2 0x0C + +#define IO_MATCH1 0x18 +#define IO_MATCH2 0x1C +#define IO_MATCH3 0x20 +#define IO_MATCH4 0x24 +#define IO_MATCH5 0x28 +#define IO_MATCH6 0x2C + +#define IO_INTR_STS 0x00 +#define IO_INTR_ACK 0x00 +#define IO_INTR_MSK 0x04 + +#define CNTL_STOP_TIMER (1<<4) +#define CNTL_RUN_TIMER (0<<4) + +#define CNTL_INC (1<<3) +#define CNTL_DEC (0<<3) + +#define CNTL_TOZERO 0 +#define CNTL_MATCH1 1 +#define CNTL_MATCH2 2 +#define CNTL_MATCH3 3 +#define CNTL_MATCH4 4 +#define CNTL_MATCH5 5 +#define CNTL_MATCH6 6 +#define CNTL_FOREVER 7 + +struct nspire_timer { + void __iomem *base; + void __iomem *timer1, *timer2; + void __iomem *interrupt_regs; + + int irqnr; + + struct clk *clk; + struct clock_event_device clkevt; + struct irqaction clkevt_irq; + + char clocksource_name[64]; + char clockevent_name[64]; +}; + +static int nspire_timer_set_event(unsigned long delta, + struct clock_event_device *dev) +{ + unsigned long flags; + struct nspire_timer *timer = container_of(dev, + struct nspire_timer, + clkevt); + + local_irq_save(flags); + + writel(delta, timer->timer1 + IO_CURRENT_VAL); + writel(CNTL_RUN_TIMER | CNTL_DEC | CNTL_MATCH1, + timer->timer1 + IO_CONTROL); + + local_irq_restore(flags); + + return 0; +} +static void nspire_timer_set_mode(enum clock_event_mode mode, + struct clock_event_device *evt) +{ + evt->mode = mode; +} + +static irqreturn_t nspire_timer_interrupt(int irq, void *dev_id) +{ + struct nspire_timer *timer = dev_id; + + writel((1<<0), timer->interrupt_regs + IO_INTR_ACK); + writel(CNTL_STOP_TIMER, timer->timer1 + IO_CONTROL); + + if (timer->clkevt.event_handler) + timer->clkevt.event_handler(&timer->clkevt); + + return IRQ_HANDLED; +} + +static int __init nspire_timer_add(struct device_node *node) +{ + struct nspire_timer *timer; + struct resource res; + int ret; + + timer = kzalloc(sizeof(*timer), GFP_ATOMIC); + if (!timer) + return -ENOMEM; + + timer->base = of_iomap(node, 0); + if (!timer->base) { + ret = -EINVAL; + goto error_free; + } + timer->timer1 = timer->base + IO_TIMER1; + timer->timer2 = timer->base + IO_TIMER2; + + timer->clk = of_clk_get(node, 0); + if (IS_ERR(timer->clk)) { + ret = PTR_ERR(timer->clk); + pr_err("Timer clock not found! (error %d)\n", ret); + goto error_unmap; + } + + timer->interrupt_regs = of_iomap(node, 1); + timer->irqnr = irq_of_parse_and_map(node, 0); + + of_address_to_resource(node, 0, &res); + scnprintf(timer->clocksource_name, sizeof(timer->clocksource_name), + "%llx.%s_clocksource", + (unsigned long long)res.start, node->name); + + scnprintf(timer->clockevent_name, sizeof(timer->clockevent_name), + "%llx.%s_clockevent", + (unsigned long long)res.start, node->name); + + if (timer->interrupt_regs && timer->irqnr) { + timer->clkevt.name = timer->clockevent_name; + timer->clkevt.set_next_event = nspire_timer_set_event; + timer->clkevt.set_mode = nspire_timer_set_mode; + timer->clkevt.rating = 200; + timer->clkevt.shift = 32; + timer->clkevt.cpumask = cpu_all_mask; + timer->clkevt.features = + CLOCK_EVT_FEAT_ONESHOT; + + writel(CNTL_STOP_TIMER, timer->timer1 + IO_CONTROL); + writel(0, timer->timer1 + IO_DIVIDER); + + /* Mask out interrupts except the one we're using */ + writel((1<<0), timer->interrupt_regs + IO_INTR_MSK); + writel(0x3F, timer->interrupt_regs + IO_INTR_ACK); + + /* Interrupt to occur when timer value matches 0 */ + writel(0, timer->base + IO_MATCH1); + + timer->clkevt_irq.name = timer->clockevent_name; + timer->clkevt_irq.handler = nspire_timer_interrupt; + timer->clkevt_irq.dev_id = timer; + timer->clkevt_irq.flags = + IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL; + + setup_irq(timer->irqnr, &timer->clkevt_irq); + + clockevents_config_and_register(&timer->clkevt, + clk_get_rate(timer->clk), 0x0001, 0xfffe); + pr_info("Added %s as clockevent\n", timer->clockevent_name); + } + + writel(CNTL_RUN_TIMER | CNTL_FOREVER | CNTL_INC, + timer->timer2 + IO_CONTROL); + + clocksource_mmio_init(timer->timer2 + IO_CURRENT_VAL, + timer->clocksource_name, + clk_get_rate(timer->clk), + 200, 16, + clocksource_mmio_readw_up); + + pr_info("Added %s as clocksource\n", timer->clocksource_name); + + return 0; +error_unmap: + iounmap(timer->base); +error_free: + kfree(timer); + return ret; +} + +CLOCKSOURCE_OF_DECLARE(nspire_classic_timer, DT_COMPAT, nspire_timer_add);
Signed-off-by: Daniel Tang <dt.tangr@gmail.com> --- drivers/clocksource/Makefile | 1 + drivers/clocksource/nspire-classic-timer.c | 199 +++++++++++++++++++++++++++++ 2 files changed, 200 insertions(+) create mode 100644 drivers/clocksource/nspire-classic-timer.c