Message-Id: <20080224234251.18B519B7DF@adsl-69-226-248-13.dsl.pltn13.pacbell.net>
Date:	Sun, 24 Feb 2008 15:42:51 -0800
From:	David Brownell <david-b@...bell.net>
To:	hskinnemoen@...el.com
Cc:	nicolas.ferre@....atmel.com, linux@...im.org.za,
	linux-kernel@...r.kernel.org, akpm@...ux-foundation.org
Subject: Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code

> 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 <david-b@...bell.net> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists