[PATCH v2] sh_tmu: compute mult and shift before registration

[PATCH v2] sh_tmu: compute mult and shift before registration

[Aurelien Jarno]

Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
clocksource wrapping during idle"), the CPU of an R2D board never goes
to idle. This commit assumes that mult and shift are assigned before
the clocksource is registered. As a consequence the safe maximum sleep
time is negative and the CPU never goes into idle.

This patch fixes the problem by moving mult and shift initialization
from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 drivers/clocksource/sh_tmu.c |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/clocksource/sh_tmu.c b/drivers/clocksource/sh_tmu.c
index 8e44e14..de71590 100644
--- a/drivers/clocksource/sh_tmu.c
+++ b/drivers/clocksource/sh_tmu.c
@@ -199,16 +199,8 @@ static cycle_t sh_tmu_clocksource_read(struct clocksource *cs)
 static int sh_tmu_clocksource_enable(struct clocksource *cs)
 {
 	struct sh_tmu_priv *p = cs_to_sh_tmu(cs);
-	int ret;
-
-	ret = sh_tmu_enable(p);
-	if (ret)
-		return ret;
 
-	/* TODO: calculate good shift from rate and counter bit width */
-	cs->shift = 10;
-	cs->mult = clocksource_hz2mult(p->rate, cs->shift);
-	return 0;
+	return sh_tmu_enable(p);
 }
 
 static void sh_tmu_clocksource_disable(struct clocksource *cs)
@@ -228,6 +220,16 @@ static int sh_tmu_register_clocksource(struct sh_tmu_priv *p,
 	cs->disable = sh_tmu_clocksource_disable;
 	cs->mask = CLOCKSOURCE_MASK(32);
 	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
+
+	/* clk_get_rate() needs an enabled clock */
+	clk_enable(p->clk);
+	/* channel will be configured at parent clock / 4 */
+	p->rate = clk_get_rate(p->clk) / 4;
+	clk_disable(p->clk);
+	/* TODO: calculate good shift from rate and counter bit width */
+	cs->shift = 10;
+	cs->mult = clocksource_hz2mult(p->rate, cs->shift);
+
 	dev_info(&p->pdev->dev, "used as clock source\n");
 	clocksource_register(cs);
 	return 0;
-- 
1.7.1

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[Paul Mundt]

On Mon, May 31, 2010 at 11:45:48PM +0200, Aurelien Jarno wrote:
> Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
> clocksource wrapping during idle"), the CPU of an R2D board never goes
> to idle. This commit assumes that mult and shift are assigned before
> the clocksource is registered. As a consequence the safe maximum sleep
> time is negative and the CPU never goes into idle.
> 
> This patch fixes the problem by moving mult and shift initialization
> from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().
> 
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

Applied, thanks.

I've made a similar change for the sh_cmt driver, too.

Both of these should show up in the next 2.6.34-stable release.

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[Aurelien Jarno]

Paul Mundt a écrit :
> On Mon, May 31, 2010 at 11:45:48PM +0200, Aurelien Jarno wrote:
>> Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
>> clocksource wrapping during idle"), the CPU of an R2D board never goes
>> to idle. This commit assumes that mult and shift are assigned before
>> the clocksource is registered. As a consequence the safe maximum sleep
>> time is negative and the CPU never goes into idle.
>>
>> This patch fixes the problem by moving mult and shift initialization
>> from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().
>>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> Applied, thanks.
> 
> I've made a similar change for the sh_cmt driver, too.
> 
> Both of these should show up in the next 2.6.34-stable release.
> 

Thanks!


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[Magnus Damm]

Hi everyone,

[Added clock source hackers to CC]

On Wed, Jun 2, 2010 at 5:23 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Mon, May 31, 2010 at 11:45:48PM +0200, Aurelien Jarno wrote:
>> Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
>> clocksource wrapping during idle"), the CPU of an R2D board never goes
>> to idle. This commit assumes that mult and shift are assigned before
>> the clocksource is registered. As a consequence the safe maximum sleep
>> time is negative and the CPU never goes into idle.
>>
>> This patch fixes the problem by moving mult and shift initialization
>> from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().
>>
>> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>
> Applied, thanks.
>
> I've made a similar change for the sh_cmt driver, too.
>
> Both of these should show up in the next 2.6.34-stable release.

Thanks for this!

However, with the CMT patch applied the clock seems to run too fast.
Requesting a 5 second sleep results in less than a second. The
LED-based heart beat is really busy! =)

