Patchwork PATCH: Fix CMOS Tables support for all boards.

login
register
about
Submitter Myles Watson
Date 2010-06-25 19:54:14
Message ID <AANLkTin2Rhs2X0Nv-4SbsSfMXrvKdgcjoT1kRkEQ1QLA@mail.gmail.com>
Download mbox | patch
Permalink /patch/1555/
State Accepted
Headers show

Comments

Myles Watson - 2010-06-25 19:54:14
On Thu, Jun 24, 2010 at 7:08 AM, Edwin Beasant
<edwin_beasant@virtensys.com> wrote:
> Re-integrated the “USE_OPTION_TABLE” code so that it will compile for all
> boards that define it at configure time.
>
> Made USE_OPTION_TABLE dependant on HAVE_OPTION_TABLE
>
> Signed-off-by: Edwin Beasant edwin_beasant@virtensys.com
I updated your patch to:
- Have a default value for USE_OPTION_TABLE even when
HAVE_OPTION_TABLE isn't defined
- Pass abuild with USE_OPTION_TABLE defaulting to y or n

use_options.diff: Your patch, slightly modified
options_fixup.diff: Fix boards to include the .h file instead of the .c file
have_option_polarity.diff: Default to no cmos.layout file so it can be
selected (like most other options)

Patch for testing with USE_OPTION_TABLE defaulting to y
     use_option_default.diff

I think it would make sense to default to using the option table if it
is present.

A next step might be to remove the "default" parameter from
read_option and make it get the default from option_table.h.

Signed-off-by: Myles Watson <mylesgw@gmail.com>

Thanks,
Myles
Edwin Beasant - 2010-06-30 17:52:29
Seems best of all worlds to me :-)
Can we get an ack and a commit?
Thanks,
Edwin
On 25 Jun 2010, at 20:54, Myles Watson wrote:

> On Thu, Jun 24, 2010 at 7:08 AM, Edwin Beasant
> <edwin_beasant@virtensys.com> wrote:
>> Re-integrated the “USE_OPTION_TABLE” code so that it will compile  
>> for all
>> boards that define it at configure time.
>>
>> Made USE_OPTION_TABLE dependant on HAVE_OPTION_TABLE
>>
>> Signed-off-by: Edwin Beasant edwin_beasant@virtensys.com
> I updated your patch to:
> - Have a default value for USE_OPTION_TABLE even when
> HAVE_OPTION_TABLE isn't defined
> - Pass abuild with USE_OPTION_TABLE defaulting to y or n
>
> use_options.diff: Your patch, slightly modified
> options_fixup.diff: Fix boards to include the .h file instead of  
> the .c file
> have_option_polarity.diff: Default to no cmos.layout file so it can be
> selected (like most other options)
>
> Patch for testing with USE_OPTION_TABLE defaulting to y
>     use_option_default.diff
>
> I think it would make sense to default to using the option table if it
> is present.
>
> A next step might be to remove the "default" parameter from
> read_option and make it get the default from option_table.h.
>
> Signed-off-by: Myles Watson <mylesgw@gmail.com>
>
> Thanks,
> Myles
> < 
> use_options 
> .diff 
> > 
> < 
> options_fixup 
> .diff><have_option_polarity.diff><use_option_default.diff>
Myles Watson - 2010-06-30 18:46:40
> Seems best of all worlds to me :-)
> Can we get an ack and a commit?
Anyone can ack.  If you've reviewed it and tested it on your board, you'd be
in the best position for that.

Thanks,
Myles

> On 25 Jun 2010, at 20:54, Myles Watson wrote:
> 
> > On Thu, Jun 24, 2010 at 7:08 AM, Edwin Beasant
> > <edwin_beasant@virtensys.com> wrote:
> >> Re-integrated the "USE_OPTION_TABLE" code so that it will compile
> >> for all
> >> boards that define it at configure time.
> >>
> >> Made USE_OPTION_TABLE dependant on HAVE_OPTION_TABLE
> >>
> >> Signed-off-by: Edwin Beasant edwin_beasant@virtensys.com
> > I updated your patch to:
> > - Have a default value for USE_OPTION_TABLE even when
> > HAVE_OPTION_TABLE isn't defined
> > - Pass abuild with USE_OPTION_TABLE defaulting to y or n
> >
> > use_options.diff: Your patch, slightly modified
> > options_fixup.diff: Fix boards to include the .h file instead of
> > the .c file
> > have_option_polarity.diff: Default to no cmos.layout file so it can be
> > selected (like most other options)
> >
> > Patch for testing with USE_OPTION_TABLE defaulting to y
> >     use_option_default.diff
> >
> > I think it would make sense to default to using the option table if it
> > is present.
> >
> > A next step might be to remove the "default" parameter from
> > read_option and make it get the default from option_table.h.
> >
> > Signed-off-by: Myles Watson <mylesgw@gmail.com>
> >
> > Thanks,
> > Myles
> > <
> > use_options
> > .diff
> > >
> > <
> > options_fixup
> > .diff><have_option_polarity.diff><use_option_default.diff>
Myles Watson - 2010-07-06 21:05:42
On Wed, Jun 30, 2010 at 11:52 AM, Edwin Beasant
<edwin_beasant@virtensys.com> wrote:
> Seems best of all worlds to me :-)
> Can we get an ack and a commit?
Rev 5653.

