[PATCH] More Kconfig changes

[Myles Watson]

Define some variables that were not defined.  There are a couple left.  Do
kbuildall then grep not.defined kbuildall.results/*
The interesting ones were GENERATE_*  I had to put them in twice to make it
work correctly.  Once outside the menu setting the defaults, and once inside
the menu.  Now they show up when they should, and are always defined

Define HAVE_INIT_TIMER to only exclude the three boards that define it to be
0 in newconfig.
Define MEM_TRAIN_SEQ to be an integer and set it correctly.
Remove CAR_FAM10 and just depend on NORTHBRIDGE_AMD_AMDFAM10
MOVNTI is a performance enhancement, and should default to 0 so it doesn't
break boards that forget to define it.

Signed-off-by: Myles Watson

Thanks,
Myles

[Peter Stuge]

Myles Watson wrote:
> +config GENERATE_ACPI_TABLES
>    bool
> default y if HAVE_ACPI_TABLES
> default n

Can it be simply:

default HAVE_ACPI_TABLES

?
> +++ svn/src/northbridge/amd/amdfam10/Kconfig
> @@ -21,11 +21,35 @@ config NORTHBRIDGE_AMD_AMDFAM10
>    bool
>    select HAVE_HIGH_TABLES
>    select HYPERTRANSPORT_PLUGIN_SUPPORT
> select HT3_SUPPORT
>
> config AGP_APERTURE_SIZE
>    hex
>    default 0x4000000
>    depends on NORTHBRIDGE_AMD_AMDFAM10
>
> +config HT3_SUPPORT
> bool
> default y
> depends on NORTHBRIDGE_AMD_AMDFAM10

Is this good?

I find having single line selects instead of repeating the horribly
long NB option name over and over very appealing but I don't know
enough about Kconfig to say if it makes a difference?

Essentially this is a doubly linked list where both links must be
explicitly described by Kconfig file authors, and I would so prefer
if only one link needed to be described explicitly.

//Peter

[Myles Watson]

On Mon, Oct 19, 2009 at 3:39 PM, Peter Stuge  wrote:
> Myles Watson wrote:
> > +config GENERATE_ACPI_TABLES
> >       bool
> > default y if HAVE_ACPI_TABLES
> > default n
>
> Can it be simply:
>
> default HAVE_ACPI_TABLES

It looks like it could.  We have a mix of the two styles right now.

src/console/Kconfig:    default MAXIMUM_CONSOLE_LOGLEVEL_8
src/console/Kconfig:    default DEFAULT_CONSOLE_LOGLEVEL_8
src/mainboard/Kconfig:    default VENDOR_EMULATION
src/mainboard/Kconfig:    default COREBOOT_ROMSIZE_KB_128 if
BOARD_ROMSIZE_KB_128
src/mainboard/Kconfig:    default COREBOOT_ROMSIZE_KB_256 if
BOARD_ROMSIZE_KB_256
> +++ svn/src/northbridge/amd/amdfam10/Kconfig
> > @@ -21,11 +21,35 @@ config NORTHBRIDGE_AMD_AMDFAM10
> >       bool
> >       select HAVE_HIGH_TABLES
> >       select HYPERTRANSPORT_PLUGIN_SUPPORT
> > select HT3_SUPPORT
> >
> > config AGP_APERTURE_SIZE
> >       hex
> >       default 0x4000000
> >       depends on NORTHBRIDGE_AMD_AMDFAM10
> >
> > +config HT3_SUPPORT
> > bool
> > default y
> > depends on NORTHBRIDGE_AMD_AMDFAM10
>
> Is this good?

It has to be defined somewhere.  The alternative is to put config
HT3_SUPPORT somewhere else, then use select in this file.  Since there are
no other devices with HT3_SUPPORT at this time, I think this is the place to
put it.

If you use select and HT3_SUPPORT isn't defined somewhere else, it silently
fails.
> I find having single line selects instead of repeating the horribly
> long NB option name over and over very appealing

Me too.
> but I don't know
> enough about Kconfig to say if it makes a difference?

In this case it does.

Thanks,
Myles

[Myles Watson]

On Mon, Oct 19, 2009 at 3:51 PM, Myles Watson  wrote:
> On Mon, Oct 19, 2009 at 3:39 PM, Peter Stuge  wrote:
> > Myles Watson wrote:
> > > +config GENERATE_ACPI_TABLES
> > >       bool
> > > default y if HAVE_ACPI_TABLES
> > > default n
> >
> > Can it be simply:
> >
> > default HAVE_ACPI_TABLES
>
> It looks like it could.  We have a mix of the two styles right now.

No.  You're right about the defaults.  Fixed.

Thanks,
Myles

[Uwe Hermann]

On Mon, Oct 19, 2009 at 03:18:07PM -0600, Myles Watson wrote:
> Define HAVE_INIT_TIMER to only exclude the three boards that define it to be
> 0 in newconfig.

What exactly does HAVE_INIT_TIMER configure? Why do those three boards
_not_ set it? It sounds like all boards would want to set it?
> MOVNTI is a performance enhancement, and should default to 0 so it doesn't
> break boards that forget to define it.

Are all Kconfig files setting to y (via "select" preferrably) where needed?
> Index: svn/src/Kconfig
>
> --- svn.orig/src/Kconfig
> +++ svn/src/Kconfig
> @@ -74,10 +74,6 @@ config CPU_ADDR_BITS
>    int
>    default 36
>
> -config AGP_APERTURE_SIZE
> hex
> default 0x0

Why is this removed?
> +config HAVE_LOW_TABLES
> bool
> default y
> help
>  This Option is unused in the code.  Since two boards try to set it to
>  'n', they may be broken.  We either need to make the option useful or
>  get rid of it.  The broken boards are:
>  asus/m2v-mx_se
>  supermicro/h8dme

Hm, indeed. Why was this added? Are there situations where we might not
want LOW_TABLES on?
> +config HAVE_HIGH_TABLES
> bool
> default n
> help
>  This variable specifies whether a given northbridge has high table
>  support.
>  It is set in northbridge/*/Kconfig.
>  Whether or not the high tables are actually written by coreboot is
>  configurable by the user via WRITE_HIGH_TABLES.
>
> config HAVE_ACPI_TABLES
>    bool
>    help
> @@ -262,14 +280,30 @@ config HAVE_PIRQ_TABLE
>      Whether or not the PIRQ table is actually generated by coreboot
>      is configurable by the user via GENERATE_PIRQ_TABLE.
>
> -config HAVE_HIGH_TABLES
> +#These Options are here to avoid "undefined" warnings.
> +#The actual selection and help texts are in the following menu.
> Index: svn/src/northbridge/amd/amdfam10/Kconfig
>
> --- svn.orig/src/northbridge/amd/amdfam10/Kconfig
> +++ svn/src/northbridge/amd/amdfam10/Kconfig
> @@ -21,11 +21,35 @@ config NORTHBRIDGE_AMD_AMDFAM10
>    bool
>    select HAVE_HIGH_TABLES
>    select HYPERTRANSPORT_PLUGIN_SUPPORT
> select HT3_SUPPORT

