[coreboot] [PATCH] more kconfig support

[Patrick Georgi]

Am Montag, den 28.09.2009, 15:23 +0200 schrieb Uwe Hermann:
> On Fri, Sep 25, 2009 at 07:37:19PM +0200, Patrick Georgi wrote:
> >   - via/pc2500e
> 
> Neat, I'll give this one a test run on hardware soonish.
I committed it, so just using HEAD should do it.
The configuration options might still be way off, I'll handle them all
once all boards are supported.

> Some parts inlined here for review:
Thanks


> > Index: src/southbridge/via/Makefile.inc
> > ===================================================================
> > --- src/southbridge/via/Makefile.inc	(Revision 4672)
> > +++ src/southbridge/via/Makefile.inc	(Arbeitskopie)
> > @@ -1,5 +1,4 @@
> > -#subdirs-y += k8t890
> > -#subdirs-y += vt8231
> > -#subdirs-y += vt8235
> > +subdirs-$(CONFIG_SOUTHBRIDGE_VIA_K8T890) += k8t890
> > +subdirs-$(CONFIG_SOUTHBRIDGE_VIA_VT8231) += vt8231
> > +subdirs-$(CONFIG_SOUTHBRIDGE_VIA_VT8235) += vt8235
> >  subdirs-$(CONFIG_SOUTHBRIDGE_VIA_VT8237R) += vt8237r
> 
> > -#subdirs-y += vt82c686
> 
> This will still be required later I guess (?).
vt82c686 is a weird stub of southbridge support: I found no use of it,
and it seems to handle serial enable, only. My proposal? Unless it will
actually be used, get rid of it.


> > Index: src/southbridge/via/k8t890/Makefile.inc
> > ===================================================================
> > --- src/southbridge/via/k8t890/Makefile.inc	(Revision 0)
> > +++ src/southbridge/via/k8t890/Makefile.inc	(Revision 0)
> [...]
> > -# arg. How does the linux kconfig handle compound conditionals?
> > -ifeq ($(CONFIG_HAVE_SMI_HANDLER),y)
> > -	object-$(CONFIG_SOUTHBRIDGE_INTEL_I82801GX) += i82801gx_smi.o
> > -	smmobj-$(CONFIG_SOUTHBRIDGE_INTEL_I82801GX) += i82801gx_smihandler.o
> > -endif
> > +object-$(CONFIG_HAVE_SMI_HANDLER) += i82801gx_smi.o
> > +smmobj-$(CONFIG_HAVE_SMI_HANDLER) += i82801gx_smihandler.o
> 
> What's the proposed resolution for these "compound conditionals"?
In this case, CONFIG_SOUTHBRIDGE_INTEL_I82801GX is already covered by
the statement that includes this very Makefile.inc.
In general, the ifeq(..) thing is our solution for compound
conditionals, but this time we got away with it.


> > Index: src/cpu/intel/ep80579/Kconfig
> > ===================================================================
> > --- src/cpu/intel/ep80579/Kconfig	(Revision 0)
> > +++ src/cpu/intel/ep80579/Kconfig	(Revision 0)
> > @@ -0,0 +1,3 @@
> > +config CPU_INTEL_EP80579
> > +	bool
> > +	default false
> 
> "default n" for consistency.
right. I think that might be the result of copy&paste from other,
similar files.


> > Index: src/cpu/intel/model_1067x/Kconfig
> > ===================================================================
> > --- src/cpu/intel/model_1067x/Kconfig	(Revision 0)
> > +++ src/cpu/intel/model_1067x/Kconfig	(Revision 0)
> > @@ -0,0 +1,5 @@
> > +config CPU_INTEL_CORE2
> > +	bool
> > +	default y
> > +	select SMP
> > +	select HAVE_MOVNTI
> 
> The syntax (i.e. using default y) means that all model_1067x are Core2?
> Is that correct?
I think so. If not, it should be renamed to CPU_INTEL_MODEL_1067X in my
opinion.


