Re: [coreboot] patch: dbe62

[Carl-Daniel Hailfinger]

On 28.02.2008 23:41, ron minnich wrote:
> This is probably just about done save for dram timing.
>
> ron
>
> This set of changes adds what is probably mostly correct support for dbe62.
>
> Signed-off-by: Ronald G. Minnich

I like it. A few minor nitpicks, the rest is

Compile tested and
Acked-by: Carl-Daniel Hailfinger

Regards,
Carl-Daniel
> Index: mainboard/artecgroup/dbe62/Kconfig
>
> --- mainboard/artecgroup/dbe62/Kconfig	(revision 0)
> +++ mainboard/artecgroup/dbe62/Kconfig	(revision 0)
> @@ -0,0 +1,28 @@
> +##
> +## This file is part of the coreboot project.
> +##
> +## Copyright (C) 2007 coresystems GmbH
> +## (Written by Stefan Reinauer  for coresystems GmbH)
> +## Copyright (C) 2007 Ronald G. Minnich 
> +##
> +## 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; either version 2 of the License, or
> +## (at your option) any later version.
> +##
> +## 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
> +##
>
> +config MAINBOARD_NAME
> string
> default artecgroup/dbe62
> depends BOARD_ARTECGROUP_DBE62
> help
>  This is the default mainboard name.
> Index: mainboard/artecgroup/dbe62/stage1.c
>
> --- mainboard/artecgroup/dbe62/stage1.c	(revision 0)
> +++ mainboard/artecgroup/dbe62/stage1.c	(revision 0)
> @@ -0,0 +1,69 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2007 Advanced Micro Devices, Inc.

Add your copyright as well?
> 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; either version 2 of the License, or
> (at your option) any later version.
> 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 <types.h>
> +#include <lib.h>
> +#include <console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <string.h>
> +#include <msr.h>
> +#include <io.h>
> +#include <amd_geodelx.h>
> +#include <southbridge/amd/cs5536/cs5536.h>
> +#include <northbridge/amd/geodelx/raminit.h>
>
> +static const struct wmsr {
> u32 reg;
> struct msr  msr;
>
> +}

