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
--
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/
