[coreboot] [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