Re: [coreboot] vx800 Example Board Patch

[Stefan Reinauer]

Dear Bari,

thanks a lot for this patch... Here's a quick first shot at a review..

On 18.07.2009 17:59, bari wrote:
> Index: src/cpu/via/car/disable_cache_as_ram.c
>
> --- src/cpu/via/car/disable_cache_as_ram.c	(Revision 0)
> +++ src/cpu/via/car/disable_cache_as_ram.c	(Revision 0)
> @@ -0,0 +1,101 @@

What's the reason for not using the mechanism that's already there?
> Index: src/cpu/via/car/post_cache_as_ram.c
>
> --- src/cpu/via/car/post_cache_as_ram.c	(Revision 0)
> +++ src/cpu/via/car/post_cache_as_ram.c	(Revision 0)
> +/* Disable Erratum 343 Workaround, see RevGuide for Fam10h, Pub#41322 Rev 3.33 */

This looks wrong.
> Index: src/cpu/via/car/cache_as_ram.inc
>
> --- src/cpu/via/car/cache_as_ram.inc	(Revision 4427)
> +++ src/cpu/via/car/cache_as_ram.inc	(Arbeitskopie)

This one mostly breaks whitespace.
> fixed_mtrr_msr:
> Index: src/cpu/via/model_c7/model_c7_init.c
> ===================================================================
> --- src/cpu/via/model_c7/model_c7_init.c	(Revision 4427)
> +++ src/cpu/via/model_c7/model_c7_init.c	(Arbeitskopie)
> @@ -60,6 +60,7 @@
>    0x0406, 0x0806,		// 400MHz, 796mV --> 800MHz, 796mV      C7-M ULV
>    0x0406, 0x0a06,		// 400MHz, 796mV --> 1000MHz, 796mV
>    0x0406, 0x0c09,		// 400MHz, 796mV --> 1200MHz, 844mV
> 0x0406, 0x0609,		// 
> 0x0806, 0x0c09,		// 800MHz, 796mV --> 1200MHz, 844mV
> 0x0406, 0x0f10,		// 400MHz, 796mV --> 1500MHz, 956mV
> 0x0806, 0x1010,		// 800MHz, 796mV --> 1600MHz, 956mV
>
> @@ -100,6 +101,13 @@
>    	}
>    }
>
> +#if 1
> /* Some value may not be included in c7d_speed_translation,
> so a more general way is needed:*/
> current=msr.hi&0xffff;
> msr.lo=msr.lo&0xffff0000;
> msr.lo=msr.lo|current;
>
> +#else