Maybe split this and move the struct declaration to a header?
> dbe62_msr[] = {
> {.reg = 0x10000020, {.lo = 0x00fff80, .hi = 0x20000000}},
> {.reg = 0x10000021, {.lo = 0x80fffe0, .hi = 0x20000000}},
> {.reg = 0x40000020, {.lo = 0x00fff80, .hi = 0x20000000}},
> {.reg = 0x40000021, {.lo = 0x80fffe0, .hi = 0x20000000}},
>
> +};
>
> +static void dbe62_msr_init(void)
> +{
> int i;
> for (i = 0; i < ARRAY_SIZE(dbe62_msr); i++)
> wrmsr(dbe62_msr[i].reg, dbe62_msr[i].msr);
>
> +}
>
> +void hardware_stage1(void)
> +{
> post_code(POST_START_OF_MAIN);
> dbe62_msr_init();
> cs5536_stage1();
> /*
> * NOTE: Must do this AFTER the early_setup! It is counting on some
> * early MSR setup for the CS5536.
> */
> cs5536_setup_onchipuart();
>
> +}
>
> +void mainboard_pre_payload(void)
> +{
> geode_pre_payload();
> banner(BIOS_DEBUG, "mainboard_pre_payload: done");
>
> +}
> Index: mainboard/artecgroup/dbe62/irq_tables.c
> ===================================================================
> --- mainboard/artecgroup/dbe62/irq_tables.c	(revision 0)
> +++ mainboard/artecgroup/dbe62/irq_tables.c	(revision 0)
> @@ -0,0 +1,120 @@
> +/*
> +* This file is part of the coreboot project.
> +*
> +* Copyright (C) 2007 Advanced Micro Devices, Inc.

Add your copyright as well?
> +*
> +* 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 <types.h>
> +#include <lib.h>
> +#include <console.h>
> +#include <device/device.h>
> +#include <device/pci.h>
> +#include <string.h>
> +#include <msr.h>
> +#include <io.h>
> +#include <pirq_routing.h>
> +#include <amd_geodelx.h>
> +#include "../../../southbridge/amd/cs5536/cs5536.h"
>
> +/* Platform IRQs */
> +#define PIRQA 10
> +#define PIRQB 11
> +#define PIRQC 10
> +#define PIRQD 11
>
> +/* Map */
> +#define M_PIRQA (1 << PIRQA)	/* Bitmap of supported IRQs */
> +#define M_PIRQB (1 << PIRQB)	/* Bitmap of supported IRQs */
> +#define M_PIRQC (1 << PIRQC)	/* Bitmap of supported IRQs */
> +#define M_PIRQD (1 << PIRQD)	/* Bitmap of supported IRQs */
>
> +/* Link */
> +#define L_PIRQA 1		/* Means Slot INTx# Connects To Chipset INTA# */
> +#define L_PIRQB 2		/* Means Slot INTx# Connects To Chipset INTB# */
> +#define L_PIRQC 3		/* Means Slot INTx# Connects To Chipset INTC# */
> +#define L_PIRQD 4		/* Means Slot INTx# Connects To Chipset INTD# */
>
> +const struct irq_routing_table intel_irq_routing_table = {
> PIRQ_SIGNATURE,
> PIRQ_VERSION,
> 32 + 16 * 5,		/* Max. number of devices on the bus */
> 0x00,			/* Where the interrupt router lies (bus) */
> (0x0F << 3) | 0x0,      /* Where the interrupt router lies (dev) */
> 0x00,			/* IRQs devoted exclusively to PCI usage */
> 0x100B,			/* Vendor */
> 0x002B,			/* Device */
> 0,			/* Crap (miniport) */
> {0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0},	/* u8 rfu[11] */
> 0x00,			/* Checksum */
> {
> /* If you change the number of entries, change IRQ_SLOT_COUNT above! */
> /* bus, dev|fn,           {link, bitmap},      {link, bitmap},     {link, bitmap},     {link, bitmap},     slot, rfu */
> /* bus, dev|fn,           {link, bitmap},      {link, bitmap},     {link, bitmap},     {link, bitmap},     slot, rfu */

The comment above seems to exist twice.
> {0x00, (0x01 << 3) | 0x0, {{L_PIRQA, M_PIRQA}, {0x00, 0x00}, {0x00, 0x00}, {0x00, 0x00}}, 0x0, 0x0},	/* cpu */
> {0x00, (0x0F << 3) | 0x0, {{L_PIRQA, M_PIRQA}, {L_PIRQB, M_PIRQB}, {L_PIRQC, M_PIRQC}, {L_PIRQD, M_PIRQD}}, 0x0, 0x0},	/* chipset */
> {0x00, (0x0D << 3) | 0x0, {{L_PIRQC, M_PIRQC}, {0x00, 0x00}, {0x00, 0x00}, {0x00, 0x00}}, 0x0, 0x0},	/* ethernet */
> }
>
> +};
>
> +unsigned long write_pirq_routing_table(unsigned long addr)
> +{
> int i, j, k, num_entries;
> unsigned char pirq[4];
> u16 chipset_irq_map;
> u32 pciAddr, pirtable_end;
> struct irq_routing_table *pirq_tbl;
> pirtable_end = copy_pirq_routing_table(addr);
> /* Set up chipset IRQ steering. */
> pciAddr = 0x80000000 | (CHIPSET_DEV_NUM << 11) | 0x5C;
> chipset_irq_map = (PIRQD << 12 | PIRQC << 8 | PIRQB << 4 | PIRQA);
> printk(BIOS_DEBUG, "%s(%08X, %04X)\n", __FUNCTION__, pciAddr,
>      chipset_irq_map);
> outl(pciAddr & ~3, 0xCF8);
> outl(chipset_irq_map, 0xCFC);
> pirq_tbl = (struct irq_routing_table *) (addr);
> num_entries = (pirq_tbl->size - 32) / 16;
> /* Set PCI IRQs. */
> for (i = 0; i < num_entries; i++) {
> printk(BIOS_DEBUG, "PIR Entry %d Dev/Fn: %X Slot: %d\n", i,
> 	     pirq_tbl->slots[i].devfn, pirq_tbl->slots[i].slot);
> for (j = 0; j < 4; j++) {
> 	printk(BIOS_DEBUG, "INT: %c bitmap: %x ", 'A' + j,
> 		     pirq_tbl->slots[i].irq[j].bitmap);
> 	/* Finds lsb in bitmap to IRQ#. */
> 	for (k = 0; 
> 	     (!((pirq_tbl->slots[i].irq[j].bitmap >> k) & 1)) 
> 		     && (pirq_tbl->slots[i].irq[j].bitmap != 0);
> 	     k++);
> 	pirq[j] = k;
> 	printk(BIOS_DEBUG, "PIRQ: %d\n", k);
> }
> /* Bus, device, slots IRQs for {A,B,C,D}. */
> pci_assign_irqs(pirq_tbl->slots[i].bus,
> 		pirq_tbl->slots[i].devfn >> 3, pirq);
> }
> /* Put the PIR table in memory and checksum. */
> return pirtable_end;
>
> +}
> Index: mainboard/artecgroup/dbe62/dts
> ===================================================================
> --- mainboard/artecgroup/dbe62/dts	(revision 0)
> +++ mainboard/artecgroup/dbe62/dts	(revision 0)
> @@ -0,0 +1,48 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2007 Ronald G. Minnich

