Typo in x86 apic.c with DIVISOR setup

Fix a typo in x86 apic.c CONFG_X86_64 -> CONFIG_X86_64. The intention looks like was to have divisor of 1 on x86_64 kernel.

Also, for DIVISOR value of 1 to work, APIC_TDR_DIV should also change depending on divide by 1 or divide by 16 is used.

Signed-off-by Venkatesh Pallipadi <[email protected]>

---
 arch/x86/kernel/apic.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: tip/arch/x86/kernel/apic.c
===================================================================

arch/x86/kernel/apic.c 2008-10-09 17:13:29.000000000 -0700 renamed from arch/x86/kernel/apic.c 2008-10-09 17:12:39.000000000 -0700 +3 -1

@@ -332,10 +332,12 @@ int lapic_get_maxlvt(void)
 */
 
 /* Clock divisor */
-#ifdef CONFG_X86_64
+#ifdef CONFIG_X86_64
 #define APIC_DIVISOR 1
+#define APIC_TDR_DIV APIC_TDR_DIV_1
 #else
 #define APIC_DIVISOR 16
+#define APIC_TDR_DIV APIC_TDR_DIV_16
 #endif
 
 /*

-----Original Message-----
From: Cyrill Gorcunov [mailto:[email protected]]
Sent: Thursday, October 09, 2008 9:38 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H. Peter Anvin; linux-kernel
Subject: Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

Quoted 9 lines · Venki Pallipadi

Fix a typo in x86 apic.c CONFG_X86_64 -> CONFIG_X86_64. The intention looks like was to have divisor of 1 on x86_64 kernel.

Also, for DIVISOR value of 1 to work, APIC_TDR_DIV should also change depending on divide by 1 or divide by 16 is used.

Signed-off-by Venkatesh Pallipadi <[email protected]>

Quoted 7 lines

---
 arch/x86/kernel/apic.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: tip/arch/x86/kernel/apic.c
===================================================================
--- tip.orig/arch/x86/kernel/apic.c 2008-10-09

17:12:39.000000000 -0700

Quoted 16 lines

+++ tip/arch/x86/kernel/apic.c 2008-10-09 17:13:29.000000000 -0700

arch/x86/kernel/apic.c +3 -1

@@ -332,10 +332,12 @@ int lapic_get_maxlvt(void)
 */
 
 /* Clock divisor */
-#ifdef CONFG_X86_64
+#ifdef CONFIG_X86_64
 #define APIC_DIVISOR 1
+#define APIC_TDR_DIV APIC_TDR_DIV_1
 #else
 #define APIC_DIVISOR 16
+#define APIC_TDR_DIV APIC_TDR_DIV_16
 #endif
 
 /*

arch/x86/kernel/apic.c static void __setup_APIC_LVTT(unsigned i · line 373

Quoted 3 lines

arch/x86/kernel/apic.c +0 -0

@@ -369,7 +371,7 @@ static void __setup_APIC_LVTT(unsigned i
       tmp_value = apic_read(APIC_TDCR);
       apic_write(APIC_TDCR,
               (tmp_value & ~(APIC_TDR_DIV_1 |

APIC_TDR_DIV_TMBASE)) |

arch/x86/kernel/apic.c static void __setup_APIC_LVTT(unsigned i · line 377

Quoted 6 lines

arch/x86/kernel/apic.c +1 -1

-               APIC_TDR_DIV_16);
+               APIC_TDR_DIV);
 
       if (!oneshot)
               apic_write(APIC_TMICT, clocks / APIC_DIVISOR);

Hi Venki,

you know I don't have code under my hands now to take a look on (i'm in office now) -- but if I recall correctly it's not a typo :) By writting (1) instead of (16) you make counter decrease faster by 16 times (if to compare with what we have now). So we could get counter underflow before we've finish calibration. That is was the original intension of this 16 divisor being used. Did you test this patch?

arch/x86/kernel/apic.c int lapic_get_maxlvt(void) · line 335

Quoted 2 lines

arch/x86/kernel/apic.c +1 -1

-#ifdef CONFG_X86_64
+#ifdef CONFIG_X86_64

Quoted 57 lines

-----Original Message-----
From: Cyrill Gorcunov [mailto:[email protected]]
Sent: Thursday, October 09, 2008 9:38 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H. Peter Anvin; linux-kernel
Subject: Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

Quoted 9 lines · Venki Pallipadi

Fix a typo in x86 apic.c CONFG_X86_64 -> CONFIG_X86_64. The intention looks like was to have divisor of 1 on x86_64 kernel.

Also, for DIVISOR value of 1 to work, APIC_TDR_DIV should also change depending on divide by 1 or divide by 16 is used.

Signed-off-by Venkatesh Pallipadi <[email protected]>

Quoted 7 lines

---
 arch/x86/kernel/apic.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: tip/arch/x86/kernel/apic.c
===================================================================
--- tip.orig/arch/x86/kernel/apic.c 2008-10-09

17:12:39.000000000 -0700

Quoted 16 lines

+++ tip/arch/x86/kernel/apic.c 2008-10-09 17:13:29.000000000 -0700

arch/x86/kernel/apic.c +3 -1

@@ -332,10 +332,12 @@ int lapic_get_maxlvt(void)
 */
 
 /* Clock divisor */
