[coreboot] [PATCH] More Kconfig changes

Uwe Hermann uwe at hermann-uwe.de
Tue Oct 20 00:17:30 CEST 2009


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.
-- 
http://www.hermann-uwe.de  | http://www.randomprojects.org
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org




More information about the coreboot mailing list