Switching back to a jiffies-based clock source work arounds the
problem. This is regardless of the CONFIG_NO_HZ setting.

Proposal:

Since the clock frequency of the clock provided by the clock framework
may vary when it is disabled, perhaps it's good to keep the existing
CMT code as-is and instead work towards updating the generic clock
source code to calculate values after the clock source has been
enabled.

The problem is that we don't know the frequency of the timers at
registration time. So most of the clock source calculation happens
after enable today. I added the ->enable() and ->disable() callbacks
last year to solve this ordering issue. The cs->max_idle_ns variable
seems to be a special case though.

Perhaps cs->max_idle_ns can be calculated after the clock source has
been enabled, for instance inside timerkeeper_setup_internals()?

Or are there better ways to deal with this?

Thanks,

/ magnus

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[john stultz]

On Mon, 2010-06-07 at 16:06 +0900, Magnus Damm wrote:
> Hi everyone,
> 
> [Added clock source hackers to CC]
> 
> On Wed, Jun 2, 2010 at 5:23 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Mon, May 31, 2010 at 11:45:48PM +0200, Aurelien Jarno wrote:
> >> Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
> >> clocksource wrapping during idle"), the CPU of an R2D board never goes
> >> to idle. This commit assumes that mult and shift are assigned before
> >> the clocksource is registered. As a consequence the safe maximum sleep
> >> time is negative and the CPU never goes into idle.
> >>
> >> This patch fixes the problem by moving mult and shift initialization
> >> from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().
> >>
> >> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> >
> > Applied, thanks.
> >
> > I've made a similar change for the sh_cmt driver, too.
> >
> > Both of these should show up in the next 2.6.34-stable release.
> 
> Thanks for this!
> 
> However, with the CMT patch applied the clock seems to run too fast.
> Requesting a 5 second sleep results in less than a second. The
> LED-based heart beat is really busy! =)
> 
> Switching back to a jiffies-based clock source work arounds the
> problem. This is regardless of the CONFIG_NO_HZ setting.
> 
> Proposal:
> 
> Since the clock frequency of the clock provided by the clock framework
> may vary when it is disabled, perhaps it's good to keep the existing
> CMT code as-is and instead work towards updating the generic clock
> source code to calculate values after the clock source has been
> enabled.
> 
> The problem is that we don't know the frequency of the timers at
> registration time. So most of the clock source calculation happens
> after enable today. I added the ->enable() and ->disable() callbacks
> last year to solve this ordering issue. The cs->max_idle_ns variable
> seems to be a special case though.
> 
> Perhaps cs->max_idle_ns can be calculated after the clock source has
> been enabled, for instance inside timerkeeper_setup_internals()?
> 
> Or are there better ways to deal with this?

I recently ran into this issue when looking to convert clocksources from
using clocksource_register() to clocksource_register_hz()/khz().

The point of those patches are to try to remove the mult/shift
calculation from the clocksource driver code, keeping that logic in the
timekeeping core (which will allow for better optimization of other
values like max_idle_ns).

So I'd suggest a generic method to the effect of
"__clocksource_update_hz/khz" which can allow clocksource drivers to
update the freq in enable call path. This can then hook into the
timekeeping core so we update things there.

Does that sound reasonable?

The timekeeping code really isn't expecting clocksource values to change
under it. So we'll also have to carefully review some of the max_idle_ns
users to make sure they can handle the change.

thanks
-john

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[Magnus Damm]