-#ifdef CONFG_X86_64
+#ifdef CONFIG_X86_64
 #define APIC_DIVISOR 1
+#define APIC_TDR_DIV APIC_TDR_DIV_1
 #else
 #define APIC_DIVISOR 16
+#define APIC_TDR_DIV APIC_TDR_DIV_16
 #endif
 
 /*

arch/x86/kernel/apic.c static void __setup_APIC_LVTT(unsigned i · line 373

Quoted 3 lines

arch/x86/kernel/apic.c +0 -0

@@ -369,7 +371,7 @@ static void __setup_APIC_LVTT(unsigned i
       tmp_value = apic_read(APIC_TDCR);
       apic_write(APIC_TDCR,
               (tmp_value & ~(APIC_TDR_DIV_1 |

APIC_TDR_DIV_TMBASE)) |

arch/x86/kernel/apic.c static void __setup_APIC_LVTT(unsigned i · line 377

Quoted 6 lines

arch/x86/kernel/apic.c +1 -1

-               APIC_TDR_DIV_16);
+               APIC_TDR_DIV);
 
       if (!oneshot)
               apic_write(APIC_TMICT, clocks / APIC_DIVISOR);

Hi Venki,

Quoted 8 lines

you know I don't have code under my hands now to take a look on (i'm in office now) -- but if I recall correctly it's not a typo :) By writting (1) instead of (16) you make counter decrease faster by 16 times (if to compare with what we have now). So we could get counter underflow before we've finish calibration. That is was the original intension of this 16 divisor being used. Did you test this patch?

Typo part I mentioned was this

Quoted 2 lines

arch/x86/kernel/apic.c int lapic_get_maxlvt(void) · line 335

Quoted 2 lines

arch/x86/kernel/apic.c +1 -1

-#ifdef CONFG_X86_64
+#ifdef CONFIG_X86_64

Looks like intention was to use DIVISOR of 1 for x86_64 and divisor of 16 for 32 bit kernel. Once I changed that, there was still one place in 64 bit code that was assuming divisor of 16. The rest of the patch changes that part.

Yes. Tested this patch on couple of systems here and worked fine.

Thanks,
Venki

Ah, it was CONFG who is guilty :) This is true indeed, thanks!

But here is not the same

       apic_write(APIC_TDCR,
               (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |

arch/x86/kernel/apic.c +1 -1

-               APIC_TDR_DIV_16);
+               APIC_TDR_DIV);

On x86_64 it will be 1 now but we've used 16 for a long time in purpose to slowdown processor's bus CLKs from APIC point of view. So I don't think it's good idea to change it now. If we start using divisor 1 today -- it would work for some time 'till processor bus raised to the some speed when we'll get counter underflow before calibration finished.

So could you please split the patch into two:
1) Plain CONFG typo fix (which is completely Ack'ed)
2) APIC divisor patch (which I'm not sure if we've to touch)

Quoted 1 line · Cyrill Gorcunov

Ah, it was CONFG who is guilty :) This is true indeed, thanks!

yeah.

Quoted 14 lines

But here is not the same

       apic_write(APIC_TDCR,
               (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |

arch/x86/kernel/apic.c +1 -1

-               APIC_TDR_DIV_16);
+               APIC_TDR_DIV);

On x86_64 it will be 1 now but we've used 16 for a long time in purpose to slowdown processor's bus CLKs from APIC point of view. So I don't think it's good idea to change it now. If we start using divisor 1 today -- it would work for some time 'till processor bus raised to the some speed when we'll get counter underflow before calibration finished.

Quoted 3 lines

So could you please split the patch into two:
1) Plain CONFG typo fix (which is completely Ack'ed)
2) APIC divisor patch (which I'm not sure if we've to touch)

okay, since we typoed that CONFG thing (sidenote: we really need a .config flag that will start a grep that fails the build if there's a non-existent CONFIG option anywhere in the tree), we basically tested the divisor of 16 on a wide range of boxes.

So .. how about just standardizing on the divisor of 16 on both 32-bit and 64-bit?

Ingo

So .. how about just standardizing on the divisor of 16 on both 32-bit and 64-bit?

Quoted 1 line · Cyrill Gorcunov

Ah, it was CONFG who is guilty :) This is true indeed, thanks!

yeah.

Quoted 14 lines

But here is not the same

       apic_write(APIC_TDCR,
               (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |

arch/x86/kernel/apic.c +1 -1

-               APIC_TDR_DIV_16);
+               APIC_TDR_DIV);

On x86_64 it will be 1 now but we've used 16 for a long time in purpose to slowdown processor's bus CLKs from APIC point of view. So I don't think it's good idea to change it now. If we start using divisor 1 today -- it would work for some time 'till processor bus raised to the some speed when we'll get counter underflow before calibration finished.

Quoted 3 lines

So could you please split the patch into two:
1) Plain CONFG typo fix (which is completely Ack'ed)
2) APIC divisor patch (which I'm not sure if we've to touch)

okay, since we typoed that CONFG thing (sidenote: we really need a .config flag that will start a grep that fails the build if there's a non-existent CONFIG option anywhere in the tree), we basically tested the divisor of 16 on a wide range of boxes.

So .. how about just standardizing on the divisor of 16 on both 32-bit and 64-bit?

Ingo

Btw I don't remember but hope it was not me who made this typo --

Quoted 1 line · Cyrill Gorcunov

Btw I don't remember but hope it was not me who made this typo --

What would be the difference so that you hope it wasn't you? All the
people make mistakes, this one has been caught and that's what really matters.

ok, what the conclusion will be on divisor side? I think something like that maybe. Maciej?

-----Original Message-----
From: Cyrill Gorcunov [mailto:[email protected]]
Sent: Thursday, October 09, 2008 10:39 PM
To: Pallipadi, Venkatesh
Cc: Ingo Molnar; Thomas Gleixner; H. Peter Anvin; linux-kernel
Subject: Re: [PATCH] Typo in x86 apic.c with DIVISOR setup

But here is not the same

      apic_write(APIC_TDCR,
              (tmp_value & ~(APIC_TDR_DIV_1 | APIC_TDR_DIV_TMBASE)) |

arch/x86/kernel/apic.c +1 -1

-               APIC_TDR_DIV_16);
+               APIC_TDR_DIV);

On x86_64 it will be 1 now but we've used 16 for a long time in purpose to slowdown processor's bus CLKs from APIC point of view. So I don't think
it's good idea to change it now. If we start using divisor 1 today -- it would
work for some time 'till processor bus raised to the some speed when we'll get counter underflow before calibration finished.

I understand that using 1 instead of 16 makes APIC timer count 16 times faster. I did that change as I thought that was the intention of making APIC_DIVISOR as 1. Otherwise, there is no real impact of APIC_DIVISOR #define from being there. It Is just changing the value of calibration_result across 32 bit and 64 bit kernels.

For example, "host bus clock speed" message will print different values across 32 bit and 64 bit kernels on the same system, if we just make APIC_DIVISOR 16 for 32 bit and 1 for 64 bit kernel.