[PATCH] Move "select CACHE_AS_RAM" lines from boards into CPU socket

[Uwe Hermann]

See patch.

Uwe.

[Patrick Georgi]

Am 06.12.2010 23:45, schrieb Uwe Hermann:
> See patch.

src/cpu/Kconfig defines CACHE_AS_RAM to "!ROMCC", so I think these can
be dropped completely. Right?

We basically assume it's CAR unless romcc is used.

Patrick

[Stefan Reinauer]

* Patrick Georgi  [101207 16:35]:
> Am 06.12.2010 23:45, schrieb Uwe Hermann:
> > See patch.
>
> src/cpu/Kconfig defines CACHE_AS_RAM to "!ROMCC", so I think these can
> be dropped completely. Right?
>
> We basically assume it's CAR unless romcc is used.

This seems to make a lot of sense.

Do we need this stuff in Kconfig at all?

Stefan

[Stefan Reinauer]

* Stefan Reinauer  [101207 23:06]:
> Patrick Georgi  [101207 16:35]:
> > Am 06.12.2010 23:45, schrieb Uwe Hermann:
> > > See patch.
> >
> > src/cpu/Kconfig defines CACHE_AS_RAM to "!ROMCC", so I think these can
> > be dropped completely. Right?
> >
> > We basically assume it's CAR unless romcc is used.
>
> This seems to make a lot of sense.
>
> Do we need this stuff in Kconfig at all?

There are only very few places in the Makefile that need the
distinction:

./src/arch/i386/Makefile.inc:ifeq ($(CONFIG_ROMCC),y)
./src/arch/i386/Makefile.inc:ifeq ($(CONFIG_ROMCC),y)
./src/pc80/Makefile.inc:romstage-$(CONFIG_CACHE_AS_RAM) += serial.c
./src/console/Makefile.inc:romstage-$(CONFIG_CACHE_AS_RAM) += console.c
./src/lib/Makefile.inc:romstage-$(CONFIG_CACHE_AS_RAM) += ramtest.c

And some in the .c files, but I think it would be safe to replace those
#if CONFIG_CACHE_AS_RAM with #ifndef __ROMCC__

Stefan

[Uwe Hermann]

On Tue, Dec 07, 2010 at 11:11:23PM +0100, Stefan Reinauer wrote:
> Stefan Reinauer  [101207 23:06]:
> > Patrick Georgi  [101207 16:35]:
> > > Am 06.12.2010 23:45, schrieb Uwe Hermann:
> > > > See patch.
> > >
> > > src/cpu/Kconfig defines CACHE_AS_RAM to "!ROMCC", so I think these can
> > > be dropped completely. Right?
> > >
> > > We basically assume it's CAR unless romcc is used.
> >
> > This seems to make a lot of sense.
> >
> > Do we need this stuff in Kconfig at all?
>
> There are only very few places in the Makefile that need the
> distinction:
>
> ./src/arch/i386/Makefile.inc:ifeq ($(CONFIG_ROMCC),y)
> ./src/arch/i386/Makefile.inc:ifeq ($(CONFIG_ROMCC),y)
> ./src/pc80/Makefile.inc:romstage-$(CONFIG_CACHE_AS_RAM) += serial.c
> ./src/console/Makefile.inc:romstage-$(CONFIG_CACHE_AS_RAM) += console.c
> ./src/lib/Makefile.inc:romstage-$(CONFIG_CACHE_AS_RAM) += ramtest.c
>
> And some in the .c files, but I think it would be safe to replace those
> #if CONFIG_CACHE_AS_RAM with #ifndef __ROMCC__

That would be pretty confusing IMHO, as sometimes "#ifndef __ROMCC__"
is used even in CAR boards (where the bootblock is compiled with romcc
AFAIK) in some header files (to "disable" function prototypes which romcc
doesn't support). The __ROMCC__ #define would then serve two different
purposes, kind of like before we splitted away __PRE_RAM__ from __ROMCC__.

Uwe.

[Stefan Reinauer]

Hi Uwe,

good approach!

* Uwe Hermann  [101206 23:45]:
> Move "select CACHE_AS_RAM" lines from boards into CPU socket.

I generally like this idea, but that is not the only thing the patch
does. If it did, it were Acked-by: Stefan Reinauer
> All K8/Fam10h boards use CAR, so move the "select CACHE_AS_RAM"
> into model_fxx/Kconfig and model_10xxx/Kconfig, and remove it from the
> individual boards and the individual sockets using model_fxx/model_10xxx.

I think CAR should be selected in the socket, not in the model. In case
of the AMD K8 CPUs it is not a big difference, but in general, a socket
should be the entity to decide whether a given setup can run CAR or not.

The rationale is that the socket is what enforces which CPU models can
run on a board. And if there is only one model for that socket that does
not support CAR, we can't do CAR for that socket (or would need to
define a new socket for that)

