Re: [PATCH] [1/1] CPU-i386-Geode: Chipset access macros do not work as expected

Re: [PATCH] [1/1] CPU-i386-Geode: Chipset access macros do not work as expected

[Mikael Pettersson]

On Mon, 30 Apr 2007 15:09:58 +0200, Juergen Beisert 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.

Looks ok, but some cleanups are in order. See below.

> +++ linux-2.6.21/include/asm-i386/processor-cyrix.h
> @@ -0,0 +1,35 @@
> +/*
> + * NSC/Cyrix CPU configuration register indexes
> + */
> +#define CX86_PCR0 0x20
> +#define CX86_GCR  0xb8
> +#define CX86_CCR0 0xc0
> +#define CX86_CCR1 0xc1
> +#define CX86_CCR2 0xc2
> +#define CX86_CCR3 0xc3
> +#define CX86_CCR4 0xe8
> +#define CX86_CCR5 0xe9
> +#define CX86_CCR6 0xea
> +#define CX86_CCR7 0xeb
> +#define CX86_PCR1 0xf0
> +#define CX86_DIR0 0xfe
> +#define CX86_DIR1 0xff
> +#define CX86_ARR_BASE 0xc4
> +#define CX86_RCR_BASE 0xdc
> +
> +/*
> + * NSC/Cyrix CPU indexed register access
> + */
> +static inline u8 getCx86(u8 reg)
> +{
> +	outb(reg,0x22);

----------------^ missing space after comma

> +	return inb(0x23);
> +}
> +
> +static inline void setCx86(u8 reg, u8 data)
> +{
> +	outb(reg,0x22);

----------------^ missing space after comma

> +	outb(data,0x23);

ditto

> +}
> +
> +/* end of file processor-cyrix.h */

Totally redundant comment. Drop it.

/Mikael

Re: [PATCH] [1/1] CPU-i386-Geode: Chipset access macros do not work as expected (2nd try)

[Juergen Beisert]

From: Juergen Beisert <juergen.beisert@weihenstephan.org>

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 outputs something like this:
	Clocksource tsc unstable (delta = -472746897 ns)
Tested on a Geode GX1 system.

This is the second version with some modifications suggested by
Mikael Pettersson

Signed-off-by: Juergen Beisert <juergen.beisert@weihenstephan.org>

Index: linux-2.6.21/include/asm-i386/processor.h
===================================================================
--- linux-2.6.21.orig/include/asm-i386/processor.h
+++ linux-2.6.21/include/asm-i386/processor.h
@@ -202,37 +202,6 @@ static inline void clear_in_cr4 (unsigne
 	write_cr4(cr4);
 }

