Patchwork kconfig: Allow per-board VGA BIOS file name and PCI ID defaults

login
register
about
Submitter Uwe Hermann
Date 2009-10-26 14:30:17
Message ID <20091026143017.GW22827@greenwood>
Download mbox | patch
Permalink /patch/489/
State Accepted
Headers show

Comments

Uwe Hermann - 2009-10-26 14:30:17
On Mon, Oct 26, 2009 at 02:24:36AM +0100, Peter Stuge wrote:
> Uwe Hermann wrote:
> > Allow per-board VGA BIOS file name and PCI ID defaults.
> 
> NAK actually. For one, please talk about the relationship between the
> proposed VGA_BIOS_{FILE,ID} and the previous
> FALLBACK_VGA_BIOS_{FILE,ID} ?

At first I thought this is needed due to a kconfig limitation. But after
some testing it seems we don't need to introduce a new variable in this
case. I think the kconfig limitation was related to not being able to
define "choice" values twice (but this is not a "choice" option, so not
a problem here).


> Also I assume there will at one point be NORMAL_*, right?

No idea. I don't know the rationale behind the FALLBACK_VGA_BIOS_* name.
Anyone else?

Maybe we should rename FALLBACK_VGA_BIOS_* to just VGA_BIOS_* if the
FALLBACK_* names don't make sense. That's for another patch though.


> If the purpose is strictly to provide a useful default for each board

Yes. And/or for each northbridge.


> then I think VGA_BIOS_FILE should be dropped, because the VGA BIOS
> only really has a well known filename for QEMU.

No, not really. For most users there is a useful per-board default
file name, namely the one that you get when you do

  awardeco bios.bin -xa

(or phnxdeco or amideco or bios_extract)

on the vendor BIOS image to get the VGA blob. In the case of the
VIA pc2500e board this is "M14CRT.ROM", hence my patch. For the
Kontron 986LCD-M/mITX it is "amipci_01.20" for example.

The user can simply copy that file into the top-level coreboot-v2
directory, select the board in menuconfig, and both the file name
for the VGA blob as well as the PCI ID defaults will already be
correct for his or her board, which I find is very user-friendly.


> The PCI ID isn't really a board property. Most of the time the it is
> a function of the northbridge, but sometimes it IS set by the board.

Yep.


> Can Kconfig deal with a single value being set in more than one
> place?

Yes (except for "choice" fields, but a workaround with additional
variables can be done for those too).


> How is precedence determined? Even if it can, that method
> feels kind of ugly.

Why is that ugly? We have variables that have sane defaults
and can be overridden per-chipset or per-board (or by the user in
menuconfig if needed). It makes perfect sense, IMHO.

 
> Can a single value get it's default from one of several other values,
> depending on settings, with a particular precedence specified?

Good question, we should check/test that.

The inclusion order in src/Kconfig is:

source src/mainboard/Kconfig
source src/arch/i386/Kconfig
source src/arch/ppc/Kconfig
source src/northbridge/Kconfig
source src/devices/Kconfig
source src/southbridge/Kconfig
source src/superio/Kconfig
source src/cpu/Kconfig

So mainboard values are included first, then northbridge ones. Setting a
default PCI ID in i945, and another one in Kontron 986LCD-M/mITX worked
fine in a quick test, i.e. the per-board one overrides the northbridge
one. This would mean the first definition is used, later ones don't
override (which sounds a bit strange, but maybe I missed something, it
was just a quick test).


> NB must be able to set VGA_PCI_ID.

That's possible, no problem.


> Board must also be able to set VGA_PCI_ID.

Also possible.


> If neither have set it, there can be no VGA BIOS and
> the option should not even be visible in the menu.

Should also be doable, but that's not what this patch is about. I can
send a follow-up patch later.

I just tested that a "menu" entry can also have "depends on" lines
and if the dependency is not satisfied the menu is not shown. Shouldn't
be a big problem.


> If only NB has
> set an ID, use that ID. If only board, or both NB and board have set
> ID, then use the board ID.

Possible too (see new patch).


Updated patch attached, which eliminateѕ the additional variables and
adds settings for the Kontron (only file name is per-board, PCI IDs are
per-northbridge). Seems to work fine.


Uwe.
Peter Stuge - 2009-10-30 02:33:22
Uwe Hermann wrote:
> > then I think VGA_BIOS_FILE should be dropped, because the VGA BIOS
> > only really has a well known filename for QEMU.
> 
> No, not really. For most users there is a useful per-board default
> file name, namely the one that you get when you do
> 
>   awardeco bios.bin -xa
..
> the file name for the VGA blob as well as the PCI ID defaults will
> already be correct for his or her board, which I find is very
> user-friendly.

Agree!


> > Can Kconfig deal with a single value being set in more than one
> > place?
> 
> Yes (except for "choice" fields, but a workaround with additional
> variables can be done for those too).
> 
> 
> > How is precedence determined? Even if it can, that method
> > feels kind of ugly.
> 
> Why is that ugly?

Because it is not immediately obvious (to us, right now) what the
precedence is.


