Patchwork Re-enable option table for the ASUS MEW-VM and fix build

On Thu, Sep 24, 2009 at 10:39:55AM +0200, Patrick Georgi wrote:
> Hi,
> 
> with this patch (and my previous one), all kconfig enabled boards build
> for me.
> 
> Changes this time:
> - Fix for superio/smsc/lpc47b272
> - disable option table for asus/mew-vm
> - change superio to use for compaq/deskpro_en_sff_p600 to be in line
>   with what newconfig/buildtarget does
> 
> 
> Signed-off-by: Patrick Georgi <patrick.georgi@coresystems.de>

This was r4666.

Most of the patch looks good, but disabling HAVE_OPTION_TABLE is
incorrect I think, as the board _does_ have a cmos.layout file.

The bug which prevented a successful build was a typo in the
Makefile. Attached patch should fix it.


> Index: src/mainboard/compaq/deskpro_en_sff_p600/Kconfig
> ===================================================================
> --- src/mainboard/compaq/deskpro_en_sff_p600/Kconfig	(Revision 4665)
> +++ src/mainboard/compaq/deskpro_en_sff_p600/Kconfig	(Arbeitskopie)
> @@ -24,7 +24,8 @@
>  	select CPU_INTEL_SLOT_2
>  	select NORTHBRIDGE_INTEL_I440BX
>  	select SOUTHBRIDGE_INTEL_I82371EB
> -	select SUPERIO_NSC_PC97307
> +	# should be SUPERIO_NSC_PC97307!
> +	select SUPERIO_NSC_PC97317
>  	select HAVE_PIRQ_TABLE
>  	select UDELAY_IO
>  	select PCI_ROM_RUN

Yep, the PC97307 doesn't properly build right now (if it ever did), will
look into it.


Uwe.

Am Donnerstag, den 24.09.2009, 14:52 +0200 schrieb Uwe Hermann:
> Most of the patch looks good, but disabling HAVE_OPTION_TABLE is
> incorrect I think, as the board _does_ have a cmos.layout file.
I simply made it correspond to the newconfig configuration:
Options.lb:default CONFIG_HAVE_OPTION_TABLE = 0

> The bug which prevented a successful build was a typo in the
> Makefile. Attached patch should fix it.
I don't care about any improvements over that at this time, but the
change in Makefile.romccboard.inc looks fine.
In case this is build tested by you (esp. that asus/mew-vm really
builds), this is

Acked-by: Patrick Georgi <patrick.georgi@coresystems.de>

On Thu, Sep 24, 2009 at 03:04:10PM +0200, Patrick Georgi wrote:
> Am Donnerstag, den 24.09.2009, 14:52 +0200 schrieb Uwe Hermann:
> > Most of the patch looks good, but disabling HAVE_OPTION_TABLE is
> > incorrect I think, as the board _does_ have a cmos.layout file.
> I simply made it correspond to the newconfig configuration:
> Options.lb:default CONFIG_HAVE_OPTION_TABLE = 0

Hm, probably yet another copy-paste occurance in our tree, the
cmos.layout got copied and never enabled or so. Anyway, we should
fix this in another patch by (IMHO) making one or two "generic"
cmos.layout files which boards without special needs can use.

I estimate that 80% or so of all those files are identical and/or
blindly copy-pasted. Those boards that _do_ need a differing cmos.layout
should have their own custom file, but the "don't care" rest of the
boards can then use a global/common one.


> > The bug which prevented a successful build was a typo in the
> > Makefile. Attached patch should fix it.
> I don't care about any improvements over that at this time, but the
> change in Makefile.romccboard.inc looks fine.
> In case this is build tested by you (esp. that asus/mew-vm really
> builds), this is
> 
> Acked-by: Patrick Georgi <patrick.georgi@coresystems.de>

Thanks, r4669. I manually tested the board via menuconfig.


Uwe.

Uwe Hermann wrote:
> Those boards that _do_ need a differing cmos.layout should have
> their own custom file, but the "don't care" rest of the boards can
> then use a global/common one.

The ideal would of course be for all board settings to be compatible!

It would be interesting to clean this up and find out how many board
specific settings there actually exist in the tree.


//Peter

My $.02?

I think few people understand the format of the cmos file, and fewer
still know how to change it to eliminate all the un-needed settings.

