Hello, 1. This patch adds CAR for Intel P6 series processors. 2. Add support for Micro-FCBGA 479 Celeron and PIII's 3. Add support for model_6bx and microcode updates 4. Add support for CAR and Tinybootblock on RCA RM4100 and Thomson IP1000
Build and boot tested (bootlog attached).
Signed-off-by: Joseph Smith joe@settoplinux.org
Dear Joseph,
Am Donnerstag, den 08.04.2010, 02:09 -0400 schrieb Joseph Smith:
- This patch adds CAR for Intel P6 series processors.
- Add support for Micro-FCBGA 479 Celeron and PIII's
- Add support for model_6bx and microcode updates
- Add support for CAR and Tinybootblock on RCA RM4100 and Thomson IP1000
sorry for my dump question. Does supporting CAR have any practical improvements besides going with coreboot features(?). For example did boot time improve?
Build and boot tested (bootlog attached).
It says »Boot failed.« at the end. But that is due to the payload, is not it?
Anyway, I just spotted one indentation error. Someone knowledgeable has to do the review. Sorry!
[…]
Index: src/cpu/intel/model_6bx/cache_as_ram_disable.c
--- src/cpu/intel/model_6bx/cache_as_ram_disable.c (revision 0) +++ src/cpu/intel/model_6bx/cache_as_ram_disable.c (revision 0) @@ -0,0 +1,89 @@ +/*
- This file is part of the coreboot project.
- Copyright (C) 2007-2009 coresystems GmbH
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; version 2 of
- the License.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
- MA 02110-1301 USA
- */
+#include "cpu/x86/car/copy_and_run.c"
+/* called from assembler code */ +void stage1_main(unsigned long bist);
+/* from romstage.c */ +void real_main(unsigned long bist);
+void stage1_main(unsigned long bist) +{
unsigned int cpu_reset = 0;real_main(bist);/* No servicable parts below this line .. */+#ifdef CAR_DEBUG
/* Check value of esp to verify if we have enough rom forstack in Cache as RAM */
unsigned v_esp;__asm__ volatile ("movl %%esp, %0\n": "=a" (v_esp));printk(BIOS_SPEW, "v_esp=%08x\n", v_esp);+#endif
printk(BIOS_SPEW, "cpu_reset = %08x\n", cpu_reset);printk(BIOS_SPEW, "No cache as ram now - ");
This indentation looks different. I do not know if it is just my MUA.
[…]
Thanks for your work,
Paul
On 04/08/2010 02:36 AM, Paul Menzel wrote:
Dear Joseph,
Am Donnerstag, den 08.04.2010, 02:09 -0400 schrieb Joseph Smith:
- This patch adds CAR for Intel P6 series processors.
- Add support for Micro-FCBGA 479 Celeron and PIII's
- Add support for model_6bx and microcode updates
- Add support for CAR and Tinybootblock on RCA RM4100 and Thomson IP1000
sorry for my dump question. Does supporting CAR have any practical improvements besides going with coreboot features(?). For example did boot time improve?
Yes it seems to boot alot faster :-)
Build and boot tested (bootlog attached).
It says »Boot failed.« at the end. But that is due to the payload, is not it?
Yes that log was without a payload.
Anyway, I just spotted one indentation error. Someone knowledgeable has to do the review. Sorry!
[…]
Index: src/cpu/intel/model_6bx/cache_as_ram_disable.c
--- src/cpu/intel/model_6bx/cache_as_ram_disable.c (revision 0) +++ src/cpu/intel/model_6bx/cache_as_ram_disable.c (revision 0) @@ -0,0 +1,89 @@ +/*
- This file is part of the coreboot project.
- Copyright (C) 2007-2009 coresystems GmbH
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; version 2 of
- the License.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 51 Franklin St, Fifth Floor, Boston,
- MA 02110-1301 USA
- */
+#include "cpu/x86/car/copy_and_run.c"
+/* called from assembler code */ +void stage1_main(unsigned long bist);
+/* from romstage.c */ +void real_main(unsigned long bist);
+void stage1_main(unsigned long bist) +{
unsigned int cpu_reset = 0;real_main(bist);/* No servicable parts below this line .. */+#ifdef CAR_DEBUG
/* Check value of esp to verify if we have enough rom forstack in Cache as RAM */
unsigned v_esp;__asm__ volatile ("movl %%esp, %0\n": "=a" (v_esp));printk(BIOS_SPEW, "v_esp=%08x\n", v_esp);+#endif
printk(BIOS_SPEW, "cpu_reset = %08x\n", cpu_reset);printk(BIOS_SPEW, "No cache as ram now - ");This indentation looks different. I do not know if it is just my MUA.
ah thanks.
On 4/8/10 8:09 AM, Joseph Smith wrote:
Signed-off-by: Joseph Smith joe@settoplinux.org
Nice! Thanks, Joseph!
Index: src/cpu/intel/Kconfig
--- src/cpu/intel/Kconfig (revision 5372) +++ src/cpu/intel/Kconfig (working copy)
+source src/cpu/intel/socket_mfcbga479/Kconfig source src/cpu/intel/socket_mFCPGA478/Kconfig source src/cpu/intel/socket_mPGA478/Kconfig source src/cpu/intel/socket_mPGA479M/Kconfig
I think you should keep the naming convention here: mFCBGA479
Index: src/cpu/intel/Makefile.inc
--- src/cpu/intel/Makefile.inc (revision 5372) +++ src/cpu/intel/Makefile.inc (working copy) @@ -6,6 +6,7 @@ subdirs-$(CONFIG_CPU_INTEL_SOCKET_441) += socket_441 subdirs-$(CONFIG_CPU_INTEL_SOCKET_BGA956) += bga956 subdirs-$(CONFIG_CPU_INTEL_EP80579) += ep80579 +subdirs-$(CONFIG_CPU_INTEL_SOCKET_MFCBGA479) += socket_mfcbga479
ditto
Index: src/cpu/intel/model_6bx/Kconfig
--- src/cpu/intel/model_6bx/Kconfig (revision 0) +++ src/cpu/intel/model_6bx/Kconfig (revision 0) @@ -0,0 +1,3 @@ +config CPU_INTEL_CORE
- bool
- select SMP
Now, this is definitely wrong.
Index: src/cpu/intel/socket_mfcbga479/Kconfig
--- src/cpu/intel/socket_mfcbga479/Kconfig (revision 0) +++ src/cpu/intel/socket_mfcbga479/Kconfig (revision 0) @@ -0,0 +1,5 @@ +config CPU_INTEL_SOCKET_MFCBGA479
- bool
- select CPU_INTEL_MODEL_6BX
Where is CPU_INTEL_MODEL_6BX defined?
With above issues fixed this is Acked-by: Stefan Reinauer stepan@coresystems.de
Stefan
On Thu, 08 Apr 2010 22:51:25 +0200, Stefan Reinauer stepan@coresystems.de wrote:
On 4/8/10 8:09 AM, Joseph Smith wrote:
Signed-off-by: Joseph Smith joe@settoplinux.org
Nice! Thanks, Joseph!
Index: src/cpu/intel/Kconfig
--- src/cpu/intel/Kconfig (revision 5372) +++ src/cpu/intel/Kconfig (working copy)
+source src/cpu/intel/socket_mfcbga479/Kconfig source src/cpu/intel/socket_mFCPGA478/Kconfig source src/cpu/intel/socket_mPGA478/Kconfig source src/cpu/intel/socket_mPGA479M/Kconfig
I think you should keep the naming convention here: mFCBGA479
Ok I can do that.
Index: src/cpu/intel/Makefile.inc
--- src/cpu/intel/Makefile.inc (revision 5372) +++ src/cpu/intel/Makefile.inc (working copy) @@ -6,6 +6,7 @@ subdirs-$(CONFIG_CPU_INTEL_SOCKET_441) += socket_441 subdirs-$(CONFIG_CPU_INTEL_SOCKET_BGA956) += bga956 subdirs-$(CONFIG_CPU_INTEL_EP80579) += ep80579 +subdirs-$(CONFIG_CPU_INTEL_SOCKET_MFCBGA479) += socket_mfcbga479
ditto
Index: src/cpu/intel/model_6bx/Kconfig
--- src/cpu/intel/model_6bx/Kconfig (revision 0) +++ src/cpu/intel/model_6bx/Kconfig (revision 0) @@ -0,0 +1,3 @@ +config CPU_INTEL_CORE
- bool
- select SMP
Now, this is definitely wrong.
Oops, I will fix that.
Index: src/cpu/intel/socket_mfcbga479/Kconfig
--- src/cpu/intel/socket_mfcbga479/Kconfig (revision 0) +++ src/cpu/intel/socket_mfcbga479/Kconfig (revision 0) @@ -0,0 +1,5 @@ +config CPU_INTEL_SOCKET_MFCBGA479
- bool
- select CPU_INTEL_MODEL_6BX
Where is CPU_INTEL_MODEL_6BX defined?
Not sure what you mean. It is defined by the socket.
With above issues fixed this is Acked-by: Stefan Reinauer stepan@coresystems.de
On 4/8/10 11:32 PM, Joseph Smith wrote:
Index: src/cpu/intel/model_6bx/Kconfig
--- src/cpu/intel/model_6bx/Kconfig (revision 0) +++ src/cpu/intel/model_6bx/Kconfig (revision 0) @@ -0,0 +1,3 @@ +config CPU_INTEL_CORE
- bool
- select SMP
Now, this is definitely wrong.
Oops, I will fix that.
Index: src/cpu/intel/socket_mfcbga479/Kconfig
--- src/cpu/intel/socket_mfcbga479/Kconfig (revision 0) +++ src/cpu/intel/socket_mfcbga479/Kconfig (revision 0) @@ -0,0 +1,5 @@ +config CPU_INTEL_SOCKET_MFCBGA479
- bool
- select CPU_INTEL_MODEL_6BX
Where is CPU_INTEL_MODEL_6BX defined?
Not sure what you mean. It is defined by the socket.
No, it's not. It's used in the socket, but it's never defined anywhere.
On Thu, 08 Apr 2010 23:37:59 +0200, Stefan Reinauer stepan@coresystems.de wrote:
On 4/8/10 11:32 PM, Joseph Smith wrote:
Index: src/cpu/intel/model_6bx/Kconfig
--- src/cpu/intel/model_6bx/Kconfig (revision 0) +++ src/cpu/intel/model_6bx/Kconfig (revision 0) @@ -0,0 +1,3 @@ +config CPU_INTEL_CORE
- bool
- select SMP
Now, this is definitely wrong.
Oops, I will fix that.
Index: src/cpu/intel/socket_mfcbga479/Kconfig
--- src/cpu/intel/socket_mfcbga479/Kconfig (revision 0) +++ src/cpu/intel/socket_mfcbga479/Kconfig (revision 0) @@ -0,0 +1,5 @@ +config CPU_INTEL_SOCKET_MFCBGA479
- bool
- select CPU_INTEL_MODEL_6BX
Where is CPU_INTEL_MODEL_6BX defined?
Not sure what you mean. It is defined by the socket.
No, it's not. It's used in the socket, but it's never defined anywhere.
hmm, ok I will look into it.
Thanks for your review Stefan.
On Thu, 08 Apr 2010 23:37:59 +0200, Stefan Reinauer <stepan@coresystems.de
wrote:
On 4/8/10 11:32 PM, Joseph Smith wrote:
Index: src/cpu/intel/model_6bx/Kconfig
--- src/cpu/intel/model_6bx/Kconfig (revision 0) +++ src/cpu/intel/model_6bx/Kconfig (revision 0) @@ -0,0 +1,3 @@ +config CPU_INTEL_CORE
- bool
- select SMP
This looks like it was copied directly from cpu/intel/model_6ex/Kconfig.
You are redefining CPU_INTEL_CORE here. This is probably where you wanted to define CPU_INTEL_MODEL_6BX.
Myles
On Thu, 8 Apr 2010 15:45:50 -0600, Myles Watson mylesgw@gmail.com wrote:
On Thu, 08 Apr 2010 23:37:59 +0200, Stefan Reinauer
<stepan@coresystems.de
wrote:
On 4/8/10 11:32 PM, Joseph Smith wrote:
Index: src/cpu/intel/model_6bx/Kconfig
--- src/cpu/intel/model_6bx/Kconfig (revision 0) +++ src/cpu/intel/model_6bx/Kconfig (revision 0) @@ -0,0 +1,3 @@ +config CPU_INTEL_CORE
- bool
- select SMP
This looks like it was copied directly from cpu/intel/model_6ex/Kconfig.
You are redefining CPU_INTEL_CORE here. This is probably where you
wanted
to define CPU_INTEL_MODEL_6BX.
Ah ok thanks Myles.
So there is not actually a:
#define CPU_INTEL_MODEL_6BX blabla
preprocessing directive anywhere. It just needs to be defined in Kconfig.
-----Original Message----- From: Joseph Smith [mailto:joe@settoplinux.org] Sent: Thursday, April 08, 2010 3:59 PM To: Myles Watson Cc: Stefan Reinauer; coreboot@coreboot.org Subject: Re: [coreboot] [PATCH] CAR on Intel P6 Series - Support for model_6bx
On 4/8/10 11:32 PM, Joseph Smith wrote:
> Index: src/cpu/intel/model_6bx/Kconfig >
===================================================================
> --- src/cpu/intel/model_6bx/Kconfig (revision 0) > +++ src/cpu/intel/model_6bx/Kconfig (revision 0) > @@ -0,0 +1,3 @@ > +config CPU_INTEL_CORE > + bool > + select SMP
This looks like it was copied directly from cpu/intel/model_6ex/Kconfig.
You are redefining CPU_INTEL_CORE here. This is probably where you
wanted
to define CPU_INTEL_MODEL_6BX.
Ah ok thanks Myles.
So there is not actually a:
#define CPU_INTEL_MODEL_6BX blabla
preprocessing directive anywhere. It just needs to be defined in Kconfig.
That's right. "config FOO" defines CONFIG_FOO. "select FOO" just sets it if it exists. You can check in your .config file to make sure the symbols you expect to be defined are showing up.
Thanks, Myles
On Thu, 8 Apr 2010 16:02:09 -0600, "Myles Watson" mylesgw@gmail.com wrote:
-----Original Message----- From: Joseph Smith [mailto:joe@settoplinux.org] Sent: Thursday, April 08, 2010 3:59 PM To: Myles Watson Cc: Stefan Reinauer; coreboot@coreboot.org Subject: Re: [coreboot] [PATCH] CAR on Intel P6 Series - Support for model_6bx
On 4/8/10 11:32 PM, Joseph Smith wrote:
>> Index: src/cpu/intel/model_6bx/Kconfig >>
===================================================================
>> --- src/cpu/intel/model_6bx/Kconfig (revision 0) >> +++ src/cpu/intel/model_6bx/Kconfig (revision 0) >> @@ -0,0 +1,3 @@ >> +config CPU_INTEL_CORE >> + bool >> + select SMP
This looks like it was copied directly from
cpu/intel/model_6ex/Kconfig.
You are redefining CPU_INTEL_CORE here. This is probably where you
wanted
to define CPU_INTEL_MODEL_6BX.
Ah ok thanks Myles.
So there is not actually a:
#define CPU_INTEL_MODEL_6BX blabla
preprocessing directive anywhere. It just needs to be defined in
Kconfig.
That's right. "config FOO" defines CONFIG_FOO. "select FOO" just sets
it
if it exists. You can check in your .config file to make sure the
symbols
you expect to be defined are showing up.
Hmm, I wonder why this did not throw an error at me?
On Thu, Apr 8, 2010 at 4:12 PM, Joseph Smith joe@settoplinux.org wrote:
On Thu, 8 Apr 2010 16:02:09 -0600, "Myles Watson" mylesgw@gmail.com wrote:
-----Original Message----- From: Joseph Smith [mailto:joe@settoplinux.org] Sent: Thursday, April 08, 2010 3:59 PM To: Myles Watson Cc: Stefan Reinauer; coreboot@coreboot.org Subject: Re: [coreboot] [PATCH] CAR on Intel P6 Series - Support for model_6bx
On 4/8/10 11:32 PM, Joseph Smith wrote: >>> Index: src/cpu/intel/model_6bx/Kconfig >>>
===================================================================
>>> --- src/cpu/intel/model_6bx/Kconfig (revision 0) >>> +++ src/cpu/intel/model_6bx/Kconfig (revision 0) >>> @@ -0,0 +1,3 @@ >>> +config CPU_INTEL_CORE >>> + bool >>> + select SMP
This looks like it was copied directly from
cpu/intel/model_6ex/Kconfig.
You are redefining CPU_INTEL_CORE here. This is probably where you
wanted
to define CPU_INTEL_MODEL_6BX.
Ah ok thanks Myles.
So there is not actually a:
#define CPU_INTEL_MODEL_6BX blabla
preprocessing directive anywhere. It just needs to be defined in
Kconfig.
That's right. "config FOO" defines CONFIG_FOO. "select FOO" just sets
it
if it exists. You can check in your .config file to make sure the
symbols
you expect to be defined are showing up.
Hmm, I wonder why this did not throw an error at me?
Selecting undefined config options doesn't throw an error. I think it would be nice if it did, but there might be some reason that I don't know of why you want to be able to select undefined things and have nothing happen.
Thanks, Myles
On 4/8/10 8:09 AM, Joseph Smith wrote:
Hello,
- This patch adds CAR for Intel P6 series processors.
- Add support for Micro-FCBGA 479 Celeron and PIII's
- Add support for model_6bx and microcode updates
- Add support for CAR and Tinybootblock on RCA RM4100 and Thomson IP1000
Build and boot tested (bootlog attached).
Signed-off-by: Joseph Smith joe@settoplinux.org
Thanks, r5388.
-
Joseph Smith -
Myles Watson -
Paul Menzel -
Stefan Reinauer