Re: [coreboot] [PATCH] more kconfig support

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