Unfortunately we can not well model the above logic in model_* Kconfig
files very nicely, so it will have keep living in the socket.

In short, please drop the following part of the patch, add select
CACHE_AS_RAM to the K8/Fam10 sockets if it's missing there instead, and
it's all fine.
> Index: src/cpu/amd/socket_940/Kconfig
>
> --- src/cpu/amd/socket_940/Kconfig	(Revision 6145)
> +++ src/cpu/amd/socket_940/Kconfig	(Arbeitskopie)
> @@ -7,7 +7,6 @@
>    def_bool y
>    select K8_HT_FREQ_1G_SUPPORT
>    select CPU_AMD_MODEL_FXX
> select CACHE_AS_RAM
>
> config CPU_ADDR_BITS
>    int
> Index: src/cpu/amd/model_fxx/Kconfig
> ===================================================================
> --- src/cpu/amd/model_fxx/Kconfig	(Revision 6145)
> +++ src/cpu/amd/model_fxx/Kconfig	(Arbeitskopie)
> @@ -3,6 +3,7 @@
>    select MMX
>    select SSE
>    select SSE2
> select CACHE_AS_RAM
>
> if CPU_AMD_MODEL_FXX
>  config UDELAY_IO
> Index: src/cpu/amd/socket_754/Kconfig
> ===================================================================
> --- src/cpu/amd/socket_754/Kconfig	(Revision 6145)
> +++ src/cpu/amd/socket_754/Kconfig	(Arbeitskopie)
> @@ -6,7 +6,6 @@
>  config SOCKET_SPECIFIC_OPTIONS
>    def_bool y
>    select CPU_AMD_MODEL_FXX
> select CACHE_AS_RAM
>
> config CPU_ADDR_BITS
>    int

In addition, this is ugly because the actual code selection for CAR
still happens in the socket_*** Makefile.inc, so if there is a good
reason for moving from socket to model (except we wouldn't have to add
a line to each socket rather than just two models, i think this is no
sufficient reason to start becoming inconsistent across CPUs), the
Makefile.inc portion should be moved, too, and live where the Kconfig
portion lives.

Best case would be to remove CACHE_AS_RAM/ROMCC from Kconfig all
together, and plug it into the Makefiles, so we have less files to look
at for this one piece of logic. (By forgetting the Makefile.inc change
you proved this point ;)

Stefan

[Uwe Hermann]

On Tue, Dec 07, 2010 at 11:05:19PM +0100, Stefan Reinauer wrote:
> Hi Uwe,
>
> good approach!
> Uwe Hermann  [101206 23:45]:
> > Move "select CACHE_AS_RAM" lines from boards into CPU socket.
>
> I generally like this idea, but that is not the only thing the patch
> does. If it did, it were Acked-by: Stefan Reinauer
> > All K8/Fam10h boards use CAR, so move the "select CACHE_AS_RAM"
> > into model_fxx/Kconfig and model_10xxx/Kconfig, and remove it from the
> > individual boards and the individual sockets using model_fxx/model_10xxx.
>
> I think CAR should be selected in the socket, not in the model. In case
> of the AMD K8 CPUs it is not a big difference, but in general, a socket
> should be the entity to decide whether a given setup can run CAR or not.

Yep, I agree, comitted as r6151 resuing your ack. I moved all those
selects to the socket, and changed the model_10xxx to do the same (it
already had the select in model_10xxx, not in the sockets).
> In addition, this is ugly because the actual code selection for CAR
> still happens in the socket_*** Makefile.inc, so if there is a good
> reason for moving from socket to model (except we wouldn't have to add
> a line to each socket rather than just two models,

Nope, no other reason, I just wanted to save a few lines. I agree
consistency across all sockets/CPUs is much more important here, though.

Uwe.