Submitter | Uwe Hermann |
---|---|
Date | 2009-09-24 12:52:41 |
Message ID | <20090924125241.GA18588@greenwood> |
Download | mbox | patch |
Permalink | /patch/291/ |
State | Accepted |
Commit | r4669 |
Headers | show |
Comments
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
Patch
Re-enable option table for the ASUS MEW-VM and fix build. Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de> Index: src/mainboard/Makefile.romccboard.inc =================================================================== --- src/mainboard/Makefile.romccboard.inc (Revision 4668) +++ src/mainboard/Makefile.romccboard.inc (Arbeitskopie) @@ -47,7 +47,7 @@ $(obj)/mainboard/$(MAINBOARDDIR)/failover.inc: $(obj)/romcc $(src)/arch/i386/lib/failover.c $(obj)/romcc -mcpu=p2 -O2 --label-prefix=failover $(INCLUDES) $(src)/arch/i386/lib/failover.c -o $@ -ifeq ($(HAVE_OPTION_TABLE),y) +ifeq ($(CONFIG_HAVE_OPTION_TABLE),y) $(obj)/mainboard/$(MAINBOARDDIR)/auto.inc: $(obj)/romcc $(src)/mainboard/$(MAINBOARDDIR)/auto.c $(obj)/option_table.h $(obj)/romcc -mcpu=p2 -O2 $(INCLUDES) $(src)/mainboard/$(MAINBOARDDIR)/auto.c -o $@ else Index: src/mainboard/asus/mew-vm/Kconfig =================================================================== --- src/mainboard/asus/mew-vm/Kconfig (Revision 4668) +++ src/mainboard/asus/mew-vm/Kconfig (Arbeitskopie) @@ -42,11 +42,6 @@ default "MEW-VM" depends on BOARD_ASUS_MEW_VM -config HAVE_OPTION_TABLE - bool - default n - depends on BOARD_ASUS_MEW_VM - config IRQ_SLOT_COUNT int default 11