Re: [coreboot] [patch] Kconfig support for M57SLI

[Uwe Hermann]

On Wed, Aug 26, 2009 at 08:14:39PM +0200, Harald Gutmann wrote:
> Index: src/mainboard/gigabyte/m57sli/Kconfig
>
> --- src/mainboard/gigabyte/m57sli/Kconfig	(revision 0)
> +++ src/mainboard/gigabyte/m57sli/Kconfig	(revision 0)
> @@ -0,0 +1,197 @@
> +choice
> prompt "Mainboard model"
> depends on VENDOR_GIGABYTE

Please check current svn, we use a different method for this now.
src/mainboard/gigabyte/Kconfig includes all board files.

Example: src/mainboard/asus/Kconfig and src/mainboard/asus/p2b-f/Kconfig
> +config BOARD_GIGABYTE_M57SLI
> bool "M57SLI"

Full names as per vendor website please, in this case:

config BOARD_GIGABYTE_GA_M57SLI_S4
    bool "GA-M57SLI-S4"

There are multiple occurences below, please fix them all.
> select ARCH_X86
> select CPU_AMD_K8
> select CPU_AMD_SOCKET_AM2
> select NORTHBRIDGE_AMD_AMDK8
> select NORTHBRIDGE_AMD_AMDK8_ROOT_COMPLEX
> select SOUTHBRIDGE_NVIDIA_MCP55
> select SUPERIO_ITE_IT8716F
> select PIRQ_TABLE

HAVE_PIRQ_TABLE
> select USE_PRINTK_IN_CAR
> help
> Gigabyte M57SLI mainboard
>
> +endchoice
>
> +config MAINBOARD_DIR
> string
> default gigabyte/m57sli

^^^^^^^
  This is the only exception, should stay "m57sli" as it's a dirname.
> depends on BOARD_GIGABYTE_M57SLI
>
> +config USE_DCACHE_RAM
> int

Should be bool, but this is no longer needed as of recent svn,
you can use "select USE_DCACHE_RAM" above.
Or just drop it, the default is y anyway in src/cpu/amd/model_fxx/Kconfig.
> default 1
> depends on BOARD_GIGABYTE_M57SLI
>
> +config DCACHE_RAM_BASE
> hex
> default 0xc8000
> depends on BOARD_GIGABYTE_M57SLI
>
> +config DCACHE_RAM_SIZE
> hex
> default 0x08000
> depends on BOARD_GIGABYTE_M57SLI
>
> +config DCACHE_RAM_GLOBAL_VAR_SIZE
> hex
> default 0x01000
> depends on BOARD_GIGABYTE_M57SLI

These three also also no longer needed, pre-defined in
src/cpu/amd/model_fxx/Kconfig.
> +config APIC_ID_OFFSET
> int
> default 16
> depends on BOARD_GIGABYTE_M57SLI
>
> +config HAVE_HARD_RESET
> bool
> default y
> depends on BOARD_GIGABYTE_M57SLI
>
> +config HAVE_HIGH_TABLES
> bool
> default y
> depends on BOARD_GIGABYTE_M57SLI
>
> +config IOAPIC
> bool
> default y
> depends on BOARD_GIGABYTE_M57SLI

All "bool"s that are set to y can be written a lot shorter as:

select HAVE_HARD_RESET
select HAVE_HIGH_TABLES
select IOAPIC

above (in the "BOARD_GIGABYTE_GA_M57SLI_S4" section)
> +config LB_CKS_RANGE_START
> int
> default 49

Not needed, default is 49 already.
> +config MAINBOARD_PART_NUMBER
> string
> default "m57sli"

default "GA-M57SLI-S4"
> depends on BOARD_GIGABYTE_M57SLI
>
> +config PCI_64BIT_PREF_MEM
> int
>    default 0
> depends on BOARD_GIGABYTE_M57SLI

This is defined bool in src/devices/Kconfig so you can use
"select PCI_64BIT_PREF_MEM".
> +config HAVE_FALLBACK_BOOT
> bool
> default n
> depends on BOARD_GIGABYTE_M57SLI
>
> +config USE_FALLBACK_IMAGE
> bool
> default n
> depends on BOARD_GIGABYTE_M57SLI

