[PATCH] Make hidden configuration flags visible (i945)

[Patrick Georgi]

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

[PATCH] Make hidden configuration flags visible (i945)

[Myles Watson]

On Thu, Sep 30, 2010 at 4:19 PM, Patrick Georgi  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

[PATCH] Make hidden configuration flags visible (i945)

[Patrick Georgi]

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

[PATCH] Make hidden configuration flags visible (i945)

[Myles Watson]

On Thu, Sep 30, 2010 at 4:31 PM, Patrick Georgi  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

[PATCH] Make hidden configuration flags visible (i945)

[Peter Stuge]

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

[PATCH] Make hidden configuration flags visible (i945)

[Patrick Georgi]

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

[PATCH] Make hidden configuration flags visible (i945)

[Patrick Georgi]

Am 01.10.2010 01:33, schrieb Peter Stuge:
> Acked-by: Peter Stuge

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