[PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[TAKADA Yoshihito]

Hi. I reported to remove pit_latch_buggy(http://lkml.org/lkml/2007/2/10/8).
In the report, I stated that TSC was unstable.
When I installed 2.6.21, GeodeGX's TSC is stable.
It was fixed by http://bugzilla.kernel.org/show_bug.cgi?id=8027 and follow:

commit 6b3964cde70cfe6db79d35b42137431ef7d2f7e4
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Thu Mar 22 22:46:18 2007 +0100

    [PATCH] i386: clockevents fix breakage on Geode/Cyrix PIT implementations

In 2.6.22, marks unstable GeodeGX's TSC. However, it is not necessary to 
mark TSC is unstable.

Signed-off-by: TAKADA Yoshihito <takada@mbf.nifty.com>

diff -Narup a/arch/i386/kernel/cpu/cyrix.c b/arch/i386/kernel/cpu/cyrix.c
--- a/arch/i386/kernel/cpu/cyrix.c	2007-07-15 16:55:29.000000000 +0900
+++ b/arch/i386/kernel/cpu/cyrix.c	2007-07-15 20:21:11.000000000 +0900
@@ -6,7 +6,6 @@
 #include <asm/io.h>
 #include <asm/processor.h>
 #include <asm/timer.h>
-#include <asm/pci-direct.h>
 #include <asm/tsc.h>
 
 #include "cpu.h"
@@ -252,8 +251,6 @@ static void __cpuinit init_cyrix(struct 
 
 	case 4: /* MediaGX/GXm or Geode GXM/GXLV/GX1 */
 #ifdef CONFIG_PCI
-	{
-		u32 vendor, device;
 		/* It isn't really a PCI quirk directly, but the cure is the
 		   same. The MediaGX has deep magic SMM stuff that handles the
 		   SB emulation. It thows away the fifo on disable_dma() which
@@ -268,21 +265,6 @@ static void __cpuinit init_cyrix(struct 
 
 		printk(KERN_INFO "Working around Cyrix MediaGX virtual DMA bugs.\n");
 		isa_dma_bridge_buggy = 2;
-
-		/* We do this before the PCI layer is running. However we
-		   are safe here as we know the bridge must be a Cyrix
-		   companion and must be present */
-		vendor = read_pci_config_16(0, 0, 0x12, PCI_VENDOR_ID);
-		device = read_pci_config_16(0, 0, 0x12, PCI_DEVICE_ID);
-
-		/*
-		 *  The 5510/5520 companion chips have a funky PIT.
-		 */  
-		if (vendor == PCI_VENDOR_ID_CYRIX &&
-	 (device == PCI_DEVICE_ID_CYRIX_5510 || device == PCI_DEVICE_ID_CYRIX_5520))
-			/*mark_tsc_unstable("cyrix 5510/5520 detected");*/
-			printk("cyrix 5510/5520 detected\n");
-	}
 #endif
 		c->x86_cache_size=16;	/* Yep 16K integrated cache thats it */
 

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Juergen Beisert]

[-- Attachment #1: Type: text/plain, Size: 680 bytes --]

Hi,

On Sunday 15 July 2007 14:00, TAKADA Yoshihito wrote:
> Hi. I reported to remove pit_latch_buggy(http://lkml.org/lkml/2007/2/10/8).
> In the report, I stated that TSC was unstable.
> When I installed 2.6.21, GeodeGX's TSC is stable.

GeodeGX1's TSC is stable until you activate halt/suspend feature. In 
arch/i386/kernel/cpu/cyrix.c:geode_configure() it will be activated by 
default. But due to a macro failure the line
  setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
does nothing.
Replace the setCx86/getCx86 macros by the attached patch, and the TSC will be 
unstable again!

So it makes sense to mark the TSC unstable if the halt/suspend feature is 
activated.

Juergen

[-- Attachment #2: setup_correct_chipset_access_functions.patch --]
[-- Type: text/x-diff, Size: 3554 bytes --]

From: Juergen Beisert <juergen@kreuzholzen.de>

Replace NSC/Cyrix specific chipset access macros by inlined functions.
With the macros a line like this fails (and does nothing):
	setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
With inlined functions this line will work as expected.

Note about a side effect: Seems on Geode GX1 based systems the
"suspend on halt power saving feature" was never enabled due to this
wrong macro expansion. With inlined functions it will be enabled, but
this will stop the TSC when the CPU runs into a HLT instruction.
Kernel output something like this:
	Clocksource tsc unstable (delta = -472746897 ns)

Signed-off-by: Juergen Beisert <juergen@kreuzholzen.de>

Index: include/asm-i386/processor.h
===================================================================
--- include/asm-i386/processor.h.orig
+++ include/asm-i386/processor.h
@@ -168,17 +168,6 @@ static inline void clear_in_cr4 (unsigne
 	write_cr4(cr4);
 }
 
-/*
- *      NSC/Cyrix CPU indexed register access macros
- */
-
-#define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
-
-#define setCx86(reg, data) do { \
-	outb((reg), 0x22); \
-	outb((data), 0x23); \
-} while (0)
-
 /* Stop speculative execution */
 static inline void sync_core(void)
 {
Index: include/asm-i386/processor-cyrix.h
===================================================================
--- /dev/null
+++ include/asm-i386/processor-cyrix.h
@@ -0,0 +1,20 @@
+#ifndef __ASM_I386_PROCESSOR_CYRIX_H
+#define __ASM_I386_PROCESSOR_CYRIX
+
+#include <asm/processor-flags.h>
+/*
+ * NSC/Cyrix CPU indexed register access
+ */
+static inline u8 getCx86(u8 reg)
+{
+	outb(reg, 0x22);
+	return inb(0x23);
+}
+
+static inline void setCx86(u8 reg, u8 data)
+{
+	outb(reg, 0x22);
+	outb(data, 0x23);
+}
+
+#endif
Index: arch/i386/kernel/cpu/cyrix.c
===================================================================
--- arch/i386/kernel/cpu/cyrix.c.orig
+++ arch/i386/kernel/cpu/cyrix.c
@@ -4,7 +4,7 @@
 #include <linux/pci.h>
 #include <asm/dma.h>
 #include <asm/io.h>
-#include <asm/processor.h>
+#include <asm/processor-cyrix.h>
 #include <asm/timer.h>
 #include <asm/pci-direct.h>
 #include <asm/tsc.h>
Index: arch/i386/kernel/cpu/mtrr/cyrix.c
===================================================================
--- arch/i386/kernel/cpu/mtrr/cyrix.c.orig
+++ arch/i386/kernel/cpu/mtrr/cyrix.c
@@ -3,6 +3,7 @@
 #include <asm/mtrr.h>
 #include <asm/msr.h>
 #include <asm/io.h>
+#include <asm/processor-cyrix.h>
 #include "mtrr.h"
 
 int arr3_protected;
Index: arch/i386/kernel/cpu/cpufreq/gx-suspmod.c
===================================================================
--- arch/i386/kernel/cpu/cpufreq/gx-suspmod.c.orig
+++ arch/i386/kernel/cpu/cpufreq/gx-suspmod.c
@@ -79,7 +79,7 @@
 #include <linux/smp.h>
 #include <linux/cpufreq.h>
 #include <linux/pci.h>
-#include <asm/processor.h>
+#include <asm/processor-cyrix.h>
 #include <asm/errno.h>
 
 /* PCI config registers, all at F0 */
Index: include/asm-x86_64/processor.h
===================================================================
--- include/asm-x86_64/processor.h.orig
+++ include/asm-x86_64/processor.h
@@ -391,17 +391,6 @@ static inline void prefetchw(void *x) 
 
 #define cpu_relax()   rep_nop()
 
-/*
- *      NSC/Cyrix CPU indexed register access macros
- */
-
-#define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
-
-#define setCx86(reg, data) do { \
-	outb((reg), 0x22); \
-	outb((data), 0x23); \
-} while (0)
-
 static inline void serialize_cpu(void)
 {
 	__asm__ __volatile__ ("cpuid" : : : "ax", "bx", "cx", "dx");

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[TAKADA Yoshihito]

Hi.
Thanks. you are right. TSC is still unstable.

On Sun, 15 Jul 2007 21:06:27 +0200
Juergen Beisert <juergen127@kreuzholzen.de> wrote:

> Hi,
> 
> On Sunday 15 July 2007 14:00, TAKADA Yoshihito wrote:
> > Hi. I reported to remove pit_latch_buggy(http://lkml.org/lkml/2007/2/10/8).
> > In the report, I stated that TSC was unstable.
> > When I installed 2.6.21, GeodeGX's TSC is stable.
> 
> GeodeGX1's TSC is stable until you activate halt/suspend feature. In 
> arch/i386/kernel/cpu/cyrix.c:geode_configure() it will be activated by 
> default. But due to a macro failure the line
>   setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
> does nothing.
> Replace the setCx86/getCx86 macros by the attached patch, and the TSC will be 
> unstable again!
> 
> So it makes sense to mark the TSC unstable if the halt/suspend feature is 
> activated.
> 
> Juergen

-- 
TAKADA <takada@mbf.nifty.com>

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Andrew Morton]

On Sun, 15 Jul 2007 21:06:27 +0200
Juergen Beisert <juergen127@kreuzholzen.de> wrote:

> Replace NSC/Cyrix specific chipset access macros by inlined functions.
> With the macros a line like this fails (and does nothing):
> 	setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
> With inlined functions this line will work as expected.

I don't get it.  Why would the macros behave differently from inlined
functions?

diff -puN /dev/null include/asm-i386/processor-cyrix.h
> --- /dev/null
> +++ a/include/asm-i386/processor-cyrix.h
> @@ -0,0 +1,20 @@
> +#ifndef __ASM_I386_PROCESSOR_CYRIX_H
> +#define __ASM_I386_PROCESSOR_CYRIX
> +
> +#include <asm/processor-flags.h>
> +/*
> + * NSC/Cyrix CPU indexed register access
> + */
> +static inline u8 getCx86(u8 reg)
> +{
> +	outb(reg, 0x22);
> +	return inb(0x23);
> +}
> +
> +static inline void setCx86(u8 reg, u8 data)
> +{
> +	outb(reg, 0x22);
> +	outb(data, 0x23);
> +}
> +
> +#endif
> diff -puN include/asm-i386/processor.h~i386-geodes-tsc-is-not-neccessary-to-mark-tu-unstable include/asm-i386/processor.h
> --- a/include/asm-i386/processor.h~i386-geodes-tsc-is-not-neccessary-to-mark-tu-unstable
> +++ a/include/asm-i386/processor.h
> @@ -168,17 +168,6 @@ static inline void clear_in_cr4 (unsigne
>  	write_cr4(cr4);
>  }
>  
> -/*
> - *      NSC/Cyrix CPU indexed register access macros
> - */
> -
> -#define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
> -
> -#define setCx86(reg, data) do { \
> -	outb((reg), 0x22); \
> -	outb((data), 0x23); \
> -} while (0)
> -
>  /* Stop speculative execution */
>  static inline void sync_core(void)
>  {
> diff -puN include/asm-x86_64/processor.h~i386-geodes-tsc-is-not-neccessary-to-mark-tu-unstable include/asm-x86_64/processor.h
> --- a/include/asm-x86_64/processor.h~i386-geodes-tsc-is-not-neccessary-to-mark-tu-unstable
> +++ a/include/asm-x86_64/processor.h
> @@ -389,17 +389,6 @@ static inline void prefetchw(void *x) 
>  
>  #define cpu_relax()   rep_nop()
>  
> -/*
> - *      NSC/Cyrix CPU indexed register access macros
> - */
> -
> -#define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
> -
> -#define setCx86(reg, data) do { \
> -	outb((reg), 0x22); \
> -	outb((data), 0x23); \
> -} while (0)
> -
>  static inline void serialize_cpu(void)
>  {
>  	__asm__ __volatile__ ("cpuid" : : : "ax", "bx", "cx", "dx");
>

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Juergen Beisert]

On Thursday 19 July 2007 03:02, Andrew Morton wrote:
> On Sun, 15 Jul 2007 21:06:27 +0200
>
> Juergen Beisert <juergen127@kreuzholzen.de> wrote:
> > Replace NSC/Cyrix specific chipset access macros by inlined functions.
> > With the macros a line like this fails (and does nothing):
> > 	setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
> > With inlined functions this line will work as expected.
>
> I don't get it.  Why would the macros behave differently from inlined
> functions?

X86 magic. The access order is important. The first access must always be the 
offset at 0x22. This access enables the next access to 0x23 (data). If you do 
it in wrong order, it fails. With the macros you get something like 0x22, 
0x22, 0x23, 0x23. With the inline functions 0x22,0x23,0x22,0x23.

Juergen

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Andres Salomon]

On Thu, 19 Jul 2007 08:49:05 +0200
Juergen Beisert <juergen127@kreuzholzen.de> wrote:

> On Thursday 19 July 2007 03:02, Andrew Morton wrote:
> > On Sun, 15 Jul 2007 21:06:27 +0200
> >
> > Juergen Beisert <juergen127@kreuzholzen.de> wrote:
> > > Replace NSC/Cyrix specific chipset access macros by inlined functions.
> > > With the macros a line like this fails (and does nothing):
> > > 	setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
> > > With inlined functions this line will work as expected.
> >
> > I don't get it.  Why would the macros behave differently from inlined
> > functions?
> 
> X86 magic. The access order is important. The first access must always be the 
> offset at 0x22. This access enables the next access to 0x23 (data). If you do 
> it in wrong order, it fails. With the macros you get something like 0x22, 
> 0x22, 0x23, 0x23. With the inline functions 0x22,0x23,0x22,0x23.
> 
> Juergen

Wow, that's a really cool bug; nice work!  Don't forget to update
arch/i386/kernel/cpu/mtrr/state.c, though; it uses setCx86() as well.  It needs
to include processor-cyrix.h.


Acked-by: Andres Salomon <dilinger@debian.org>

-- 
Andres Salomon <dilinger@queued.net>

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Andi Kleen]

> Wow, that's a really cool bug; nice work!  Don't forget to update
> arch/i386/kernel/cpu/mtrr/state.c, though; it uses setCx86() as well.  It needs
> to include processor-cyrix.h.

It also needs some big fat comments

-Andi

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Juergen Beisert]

[-- Attachment #1: Type: text/plain, Size: 1296 bytes --]

On Thursday 19 July 2007 09:17, Andres Salomon wrote:
> On Thu, 19 Jul 2007 08:49:05 +0200
>
> Juergen Beisert <juergen127@kreuzholzen.de> wrote:
> > On Thursday 19 July 2007 03:02, Andrew Morton wrote:
> > > On Sun, 15 Jul 2007 21:06:27 +0200
> > >
> > > Juergen Beisert <juergen127@kreuzholzen.de> wrote:
> > > > Replace NSC/Cyrix specific chipset access macros by inlined
> > > > functions. With the macros a line like this fails (and does nothing):
> > > > 	setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
> > > > With inlined functions this line will work as expected.
> > >
> > > I don't get it.  Why would the macros behave differently from inlined
> > > functions?
> >
> > X86 magic. The access order is important. The first access must always be
> > the offset at 0x22. This access enables the next access to 0x23 (data).
> > If you do it in wrong order, it fails. With the macros you get something
> > like 0x22, 0x22, 0x23, 0x23. With the inline functions
> > 0x22,0x23,0x22,0x23.
> >
> > Juergen
>
> Wow, that's a really cool bug; nice work!  Don't forget to update
> arch/i386/kernel/cpu/mtrr/state.c, though; it uses setCx86() as well.  It
> needs to include processor-cyrix.h.
>
>
> Acked-by: Andres Salomon <dilinger@debian.org>

Missing include patch attached. Thanks.

Juergen



[-- Attachment #2: missing-include.patch --]
[-- Type: text/x-diff, Size: 322 bytes --]

Index: arch/i386/kernel/cpu/mtrr/state.c
===================================================================
--- arch/i386/kernel/cpu/mtrr/state.c
+++ arch/i386/kernel/cpu/mtrr/state.c
@@ -3,6 +3,7 @@
 #include <asm/io.h>
 #include <asm/mtrr.h>
 #include <asm/msr.h>
+#include <asm/processor-cyrix.h>
 #include "mtrr.h"

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Juergen Beisert]

On Thursday 19 July 2007 10:22, Andi Kleen wrote:
> > Wow, that's a really cool bug; nice work!  Don't forget to update
> > arch/i386/kernel/cpu/mtrr/state.c, though; it uses setCx86() as well.  It
> > needs to include processor-cyrix.h.
>
> It also needs some big fat comments

No problem. Where to add?

Juergen

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Andi Kleen]

On Thursday 19 July 2007 10:52:48 Juergen Beisert wrote:
> On Thursday 19 July 2007 10:22, Andi Kleen wrote:
> > > Wow, that's a really cool bug; nice work!  Don't forget to update
> > > arch/i386/kernel/cpu/mtrr/state.c, though; it uses setCx86() as well.  It
> > > needs to include processor-cyrix.h.
> >
> > It also needs some big fat comments
> 
> No problem. Where to add?

Where the inlines are defined.

Also can you please resubmit full patches with full description
and Signed off lines, not incrementals? Thanks.

-Andi

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Juergen Beisert]