2008?
> 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; either version 2 of the License, or
> (at your option) any later version.
> 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
> */
>
> +/{
> mainboard-vendor = "Artec";
> mainboard-name = "DBE62";
> cpus { };
> apic@0 {
> /config/("northbridge/amd/geodelx/apic");
> };
> domain@0 {
> /config/("northbridge/amd/geodelx/domain");
> pci@1,0 {
> 	/config/("northbridge/amd/geodelx/pci");
> };
> pci@15,0 {
> 	/config/("southbridge/amd/cs5536/dts");
> 	enable_ide = "1";
> 	/* Interrupt enables for LPC bus.
> 	 *  Each bit is an IRQ 0-15. */
> 	lpc_serirq_enable = "0x00001002";
> 	/* LPC IRQ polarity. Each bit is an IRQ 0-15. */
> 	lpc_serirq_polarity = "0x0000EFFD";
> 	/* 0:continuous 1:quiet */
> 	lpc_serirq_mode = "1";
> 	/* GPIO(0-0x20) for INT D:C:B:A, 0xFF=none. 
> 	 * See virtual PIC spec. */
> 	enable_gpio_int_route = "0x0D0C0700";
> };
> };
>
> +};
> Index: mainboard/artecgroup/dbe62/cmos.layout
> ===================================================================
> --- mainboard/artecgroup/dbe62/cmos.layout	(revision 0)
> +++ mainboard/artecgroup/dbe62/cmos.layout	(revision 0)
> @@ -0,0 +1,74 @@
> +entries
>
> +#start-bit length  config config-ID    name
> +#0            8       r       0        seconds
> +#8            8       r       0        alarm_seconds
> +#16           8       r       0        minutes
> +#24           8       r       0        alarm_minutes
> +#32           8       r       0        hours
> +#40           8       r       0        alarm_hours
> +#48           8       r       0        day_of_week
> +#56           8       r       0        day_of_month
> +#64           8       r       0        month
> +#72           8       r       0        year
> +#80           4       r       0        rate_select
> +#84           3       r       0        REF_Clock
> +#87           1       r       0        UIP
> +#88           1       r       0        auto_switch_DST
> +#89           1       r       0        24_hour_mode
> +#90           1       r       0        binary_values_enable
> +#91           1       r       0        square-wave_out_enable
> +#92           1       r       0        update_finished_enable
> +#93           1       r       0        alarm_interrupt_enable
> +#94           1       r       0        periodic_interrupt_enable
> +#95           1       r       0        disable_clock_updates
> +#96         288       r       0        temporary_filler
> +0          384       r       0        reserved_memory
> +384          1       e       4        boot_option
> +385          1       e       4        last_boot
> +386          1       e       1        ECC_memory
> +388          4       r       0        reboot_bits
> +392          3       e       5        baud_rate
> +400          1       e       1        power_on_after_fail
> +412          4       e       6        debug_level
> +416          4       e       7        boot_first
> +420          4       e       7        boot_second
> +424          4       e       7        boot_third
> +428          4       h       0        boot_index
> +432	     8       h       0        boot_countdown
> +1008         16      h       0        check_sum
>
> +enumerations
>
> +#ID value   text
> +1     0     Disable
> +1     1     Enable
> +2     0     Enable
> +2     1     Disable
> +4     0     Fallback
> +4     1     Normal
> +5     0     115200
> +5     1     57600
> +5     2     38400
> +5     3     19200
> +5     4     9600
> +5     5     4800
> +5     6     2400
> +5     7     1200
> +6     6     Notice
> +6     7     Info
> +6     8     Debug
> +6     9     Spew
> +7     0     Network
> +7     1     HDD
> +7     2     Floppy
> +7     8     Fallback_Network
> +7     9     Fallback_HDD
> +7     10    Fallback_Floppy
> +#7     3     ROM
>
> +checksums
>
> +checksum 392 1007 1008
> Index: mainboard/artecgroup/dbe62/Makefile
>
> --- mainboard/artecgroup/dbe62/Makefile	(revision 0)
> +++ mainboard/artecgroup/dbe62/Makefile	(revision 0)
> @@ -0,0 +1,35 @@
> +##
> +## This file is part of the coreboot project.
> +##
> +## Copyright (C) 2006-2007 coresystems GmbH
> +## (Written by Stefan Reinauer  for 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; either version 2 of the License, or
> +## (at your option) any later version.
> +##
> +## 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
> +##
>
> +STAGE0_MAINBOARD_OBJ := $(obj)/mainboard/$(MAINBOARDDIR)/stage1.o
>
> +INITRAM_OBJ =   $(src)/mainboard/$(MAINBOARDDIR)/initram.c \
> $(src)/northbridge/amd/geodelx/raminit.c \
> $(src)/arch/x86/geodelx/geodelx.c
>
> +STAGE2_MAINBOARD_OBJ =
>
> +STAGE2_MAINBOARD_OBJ += irq_tables.o

