On Mon, 30 Apr 2007 17:33:41 +0200
Juergen Beisert <[email protected]> wrote:
> From: Juergen Beisert <[email protected]>
>
> 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 <[email protected]>
>
> 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?
Hi Andrew,
On Wednesday 02 May 2007 02:48, Andrew Morton wrote:
> On Mon, 30 Apr 2007 17:33:41 +0200
>
> Juergen Beisert <[email protected]> wrote:
> > From: Juergen Beisert <[email protected]>
> >
> > 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 <[email protected]>
> >
> > 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
On Wed, 2 May 2007 09:16:59 +0200 Juergen Beisert <[email protected]> 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.
> 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