On Tue, Jun 8, 2010 at 4:28 AM, john stultz <johnstul@us.ibm.com> wrote:
> On Mon, 2010-06-07 at 16:06 +0900, Magnus Damm wrote:
>> On Wed, Jun 2, 2010 at 5:23 PM, Paul Mundt <lethal@linux-sh.org> wrote:
>> > On Mon, May 31, 2010 at 11:45:48PM +0200, Aurelien Jarno wrote:
>> >> Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
>> >> clocksource wrapping during idle"), the CPU of an R2D board never goes
>> >> to idle. This commit assumes that mult and shift are assigned before
>> >> the clocksource is registered. As a consequence the safe maximum sleep
>> >> time is negative and the CPU never goes into idle.
>> >>
>> >> This patch fixes the problem by moving mult and shift initialization
>> >> from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().
>> >>
>> >> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
>> >
>> > Applied, thanks.
>> >
>> > I've made a similar change for the sh_cmt driver, too.
>> >
>> > Both of these should show up in the next 2.6.34-stable release.
>>
>> Thanks for this!
>>
>> However, with the CMT patch applied the clock seems to run too fast.
>> Requesting a 5 second sleep results in less than a second. The
>> LED-based heart beat is really busy! =)
>>
>> Switching back to a jiffies-based clock source work arounds the
>> problem. This is regardless of the CONFIG_NO_HZ setting.
>>
>> Proposal:
>>
>> Since the clock frequency of the clock provided by the clock framework
>> may vary when it is disabled, perhaps it's good to keep the existing
>> CMT code as-is and instead work towards updating the generic clock
>> source code to calculate values after the clock source has been
>> enabled.
>>
>> The problem is that we don't know the frequency of the timers at
>> registration time. So most of the clock source calculation happens
>> after enable today. I added the ->enable() and ->disable() callbacks
>> last year to solve this ordering issue. The cs->max_idle_ns variable
>> seems to be a special case though.
>>
>> Perhaps cs->max_idle_ns can be calculated after the clock source has
>> been enabled, for instance inside timerkeeper_setup_internals()?
>>
>> Or are there better ways to deal with this?
>
> I recently ran into this issue when looking to convert clocksources from
> using clocksource_register() to clocksource_register_hz()/khz().
>
> The point of those patches are to try to remove the mult/shift
> calculation from the clocksource driver code, keeping that logic in the
> timekeeping core (which will allow for better optimization of other
> values like max_idle_ns).

This common code for mult/shift calculation is very useful I think.

That aside, I have to admit that I don't fully understand which of the
_hz()/khz() I'm supposed to use. Say that I have a hardware counter
that is in the MHz range, which should I use?

And for a 4Khz counter?

Isn't it possible to just have a single __clocksource_update_freq()
function that checks the range and adjusts the scale? Perhaps there is
something that I'm not getting...

> So I'd suggest a generic method to the effect of
> "__clocksource_update_hz/khz" which can allow clocksource drivers to
> update the freq in enable call path. This can then hook into the
> timekeeping core so we update things there.
>
> Does that sound reasonable?

I think so. So like I mentioned earlier, we don't know what frequency
range is suitable at clock source registration time. So from that
point of view I can't really see why we should keep the
clocksource_register_hz()/khz() functions. Updating the frequency in
the ->enable() callback and using clocksource_register() sounds simple
and straightforward to me. Ideally I'd like to avoid setting up the
mult and shift values before registration and instead put all logic in
the ->enable() callback.

The ->enable() callback is optional so we will have to deal with
clocksources without ->enable() callbacks. This should be trivial -
such drivers can manually setup shift/mult or call
__clocksource_update_freq() before clocksource_register().

> The timekeeping code really isn't expecting clocksource values to change
> under it. So we'll also have to carefully review some of the max_idle_ns
> users to make sure they can handle the change.

I may be wrong, but it looks like the clocksource is still unused when
->enable() callback is invoked.

Thanks,

/ magnus

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[john stultz]

