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