Re: [coreboot] [v2] r4734 - in trunk/coreboot-v2/src: . arch/i386 arch/ppc console devices mainboard

[Peter Stuge]

[e-mail] wrote:
> Author: uwe
> Log:
> Various Kconfig fixes and improvements:

It would have been nice to discuss some of these things first.
> +++ trunk/coreboot-v2/src/Kconfig	2009-10-07 16:15:40 UTC (rev 4734)

..
> +# TODO: Defined if no payload? Breaks build?
> +config COMPRESSED_PAYLOAD_LZMA
> bool "Use LZMA compression for payloads"
> default y
> depends on PAYLOAD_ELF
> help
>  In order to reduce the size payloads take up in the ROM chip
>  coreboot can compress them using the LZMA algorithm.

Maybe:

default !PAYLOAD_NONE
> @@ -319,11 +330,20 @@
>    depends on VGA_BIOS
>    default "1106,3230"
>    help
>  The ID that would associate your VGA BIOS to your video card.
>  (PCI VendorID, PCI Device ID)
>  The comma-separated PCI vendor and device ID that would associate
>  your VGA BIOS to your video card.
>
>  Example: 1106,3230
>  In the above example 1106 is the PCI vendor ID (in hex, but without
>  the "0x" prefix) and 3230 specifies the PCI device ID of the
>  video card (also in hex, without "0x" prefix).

This is a fun one. I think the default here should be empty, and
should be overridden in suitable component Kconfigs.
> +menu "Debugging"

Good. Loglevel should go in here as well.
> +++ trunk/coreboot-v2/src/arch/i386/Kconfig	2009-10-07 16:15:40 UTC (rev 4734)
> @@ -1,23 +1,22 @@
> +# This option is used to set the architecture of a mainboard to X86.
> +# It is usually set in mainboard/*/Kconfig.
>  config ARCH_X86
>    bool
> help
>  This option is used to set the architecture of a mainboard.
>  It is usually set in mainboard/*/Kconfig.
> default n

I'm not so sure about this. There is a point in having these help
texts in the Kconfig structure.

It is becoming more and more clear to me that the upstream Kconfig
may not be completely sufficient for us on it's own.
> +++ trunk/coreboot-v2/src/console/Kconfig	2009-10-07 16:15:40 UTC (rev 4734)
> @@ -1,53 +1,96 @@
>  config CONSOLE_SERIAL8250

..
> bool "Enable serial port console output"
> default y
> help
>  Send coreboot debug output to a serial port console.

Put the help message in the flag description. But maybe use
"coreboot message" rather than "coreboot debug output".
> config SERIAL_SET_SPEED
> bool "Override the serial console BAUD rate"
> bool "Override the serial port BAUD rate"
> default y
> depends on CONSOLE_SERIAL8250

Override what? There is noone before us.
> config TTYS0_BAUD
> int "Serial console BAUD rate"
> int "Serial port BAUD rate"
> depends on SERIAL_SET_SPEED
> default 115200

This should become a list of options.
> +# TODO: FIX DEPENDENCY HERE
>  config USBDEBUG_DIRECT
> bool "USB debug dongle support. Not supported on all chipsets."
> bool "USB 2.0 EHCI debug dongle support"

This should probably be either "EHCI Debug Port" or
"USB Debug Class compliant device"
> config CONSOLE_VGA
> bool "Use VGA console, once initialized."
> bool "Use VGA console once initialized"
> default n

What is it that can use the VGA console? This setting is not really
about using VGA as console - or is it? Can anything actually ever be
seen there?
> +++ trunk/coreboot-v2/src/devices/Kconfig	2009-10-07 16:15:40 UTC (rev 4734)

..
> config VGA_ROM_RUN
> bool "Run VGA Option ROMs"
> bool "Run VGA option ROMs"
> default y
> help
>  Execute VGA option ROMs if found. This is required to enable PCI/AGP
>  VGA plugin cards.
>  Execute VGA option ROMs, if found. This is required to enable
>  PCI/AGP/PCI-E video cards.

Does this option have anything to do with the VGA BIOS stuff which is
in another place in Kconfig? One idea would be to move these things
together into the same menu.
> +config PCI_OPTION_ROM_RUN_REALMODE

..
>  If you select this option, PCI option ROMs will be executed
>  natively on the hardware (a 32bit x86 system is required).

So this should depend on something.
> +++ trunk/coreboot-v2/src/mainboard/Kconfig	2009-10-07 16:15:40 UTC (rev 4734)

..
> +# TODO: No help text possible for choice fields?

Dunno.
> choice
>    prompt "ROM chip size"
>    default COREBOOT_ROMSIZE_KB_256

Does this need CONFIG_ prepended? Hopefully not.
> +# Map the config names to a hex value (bytes).
>  config ROM_SIZE
>    hex
>    default 0x20000 if COREBOOT_ROMSIZE_KB_128

Are more default lines needed here?

//Peter