[-- Attachment #1: Type: text/plain, Size: 651 bytes --]

Hi Andi,

On Thursday 19 July 2007 11:25, Andi Kleen wrote:
> On Thursday 19 July 2007 10:52:48 Juergen Beisert wrote:
> > On Thursday 19 July 2007 10:22, Andi Kleen wrote:
> > > > Wow, that's a really cool bug; nice work!  Don't forget to update
> > > > arch/i386/kernel/cpu/mtrr/state.c, though; it uses setCx86() as well.
> > > >  It needs to include processor-cyrix.h.
> > >
> > > It also needs some big fat comments
> >
> > No problem. Where to add?
>
> Where the inlines are defined.
>
> Also can you please resubmit full patches with full description
> and Signed off lines, not incrementals? Thanks.

Find attached. Hope its ok now.

Juergen


[-- Attachment #2: setup_correct_chipset_access_functions.patch --]
[-- Type: text/x-diff, Size: 4452 bytes --]

From: Juergen Beisert <juergen@kreuzholzen.de>

Replace NSC/Cyrix specific chipset access macros by inlined functions.
With the macros a line like this fails (and does nothing):
	setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
With inlined functions this line will work as expected.

Note about a side effect: Seems on Geode GX1 based systems the
"suspend on halt power saving feature" was never enabled due to this
wrong macro expansion. With inlined functions it will be enabled, but
this will stop the TSC when the CPU runs into a HLT instruction.
Kernel output something like this:
	Clocksource tsc unstable (delta = -472746897 ns)

This is the 3rd version of this patch.

 - Adding missed arch/i386/kernel/cpu/mtrr/state.c
	Thanks to Andres Salomon
 - Adding some big fat comments into the new header file
 	Suggested by Andi Kleen

Signed-off-by: Juergen Beisert <juergen@kreuzholzen.de>

Index: include/asm-i386/processor.h
===================================================================
--- include/asm-i386/processor.h.orig
+++ include/asm-i386/processor.h
@@ -168,17 +168,6 @@ static inline void clear_in_cr4 (unsigne
 	write_cr4(cr4);
 }
 
-/*
- *      NSC/Cyrix CPU indexed register access macros
- */
-
-#define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
-
-#define setCx86(reg, data) do { \
-	outb((reg), 0x22); \
-	outb((data), 0x23); \
-} while (0)
-
 /* Stop speculative execution */
 static inline void sync_core(void)
 {
Index: include/asm-i386/processor-cyrix.h
===================================================================
--- /dev/null
+++ include/asm-i386/processor-cyrix.h
@@ -0,0 +1,30 @@
+/*
+ * NSC/Cyrix CPU indexed register access. Must be inlined instead of
+ * macros to ensure correct access ordering
+ * Access order is always 0x22 (=offset), 0x23 (=value)
+ *
+ * When using the old macros a line like
+ *   setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
+ * gets expanded to:
+ *  do {
+ *    outb((CX86_CCR2), 0x22);
+ *    outb((({
+ *        outb((CX86_CCR2), 0x22);
+ *        inb(0x23);
+ *    }) | 0x88), 0x23);
+ *  } while (0);
+ *
+ * which in fact violates the access order (= 0x22, 0x22, 0x23, 0x23).
+ */
+
+static inline u8 getCx86(u8 reg)
+{
+	outb(reg, 0x22);
+	return inb(0x23);
+}
+
+static inline void setCx86(u8 reg, u8 data)
+{
+	outb(reg, 0x22);
+	outb(data, 0x23);
+}
Index: arch/i386/kernel/cpu/cyrix.c
===================================================================
--- arch/i386/kernel/cpu/cyrix.c.orig
+++ arch/i386/kernel/cpu/cyrix.c
@@ -4,7 +4,7 @@
 #include <linux/pci.h>
 #include <asm/dma.h>
 #include <asm/io.h>
-#include <asm/processor.h>
+#include <asm/processor-cyrix.h>
 #include <asm/timer.h>
 #include <asm/pci-direct.h>
 #include <asm/tsc.h>
Index: arch/i386/kernel/cpu/mtrr/cyrix.c
===================================================================
--- arch/i386/kernel/cpu/mtrr/cyrix.c.orig
+++ arch/i386/kernel/cpu/mtrr/cyrix.c
@@ -3,6 +3,7 @@
 #include <asm/mtrr.h>
 #include <asm/msr.h>
 #include <asm/io.h>
+#include <asm/processor-cyrix.h>
 #include "mtrr.h"
 
 int arr3_protected;
Index: arch/i386/kernel/cpu/cpufreq/gx-suspmod.c
===================================================================
--- arch/i386/kernel/cpu/cpufreq/gx-suspmod.c.orig
+++ arch/i386/kernel/cpu/cpufreq/gx-suspmod.c
@@ -79,7 +79,7 @@
 #include <linux/smp.h>
 #include <linux/cpufreq.h>
 #include <linux/pci.h>
-#include <asm/processor.h>
+#include <asm/processor-cyrix.h>
 #include <asm/errno.h>
 
 /* PCI config registers, all at F0 */
Index: include/asm-x86_64/processor.h
===================================================================
--- include/asm-x86_64/processor.h.orig
+++ include/asm-x86_64/processor.h
@@ -391,17 +391,6 @@ static inline void prefetchw(void *x)
 
 #define cpu_relax()   rep_nop()
 
-/*
- *      NSC/Cyrix CPU indexed register access macros
- */
-
-#define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
-
-#define setCx86(reg, data) do { \
-	outb((reg), 0x22); \
-	outb((data), 0x23); \
-} while (0)
-
 static inline void serialize_cpu(void)
 {
 	__asm__ __volatile__ ("cpuid" : : : "ax", "bx", "cx", "dx");
Index: arch/i386/kernel/cpu/mtrr/state.c
===================================================================
--- arch/i386/kernel/cpu/mtrr/state.c.orig
+++ arch/i386/kernel/cpu/mtrr/state.c
@@ -3,6 +3,7 @@
 #include <asm/io.h>
 #include <asm/mtrr.h>
 #include <asm/msr.h>
+#include <asm/processor-cyrix.h>
 #include "mtrr.h"
 
 

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Andi Kleen]

Juergen Beisert <juergen127@kreuzholzen.de> writes:

> Hi Andi,
> 
> On Thursday 19 July 2007 11:25, Andi Kleen wrote:
> > On Thursday 19 July 2007 10:52:48 Juergen Beisert wrote:
> > > On Thursday 19 July 2007 10:22, Andi Kleen wrote:
> > > > > Wow, that's a really cool bug; nice work!  Don't forget to update
> > > > > arch/i386/kernel/cpu/mtrr/state.c, though; it uses setCx86() as well.
> > > > >  It needs to include processor-cyrix.h.
> > > >
> > > > It also needs some big fat comments
> > >
> > > No problem. Where to add?
> >
> > Where the inlines are defined.
> >
> > Also can you please resubmit full patches with full description
> > and Signed off lines, not incrementals? Thanks.
> 
> Find attached. Hope its ok now.

For future reference:
- Please send the patches inline if possible
- Generate them from top level (so path start with linux...) 

I fixed it all up

-Andi

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Andrew Morton]

