lkml.org 
[lkml]   [2008]   [Feb]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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


    \
     
     \ /
      Last update: 2008-02-26 10:19    [W:0.027 / U:0.292 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site