Messages in this thread | | | Date | Tue, 26 Feb 2008 10:16:25 +0100 | From | Haavard Skinnemoen <> | Subject | Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code |
| |
On Mon, 25 Feb 2008 09:51:16 -0800 David Brownell <david-b@pacbell.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. > > > > 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
| |