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

[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.

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

[Peter Stuge]

Uwe Hermann wrote:
> 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?

Yes, that too. Maybe default y is OK with depends on !PAYLOAD_NONE?

Is the default value ignored if the dependency is not satisfied?
> > > default "1106,3230"
> > >    help
> > >  The comma-separated PCI vendor and device ID that would associate
> > >  your VGA BIOS to your video card.

..
> > 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.

Not really, more like northbridge. Most boards which need a cbfs VGA
BIOS have the PCI device in the northbridge, so that's where I think
the default value belongs.

Some boards have discrete onboard graphics and need a VGA BIOS for
that - then let the mainboard set another default.
> 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).

Even if the VGA BIOS is different from one board to another with the
same northbridge (think VIA boards with and without TV driver e.g.) I
think the PCI ID will stay the same. No?
> > > +++ 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.
> >
> > I'm not so sure about this. There is a point in having these help
> > texts in the Kconfig structure.

..
> 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.

If the help messages are within the Kconfig structure it will be
possible to automatically produce documentation for developers who
will eventually work inside Kconfig files. I think this is valuable.
Like doxygen for Kconfig. If this information is in comments, then it
is unneccessarily difficult to get an overview of all the options.

I only recently found that the master Options.lb has a description of
each option. That is very nice, and I would like to have that also
with Kconfig.
> Not all help texts are distinguished in this way yet, that's what I
> intend to fix over time.

I understand that this was the intention, but I'm not sure it's the
best place to store those comments.
> > > +++ 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.

Suggest

-	bool "Enable serial port console output"
+	bool "Send coreboot messages to a serial port console."

and remove the help text.
> > > 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?

Interesting.
> Or maybe "override" the hardware defaults is meant here? Not sure.

Me neither. Does anyone else know? In any case, maybe we just need to
change the wording slightly to make it more clear?

"Set a serial port baud rate"

With a help text describing what happens if it is not selected.
> > > 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.

I'm fine with that.

VGA BIOS options control if the payload will be able to use VGA.
That's all that matters.
> > > +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

I was thinking about "a 32bit x86 system" in particular. If it
doesn't run on 64bit systems, I think we should use Kconfig to hide
this choice.
> > 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_.

Good. Thanks for clarification! :)
> > > +# 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.

Yeah, sorry, I thought they weren't there but they were just not in
the context of the hunk.

//Peter