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

[Uwe Hermann]

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.