[coreboot] [PATCH] More Kconfig changes

[Myles Watson]

On Mon, Oct 19, 2009 at 4:17 PM, Uwe Hermann <uwe at hermann-uwe.de> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20091019/a01356e1/attachment.html>