On 19 Jul 2007 15:56:51 +0200
Andi Kleen <andi@firstfloor.org> wrote:

> Juergen Beisert <juergen127@kreuzholzen.de> writes:
> 
> > Hi Andi,
> > 
> > On Thursday 19 July 2007 11:25, Andi Kleen wrote:
> > > On Thursday 19 July 2007 10:52:48 Juergen Beisert wrote:
> > > > On Thursday 19 July 2007 10:22, Andi Kleen wrote:
> > > > > > Wow, that's a really cool bug; nice work!  Don't forget to update
> > > > > > arch/i386/kernel/cpu/mtrr/state.c, though; it uses setCx86() as well.
> > > > > >  It needs to include processor-cyrix.h.
> > > > >
> > > > > It also needs some big fat comments
> > > >
> > > > No problem. Where to add?
> > >
> > > Where the inlines are defined.
> > >
> > > Also can you please resubmit full patches with full description
> > > and Signed off lines, not incrementals? Thanks.
> > 
> > Find attached. Hope its ok now.
> 
> For future reference:
> - Please send the patches inline if possible

+1!

> - Generate them from top level (so path start with linux...) 

+2!

> 
> I fixed it all up

I have a sad little patch from Nick here which adds a new
include/asm-x86_64/processor-cyrix.h.  I guess this patch broke the x86_64
build.

 arch/i386/kernel/cpu/cpufreq/gx-suspmod.c |    2 +-
 arch/i386/kernel/cpu/cyrix.c              |    2 +-
 arch/i386/kernel/cpu/mtrr/cyrix.c         |    1 +
 arch/i386/kernel/cpu/mtrr/state.c         |    1 +
 include/asm-i386/processor-cyrix.h        |   30 ++++++++++++++++++++++++++++++
 include/asm-i386/processor.h              |   11 -----------
 include/asm-x86_64/processor.h            |   11 -----------

