[PATCH] more kconfig support

[Patrick Georgi]

Hi,

it's not going as fast as I thought, but still, some progress:
(all of this excluding PPC hardware, and I might have missed something)
- northbridges are done
- southbridges are done
- Intel CPUs are done, with a design that the board only has to specify
  the socket it has, and the CPUs are pulled in automatically. There is
  some more cleanup possible in that area, but I'll do that later
- a couple more mainboards compile:
  - intel/eagleheights
  - intel/jarrell
  - intel/mtarvon
  - intel/truxton
  - intel/xe7501devkit
  - sunw/ultra40
  - supermicro/h8dme
  - tyan/s2850
  - tyan/s2875
  - via/epia
  - via/epia-cn
  - via/epia-m
  - via/epia-m700
  - via/epia-n
  - via/pc2500e

Given that pretty much all devices should be kconfig'd now, I guess that
I'll be faster with the others.
All of them only _build_, I guess some options are still completely
wrong, I'll handle that in one big pass (with tool support) when all
boards build.

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

Regards,
Patrick

[ron minnich]

Acked-by: Ronald G. Minnich

Wonderful.

ron

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

[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

[Stefan Reinauer]

Patrick Georgi wrote:
> > > 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.

Acked-by: Stefan Reinauer
> > > 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.

Is that really the implication? I thought it means that if
model_1067x/Kconfig is loaded, code that belongs to core2 is also
selected. That's good, and it should be different from the assumption
that all mode_1067x are core2, even though I think this is correct (some
might be Atom, though, not 100% sure)

Stefan

[Peter Stuge]

Stefan Reinauer wrote:
> > > > +++ 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.
>
> Is that really the implication? I thought it means that if
> model_1067x/Kconfig is loaded, code that belongs to core2 is also
> selected. That's good, and it should be different from the
> assumption that all mode_1067x are core2, even though I think this
> is correct (some might be Atom, though, not 100% sure)

Example from Linux:

--8<-- drivers/parport/Kconfig
if PARPORT
config PARPORT_PC
        tristate "PC-style hardware"
        depends on (!SPARC64 || PCI) && !SPARC32 && !M32R && !FRV && \
                   (!M68K || ISA) && !MN10300 && !AVR32 && !BLACKFIN
..
config PARPORT_PC_FIFO
        bool "Use FIFO/DMA if available (EXPERIMENTAL)"
        depends on PARPORT_PC && EXPERIMENTAL
..
config PARPORT_PC_PCMCIA
        tristate "Support for PCMCIA management for PC-style ports"
        depends on PCMCIA && PARPORT_PC
..
config PARPORT_AMIGA
        tristate "Amiga builtin port"
        depends on AMIGA
        select PARPORT_NOT_PC
..
config PARPORT_AX88796
        tristate "AX88796 Parallel Port"
        select PARPORT_NOT_PC
..
config PARPORT_NOT_PC
        bool

endif # PARPORT
-->8--

--8<-- include/linux/parport.h
/* If PC hardware is the only type supported, we can optimise a bit. 
*/
#if !defined(CONFIG_PARPORT_NOT_PC)

#include <linux/parport_pc.h>
#define parport_write_data(p,x)            parport_pc_write_data(p,x)
#define parport_read_data(p)               parport_pc_read_data(p)
..
#else  /*  !CONFIG_PARPORT_NOT_PC  */

/* Generic operations vector through the dispatch table. */
#define parport_write_data(p,x)            (p)->ops->write_data(p,x)
#define parport_read_data(p)               (p)->ops->read_data(p)
..
#endif /*  !CONFIG_PARPORT_NOT_PC  */
-->8--

We might make more use of Kconfig if in our Kconfigs. But for select,
it seems it does mean that stuff will be defined for the source code.

//Peter

[Uwe Hermann]

On Tue, Sep 29, 2009 at 01:30:04AM +0200, Stefan Reinauer wrote:
> Patrick Georgi wrote:
> > > > 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.
>
> Acked-by: Stefan Reinauer

Please don't drop this. I have some boards with this southbridge, and
the code that's there looks OK (though unfinished, of course). I'll
put this on my TODO list and improve the SB code one of these days.

Uwe.

[Stefan Reinauer]

Uwe Hermann wrote:
> On Tue, Sep 29, 2009 at 01:30:04AM +0200, Stefan Reinauer wrote:
> > Patrick Georgi wrote:
> > > > > 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.
> >
> > Acked-by: Stefan Reinauer
>
> Please don't drop this. I have some boards with this southbridge, and
> the code that's there looks OK (though unfinished, of course). I'll
> put this on my TODO list and improve the SB code one of these days.

I think we should not keep around broken components like that. An early
serial init like that is trivial and not more than half an hour of
effort. On the other side, the existence of the component in the tree
will create a wrong impression about what coreboot can do or can not do.

This code is not unfinished, as it's not being worked on..  It's in svn
history in case we'll some day have the time to support this.

Same reasoning as goes for the ppc port...

Stefan

[Peter Stuge]

Uwe Hermann wrote:
> > 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?

PLEASE don't wait until later!

New code being developed between first introduction of Kconfig and
"later" will inherit improper use of Kconfig - I so don't want that.

If a board is not using Kconfig perfectly I think it's better that it
does not build!

That is one (IMO good) way to encourage proper Kconfig use.

At this point only very few developers really know how Kconfig is
supposed to be used, and the rest of us are fighting the learning
curve - with delight. However, it is very easy to use bad examples -
which then causes more things that need fixing.

Can we avoid this?
> > +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?

Yes, I'd also like to know.
> > +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.

Go for it.
> > ifdef POST_EVALUATION

Again with POST_EVALUATION. What is that?
> > +CPUTYPE ?= p2
>
> Yep, this kind of fix was on my TODO list also.
>
> Maybe ROMCC_MCPU (a bit more descriptive I guess)?

Yes please!
> > +++ 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

Yes please!

Thanks for finding this Uwe.

//Peter

[Patrick Georgi]

Am Dienstag, den 29.09.2009, 01:38 +0200 schrieb Peter Stuge:
> > This can be a simple "select HAVE_HARD_RESET" above, but I assume
> > you'll do this kind of changes with a script later?
>
> PLEASE don't wait until later!

"later" means immediately after all boards are kconfig-aware.
My plan is:
1. get them to build
2. compare old and new build, and make new match old in terms of
   configuration options
3. let other people fix up the remaining issues as they run into them
   with their boards (I don't have all boards around)

I won't fight fixes to existing boards, but I'm not working
board-by-board, that's just too much a hassle.
> New code being developed between first introduction of Kconfig and
> "later" will inherit improper use of Kconfig - I so don't want that.
>
> If a board is not using Kconfig perfectly I think it's better that it
> does not build!

I can easily add a typo everywhere - that won't help you much.
> Can we avoid this?

The issue is mostly that configuration options are missing, but nothing
"structural".
While there are many things I want to clean up once the old build system
is gone, that will be _later_ steps. I don't want kconfig be killed by
too-many-things-at-once.
> > Can't we eliminate MAINBOARD_OPTIONS?
>
> Yes, I'd also like to know.

Legacy stuff from early development. Soon to be gone (see other mail).
> > Hm, this is a bit of a hack, maybe make a generic ROMCC_CFLAGS then
> > like this?
> >
> > ROMCC_CFLGAGS = -mcpu=p4 -fno-simplify-phi
>
> Yes please!

Done, called ROMCCFLAGS, with -mcpu=p2 as default