-/*
- *      NSC/Cyrix CPU configuration register indexes
- */
-
-#define CX86_PCR0 0x20
-#define CX86_GCR  0xb8
-#define CX86_CCR0 0xc0
-#define CX86_CCR1 0xc1
-#define CX86_CCR2 0xc2
-#define CX86_CCR3 0xc3
-#define CX86_CCR4 0xe8
-#define CX86_CCR5 0xe9
-#define CX86_CCR6 0xea
-#define CX86_CCR7 0xeb
-#define CX86_PCR1 0xf0
-#define CX86_DIR0 0xfe
-#define CX86_DIR1 0xff
-#define CX86_ARR_BASE 0xc4
-#define CX86_RCR_BASE 0xdc
-
-/*
- *      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: linux-2.6.21/include/asm-i386/processor-cyrix.h
===================================================================
--- /dev/null
+++ linux-2.6.21/include/asm-i386/processor-cyrix.h
@@ -0,0 +1,33 @@
+/*
+ * NSC/Cyrix CPU configuration register indexes
+ */
+#define CX86_PCR0 0x20
+#define CX86_GCR  0xb8
+#define CX86_CCR0 0xc0
+#define CX86_CCR1 0xc1
+#define CX86_CCR2 0xc2
+#define CX86_CCR3 0xc3
+#define CX86_CCR4 0xe8
+#define CX86_CCR5 0xe9
+#define CX86_CCR6 0xea
+#define CX86_CCR7 0xeb
+#define CX86_PCR1 0xf0
+#define CX86_DIR0 0xfe
+#define CX86_DIR1 0xff
+#define CX86_ARR_BASE 0xc4
+#define CX86_RCR_BASE 0xdc
+
+/*
+ * 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);
+}
Index: linux-2.6.21/arch/i386/kernel/cpu/cyrix.c
===================================================================
--- linux-2.6.21.orig/arch/i386/kernel/cpu/cyrix.c
+++ linux-2.6.21/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>

Index: linux-2.6.21/arch/i386/kernel/cpu/mtrr/cyrix.c
===================================================================
--- linux-2.6.21.orig/arch/i386/kernel/cpu/mtrr/cyrix.c
+++ linux-2.6.21/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;

Re: [PATCH] [1/1] CPU-i386-Geode: Chipset access macros do not work as expected (2nd try)

[Andrew Morton]

On Mon, 30 Apr 2007 17:33:41 +0200
Juergen Beisert <juergen127@kreuzholzen.de> wrote:

> From: Juergen Beisert <juergen.beisert@weihenstephan.org>
> 
> 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 outputs something like this:
> 	Clocksource tsc unstable (delta = -472746897 ns)
> Tested on a Geode GX1 system.
> 
> This is the second version with some modifications suggested by
> Mikael Pettersson
> 
> Signed-off-by: Juergen Beisert <juergen.beisert@weihenstephan.org>
> 
> Index: linux-2.6.21/include/asm-i386/processor.h
> ===================================================================
> --- linux-2.6.21.orig/include/asm-i386/processor.h
> +++ linux-2.6.21/include/asm-i386/processor.h
> @@ -202,37 +202,6 @@ static inline void clear_in_cr4 (unsigne
>  	write_cr4(cr4);
>  }
> 
> -/*
> - *      NSC/Cyrix CPU configuration register indexes
> - */
> -
> -#define CX86_PCR0 0x20
> -#define CX86_GCR  0xb8
> -#define CX86_CCR0 0xc0
> -#define CX86_CCR1 0xc1
> -#define CX86_CCR2 0xc2
> -#define CX86_CCR3 0xc3
> -#define CX86_CCR4 0xe8
> -#define CX86_CCR5 0xe9
> -#define CX86_CCR6 0xea
> -#define CX86_CCR7 0xeb
> -#define CX86_PCR1 0xf0
> -#define CX86_DIR0 0xfe
> -#define CX86_DIR1 0xff
> -#define CX86_ARR_BASE 0xc4
> -#define CX86_RCR_BASE 0xdc

This clashes with Andi's "msr-index" patch:

ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches/msr-index

Perhaps it'd be best to wait until msr-index goes upstream and to raise a
patch then.  Or to redo and retest against 2.6.21-rc7-mm2, which includes
msr-index.

Also, include/asm-x86_64/processor.h has a getCx86(), too.  Does it also
need fixing?

Re: [PATCH] [1/1] CPU-i386-Geode: Chipset access macros do not work as expected (2nd try)

[Juergen Beisert]

Hi Andrew,