Some of those .c files will be used by x86_64 too.

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Andi Kleen]

> > I fixed it all up
> 
> I have a sad little patch from Nick here which adds a new
> include/asm-x86_64/processor-cyrix.h.  I guess this patch broke the x86_64
> build.

I fixed that up too, but in a different way (just included asm-i386/processor-cyrix.h)

-Andi

Re: [PATCH 1/1] i386: Geode's TSC is not neccessary to mark tu unstable

[Satyam Sharma]

> > > On Sun, 15 Jul 2007 21:06:27 +0200
> > > Juergen Beisert <juergen127@kreuzholzen.de> wrote:
> > > > Replace NSC/Cyrix specific chipset access macros by inlined functions.
> > > > With the macros a line like this fails (and does nothing):
> > > >   setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
> > > > With inlined functions this line will work as expected.


> > On Thursday 19 July 2007 03:02, Andrew Morton wrote:
> > >
> > > I don't get it.  Why would the macros behave differently from inlined
> > > functions?


> On Thu, 19 Jul 2007 08:49:05 +0200
> Juergen Beisert <juergen127@kreuzholzen.de> wrote:
> >
> > X86 magic. The access order is important. The first access must always be the
> > offset at 0x22. This access enables the next access to 0x23 (data). If you do
> > it in wrong order, it fails. With the macros you get something like 0x22,
> > 0x22, 0x23, 0x23. With the inline functions 0x22,0x23,0x22,0x23.


On 7/19/07, Andres Salomon <dilinger@queued.net> wrote:
>
> Wow, that's a really cool bug; nice work!  Don't forget to update
> arch/i386/kernel/cpu/mtrr/state.c, though; it uses setCx86() as well.  It needs
> to include processor-cyrix.h.


On 7/19/07, Andi Kleen <ak@suse.de> wrote:
>
> It also needs some big fat comments


Ok, I was discussing macros-in-C on some other thread and got
reminded about this one. Anyway, I don't really think there was
anything weird / surprising about this case at all -- it's just another
manifestation of the same age-old time-tested advise all our respective
grandmothers have always given us:

        Never pass arguments that have side-effects to macros.

Of course, ideally the user shouldn't even know that the API call he's
using is a macro or a function -- which puts the onus upon the person
who *wrote* that API to ensure that he doesn't write macros for what
could, and should, easily be functions. Macros are generally evil and
always horrible, all IMHO, of course.

Satyam