[...]
> +config HT3_SUPPORT
> bool
> default y
> depends on NORTHBRIDGE_AMD_AMDFAM10

Why this? "select HT3_SUPPORT" should work just fine, no?
Even if that's not the case I think it would be nicer to set it in some
global Kconfig file so we can "select" it. IMHO _all_ y variables should
be "select"ed so we avoid the 4-line chunks...
> +config AMDMCT
> bool
> default y
> depends on NORTHBRIDGE_AMD_AMDFAM10

Ditto.
> +config MEM_TRAIN_SEQ
> +config HW_MEM_HOLE_SIZEK
> +config HW_MEM_HOLE_SIZE_AUTO_INC
> Index: svn/src/northbridge/amd/amdk8/Kconfig
> ===================================================================
> --- svn.orig/src/northbridge/amd/amdk8/Kconfig
> +++ svn/src/northbridge/amd/amdk8/Kconfig
> +config MEM_TRAIN_SEQ
> +config HW_MEM_HOLE_SIZEK
> +config HW_MEM_HOLE_SIZE_AUTO_INC

Why are these three defined twice?
> Index: svn/src/southbridge/via/vt8237r/vt8237r.h
>
> --- svn.orig/src/southbridge/via/vt8237r/vt8237r.h
> +++ svn/src/southbridge/via/vt8237r/vt8237r.h
> @@ -65,7 +65,7 @@
>  #define I2C_TRANS_CMD			0x40
>  #define CLOCK_SLAVE_ADDRESS		0x69
>
> -#if DEBUG_SMBUS == 1
> +#if defined(DEBUG_SMBUS) && DEBUG_SMBUS == 1
>  #define PRINT_DEBUG(x)		print_debug(x)
>  #define PRINT_DEBUG_HEX16(x)	print_debug_hex16(x)
>  #else