Please do not use this. It does fail on C7 CPUs we've encountered.
Either only execute it if the values are not in the list, or drop it
completely.
> Index: src/northbridge/via/vx800/vx800_ide.c
>
> --- src/northbridge/via/vx800/vx800_ide.c	(Revision 4427)
> +++ src/northbridge/via/vx800/vx800_ide.c	(Arbeitskopie)
> @@ -25,244 +25,78 @@
>  #include "chip.h"
>  #include <arch/io.h>
>  #include "vx800.h"
> +#include "vx800_pci_io_modify_ops.h"
>
> -static const idedevicepcitable[16 * 12] = {
> /*
> 0x02, 0x00, 0x00, 0x00, 0x00, 0x82, 0x00, 0x00,
> 0x00, 0x00, 0xA8, 0xA8, 0xF0, 0x00, 0x00, 0xB6,
> 0x00, 0x00, 0x01, 0x21, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x01, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x01, 0x09, 0xC4, 0x06, 0x11, 0x09, 0xC4,
> 0x00, 0xC2, 0xF9, 0x01, 0x10, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x0C, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x02, 0x01, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> */
> 0x02, 0x00, 0x00, 0x00, 0x00, 0x82, 0x00, 0x00,
> 0x00, 0x00, 0x99, 0x20, 0xf0, 0x00, 0x00, 0x20,
> 0x00, 0x00, 0x17, 0xF1, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x01, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x01, 0x09, 0xC4, 0x06, 0x11, 0x09, 0xC4,
> 0x00, 0xc2, 0x09, 0x01, 0x10, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x0c, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x02, 0x01, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> /* Legacy BIOS XP PCI value */
> /*
> 0x02, 0x00, 0x00, 0x00, 0x00, 0x82, 0x00, 0x00,
> 0x00, 0x00, 0xa8, 0x20, 0x00, 0x00, 0x00, 0xb6,
> 0x00, 0x00, 0x16, 0xF1, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x01, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x01, 0x09, 0xC4, 0x06, 0x11, 0x09, 0xC4,
> 0x00, 0x02, 0x09, 0x00, 0x18, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x34, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x02, 0x01, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> */
> /* ROM legacy BIOS on cn_8562b */
> /*
> 0x03, 0x00, 0x00, 0x00, 0x00, 0x82, 0x00, 0x00,
> 0x00, 0x00, 0x99, 0x20, 0x60, 0x00, 0x00, 0x20,
> 0x00, 0x00, 0x1E, 0xF1, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x01, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x01, 0x09, 0xC4, 0x06, 0x11, 0x09, 0xC4,
> 0x00, 0x02, 0x09, 0x01, 0x18, 0x0C, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x34, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x02, 0x01, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> */
> /* From legacy BIOS on c7_8562b */
> /*
> 0x03, 0x00, 0x00, 0x00, 0x00, 0x82, 0x00, 0x00,
> 0x00, 0x00, 0x5E, 0x20, 0x60, 0x00, 0x00, 0xB6,
> 0x00, 0x00, 0x1E, 0xF1, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x01, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x01, 0x09, 0xC4, 0x06, 0x11, 0x09, 0xC4,
> 0x00, 0x02, 0x09, 0x01, 0x18, 0x0C, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x34, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x02, 0x01, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> */
>
> +static const idedevicepcitable [16*12]={
> +0x02, 0x00, 0x00, 0x00, 0x00, 0x82, 0x00, 0x00, 0x00, 0x00, 0x99, 0x20, 0xf0, 0x00, 0x00, 0x20, 
> +0x00, 0x00, 0x17, 0xF1, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> +0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> +0x01, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x09, 0xC4, 0x06, 0x11, 0x09, 0xC4, 
> +0x00, 0xc2, 0x09, 0x01, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> +0x00, 0x00, 0x00, 0x00, 0x0c, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> +0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
> +0x02, 0x01, 0x24, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
>  };

I don't think this is an improvement.
> Index: src/northbridge/via/vx800/northbridge.c
>
> --- src/northbridge/via/vx800/northbridge.c	(Revision 4427)
> +++ src/northbridge/via/vx800/northbridge.c	(Arbeitskopie)
> @@ -41,26 +41,26 @@
>  u8 acpi_sleep_type = 0;
>
> static void memctrl_init(device_t dev)
> -{
> +{    
>  /*
> set VGA in uma_ram_setting.c, not in this function.
> set VGA in uma_ram_setting.c, not in this function.
>
> */
>  #if 0
>    pci_write_config8(dev, 0x85, 0x20);
>    pci_write_config8(dev, 0x86, 0x2d);
> /* Set up VGA timers */
> pci_write_config8(dev, 0xa2, 0x44);
> /* Enable VGA with a 32mb framebuffer */
> pci_write_config16(dev, 0xa0, 0xd000);
> pci_write_config16(dev, 0xa4, 0x0010);
> //b0: 60 aa aa 5a 0f 00 00 00 08
> pci_write_config16(dev, 0xb0, 0xaa00);
> pci_write_config8(dev, 0xb8, 0x08);
>
> -#endif
> +#endif	
>  }

Adds plenty of whitespace breakage.

Stopping here with the review, as I believe this needs quite some work
before it can be merged...