> > Index: src/mainboard/supermicro/h8dme/Kconfig
> > ===================================================================
> > --- src/mainboard/supermicro/h8dme/Kconfig	(Revision 0)
> > +++ src/mainboard/supermicro/h8dme/Kconfig	(Revision 0)
> [...]
> > +config HAVE_HARD_RESET
> > +	bool
> > +	default y
> > +	depends on BOARD_SUPERMICRO_H8DME
> 
> This can be a simple "select HAVE_HARD_RESET" above, but I assume
> you'll do this kind of changes with a script later?
Not sure if these can be scripted. I had this default to "n" first, but
that didn't compile, and I'm not sure yet what the right selection here
is.
And defaulting to "n" requires this more verbose statement.


> > Index: src/mainboard/supermicro/h8dme/Makefile.inc
> > ===================================================================
> > --- src/mainboard/supermicro/h8dme/Makefile.inc	(Revision 0)
> > +++ src/mainboard/supermicro/h8dme/Makefile.inc	(Revision 0)
> [...]
> > +ifdef POST_EVALUATION
> > +
> > +MAINBOARD_OPTIONS=\
> > +	-DCONFIG_AP_IN_SIPI_WAIT=0 \
> > +	-DCONFIG_USE_PRINTK_IN_CAR=1 \
> > +	-DCONFIG_HAVE_HIGH_TABLES=1
> 
> What's the difference between using MAINBOARD_OPTIONS here vs. adding
> these settings in the Kconfig file for the respective mainboard?
> Can't we eliminate MAINBOARD_OPTIONS?
It might be possible to eliminate it by now. I _think_ it started out as
a workaround in the early stages of kconfig, but I didn't dare to remove
it, yet.
That might be a good, isolated, low-risk issue to look into: Do we still
need it? Could we kill it by changing the makefile?


> > Index: src/mainboard/tyan/s2850/Kconfig
> > ===================================================================
> > --- src/mainboard/tyan/s2850/Kconfig	(Revision 0)
> > +++ src/mainboard/tyan/s2850/Kconfig	(Revision 0)
> > @@ -0,0 +1,65 @@
> > +config BOARD_TYAN_S2850
> > +	bool "Tyan S2850"
> 
> Nope, only "S2850" here, vendor name should not be in here.
> Maybe "Tomcat K8S (S2850)" as per wiki / vendor website, see
> http://www.coreboot.org/Supported_Motherboards
> 
> Same for some other Tyan boards, and maybe also some non-Tyan, didn't
> check.
The names were guesswork in many places. Validating them against
official names is another such "low risk" job.


> > Index: src/mainboard/via/pc2500e/Kconfig
> > ===================================================================
> > --- src/mainboard/via/pc2500e/Kconfig	(Revision 0)
> > +++ src/mainboard/via/pc2500e/Kconfig	(Revision 0)
> > @@ -0,0 +1,45 @@
> > +config BOARD_VIA_PC2500E
> > +	bool "PC2500E"
> 
> pc2500e in this case, the vendor also writes it in all-lowercase.
> Maybe I'll fix this type of strings in one big committ soonish.
I'd be glad!


> > Index: src/mainboard/Makefile.romccboard.inc
> > ===================================================================
> > --- src/mainboard/Makefile.romccboard.inc	(Revision 4672)
> > +++ src/mainboard/Makefile.romccboard.inc	(Arbeitskopie)
> > @@ -44,15 +44,17 @@
> >  
> >  ifdef POST_EVALUATION
> >  
> > +CPUTYPE ?= p2
> 
> Yep, this kind of fix was on my TODO list also.
> 
> Maybe ROMCC_MCPU (a bit more descriptive I guess)?
This is a more generic ROMCCFLAGS, containing -mcpu=p2 now (with a tiny
issue Myles fixed. I should stop doing last minute changes before
commits)


