x86: apic - unify APIC_DIVISOR

Use APIC_DIVISOR being set to 16 for both 32/64bit mode. To escape APIC timer underflow during calibration set it to the maximum possible value.

Also typo error (CONFG instead of proper CONFIG) fixed. The error was catched by Venkatesh Pallipadi, thanks a lot Venkatesh! See details on http://lkml.org/lkml/2008/10/9/425

---

Index: linux-2.6.git/arch/x86/kernel/apic.c
===================================================================
@@ -332,11 +332,7 @@ int lapic_get_maxlvt(void)
  */
 
 /* Clock divisor */
-#ifdef CONFG_X86_64
-#define APIC_DIVISOR 1
-#else
 #define APIC_DIVISOR 16
-#endif
 
 /*
  * This function sets up the local APIC timer, with a timeout of

@@ -592,10 +588,10 @@ static int __init calibrate_APIC_clock(v
 	global_clock_event->event_handler = lapic_cal_handler;
 
 	/*
-	 * Setup the APIC counter to 1e9. There is no way the lapic
+	 * Setup the APIC counter to maximum. There is no way the lapic
 	 * can underflow in the 100ms detection time frame
 	 */
-	__setup_APIC_LVTT(1000000000, 0, 0);
+	__setup_APIC_LVTT(0xffffffff, 0, 0);
 
 	/* Let the interrupts run */
 	local_irq_enable();

applied to tip/irq/sparseirq (due to lapic unification), thanks Cyrill.

( Maciej: i've added your Acked-by based on your agreement to this
approach earlier in the thread - let me know if that does not hold for this patch. )

Ingo

Maciej

[Pallipadi, Venkatesh - Fri, Oct 10, 2008 at 09:11:06AM -0700]

| 
| 
| >-----Original Message-----
| >From: Cyrill Gorcunov [mailto:[email protected]]
| >Sent: Friday, October 10, 2008 8:00 AM
| >To: Ingo Molnar; Maciej W. Rozycki
| >Cc: LKML; Pallipadi, Venkatesh
| >Subject: [PATCH] x86: apic - unify APIC_DIVISOR
| >
| >Use APIC_DIVISOR being set to 16 for both 32/64bit
| >mode. To escape APIC timer underflow during calibration
| >set it to the maximum possible value.
| >
| >Also typo error (CONFG instead of proper CONFIG) fixed.
| >The error was catched by Venkatesh Pallipadi, thanks a lot Venkatesh!
| >See details on http://lkml.org/lkml/2008/10/9/425
| >
| >Reported-by: Venkatesh Pallipad <[email protected]>
| >Signed-off-by: Cyrill Gorcunov <[email protected]>
| >---
| >
| >Index: linux-2.6.git/arch/x86/kernel/apic.c
| >===================================================================
| >--- linux-2.6.git.orig/arch/x86/kernel/apic.c   2008-09-26
| >20:43:47.000000000 +0400
| >+++ linux-2.6.git/arch/x86/kernel/apic.c        2008-10-10
| >16:37:26.000000000 +0400
| >@@ -332,11 +332,7 @@ int lapic_get_maxlvt(void)
| >  */
| >
| > /* Clock divisor */
| >-#ifdef CONFG_X86_64
| >-#define APIC_DIVISOR 1
| >-#else
| > #define APIC_DIVISOR 16
| >-#endif
| >
| > /*
| >  * This function sets up the local APIC timer, with a timeout of
| >@@ -592,10 +588,10 @@ static int __init calibrate_APIC_clock(v
| >        global_clock_event->event_handler = lapic_cal_handler;
| >
| >        /*
| >-        * Setup the APIC counter to 1e9. There is no way the lapic
| >+        * Setup the APIC counter to maximum. There is no way the lapic
| >         * can underflow in the 100ms detection time frame
| >         */
| >-       __setup_APIC_LVTT(1000000000, 0, 0);
| >+       __setup_APIC_LVTT(0xffffffff, 0, 0);
| >
| >        /* Let the interrupts run */
| >        local_irq_enable();
| >
| 
| Agree with the APIC_DIVISOR part.
| 
| But, not sure why/how the second change is related to this APIC_DIVISOR being 16.
| Also, another nit. Technically we are not setting the "APIC counter to maximum"
| as we do divide by 16 before programming initial count register in __setup_APIC_LVTT().
| 
| Thanks,
| Venki
| 

set maximum possible value. How you could make it bigger? (without additional changes _inside_ __setup_APIC_LVTT itself). Actually I wouldn't mind if you fix the comment if you don't like this 'correlation' btw CLKs divisor and APIC_DIVISOR. No problem :)

- Cyrill -

The basic question is why are we making this change now? Is the old value breaking some system today? Or Is it not to break some future system with very high bus clock freq? If we are doing it to future proof things, we should be making more changes inside __setup_APIC_LVTT and make this maximum possible value..

No problem. I can send a separate patch to change this calibration count to maximum once I understand why exactly we are doing it now :).

Thanks,
Venki

[Pallipadi, Venkatesh - Fri, Oct 10, 2008 at 10:07:38AM -0700]

