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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