This looks strange. You can probably drop them, but if they stay
HAVE_FALLBACK_BOOT should be y (it's enabled in Options.lb),
not sure about USE_FALLBACK_IMAGE...
> +config HW_MEM_HOLE_SIZEK
> hex
> default 0x100000
> depends on BOARD_GIGABYTE_M57SLI

Hm, not yet defined in kconfig, should probably move somewhere global
(this is work in progress), but I'm not sure.
> +config HAVE_FAILOVER_BOOT
> int
> default 0
> depends on BOARD_GIGABYTE_M57SLI
>
> +config USE_FAILOVER_IMAGE
> int
> default 0
> depends on BOARD_GIGABYTE_M57SLI

Not sure either, see above.
> +config AP_CODE_IN_CAR
> int
> default 0
> depends on BOARD_GIGABYTE_M57SLI

bool
> +config HW_MEM_HOLE_SIZE_AUTO_INC
> int
> default 0
> depends on BOARD_GIGABYTE_M57SLI

Probably bool.
> +config HT_CHAIN_END_UNITID_BASE
> int
> default 0
> depends on BOARD_GIGABYTE_M57SLI

Looks ok, this is _not_ a bool, AFAIK.
> +config USE_INIT
> int
> default 0
> depends on BOARD_GIGABYTE_M57SLI

bool
> +config SERIAL_CPU_INIT
> bool
> default n
> depends on BOARD_GIGABYTE_M57SLI

It's actually commented in Options.lb, so the default from
src/config/Options.lb probably is in effect, which is:

define CONFIG_SERIAL_CPU_INIT
        default 1
        export always
        comment "Serialize CPU init"
end

So this should be "select SERIAL_CPU_INIT" I think.
> +config WAIT_BEFORE_CPUS_INIT
> int
> default 0
> depends on BOARD_GIGABYTE_M57SLI

bool
> +config CONSOLE_VGA
> bool
> default y
> depends on BOARD_GIGABYTE_M57SLI
>
> +config PCI_ROM_RUN
> int
> default 1
> depends on BOARD_GIGABYTE_M57SLI

Both are bool so you can use "select", but these two will probably
be moved to a global place anyway.
> +config K8_REV_F_SUPPORT
> hex
> default 1
> depends on BOARD_GIGABYTE_M57SLI

Probably not needed, the Kconfig file for the CPU socket
sets this one.
> +config FANCTL
> int
> default 1
> depends on BOARD_GIGABYTE_M57SLI

Never defined in kconfig so far, but it should be bool. Also,
you use CONFIG_HAVE_FANCTL below, so it must be named "HAVE_FANCTL"
here.
> +config HAVE_ACPI_TABLES
> bool
> default y
> depends on BOARD_GIGABYTE_M57SLI

select HAVE_ACPI_TABLES
> Index: src/mainboard/gigabyte/m57sli/devicetree.cb
>
> --- src/mainboard/gigabyte/m57sli/devicetree.cb	(revision 4583)
> +++ src/mainboard/gigabyte/m57sli/devicetree.cb	(working copy)
> @@ -1,202 +1,175 @@
>  chip northbridge/amd/amdk8/root_complex
>    device apic_cluster 0 on
>            chip cpu/amd/socket_AM2
>                    device apic 0 on end
>            end
>    end

[...]

The diff for this is very hard to read and has many whitespace changes.
What is it supposed to do? Purely cosmetic changes? Or are there
functional changes in there? Which?
> Index: src/mainboard/gigabyte/Kconfig
>
> --- src/mainboard/gigabyte/Kconfig	(revision 4583)
> +++ src/mainboard/gigabyte/Kconfig	(working copy)
> @@ -1 +1 @@
> -#
> +source "src/mainboard/gigabyte/m57sli/Kconfig"

This part is correct but some additional stuff is needed, see
asus/Kconfig for an example.
> Index: src/northbridge/amd/amdk8/Makefile.inc
>
> --- src/northbridge/amd/amdk8/Makefile.inc	(revision 4583)
> +++ src/northbridge/amd/amdk8/Makefile.inc	(working copy)
> @@ -1,8 +1,3 @@
>
> -#default CONFIG_AGP_APERTURE_SIZE=0x4000000
> -#default CONFIG_HAVE_HIGH_TABLES=1

Yep, Makefile.inc is the wrong place. Are they in Kconfig now or
dropped?

Uwe.