| 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
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>
> 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>
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
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
> 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
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
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
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"