Re: Geode-GX1 processor in 2.6.21
> > In arch/i386/kernel/cpu/cyrix.c function geode_configure() tries to enable
> > the "suspend on halt power saving feature". This is the line:
> >
> > setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
That will go wrong
> > #define setCx86(reg, data) do { \
> > outb((reg), 0x22); \
> > outb((data), 0x23); \
> > } while (0)
> >
> > Maybe the compiler does the wrong thing if someone uses these macros in the
> > same instruction?
>
> Sounds quite likely.
The problem is the macro evaluates to
outb(reg, 0x22);
[Then the getCx86]
outb(data, 0x23);
which isn't valid for 6x86 control. Looks like someone overoptimised it.
Either make it "inline" or split it up and it'll work
> Also, it would probably be good to convert these macros into inline functions
> in this header.
Agreed - they must be issued in order in the right way as they are CPU
level magic.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Geode-GX1 processor in 2.6.21
On Monday 30 April 2007 14:20, Andreas Mohr wrote: > Also, it would probably be good to convert these macros into inline > functions in this header. I tried with inlined functions and it works now as expected. I sent a patch, maybe it helps others too. Juergen - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Geode-GX1 processor in 2.6.21
Hi,
On Mon, Apr 30, 2007 at 12:51:05PM +0200, Juergen Beisert wrote:
> Hi all,
>
> I do not understand it, but setting up some chipset features (tweaks) fail on
> Geode GX1.
>
> In arch/i386/kernel/cpu/cyrix.c function geode_configure() tries to enable
> the "suspend on halt power saving feature". This is the line:
>
> setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
>
> If this register is 0x00 before, it is still 0x00 after this line. If I
> change
> the line into this:
>
> ccr2 = getCx86(CX86_CCR2);
> ccr2 |= 0x88;
> setCx86(CX86_CCR2, ccr2);
>
> register ccr2 is 0x88 after the setCx86 call and the power saving feature is
> active (and BTW: the TSC is useless then, because it also stops when the CPU
> runs into a HLT instruction).
>
> setCx86 and getCx86 are macros defined in include/asm-i386/processor.h:
>
> #define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
>
> #define setCx86(reg, data) do { \
> outb((reg), 0x22); \
> outb((data), 0x23); \
> } while (0)
>
> Maybe the compiler does the wrong thing if someone uses these macros in the
> same instruction?
Sounds quite likely.
You should check objdump -d output of cyrix.o to verify whether the compiler
"illegally" reorders some I/O instructions in geode_configure().
Probably you need to add a barrier() or wmb() or similar to these macros.
Or should this be a (code-invisible) PCI posting issue? In that case you need
to add some dummy read operations.
Also, it would probably be good to convert these macros into inline functions
in this header.
Andreas Mohr
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Geode-GX1 processor in 2.6.21
On Mon, 30 Apr 2007 12:51:05 +0200, Juergen Beisert wrote:
> setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
>
> If this register is 0x00 before, it is still 0x00 after this line. If I
> change
> the line into this:
>
> ccr2 = getCx86(CX86_CCR2);
> ccr2 |= 0x88;
> setCx86(CX86_CCR2, ccr2);
>
> register ccr2 is 0x88 after the setCx86 call and the power saving feature is
> active (and BTW: the TSC is useless then, because it also stops when the CPU
> runs into a HLT instruction).
>
> setCx86 and getCx86 are macros defined in include/asm-i386/processor.h:
>
> #define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
>
> #define setCx86(reg, data) do { \
> outb((reg), 0x22); \
> outb((data), 0x23); \
> } while (0)
>
> Maybe the compiler does the wrong thing if someone uses these macros in the
> same instruction?
The compiler is fine, it's the setCx86() macro that is utter crap
because it fails to behave like a function by not evaluating its
arguments before performing its outb() statements.
Converting them to static inlines would be the best solution. Something like
static inline u8 getCx86(unsigned int reg)
{
outb(reg, 0x22);
return inb(0x23);
}
static inline void setCx86(unsigned int reg, u8 data)
{
outb(reg, 0x22);
outb(data, 0x23);
}
ought to work.
/Mikael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Geode-GX1 processor in 2.6.21
On Mon, 30 Apr 2007 12:51:05 +0200, Juergen Beisert wrote:
setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
If this register is 0x00 before, it is still 0x00 after this line. If I
change
the line into this:
ccr2 = getCx86(CX86_CCR2);
ccr2 |= 0x88;
setCx86(CX86_CCR2, ccr2);
register ccr2 is 0x88 after the setCx86 call and the power saving feature is
active (and BTW: the TSC is useless then, because it also stops when the CPU
runs into a HLT instruction).
setCx86 and getCx86 are macros defined in include/asm-i386/processor.h:
#define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
#define setCx86(reg, data) do { \
outb((reg), 0x22); \
outb((data), 0x23); \
} while (0)
Maybe the compiler does the wrong thing if someone uses these macros in the
same instruction?
The compiler is fine, it's the setCx86() macro that is utter crap
because it fails to behave like a function by not evaluating its
arguments before performing its outb() statements.
Converting them to static inlines would be the best solution. Something like
static inline u8 getCx86(unsigned int reg)
{
outb(reg, 0x22);
return inb(0x23);
}
static inline void setCx86(unsigned int reg, u8 data)
{
outb(reg, 0x22);
outb(data, 0x23);
}
ought to work.
/Mikael
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Geode-GX1 processor in 2.6.21
Hi,
On Mon, Apr 30, 2007 at 12:51:05PM +0200, Juergen Beisert wrote:
Hi all,
I do not understand it, but setting up some chipset features (tweaks) fail on
Geode GX1.
In arch/i386/kernel/cpu/cyrix.c function geode_configure() tries to enable
the suspend on halt power saving feature. This is the line:
setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
If this register is 0x00 before, it is still 0x00 after this line. If I
change
the line into this:
ccr2 = getCx86(CX86_CCR2);
ccr2 |= 0x88;
setCx86(CX86_CCR2, ccr2);
register ccr2 is 0x88 after the setCx86 call and the power saving feature is
active (and BTW: the TSC is useless then, because it also stops when the CPU
runs into a HLT instruction).
setCx86 and getCx86 are macros defined in include/asm-i386/processor.h:
#define getCx86(reg) ({ outb((reg), 0x22); inb(0x23); })
#define setCx86(reg, data) do { \
outb((reg), 0x22); \
outb((data), 0x23); \
} while (0)
Maybe the compiler does the wrong thing if someone uses these macros in the
same instruction?
Sounds quite likely.
You should check objdump -d output of cyrix.o to verify whether the compiler
illegally reorders some I/O instructions in geode_configure().
Probably you need to add a barrier() or wmb() or similar to these macros.
Or should this be a (code-invisible) PCI posting issue? In that case you need
to add some dummy read operations.
Also, it would probably be good to convert these macros into inline functions
in this header.
Andreas Mohr
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Geode-GX1 processor in 2.6.21
On Monday 30 April 2007 14:20, Andreas Mohr wrote: Also, it would probably be good to convert these macros into inline functions in this header. I tried with inlined functions and it works now as expected. I sent a patch, maybe it helps others too. Juergen - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Geode-GX1 processor in 2.6.21
In arch/i386/kernel/cpu/cyrix.c function geode_configure() tries to enable
the suspend on halt power saving feature. This is the line:
setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
That will go wrong
#define setCx86(reg, data) do { \
outb((reg), 0x22); \
outb((data), 0x23); \
} while (0)
Maybe the compiler does the wrong thing if someone uses these macros in the
same instruction?
Sounds quite likely.
The problem is the macro evaluates to
outb(reg, 0x22);
[Then the getCx86]
outb(data, 0x23);
which isn't valid for 6x86 control. Looks like someone overoptimised it.
Either make it inline or split it up and it'll work
Also, it would probably be good to convert these macros into inline functions
in this header.
Agreed - they must be issued in order in the right way as they are CPU
level magic.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
