RE: [PATCH] x86: apic - unify APIC_DIVISOR

[Pallipadi, Venkatesh]

>-----Original Message-----
>From: Cyrill Gorcunov [mailto:gorcunov@xxxxxxxxx]
>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@xxxxxxxxx]
>| >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@xxxxxxxxx>
>| >Signed-off-by: Cyrill Gorcunov <gorcunov@xxxxxxxxx>
>| >---
>| >
>| >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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/