Re: [coreboot] [v2] r4701 - trunk/coreboot-v2/src/mainboard/dell/s1850

[Patrick Georgi]

Am Donnerstag, den 01.10.2009, 17:59 +0200 schrieb [e-mail]:
> Author: rminnich
> Date: 2009-10-01 17:59:49 +0200 (Thu, 01 Oct 2009)
> New Revision: 4701
>
> Modified:
>    trunk/coreboot-v2/src/mainboard/dell/s1850/Kconfig
>    trunk/coreboot-v2/src/mainboard/dell/s1850/Makefile.inc
> Log:
> OK, this builds and even looks right. dell needs its own Makefile.inc because 
> it is a P4 and it needs SSE for romcc not to go into infinite loop.

Just setting ROMCCFLAGS should be enough, right?

ROMCCFLAGS := -mcpu=p4
include $(src)/mainboard/Makefile.romccboard.inc
> -include $(src)/mainboard/Makefile.romccboard.inc
> [..]
>
> +ROMCCFLAGS ?= -mcpu=p4

[v2] r4701 - trunk/coreboot-v2/src/mainboard/dell/s1850

[Myles Watson]

On Thu, Oct 1, 2009 at 11:24 AM, Patrick Georgi  wrote:
> > Modified:
> >    trunk/coreboot-v2/src/mainboard/dell/s1850/Kconfig
> >    trunk/coreboot-v2/src/mainboard/dell/s1850/Makefile.inc
> > Log:
> > OK, this builds and even looks right. dell needs its own Makefile.inc because
> > it is a P4 and it needs SSE for romcc not to go into infinite loop.
>
> Just setting ROMCCFLAGS should be enough, right?

Almost.  See patch.

Signed-off-by: Myles Watson

Thanks,
Myles

[v2] r4701 - trunk/coreboot-v2/src/mainboard/dell/s1850

[ron minnich]

But I need to inlude the sse regiisters.

Here is a possibility

crt0-$(CONFIG_CPU_INTEL_P4) += ../../../../src/cpu/x86/sse/enable_sse.inc

that ok? I can put that in.

ron

[v2] r4701 - trunk/coreboot-v2/src/mainboard/dell/s1850

[Uwe Hermann]

On Thu, Oct 01, 2009 at 11:15:32AM -0700, ron minnich wrote:
> But I need to inlude the sse regiisters.
>
> Here is a possibility
>
> crt0-$(CONFIG_CPU_INTEL_P4) += ../../../../src/cpu/x86/sse/enable_sse.inc
>
> that ok? I can put that in.

Here's what I did for GX1 support (patch not yet finished and not yet
submitted):

+ifeq ($(CONFIG_CPU_AMD_GX1),y)
+crt0-y += ../../../../src/cpu/amd/model_gx1/cpu_setup.inc
+crt0-y += ../../../../src/cpu/amd/model_gx1/gx_setup.inc
+crt0-y += auto.inc
+else
 crt0-y += ../../../../src/cpu/x86/mmx/enable_mmx.inc
 crt0-y += auto.inc
 crt0-y += ../../../../src/cpu/x86/mmx/disable_mmx.inc
+endif

Something similar can be done for SSE, probably with a kconfig
variable CONFIG_HAVE_SSE which is set (or not) per board or more
likely per chipset.

Uwe.

[v2] r4701 - trunk/coreboot-v2/src/mainboard/dell/s1850

[ron minnich]

On Thu, Oct 1, 2009 at 11:29 AM, Uwe Hermann  wrote:
> +ifeq ($(CONFIG_CPU_AMD_GX1),y)
> +crt0-y += ../../../../src/cpu/amd/model_gx1/cpu_setup.inc
> +crt0-y += ../../../../src/cpu/amd/model_gx1/gx_setup.inc
> +crt0-y += auto.inc

let's stick with the standard way of doing this. It makes any
automated tools easier.

crt0-$(CONFIG_CPU_AMD_GX1) += ./../../../src/cpu/amd/model_gx1/cpu_setup.inc
crt0-$(CONFIG_CPU_AMD_GX1) +=../../../../src/cpu/amd/model_gx1/gx_setup.inc

Pretty much all the crt0-y in that makefile should actually have
conditional variables if we really want to make it that general.

and so on.

thanks

ron

[v2] r4701 - trunk/coreboot-v2/src/mainboard/dell/s1850

[Uwe Hermann]

