Re: [coreboot] [PATCH] More Kconfig changes

[Uwe Hermann]

On Mon, Oct 19, 2009 at 03:18:07PM -0600, Myles Watson wrote:
> Define HAVE_INIT_TIMER to only exclude the three boards that define it to be
> 0 in newconfig.

What exactly does HAVE_INIT_TIMER configure? Why do those three boards
_not_ set it? It sounds like all boards would want to set it?
> MOVNTI is a performance enhancement, and should default to 0 so it doesn't
> break boards that forget to define it.

Are all Kconfig files setting to y (via "select" preferrably) where needed?
> Index: svn/src/Kconfig
>
> --- svn.orig/src/Kconfig
> +++ svn/src/Kconfig
> @@ -74,10 +74,6 @@ config CPU_ADDR_BITS
>    int
>    default 36
>
> -config AGP_APERTURE_SIZE
> hex
> default 0x0

Why is this removed?
> +config HAVE_LOW_TABLES
> bool
> default y
> help
>  This Option is unused in the code.  Since two boards try to set it to
>  'n', they may be broken.  We either need to make the option useful or
>  get rid of it.  The broken boards are:
>  asus/m2v-mx_se
>  supermicro/h8dme

Hm, indeed. Why was this added? Are there situations where we might not
want LOW_TABLES on?
> +config HAVE_HIGH_TABLES
> bool
> default n
> help
>  This variable specifies whether a given northbridge has high table
>  support.
>  It is set in northbridge/*/Kconfig.
>  Whether or not the high tables are actually written by coreboot is
>  configurable by the user via WRITE_HIGH_TABLES.
>
> config HAVE_ACPI_TABLES
>    bool
>    help
> @@ -262,14 +280,30 @@ config HAVE_PIRQ_TABLE
>      Whether or not the PIRQ table is actually generated by coreboot
>      is configurable by the user via GENERATE_PIRQ_TABLE.
>
> -config HAVE_HIGH_TABLES
> +#These Options are here to avoid "undefined" warnings.
> +#The actual selection and help texts are in the following menu.
> Index: svn/src/northbridge/amd/amdfam10/Kconfig
>
> --- svn.orig/src/northbridge/amd/amdfam10/Kconfig
> +++ svn/src/northbridge/amd/amdfam10/Kconfig
> @@ -21,11 +21,35 @@ config NORTHBRIDGE_AMD_AMDFAM10
>    bool
>    select HAVE_HIGH_TABLES
>    select HYPERTRANSPORT_PLUGIN_SUPPORT
> select HT3_SUPPORT

[...]
> +config HT3_SUPPORT
> bool
> default y
> depends on NORTHBRIDGE_AMD_AMDFAM10

Why this? "select HT3_SUPPORT" should work just fine, no?
Even if that's not the case I think it would be nicer to set it in some
global Kconfig file so we can "select" it. IMHO _all_ y variables should
be "select"ed so we avoid the 4-line chunks...
> +config AMDMCT
> bool
> default y
> depends on NORTHBRIDGE_AMD_AMDFAM10

Ditto.
> +config MEM_TRAIN_SEQ
> +config HW_MEM_HOLE_SIZEK
> +config HW_MEM_HOLE_SIZE_AUTO_INC
> Index: svn/src/northbridge/amd/amdk8/Kconfig
> ===================================================================
> --- svn.orig/src/northbridge/amd/amdk8/Kconfig
> +++ svn/src/northbridge/amd/amdk8/Kconfig
> +config MEM_TRAIN_SEQ
> +config HW_MEM_HOLE_SIZEK
> +config HW_MEM_HOLE_SIZE_AUTO_INC

Why are these three defined twice?
> Index: svn/src/southbridge/via/vt8237r/vt8237r.h
>
> --- svn.orig/src/southbridge/via/vt8237r/vt8237r.h
> +++ svn/src/southbridge/via/vt8237r/vt8237r.h
> @@ -65,7 +65,7 @@
>  #define I2C_TRANS_CMD			0x40
>  #define CLOCK_SLAVE_ADDRESS		0x69
>
> -#if DEBUG_SMBUS == 1
> +#if defined(DEBUG_SMBUS) && DEBUG_SMBUS == 1
>  #define PRINT_DEBUG(x)		print_debug(x)
>  #define PRINT_DEBUG_HEX16(x)	print_debug_hex16(x)
>  #else

Please drop this hunk, I have a patch in the queue to redo DEBUG_SMBUS
and other DEBUG options globally anyway.
> Index: svn/src/cpu/amd/model_10xxx/init_cpus.c
>
> --- svn.orig/src/cpu/amd/model_10xxx/init_cpus.c
> +++ svn/src/cpu/amd/model_10xxx/init_cpus.c
> @@ -473,8 +473,8 @@ static void start_node(u8 node)
>    /* Enable routing table */
>    printk_debug("Start node %02x", node);
>
> -#if CONFIG_CAR_FAM10 == 1
> /* For CONFIG_CAR_FAM10 support, we need to set Dram base/limit for the new node */
>
> +#if CONFIG_NORTHBRIDGE_AMD_AMDFAM10 == 1

#if CONFIG_NORTHBRIDGE_AMD_AMDFAM10
should suffice, I think.

Uwe.