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

[Uwe Hermann]

On Wed, Oct 07, 2009 at 08:22:52PM +0200, Peter Stuge wrote:
> > +++ 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

Probably, but it's equivalent right now as we only support one type of
payload in v2 so far (unless I'm mistaken).

Oh, wait. I'm confused. Do you mean "depends on !PAYLOAD_NONE" maybe?
> > @@ -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.

If by "component" you mean mainboard I agree. As the VGA blobs are
extracted from BIOS images and we can check the IDs for a given board
we can easily set those as per-board defaults (but still leave them
changeable by the user).
> > +menu "Debugging"
>
> Good. Loglevel should go in here as well.

Maybe, maybe not. Loglevel also looks correct in "Console options",
but both have advantages.
> > +++ 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.

Ah, that's part of another patch I'm preparing, as discussed on IRC.

- User-visible (viewable in menuconfig) help texts use the "help"
   keyword (otherwise they don't appear in menuconfig).

- Invisible help texts (those for options which don't appear in
   menuconfig) will be changed to #-comments. These are _not_ meant for
   users (they won't see them in menuconfig anyway) but rather
   for developers viewing the Kconfig files.

Not all help texts are distinguished in this way yet, that's what I
intend to fix over time.
> > +++ 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.

I don't understand what you mean here.
> 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.

Hm, true. Are there corner cases (soft-reboots, resume, ...) where we
may _not_ want to re-init serial or re-set the BAUD rate?

Or maybe "override" the hardware defaults is meant here? Not sure.
> > 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.

Yep, it's on my TODO list.
> > +# 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"

Will change to "EHCI Debug Port".
> > 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?

I think Stefan mentioned on IRC recently that it's pretty useless and we
might remove support for this completely in future.
> > +++ 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.

Yep, sounds good.
> > +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.

The whole "choice" construct depends on stuff:

depends on PCI_ROM_RUN || VGA_ROM_RUN
> > choice
> >    prompt "ROM chip size"
> >    default COREBOOT_ROMSIZE_KB_256
>
> Does this need CONFIG_ prepended? Hopefully not.

No. In Kconfig files CONFIG_ prefixes are not needed (and not allowed),
in Makefiles and C code, you need CONFIG_.
> > +# 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?

Hm, why? There should be as many default lines as we allow ROM sizes to
be selected in menuconfig.

Uwe.