Re: [PATCH] x86: apic - unify APIC_DIVISOR

[Venki Pallipadi <venkatesh.pallipadi@intel.com>]

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