| Submitter | Myles Watson |
|---|---|
| Date | 2009-10-19 21:18:07 |
| Message ID | <2831fecf0910191418lb8d7aedy2595e65e42904a00@mail.gmail.com> |
| Download | mbox | patch |
| Permalink | /patch/439/ |
| State | Superseded |
| Headers | show |
Comments
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
On Mon, Oct 19, 2009 at 3:39 PM, Peter Stuge <peter@stuge.se> 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
On Mon, Oct 19, 2009 at 3:51 PM, Myles Watson <mylesgw@gmail.com> wrote: > > > On Mon, Oct 19, 2009 at 3:39 PM, Peter Stuge <peter@stuge.se> 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
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.
On Mon, Oct 19, 2009 at 4:17 PM, Uwe Hermann <uwe@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
Ping. I think I've addressed all of Peter and Uwe's concerns. Thanks, Myles
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.
Patch
Index: svn/src/Kconfig =================================================================== --- svn.orig/src/Kconfig +++ svn/src/Kconfig @@ -185,7 +185,7 @@ config HAVE_MAINBOARD_RESOURCES config HAVE_MOVNTI bool - default y + default n config HAVE_OPTION_TABLE bool
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 <mylesgw@gmail.com> Thanks, Myles