On Wednesday 02 May 2007 02:48, Andrew Morton wrote:
> On Mon, 30 Apr 2007 17:33:41 +0200
>
> Juergen Beisert <juergen127@kreuzholzen.de> wrote:
> > From: Juergen Beisert <juergen.beisert@weihenstephan.org>
> >
> > 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 outputs something like this:
> > 	Clocksource tsc unstable (delta = -472746897 ns)
> > Tested on a Geode GX1 system.
> >
> > This is the second version with some modifications suggested by
> > Mikael Pettersson
> >
> > Signed-off-by: Juergen Beisert <juergen.beisert@weihenstephan.org>
> >
> > Index: linux-2.6.21/include/asm-i386/processor.h
> > ===================================================================
> > --- linux-2.6.21.orig/include/asm-i386/processor.h
> > +++ linux-2.6.21/include/asm-i386/processor.h
> > @@ -202,37 +202,6 @@ static inline void clear_in_cr4 (unsigne
> >  	write_cr4(cr4);
> >  }
> >
> > -/*
> > - *      NSC/Cyrix CPU configuration register indexes
> > - */
> > -
> > -#define CX86_PCR0 0x20
> > -#define CX86_GCR  0xb8
> > -#define CX86_CCR0 0xc0
> > -#define CX86_CCR1 0xc1
> > -#define CX86_CCR2 0xc2
> > -#define CX86_CCR3 0xc3
> > -#define CX86_CCR4 0xe8
> > -#define CX86_CCR5 0xe9
> > -#define CX86_CCR6 0xea
> > -#define CX86_CCR7 0xeb
> > -#define CX86_PCR1 0xf0
> > -#define CX86_DIR0 0xfe
> > -#define CX86_DIR1 0xff
> > -#define CX86_ARR_BASE 0xc4
> > -#define CX86_RCR_BASE 0xdc
>
> This clashes with Andi's "msr-index" patch:
>
> ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches/msr-index

I see. He also moves these defines to another file. Not problem where they 
are. But it isn't possible to replace the macros by the inlined functions in 
processor.h. The used outb/inb macros fail for various other files.

> Perhaps it'd be best to wait until msr-index goes upstream and to raise a
> patch then.  Or to redo and retest against 2.6.21-rc7-mm2, which includes
> msr-index.

All right. To wait is no problem for me (my system works with my 
patch.... ;-) )

> Also, include/asm-x86_64/processor.h has a getCx86(), too.  Does it also
> need fixing?

As I can see the macros are used in:
 - i386/kernel/cpu/cpufreq/gx-suspmod.c (ups, its not in my patch, so it is
   currently no complete)
 - i386/kernel/cpu/mtrr/state.c (also not part of my patch yet)
 - i386/kernel/cpu/cyrix.c
 - i386/kernel/cpu/mtrr/cyrix.c

Most comments states Cyrix CPUs when they are using the macros. Is anything 
with Cyrix 64 bit relevant? Maybe "include/asm-x86_64/processor.h" is a 
simple copy of "include/asm-i386/processor.h" and nobody delete the unused 
macros?

Please keep me informed and I will resend the patch.

Juergen

Re: [PATCH] [1/1] CPU-i386-Geode: Chipset access macros do not work as expected (2nd try)

[Andrew Morton]

On Wed, 2 May 2007 09:16:59 +0200 Juergen Beisert <juergen127@kreuzholzen.de> wrote:

> >
> > This clashes with Andi's "msr-index" patch:
> >
> > ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches/msr-index
> 
> I see. He also moves these defines to another file. Not problem where they 
> are. But it isn't possible to replace the macros by the inlined functions in 
> processor.h. The used outb/inb macros fail for various other files.

Oh well.  Please work out what you think needs to be done.

> > Perhaps it'd be best to wait until msr-index goes upstream and to raise a
> > patch then.  Or to redo and retest against 2.6.21-rc7-mm2, which includes
> > msr-index.
> 
> All right. To wait is no problem for me (my system works with my 
> patch.... ;-) )
> 
> > Also, include/asm-x86_64/processor.h has a getCx86(), too.  Does it also
> > need fixing?
> 
> As I can see the macros are used in:
>  - i386/kernel/cpu/cpufreq/gx-suspmod.c (ups, its not in my patch, so it is
>    currently no complete)
>  - i386/kernel/cpu/mtrr/state.c (also not part of my patch yet)
>  - i386/kernel/cpu/cyrix.c
>  - i386/kernel/cpu/mtrr/cyrix.c
> 
> Most comments states Cyrix CPUs when they are using the macros. Is anything 
> with Cyrix 64 bit relevant? Maybe "include/asm-x86_64/processor.h" is a 
> simple copy of "include/asm-i386/processor.h" and nobody delete the unused 
> macros?

I don't think Cyrix make 64-bit stuff, surely.  Andi?

> Please keep me informed and I will resend the patch.
> 

Thanks.  I think Andi was after a better explanation of what was going
wrong with the macros, too.  Please include that in the changelog.