> We have variables that have sane defaults and can be overridden
> per-chipset or per-board (or by the user in menuconfig if needed).
> It makes perfect sense, IMHO.

Except it's not quite clear how it works.. That doesn't make sense.


> So mainboard values are included first, then northbridge ones. Setting a
> default PCI ID in i945, and another one in Kontron 986LCD-M/mITX worked
> fine in a quick test, i.e. the per-board one overrides the northbridge
> one. This would mean the first definition is used, later ones don't
> override (which sounds a bit strange, but maybe I missed something, it
> was just a quick test).

It could well be that this is how it works, but we have to know if it
is a rather firm design property of confauto, or if it's just a
coincidence.


> Allow per-northbridge and per-board VGA BIOS file name and PCI ID defaults.
> 
> Of course, the user can still override those defaults, if needed.
> 
> Add defaults for the VIA pc2500e and the Kontron 986LCD-M/mITX board.
> 
> Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>

Acked-by: Peter Stuge <peter@stuge.se>


> +++ src/Kconfig	(Arbeitskopie)
> @@ -379,7 +379,7 @@
>  	  The path and filename of the file to use as VGA BIOS.
>  
>  config FALLBACK_VGA_BIOS_ID
> -	string "VGA BIOS ID"
> +	string "VGA BIOS PCI IDs"
>  	depends on VGA_BIOS
>  	default "1106,3230"
>  	help

Something like "VGA PCI ID" or "VGA PCI device ID" please. The ID is
not a property of the BIOS but of the hardware device that this BIOS
should be attached to.


//Peter
Uwe Hermann - 2009-10-30 12:58:27
On Fri, Oct 30, 2009 at 03:33:22AM +0100, Peter Stuge wrote:
> Acked-by: Peter Stuge <peter@stuge.se>

Thanks, r4891.


> > +++ src/Kconfig	(Arbeitskopie)
> > @@ -379,7 +379,7 @@
> >  	  The path and filename of the file to use as VGA BIOS.
> >  
> >  config FALLBACK_VGA_BIOS_ID
> > -	string "VGA BIOS ID"
> > +	string "VGA BIOS PCI IDs"
> >  	depends on VGA_BIOS
> >  	default "1106,3230"
> >  	help
> 
> Something like "VGA PCI ID" or "VGA PCI device ID" please. The ID is
> not a property of the BIOS but of the hardware device that this BIOS
> should be attached to.

Fixed.

I also took the freedom to add the IDs and file name for the MSI MS-6178
which I know about and tested on hardware.


Uwe.

Patch

Allow per-northbridge and per-board VGA BIOS file name and PCI ID defaults.

Of course, the user can still override those defaults, if needed.

Add defaults for the VIA pc2500e and the Kontron 986LCD-M/mITX board.

Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>

Index: src/Kconfig
===================================================================
--- src/Kconfig	(Revision 4855)
+++ src/Kconfig	(Arbeitskopie)
@@ -379,7 +379,7 @@ 
 	  The path and filename of the file to use as VGA BIOS.
 
 config FALLBACK_VGA_BIOS_ID
-	string "VGA BIOS ID"
+	string "VGA BIOS PCI IDs"
 	depends on VGA_BIOS
 	default "1106,3230"
 	help
Index: src/mainboard/kontron/986lcd-m/Kconfig
===================================================================
--- src/mainboard/kontron/986lcd-m/Kconfig	(Revision 4855)
+++ src/mainboard/kontron/986lcd-m/Kconfig	(Arbeitskopie)
@@ -60,3 +60,9 @@ 
 	int
 	default 2
 	depends on BOARD_KONTRON_986LCD_M
+
+config FALLBACK_VGA_BIOS_FILE
+	string
+	default "amipci_01.20"
+	depends on BOARD_KONTRON_986LCD_M
+
Index: src/mainboard/via/pc2500e/Kconfig
===================================================================
--- src/mainboard/via/pc2500e/Kconfig	(Revision 4855)
+++ src/mainboard/via/pc2500e/Kconfig	(Arbeitskopie)
@@ -47,3 +47,13 @@ 
 	default 0xaa51
 	depends on BOARD_VIA_PC2500E
 
+config FALLBACK_VGA_BIOS_FILE
+	string
+	default "M14CRT.ROM"
+	depends on BOARD_VIA_PC2500E
+
+config FALLBACK_VGA_BIOS_ID
+	string
+	default "1106,3344"
+	depends on BOARD_VIA_PC2500E
+
Index: src/northbridge/intel/i945/Kconfig
===================================================================
--- src/northbridge/intel/i945/Kconfig	(Revision 4855)
+++ src/northbridge/intel/i945/Kconfig	(Arbeitskopie)
@@ -20,3 +20,9 @@ 
 config NORTHBRIDGE_INTEL_I945
 	bool
 	select HAVE_HIGH_TABLES
+
+config FALLBACK_VGA_BIOS_ID
+	string
+	default "8086,27a2"
+	depends on NORTHBRIDGE_INTEL_I945
+