Clocksource and clockevent device based on the Atmel TC blocks.
The clockevent device handles both periodic and oneshot modes, so this
enables NO_HZ and high res timers on some platforms that previously
couldn't use those mechanisms.
This works on both AVR32 and AT91 chips, given relevant patches for
tclib support (always) and clockevents (or else this will only look
like a higher precision clocksource). It's an updated and modularized
version of an AT91-only patch that has circulated for some time now.
Signed-off-by: David Brownell <[email protected]>
---
As with the preceding patch, this depends on platform updates that
won't merge before 2.6.26 ... in this case, updates to switch over
to clocksources (at91sam9 chips, but not at91rm9200) and clockevents
(avr32 and at91sam9), on top of platform_device setup.
drivers/clocksource/Makefile | 1
drivers/clocksource/tcb_clksrc.c | 303 +++++++++++++++++++++++++++++++++++++++
drivers/misc/Kconfig | 25 +++
3 files changed, 329 insertions(+)
--- a/drivers/clocksource/Makefile
+++ b/drivers/clocksource/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_ATMEL_TCB_CLKSRC) += tcb_clksrc.o
obj-$(CONFIG_X86_CYCLONE_TIMER) += cyclone.o
obj-$(CONFIG_X86_PM_TIMER) += acpi_pm.o
obj-$(CONFIG_SCx200HR_TIMER) += scx200_hrt.o
--- /dev/null
+++ b/drivers/clocksource/tcb_clksrc.c
@@ -0,0 +1,303 @@
+#include <linux/init.h>
+#include <linux/clocksource.h>
+#include <linux/clockchips.h>
+#include <linux/interrupt.h>
+#include <linux/irq.h>
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/atmel_tc.h>
+
+
+/*
+ * We're configured to use a specific TC block, one that's not hooked
+ * up to external hardware, to provide a time solution:
+ *
+ * - Two channels combine to create a free-running 32 bit counter
+ * with a base rate of 5+ MHz, packaged as a clocksource (with
+ * resolution better than 200 nsec).
+ *
+ * - The third channel may be used to provide a 16-bit clockevent
+ * source, used in either periodic or oneshot mode. This runs
+ * at 32 KiHZ, and can handle delays of up to two seconds.
+ *
+ * A boot clocksource and clockevent source are also currently needed,
+ * unless the relevant platforms (ARM/AT91, AVR32/AT32) are changed so
+ * this code can be used when init_timers() is called, well before most
+ * devices are set up. (Some low end AT91 parts, which can run uClinux,
+ * have only the timers in one TC block... they currently don't support
+ * the tclib code, because of that initialization issue.)
+ *
+ * REVISIT behavior during system suspend states... we should disable
+ * all clocks and save the power. Easily done for clockevent devices,
+ * but clocksources won't necessarily get the needed notifications.
+ * For deeper system sleep states, this will be mandatory...
+ */
+
+static void __iomem *tcaddr;
+
+static cycle_t tc_get_cycles(void)
+{
+ unsigned long flags;
+ u32 lower, upper;
+
+ raw_local_irq_save(flags);
+retry:
+ upper = __raw_readl(tcaddr + ATMEL_TC_REG(1, CV));
+ lower = __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
+ if (upper != __raw_readl(tcaddr + ATMEL_TC_REG(1, CV)))
+ goto retry;
+
+ raw_local_irq_restore(flags);
+ return (upper << 16) | lower;
+}
+
+static struct clocksource clksrc = {
+ .name = "tcb_clksrc",
+ .rating = 200,
+ .read = tc_get_cycles,
+ .mask = CLOCKSOURCE_MASK(32),
+ .shift = 18,
+ .flags = CLOCK_SOURCE_IS_CONTINUOUS,
+};
+
+#ifdef CONFIG_GENERIC_CLOCKEVENTS
+#define USE_TC_CLKEVT
+#endif
+
+#ifdef CONFIG_ARCH_AT91RM9200
+/* The AT91rm9200 system timer is a oneshot-capable 32k timer that's
+ * always running ... configuring a TC channel to work the same way
+ * would just waste some power.
+ */
+#undef USE_TC_CLKEVT
+#endif
+
+#ifdef USE_TC_CLKEVT
+
+/* For now, we always use the 32K clock ... this optimizes for NO_HZ,
+ * because using one of the divided clocks would usually mean the
+ * tick rate can never be less than several dozen Hz (vs 0.5 Hz).
+ *
+ * A divided clock could be good for high resolution timers, since
+ * 30.5 usec resolution can seem "low".
+ */
+static u32 timer_clock;
+
+static void tc_mode(enum clock_event_mode m, struct clock_event_device *d)
+{
+ __raw_writel(0xff, tcaddr + ATMEL_TC_REG(2, IDR));
+ __raw_writel(ATMEL_TC_CLKDIS, tcaddr + ATMEL_TC_REG(2, CCR));
+
+ switch (m) {
+
+ /* By not making the gentime core emulate periodic mode on top
+ * of oneshot, we get lower overhead and improved accuracy.
+ */
+ case CLOCK_EVT_MODE_PERIODIC:
+ /* slow clock, count up to RC, then irq and restart */
+ __raw_writel(timer_clock
+ | ATMEL_TC_WAVE | ATMEL_TC_WAVESEL_UP_AUTO,
+ tcaddr + ATMEL_TC_REG(2, CMR));
+ __raw_writel((32768 + HZ/2) / HZ, tcaddr + ATMEL_TC_REG(2, RC));
+
+ /* Enable clock and interrupts on RC compare */
+ __raw_writel(ATMEL_TC_CPCS, tcaddr + ATMEL_TC_REG(2, IER));
+
+ /* go go gadget! */
+ __raw_writel(ATMEL_TC_CLKEN | ATMEL_TC_SWTRG,
+ tcaddr + ATMEL_TC_REG(2, CCR));
+ break;
+
+ case CLOCK_EVT_MODE_ONESHOT:
+ /* slow clock, count up to RC, then irq and stop */
+ __raw_writel(timer_clock | ATMEL_TC_CPCSTOP
+ | ATMEL_TC_WAVE | ATMEL_TC_WAVESEL_UP_AUTO,
+ tcaddr + ATMEL_TC_REG(2, CMR));
+ __raw_writel(ATMEL_TC_CPCS, tcaddr + ATMEL_TC_REG(2, IER));
+
+ /* set_next_event() configures and starts the timer */
+ break;
+
+ default:
+ break;
+ }
+}
+
+static int tc_next_event(unsigned long delta, struct clock_event_device *d)
+{
+ __raw_writel(delta, tcaddr + ATMEL_TC_REG(2, RC));
+
+ /* go go gadget! */
+ __raw_writel(ATMEL_TC_CLKEN | ATMEL_TC_SWTRG,
+ tcaddr + ATMEL_TC_REG(2, CCR));
+ return 0;
+}
+
+static struct clock_event_device clkevt = {
+ .name = "tc_clkevt",
+ .features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT,
+ .shift = 32,
+ .rating = 125,
+ .cpumask = CPU_MASK_CPU0,
+ .set_next_event = tc_next_event,
+ .set_mode = tc_mode,
+};
+
+static irqreturn_t ch2_irq(int irq, void *handle)
+{
+ unsigned int sr = __raw_readl(tcaddr + ATMEL_TC_REG(2, SR));
+
+ if (sr & ATMEL_TC_CPCS) {
+ clkevt.event_handler(&clkevt);
+ return IRQ_HANDLED;
+ }
+
+ return IRQ_NONE;
+}
+
+static struct irqaction tc_irqaction = {
+ .name = "tc_clkevt",
+ .flags = IRQF_TIMER | IRQF_DISABLED,
+ .handler = ch2_irq,
+};
+
+static void __init setup_clkevents(struct platform_device *pdev,
+ struct clk *t0_clk, int clk32k_divisor_idx)
+{
+ struct clk *t2_clk = clk_get(&pdev->dev, "t2_clk");
+ int irq;
+
+ /* SOCs have either one IRQ and one clock for the whole
+ * TC block; or one each per channel. We use channel 2.
+ */
+ if (IS_ERR(t2_clk)) {
+ t2_clk = t0_clk;
+ irq = platform_get_irq(pdev, 0);
+ } else {
+ irq = platform_get_irq(pdev, 2);
+ clk_enable(t2_clk);
+ }
+
+ timer_clock = clk32k_divisor_idx;
+
+ clkevt.mult = div_sc(32768, NSEC_PER_SEC, clkevt.shift);
+ clkevt.max_delta_ns = clockevent_delta2ns(0xffff, &clkevt);
+ clkevt.min_delta_ns = clockevent_delta2ns(1, &clkevt) + 1;
+
+ setup_irq(irq, &tc_irqaction);
+
+ clockevents_register_device(&clkevt);
+}
+
+#else /* !USE_TC_CLKEVT */
+
+static void __init setup_clkevents(struct platform_device *pdev,
+ struct clk *t0_clk, int clk32k_divisor_idx)
+{
+ /* NOTHING */
+}
+
+#endif
+
+static int __init tcb_clksrc_init(void)
+{
+ static char bootinfo[] __initdata
+ = KERN_DEBUG "%s: tc%d at %d.%03d MHz\n";
+
+ struct platform_device *pdev;
+ struct clk *t0_clk, *t1_clk;
+ u32 rate, divided_rate = 0;
+ int best_divisor_idx = -1;
+ int clk32k_divisor_idx = -1;
+ int i;
+ struct resource *iomem;
+
+ pdev = atmel_tc_alloc(CONFIG_ATMEL_TCB_CLKSRC_BLOCK, &iomem);
+ if (pdev == NULL) {
+ pr_debug("can't alloc TC for clocksource\n");
+ return -ENODEV;
+ }
+ rename_region(iomem, clksrc.name);
+ tcaddr = ioremap(iomem->start, 256);
+
+ t0_clk = clk_get(&pdev->dev, "t0_clk");
+ if (IS_ERR(t0_clk)) {
+ atmel_tc_free(pdev);
+ return PTR_ERR(t0_clk);
+ }
+ clk_enable(t0_clk);
+
+ /* How fast will we be counting? Pick something over 5 MHz. */
+ rate = (u32) clk_get_rate(t0_clk);
+ for (i = 0; i < 5; i++) {
+ unsigned divisor = atmel_tc_divisors[i];
+ unsigned tmp;
+
+ /* remember 32 KiHz clock for later */
+ if (!divisor) {
+ clk32k_divisor_idx = i;
+ continue;
+ }
+
+ tmp = rate / divisor;
+ pr_debug("TC: %u / %-3u [%d] --> %u\n", rate, divisor, i, tmp);
+ if (best_divisor_idx > 0) {
+ if (tmp < 5 * 1000 * 1000)
+ continue;
+ }
+ divided_rate = tmp;
+ best_divisor_idx = i;
+ }
+
+ clksrc.mult = clocksource_hz2mult(divided_rate, clksrc.shift);
+
+ printk(bootinfo, clksrc.name, CONFIG_ATMEL_TCB_CLKSRC_BLOCK,
+ divided_rate / 1000000,
+ ((divided_rate + 500000) % 1000000) / 1000);
+
+ /* Some platforms (like at91rm9200, at91sam9260, etc) have one clock
+ * (and IRQ) per timer. Others (at91sam9263, ap700x, etc) don't.
+ */
+ t1_clk = clk_get(&pdev->dev, "t1_clk");
+ if (IS_ERR(t1_clk))
+ t1_clk = t0_clk;
+ else
+ clk_enable(t1_clk);
+
+ /* channel 0: waveform mode, input mclk/8, clock TIOA0 on overflow */
+ __raw_writel(best_divisor_idx /* likely divide-by-8 */
+ | ATMEL_TC_WAVE
+ | ATMEL_TC_WAVESEL_UP /* free-run */
+ | ATMEL_TC_ACPA_SET /* TIOA0 rises at 0 */
+ | ATMEL_TC_ACPC_CLEAR, /* (duty cycle 50%) */
+ tcaddr + ATMEL_TC_REG(0, CMR));
+ __raw_writel(0x0000, tcaddr + ATMEL_TC_REG(0, RA));
+ __raw_writel(0x8000, tcaddr + ATMEL_TC_REG(0, RC));
+ __raw_writel(0xff, tcaddr + ATMEL_TC_REG(0, IDR)); /* no irqs */
+ __raw_writel(ATMEL_TC_CLKEN, tcaddr + ATMEL_TC_REG(0, CCR));
+
+ /* channel 1: waveform mode, input TIOA0 */
+ __raw_writel(ATMEL_TC_XC1 /* input: TIOA0 */
+ | ATMEL_TC_WAVE
+ | ATMEL_TC_WAVESEL_UP, /* free-run */
+ tcaddr + ATMEL_TC_REG(1, CMR));
+ __raw_writel(0xff, tcaddr + ATMEL_TC_REG(1, IDR)); /* no irqs */
+ __raw_writel(ATMEL_TC_CLKEN, tcaddr + ATMEL_TC_REG(1, CCR));
+
+ /* chain channel 0 to channel 1, then reset all the timers */
+ __raw_writel(ATMEL_TC_TC1XC1S_TIOA0, tcaddr + ATMEL_TC_BMR);
+ __raw_writel(ATMEL_TC_SYNC, tcaddr + ATMEL_TC_BCR);
+
+ /* and away we go! */
+ clocksource_register(&clksrc);
+
+ /* channel 2: periodic and oneshot timer support */
+ setup_clkevents(pdev, t0_clk, clk32k_divisor_idx);
+
+ return 0;
+}
+arch_initcall(tcb_clksrc_init);
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -30,6 +30,31 @@ config ATMEL_TCLIB
blocks found on many Atmel processors. This facilitates using
these blocks by different drivers despite processor differences.
+config ATMEL_TCB_CLKSRC
+ bool "TC Block Clocksource"
+ depends on ATMEL_TCLIB && GENERIC_TIME
+ default y
+ help
+ Select this to get a high precision clocksource based on a
+ TC block with a 5+ MHz base clock rate. Two timer channels
+ are combined to make a single 32-bit timer.
+
+ When GENERIC_CLOCKEVENTS is defined, the third timer channel
+ may be used as a clock event device supporting oneshot mode
+ (delays of up to two seconds) based on the 32 KiHz clock.
+
+config ATMEL_TCB_CLKSRC_BLOCK
+ int
+ depends on ATMEL_TCB_CLKSRC
+ prompt "TC Block" if ARCH_AT91RM9200 || ARCH_AT91SAM9260 || CPU_AT32AP700X
+ default 0
+ range 0 1
+ help
+ Some chips provide more than one TC block, so you have the
+ choice of which one to use for the clock framework. The other
+ TC can be used for other purposes, such as PWM generation and
+ interval timing.
+
config IBM_ASM
tristate "Device driver for IBM RSA service processor"
depends on X86 && PCI && INPUT && EXPERIMENTAL
Again, sorry for the delay...it really sucks that I haven't been able
to look at this stuff closely until now. Hopefully a late review is
better than no review.
On Fri, 22 Feb 2008 17:28:37 -0800
David Brownell <[email protected]> wrote:
> +static cycle_t tc_get_cycles(void)
> +{
> + unsigned long flags;
> + u32 lower, upper;
> +
> + raw_local_irq_save(flags);
Why do you need to use the raw version?
> +retry:
> + upper = __raw_readl(tcaddr + ATMEL_TC_REG(1, CV));
> + lower = __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
> + if (upper != __raw_readl(tcaddr + ATMEL_TC_REG(1, CV)))
> + goto retry;
Did you just open-code a do/while loop using goto? ;-)
> +#ifdef CONFIG_GENERIC_CLOCKEVENTS
> +#define USE_TC_CLKEVT
> +#endif
> +
> +#ifdef CONFIG_ARCH_AT91RM9200
> +/* The AT91rm9200 system timer is a oneshot-capable 32k timer that's
> + * always running ... configuring a TC channel to work the same way
> + * would just waste some power.
> + */
> +#undef USE_TC_CLKEVT
> +#endif
> +
> +#ifdef USE_TC_CLKEVT
Can't you just use #ifndef CONFIG_ARCH_AT91RM9200 and avoid the whole
ifdef/define/undef dance above?
> + case CLOCK_EVT_MODE_ONESHOT:
> + /* slow clock, count up to RC, then irq and stop */
Hmm. Do you really want to stop it? Won't you get better accuracy if
you let it run and program the next event as (prev_event + delta)?
> +static struct irqaction tc_irqaction = {
> + .name = "tc_clkevt",
> + .flags = IRQF_TIMER | IRQF_DISABLED,
> + .handler = ch2_irq,
> +};
I don't think you need to define this statically. You can call
request_irq() at arch_initcall() time.
> +static void __init setup_clkevents(struct platform_device *pdev,
> + struct clk *t0_clk, int clk32k_divisor_idx)
> +{
> + struct clk *t2_clk = clk_get(&pdev->dev, "t2_clk");
> + int irq;
> +
> + /* SOCs have either one IRQ and one clock for the whole
> + * TC block; or one each per channel. We use channel 2.
> + */
> + if (IS_ERR(t2_clk)) {
> + t2_clk = t0_clk;
> + irq = platform_get_irq(pdev, 0);
> + } else {
> + irq = platform_get_irq(pdev, 2);
> + clk_enable(t2_clk);
> + }
I don't think it is safe to assume that one clock per channel always
means one irq per channel as well...
What's wrong with
irq = platform_get_irq(pdev, 2);
if (irq < 0)
irq = platform_get_irq(pdev, 0);
?
> +config ATMEL_TCB_CLKSRC
> + bool "TC Block Clocksource"
> + depends on ATMEL_TCLIB && GENERIC_TIME
> + default y
> + help
> + Select this to get a high precision clocksource based on a
> + TC block with a 5+ MHz base clock rate. Two timer channels
> + are combined to make a single 32-bit timer.
> +
> + When GENERIC_CLOCKEVENTS is defined, the third timer channel
> + may be used as a clock event device supporting oneshot mode
> + (delays of up to two seconds) based on the 32 KiHz clock.
> +
> +config ATMEL_TCB_CLKSRC_BLOCK
> + int
> + depends on ATMEL_TCB_CLKSRC
> + prompt "TC Block" if ARCH_AT91RM9200 || ARCH_AT91SAM9260 || CPU_AT32AP700X
> + default 0
> + range 0 1
Hmm...I don't like the direction this is going...
How about we make tcb_clksrc_init() global and call it from the
platform code with whatever TCB the platform thinks is appropriate?
Perhaps remove the prompt from ATMEL_TCB_CLKSRC and have the platform
select it as well? I certainly want to force this stuff on on the
AP7000...otherwise we'll just get lots of complaints that the thing is
using 4x more power than it's supposed to...
Haavard
> Again, sorry for the delay...it really sucks that I haven't been able
> to look at this stuff closely until now. Hopefully a late review is
> better than no review.
Yes. The review I've gotten so far has been of the "it works for me,
thanks" variety.
(The only issue reported to me is that NO_HZ on AT91 seems to make the
DBGU lose characters sometimes ... that one-byte "fifo" makes trouble
whenever there's the slightest delay in IRQ handling, and NO_HZ can
easily add such delays as part of ensuring that "jiffies" is current
before invoking handlers.)
> On Fri, 22 Feb 2008 17:28:37 -0800
> David Brownell <[email protected]> wrote:
>
> > +static cycle_t tc_get_cycles(void)
> > +{
> > + unsigned long flags;
> > + u32 lower, upper;
> > +
> > + raw_local_irq_save(flags);
>
> Why do you need to use the raw version?
This is part of the system timer code, and it should never be a
preemption point. Plus I didn't want to waste any instruction
cycles in code that runs before e.g. the DBGU IRQ handler would
get called... observably, such extra cycles *do* hurt.
> > +retry:
> > + upper = __raw_readl(tcaddr + ATMEL_TC_REG(1, CV));
> > + lower = __raw_readl(tcaddr + ATMEL_TC_REG(0, CV));
> > + if (upper != __raw_readl(tcaddr + ATMEL_TC_REG(1, CV)))
> > + goto retry;
>
> Did you just open-code a do/while loop using goto? ;-)
I take that back about late review being better than none. ;)
> > +#ifdef CONFIG_GENERIC_CLOCKEVENTS
> > +#define USE_TC_CLKEVT
> > +#endif
> > +
> > +#ifdef CONFIG_ARCH_AT91RM9200
> > +/* The AT91rm9200 system timer is a oneshot-capable 32k timer that's
> > + * always running ... configuring a TC channel to work the same way
> > + * would just waste some power.
> > + */
> > +#undef USE_TC_CLKEVT
> > +#endif
> > +
> > +#ifdef USE_TC_CLKEVT
>
> Can't you just use #ifndef CONFIG_ARCH_AT91RM9200 and avoid the whole
> ifdef/define/undef dance above?
I can't know that some other platform won't have a better system
timer solution, and didn't want to assume that only that single
venerable chip had such a solution ... it's kind of puzzling to
me (software guy) that none of the newest Atmel SOCs have better
timer infrastructure than they do. ;)
> > + case CLOCK_EVT_MODE_ONESHOT:
> > + /* slow clock, count up to RC, then irq and stop */
>
> Hmm. Do you really want to stop it? Won't you get better accuracy if
> you let it run and program the next event as (prev_event + delta)?
No, ONESHOT means "stop after the IRQ I asked for". And when
tc_next_event() is asked to trigger that IRQ, it's relative to
the instant it's requested, not relative to the last one that
was requested (which may not have triggered yet, or may have
been quite some time ago).
> > +static struct irqaction tc_irqaction = {
> > + .name = "tc_clkevt",
> > + .flags = IRQF_TIMER | IRQF_DISABLED,
> > + .handler = ch2_irq,
> > +};
>
> I don't think you need to define this statically. You can call
> request_irq() at arch_initcall() time.
That could be done, yes ... and using request_irq()/free_irq()
would also let this be linked as a module, not just statically.
On the other hand, doing it this way doesn't hurt either does it?
Unless a modular build is important.
> > +static void __init setup_clkevents(struct platform_device *pdev,
> > + struct clk *t0_clk, int clk32k_divisor_idx)
> > +{
> > + struct clk *t2_clk = clk_get(&pdev->dev, "t2_clk");
> > + int irq;
> > +
> > + /* SOCs have either one IRQ and one clock for the whole
> > + * TC block; or one each per channel. We use channel 2.
> > + */
> > + if (IS_ERR(t2_clk)) {
> > + t2_clk = t0_clk;
> > + irq = platform_get_irq(pdev, 0);
> > + } else {
> > + irq = platform_get_irq(pdev, 2);
> > + clk_enable(t2_clk);
> > + }
>
> I don't think it is safe to assume that one clock per channel always
> means one irq per channel as well...
On current chips, that's how it works.
> What's wrong with
>
> irq = platform_get_irq(pdev, 2);
> if (irq < 0)
> irq = platform_get_irq(pdev, 0);
Nothing much, except that I took the more concise path. Got patch?
> > +config ATMEL_TCB_CLKSRC
> > + bool "TC Block Clocksource"
> > + depends on ATMEL_TCLIB && GENERIC_TIME
> > + default y
> > + help
> > + Select this to get a high precision clocksource based on a
> > + TC block with a 5+ MHz base clock rate. Two timer channels
> > + are combined to make a single 32-bit timer.
> > +
> > + When GENERIC_CLOCKEVENTS is defined, the third timer channel
> > + may be used as a clock event device supporting oneshot mode
> > + (delays of up to two seconds) based on the 32 KiHz clock.
> > +
> > +config ATMEL_TCB_CLKSRC_BLOCK
> > + int
> > + depends on ATMEL_TCB_CLKSRC
> > + prompt "TC Block" if ARCH_AT91RM9200 || ARCH_AT91SAM9260 || CPU_AT32AP700X
> > + default 0
> > + range 0 1
>
> Hmm...I don't like the direction this is going...
>
> How about we make tcb_clksrc_init() global and call it from the
> platform code with whatever TCB the platform thinks is appropriate?
That could work too, but if it goes that way then there's no
point in changes to support a modular build (e.g. the irqaction
thing you noted above).
> Perhaps remove the prompt from ATMEL_TCB_CLKSRC and have the platform
> select it as well? I certainly want to force this stuff on on the
> AP7000...otherwise we'll just get lots of complaints that the thing is
> using 4x more power than it's supposed to...
Well, "force" seems the wrong approach to me. That should be a
board-specific choice. The ap700x power budget may not be the
most important system design consideration, so making such a
decision at the platform ("ap700x") level seems wrong.
Suppose someone has to build an AVR32 based system that uses both
TCB modules to hook up to external hardware, so that neither one
is available for use as a "system timer"?
- Dave
On Sun, 24 Feb 2008 15:42:51 -0800
David Brownell <[email protected]> wrote:
> > On Fri, 22 Feb 2008 17:28:37 -0800
> > David Brownell <[email protected]> wrote:
> >
> > > +static cycle_t tc_get_cycles(void)
> > > +{
> > > + unsigned long flags;
> > > + u32 lower, upper;
> > > +
> > > + raw_local_irq_save(flags);
> >
> > Why do you need to use the raw version?
>
> This is part of the system timer code, and it should never be a
> preemption point. Plus I didn't want to waste any instruction
> cycles in code that runs before e.g. the DBGU IRQ handler would
> get called... observably, such extra cycles *do* hurt.
I don't understand what you mean by preemption point, but I guess the
non-raw version may consume some extra cycles when lockdep is enabled.
> > > +#ifdef CONFIG_GENERIC_CLOCKEVENTS
> > > +#define USE_TC_CLKEVT
> > > +#endif
> > > +
> > > +#ifdef CONFIG_ARCH_AT91RM9200
> > > +/* The AT91rm9200 system timer is a oneshot-capable 32k timer that's
> > > + * always running ... configuring a TC channel to work the same way
> > > + * would just waste some power.
> > > + */
> > > +#undef USE_TC_CLKEVT
> > > +#endif
> > > +
> > > +#ifdef USE_TC_CLKEVT
> >
> > Can't you just use #ifndef CONFIG_ARCH_AT91RM9200 and avoid the whole
> > ifdef/define/undef dance above?
>
> I can't know that some other platform won't have a better system
> timer solution, and didn't want to assume that only that single
> venerable chip had such a solution ... it's kind of puzzling to
> me (software guy) that none of the newest Atmel SOCs have better
> timer infrastructure than they do. ;)
Heh.
If we really expect using TC as a clocksource but not as a clockevent
is going to be a common usage, perhaps we should move the decision into
Kconfig?
Besides, I don't really see the difference between having a big #ifdef
expression around the whole clockevent block and having a big #ifdef
expression around the definition of USE_TC_CLKEVT except that the
latter is more code...
> > > + case CLOCK_EVT_MODE_ONESHOT:
> > > + /* slow clock, count up to RC, then irq and stop */
> >
> > Hmm. Do you really want to stop it? Won't you get better accuracy if
> > you let it run and program the next event as (prev_event + delta)?
>
> No, ONESHOT means "stop after the IRQ I asked for". And when
> tc_next_event() is asked to trigger that IRQ, it's relative to
> the instant it's requested, not relative to the last one that
> was requested (which may not have triggered yet, or may have
> been quite some time ago).
Right. Sounds like it might introduce some inaccuracy, but I guess it's
the clocksource's job to keep track of the actual time at the time of
the event.
> > > +static struct irqaction tc_irqaction = {
> > > + .name = "tc_clkevt",
> > > + .flags = IRQF_TIMER | IRQF_DISABLED,
> > > + .handler = ch2_irq,
> > > +};
> >
> > I don't think you need to define this statically. You can call
> > request_irq() at arch_initcall() time.
>
> That could be done, yes ... and using request_irq()/free_irq()
> would also let this be linked as a module, not just statically.
>
> On the other hand, doing it this way doesn't hurt either does it?
> Unless a modular build is important.
No, it doesn't hurt. Maybe we should keep it static so that we have the
option of initializing it earlier if we need to.
> > I don't think it is safe to assume that one clock per channel always
> > means one irq per channel as well...
>
> On current chips, that's how it works.
Indeed. I just don't see any fundamental reason why it has to work that
way, which is why I don't think we should depend on it.
> > What's wrong with
> >
> > irq = platform_get_irq(pdev, 2);
> > if (irq < 0)
> > irq = platform_get_irq(pdev, 0);
>
> Nothing much, except that I took the more concise path. Got patch?
Not until we reach a conclusion about the tclib core.
> > How about we make tcb_clksrc_init() global and call it from the
> > platform code with whatever TCB the platform thinks is appropriate?
>
> That could work too, but if it goes that way then there's no
> point in changes to support a modular build (e.g. the irqaction
> thing you noted above).
True.
> > Perhaps remove the prompt from ATMEL_TCB_CLKSRC and have the platform
> > select it as well? I certainly want to force this stuff on on the
> > AP7000...otherwise we'll just get lots of complaints that the thing is
> > using 4x more power than it's supposed to...
>
> Well, "force" seems the wrong approach to me. That should be a
> board-specific choice. The ap700x power budget may not be the
> most important system design consideration, so making such a
> decision at the platform ("ap700x") level seems wrong.
>
> Suppose someone has to build an AVR32 based system that uses both
> TCB modules to hook up to external hardware, so that neither one
> is available for use as a "system timer"?
Good point. Let's keep it as it is and let the board "select" it
through its defconfig.
Haavard
> > > > +static cycle_t tc_get_cycles(void)
> > > > +{
> > > > + unsigned long flags;
> > > > + u32 lower, upper;
> > > > +
> > > > + raw_local_irq_save(flags);
> > >
> > > Why do you need to use the raw version?
> >
> > This is part of the system timer code, and it should never be a
> > preemption point. Plus I didn't want to waste any instruction
> > cycles in code that runs before e.g. the DBGU IRQ handler would
> > get called... observably, such extra cycles *do* hurt.
>
> I don't understand what you mean by preemption point, but I guess the
> non-raw version may consume some extra cycles when lockdep is enabled.
A preemption point is where CONFIG_PREEMPT kicks in task switching
logic; lockdep is different.
> If we really expect using TC as a clocksource but not as a clockevent
> is going to be a common usage, perhaps we should move the decision into
> Kconfig?
Maybe, but I already spent lots more time on this than I wanted. :(
Another way to address that rm9200 issue would be to just rate
the TC clockevent source lower than the one based on the system
timer, so it's set up but never enabled ... and remember "t2_clk",
calling clk_enable() only when that clockevent device is active.
That would address one half of the suspend/resume equation too,
ensuring that clock is disabled during suspend...
The other half being: how to clk_disable(t0_clk) during system
suspend? (And t1_clk on some systems.) There's currently no
clocksource.set_mode() call; evidently there's an assumption that
such counters cost no power, so they can always be left running.
> > > I don't think it is safe to assume that one clock per channel always
> > > means one irq per channel as well...
> >
> > On current chips, that's how it works.
>
> Indeed. I just don't see any fundamental reason why it has to work that
> way, which is why I don't think we should depend on it.
AT91 chips share identifiers between clocks and interrupts; that's
fundamental, yes?
If some future chip works differently, that's a good time to change
things. Otherwise I see little point in caring about such issues.
- Dave
On Mon, 25 Feb 2008 09:51:16 -0800
David Brownell <[email protected]> wrote:
> > > > > +static cycle_t tc_get_cycles(void)
> > > > > +{
> > > > > + unsigned long flags;
> > > > > + u32 lower, upper;
> > > > > +
> > > > > + raw_local_irq_save(flags);
> > > >
> > > > Why do you need to use the raw version?
> > >
> > > This is part of the system timer code, and it should never be a
> > > preemption point. Plus I didn't want to waste any instruction
> > > cycles in code that runs before e.g. the DBGU IRQ handler would
> > > get called... observably, such extra cycles *do* hurt.
> >
> > I don't understand what you mean by preemption point, but I guess the
> > non-raw version may consume some extra cycles when lockdep is enabled.
>
> A preemption point is where CONFIG_PREEMPT kicks in task switching
> logic; lockdep is different.
I know, but I dont' see how local_irq_save/restore has anything to do
with it, raw or not. There would be absolutely no point checking for
preemption on local_irq_restore() since no one would have been able to
set the TIF_NEED_RESCHED flag while interrupts were disabled...
raw_local_irq_save/restore is only different from
local_irq_save/restore when lockdep is enabled. That's why I don't
understand why you're talking about preemption.
> > If we really expect using TC as a clocksource but not as a clockevent
> > is going to be a common usage, perhaps we should move the decision into
> > Kconfig?
>
> Maybe, but I already spent lots more time on this than I wanted. :(
I'm not asking you to do it. I'm asking if it would be a good thing to
do. I said that I can take these patches off your back if you want, but
I want to make sure I don't do anything with them that you disagree
with.
> Another way to address that rm9200 issue would be to just rate
> the TC clockevent source lower than the one based on the system
> timer, so it's set up but never enabled ... and remember "t2_clk",
> calling clk_enable() only when that clockevent device is active.
>
> That would address one half of the suspend/resume equation too,
> ensuring that clock is disabled during suspend...
Yes, we could probably do that. Which means we can just remove all the
ifdeffery?
> The other half being: how to clk_disable(t0_clk) during system
> suspend? (And t1_clk on some systems.) There's currently no
> clocksource.set_mode() call; evidently there's an assumption that
> such counters cost no power, so they can always be left running.
Yes...that would be a clocksource core issue I guess. Better leave that
for later...
> > > > I don't think it is safe to assume that one clock per channel always
> > > > means one irq per channel as well...
> > >
> > > On current chips, that's how it works.
> >
> > Indeed. I just don't see any fundamental reason why it has to work that
> > way, which is why I don't think we should depend on it.
>
> AT91 chips share identifiers between clocks and interrupts; that's
> fundamental, yes?
>
> If some future chip works differently, that's a good time to change
> things. Otherwise I see little point in caring about such issues.
Agreed.
Haavard
On Tuesday 26 February 2008, Haavard Skinnemoen wrote:
> > Another way to address that rm9200 issue would be to just rate
> > the TC clockevent source lower than the one based on the system
> > timer, so it's set up but never enabled ... and remember "t2_clk",
> > calling clk_enable() only when that clockevent device is active.
> >
> > That would address one half of the suspend/resume equation too,
> > ensuring that clock is disabled during suspend...
>
> Yes, we could probably do that. Which means we can just remove all the
> ifdeffery?
There'd still be the #ifdef for CONFIG_GENERIC_CLOCKEVENTS,
unless all the platforms get updated to support that. Right
now it's a "patches available but not merged" situation.
> > The other half being: ?how to clk_disable(t0_clk) during system
> > suspend? ?(And t1_clk on some systems.) ?There's currently no
> > clocksource.set_mode() call; evidently there's an assumption that
> > such counters cost no power, so they can always be left running.
>
> Yes...that would be a clocksource core issue I guess. Better leave that
> for later...
My thoughts exactly. ;)
Plus a bit of puzzlement why that didn't trigger at least a
warning during AT91 suspend testing. There used to be warnings
there about clocks which were wrongly left enabled...
- Dave