[PATCH 5/5] Intel 3100, version 3: Intel 3100 development kit mainboard

[Ed Swierk]

This patch implements support for the Intel 3100 Development Kit
mainboard, aka "Mt. Arvon".

Also attached is the output of lspci -tnvn, lspci -vv, and lspci -xxxx.

--Ed

[Ed Swierk]

Signed-off-by: Ed Swierk

--Ed

[Peter Stuge]

On Thu, Mar 13, 2008 at 02:49:55PM -0700, Ed Swierk wrote:
> +#if 0
> print_pci_devices();
>
> +#endif

..
> +#if 0
> dump_spd_registers();
>
> +#endif

..
> +#if 0
> dump_pci_devices();
> dump_pci_device(PCI_DEV(0, 0x00, 0));
> dump_bar14(PCI_DEV(0, 0x00, 0));
>
> +#endif
>
> +#if 1
> ram_check(0x00000000, 0x00100000);
>
> +#endif
>
> +#if 0
> while(1) {
> hlt();
> }
>
> +#endif

These aren't so nice.. Possible to use the loglevel somehow?

I'm inclined to ack anyway though.

//Peter

[PATCH 5/5] Intel 3100, version 3: Intel 3100 development kit mainboard

[Uwe Hermann]

On Thu, Mar 13, 2008 at 02:49:55PM -0700, Ed Swierk wrote:
> Index: coreboot-v2-3137/src/mainboard/intel/mtarvon/Config.lb

[...]
> +makerule ./failover.E
>    depends "$(MAINBOARD)/failover.c ./romcc"
>    action "./romcc -E -O --label-prefix=failover -I$(TOP)/src -I. $(CPPFLAGS) $(MAINBOARD)/failover.c -o $@"
>
> +end
>
> +makerule ./failover.inc
>    depends "$(MAINBOARD)/failover.c ./romcc"
>    action "./romcc    -O --label-prefix=failover -I$(TOP)/src -I. $(CPPFLAGS) $(MAINBOARD)/failover.c -o $@"
>
> +end

Please use the global failover.c file instead of duplicating it.
See e.g. src/mainboard/rca/rm4100/Config.lb for how to do that.

Maybe copy that file and adapt it, it's less cluttered and
cleaner, too.
> +chip northbridge/intel/i3100
>    device pci_domain 0 on
>            device pci 00.0 on end # IMCH
>            device pci 00.1 on end # IMCH error status
>            device pci 01.0 on end # IMCH EDMA engine
>            device pci 02.0 on end # PCIe port A/A0
>            device pci 03.0 on end # PCIe port A1
>            chip southbridge/intel/i3100
>                    # PIRQ line -> legacy IRQ mappings
>                    register "pirq_a_d" = "0x0b070a05"
>                    register "pirq_e_h" = "0x0a808080"
>                    device pci 1c.0 on end # PCIe port B0
>                    device pci 1c.1 on end # PCIe port B1
>                    device pci 1c.2 on end # PCIe port B2
>                    device pci 1c.3 on end # PCIe port B3
>                    device pci 1d.0 on end # USB (UHCI) 1
>                    device pci 1d.1 on end # USB (UHCI) 2
>                    device pci 1d.7 on end # USB (EHCI)
>                    device pci 1e.0 on end # PCI bridge
>                    device pci 1e.2 on end # audio
>                    device pci 1e.3 on end # modem
>                    device pci 1f.0 on     # LPC bridge
>                            chip superio/intel/i3100
>                                    device pnp 4e.4 on

Add comment "Com1".
>                                             io 0x60 = 0x3f8
>                                            irq 0x70 = 4
>                                    end
>                                    device pnp 4e.5 on

Add comment "Com2".

(or vice versa?)
>                                             io 0x60 = 0x2f8
>                                            irq 0x70 = 3
>                                    end
>                            end
>                    end
>                    device pci 1f.2 on end # SATA
>                    device pci 1f.3 on end # SMBus
>                    device pci 1f.4 on end

What is 1f.4? Add a comment for that, please.
>            end
>    end
>    device apic_cluster 0 on
>            chip cpu/intel/socket_mPGA479M
>                    device apic 0 on end
>            end
>    end
>
> +end
> Index: coreboot-v2-3137/src/mainboard/intel/mtarvon/Options.lb
> ===================================================================
> --- /dev/null
> +++ coreboot-v2-3137/src/mainboard/intel/mtarvon/Options.lb
> @@ -0,0 +1,261 @@

[...]
> +uses CROSS_COMPILE
> +uses OBJCOPY
> +uses MAX_REBOOT_CNT
> +uses USE_WATCHDOG_ON_BOOT

