Message-ID: <aec7e5c30906140320r4d24c9efqb2bc2e30d715de02@mail.gmail.com>
Date:	Sun, 14 Jun 2009 19:20:12 +0900
From:	Magnus Damm <magnus.damm@...il.com>
To:	john stultz <johnstul@...ibm.com>
Cc:	linux-kernel@...r.kernel.org, mingo@...e.hu, lethal@...ux-sh.org,
	tglx@...utronix.de, akpm@...ux-foundation.org
Subject: Re: [PATCH] clocksource: setup mult_orig in clocksource_enable()

On Sat, Jun 13, 2009 at 8:56 AM, john stultz<johnstul@...ibm.com> wrote:
> On Thu, 2009-06-11 at 14:51 +0900, Magnus Damm wrote:
>> On Thu, Jun 11, 2009 at 6:04 AM, john stultz<johnstul@...ibm.com> wrote:
>> > Is there really no way to calculate the mult value prior to
>> > registration? Maybe quickly enabling, getting the freq, and then
>> > disabling?
>>
>> I can't think of any way that would work. The clock frequency can be
>> changed while the clock is disabled. And we can only know the rate
>> after enabling the clock, see these lines from include/linux/clk.h:
>>
>> /**
>>  * clk_get_rate - obtain the current clock rate (in Hz) for a clock source.
>>  *              This is only valid once the clock source has been enabled.
>>  * @clk: clock source
>>  */
>> unsigned long clk_get_rate(struct clk *clk);
>
> Hrmm.. Yuck.
>
> Is this really expected behavior that a clk would change frequencies
> between uses as a clocksource?

Yes, I think so. The clock frequency can change through cpufreq or
clk_set_rate().

> Do you have some examples where this code is actually used?

Everywhere. =) Many embedded platforms use the clock framework for
(runtime power) management of clocks, and clk_get_rate() is the
standard way of getting the frequency of a certain clock. Just grep in
drivers/, or check out the timer code currently used by SuperH in
driver/clocksource/ or drivers/clocksource/tcb_clksrc.c.

>> > So this seems like it will break if a clocksource is switched away from
>> > and then back to (the reason we added mult_orig in the first place).
>> > Since the re-enabled clocksource would then save off its modified mult
>> > value into mult_orig.
>>
>> Oh, I see. Sorry about that. I wonder if adding a "cs->mult =
>> cs->orig_mult;" to clock_disable() would help?
>
> Technically it would. Although we lose the corrective factor that had
> already been applied, but that should readjust fairly quickly.
>
> So yea, at a minimum setting mult back to orig_mult would be needed for
> this patch to work.

Want me to write a patch for it, or do you prefer to handle it yourself?

> However, its just ugly. I don't really like the idea of clocksources
> freq changes under us (even if they're not actively in use). But I may
> have to just deal with the reality. :(

Yeah, I agree that the mult/org_mult save/restore code is far from
pretty. Unfortunately I think we all have to live with that unused
clocks can get their frequencies changed. It's just the way the clock
framework is designed. I'm open to any suggestions how to deal with it
in a cleaner way...

Another option would be that we don't register multiple clocksources -
only one at a time - but I then we would have to invent some layer on
top of clocksources. I prefer registering a bunch of clocksources and
letting the generic clocksource code use the rating to decide which
one to enable. I think that's pretty close to how x86 does things, no?

Any ideas?

Cheers,

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