A very nice contribution for someone to make would be a new file
format which people could understand. I don't have any good ideas in
that area. The current format has the advantage of being very
bit-efficient with the limited CMOS memory. But it's not much use if
people are blindly cutting and pasting.

ron

rminnich wrote:
> I think few people understand the format of the cmos file, and
> fewer still know how to change it to eliminate all the un-needed
> settings.

Personally I would just copy an existing file into a new port because
I have no particular requirements for the new port. I think that's
common and would like to optimize for it, by having defaults, and
allowing them to be extended.

Which tools are currently using the files? nvramtool, and.. ?


//Peter

Am Donnerstag, den 24.09.2009, 18:37 +0200 schrieb Peter Stuge:
> rminnich wrote:
> > I think few people understand the format of the cmos file, and
> > fewer still know how to change it to eliminate all the un-needed
> > settings.
> 
> Personally I would just copy an existing file into a new port because
> I have no particular requirements for the new port. I think that's
> common and would like to optimize for it, by having defaults, and
> allowing them to be extended.
> 
> Which tools are currently using the files? nvramtool, and.. ?
Another thing to consider when/if this is being worked on, is that there
should be defaults defined in that file somehow. That way, we can build
a table with sensible values to choose if the cmos values are obviously
incorrect (eg. invalid encodings, broken checksum, etc).

This would get rid of a couple of
#if HAVE_OPTION_TABLE
   baud = get_cmos(blabla)
#else
   baud = 115200
#endif
-style statements in the code, while making it more robust at the same
time - and without the need for fallback/normal for this purpose (normal
is usually compiled with option table, fallback without - so that a
broken cmos configuration leads to an eventual reset into fallback, with
"stable" values).

With some smart macro management, this could lead to
non-HAVE_OPTION_TABLE builds using those defaults because they're
converted to an .h file that includes a get_cmos() macro doing the right
thing.

Anyway, I'm happy with every small improvement, too :-)


Patrick

rminnich wrote:
> My $.02?
>
> I think few people understand the format of the cmos file, and fewer
> still know how to change it to eliminate all the un-needed settings.
>   
This comes up from time to time.. But I'm wondering... How could it be
simpler?  What is it that people find complicated?

Let's discuss this with a sample, so it's easier to grab

http://tracker.coreboot.org/trac/coreboot/browser/trunk/coreboot-v2/src/mainboard/kontron/986lcd-m/cmos.layout

The tricky part is to know which options are used in the code of a given
board, but that's not something a file system can really improve.


> The current format has the advantage of being very
> bit-efficient with the limited CMOS memory. But it's not much use if
> people are blindly cutting and pasting.
>   
Any format or data structure will suffer from cut and paste frenzy ...

Stefan

Patrick Georgi wrote:
> there should be defaults defined in that file somehow
..
> non-HAVE_OPTION_TABLE builds using those defaults

Yeah!


//Peter

Stefan Reinauer wrote:
> Any format or data structure will suffer from cut and paste frenzy
> ...

Only if data is _required_. If there are defaults, and local changes
are possible but optional, there's no reason to copypaste.


//Peter

Peter Stuge wrote:
> Stefan Reinauer wrote:
>   
>> Any format or data structure will suffer from cut and paste frenzy
>> ...
>>     
>
> Only if data is _required_. If there are defaults, and local changes
> are possible but optional, there's no reason to copypaste.
>   
Oh, yes... but the general simplicity does not decrease by adding config
file inheritance.

People will assume wrong files they inherit from, or have conflicts
between different parents and childs.  Now they have a few copy and
pasted extra entries, that do nothing, if the board porter was not very
thorough.

Stefan

On Thu, Sep 24, 2009 at 9:56 AM, Stefan Reinauer <stepan@coresystems.de> wrote:

> http://tracker.coreboot.org/trac/coreboot/browser/trunk/coreboot-v2/src/mainboard/kontron/986lcd-m/cmos.layout

Beautiful sample. Best one I've seen anyway.

Perhaps if we had had that sample from the start we would be better
off. The current samples everyone uses have settings in them that were
initially created for a machine that was sold to LANL 7 years ago :-)

Maybe my real concern is that the error messages are, in my view,
incomprehensible. You really
have to read the code to see what they really mean. That's easy to
fix. In fact I thought I had but maybe not.

ron