See above, please use src/mainboard/rca/rm4100/Options.lb
which is more concise and clean.
> +##
> +## Move the default coreboot cmos range off of AMD RTC registers
> +##
> +default LB_CKS_RANGE_START=49
> +default LB_CKS_RANGE_END=122
> +default LB_CKS_LOC=123

Needed for Intel board?
> +##
> +## Don't enable the btext console
> +##
> +default  CONFIG_CONSOLE_BTEXT=0

Needed?
> Index: coreboot-v2-3137/src/mainboard/intel/mtarvon/auto.c
>
> --- /dev/null
> +++ coreboot-v2-3137/src/mainboard/intel/mtarvon/auto.c
> @@ -0,0 +1,154 @@
> +/*
> 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 code is based on src/mainboard/intel/jarrell/auto.c */

Not needed, this is trivial/generic code which can't be written much
differently.
> /* Halt if there was a built in self test failure */
> report_bist_failure(bist);
>
> +#if 0
> print_pci_devices();
>
> +#endif
> enable_smbus();
>
> +#if 0
> dump_spd_registers();
>
> +#endif

Rather use /* */ comments here instead of '#if 0', looks cleaner
in this case (IMHO).
> power_down_reset_check();
> /* Enable SpeedStep and automatic thermal throttling */
> /* FIXME: move to Pentium M init code */
> msr = rdmsr(0x1a0);
> msr.lo |= (1 << 3) | (1 << 16);
> wrmsr(0x1a0, msr);
> msr = rdmsr(0x19d);
> msr.lo |= (1 << 16);
> wrmsr(0x19d, msr);
> /* Set CPU frequency/voltage to maximum */
> /* FIXME: move to Pentium M init code */
> msr = rdmsr(0x198);
> perf = msr.hi & 0xffff;
> msr = rdmsr(0x199);
> msr.lo &= 0xffff0000;
> msr.lo |= perf;
> wrmsr(0x199, msr);

How board-specific is this?
> sdram_initialize(sizeof(mch)/sizeof(mch[0]), mch);

Use ARRAY_SIZE.
> +#if 0
> dump_pci_devices();
> dump_pci_device(PCI_DEV(0, 0x00, 0));
> dump_bar14(PCI_DEV(0, 0x00, 0));
>
> +#endif
>
> +#if 1
> ram_check(0x00000000, 0x00100000);

ram_check(0, 1024 * 1024);
> +#endif
>
> +#if 0
> while(1) {
> hlt();
> }
>
> +#endif

Why? Drop?
> Index: coreboot-v2-3137/src/mainboard/intel/mtarvon/cmos.layout
>
> --- /dev/null
> +++ coreboot-v2-3137/src/mainboard/intel/mtarvon/cmos.layout
> @@ -0,0 +1,82 @@
> +entries

Is this tested? Otherwise drop it for now and only add it when it's
needed and has been tested.
> Index: coreboot-v2-3137/src/mainboard/intel/mtarvon/debug.c
>
> --- /dev/null
> +++ coreboot-v2-3137/src/mainboard/intel/mtarvon/debug.c
> @@ -0,0 +1,144 @@

Nah, please check one of the other gazillion debug.c files, and use one
of those. We should use one to lib/ or so anyway, but even until then
you can use debug.c from some other board, if needed by using a symlink.
> Index: coreboot-v2-3137/src/mainboard/intel/mtarvon/failover.c
>
> --- /dev/null
> +++ coreboot-v2-3137/src/mainboard/intel/mtarvon/failover.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 code is based on src/mainboard/intel/jarrell/failover.c */

Not needed, this is likely a trivial enough file.
> +#define ASSEMBLY 1
> +#include <stdint.h>
> +#include <device/pci_def.h>
> +#include <device/pci_ids.h>
> +#include <arch/io.h>
> +#include <arch/romcc_io.h>
> +#include <cpu/x86/lapic.h>
> +#include "pc80/serial.c"
> +#include "arch/i386/lib/console.c"
> +#include "pc80/mc146818rtc_early.c"
> +#include "cpu/x86/lapic/boot_cpu.c"
> +#include "northbridge/intel/i3100/memory_initialized.c"
>
> +static uint32_t main(uint32_t bist)
> +{
> /* Did just the cpu reset? */
> if (memory_initialized()) {
> 	if (last_boot_normal()) {
> 	goto normal_image;
> } else {
> 	goto cpu_reset;
> }
> }

This file is _almost_ the same as src/arch/i386/lib/failover.c, can they
be merged easily (to also support i3100)?
> /* This is the primary cpu how should I boot? */
> else if (do_normal_boot()) {
> goto normal_image;
> }
> else {
> goto fallback_image;
> }
> normal_image:
> asm volatile ("jmp __normal_image"
> : /* outputs */
> : "a" (bist) /* inputs */
> : /* clobbers */
> );
> cpu_reset:
> asm volatile ("jmp __cpu_reset"
> : /* outputs */
> : "a"(bist) /* inputs */
> : /* clobbers */
> );
> fallback_image:
> return bist;
>
> +}
> Index: coreboot-v2-3137/src/mainboard/intel/mtarvon/mptable.c
>
> --- /dev/null
> +++ coreboot-v2-3137/src/mainboard/intel/mtarvon/mptable.c
> @@ -0,0 +1,164 @@

