On Fri, 2009-05-01 at 14:45 +0900, Magnus Damm wrote:
> From: Magnus Damm <[email protected]>
>
> Setup clocksource mult_orig in clocksource_enable().
Hey Magnus,
Sorry I missed this earlier, I just noticed it in the -tip tree.
I've some concerns below.
> Clocksource drivers can save power by using keeping the
> device clock disabled while the clocksource is unused.
>
> In practice this means that the enable() and disable()
> callbacks perform clk_enable() and clk_disable().
>
> The enable() callback may also use clk_get_rate() to get
> the clock rate from the clock framework. This information
> can then be used to calculate the shift and mult variables.
Hrmmm.. So when the clocksource code was designed, it was expected that
the clocksource mult value would be set prior to registration, and then
would not be modified by any user other then the timekeeping core. As
changing the mult value directly (on a clocksource thats being used)
could cause time inconsistencies.
> Currently the mult_orig variable is setup from mult at
> registration time only. This is conflicting with the above
> case since the clock is disabled and the mult variable is
> not yet calculated at the time of registration.
Is there really no way to calculate the mult value prior to
registration? Maybe quickly enabling, getting the freq, and then
disabling?
> Moving the mult_orig setup code to clocksource_enable()
> allows us to both handle the common case with no enable()
> callback and the mult-changed-after-enable() case.
>
> Signed-off-by: Magnus Damm <[email protected]>
> ---
>
> include/linux/clocksource.h | 10 +++++++++-
> kernel/time/clocksource.c | 3 ---
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> --- 0001/include/linux/clocksource.h
> +++ work/include/linux/clocksource.h 2009-05-01 12:59:27.000000000 +0900
> @@ -288,7 +288,15 @@ static inline cycle_t clocksource_read(s
> */
> static inline int clocksource_enable(struct clocksource *cs)
> {
> - return cs->enable ? cs->enable(cs) : 0;
> + int ret = 0;
> +
> + if (cs->enable)
> + ret = cs->enable(cs);
> +
> + /* save mult_orig on enable */
> + cs->mult_orig = cs->mult;
> +
> + return ret;
> }
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.
thanks
-john
Hi John,
On Thu, Jun 11, 2009 at 6:04 AM, john stultz<[email protected]> wrote:
> On Fri, 2009-05-01 at 14:45 +0900, Magnus Damm wrote:
>> From: Magnus Damm <[email protected]>
>>
>> Setup clocksource mult_orig in clocksource_enable().
>
> Hey Magnus,
> Sorry I missed this earlier, I just noticed it in the -tip tree.
No worries!
> I've some concerns below.
>
>> Clocksource drivers can save power by using keeping the
>> device clock disabled while the clocksource is unused.
>>
>> In practice this means that the enable() and disable()
>> callbacks perform clk_enable() and clk_disable().
>>
>> The enable() callback may also use clk_get_rate() to get
>> the clock rate from the clock framework. This information
>> can then be used to calculate the shift and mult variables.
>
> Hrmmm.. So when the clocksource code was designed, it was expected that
> the clocksource mult value would be set prior to registration, and then
> would not be modified by any user other then the timekeeping core. As
> changing the mult value directly (on a clocksource thats being used)
> could cause time inconsistencies.
But no one is changing the mult value on a clocksource that is being
used in this case, no? I may remember wrong, but isn't
clocksource_enable() called on unused clocksources that soon will
become used?
>> Currently the mult_orig variable is setup from mult at
>> registration time only. This is conflicting with the above
>> case since the clock is disabled and the mult variable is
>> not yet calculated at the time of registration.
>
> 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);
>> --- 0001/include/linux/clocksource.h
>> +++ work/include/linux/clocksource.h ?2009-05-01 12:59:27.000000000 +0900
>> @@ -288,7 +288,15 @@ static inline cycle_t clocksource_read(s
>> ? */
>> ?static inline int clocksource_enable(struct clocksource *cs)
>> ?{
>> - ? ? return cs->enable ? cs->enable(cs) : 0;
>> + ? ? int ret = 0;
>> +
>> + ? ? if (cs->enable)
>> + ? ? ? ? ? ? ret = cs->enable(cs);
>> +
>> + ? ? /* save mult_orig on enable */
>> + ? ? cs->mult_orig = cs->mult;
>> +
>> + ? ? return ret;
>> ?}
>
> 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?
/ magnus
On Thu, 2009-06-11 at 14:51 +0900, Magnus Damm wrote:
> Hi John,
>
> On Thu, Jun 11, 2009 at 6:04 AM, john stultz<[email protected]> wrote:
> > On Fri, 2009-05-01 at 14:45 +0900, Magnus Damm wrote:
> >> From: Magnus Damm <[email protected]>
> >>
> >> Setup clocksource mult_orig in clocksource_enable().
> >
> > Hey Magnus,
> > Sorry I missed this earlier, I just noticed it in the -tip tree.
>
> No worries!
>
> > I've some concerns below.
> >
> >> Clocksource drivers can save power by using keeping the
> >> device clock disabled while the clocksource is unused.
> >>
> >> In practice this means that the enable() and disable()
> >> callbacks perform clk_enable() and clk_disable().
> >>
> >> The enable() callback may also use clk_get_rate() to get
> >> the clock rate from the clock framework. This information
> >> can then be used to calculate the shift and mult variables.
> >
> > Hrmmm.. So when the clocksource code was designed, it was expected that
> > the clocksource mult value would be set prior to registration, and then
> > would not be modified by any user other then the timekeeping core. As
> > changing the mult value directly (on a clocksource thats being used)
> > could cause time inconsistencies.
>
> But no one is changing the mult value on a clocksource that is being
> used in this case, no? I may remember wrong, but isn't
> clocksource_enable() called on unused clocksources that soon will
> become used?
True, but having mult change after registration is still somewhat
unexpected behavior (to me at least).
> >> Currently the mult_orig variable is setup from mult at
> >> registration time only. This is conflicting with the above
> >> case since the clock is disabled and the mult variable is
> >> not yet calculated at the time of registration.
> >
> > 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?
Do you have some examples where this code is actually used?
> >> --- 0001/include/linux/clocksource.h
> >> +++ work/include/linux/clocksource.h 2009-05-01 12:59:27.000000000 +0900
> >> @@ -288,7 +288,15 @@ static inline cycle_t clocksource_read(s
> >> */
> >> static inline int clocksource_enable(struct clocksource *cs)
> >> {
> >> - return cs->enable ? cs->enable(cs) : 0;
> >> + int ret = 0;
> >> +
> >> + if (cs->enable)
> >> + ret = cs->enable(cs);
> >> +
> >> + /* save mult_orig on enable */
> >> + cs->mult_orig = cs->mult;
> >> +
> >> + return ret;
> >> }
> >
> > 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.
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. :(
thanks
-john
On Sat, Jun 13, 2009 at 8:56 AM, john stultz<[email protected]> wrote:
> On Thu, 2009-06-11 at 14:51 +0900, Magnus Damm wrote:
>> On Thu, Jun 11, 2009 at 6:04 AM, john stultz<[email protected]> 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
On Sun, 2009-06-14 at 19:20 +0900, Magnus Damm wrote:
> On Sat, Jun 13, 2009 at 8:56 AM, john stultz<[email protected]> wrote:
> > On Thu, 2009-06-11 at 14:51 +0900, Magnus Damm wrote:
> >> On Thu, Jun 11, 2009 at 6:04 AM, john stultz<[email protected]> 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().
But they do not change freq (through cpufreq or anything else) after the
enable() call, right? That would be pretty critical. Otherwise they'd
need to be disqualified like we do the TSC on x86.
> > 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.
Yea, I was just curious which clocksources were actually using the clk
framework. Thanks for the pointer, I'll take a look.
> >> > 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?
You should submit it. Its just a tweak on your prior patch.
> > 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?
Right. We don't want to duplicate the clocksource selection code.
So we'll just live with it. If you could please throw a big comment
around the orig_mult/mult assignments explaining why its necessary to do
it there.
thanks
-john
On Mon, Jun 15, 2009 at 12:08:37PM -0700, john stultz wrote:
> On Sun, 2009-06-14 at 19:20 +0900, Magnus Damm wrote:
> > On Sat, Jun 13, 2009 at 8:56 AM, john stultz<[email protected]> wrote:
> > > On Thu, 2009-06-11 at 14:51 +0900, Magnus Damm wrote:
> > >> 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().
>
> But they do not change freq (through cpufreq or anything else) after the
> enable() call, right? That would be pretty critical. Otherwise they'd
> need to be disqualified like we do the TSC on x86.
>
This is a bit tricky, the clock needs to be able to adjust its parent
divisors/multipliers in order to maintain its current frequency if a
parent clock changes frequency. We do not presently prohibit a frequency
change that deviates from the current frequency on these clocks, but
this would be trivially handled by setting a fixed rate flag for those
clocks. This sort of logic is necessary to block frequency changes in the
parent clock topology that would throw the child clock's frequency out of
sync. Note that in the general case we do not want to disable frequency
changes on enabled clocks, enabled clocks only need to know whether they
can handle a frequency change or not without destabilizing the system.
The only thing the usecount presently disables on an enabled clock is
reparenting it. ie, migrating between different parent PLLs while in
active use.
There is no fundamental limitation as with the TSC, we can have as much
or as little flexibility as we like.
On Tue, 2009-06-16 at 05:04 +0900, Paul Mundt wrote:
> On Mon, Jun 15, 2009 at 12:08:37PM -0700, john stultz wrote:
> > On Sun, 2009-06-14 at 19:20 +0900, Magnus Damm wrote:
> > > On Sat, Jun 13, 2009 at 8:56 AM, john stultz<[email protected]> wrote:
> > > > On Thu, 2009-06-11 at 14:51 +0900, Magnus Damm wrote:
> > > >> 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().
> >
> > But they do not change freq (through cpufreq or anything else) after the
> > enable() call, right? That would be pretty critical. Otherwise they'd
> > need to be disqualified like we do the TSC on x86.
> >
> This is a bit tricky, the clock needs to be able to adjust its parent
> divisors/multipliers in order to maintain its current frequency if a
> parent clock changes frequency. We do not presently prohibit a frequency
> change that deviates from the current frequency on these clocks, but
> this would be trivially handled by setting a fixed rate flag for those
> clocks. This sort of logic is necessary to block frequency changes in the
> parent clock topology that would throw the child clock's frequency out of
> sync. Note that in the general case we do not want to disable frequency
> changes on enabled clocks, enabled clocks only need to know whether they
> can handle a frequency change or not without destabilizing the system.
Any change to the freq mult value in the clocksource would be
destabilizing. So I'd advise pretty strongly to make sure that clk's
being used as clocksources cannot change frequency once they're enabled
and in use.
thanks
-john
On Mon, 2009-06-15 at 12:08 -0700, john stultz wrote:
> On Sun, 2009-06-14 at 19:20 +0900, Magnus Damm wrote:
> > On Sat, Jun 13, 2009 at 8:56 AM, john stultz<[email protected]> wrote:
> > > 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?
>
> You should submit it. Its just a tweak on your prior patch.
So Linus already merged your patch that breaks mult_orig, so fixing this
is a bit more of a priority. Let me know if you have the cycles to fix
it and if not I'll take a swing at it.
thanks
-john