Re: [SeaBIOS] [PATCH 3/3]

[Nils]

Op vrijdag 06-01-2012 om 20:41 uur [tijdzone -0500], schreef Kevin
O'Connor:
> On Sat, Jan 07, 2012 at 12:28:55AM +0100, Nils wrote:
> > Add the GX2 functionality.
> > Improve Kconfig VGA options.
> > Conditionally build VGA bios rom if selected in Kconfig.
>
> Thanks.
>
> I'd like to see Kconfig select whether or not the vgabios is built.
> However, that should really be in a patch separate from the addition
> of GX2 support.
>
> Also, I'd prefer to avoid #ifdefs where possible.
>
> The first two patches look good to me.
>
> [...]
> > +ifneq ($(CONFIG_NO_VGABIOS),y)
> > +all: $(OUT) $(OUT)bios.bin $(OUT)vgabios.bin
> > +else
> >  all: $(OUT) $(OUT)bios.bin
> > +endif
>
> all-y: $(OUT) $(OUT)bios.bin
> all-$(CONFIG_BUILD_VGABIOS) += $(OUT)vgabios.bin
>
> [...]
> > SRCVGA=src/output.c src/util.c vgasrc/vgabios.c vgasrc/vgafb.c \
> >      vgasrc/vgatables.c vgasrc/vgafonts.c vgasrc/vbe.c \
> > vgasrc/stdvga.c vgasrc/clext.c vgasrc/bochsvga.c vgasrc/geodevga.c
> > vgasrc/stdvga.c vgasrc/clext.c vgasrc/bochsvga.c
> >
> > +ifeq ($(CONFIG_VGA_GEODEGX2),y)
> > +SRCVGA +=  vgasrc/geodevga.c
> > +endif
> > +ifeq ($(CONFIG_VGA_GEODELX),y)
> > +SRCVGA +=  vgasrc/geodevga.c
> > +endif
>
> Lets just build vgasrc/geodevga.c unconditionally - the
> compiler/linker will strip it out if it isn't configured.
>
> [...]
> > --- vgasrc/geodevga.c
> > +++ vgasrc/geodevga.c
> > @@ -87,7 +87,12 @@
> >      int ret=0;
> >      union u64_u32_u val;
> >
> > +#if (CONFIG_VGA_GEODEGX2)
> > val=geode_msrRead(GLIU0_P2D_BM_4);
> >
> > +#endif
> > +#if (CONFIG_VGA_GEODELX)
> >      val=geode_msrRead(MSR_GLIU0_BASE4);
> > +#endif
>    if (CONFIG_VGA_GEODEGX2)
>        val=geode_msrRead(GLIU0_P2D_BM_4);
>    else
>        val=geode_msrRead(MSR_GLIU0_BASE4);
>
> [...]
> > +#if (CONFIG_VGA_GEODEGX2)
> > #define VP_MSR_CONFIG               0xc0002001  /* GX2 */
> >
> > +#endif
> > +#if (CONFIG_VGA_GEODELX)
> > #define VP_MSR_CONFIG               0x48002001  /* LX */
> >
> > +#endif
>
> #define VP_MSR_CONFIG_GX2               0xc0002001  /* GX2 */
> #define VP_MSR_CONFIG_LX                0x48002001  /* LX */
>
> and update the code to use an if statement?
>
> [...]
> > --- vgasrc/vgahw.h
> > +++ vgasrc/vgahw.h
> > @@ -22,6 +22,8 @@
> >          return clext_init();
> >      if (CONFIG_VGA_BOCHS)
> >          return bochsvga_init();
> > if (CONFIG_VGA_GEODEGX2)
> >    return geodevga_init();
> > if (CONFIG_VGA_GEODELX)
> >        return geodevga_init();
>    if (CONFIG_VGA_GEODELX || CONFIG_VGA_GEODEGX2)
>        return geodevga_init();

I'm fine with your changes.
You should test the unconditional compilation of vgasrc/geodevga.c when
cirrus or bochs are selected i got a error in that case.

If you make the patches i gladly test them.

Thanks, Nils.