[coreboot] [PATCH 6/6] Intel EP80579 Development Board mainboard

Uwe Hermann uwe at hermann-uwe.de
Mon Aug 25 22:49:51 CEST 2008


Comments below, but this should be the last commit anyway (after all the
NB/SB code is committed; I'll try to review that ASAP).

On Wed, Aug 20, 2008 at 09:19:35AM -0700, Ed Swierk wrote:
> Index: coreboot-v2-3363/src/mainboard/intel/truxton/Config.lb
> ===================================================================
> --- /dev/null
[...]
> +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

> +                device pci 04.0 on end
> +                device pci 08.0 off end
> +                device pci 0d.0 off end
> +                device pci 0d.1 off end

What are these? Please add comments. Why are three of them disabled?
Not available on this board?


> +                        device pci 1f.2 on end  # SATA
> +                        device pci 1f.3 on end  # SMBus

> +                        device pci 1f.4 on end

Same here.


> +                end
> +        end
> +        device apic_cluster 0 on
> +                chip cpu/intel/ep80579
> +                        device apic 0 on end
> +                end
> +        end
> +end


> Index: coreboot-v2-3363/src/mainboard/intel/truxton/auto.c
> ===================================================================
> --- /dev/null
> +++ coreboot-v2-3363/src/mainboard/intel/truxton/auto.c
> @@ -0,0 +1,114 @@
[...]
> +static void main(unsigned long bist)
> +{
> +	msr_t msr;
> +	u16 perf;
> +	static const struct mem_controller mch[] = {
> +		{
> +			.node_id = 0,
> +			.f0 = PCI_DEV(0, 0x00, 0),
> +			.channel0 = { (0xa<<3)|2, (0xa<<3)|3 },
> +		}
> +	};
> +
> +	if (bist == 0) {
> +		/* Skip this if there was a built in self test failure */
> +		early_mtrr_init();
> +		if (memory_initialized()) {
> +			asm volatile ("jmp __cpu_reset");
> +		}
> +	}
> +	/* Set up the console */
> +	i3100_enable_superio();
> +	i3100_enable_serial(0x4e, I3100_SP1, TTYS0_BASE);

Maybe make the 0x4e a #define (e.g. I3100_SUPERIO_CONFIG_PORT) for
better readability.


> +	uart_init();
> +	console_init();
> +
> +	/* Prevent the TCO timer from rebooting us */
> +	i3100_halt_tco_timer();
> +
> +	/* Halt if there was a built in self test failure */
> +	report_bist_failure(bist);
> +
> +#if 0
> +	print_pci_devices();
> +#endif
> +	enable_smbus();
> +#if 1
> +	dump_spd_registers();
> +#endif
> +
> +	sdram_initialize(ARRAY_SIZE(mch), mch);
> +#if 1
> +	dump_pci_devices();
> +#endif
> +	dump_pci_device(PCI_DEV(0, 0x00, 0));
> +#if 0
> +	dump_bar14(PCI_DEV(0, 0x00, 0));
> +#endif
> +
> +#if 0
> +	ram_fill(0x00000000, 0x20000000);
> +	ram_verify(0x00000000, 0x20000000);
> +#endif

Remove the "#if 1" please, and turn "#if 0" into regular
/*foo*/-style comments if the functions are needed/wanted.


> Index: coreboot-v2-3363/src/mainboard/intel/truxton/irq_tables.c
> ===================================================================
> --- /dev/null
> +++ coreboot-v2-3363/src/mainboard/intel/truxton/irq_tables.c
> @@ -0,0 +1,44 @@
> +/*
> + * 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
> + *
> + */
> +
> +#include <arch/pirq_routing.h>
> +
> +const struct irq_routing_table intel_irq_routing_table = {
> +	PIRQ_SIGNATURE, /* u32 signature */
> +	PIRQ_VERSION,   /* u16 version   */
> +	32+16*IRQ_SLOT_COUNT, /* u16 Table size 32+(16*devices)  */
> +	0x00,       /* u8 Bus 0 */
> +	(0x1f << 3) | 0x0, /* u8 Device 1f, Function 0 */
> +	0x0000,     /* u16 reserve IRQ for PCI */
> +	0x8086,     /* u16 Vendor */
> +	0x5031,     /* Device ID */
> +	0x00000000, /* u32 miniport_data */
> +	{ 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 }, /* u8 rfu[11] */
> +	0x5e,   /*  u8 checksum - mod 256 checksum must give zero */
> +	{  /* bus, devfn, {link, bitmap}, {link, bitmap}, {link, bitmap}, {link, bitmap}, slot, rfu  */

> +	    {0x00, 0xf8, {{0x62, 0xdc78}, {0x61, 0xdcf8}, {0x00, 0x0000}, {0x00, 0x0000}}, 0x00,  0x00},

Please add a coment what device/slot this is.


> Index: coreboot-v2-3363/src/mainboard/intel/truxton/mptable.c
> ===================================================================
> --- /dev/null
> +++ coreboot-v2-3363/src/mainboard/intel/truxton/mptable.c
> @@ -0,0 +1,199 @@
[...]
> +void *smp_write_config_table(void *v)
> +{
> +	static const char sig[4] = "PCMP";
> +	static const char oem[8] = "Intel   ";
> +	static const char productid[12] = "Truxton     ";
> +	struct mp_config_table *mc;
> +	u8 bus_num;
> +	u8 bus_isa;
> +	u8 bus_pea0 = 0;
> +	u8 bus_pea1 = 0;
> +	u8 bus_aioc;
> +
> +	mc = (void *)(((char *)v) + SMP_FLOATING_TABLE_LEN);
> +	memset(mc, 0, sizeof(*mc));
> +
> +	memcpy(mc->mpc_signature, sig, sizeof(sig));
> +	mc->mpc_length = sizeof(*mc); /* initially just the header */
> +	mc->mpc_spec = 0x04;
> +	mc->mpc_checksum = 0; /* not yet computed */
> +	memcpy(mc->mpc_oem, oem, sizeof(oem));
> +	memcpy(mc->mpc_productid, productid, sizeof(productid));
> +	mc->mpc_oemptr = 0;
> +	mc->mpc_oemsize = 0;
> +	mc->mpc_entry_count = 0; /* No entries yet... */
> +	mc->mpc_lapic = LAPIC_ADDR;
> +	mc->mpe_length = 0;
> +	mc->mpe_checksum = 0;
> +	mc->reserved = 0;
> +
> +	smp_write_processors(mc);
> +
> +	{
> +		device_t dev;
> +
> +		/* AIOC bridge */
> +		dev = dev_find_slot(0, PCI_DEVFN(0x04,0));
> +		if (dev) {
> +			bus_aioc = pci_read_config8(dev, PCI_SECONDARY_BUS);
> +			bus_isa = pci_read_config8(dev, PCI_SUBORDINATE_BUS);
> +			bus_isa++;
> +		}
> +		else {
> +			printk_debug("ERROR - could not find PCI 0:04.0\n");
> +			bus_aioc = 0;
> +			bus_isa = 9;
> +		}
> +		/* PCIe A0 */
> +		dev = dev_find_slot(0, PCI_DEVFN(0x02,0));
> +		if (dev) {
> +			bus_pea0 = pci_read_config8(dev, PCI_SECONDARY_BUS);
> +		}
> +		else {
> +			printk_debug("ERROR - could not find PCI 0:02.0\n");
> +			bus_pea0 = 0;
> +		}
> +		/* PCIe A1 */
> +		dev = dev_find_slot(0, PCI_DEVFN(0x03,0));
> +		if (dev) {
> +			bus_pea1 = pci_read_config8(dev, PCI_SECONDARY_BUS);
> +		}
> +		else {
> +			printk_debug("ERROR - could not find PCI 0:03.0\n");
> +			bus_pea1 = 0;
> +		}
> +	}

Thy is this an extra block? Any technical reason? If yes, it should be
documented in a comment, or the block be "inlined".


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org




More information about the coreboot mailing list