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