Re: [coreboot] [PATCH 2/6] Intel EP80579 CPU core

[Uwe Hermann]

On Wed, Aug 20, 2008 at 09:18:18AM -0700, Ed Swierk wrote:
> This patch implements support for the CPU core of the Intel EP80579
> Integrated Processor.
>
> Signed-off-by: Ed Swierk

Acked-by: Uwe Hermann

but see minor comments below...
> Index: coreboot-v2-3363/src/cpu/intel/ep80579/Config.lb
>
> --- /dev/null
> +++ coreboot-v2-3363/src/cpu/intel/ep80579/Config.lb
> @@ -0,0 +1,11 @@

Add the usual license header please, ...
> +config chip.h
> +object ep80579.o
> +dir /cpu/x86/tsc
> +dir /cpu/x86/mtrr
> +dir /cpu/x86/fpu
> +dir /cpu/x86/mmx
> +dir /cpu/x86/sse
> +dir /cpu/x86/lapic
> +dir /cpu/x86/cache
> +dir /cpu/intel/microcode
> +driver ep80579_init.o
> Index: coreboot-v2-3363/src/cpu/intel/ep80579/chip.h
> ===================================================================
> --- /dev/null
> +++ coreboot-v2-3363/src/cpu/intel/ep80579/chip.h
> @@ -0,0 +1,4 @@
> +extern struct chip_operations cpu_intel_ep80579_ops;
>
> +struct cpu_intel_ep80579_config {
> +};

Ditto.
> Index: coreboot-v2-3363/src/cpu/intel/ep80579/ep80579.c
>
> --- /dev/null
> +++ coreboot-v2-3363/src/cpu/intel/ep80579/ep80579.c

Ditto.
> @@ -0,0 +1,7 @@
> +#include <device/device.h>
> +#include "chip.h"

One empty line suffices.
> +struct chip_operations cpu_intel_ep80579_ops = {
> CHIP_NAME("EP80579 CPU")
>
> +};
> Index: coreboot-v2-3363/src/cpu/intel/ep80579/ep80579_init.c
>
> --- /dev/null
> +++ coreboot-v2-3363/src/cpu/intel/ep80579/ep80579_init.c
> @@ -0,0 +1,68 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Arastra, Inc.
> This program is free software; you can redistribute it and/or modify
> it under the terms of the GNU General Public License version 2 as
> published by the Free Software Foundation.
> 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
>

^^^^^
This empty comment line not needed.
> */
>
> +#include <console/console.h>
> +#include <device/device.h>
> +#include <device/device.h>

One #include suffices.
> +#include <device/pci.h>
> +#include <string.h>
> +#include <cpu/cpu.h>
> +#include <cpu/x86/mtrr.h>
> +#include <cpu/x86/msr.h>
> +#include <cpu/x86/lapic.h>
> +#include <cpu/intel/microcode.h>
> +#include <cpu/x86/cache.h>
> +#include <cpu/x86/mtrr.h>
>
> +static uint32_t microcode_updates[] = {

s/uint32_t/u32/, we'll need that for v3 anyway (later).
> /*  Dummy terminator  */
> 0x0, 0x0, 0x0, 0x0,
> 0x0, 0x0, 0x0, 0x0,
> 0x0, 0x0, 0x0, 0x0,
> 0x0, 0x0, 0x0, 0x0,
>
> +};

Only one newline.
> +static void ep80579_init(device_t dev)
> +{
> /* Turn on caching if we haven't already */
> x86_enable_cache();
> x86_setup_mtrrs(36);
> x86_mtrr_check();
> /* Update the microcode */
> intel_update_microcode(microcode_updates);
> /* Enable the local cpu apics */
> setup_lapic();
>
> +};
>
> +static struct device_operations cpu_dev_ops = {
> .init = ep80579_init,
>
> +};

<---- missing newline
> +static struct cpu_device_id cpu_table[] = {
> { X86_VENDOR_INTEL, 0x10650 }, /* EP80579 */
> { 0, 0 },
>
> +};

Is 0x10650 currently the only known/existing CPU ID?
> +static struct cpu_driver driver __cpu_driver = {
> .ops = &cpu_dev_ops,
> .id_table = cpu_table,
>
> +};

Uwe.