Re: [coreboot] [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