[...]
> +/* This code is based on src/mainboard/intel/jarrell/mptable.c */

Not needed IMHO, this is highly board specific stuff and cannot be
written drastically different, I suppose.
> Index: coreboot-v2-3137/src/mainboard/intel/mtarvon/reset.c
>
> --- /dev/null
> +++ coreboot-v2-3137/src/mainboard/intel/mtarvon/reset.c
> @@ -0,0 +1,66 @@
> +/*
> 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 code is based on src/mainboard/intel/jarrell/reset.c */

Ditto, this is rather trivial/simple code and can't be written very
differently. Drop the line.
> +#include <arch/io.h>
> +#include <device/pci_def.h>
> +#include <device/pci_ids.h>
>
> +#ifndef __ROMCC__
> +#include <device/device.h>
>
> +#define PCI_ID(VENDOR_ID, DEVICE_ID) \
> ((((DEVICE_ID) & 0xFFFF) << 16) | ((VENDOR_ID) & 0xFFFF))

This should be in some other header maybe, it's not specific to i3100 in
any way.
> +#define PCI_DEV_INVALID 0
>
> +static inline device_t pci_locate_device(unsigned pci_id, device_t from)
> +{
> return dev_find_device(pci_id >> 16, pci_id & 0xffff, from);
>
> +}
> +#endif
>
> +void soft_reset(void)
> +{
> outb(0x04, 0xcf9);
>
> +}
>
> +void hard_reset(void)
> +{
> outb(0x02, 0xcf9);
> outb(0x06, 0xcf9);
>
> +}
>
> +void full_reset(void)

Maybe add a comment for this function.
> +{
> device_t dev;
> /* Enable power on after power fail... */
> dev = pci_locate_device(PCI_ID(PCI_VENDOR_ID_INTEL,
> 		       PCI_DEVICE_ID_INTEL_3100_LPC), 0);
> if (dev != PCI_DEV_INVALID) {
> unsigned byte;
> byte = pci_read_config8(dev, 0xa4);
> byte &= 0xfe;
> pci_write_config8(dev, 0xa4, byte);
> }
> outb(0x0e, 0xcf9);
>
> +}
> Index: coreboot-v2-3137/targets/intel/mtarvon/Config.lb
> ===================================================================
> --- /dev/null
> +++ coreboot-v2-3137/targets/intel/mtarvon/Config.lb
> @@ -0,0 +1,21 @@

Missing license header.
> +target mtarvon
> +mainboard intel/mtarvon
>
> +## ROM_SIZE is the total number of bytes allocated for coreboot use
> +## (normal AND fallback images and payloads).
> +option ROM_SIZE = 2 * 1024 * 1024
>
> +## ROM_IMAGE_SIZE is the maximum number of bytes allowed for a coreboot image,
> +## not including any payload.
> +option ROM_IMAGE_SIZE = 128 * 1024
>
> +## FALLBACK_SIZE is the amount of the ROM the complete fallback image
> +## (including payload) will use
> +option FALLBACK_SIZE = ROM_SIZE
>
> +romimage "fallback"
> option USE_FALLBACK_IMAGE=1
> payload /dev/null

Maybe something else here, e.g. /tmp/filo.elf? Yes, no matter what you
choose, it won't work for all users, but /dev/null won't work for _any_
user at all ;)
> +end
>
> +buildrom ./coreboot.rom ROM_SIZE "fallback"

Uwe.

[Ed Swierk]

Here is an updated patch that addresses most of Uwe's comments. I
removed debug.c and used the jarrell mainboard's debug.c instead;
removed failover.c and used the global version instead; removed
reset.c and added a hard_reset() function to the i3100 southbridge
code; and removed cmos.layout. (I love deleting code!)

Signed-off-by: Ed Swierk

--Ed