Re: [coreboot] [PATCH] More Kconfig changes

[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