Why set STAGE2_MAINBOARD_OBJ once and add to it once?
> +$(obj)/coreboot.vpd:
> $(Q)printf "  BUILD   DUMMY VPD\n"
> $(Q)dd if=/dev/zero of=$(obj)/coreboot.vpd bs=256 count=1 $(SILENT)
> Index: mainboard/artecgroup/Kconfig
>
> --- mainboard/artecgroup/Kconfig	(revision 623)
> +++ mainboard/artecgroup/Kconfig	(working copy)
> @@ -30,10 +30,23 @@
>    select OPTION_TABLE
>    select NORTHBRIDGE_AMD_GEODELX
>    select SOUTHBRIDGE_AMD_CS5536
> select PIRQ_TABLE
> help
> Artec Group DBE61, also known as ThinCan and Linutop.
>
> +source "mainboard/artecgroup/dbe61/Kconfig"

Please move the line above to the end of the file. Otherwise you get 
strange artifacts in menuconfig after selecting dbe61.
> +config BOARD_ARTECGROUP_DBE62
> bool "DBE62"
> select ARCH_X86
> select CPU_AMD_GEODELX
> select OPTION_TABLE
> select NORTHBRIDGE_AMD_GEODELX
> select SOUTHBRIDGE_AMD_CS5536
> select PIRQ_TABLE
> help
>  Artec Group DBE62, also known as ThinCan and Linutop.

AFAIK the Linutop 1 is equivalent to DBE61 and Linutop 2 is equivalent 
to DBE62.
> endchoice
>
> -source "mainboard/artecgroup/dbe61/Kconfig"
>
> +source "mainboard/artecgroup/dbe62/Kconfig"