Patchwork Make hidden configuration flags visible (i945)

login
register
about
Submitter Patrick Georgi
Date 2010-09-30 22:19:56
Message ID <4CA50D0C.8050208@georgi-clan.de>
Download mbox | patch
Permalink /patch/2011/
State Accepted
Headers show

Comments

Patrick Georgi - 2010-09-30 22:19:56
Hi,

attached patch moves several config flags that, for historical reasons,
were put in romstage.c into Kconfig.

This ensures that all parts of the coreboot build on affected boards
have a chance to use these flags, which will be useful when removing
.c-includes in romstage.c

It's also a first step towards removing yet another config system
(manual #defines) from the tree.

Maybe these flags could be done away with somehow, maybe they should be
declared in other parts of the tree (eg. chipset). This is a first safe
step, so I didn't think too much about moving them elsewhere.

Signed-off-by: Patrick Georgi <patrick.georgi@coresystems.de>
Myles Watson - 2010-09-30 22:24:40
On Thu, Sep 30, 2010 at 4:19 PM, Patrick Georgi <patrick@georgi-clan.de> wrote:
> Hi,
>
> attached patch moves several config flags that, for historical reasons,
> were put in romstage.c into Kconfig.

+choice
+	prompt "Chipset variant"
+	default I945GM
+	depends on NORTHBRIDGE_INTEL_I945
+	help
+	  Different i945 variants require slightly different setup.
+
+config I945GM
+	bool "i945GM (Mobile) chipset"
+
+config I945GC
+	bool "i945GC chipset"
+
+endchoice

I don't think this should be exposed to the user.  A board Kconfig
should select it, but there shouldn't be a menu prompt for it.

Thanks,
Myles
Patrick Georgi - 2010-09-30 22:31:21
Am 01.10.2010 00:24, schrieb Myles Watson:
> +choice
> +	prompt "Chipset variant"
> +	default I945GM
> +	depends on NORTHBRIDGE_INTEL_I945
> +	help
> +	  Different i945 variants require slightly different setup.
> +
> +config I945GM
> +	bool "i945GM (Mobile) chipset"
> +
> +config I945GC
> +	bool "i945GC chipset"
> +
> +endchoice
> 
> I don't think this should be exposed to the user.  A board Kconfig
> should select it, but there shouldn't be a menu prompt for it.
How to make it invisible, simply by removing the "prompt"? My primary
interest was in using choice to make sure Kconfig allows only either of
the values.


Patrick
Myles Watson - 2010-09-30 22:35:06
On Thu, Sep 30, 2010 at 4:31 PM, Patrick Georgi <patrick@georgi-clan.de> wrote:
> Am 01.10.2010 00:24, schrieb Myles Watson:
>> +choice
>> +     prompt "Chipset variant"
>> +     default I945GM
>> +     depends on NORTHBRIDGE_INTEL_I945
>> +     help
>> +       Different i945 variants require slightly different setup.
>> +
>> +config I945GM
>> +     bool "i945GM (Mobile) chipset"
>> +
>> +config I945GC
>> +     bool "i945GC chipset"
>> +
>> +endchoice
>>
>> I don't think this should be exposed to the user.  A board Kconfig
>> should select it, but there shouldn't be a menu prompt for it.
> How to make it invisible, simply by removing the "prompt"? My primary
> interest was in using choice to make sure Kconfig allows only either of
> the values.

If that works, it's fine with me.  Otherwise, if there's only these
two variants, it could be collapsed to a single Kconfig option.  I
just think it's good to avoid options that are never correct in
user-visible Kconfig.

Thanks,
Myles
Peter Stuge - 2010-09-30 23:33:54
Patrick Georgi wrote:
> attached patch moves several config flags that, for historical
> reasons, were put in romstage.c into Kconfig.

In principle I think this is a great improvement!

But some comments..


> +++ src/northbridge/intel/i945/Kconfig	(Arbeitskopie)
> @@ -26,3 +26,43 @@
>  	default "8086,27a2"
>  	depends on NORTHBRIDGE_INTEL_I945
>  
> +choice
> +	prompt "Chipset variant"
> +	default I945GM
> +	depends on NORTHBRIDGE_INTEL_I945
> +	help
> +	  Different i945 variants require slightly different setup.
> +
> +config I945GM
> +	bool "i945GM (Mobile) chipset"
> +
> +config I945GC
> +	bool "i945GC chipset"
> +
> +endchoice

I think GC should come first and maybe even be the default, because
it's the base chipset and C < M.

That said, would it work to simply make these options be
NORTHBRIDGE_INTEL_I945GC and _I945GM, and have both of them then
select in the common (current) I945 code? That way there only needs
to be one select for this in mainboards, it's hidden from users, and
it can't really become incorrect.


> +config OVERRIDE_CLOCK_DISABLE
> +	bool
> +	default n
> +	depends on NORTHBRIDGE_INTEL_I945
> +	help
> +	  Usually system firmware turns off system memory clock signals to
> +	  unused SO-DIMM slots to reduce EMI and power consumption.
> +	  However, the Kontron 986LCD-M does not like unused clock signals to
> +	  be disabled. If other similar mainboard occur, it would make sense
> +	  to make this an entry in the sysinfo structure, and pre-initialize that
> +	  structure in the mainboard's romstage.c main() function. For now a
> +	  #define will do.

Well, it is still a #define, but maybe fix up this comment a little
now that it is being touched anyway.. Also, second last line is a bit
long.


> +config MAXIMUM_SUPPORTED_FREQUENCY
> +	int
> +	default 0
> +	depends on NORTHBRIDGE_INTEL_I945
> +	help
> +	  If non-zero, this designates the maximum DDR frequency the board supports,
> +	  despite what the chipset should be capable of.

Maybe shorten this long line a little as well.


If it works to create different NORTHBRIDGE_ configs, this is

Acked-by: Peter Stuge <peter@stuge.se>
Patrick Georgi - 2010-10-01 07:00:49
Am 01.10.2010 01:33, schrieb Peter Stuge:
> That said, would it work to simply make these options be
> NORTHBRIDGE_INTEL_I945GC and _I945GM, and have both of them then
> select in the common (current) I945 code? That way there only needs
> to be one select for this in mainboards, it's hidden from users, and
> it can't really become incorrect.
Thought about it - why I didn't do that: It significantly changes the
design of that part of the code.
The change above simply moves stuff around.


Patrick
Patrick Georgi - 2010-10-01 08:04:22
Am 01.10.2010 01:33, schrieb Peter Stuge:
> Acked-by: Peter Stuge <peter@stuge.se>
Thanks, r5891

The help texts are cleaned up, and the chipset variant setting isn't
user visible anymore (I think - tested in "make config" only)


Patrick

Patch

Index: src/mainboard/getac/p470/Kconfig

===================================================================
--- src/mainboard/getac/p470/Kconfig	(Revision 5887)

+++ src/mainboard/getac/p470/Kconfig	(Arbeitskopie)

@@ -43,6 +43,8 @@ 

 	select CACHE_AS_RAM
 	select GFXUMA
 	select TINY_BOOTBLOCK
+	select I945GM
+	select CHANNEL_XOR_RANDOMIZATION
 
 config MAINBOARD_DIR
 	string
Index: src/mainboard/getac/p470/romstage.c

===================================================================
--- src/mainboard/getac/p470/romstage.c	(Revision 5887)

+++ src/mainboard/getac/p470/romstage.c	(Arbeitskopie)

@@ -19,10 +19,6 @@ 

  * MA 02110-1301 USA
  */
 
-/* Configuration of the i945 driver */
-#define CHIPSET_I945GM 1
-#define CHANNEL_XOR_RANDOMIZATION 1
-
 #include <stdint.h>
 #include <string.h>
 #include <arch/io.h>
Index: src/mainboard/kontron/986lcd-m/Kconfig

===================================================================
--- src/mainboard/kontron/986lcd-m/Kconfig	(Revision 5887)

+++ src/mainboard/kontron/986lcd-m/Kconfig	(Arbeitskopie)

@@ -20,6 +20,9 @@ 

 	select CACHE_AS_RAM
 	select GFXUMA
 	select TINY_BOOTBLOCK
+	select CHANNEL_XOR_RANDOMIZATION
+	select I945GM
+	select OVERRIDE_CLOCK_DISABLE
 
 config MAINBOARD_DIR
 	string
Index: src/mainboard/kontron/986lcd-m/romstage.c

===================================================================
--- src/mainboard/kontron/986lcd-m/romstage.c	(Revision 5887)

+++ src/mainboard/kontron/986lcd-m/romstage.c	(Arbeitskopie)

@@ -19,19 +19,6 @@ 

 
 // __PRE_RAM__ means: use "unsigned" for device, not a struct.
 
-/* Configuration of the i945 driver */
-#define CHIPSET_I945GM 1
-/* Usually system firmware turns off system memory clock signals to
- * unused SO-DIMM slots to reduce EMI and power consumption.
- * However, the Kontron 986LCD-M does not like unused clock signals to
- * be disabled. If other similar mainboard occur, it would make sense
- * to make this an entry in the sysinfo structure, and pre-initialize that
- * structure in the mainboard's romstage.c main() function. For now a
- * #define will do.
- */
-#define OVERRIDE_CLOCK_DISABLE 1
-#define CHANNEL_XOR_RANDOMIZATION 1
-
 #include <stdint.h>
 #include <string.h>
 #include <arch/io.h>
Index: src/mainboard/ibase/mb899/Kconfig

===================================================================
--- src/mainboard/ibase/mb899/Kconfig	(Revision 5887)

+++ src/mainboard/ibase/mb899/Kconfig	(Arbeitskopie)

@@ -20,6 +20,8 @@ 

 	select CACHE_AS_RAM
 	select GFXUMA
 	select TINY_BOOTBLOCK
+	select I945GM
+	select CHANNEL_XOR_RANDOMIZATION
 
 config MAINBOARD_DIR
 	string
Index: src/mainboard/ibase/mb899/romstage.c

===================================================================
--- src/mainboard/ibase/mb899/romstage.c	(Revision 5887)

+++ src/mainboard/ibase/mb899/romstage.c	(Arbeitskopie)

@@ -19,11 +19,6 @@ 

 
 // __PRE_RAM__ means: use "unsigned" for device, not a struct.
 
-/* Configuration of the i945 driver */
-#define CHIPSET_I945GM 1
-//#define OVERRIDE_CLOCK_DISABLE 1
-#define CHANNEL_XOR_RANDOMIZATION 1
-
 #include <stdint.h>
 #include <string.h>
 #include <arch/io.h>
Index: src/mainboard/roda/rk886ex/Kconfig

===================================================================
--- src/mainboard/roda/rk886ex/Kconfig	(Revision 5887)

+++ src/mainboard/roda/rk886ex/Kconfig	(Arbeitskopie)

@@ -19,6 +19,8 @@ 

 	select HAVE_ACPI_TABLES
 	select HAVE_ACPI_RESUME
 	select BOARD_ROMSIZE_KB_1024
+	select I945GM
+	select CHANNEL_XOR_RANDOMIZATION
 
 config MAINBOARD_DIR
 	string
@@ -56,4 +58,8 @@ 

 	hex
 	default 0x6886
 
+config MAXIMUM_SUPPORTED_FREQUENCY
+	int
+	default 400
+
 endif # BOARD_RODA_RK886EX
Index: src/mainboard/roda/rk886ex/romstage.c

===================================================================
--- src/mainboard/roda/rk886ex/romstage.c	(Revision 5887)

+++ src/mainboard/roda/rk886ex/romstage.c	(Arbeitskopie)

@@ -21,12 +21,6 @@ 

 
 // __PRE_RAM__ means: use "unsigned" for device, not a struct.
 
-/* Configuration of the i945 driver */
-#define CHIPSET_I945GM 1
-#define CHANNEL_XOR_RANDOMIZATION 1
-// Rocky freezing temperature settings:
-#define MAXIMUM_SUPPORTED_FREQUENCY 400
-
 #include <stdint.h>
 #include <string.h>
 #include <arch/io.h>
Index: src/mainboard/intel/d945gclf/Kconfig

===================================================================
--- src/mainboard/intel/d945gclf/Kconfig	(Revision 5887)

+++ src/mainboard/intel/d945gclf/Kconfig	(Arbeitskopie)

@@ -40,6 +40,8 @@ 

 	select BOARD_ROMSIZE_KB_512
 	select GFXUMA
 	select TINY_BOOTBLOCK
+	select I945GC
+	select CHANNEL_XOR_RANDOMIZATION
 
 config MAINBOARD_DIR
 	string
Index: src/mainboard/intel/d945gclf/romstage.c

===================================================================
--- src/mainboard/intel/d945gclf/romstage.c	(Revision 5887)

+++ src/mainboard/intel/d945gclf/romstage.c	(Arbeitskopie)

@@ -19,10 +19,6 @@ 

 
 // __PRE_RAM__ means: use "unsigned" for device, not a struct.
 
-/* Configuration of the i945 driver */
-#define CHIPSET_I945GC 1
-#define CHANNEL_XOR_RANDOMIZATION 1
-
 #include <stdint.h>
 #include <string.h>
 #include <arch/io.h>
Index: src/northbridge/intel/i945/Kconfig

===================================================================
--- src/northbridge/intel/i945/Kconfig	(Revision 5887)

+++ src/northbridge/intel/i945/Kconfig	(Arbeitskopie)

@@ -26,3 +26,43 @@ 

 	default "8086,27a2"
 	depends on NORTHBRIDGE_INTEL_I945
 
+choice
+	prompt "Chipset variant"
+	default I945GM
+	depends on NORTHBRIDGE_INTEL_I945
+	help
+	  Different i945 variants require slightly different setup.
+
+config I945GM
+	bool "i945GM (Mobile) chipset"
+
+config I945GC
+	bool "i945GC chipset"
+
+endchoice
+
+config CHANNEL_XOR_RANDOMIZATION
+	bool
+	default n
+	depends on NORTHBRIDGE_INTEL_I945
+
+config OVERRIDE_CLOCK_DISABLE
+	bool
+	default n
+	depends on NORTHBRIDGE_INTEL_I945
+	help
+	  Usually system firmware turns off system memory clock signals to
+	  unused SO-DIMM slots to reduce EMI and power consumption.
+	  However, the Kontron 986LCD-M does not like unused clock signals to
+	  be disabled. If other similar mainboard occur, it would make sense
+	  to make this an entry in the sysinfo structure, and pre-initialize that
+	  structure in the mainboard's romstage.c main() function. For now a
+	  #define will do.
+
+config MAXIMUM_SUPPORTED_FREQUENCY
+	int
+	default 0
+	depends on NORTHBRIDGE_INTEL_I945
+	help
+	  If non-zero, this designates the maximum DDR frequency the board supports,
+	  despite what the chipset should be capable of.
Index: src/northbridge/intel/i945/raminit.c

===================================================================
--- src/northbridge/intel/i945/raminit.c	(Revision 5887)

+++ src/northbridge/intel/i945/raminit.c	(Arbeitskopie)

@@ -90,7 +90,7 @@ 

 static int memclk(void)
 {
 	int offset = 0;
-#ifdef CHIPSET_I945GM
+#if CONFIG_I945GM
 	offset++;
 #endif
 	switch (((MCHBAR32(CLKCFG) >> 4) & 7) - offset) {
@@ -102,7 +102,7 @@ 

 	return -1;
 }
 
-#ifdef CHIPSET_I945GM
+#if CONFIG_I945GM
 static int fsbclk(void)
 {
 	switch (MCHBAR32(CLKCFG) & 7) {
@@ -114,7 +114,7 @@ 

 	return -1;
 }
 #endif
-#ifdef CHIPSET_I945GC
+#if CONFIG_I945GC
 static int fsbclk(void)
 {
 	switch (MCHBAR32(CLKCFG) & 7) {
@@ -131,8 +131,8 @@ 

 {
 	u32 reg32;
 
-#ifdef MAXIMUM_SUPPORTED_FREQUENCY
-	return MAXIMUM_SUPPORTED_FREQUENCY;
+#if CONFIG_MAXIMUM_SUPPORTED_FREQUENCY
+	return CONFIG_MAXIMUM_SUPPORTED_FREQUENCY;
 #endif
 
 	reg32 = pci_read_config32(PCI_DEV(0, 0x00, 0), 0xe4); /* CAPID0 + 4 */
@@ -1045,7 +1045,7 @@ 

 	return nc;
 }
 
-#ifdef CHIPSET_I945GM
+#if CONFIG_I945GM
 /* Strength multiplier tables */
 static const u8 dual_channel_strength_multiplier[] = {
 	0x44, 0x11, 0x11, 0x11, 0x44, 0x44, 0x44, 0x11,
@@ -1101,7 +1101,7 @@ 

 	0x33, 0x00, 0x11, 0x00, 0x44, 0x44, 0x33, 0x11
 };
 #endif
-#ifdef CHIPSET_I945GC
+#if CONFIG_I945GC
 static const u8 dual_channel_strength_multiplier[] = {
 	0x44, 0x22, 0x00, 0x00, 0x44, 0x44, 0x44, 0x22,
 	0x44, 0x22, 0x00, 0x00, 0x44, 0x44, 0x44, 0x22,
@@ -2155,7 +2155,7 @@ 

 	/**
 	 * We add the indices according to our clocks from CLKCFG.
 	 */
-#ifdef CHIPSET_I945GM
+#if CONFIG_I945GM
 	static const u32 data_clock_crossing[] = {
 		0x00100401, 0x00000000, /* DDR400 FSB400 */
 		0xffffffff, 0xffffffff, /*  nonexistant  */
@@ -2201,7 +2201,7 @@ 

 	};
 
 #endif
-#ifdef CHIPSET_I945GC
+#if CONFIG_I945GC
 	/* i945 G/P */
 	static const u32 data_clock_crossing[] = {
 		0xffffffff, 0xffffffff, /*  nonexistant  */
@@ -2420,7 +2420,7 @@ 

 	if (sysinfo->interleaved) {
 
 		reg32 = MCHBAR32(DCC);
-#if CHANNEL_XOR_RANDOMIZATION
+#if CONFIG_CHANNEL_XOR_RANDOMIZATION
 		reg32 &= ~(1 << 10);
 		reg32 |= (1 << 9);
 #else
@@ -2792,10 +2792,10 @@ 

 {
 	u8 clocks[2] = { 0, 0 };
 
-#ifdef CHIPSET_I945GM
+#if CONFIG_I945GM
 #define CLOCKS_WIDTH 2
 #endif
-#ifdef CHIPSET_I945GC
+#if CONFIG_I945GC
 #define CLOCKS_WIDTH 3
 #endif
 	if (sysinfo->dimm[0] != SYSINFO_DIMM_NOT_POPULATED)
@@ -2810,15 +2810,11 @@ 

 	if (sysinfo->dimm[3] != SYSINFO_DIMM_NOT_POPULATED)
 		clocks[1] |= ((1 << CLOCKS_WIDTH)-1) << CLOCKS_WIDTH;
 
-#ifdef OVERRIDE_CLOCK_DISABLE
+#if CONFIG_OVERRIDE_CLOCK_DISABLE
 	/* Usually system firmware turns off system memory clock signals
 	 * to unused SO-DIMM slots to reduce EMI and power consumption.
 	 * However, the Kontron 986LCD-M does not like unused clock
 	 * signals to be disabled.
-	 * If other similar mainboard occur, it would make sense to make
-	 * this an entry in the sysinfo structure, and pre-initialize that
-	 * structure in the mainboard's romstage.c main() function.
-	 * For now an #ifdef will do.
 	 */
 
 	clocks[0] = 0xf; /* force all clock gate pairs to enable */