Re: [PATCH] [1/1] CPU-i386-Geode: Chipset access macros do not work as expected (2nd try)

[Andi Kleen]

> Most comments states Cyrix CPUs when they are using the macros. Is anything 
> with Cyrix 64 bit relevant? Maybe "include/asm-x86_64/processor.h" is a 
> simple copy of "include/asm-i386/processor.h" and nobody delete the unused 
> macros?

It was originally deleted but later readded when the MTRR code was merged together.
That's because of i386/kernel/cpu/mtrr/state.c which is compiled on x86-64 too.
If you can move that code into cyrix.c it can be removed. Might be a good opportunity
to clean this up.
 
> Please keep me informed and I will resend the patch.

I would need an patch against the current ff tree that doesn't break x86-64.

-Andi
 

Re: [PATCH] [1/1] CPU-i386-Geode: Chipset access macros do not work as expected (2nd try)

[Mikael Pettersson]

On Mon, 30 Apr 2007 17:33:41 +0200, Juergen Beisert 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.
> 
> 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 outputs something like this:
> 	Clocksource tsc unstable (delta = -472746897 ns)
> Tested on a Geode GX1 system.
> 
> This is the second version with some modifications suggested by
> Mikael Pettersson
> 
> Signed-off-by: Juergen Beisert <juergen.beisert@weihenstephan.org>

Acked-by: Mikael Pettersson <mikpe@it.uu.se>

> Index: linux-2.6.21/include/asm-i386/processor.h
> ===================================================================
> --- linux-2.6.21.orig/include/asm-i386/processor.h
> +++ linux-2.6.21/include/asm-i386/processor.h
> @@ -202,37 +202,6 @@ static inline void clear_in_cr4 (unsigne
>  	write_cr4(cr4);
>  }
> 
> -/*
> - *      NSC/Cyrix CPU configuration register indexes
> - */
> -
> -#define CX86_PCR0 0x20
> -#define CX86_GCR  0xb8
> -#define CX86_CCR0 0xc0
> -#define CX86_CCR1 0xc1
> -#define CX86_CCR2 0xc2
> -#define CX86_CCR3 0xc3
> -#define CX86_CCR4 0xe8
> -#define CX86_CCR5 0xe9
> -#define CX86_CCR6 0xea
> -#define CX86_CCR7 0xeb
> -#define CX86_PCR1 0xf0
> -#define CX86_DIR0 0xfe
> -#define CX86_DIR1 0xff
> -#define CX86_ARR_BASE 0xc4
> -#define CX86_RCR_BASE 0xdc
> -
> -/*
> - *      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: linux-2.6.21/include/asm-i386/processor-cyrix.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6.21/include/asm-i386/processor-cyrix.h
> @@ -0,0 +1,33 @@
> +/*
> + * NSC/Cyrix CPU configuration register indexes
> + */
> +#define CX86_PCR0 0x20
> +#define CX86_GCR  0xb8
> +#define CX86_CCR0 0xc0
> +#define CX86_CCR1 0xc1
> +#define CX86_CCR2 0xc2
> +#define CX86_CCR3 0xc3
> +#define CX86_CCR4 0xe8
> +#define CX86_CCR5 0xe9
> +#define CX86_CCR6 0xea
> +#define CX86_CCR7 0xeb
> +#define CX86_PCR1 0xf0
> +#define CX86_DIR0 0xfe
> +#define CX86_DIR1 0xff
> +#define CX86_ARR_BASE 0xc4
> +#define CX86_RCR_BASE 0xdc
> +
> +/*
> + * 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);
> +}
> Index: linux-2.6.21/arch/i386/kernel/cpu/cyrix.c
> ===================================================================
> --- linux-2.6.21.orig/arch/i386/kernel/cpu/cyrix.c
> +++ linux-2.6.21/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>
> 
> Index: linux-2.6.21/arch/i386/kernel/cpu/mtrr/cyrix.c
> ===================================================================
> --- linux-2.6.21.orig/arch/i386/kernel/cpu/mtrr/cyrix.c
> +++ linux-2.6.21/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;
>