[coreboot] Geode LX VGA BIOS Patch

[Chris Kindt]

Kevin O'Connor wrote:
> 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.
This is a magic port. The geode SMM (VSA2) provides a few functions from 
this interface.

> 
> [...]
>> +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.
I am replacing it with direct calls.

> 
>> +/* 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.
> 
This was originally added in vgaentry.S. It was relocated in an attempt 
to contain everything in geodelx.* files. I agree that a struct would be 
a much better format.

>> +    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?
> 
I missed that.


> [... 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.

I intended this patch to be used by someone wishing to build a geode 
rom. I wasn't thinking about inclusion yet.

> 
> 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
> 

I am happy that you would be willing to commit. An updated file is on 
the way.

	Thanks,
	Chris Kindt