Re: [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