LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Haavard Skinnemoen <hskinnemoen@atmel.com>
To: David Brownell <david-b@pacbell.net>
Cc: nicolas.ferre@rfo.atmel.com, linux@maxim.org.za,
	linux-kernel@vger.kernel.org, akpm@linux-foundation.org
Subject: Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code
Date: Tue, 26 Feb 2008 10:16:25 +0100	[thread overview]
Message-ID: <20080226101625.31f6954d@hskinnemoen.norway.atmel.com> (raw)
In-Reply-To: <20080225175116.A2C071E564F@adsl-69-226-248-13.dsl.pltn13.pacbell.net>

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

  reply	other threads:[~2008-02-26  9:17 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-23  1:28 David Brownell
2008-02-24 18:21 ` Haavard Skinnemoen
2008-02-24 23:42   ` David Brownell
2008-02-25 11:31     ` Haavard Skinnemoen
2008-02-25 17:51       ` David Brownell
2008-02-26  9:16         ` Haavard Skinnemoen [this message]
2008-02-26  9:25           ` David Brownell

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080226101625.31f6954d@hskinnemoen.norway.atmel.com \
    --to=hskinnemoen@atmel.com \
    --cc=akpm@linux-foundation.org \
    --cc=david-b@pacbell.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@maxim.org.za \
    --cc=nicolas.ferre@rfo.atmel.com \
    --subject='Re: [patch 2.6.25-rc2-git 2/2] atmel_tc clocksource/clockevent code' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).