| 
| 
| >-----Original Message-----
| >From: Cyrill Gorcunov [mailto:[email protected]]
| >Sent: Friday, October 10, 2008 9:31 AM
| >To: Pallipadi, Venkatesh
| >Cc: Ingo Molnar; Maciej W. Rozycki; LKML
| >Subject: Re: [PATCH] x86: apic - unify APIC_DIVISOR
| >
| >[Pallipadi, Venkatesh - Fri, Oct 10, 2008 at 09:11:06AM -0700]
| >|
| >|
| >| >-----Original Message-----
| >| >From: Cyrill Gorcunov [mailto:[email protected]]
| >| >Sent: Friday, October 10, 2008 8:00 AM
| >| >To: Ingo Molnar; Maciej W. Rozycki
| >| >Cc: LKML; Pallipadi, Venkatesh
| >| >Subject: [PATCH] x86: apic - unify APIC_DIVISOR
| >| >
| >| >Use APIC_DIVISOR being set to 16 for both 32/64bit
| >| >mode. To escape APIC timer underflow during calibration
| >| >set it to the maximum possible value.
| >| >
| >| >Also typo error (CONFG instead of proper CONFIG) fixed.
| >| >The error was catched by Venkatesh Pallipadi, thanks a lot
| >Venkatesh!
| >| >See details on http://lkml.org/lkml/2008/10/9/425
| >| >
| >| >Reported-by: Venkatesh Pallipad <[email protected]>
| >| >Signed-off-by: Cyrill Gorcunov <[email protected]>
| >| >---
| >| >
| >| >Index: linux-2.6.git/arch/x86/kernel/apic.c
| >| >===================================================================
| >| >--- linux-2.6.git.orig/arch/x86/kernel/apic.c   2008-09-26
| >| >20:43:47.000000000 +0400
| >| >+++ linux-2.6.git/arch/x86/kernel/apic.c        2008-10-10
| >| >16:37:26.000000000 +0400
| >| >@@ -332,11 +332,7 @@ int lapic_get_maxlvt(void)
| >| >  */
| >| >
| >| > /* Clock divisor */
| >| >-#ifdef CONFG_X86_64
| >| >-#define APIC_DIVISOR 1
| >| >-#else
| >| > #define APIC_DIVISOR 16
| >| >-#endif
| >| >
| >| > /*
| >| >  * This function sets up the local APIC timer, with a timeout of
| >| >@@ -592,10 +588,10 @@ static int __init calibrate_APIC_clock(v
| >| >        global_clock_event->event_handler = lapic_cal_handler;
| >| >
| >| >        /*
| >| >-        * Setup the APIC counter to 1e9. There is no way the lapic
| >| >+        * Setup the APIC counter to maximum. There is no
| >way the lapic
| >| >         * can underflow in the 100ms detection time frame
| >| >         */
| >| >-       __setup_APIC_LVTT(1000000000, 0, 0);
| >| >+       __setup_APIC_LVTT(0xffffffff, 0, 0);
| >| >
| >| >        /* Let the interrupts run */
| >| >        local_irq_enable();
| >| >
| >|
| >| Agree with the APIC_DIVISOR part.
| >|
| >| But, not sure why/how the second change is related to this
| >APIC_DIVISOR being 16.
| >| Also, another nit. Technically we are not setting the "APIC
| >counter to maximum"
| >| as we do divide by 16 before programming initial count
| >register in __setup_APIC_LVTT().
| >|
| >| Thanks,
| >| Venki
| >|
| >
| >From __setup_APIC_LVTT(0xffffffff, 0, 0) caller point of view we do
| >set maximum possible value. How you could make it bigger?
| >(without additional changes _inside_ __setup_APIC_LVTT itself).
| 
| 
| The basic question is why are we making this change now? Is the old value
| breaking some system today? Or Is it not to break some future system with
| very high bus clock freq? If we are doing it to future proof things,
| we should be making more changes inside __setup_APIC_LVTT and make
| this maximum possible value..
| 
| >Actually I wouldn't mind if you fix the comment if you don't like
| >this 'correlation' btw CLKs divisor and APIC_DIVISOR. No problem :)
| 
| No problem. I can send a separate patch to change this calibration
| count to maximum once I understand why exactly we are doing it now :).
| 
| Thanks,
| Venki
|

We do unify apic code I would say.

- Cyrill -

If there is no pressing reason to change the initial calibration value, how about the simple patch below.

The 10^9 value that is used for 100 mS calibration time is pretty big as tglx's comment points out. We will only underflow it if there is a bus clock running at 10 GHz * 16 = 160 Ghz.

This way we will not fix something that is not really broken today and will not break in foreseeable future.

Thanks,
Venki

From: Cyrill Gorcunov <[email protected]>
Author: Cyrill Gorcunov <[email protected]>

x86: apic - unify APIC_DIVISOR

Use APIC_DIVISOR being set to 16 for both 32/64bit mode.

Also typo error (CONFG instead of proper CONFIG) fixed. The error was caught by Venkatesh Pallipadi, thanks a lot Venkatesh!

See details on http://lkml.org/lkml/2008/10/9/425

@@ -332,11 +332,7 @@ int lapic_get_maxlvt(void)
  */
 
 /* Clock divisor */
-#ifdef CONFG_X86_64
-#define APIC_DIVISOR 1
-#else
 #define APIC_DIVISOR 16
-#endif
 
 /*
  * This function sets up the local APIC timer, with a timeout of

[Venki Pallipadi - Fri, Oct 10, 2008 at 12:03:50PM -0700]
...

| 
| If there is no pressing reason to change the initial calibration value, how
| about the simple patch below.
| 
| The 10^9 value that is used for 100 mS calibration time is pretty big as
| tglx's comment points out. We will only underflow it if there is a
| bus clock running at 10 GHz * 16 = 160 Ghz.
| 
| This way we will not fix something that is not really broken today and will not
| break in foreseeable future.
| 
| Thanks,
| Venki

I personally have no objection. Thanks Venki. Btw since Ingo already picked the patch from me maybe you could make a patch with numbers you mentoined (ie changelog) and fix 0xffffffff to 10^9?

- Cyrill -

I am fine with this version too. The initial count value used for
calibration is really arbitrary -- for all the systems I have seen the APIC clock was in the sub-MHz range. And 64-bit hardware runs both code variations so the choice of what the 32-bit code version does is appropriate as it is also known to work with 32-bit equipment.

Maciej