This moves the clocksource_set_clock() and clockevent_set_clock()
from the MIPS timer code into clockchips and clocksource where
it belongs. The patch was triggered by code posted by Mikael
Pettersson duplicating this code for the IOP ARM system. The
function signatures where altered slightly to fit into their
destination header files, unsigned int changed to u32 and inlined.
Signed-off-by: Linus Walleij <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Mikael Pettersson <[email protected]>
Cc: Ralf Baechle <[email protected]>
---
Ralf has stated in earlier conversation that this should be moved,
now we risk duplicating code so let's move it.
I don't have access to a MIPS cross-compiler so please can the
MIPS people test this?
Can you test it on the IOP too, Mikael?
---
arch/mips/include/asm/time.h | 4 ----
arch/mips/kernel/time.c | 33 ---------------------------------
include/linux/clockchips.h | 24 ++++++++++++++++++++++++
include/linux/clocksource.h | 24 +++++++++++++++++++++++-
4 files changed, 47 insertions(+), 38 deletions(-)
diff --git a/arch/mips/include/asm/time.h b/arch/mips/include/asm/time.h
index df6a430..e9b3f92 100644
--- a/arch/mips/include/asm/time.h
+++ b/arch/mips/include/asm/time.h
@@ -84,8 +84,4 @@ static inline int init_mips_clocksource(void)
#endif
}
-extern void clocksource_set_clock(struct clocksource *cs, unsigned int clock);
-extern void clockevent_set_clock(struct clock_event_device *cd,
- unsigned int clock);
-
#endif /* _ASM_TIME_H */
diff --git a/arch/mips/kernel/time.c b/arch/mips/kernel/time.c
index 1f467d5..fb74974 100644
--- a/arch/mips/kernel/time.c
+++ b/arch/mips/kernel/time.c
@@ -71,39 +71,6 @@ EXPORT_SYMBOL(perf_irq);
unsigned int mips_hpt_frequency;
-void __init clocksource_set_clock(struct clocksource *cs, unsigned int clock)
-{
- u64 temp;
- u32 shift;
-
- /* Find a shift value */
- for (shift = 32; shift > 0; shift--) {
- temp = (u64) NSEC_PER_SEC << shift;
- do_div(temp, clock);
- if ((temp >> 32) == 0)
- break;
- }
- cs->shift = shift;
- cs->mult = (u32) temp;
-}
-
-void __cpuinit clockevent_set_clock(struct clock_event_device *cd,
- unsigned int clock)
-{
- u64 temp;
- u32 shift;
-
- /* Find a shift value */
- for (shift = 32; shift > 0; shift--) {
- temp = (u64) clock << shift;
- do_div(temp, NSEC_PER_SEC);
- if ((temp >> 32) == 0)
- break;
- }
- cd->shift = shift;
- cd->mult = (u32) temp;
-}
-
/*
* This function exists in order to cause an error due to a duplicate
* definition if platform code should have its own implementation. The hook
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 3a1dbba..41aa95e 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -115,6 +115,30 @@ static inline unsigned long div_sc(unsigned long ticks, unsigned long nsec,
return (unsigned long) tmp;
}
+/**
+ * clockevent_set_clock - dynamically calculates an appropriate shift
+ * and mult value for a clocksource given a
+ * known clock frequency
+ * @dev: Clockevent device to initialize
+ * @hz: Clockevent clock frequency in Hz
+ */
+static inline void clockevent_set_clock(struct clock_event_device *dev, u32 hz)
+{
+ u64 temp;
+ u32 shift;
+
+ /* Find a shift value */
+ for (shift = 32; shift > 0; shift--) {
+ temp = (u64) hz << shift;
+ do_div(temp, NSEC_PER_SEC);
+ if ((temp >> 32) == 0)
+ break;
+ }
+ dev->shift = shift;
+ dev->mult = (u32) temp;
+}
+
+
/* Clock event layer functions */
extern unsigned long clockevent_delta2ns(unsigned long latch,
struct clock_event_device *evt);
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 9ea40ff..807fb37 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -257,6 +257,29 @@ static inline u32 clocksource_hz2mult(u32 hz, u32 shift_constant)
}
/**
+ * clocksource_set_clock - dynamically calculates an appropriate shift
+ * and mult value for a clocksource given a
+ * known clock frequency
+ * @cs: Clocksource to initialize
+ * @hz: Clocksource frequency in Hz
+ */
+static inline void clocksource_set_clock(struct clocksource *cs, u32 hz)
+{
+ u64 temp;
+ u32 shift;
+
+ /* Find a shift value */
+ for (shift = 32; shift > 0; shift--) {
+ temp = (u64) NSEC_PER_SEC << shift;
+ do_div(temp, hz);
+ if ((temp >> 32) == 0)
+ break;
+ }
+ cs->shift = shift;
+ cs->mult = (u32) temp;
+}
+
+/**
* clocksource_cyc2ns - converts clocksource cycles to nanoseconds
*
* Converts cycles to nanoseconds, using the given mult and shift.
@@ -268,7 +291,6 @@ static inline s64 clocksource_cyc2ns(cycle_t cycles, u32 mult, u32 shift)
return ((u64) cycles * mult) >> shift;
}
-
/* used to install a new clocksource */
extern int clocksource_register(struct clocksource*);
extern void clocksource_unregister(struct clocksource*);
--
1.6.2.5
Linus Walleij writes:
> This moves the clocksource_set_clock() and clockevent_set_clock()
> from the MIPS timer code into clockchips and clocksource where
> it belongs. The patch was triggered by code posted by Mikael
> Pettersson duplicating this code for the IOP ARM system. The
> function signatures where altered slightly to fit into their
> destination header files, unsigned int changed to u32 and inlined.
>
> Signed-off-by: Linus Walleij <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Mikael Pettersson <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> ---
> Ralf has stated in earlier conversation that this should be moved,
> now we risk duplicating code so let's move it.
>
> I don't have access to a MIPS cross-compiler so please can the
> MIPS people test this?
>
> Can you test it on the IOP too, Mikael?
Changing my ARM IOP clock code to use these now shared functions
was easy, and I get the same shift/mult values as I got before. So:
Tested-by: Mikael Pettersson <[email protected]>
A few tiny comments about the patch follow below.
> --- a/include/linux/clockchips.h
> +++ b/include/linux/clockchips.h
> @@ -115,6 +115,30 @@ static inline unsigned long div_sc(unsigned long ticks, unsigned long nsec,
> return (unsigned long) tmp;
> }
>
> +/**
> + * clockevent_set_clock - dynamically calculates an appropriate shift
> + * and mult value for a clocksource given a
drop "dynamically"
"calculates appropriate shift and mult values" ?
s/clocksource/clockevent/
> + * known clock frequency
> + * @dev: Clockevent device to initialize
> + * @hz: Clockevent clock frequency in Hz
> + */
> +static inline void clockevent_set_clock(struct clock_event_device *dev, u32 hz)
> +{
> + u64 temp;
> + u32 shift;
> +
> + /* Find a shift value */
This comment is inaccurate. It should say "Find shift and mult values",
or you could remove it and rely on the comment above the function
definition to document the intended behaviour.
> + for (shift = 32; shift > 0; shift--) {
> + temp = (u64) hz << shift;
> + do_div(temp, NSEC_PER_SEC);
> + if ((temp >> 32) == 0)
> + break;
> + }
> + dev->shift = shift;
> + dev->mult = (u32) temp;
> +}
> +
> +
Two empty lines?
> /* Clock event layer functions */
> extern unsigned long clockevent_delta2ns(unsigned long latch,
> struct clock_event_device *evt);
> diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
> index 9ea40ff..807fb37 100644
> --- a/include/linux/clocksource.h
> +++ b/include/linux/clocksource.h
> @@ -257,6 +257,29 @@ static inline u32 clocksource_hz2mult(u32 hz, u32 shift_constant)
> }
>
> /**
> + * clocksource_set_clock - dynamically calculates an appropriate shift
> + * and mult value for a clocksource given a
drop "dynamically"
"calculates appropriate shift and mult values" ?
> + * known clock frequency
> + * @cs: Clocksource to initialize
> + * @hz: Clocksource frequency in Hz
> + */
> +static inline void clocksource_set_clock(struct clocksource *cs, u32 hz)
> +{
> + u64 temp;
> + u32 shift;
> +
> + /* Find a shift value */
Same issue as with the comment in clockevent_set_clock().
On Sun, Oct 18, 2009 at 12:48:35AM +0200, Linus Walleij wrote:
Reviewed-by: Ralf Baechle <[email protected]>
Ralf
On Sun, 18 Oct 2009, Linus Walleij wrote:
> This moves the clocksource_set_clock() and clockevent_set_clock()
> from the MIPS timer code into clockchips and clocksource where
> it belongs. The patch was triggered by code posted by Mikael
> Pettersson duplicating this code for the IOP ARM system. The
> function signatures where altered slightly to fit into their
> destination header files, unsigned int changed to u32 and inlined.
>
> Signed-off-by: Linus Walleij <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Mikael Pettersson <[email protected]>
> Cc: Ralf Baechle <[email protected]>
> ---
> Ralf has stated in earlier conversation that this should be moved,
> now we risk duplicating code so let's move it.
Please do not make that functions inline. They are too large and there
is no benefit of inlining them.
> +/**
> + * clockevent_set_clock - dynamically calculates an appropriate shift
> + * and mult value for a clocksource given a
> + * known clock frequency
> + * @dev: Clockevent device to initialize
> + * @hz: Clockevent clock frequency in Hz
> + */
> +static inline void clockevent_set_clock(struct clock_event_device *dev, u32 hz)
> +{
> + u64 temp;
> + u32 shift;
> +
> + /* Find a shift value */
> + for (shift = 32; shift > 0; shift--) {
> + temp = (u64) hz << shift;
> + do_div(temp, NSEC_PER_SEC);
> + if ((temp >> 32) == 0)
> + break;
> + }
> + dev->shift = shift;
> + dev->mult = (u32) temp;
> +}
> +
> +
> +static inline void clocksource_set_clock(struct clocksource *cs, u32 hz)
> +{
> + u64 temp;
> + u32 shift;
> +
> + /* Find a shift value */
> + for (shift = 32; shift > 0; shift--) {
> + temp = (u64) NSEC_PER_SEC << shift;
> + do_div(temp, hz);
> + if ((temp >> 32) == 0)
> + break;
> + }
> + cs->shift = shift;
> + cs->mult = (u32) temp;
> +}
> +
So that's the same function twice, right ? The reason for it are the
different data structures. So could you please do something like:
void calc_shift_mult(u32 *shift, u32 *mult, u32 hz)
{
do the magic math
}
and have wrapper inline functions for clocksource and clockevents
around it.
Thanks,
tglx
Thomas Gleixner writes:
> > +static inline void clockevent_set_clock(struct clock_event_device *dev, u32 hz)
> > +{
> > + u64 temp;
> > + u32 shift;
> > +
> > + /* Find a shift value */
> > + for (shift = 32; shift > 0; shift--) {
> > + temp = (u64) hz << shift;
> > + do_div(temp, NSEC_PER_SEC);
> > + if ((temp >> 32) == 0)
> > + break;
> > + }
> > + dev->shift = shift;
> > + dev->mult = (u32) temp;
> > +}
> > +
> > +
> > +static inline void clocksource_set_clock(struct clocksource *cs, u32 hz)
> > +{
> > + u64 temp;
> > + u32 shift;
> > +
> > + /* Find a shift value */
> > + for (shift = 32; shift > 0; shift--) {
> > + temp = (u64) NSEC_PER_SEC << shift;
> > + do_div(temp, hz);
> > + if ((temp >> 32) == 0)
> > + break;
> > + }
> > + cs->shift = shift;
> > + cs->mult = (u32) temp;
> > +}
> > +
>
> So that's the same function twice, right ?
They are similar but not identical:
> > + temp = (u64) hz << shift;
> > + do_div(temp, NSEC_PER_SEC);
vs
> > + temp = (u64) NSEC_PER_SEC << shift;
> > + do_div(temp, hz);
I suppose both functions could be implemented by a suitably
parametric third function, but IMO that would just obscure things.
Making them non-inline I fully agree with.
2009/10/20 Thomas Gleixner <[email protected]>:
> On Sun, 18 Oct 2009, Linus Walleij wrote:
>> This moves the clocksource_set_clock() and clockevent_set_clock()
>> from the MIPS timer code into clockchips and clocksource where
>> it belongs.
> (...)
> Please do not make that functions inline. They are too large and there
> is no benefit of inlining them.
I think there is, because these functions in MIPS if I'm not misreading it, are
used once each in one and one spot only in the clocksource/clockevent
set-up at boot time for each platform in which they are currently used.
Further these spots tend to be __init or even __cpuinit code segments,
so they will be discarded and if we inline the code it will be discarded
after boot as well.
If you foresee that the code will be used in other ways under other
circumstances, I will make them non-inlined of course, but I have a
hard time seeing that.
OK?
Linus Walleij
Thomas Gleixner wrote:
> On Sun, 18 Oct 2009, Linus Walleij wrote:
>> This moves the clocksource_set_clock() and clockevent_set_clock()
>> from the MIPS timer code into clockchips and clocksource where
>> it belongs. The patch was triggered by code posted by Mikael
>> Pettersson duplicating this code for the IOP ARM system. The
>> function signatures where altered slightly to fit into their
>> destination header files, unsigned int changed to u32 and inlined.
>>
>> Signed-off-by: Linus Walleij <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Mikael Pettersson <[email protected]>
>> Cc: Ralf Baechle <[email protected]>
>> ---
>> Ralf has stated in earlier conversation that this should be moved,
>> now we risk duplicating code so let's move it.
>
> Please do not make that functions inline. They are too large and there
> is no benefit of inlining them.
>
If that is the case, then perhaps they should not be defined in a header
file.
IMHO if you are defining a function in a header file it should always be
'static inline'. If you don't want it in-lined, put it in some
library so we only pick up a single instance of it.
David Daney
2009/10/20 David Daney <[email protected]>:
>> Please do not make that functions inline. They are too large and there
>> is no benefit of inlining them.
>
> If that is the case, then perhaps they should not be defined in a header
> file.
Of course not. There are apropriate places to put them, but as stated
I think the use as it is today warrants having them inlined.
Linus Walleij
Linus Walleij wrote:
> 2009/10/20 David Daney <[email protected]>:
>
>>> Please do not make that functions inline. They are too large and there
>>> is no benefit of inlining them.
>> If that is the case, then perhaps they should not be defined in a header
>> file.
>
> Of course not. There are apropriate places to put them, but as stated
> I think the use as it is today warrants having them inlined.
>
I wasn't trying to imply that they shouldn't be inlined, only that if
you choose (as you did) to define them in a header file, that they
*should* be inline.
David Daney