Please drop this hunk, I have a patch in the queue to redo DEBUG_SMBUS
and other DEBUG options globally anyway.
> Index: svn/src/cpu/amd/model_10xxx/init_cpus.c
>
> --- svn.orig/src/cpu/amd/model_10xxx/init_cpus.c
> +++ svn/src/cpu/amd/model_10xxx/init_cpus.c
> @@ -473,8 +473,8 @@ static void start_node(u8 node)
>    /* Enable routing table */
>    printk_debug("Start node %02x", node);
>
> -#if CONFIG_CAR_FAM10 == 1
> /* For CONFIG_CAR_FAM10 support, we need to set Dram base/limit for the new node */
>
> +#if CONFIG_NORTHBRIDGE_AMD_AMDFAM10 == 1

#if CONFIG_NORTHBRIDGE_AMD_AMDFAM10
should suffice, I think.

Uwe.

[Myles Watson]

On Mon, Oct 19, 2009 at 4:17 PM, Uwe Hermann  wrote:
> On Mon, Oct 19, 2009 at 03:18:07PM -0600, Myles Watson wrote:
> > Define HAVE_INIT_TIMER to only exclude the three boards that define it to
>
> be
> > 0 in newconfig.
>
> What exactly does HAVE_INIT_TIMER configure? Why do those three boards
> _not_ set it? It sounds like all boards would want to set it?

I don't know.  Since one of the boards is qemu, I figured that it might not
have some functionality that hardware has.
> > MOVNTI is a performance enhancement, and should default to 0 so it
>
> doesn't
> > break boards that forget to define it.
>
> Are all Kconfig files setting to y (via "select" preferrably) where needed?

It's hard to check since this option is set in sockets.  That is a
worthwhile follow up patch.  It just doesn't make sense to break booting for
performance.
> Index: svn/src/Kconfig
> > ===================================================================
> > --- svn.orig/src/Kconfig
> > +++ svn/src/Kconfig
> > @@ -74,10 +74,6 @@ config CPU_ADDR_BITS
> >       int
> >       default 36
> >
> > -config AGP_APERTURE_SIZE
> > hex
> > default 0x0
>
> Why is this removed?

It shouldn't be a global option.  It's only used by amd northbridges.
Setting global defaults for specific options makes it harder to see when
something is broken.
> > +config HAVE_LOW_TABLES
> > bool
> > default y
> > help
> >   This Option is unused in the code.  Since two boards try to set
>
> it to
> >   'n', they may be broken.  We either need to make the option
>
> useful or
> >   get rid of it.  The broken boards are:
> >   asus/m2v-mx_se
> >   supermicro/h8dme
>
> Hm, indeed. Why was this added? Are there situations where we might not
> want LOW_TABLES on?

Good question.  I think there might be times where you don't want to corrupt
low memory.
> > Index: svn/src/northbridge/amd/amdfam10/Kconfig
> >
> > --- svn.orig/src/northbridge/amd/amdfam10/Kconfig
> > +++ svn/src/northbridge/amd/amdfam10/Kconfig
> > @@ -21,11 +21,35 @@ config NORTHBRIDGE_AMD_AMDFAM10
> >       bool
> >       select HAVE_HIGH_TABLES
> >       select HYPERTRANSPORT_PLUGIN_SUPPORT
> > select HT3_SUPPORT
>
> [...]
> > +config HT3_SUPPORT
> > bool
> > default y
> > depends on NORTHBRIDGE_AMD_AMDFAM10
>
> Why this? "select HT3_SUPPORT" should work just fine, no?
> Even if that's not the case I think it would be nicer to set it in some
> global Kconfig file so we can "select" it. IMHO _all_ y variables should
> be "select"ed so we avoid the 4-line chunks...

It doesn't work unless HT3_SUPPORT is defined somewhere else.  It doesn't
make sense to define it anywhere else to me, since only FAM10 uses it.
> > +config AMDMCT
> > bool
> > default y
> > depends on NORTHBRIDGE_AMD_AMDFAM10
>
> Ditto.

