[coreboot] [PATCH] more kconfig support
Uwe Hermann
uwe at hermann-uwe.de
Mon Sep 28 15:23:47 CEST 2009
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.
> Current status:
> The tree has 114 boards, of which 46 build with kconfig now, so there
> are 68 missing.
>
> As the patch has grown a bit, see
> http://www.coresystems.de/~patrick/current.patch
Please post to the list nevertheless (with Signed-off-by), it's much
easier to review that way.
Some parts inlined here for review:
> 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 (?).
> 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"?
> 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.
> ===================================================================
> --- src/cpu/intel/socket_mPGA603/Kconfig (Revision 0)
> +++ src/cpu/intel/socket_mPGA603/Kconfig (Revision 0)
> @@ -0,0 +1,6 @@
> +config CPU_INTEL_SOCKET_MPGA603
> + bool
> + default false
Ditto.
> Index: src/cpu/intel/socket_mPGA604/Kconfig
> ===================================================================
> --- src/cpu/intel/socket_mPGA604/Kconfig (Revision 0)
> +++ src/cpu/intel/socket_mPGA604/Kconfig (Revision 0)
> @@ -0,0 +1,6 @@
> +config CPU_INTEL_SOCKET_MPGA604
> + bool
> + default false
Ditto.
> Index: src/cpu/intel/socket_mPGA478/Kconfig
> ===================================================================
> --- src/cpu/intel/socket_mPGA478/Kconfig (Revision 0)
> +++ src/cpu/intel/socket_mPGA478/Kconfig (Revision 0)
> @@ -0,0 +1,5 @@
> +config CPU_INTEL_SOCKET_MPGA478
> + bool
> + default false
Ditto.
> 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?
> Index: src/cpu/intel/socket_mPGA479M/Kconfig
> ===================================================================
> --- src/cpu/intel/socket_mPGA479M/Kconfig (Revision 0)
> +++ src/cpu/intel/socket_mPGA479M/Kconfig (Revision 0)
> @@ -0,0 +1,5 @@
> +config CPU_INTEL_SOCKET_MPGA479M
> + bool
> + default false
See above.
> 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?
> 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?
> 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.
> Index: src/mainboard/via/epia-m/auto.c
> ===================================================================
> --- src/mainboard/via/epia-m/auto.c (Revision 4672)
> +++ src/mainboard/via/epia-m/auto.c (Arbeitskopie)
> @@ -20,7 +20,7 @@
>
> /*
> */
> -void udelay(int usecs)
> +void udelay(unsigned usecs)
"unsigned int" then, for consistency.
> Index: src/mainboard/via/epia/auto.c
> ===================================================================
> --- src/mainboard/via/epia/auto.c (Revision 4672)
> +++ src/mainboard/via/epia/auto.c (Arbeitskopie)
> @@ -16,7 +16,7 @@
>
> /*
> */
> -void udelay(int usecs)
> +void udelay(unsigned usecs)
Ditto.
> 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.
> 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)?
> +
> $(obj)/mainboard/$(MAINBOARDDIR)/failover.inc: $(obj)/romcc $(src)/arch/i386/lib/failover.c
> - $(obj)/romcc -mcpu=p2 -O2 --label-prefix=failover $(INCLUDES) $(src)/arch/i386/lib/failover.c -o $@
> + $(obj)/romcc -mcpu=$(CPUTYPE) -O2 --label-prefix=failover $(INCLUDES) $(src)/arch/i386/lib/failover.c -o $@
>
> ifeq ($(CONFIG_HAVE_OPTION_TABLE),y)
> $(obj)/mainboard/$(MAINBOARDDIR)/auto.inc: $(obj)/romcc $(src)/mainboard/$(MAINBOARDDIR)/auto.c $(obj)/option_table.h
> - $(obj)/romcc -mcpu=p2 -O2 $(INCLUDES) $(src)/mainboard/$(MAINBOARDDIR)/auto.c -o $@
> + $(obj)/romcc -mcpu=$(CPUTYPE) -O2 $(INCLUDES) $(src)/mainboard/$(MAINBOARDDIR)/auto.c -o $@
> else
> $(obj)/mainboard/$(MAINBOARDDIR)/auto.inc: $(obj)/romcc $(src)/mainboard/$(MAINBOARDDIR)/auto.c
> - $(obj)/romcc -mcpu=p2 -O2 $(INCLUDES) $(src)/mainboard/$(MAINBOARDDIR)/auto.c -o $@
> + $(obj)/romcc -mcpu=$(CPUTYPE) -O2 $(INCLUDES) $(src)/mainboard/$(MAINBOARDDIR)/auto.c -o $@
> endif
> 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?
> + obj-y += vgarom.o
> + else
> + obj-y += no_vgarom.o
> + endif
> +else
> + obj-y += no_vgarom.o
> +endif
> +include $(src)/mainboard/Makefile.romccboard.inc
> Index: src/mainboard/intel/truxton/Makefile.inc
> ===================================================================
> --- src/mainboard/intel/truxton/Makefile.inc (Revision 0)
> +++ src/mainboard/intel/truxton/Makefile.inc (Revision 0)
> @@ -0,0 +1,3 @@
> +CPUTYPE := p4 -fno-simplify-phi
Hm, this is a bit of a hack, maybe make a generic ROMCC_CFLAGS then
like this?
ROMCC_CFLGAGS = -mcpu=p4 -fno-simplify-phi
Or add another variable in addition to CPUTYPE.
> 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.
> +CPUTYPE := p4
> +include $(src)/mainboard/Makefile.romccboard.inc
> Index: src/northbridge/via/cn700/Kconfig
> ===================================================================
> --- src/northbridge/via/cn700/Kconfig (Revision 0)
> +++ src/northbridge/via/cn700/Kconfig (Revision 0)
[...]
> +config HAVE_HIGH_TABLES
> + bool
> + default y
> + depends on NORTHBRIDGE_VIA_CN700
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)...
The same issue also applies to some other boards.
> 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?
> 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?
> Index: src/northbridge/intel/i945/Makefile.inc
> ===================================================================
> --- src/northbridge/intel/i945/Makefile.inc (Revision 4672)
> +++ src/northbridge/intel/i945/Makefile.inc (Arbeitskopie)
> @@ -17,8 +17,6 @@
> # Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> #
>
> -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?
> @@ -233,7 +242,7 @@
> CFLAGS = $(STACKPROTECT) $(INCLUDES) $(MAINBOARD_OPTIONS) -Os -nostdinc
> CFLAGS += -nostdlib -Wall -Wundef -Wstrict-prototypes -Wmissing-prototypes
> CFLAGS +=-Wwrite-strings -Wredundant-decls -Wno-trigraphs
> -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?
Uwe.
--
http://www.hermann-uwe.de | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
More information about the coreboot
mailing list