clocksource: shift helper

This is a little helper I pulled from the mips tree. I've seen a couple of bogus shift values, and this helper calculates the shift. This should move the shift selection away from the user, and systematically pick the value base on the hz.

I also modified the acpi_pm timer to use this new interface. The original shift was 22 , and after this patch it's increased to 23. Which should make the clocksource slightly more accurate, but shouldn't have much of a visible effect.

...

===================================================================

drivers/clocksource/acpi_pm.c +4 -1

@@ -72,7 +72,7 @@ static struct clocksource clocksource_ac
 	.read		= acpi_pm_read,
 	.mask		= (cycle_t)ACPI_PM_MASK,
 	.mult		= 0, /*to be calculated*/
-	.shift		= 22,
+	.shift		= 0,
 	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
 
 };

@@ -183,6 +183,9 @@ static int __init init_acpi_pm_clocksour
 	if (!pmtmr_ioport)
 		return -ENODEV;
 
+	clocksource_acpi_pm.shift =
+		clocksource_hz2shift(ACPI_PM_BITS, PMTMR_TICKS_PER_SEC);
+
 	clocksource_acpi_pm.mult = clocksource_hz2mult(PMTMR_TICKS_PER_SEC,
 						clocksource_acpi_pm.shift);
 

Index: linux-2.6.25/include/linux/acpi_pmtmr.h
===================================================================

include/linux/acpi_pmtmr.h +3 -2

@@ -7,10 +7,11 @@
 #define PMTMR_TICKS_PER_SEC 3579545
 
 /* limit it to 24 bits */
-#define ACPI_PM_MASK CLOCKSOURCE_MASK(24)
+#define ACPI_PM_BITS	24
+#define ACPI_PM_MASK CLOCKSOURCE_MASK(ACPI_PM_BITS)
 
 /* Overrun value */
-#define ACPI_PM_OVRRUN	(1<<24)
+#define ACPI_PM_OVRRUN	(1 << ACPI_PM_BITS)
 
 #ifdef CONFIG_X86_PM_TIMER
 

Index: linux-2.6.25/include/linux/clocksource.h
===================================================================

include/linux/clocksource.h +9 -0

