[coreboot] Geode LX VGA BIOS Patch

[Kevin O'Connor]

On Wed, Aug 12, 2009 at 06:02:08AM -0400, Chris Kindt wrote:
>     These initial patches enable booting with the SeaBIOS VGA BIOS on
> Geode LX hardware.

Wow.  I was unaware that there was a GSOC project for this.

I have a few comments - see below.

> +static union u64_u32_u lx_msrRead(u32 msrAddr)
> +{
> +    union u64_u32_u val;
> +    asm __volatile__ (
> +        "movw   $0x0AC1C, %%dx          \n"
> +        "movl   $0xFC530007, %%eax      \n"
> +        "outl   %%eax, %%dx             \n"
> +        "addb   $2, %%dl                \n"
> +        "inw    %%dx, %%ax              \n"
> +        : "=a" (val.lo), "=d"(val.hi)
> +        : "c"(msrAddr)

Does the geode do something "magical" with the above sequence?
Otherwise, I'm a bit confused on how the asm works.

[...]
> +static inline void
> +call16_vgaint_lx(u32 eax, u32 ebx,u32 ecx, u32 edx)
> +{
> +    asm volatile(
> +        "int $0x10\n"
> +        "cli\n"
> +        "cld"
> +        :
> +        : "a"(eax), "b"(ebx),"c"(ecx), "d"(edx)
> +        : "cc", "memory");
> +}

It looks like you're not indicating that the registers could get
clobberred.  Also, the existing call16_simpint() or direct calls to
the bios handlers may work.

> +/* PCI Header
> +*
> +* This might deserve a pci_header.S file, it is here for now
> +*/
> +
> +ASM16(
> +    "   .globl _rom_pcidata             \n"
> +    "_rom_pcidata:                      \n"
> +    "_rom_pcidata_sig:                  \n"
> +    "   .ascii \"PCIR\"                 \n"
> +    "_rom_pcidata_venderid:             \n"
> +    "   .word 0x1022                    \n"
[...]

I think it would be better to define this with a C struct - see the
"struct pci_data" defined in src/optionroms.c.

> +    if(CONFIG_GEODELX)
> +        geodelx_demo();
> +
> +    // Fixup checksum
> +    extern u8 _rom_header_size, _rom_header_checksum;
>      // Fixup checksum
>      extern u8 _rom_header_size, _rom_header_checksum;

Bad merge?

[... out of order ...]
>  // XXX
>  #define CONFIG_VBE 0
>  #define CONFIG_CIRRUS 0
> +#define CONFIG_GEODELX 1

For SeaBIOS, I don't thing geode should be the default.

Finally, I'd be happy to commit this to SeaBIOS.  I think it does
re-raise the question of whether SeaBIOS' VGA support should be
spun-off into it's own repository, though.

-Kevin