On Thu, 2010-06-10 at 15:20 +0900, Magnus Damm wrote:
> On Tue, Jun 8, 2010 at 4:28 AM, john stultz <johnstul@us.ibm.com> wrote:
> > On Mon, 2010-06-07 at 16:06 +0900, Magnus Damm wrote:
> >> On Wed, Jun 2, 2010 at 5:23 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> >> > On Mon, May 31, 2010 at 11:45:48PM +0200, Aurelien Jarno wrote:
> >> >> Since commit 98962465ed9e6ea99c38e0af63fe1dcb5a79dc25 ("nohz: Prevent
> >> >> clocksource wrapping during idle"), the CPU of an R2D board never goes
> >> >> to idle. This commit assumes that mult and shift are assigned before
> >> >> the clocksource is registered. As a consequence the safe maximum sleep
> >> >> time is negative and the CPU never goes into idle.
> >> >>
> >> >> This patch fixes the problem by moving mult and shift initialization
> >> >> from sh_tmu_clocksource_enable() to sh_tmu_register_clocksource().
> >> >>
> >> >> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> >> >
> >> > Applied, thanks.
> >> >
> >> > I've made a similar change for the sh_cmt driver, too.
> >> >
> >> > Both of these should show up in the next 2.6.34-stable release.
> >>
> >> Thanks for this!
> >>
> >> However, with the CMT patch applied the clock seems to run too fast.
> >> Requesting a 5 second sleep results in less than a second. The
> >> LED-based heart beat is really busy! =)
> >>
> >> Switching back to a jiffies-based clock source work arounds the
> >> problem. This is regardless of the CONFIG_NO_HZ setting.
> >>
> >> Proposal:
> >>
> >> Since the clock frequency of the clock provided by the clock framework
> >> may vary when it is disabled, perhaps it's good to keep the existing
> >> CMT code as-is and instead work towards updating the generic clock
> >> source code to calculate values after the clock source has been
> >> enabled.
> >>
> >> The problem is that we don't know the frequency of the timers at
> >> registration time. So most of the clock source calculation happens
> >> after enable today. I added the ->enable() and ->disable() callbacks
> >> last year to solve this ordering issue. The cs->max_idle_ns variable
> >> seems to be a special case though.
> >>
> >> Perhaps cs->max_idle_ns can be calculated after the clock source has
> >> been enabled, for instance inside timerkeeper_setup_internals()?
> >>
> >> Or are there better ways to deal with this?
> >
> > I recently ran into this issue when looking to convert clocksources from
> > using clocksource_register() to clocksource_register_hz()/khz().
> >
> > The point of those patches are to try to remove the mult/shift
> > calculation from the clocksource driver code, keeping that logic in the
> > timekeeping core (which will allow for better optimization of other
> > values like max_idle_ns).
> 
> This common code for mult/shift calculation is very useful I think.
> 
> That aside, I have to admit that I don't fully understand which of the
> _hz()/khz() I'm supposed to use. Say that I have a hardware counter
> that is in the MHz range, which should I use?

You probably want use the _hz interface if possible. The khz() interface
is for very high frequency counters (like the TSC) where its possible
the hz value wouldn't fit in a u32.

> And for a 4Khz counter?

Again, _hz is probably ideal for precision, but there's no issue using
the khz value if that's what the platform provides you with (ie: you
won't get any better precision by multiplying it up).


> Isn't it possible to just have a single __clocksource_update_freq()
> function that checks the range and adjusts the scale? Perhaps there is
> something that I'm not getting...
> 
> > So I'd suggest a generic method to the effect of
> > "__clocksource_update_hz/khz" which can allow clocksource drivers to
> > update the freq in enable call path. This can then hook into the
> > timekeeping core so we update things there.
> >
> > Does that sound reasonable?
> 
> I think so. So like I mentioned earlier, we don't know what frequency
> range is suitable at clock source registration time. So from that
> point of view I can't really see why we should keep the
> clocksource_register_hz()/khz() functions. Updating the frequency in
> the ->enable() callback and using clocksource_register() sounds simple
> and straightforward to me. Ideally I'd like to avoid setting up the
> mult and shift values before registration and instead put all logic in
> the ->enable() callback.
> 
> The ->enable() callback is optional so we will have to deal with
> clocksources without ->enable() callbacks. This should be trivial -
> such drivers can manually setup shift/mult or call
> __clocksource_update_freq() before clocksource_register().

Why not instead register the tmu clocksource with a dummy freq initially
and then update it in ->enable()?

I'd just prefer to not complicate the majority of clocksources just to
handle the few (2) cases where we don't know the frequency at
registration time.

> > The timekeeping code really isn't expecting clocksource values to change
> > under it. So we'll also have to carefully review some of the max_idle_ns
> > users to make sure they can handle the change.
> 
> I may be wrong, but it looks like the clocksource is still unused when
> ->enable() callback is invoked.

Correct, timekeeping core shouldn't use it, but I'm more concerned about
other code possibly having stale values of max_idle_ns immediately after
resume. 

thanks
-john

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[john stultz]

On Thu, 2010-06-10 at 12:27 -0700, john stultz wrote:
> On Thu, 2010-06-10 at 15:20 +0900, Magnus Damm wrote:
> > On Tue, Jun 8, 2010 at 4:28 AM, john stultz <johnstul@us.ibm.com> wrote:
> > > Does that sound reasonable?
> > 
> > I think so. So like I mentioned earlier, we don't know what frequency
> > range is suitable at clock source registration time. So from that
> > point of view I can't really see why we should keep the
> > clocksource_register_hz()/khz() functions. Updating the frequency in
> > the ->enable() callback and using clocksource_register() sounds simple
> > and straightforward to me. Ideally I'd like to avoid setting up the
> > mult and shift values before registration and instead put all logic in
> > the ->enable() callback.
> > 
> > The ->enable() callback is optional so we will have to deal with
> > clocksources without ->enable() callbacks. This should be trivial -
> > such drivers can manually setup shift/mult or call
> > __clocksource_update_freq() before clocksource_register().
> 
> Why not instead register the tmu clocksource with a dummy freq initially
> and then update it in ->enable()?
> 
> I'd just prefer to not complicate the majority of clocksources just to
> handle the few (2) cases where we don't know the frequency at
> registration time.