On Thu, Oct 01, 2009 at 11:40:23AM -0700, ron minnich wrote:
> On Thu, Oct 1, 2009 at 11:29 AM, Uwe Hermann  wrote:
> > +ifeq ($(CONFIG_CPU_AMD_GX1),y)
> > +crt0-y += ../../../../src/cpu/amd/model_gx1/cpu_setup.inc
> > +crt0-y += ../../../../src/cpu/amd/model_gx1/gx_setup.inc
> > +crt0-y += auto.inc
>
> let's stick with the standard way of doing this. It makes any
> automated tools easier.
>
> crt0-$(CONFIG_CPU_AMD_GX1) += ./../../../src/cpu/amd/model_gx1/cpu_setup.inc
> crt0-$(CONFIG_CPU_AMD_GX1) +=../../../../src/cpu/amd/model_gx1/gx_setup.inc

In general, yes. But in the GX1 case I think it cannot be done this way
(but please correct me if I'm wrong), see full patch:
> +ifeq ($(CONFIG_CPU_AMD_GX1),y)
> +crt0-y += ../../../../src/cpu/amd/model_gx1/cpu_setup.inc
> +crt0-y += ../../../../src/cpu/amd/model_gx1/gx_setup.inc
> +crt0-y += auto.inc
> +else
>  crt0-y += ../../../../src/cpu/x86/mmx/enable_mmx.inc
>  crt0-y += auto.inc
>  crt0-y += ../../../../src/cpu/x86/mmx/disable_mmx.inc
> +endif

In the GX1 case we need to include cpu_setup.inc, then gx_setup.inc,
then auto.inc. However, enable_mmx.inc and disable_mmx.inc must _not_
be included in the GX1 case. Hence the ifeq/else construct.

Uwe.

[v2] r4701 - trunk/coreboot-v2/src/mainboard/dell/s1850

[ron minnich]

On Thu, Oct 1, 2009 at 11:59 AM, Uwe Hermann  wrote:
> On Thu, Oct 01, 2009 at 11:40:23AM -0700, ron minnich wrote:
> > On Thu, Oct 1, 2009 at 11:29 AM, Uwe Hermann  wrote:
> > > +ifeq ($(CONFIG_CPU_AMD_GX1),y)
> > > +crt0-y += ../../../../src/cpu/amd/model_gx1/cpu_setup.inc
> > > +crt0-y += ../../../../src/cpu/amd/model_gx1/gx_setup.inc
> > > +crt0-y += auto.inc
> >
> > let's stick with the standard way of doing this. It makes any
> > automated tools easier.
> >
> > crt0-$(CONFIG_CPU_AMD_GX1) += ./../../../src/cpu/amd/model_gx1/cpu_setup.inc
> > crt0-$(CONFIG_CPU_AMD_GX1) +=../../../../src/cpu/amd/model_gx1/gx_setup.inc
>
> In general, yes. But in the GX1 case I think it cannot be done this way
> (but please correct me if I'm wrong), see full patch:
> > +ifeq ($(CONFIG_CPU_AMD_GX1),y)
> > +crt0-y += ../../../../src/cpu/amd/model_gx1/cpu_setup.inc
> > +crt0-y += ../../../../src/cpu/amd/model_gx1/gx_setup.inc
> > +crt0-y += auto.inc
> > +else
> >  crt0-y += ../../../../src/cpu/x86/mmx/enable_mmx.inc
> >  crt0-y += auto.inc
> >  crt0-y += ../../../../src/cpu/x86/mmx/disable_mmx.inc
> > +endif
>
> In the GX1 case we need to include cpu_setup.inc, then gx_setup.inc,
> then auto.inc. However, enable_mmx.inc and disable_mmx.inc must _not_
> be included in the GX1 case. Hence the ifeq/else construct.

I understand your point. You are correct about the mmx stuff. So what
must happen is that the mmx stuff must ALSO be conditional. Hence my
point, that
to make this Makefile completely general, we need to make ALL of the
crt0-y entries conditional.

thanks

ron

[v2] r4701 - trunk/coreboot-v2/src/mainboard/dell/s1850

[Peter Stuge]

ron minnich wrote:
> > In the GX1 case we need to include cpu_setup.inc, then gx_setup.inc,
> > then auto.inc. However, enable_mmx.inc and disable_mmx.inc must _not_
> > be included in the GX1 case. Hence the ifeq/else construct.
>
> I understand your point. You are correct about the mmx stuff. So what
> must happen is that the mmx stuff must ALSO be conditional. Hence my
> point, that to make this Makefile completely general, we need to
> make ALL of the crt0-y entries conditional.

Agree. So CONFIG_CPU_MMX or some such should then be select:ed by
many CPUs, but not by GX1, right?

//Peter

[v2] r4701 - trunk/coreboot-v2/src/mainboard/dell/s1850

[ron minnich]

On Thu, Oct 1, 2009 at 4:01 PM, Peter Stuge  wrote:
> Agree. So CONFIG_CPU_MMX or some such should then be select:ed by
> many CPUs, but not by GX1, right?

Yes, same for SSE. That way the generic makefile should be fine.

I just called it MMX, you all want CPU_MMX? that if fine too.

thanks

ron