Yep.
> > +config MEM_TRAIN_SEQ
> > +config HW_MEM_HOLE_SIZEK
> > +config HW_MEM_HOLE_SIZE_AUTO_INC
> > Index: svn/src/northbridge/amd/amdk8/Kconfig
> > ===================================================================
> > --- svn.orig/src/northbridge/amd/amdk8/Kconfig
> > +++ svn/src/northbridge/amd/amdk8/Kconfig
> > +config MEM_TRAIN_SEQ
> > +config HW_MEM_HOLE_SIZEK
> > +config HW_MEM_HOLE_SIZE_AUTO_INC
>
> Why are these three defined twice?

So that they only show up when they're used.
> > Index: svn/src/southbridge/via/vt8237r/vt8237r.h
> >
> > --- svn.orig/src/southbridge/via/vt8237r/vt8237r.h
> > +++ svn/src/southbridge/via/vt8237r/vt8237r.h
> > @@ -65,7 +65,7 @@
> >  #define I2C_TRANS_CMD                        0x40
> >  #define CLOCK_SLAVE_ADDRESS          0x69
> >
> > -#if DEBUG_SMBUS == 1
> > +#if defined(DEBUG_SMBUS) && DEBUG_SMBUS == 1
> >  #define PRINT_DEBUG(x)               print_debug(x)
> >  #define PRINT_DEBUG_HEX16(x) print_debug_hex16(x)
> >  #else
>
> Please drop this hunk, I have a patch in the queue to redo DEBUG_SMBUS
> and other DEBUG options globally anyway.

Sure.  I was just trying to get most of the not defined warnings.  Dropped.
> > Index: svn/src/cpu/amd/model_10xxx/init_cpus.c
> >
> > --- svn.orig/src/cpu/amd/model_10xxx/init_cpus.c
> > +++ svn/src/cpu/amd/model_10xxx/init_cpus.c
> > @@ -473,8 +473,8 @@ static void start_node(u8 node)
> >       /* Enable routing table */
> >       printk_debug("Start node %02x", node);
> >
> > -#if CONFIG_CAR_FAM10 == 1
> > /* For CONFIG_CAR_FAM10 support, we need to set Dram base/limit for
>
> the new node */
> > +#if CONFIG_NORTHBRIDGE_AMD_AMDFAM10 == 1
>
> #if CONFIG_NORTHBRIDGE_AMD_AMDFAM10
> should suffice, I think.

Yes.  Fixed.

Thanks,
Myles

[Myles Watson]

Ping.  I think I've addressed all of Peter and Uwe's concerns.

Thanks,
Myles

[Uwe Hermann]

On Fri, Oct 23, 2009 at 02:43:27PM -0600, Myles Watson wrote:
> Ping.  I think I've addressed all of Peter and Uwe's concerns.

I Think so, but please repost the updated patch so we can have a look
at the final version.

Thanks, Uwe.

[Myles Watson]

On Fri, Oct 23, 2009 at 4:56 PM, Uwe Hermann  wrote:
> On Fri, Oct 23, 2009 at 02:43:27PM -0600, Myles Watson wrote:
> > Ping.  I think I've addressed all of Peter and Uwe's concerns.
>
> I Think so, but please repost the updated patch so we can have a look
> at the final version.

I don't think the first patches have changed much, but there are two new
ones that apply on top: peter.diff and uwe.diff.

Signed-off-by: Myles Watson

Thanks,
Myles

[Uwe Hermann]

On Fri, Oct 23, 2009 at 05:06:35PM -0600, Myles Watson wrote:
> On Fri, Oct 23, 2009 at 4:56 PM, Uwe Hermann  wrote:
> > On Fri, Oct 23, 2009 at 02:43:27PM -0600, Myles Watson wrote:
> > > Ping.  I think I've addressed all of Peter and Uwe's concerns.
> >
> > I Think so, but please repost the updated patch so we can have a look
> > at the final version.
>
> I don't think the first patches have changed much, but there are two new
> ones that apply on top: peter.diff and uwe.diff.
>
> Signed-off-by: Myles Watson

Acked-by: Uwe Hermann
> +config HAVE_LOW_TABLES
> bool
> default y
> help
>  This Option is unused in the code.  Since two boards try to set it to
>  'n', they may be broken.  We either need to make the option useful or
>  get rid of it.  The broken boards are:
>  asus/m2v-mx_se
>  supermicro/h8dme

