Re: [coreboot] PATCH: Fix CMOS Tables support for all boards.

[Myles Watson]

On Thu, Jun 24, 2010 at 7:08 AM, Edwin Beasant
 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 [e-mail]

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

Thanks,
Myles

PATCH: Fix CMOS Tables support for all boards.

[Edwin Beasant]

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
>  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 [e-mail]
>
> 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
>
> Thanks,
> Myles
> < 
> use_options 
> .diff
> >
>
> < 
> options_fixup 
> .diff><have_option_polarity.diff><use_option_default.diff>

PATCH: Fix CMOS Tables support for all boards.

[Myles Watson]

> 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
> >  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 [e-mail]
> >
> > 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
> >
> > Thanks,
> > Myles
> > <
> > use_options
> > .diff
> > >
> >
> > <
> > options_fixup
> > .diff><have_option_polarity.diff><use_option_default.diff>

PATCH: Fix CMOS Tables support for all boards.

[Myles Watson]

On Wed, Jun 30, 2010 at 11:52 AM, Edwin Beasant
 wrote:
> Seems best of all worlds to me :-)
> Can we get an ack and a commit?

Rev 5653.

Thanks,
Myles

PATCH: Fix CMOS Tables support for all boards.

[Stefan Reinauer]

On 7/6/10 11:05 PM, Myles Watson wrote:
> On Wed, Jun 30, 2010 at 11:52 AM, Edwin Beasant
>  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

PATCH: Fix CMOS Tables support for all boards.

[Myles Watson]

> 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

PATCH: Fix CMOS Tables support for all boards.

[Myles Watson]

On Wed, Jul 7, 2010 at 8:24 AM, Myles Watson  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&...

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

PATCH: Fix CMOS Tables support for all boards.

[Stefan Reinauer]

On 7/7/10 8:28 PM, Myles Watson wrote:
> On Wed, Jul 7, 2010 at 8:24 AM, Myles Watson  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&...
>
> 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

PATCH: Fix CMOS Tables support for all boards.

[Myles Watson]

On Wed, Jul 7, 2010 at 4:02 PM, Stefan Reinauer
 wrote:
> On 7/7/10 8:28 PM, Myles Watson wrote:
> > On Wed, Jul 7, 2010 at 8:24 AM, Myles Watson  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&...
> >
> > 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