Re: [LinuxBIOS] [patch 06/10] Geode LX: CPU

[Peter Stuge]

On Thu, May 03, 2007 at 12:15:54PM -0600, Marc Jones wrote:
> This patch adds support for the AMD Geode LX CPU.

Sorry, I would still reject it because of all the whitespace
breakage.
> CPUbug784
> Bugtool #784 + #792
> Fix CPUID instructions for < 3.0 CPUs
>
> -void bug784(void)

And what about all these bug functions?
> -/*  FooGlue Setup*/

Hahaha, "FooGlue" ?! :)
> Index: LinuxBIOSv2/src/cpu/amd/model_lx/vsmsetup.c
>
> --- LinuxBIOSv2.orig/src/cpu/amd/model_lx/vsmsetup.c	2007-05-03 11:20:48.000000000 -0600
> +++ LinuxBIOSv2/src/cpu/amd/model_lx/vsmsetup.c	2007-05-03 11:22:07.000000000 -0600
> @@ -17,62 +17,65 @@
>  /* vsmsetup.c derived from vgabios.c. Derived from: */
>
> /*------------------------------------------------------------ -*- C -*-
> 2 Kernel Monte a.k.a. Linux loading Linux on x86
> 2 Kernel Monte a.k.a. Linux loading Linux on x86

At least 90% of the changes in this file is whitespace.
That's just not OK IMHO.

There's more in other files and the other patches but this may be the
worst.
> @@ -85,7 +88,7 @@
> "	.long	gdt	       	\n"
> "	.long	gdt	       	\n"
>
> @@ -105,7 +108,7 @@
> "	.byte	0x00, 0x9b, 0xcf, 0x00	\n"
> "	.byte	0x00, 0x9b, 0xcf, 0x00	\n"
>
> @@ -115,7 +118,7 @@
>    /* selgdt 0x28 16-bit 64k code at 0x00000000 */
> /* selgdt 0x28 16-bit 64k code at 0x00000000 */

..in the GDT too.. :\
> /* the VSA starts at the base of rom - 64 */
> //rom = ((unsigned long) 0) - (ROM_SIZE  + 64*1024);
> rom = 0xfffc8000;
>
> //rom = ((unsigned long) 0) - (ROM_SIZE + 64*1024);
>
> //rom = 0xfffc8000;
>
> //VSA is cat onto the end after LB builds
>
> rom = ((unsigned long) 0) - (ROM_SIZE + 36 * 1024);

This hardcoded size doesn't look so nice, especially if the VSA blob
will get hacked on. And, if code is dead then please just delete it
instead of commenting out the old assignment and adding a new one.
> Index: LinuxBIOSv2/src/include/cpu/amd/lxdef.h
>
> --- LinuxBIOSv2.orig/src/include/cpu/amd/lxdef.h	2007-05-03 11:20:49.000000000 -0600
> +++ LinuxBIOSv2/src/include/cpu/amd/lxdef.h	2007-05-03 11:30:00.000000000 -0600
> @@ -24,42 +24,21 @@
>
> #ifndef CPU_AMD_LXDEF_H
>  #define CPU_AMD_LXDEF_H
> -#define	CPU_ID_1_X							0x540		/* Stepping ID 1.x*/
> -#define	CPU_ID_2_0							0x551		/* Stepping ID 2.0*/
> -#define	CPU_ID_2_1							0x552		/* Stepping ID 2.1*/
> -#define	CPU_ID_2_2							0x553		/* Stepping ID 2.2*/

[...]
> +#define CPU_ID_1_X						0x00000560		/* Stepping ID 1.x CPUbug fix to change it to 5A0*/
> +#define CPU_ID_2_0						0x000005A1
> +#define CPU_ID_3_0						0x000005A2

Apart from the massive whitespace changes also in this file, please
comment on the code changes related to hardware revisions. If there
are big changes between revs they should just be multiple CPU types..

Is it really nice to change the IDs?
> -#define	GL0_GLIU0			0
> -#define	GL0_MC				1
> -#define	GL0_GLIU1			2
> -#define	GL0_CPU				3
> -#define	GL0_VG				4
> -#define	GL0_GP				5
> -//#define	GL0_DF				6		//GX3 no such thing as VP port
> +#define GL0_GLIU0		0
> +#define GL0_MC			1
> +#define GL0_GLIU1		2
> +#define GL0_CPU			3
> +#define GL0_VG			4
> +#define GL0_GP			5

...
> Index: LinuxBIOSv2/src/include/cpu/amd/vr.h
>
> --- LinuxBIOSv2.orig/src/include/cpu/amd/vr.h	2007-05-03 11:20:49.000000000 -0600
> +++ LinuxBIOSv2/src/include/cpu/amd/vr.h	2007-05-03 11:22:07.000000000 -0600

Again full of whitespace changes.
> #define VSA_VERSION_NUM     0x00
> #define VSA_VERSION_NUM		0x00
> #define HIGH_MEM_ACCESS		0x01
> #define GET_VSM_INFO        0x02	// Used by INFO
>   #define GET_BASICS       0x00
>   #define GET_EVENT        0x01
>   #define GET_STATISTICS   0x02
>   #define GET_HISTORY      0x03
>   #define GET_HARDWARE     0x04
>   #define GET_ERROR        0x05
>   #define SET_VSM_TYPE     0x06
> #define GET_VSM_INFO		0x02	// Used by INFO
>   #define GET_BASICS		0x00
>   #define GET_EVENT		0x01
>   #define GET_STATISTICS	0x02
>   #define GET_HISTORY		0x03
>   #define GET_HARDWARE		0x04
>   #define GET_ERROR		0x05
>   #define SET_VSM_TYPE		0x06
> #define SIGNATURE			0x03
>   #define VSA2_SIGNATURE	0x56534132	// 'VSA2' returned in EAX 
>   #define VSA2_SIGNATURE	0x56534132	// 'VSA2' returned in EAX

So much noise my head actually hurts!

Frankly, I think this patch makes AMD look bad and I don't want that
in the repo. Should anyone ever diff over it they will curse long,
hard and loud - at least I would.

But, even if I feel strongly about this I don't feel very
authoritative here. If others are fine with it I'll shut up, at least
for this patchset.

//Peter