Please add a "# TODO" comment on top of that for easy grepping.
> +config MEM_TRAIN_SEQ
> int
> default 2

We should add a MEM_TRAIN_SEQ comment or help text, it's unclear to me what it is
supposed to do. Is it a per-chipset, per-cpu, or per-board option? What
do the values of the variable mean?

Regardless of that, it seems like it should be a user-visible option in
menuconfig with useful option names for the values of 0, 1, and 2 (which
seem to be the only valid ones).

That's for another patch though.

Uwe.

[Stefan Reinauer]

Uwe Hermann wrote:
> We should add a MEM_TRAIN_SEQ comment or help text, it's unclear to me what it is
> supposed to do. Is it a per-chipset, per-cpu, or per-board option? What
> do the values of the variable mean?
>
> Regardless of that, it seems like it should be a user-visible option in
> menuconfig

Shouldn't we clarify what it does _before_ deciding it should be user
visible? ;-)

Just an idea....

Going through the AMD ram init code it loops like the code is used to
trigger certain code paths during ram init.

It does look somewhat random to me, but I guess it's due to board
requirements to choose one or the other option. In which case adding it
as a user visible option just creates another way to produce a
non-booting image without any real gain..

The Tyan s2912 uses 1, the s2912_fam10 uses 2.

Maybe we can even drop the option completely?

Going through more code I see a lot of this:

#if CONFIG_MEM_TRAIN_SEQ == 0
        #define DQS_DELAY_COPY NODE_NUMS
#else
//      #define DQS_DELAY_COPY 1
        #define DQS_DELAY_COPY NODE_NUMS
#endif

which is about as good as

#define DQS_DELAY_COPY NODE_NUMS

Anyone knows more details what the option is good for?

Stefan

[Myles Watson]

> > We should add a MEM_TRAIN_SEQ comment or help text, it's unclear to me
>
> what it is
> > supposed to do. Is it a per-chipset, per-cpu, or per-board option? What
> > do the values of the variable mean?
> >
> > Regardless of that, it seems like it should be a user-visible option in
> > menuconfig
>
> Shouldn't we clarify what it does _before_ deciding it should be user
> visible? ;-)

Agreed.
> Maybe we can even drop the option completely?
> Anyone knows more details what the option is good for?

Unfortunately, I don't.  I was just trying to match newconfig.

Thanks,
Myles

[ron minnich]

I don't think any of these options should be user configurable ...
it's all black magic and setting it wrong is going to ruin  your day.

ron

[Myles Watson]

On Sat, Oct 24, 2009 at 5:11 PM, Uwe Hermann  wrote:
> On Fri, Oct 23, 2009 at 05:06:35PM -0600, Myles Watson wrote:
> > On Fri, Oct 23, 2009 at 4:56 PM, Uwe Hermann  wrote:
> > > On Fri, Oct 23, 2009 at 02:43:27PM -0600, Myles Watson wrote:
> > > > Ping.  I think I've addressed all of Peter and Uwe's concerns.
> > >
> > > I Think so, but please repost the updated patch so we can have a look
> > > at the final version.
> >
> > I don't think the first patches have changed much, but there are two new
> > ones that apply on top: peter.diff and uwe.diff.
> >
> > Signed-off-by: Myles Watson
>
> Acked-by: Uwe Hermann

Rev 4856.
> > +config HAVE_LOW_TABLES
> > bool
> > default y
> > help
> >   This Option is unused in the code.  Since two boards try to set
>
> it to
> >   'n', they may be broken.  We either need to make the option
>
> useful or
> >   get rid of it.  The broken boards are:
> >   asus/m2v-mx_se
> >   supermicro/h8dme
>
> Please add a "# TODO" comment on top of that for easy grepping.

Done.
> > +config MEM_TRAIN_SEQ
> > int
> > default 2
>
> We should add a MEM_TRAIN_SEQ comment or help text, it's unclear to me what
> it is
> supposed to do. Is it a per-chipset, per-cpu, or per-board option? What
> do the values of the variable mean?

All good questions.  It's unclear to me.
> Regardless of that, it seems like it should be a user-visible option in
> menuconfig with useful option names for the values of 0, 1, and 2 (which
> seem to be the only valid ones).

The more things we make "user configurable", the more ways a user can break
the build/ brick his board.

Thanks,
Myles