Thanks,
Myles
Stefan Reinauer - 2010-07-07 11:23:54
On 7/6/10 11:05 PM, Myles Watson wrote:
> On Wed, Jun 30, 2010 at 11:52 AM, Edwin Beasant
> <edwin_beasant@virtensys.com> wrote:
>> Seems best of all worlds to me :-)
>> Can we get an ack and a commit?
> Rev 5653.
>
> Thanks,
> Myles
>
Not sure why there were no errors, but:

      9 src/include/pc80/mc146818rtc.h:89:26: warning: option_table.h: No such file or directory
Myles Watson - 2010-07-07 14:24:14
> Rev 5653.
>
> Thanks,
> Myles
>
> Not sure why there were no errors, but:
>
>       9 src/include/pc80/mc146818rtc.h:89:26: warning: option_table.h: No
> such file or directory

That's odd.  I don't see that warning.  Does it have to do with
parallel compilation and insufficient dependency checking?  Which
boards have the warning?

Thanks,
Myles
Myles Watson - 2010-07-07 18:28:14
On Wed, Jul 7, 2010 at 8:24 AM, Myles Watson <mylesgw@gmail.com> wrote:
>> Rev 5653.
>>
>> Thanks,
>> Myles
>>
>> Not sure why there were no errors, but:
>>
>>       9 src/include/pc80/mc146818rtc.h:89:26: warning: option_table.h: No
>> such file or directory
>
> That's odd.  I don't see that warning.  Does it have to do with
> parallel compilation and insufficient dependency checking?  Which
> boards have the warning?
http://qa.coreboot.org/log_buildbrd.php?revision=5657&device=dbm690t&vendor=amd&num=2

option_table.h doesn't get generated until way down the list (after
the warnings)  It looks like the first board that needs this:

 HOSTCC     coreboot-builds/sharedutils/options/build_opt_tbl

And the rest of the build doesn't wait for it.

Thanks,
Myles
Stefan Reinauer - 2010-07-07 22:02:13
On 7/7/10 8:28 PM, Myles Watson wrote:
> On Wed, Jul 7, 2010 at 8:24 AM, Myles Watson <mylesgw@gmail.com> wrote:
>>> Rev 5653.
>>>
>>> Thanks,
>>> Myles
>>>
>>> Not sure why there were no errors, but:
>>>
>>>       9 src/include/pc80/mc146818rtc.h:89:26: warning: option_table.h: No
>>> such file or directory
>> That's odd.  I don't see that warning.  Does it have to do with
>> parallel compilation and insufficient dependency checking?  Which
>> boards have the warning?
> http://qa.coreboot.org/log_buildbrd.php?revision=5657&device=dbm690t&vendor=amd&num=2
>
> option_table.h doesn't get generated until way down the list (after
> the warnings)  It looks like the first board that needs this:
>
>  HOSTCC     coreboot-builds/sharedutils/options/build_opt_tbl
>
> And the rest of the build doesn't wait for it.

This sure looks like a race... question is, what do we do about it?
Force an early build of option_table.h outside of the normal dependency
system?

Why does gcc only print a warning instead of an error here? Are those
#includes not needed at all?

Stefan
Myles Watson - 2010-07-07 22:17:29
On Wed, Jul 7, 2010 at 4:02 PM, Stefan Reinauer
<stefan.reinauer@coresystems.de> wrote:
>  On 7/7/10 8:28 PM, Myles Watson wrote:
>> On Wed, Jul 7, 2010 at 8:24 AM, Myles Watson <mylesgw@gmail.com> wrote:
>>>> Rev 5653.
>>>>
>>>> Thanks,
>>>> Myles
>>>>
>>>> Not sure why there were no errors, but:
>>>>
>>>>       9 src/include/pc80/mc146818rtc.h:89:26: warning: option_table.h: No
>>>> such file or directory
>>> That's odd.  I don't see that warning.  Does it have to do with
>>> parallel compilation and insufficient dependency checking?  Which
>>> boards have the warning?
>> http://qa.coreboot.org/log_buildbrd.php?revision=5657&device=dbm690t&vendor=amd&num=2
>>
>> option_table.h doesn't get generated until way down the list (after
>> the warnings)  It looks like the first board that needs this:
>>
>>  HOSTCC     coreboot-builds/sharedutils/options/build_opt_tbl
>>
>> And the rest of the build doesn't wait for it.
>
> This sure looks like a race... question is, what do we do about it?
> Force an early build of option_table.h outside of the normal dependency
> system?
I don't know what the best fix is.  The problem is the building of the
tool, so abuild could just build the tool before building any boards.
We could also create option_table.h as part of the configure process.

> Why does gcc only print a warning instead of an error here? Are those
> #includes not needed at all?

USE_OPTION_TABLE defaults to false.  HAVE_OPTION_TABLE triggers the
#include so that the options get defined.  If you select
USE_OPTION_TABLE and create the race condition, gcc will error out.

Steps to exercise the race condition for the curious:

rm -rf build
make oldconfig
make -j4

Thanks,
Myles

Patch

Index: svn/src/Kconfig
===================================================================
--- svn.orig/src/Kconfig
+++ svn/src/Kconfig
@@ -82,7 +82,7 @@  config CCACHE
 
 config USE_OPTION_TABLE
 	bool "Use CMOS for configuration values"
-	default n
+	default y
 	depends on HAVE_OPTION_TABLE
 	help
 	  Enable this option if coreboot shall read options from the "CMOS"