@@ -157,6 +157,27 @@ static inline u32 clocksource_hz2mult(u3
 }
 
 /**
+ * clocksource_hz2shift - calculates shift from hz and # of bits
+ * @bits:		Number of bits this clocksource uses
+ * @hz:			Clocksource frequency in Hz
+ *
+ * Helper functions that calculates the best shift value
+ * based on the hz and # of bits of any given clock.
+ */
+static inline u32 clocksource_hz2shift(u32 bits, u32 hz)
+{

include/linux/clocksource.h static inline u32 clocksource_hz2mult(u3 · line 178

Quoted 12 lines

include/linux/clocksource.h +12 -0

+static inline u32 clocksource_hz2shift(u32 bits, u32 hz)
+{
+	u64 temp;
+
+	for (; bits > 0; bits--) {
+		temp = (u64) NSEC_PER_SEC << bits;
+		do_div(temp, hz);
+		if ((temp >> 32) == 0)
+			break;
+	}
+	return bits;
+}

If we expect this to have more than one callsite then it would be best to uninline it.

Unless we always expect it to be called from __init code, in which case it's best to inline it ;)

Quoted 5 lines

If we expect this to have more than one callsite then it would be best to uninline it.

Unless we always expect it to be called from __init code, in which case it's best to inline it ;)

I expect it would always get called from __init flagged functions. The clocksource can't get registed/used with out the shift and mult values.

If we did uninline this one, we would have to do the other helpers too. I imagine the use case is the same for all of them ..

This is a little helper I pulled from the mips tree.

Signed-off-by Daniel Walker <[email protected]>

Quoted 1 line · Daniel Walker

This is a little helper I pulled from the mips tree.

...

Quoted 1 line

Signed-off-by Daniel Walker <[email protected]>

And who is the original author of this little helper in the MIPS tree?

Quoted 7 lines · Thomas Gleixner

Quoted 1 line · Daniel Walker

This is a little helper I pulled from the mips tree.

...

Quoted 1 line

Signed-off-by Daniel Walker <[email protected]>

And who is the original author of this little helper in the MIPS tree?

commit 16b7b2ac0148e839da86af8747b6fa4aad43a9b7
Author: Atsushi Nemoto <[email protected]>

Quoted 11 lines · Daniel Walker

Quoted 7 lines · Thomas Gleixner

Quoted 1 line · Daniel Walker

This is a little helper I pulled from the mips tree.

...

Quoted 1 line

Signed-off-by Daniel Walker <[email protected]>

And who is the original author of this little helper in the MIPS tree?

commit 16b7b2ac0148e839da86af8747b6fa4aad43a9b7
Author: Atsushi Nemoto <[email protected]>

Wouldn't it be appropriate to assign the authorship in the usual form i.e.:

From: Atsushi Nemoto <[email protected]>

on top of the patch and keep the original changelog message intact including the Signed-off-by of the author and Ralf ?

Aside of the broken S-O-B chain this is a blatant usurpation of somebody else's work.

Quoted 21 lines · Thomas Gleixner

Quoted 11 lines · Daniel Walker

Quoted 7 lines · Thomas Gleixner

Quoted 1 line · Daniel Walker

This is a little helper I pulled from the mips tree.

...

Quoted 1 line

Signed-off-by Daniel Walker <[email protected]>

And who is the original author of this little helper in the MIPS tree?

commit 16b7b2ac0148e839da86af8747b6fa4aad43a9b7
Author: Atsushi Nemoto <[email protected]>

Wouldn't it be appropriate to assign the authorship in the usual form i.e.:

From: Atsushi Nemoto <[email protected]>

on top of the patch and keep the original changelog message intact including the Signed-off-by of the author and Ralf ?

Quoted 3 lines

Aside of the broken S-O-B chain this is a blatant usurpation of somebody else's work.

I took a function and modified it for my usage, which happens all the time.

And your description led me to the assumption that the code was pulled from the MIPS git tree and not from the MIPS architecture code in mainline. Everyone else I talked to was having the same impression.

I was wrong, but can you understand that your sloppy wording "pulled from the MIPS tree" along with the fact that you just copied code instead of moving it into the generic space and cleanup the MIPS code where it is duplicated from is causing such a misunderstanding ?

I'm fine with the change itself, but it needs to move the code out of MIPS in the first place and not just duplicating code for no good reason.

your sloppy wording "pulled from the MIPS tree"

Quoted 1 line · On Tue, 06 May 2008 14:01:15 -0700 Daniel Walker <[email protected]> wrote:

your sloppy wording "pulled from the MIPS tree"

Let it just be said: the amount of time which is lost due to poor patch descriptions exceeds the amount of time which was saved in not bothering to write good patch descriptions.

That it all.

Quoted 5 lines · Thomas Gleixner

And your description led me to the assumption that the code was pulled from the MIPS git tree and not from the MIPS architecture code in mainline. Everyone else I talked to was having the same impression.

Quoted 4 lines

I was wrong, but can you understand that your sloppy wording "pulled from the MIPS tree" along with the fact that you just copied code instead of moving it into the generic space and cleanup the MIPS code where it is duplicated from is causing such a misunderstanding ?

You could have asked me if it was mainline or not. I can see how words could be misunderstood .

Quoted 3 lines

I'm fine with the change itself, but it needs to move the code out of MIPS in the first place and not just duplicating code for no good reason.

I can work with Atsushi, but I still think the generic version should be merged. We at least would need it in order to modify the mips code.

You - the patch submitter - have to provide useful information to those who review the patches and not the reviewers are supposed to decode your sloppy commit log.

"... which I pulled from the MIPS tree"

Why should I assume that this is from mainline ?

Quoted 6 lines

Quoted 3 lines

I'm fine with the change itself, but it needs to move the code out of MIPS in the first place and not just duplicating code for no good reason.

I can work with Atsushi, but I still think the generic version should be merged. We at least would need it in order to modify the mips code.

You can think what you want, it's not the way it works.

You duplicate code, so the first thing to do is to replace the code which you copied and make sure it is still fully functional. Then you can add a second user to make the point that the generic code move is actually useful. btw, pmtimer is the worst example for a good use case as everything there is constant so we can do it at compile time.

Quoted 5 lines

Quoted 1 line

This is a little helper I pulled from the mips tree.

...

Quoted 1 line

Signed-off-by Daniel Walker <[email protected]>

And who is the original author of this little helper in the MIPS tree?

commit 16b7b2ac0148e839da86af8747b6fa4aad43a9b7
Author: Atsushi Nemoto <[email protected]>

This is a little helper I pulled from the mips tree. I've seen a couple of bogus shift values, and this helper calculates the shift. This should move the shift selection away from the user, and systematically pick the value base on the hz.

I also modified the acpi_pm timer to use this new interface. The original shift was 22 , and after this patch it's increased to 23. Which should make the clocksource slightly more accurate, but shouldn't have much of a visible effect.

Hi,

Quoted 5 lines · Daniel Walker

This is a little helper I pulled from the mips tree. I've seen a couple of bogus shift values, and this helper calculates the shift. This should move the shift selection away from the user, and systematically pick the value base on the hz.

Quoted 4 lines

I also modified the acpi_pm timer to use this new interface. The original shift was 22 , and after this patch it's increased to 23. Which should make the clocksource slightly more accurate, but shouldn't have much of a visible effect.

What is this calculation based on?

Unless I miss something even 22 is far more than enough. acpi_pm has a resolution of 279ns, thus the clock can't be more accurate than half of this on average and increasing the shift doesn't change much directly about it. What the shift value does influence is the size of the adjustment step the clock code can do during an update, so with shift=22 and HZ=100 the possible

adjustment step would be 0.008ns (freq/2^shift/ntp_hz). The problem is that this value is also used for how soon the clock is being adjusted, this means the clock code is constantly busy adjusting for a very small error. So from a clock adjustment perspective a shift value close to log2 (freq/res/10^9/ntp_hz) (with res=1/freq and being limited to 1ns<=res<=1us) would provide sufficient accuracy, while keeping the need for adjustment low, this means with the above example a shift value of much more than 8 doesn't improve accuracy significantly.