So I took a rough shot at this. Would you mind checking it out and
seeing if its sufficient?

Its my first two patches from the conversion patchset here:
http://sr71.net/~jstultz/timekeeping/clocksourceregister_hz/patches/

Specifically: 
http://sr71.net/~jstultz/timekeeping/clocksourceregister_hz/patches/0001-Add-__clocksource_updatefreq_hz-khz-methods.patch
http://sr71.net/~jstultz/timekeeping/clocksourceregister_hz/patches/0002-Convert-sh_tmu-sh_cmt-clocksources-to-clocksource_re.patch


Let me know if there's anything I'm missing.

thanks
-john

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[Magnus Damm]

On Fri, Jun 11, 2010 at 4:27 AM, john stultz <johnstul@us.ibm.com> wrote:
> On Thu, 2010-06-10 at 15:20 +0900, Magnus Damm wrote:
>> On Tue, Jun 8, 2010 at 4:28 AM, john stultz <johnstul@us.ibm.com> wrote:
>> > I recently ran into this issue when looking to convert clocksources from
>> > using clocksource_register() to clocksource_register_hz()/khz().
>> >
>> > The point of those patches are to try to remove the mult/shift
>> > calculation from the clocksource driver code, keeping that logic in the
>> > timekeeping core (which will allow for better optimization of other
>> > values like max_idle_ns).
>>
>> This common code for mult/shift calculation is very useful I think.
>>
>> That aside, I have to admit that I don't fully understand which of the
>> _hz()/khz() I'm supposed to use. Say that I have a hardware counter
>> that is in the MHz range, which should I use?
>
> You probably want use the _hz interface if possible. The khz() interface
> is for very high frequency counters (like the TSC) where its possible
> the hz value wouldn't fit in a u32.

Ah, I see. Thanks for the explanation.

>> And for a 4Khz counter?
>
> Again, _hz is probably ideal for precision, but there's no issue using
> the khz value if that's what the platform provides you with (ie: you
> won't get any better precision by multiplying it up).
>
>
>> Isn't it possible to just have a single __clocksource_update_freq()
>> function that checks the range and adjusts the scale? Perhaps there is
>> something that I'm not getting...

I don't understand why you need two separate functions for the
scaling? Perhaps for readability or making the code more determinstic
somehow?

Sorry if this has been discussed before, but to me it seems easier to
just pass the frequency as an u64 and calculate the khz/hz scaling
from that. Although I have to admit that I'm somewhat lost in all the
time calculations, so perhaps my suggestion is impossible...

Thanks,

/ magnus

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[Magnus Damm]

On Fri, Jun 11, 2010 at 12:03 PM, john stultz <johnstul@us.ibm.com> wrote:
> On Thu, 2010-06-10 at 12:27 -0700, john stultz wrote:
>> On Thu, 2010-06-10 at 15:20 +0900, Magnus Damm wrote:
>> > On Tue, Jun 8, 2010 at 4:28 AM, john stultz <johnstul@us.ibm.com> wrote:
>> > > Does that sound reasonable?
>> >
>> > I think so. So like I mentioned earlier, we don't know what frequency
>> > range is suitable at clock source registration time. So from that
>> > point of view I can't really see why we should keep the
>> > clocksource_register_hz()/khz() functions. Updating the frequency in
>> > the ->enable() callback and using clocksource_register() sounds simple
>> > and straightforward to me. Ideally I'd like to avoid setting up the
>> > mult and shift values before registration and instead put all logic in
>> > the ->enable() callback.
>> >
>> > The ->enable() callback is optional so we will have to deal with
>> > clocksources without ->enable() callbacks. This should be trivial -
>> > such drivers can manually setup shift/mult or call
>> > __clocksource_update_freq() before clocksource_register().
>>
>> Why not instead register the tmu clocksource with a dummy freq initially
>> and then update it in ->enable()?