> > Index: src/mainboard/intel/xe7501devkit/Makefile.inc
> > ===================================================================
> > --- src/mainboard/intel/xe7501devkit/Makefile.inc	(Revision 0)
> > +++ src/mainboard/intel/xe7501devkit/Makefile.inc	(Revision 0)
> > @@ -0,0 +1,13 @@
> > +CPUTYPE := p4
> > +obj-$(CONFIG_HAVE_ACPI_TABLES) += acpi_tables.o
> 
> > +ifeq ($(CONFIG_PCI_ROM_RUN),y)
> > +	ifeq ($(CONFIG_PCI_ROM_RUN),y)
> 
> This looks incorrect. Wrong variable name in the second ifeq?
uhmm.. yes. CONFIG_CONSOLE_VGA is the right one. fixed locally.


> Hm, this is a bit of a hack, maybe make a generic ROMCC_CFLAGS then
> like this?
> 
>   ROMCC_CFLGAGS = -mcpu=p4 -fno-simplify-phi
It's like that in upstream, yes. It really was a hack.


> > Index: src/mainboard/intel/jarrell/Makefile.inc
> > ===================================================================
> > --- src/mainboard/intel/jarrell/Makefile.inc	(Revision 0)
> > +++ src/mainboard/intel/jarrell/Makefile.inc	(Revision 0)
> > @@ -0,0 +1,4 @@
> > +obj-y += reset.o
> 
> This should probably be protected via CONFIG_HAVE_HARD_RESET and can
> then go into the generic Makefile.romccboard.inc, I think.
It's unprotected in Config.lb, but that might be because HAVE_HARD_RESET
defaults to 1 there.


> I think HAVE_HIGH_TABLES is y per default already, so this can be
> omitted (?)
> 
> The interesting question is whether or not all boards/chipsets fully
> work with HAVE_HIGH_TABLES already (I don't know the answer)...
If everything would work fully, I would have promoted dropping
HAVE_HIGH_TABLES for a long time already. There were some remaining
problems.


> > Index: src/northbridge/amd/Makefile.inc
> > ===================================================================
> > --- src/northbridge/amd/Makefile.inc	(Revision 4672)
> > +++ src/northbridge/amd/Makefile.inc	(Arbeitskopie)
> > @@ -1,7 +1,5 @@
> >  subdirs-$(CONFIG_NORTHBRIDGE_AMD_AMDFAM10) += amdfam10
> > -subdirs-$(CONFIG_NORTHBRIDGE_AMD_AMDHT) += amdht
> >  subdirs-$(CONFIG_NORTHBRIDGE_AMD_AMDK8) += amdk8
> > -subdirs-$(CONFIG_NORTHBRIDGE_AMD_AMDMCT) += amdmct
> 
> Why are these two removed?
As far as I can see, these directories contain common support code for
the northbridges, but aren't northbridges themselves.
They can be included from within the northbridges that require them.

> > Index: src/northbridge/intel/i440bx/Kconfig
> > ===================================================================
> > --- src/northbridge/intel/i440bx/Kconfig	(Revision 4672)
> > +++ src/northbridge/intel/i440bx/Kconfig	(Arbeitskopie)
> > +config HAVE_HIGH_TABLES
> > +	bool
> > +	default y
> 
> No depends on 440BX NB here?
Oops. Fixed locally.

> > -driver-$(CONFIG_NORTHBRIDGE_INTEL_I945) += northbridge.o
> > -driver-$(CONFIG_NORTHBRIDGE_INTEL_I945) += gma.o
> > -ifeq ($(CONFIG_HAVE_ACPI_TABLES),y)
> > -	obj-$(CONFIG_NORTHBRIDGE_INTEL_I945) += acpi.o
> > -endif
> > +driver-y += northbridge.o
> > +driver-y += gma.o
> > +obj-$(CONFIG_HAVE_ACPI_TABLES) += acpi.o
> 
> Are the two versions really equivalent?
As the file is only included with CONFIG_NORTHBRIDGE_INTEL_I945, yes.


> > -CFLAGS += -Werror-implicit-function-declaration -Wstrict-aliasing -Wshadow 
> > +CFLAGS += -Wstrict-aliasing -Wshadow 
> >  CFLAGS += -fno-common -ffreestanding -fno-builtin -fomit-frame-pointer
> 
> Needed for romcc boards which have lots of these warnings/errors I assume?
Yes. Eventually that should come back, once the code is clean.


Patrick