[PATCH] x86: apic - unify APIC_DIVISOR

[PATCH] x86: apic - unify APIC_DIVISOR

[Cyrill Gorcunov]

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 <venkatesh.pallipadi@intel.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
---

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();

Re: [PATCH] x86: apic - unify APIC_DIVISOR

[Ingo Molnar]

* Cyrill Gorcunov <gorcunov@gmail.com> wrote:

> 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 <venkatesh.pallipadi@intel.com>
> Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>

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

Re: [PATCH] x86: apic - unify APIC_DIVISOR

[Maciej W. Rozycki]

On Fri, 10 Oct 2008, Ingo Molnar wrote:

> ( 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. )

Acked-by: Maciej W. Rozycki <macro@linux-mips.org>

  Maciej

RE: [PATCH] x86: apic - unify APIC_DIVISOR

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Cyrill Gorcunov [mailto:gorcunov@gmail.com]
>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 <venkatesh.pallipadi@intel.com>
>Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>---
>
>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

Re: [PATCH] x86: apic - unify APIC_DIVISOR

[Cyrill Gorcunov]

[Pallipadi, Venkatesh - Fri, Oct 10, 2008 at 09:11:06AM -0700]
| 
| 
| >-----Original Message-----
| >From: Cyrill Gorcunov [mailto:gorcunov@gmail.com]
| >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 <venkatesh.pallipadi@intel.com>
| >Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| >---
| >
| >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).
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 -

RE: [PATCH] x86: apic - unify APIC_DIVISOR

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Cyrill Gorcunov [mailto:gorcunov@gmail.com]
>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:gorcunov@gmail.com]
>| >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 <venkatesh.pallipadi@intel.com>
>| >Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
>| >---
>| >
>| >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

Re: [PATCH] x86: apic - unify APIC_DIVISOR

[Cyrill Gorcunov]

[Pallipadi, Venkatesh - Fri, Oct 10, 2008 at 10:07:38AM -0700]
| 
| 
| >-----Original Message-----
| >From: Cyrill Gorcunov [mailto:gorcunov@gmail.com]
| >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:gorcunov@gmail.com]
| >| >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 <venkatesh.pallipadi@intel.com>
| >| >Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
| >| >---
| >| >
| >| >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 -

Re: [PATCH] x86: apic - unify APIC_DIVISOR

[Venki Pallipadi]

On Fri, Oct 10, 2008 at 10:16:45AM -0700, Cyrill Gorcunov wrote:
> [Pallipadi, Venkatesh - Fri, Oct 10, 2008 at 10:07:38AM -0700]
> |
> |
> | >-----Original Message-----
> | >From: Cyrill Gorcunov [mailto:gorcunov@gmail.com]
> | >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]
> | >|
> | >|
> | >| 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.
> 

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 <gorcunov@gmail.com>
Author: Cyrill Gorcunov <gorcunov@gmail.com>

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

Reported-by: Venkatesh Pallipad <venkatesh.pallipadi@intel.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
Acked-by: "Maciej W. Rozycki" <macro@linux-mips.org>
Acked-by: Venkatesh Pallipadi <Venkatesh Pallipadi@intel.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>

diff --git a/arch/x86/kernel/apic.c b/arch/x86/kernel/apic.c
--- a/arch/x86/kernel/apic.c
+++ b/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

Re: [PATCH] x86: apic - unify APIC_DIVISOR

[Cyrill Gorcunov]

[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 -

Re: [PATCH] x86: apic - unify APIC_DIVISOR

[Maciej W. Rozycki]

On Fri, 10 Oct 2008, Venki Pallipadi wrote:

> 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.

 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