Sure, that's doable, no problem. If that's what you prefer then that's fine.

I do however think that the interface would be cleaner without an
initial dummy frequency.

Is there anything that actually needs the frequency at registration time?

I may be wrong, but wouldn't the same thing be accomplished by keeping
clocksource_register() as-is, use
__clocksource_updatefreq_hz() before clocksource_register() for
drivers that doesn't use ->enable(), and on top of all this extend
timekeeper_setup_internals() to include the cs->max_idle_ns
calculation?

>> I'd just prefer to not complicate the majority of clocksources just to
>> handle the few (2) cases where we don't know the frequency at
>> registration time.

Sure, I can understand that.

> So I took a rough shot at this. Would you mind checking it out and
> seeing if its sufficient?
>
> Its my first two patches from the conversion patchset here:
> http://sr71.net/~jstultz/timekeeping/clocksourceregister_hz/patches/

Thanks for your work on this!

> Specifically:
> http://sr71.net/~jstultz/timekeeping/clocksourceregister_hz/patches/0001-Add-__clocksource_updatefreq_hz-khz-methods.patch

The patch above looks good, but if you decide to update then you may
want to spellcheck the comment in the last hunk:
s/clcoksource/clocksource/g

> http://sr71.net/~jstultz/timekeeping/clocksourceregister_hz/patches/0002-Convert-sh_tmu-sh_cmt-clocksources-to-clocksource_re.patch
>
> Let me know if there's anything I'm missing.

For the CMT driver I think it's best to begin with reverting the
commit f4d7c3565c1692c54d9152b52090fe73f0029e37 and then add your
changes on top of that.

From my point of view we really don't want to have the unused
clk_enable((), clk_get_rate(), clk_disable() hackery left in the
drivers.

Perhaps the same thing should be done for the TMU as well, any ideas Paul?

I agree with adding a __clocksource_updatefreq_hz(cs, p->rate) line to
sh_cmt_clocksource_enable(), but the p->rate value should already be
up to date so I don't think you need to add any clk_get_rate() call.

Want me to hack up a replacement patch?

Thanks,

/ magnus

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[Paul Mundt]

On Fri, Jun 11, 2010 at 02:02:26PM +0900, Magnus Damm wrote:
> For the CMT driver I think it's best to begin with reverting the
> commit f4d7c3565c1692c54d9152b52090fe73f0029e37 and then add your
> changes on top of that.
> 
> From my point of view we really don't want to have the unused
> clk_enable((), clk_get_rate(), clk_disable() hackery left in the
> drivers.
> 
At the moment the enable/get_rate/disable sequence isn't unused, it's
just not terribly desirable. As soon as we have a better way around it
then of course killing that off would be ideal. We only got in to this
mess in the first place due to the registration/enabling ordering.

> Perhaps the same thing should be done for the TMU as well, any ideas Paul?
> 
Yes, the same can be carried over there. Once we have a solution that
forks for the CMT and TMU case then we'll also want to use it for MTU2.
The main reason for MTU2 not having the same interface initially was
simply because none of the SH-2A parts provided sensible clock
definitions for MTU2, and mostly tended to prefer the CMT for wakeups
anyways.

> I agree with adding a __clocksource_updatefreq_hz(cs, p->rate) line to
> sh_cmt_clocksource_enable(), but the p->rate value should already be
> up to date so I don't think you need to add any clk_get_rate() call.
> 
> Want me to hack up a replacement patch?
> 
This all started out from Aurelien's bug report, so as long as that
remains addressed I'm pretty flexible with how we handle the
registration/enable case. 

Note that the clock framework is initialized before the early timer is,
but some boards might still have unusual ordering due to the propagation
of their initial XTAL values.

Re: [PATCH v2] sh_tmu: compute mult and shift before registration

[Aurelien Jarno]

On Fri, Jun 11, 2010 at 03:01:51PM +0900, Paul Mundt wrote:
> > I agree with adding a __clocksource_updatefreq_hz(cs, p->rate) line to
> > sh_cmt_clocksource_enable(), but the p->rate value should already be
> > up to date so I don't think you need to add any clk_get_rate() call.
> > 
> > Want me to hack up a replacement patch?
> > 
> This all started out from Aurelien's bug report, so as long as that
> remains addressed I'm pretty flexible with how we handle the
> registration/enable case. 
> 

I am ready to test new patches if needed.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net