Re: fix typo in geode_configre()@cyrix.c

[takada]

From: lsorense@xxxxxxxxxxxxxxxxxxx (Lennart Sorensen)
Subject: Re: fix typo in geode_configre()@cyrix.c
Date: Tue, 9 Jan 2007 12:33:48 -0500

Thank you for comments.

> On Tue, Jan 09, 2007 at 06:41:56PM +0900, takada wrote:
> > In kernel 2.6, write back wrong register when configure Geode processor.
> > Instead of storing to CCR4, it stores to CCR3.
> > 
> > --- linux-2.6.19/arch/i386/kernel/cpu/cyrix.c.orig	2007-01-09 16:45:21.000000000 +0900
> > +++ linux-2.6.19/arch/i386/kernel/cpu/cyrix.c	2007-01-09 17:10:13.000000000 +0900
> > @@ -173,7 +173,7 @@ static void __cpuinit geode_configure(vo
> >  	ccr4 = getCx86(CX86_CCR4);
> >  	ccr4 |= 0x38;		/* FPU fast, DTE cache, Mem bypass */
> >  	
> > -	setCx86(CX86_CCR3, ccr3);
> > +	setCx86(CX86_CCR4, ccr4);
> >  	
> >  	set_cx86_memwb();
> >  	set_cx86_reorder();	
> > -
> 
> One more comment on this:
> 
> Why is this function using 3 different styles to do the same thing to 3
> different registers?
> 
> First it does CCR2 by doing:
> setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
> 
> Nice, no temp values, and it is obvious this is a permanent change.
> 
> Then for the next one it does:
> ccr3 = GetCx86(CX86_CCR3);
> setCx86(CX86_CCR3, (ccr3 & 0x0f) | 0x10);
> 
> Couldn't that have been:
> setCx86(CX86_CCR3, (getCx86(CX86_CCR3) & 0x0f) | 0x10);
> 
> No temp variable, and again it clearly does not intend to restore the
> value again later (even though the bug actually did cause the value to
> be restored by accident).
> 
> Then the last case does:
> ccr4 = GetCx86(CX86_CCR4);
> ccr4 |= 0x38;
> setCx86(CX86_CCR4, ccr4); (after fixing the typo you found of course).
> 
> Why did this one have to modify the variable first before using it?
> Maybe the ccr3 and ccr4 cases would have made the line too long and
> needed wrapping, but at least those two cases should be done the same
> way.  It makes it look like it was written by 3 different people who
> didn't look at the lines around the changes they were making.

You are right. I agree to your comment. These variables are needless.
I made a patch again.

diff -Narup linux-2.6.19.orig/arch/i386/kernel/cpu/cyrix.c linux-2.6.19/arch/i386/kernel/cpu/cyrix.c
--- linux-2.6.19.orig/arch/i386/kernel/cpu/cyrix.c	2006-11-30 06:57:37.000000000 +0900
+++ linux-2.6.19/arch/i386/kernel/cpu/cyrix.c	2007-01-16 19:55:05.000000000 +0900
@@ -161,19 +161,15 @@ static void __cpuinit set_cx86_inc(void)
 static void __cpuinit geode_configure(void)
 {
 	unsigned long flags;
-	u8 ccr3, ccr4;
 	local_irq_save(flags);
 
 	/* Suspend on halt power saving and enable #SUSP pin */
 	setCx86(CX86_CCR2, getCx86(CX86_CCR2) | 0x88);
 
-	ccr3 = getCx86(CX86_CCR3);
-	setCx86(CX86_CCR3, (ccr3 & 0x0f) | 0x10);	/* Enable */
+	setCx86(CX86_CCR3, (getCx86(CX86_CCR3) & 0x0f) | 0x10);	/* Enable */
 	
-	ccr4 = getCx86(CX86_CCR4);
-	ccr4 |= 0x38;		/* FPU fast, DTE cache, Mem bypass */
-	
-	setCx86(CX86_CCR3, ccr3);
+	/* FPU fast, DTE cache, Mem bypass */
+	setCx86(CX86_CCR4, getCx86(CX86_CCR4) | 0x30);
 	
 	set_cx86_memwb();
 	set_cx86_reorder();	
@@ -415,15 +411,14 @@ static void __cpuinit cyrix_identify(str
 		
    	        if (dir0 == 5 || dir0 == 3)
    	        {
-			unsigned char ccr3, ccr4;
+			unsigned char ccr3;
 			unsigned long flags;
 			printk(KERN_INFO "Enabling CPUID on Cyrix processor.\n");
 			local_irq_save(flags);
 			ccr3 = getCx86(CX86_CCR3);
-			setCx86(CX86_CCR3, (ccr3 & 0x0f) | 0x10); /* enable MAPEN  */
-			ccr4 = getCx86(CX86_CCR4);
-			setCx86(CX86_CCR4, ccr4 | 0x80);          /* enable cpuid  */
-			setCx86(CX86_CCR3, ccr3);                 /* disable MAPEN */
+			setCx86(CX86_CCR3, (ccr3 & 0x0f) | 0x10);      /* enable MAPEN  */
+			setCx86(CX86_CCR4, getCx86(CX86_CCR4) | 0x80); /* enable cpuid  */
+			setCx86(CX86_CCR3, ccr3);                      /* disable MAPEN */
 			local_irq_restore(flags);
 		}
 	}



> 
> --
> Len Sorensen
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/