On Fri, May 14, 2010 at 09:27:46PM +0200, Stefan Reinauer wrote: > Add support for the Getac P470 > > Signed-off-by: Stefan Reinauer <[email protected]>
Great, thanks! Acked-by: Uwe Hermann <[email protected]> See below for a quick review. > Index: mainboard/getac/Kconfig > =================================================================== > --- mainboard/getac/Kconfig (revision 0) > +++ mainboard/getac/Kconfig (revision 0) > @@ -0,0 +1,7 @@ Missing license header, please add. > Index: mainboard/getac/p470/Kconfig > =================================================================== > --- mainboard/getac/p470/Kconfig (revision 0) > +++ mainboard/getac/p470/Kconfig (revision 0) > @@ -0,0 +1,77 @@ Ditto, license header. > + select SUPERIO_SMSC_FDC37N972 > + select SUPERIO_SMSC_SIO10N268 Is this correct? Both are on the same board? > +config MAX_CPUS > + int > + default 4 > + depends on BOARD_GETAC_P470 > + > +config MAX_PHYSICAL_CPUS > + int > + default 2 > + depends on BOARD_GETAC_P470 Are these correct? 2 CPUs, 4 cores? Or am I reading this incorrectly? > +config FALLBACK_VGA_BIOS_FILE > + string > + default "../misc/getac-pci8086,27a2.rom" > + depends on BOARD_GETAC_P470 Not sure about this, we put lots of efforts into removing hardcoded paths to payloads and VGA blobs etc, we should not introduce new ones here I guess. Putting in the filename as default (the filename as generated by amideco or bios_extract etc) is useful, but I'd not leave in the ../misc/ path. > Index: mainboard/getac/p470/Makefile.inc > =================================================================== > --- mainboard/getac/p470/Makefile.inc (revision 0) > +++ mainboard/getac/p470/Makefile.inc (revision 0) > @@ -0,0 +1,27 @@ > +## > +## This file is part of the coreboot project. > +## > +## Copyright (C) 2007-2008 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; version 2 of the License. > +## > +## 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 mainboard requires DCACHE_AS_RAM enabled. It won't work without. > +## Is this comment still required? Doesn't seem to refer to any of the Makefile.inc entries here. Drop? > +driver-y += rtl8168.o > +obj-$(CONFIG_HAVE_ACPI_SLIC) += acpi_slic.o > + > +smmobj-$(CONFIG_HAVE_SMI_HANDLER) += mainboard_smi.o > Index: mainboard/getac/p470/mainboard_smi.c > =================================================================== > --- mainboard/getac/p470/mainboard_smi.c (revision 0) > +++ mainboard/getac/p470/mainboard_smi.c (revision 0) > @@ -0,0 +1,203 @@ [...] > +#define MAX_LCD_BRIGHTNESS 0xd8 This is redefined in some later file, can we put it in some header? > + switch (hotkey) { > + case 0x3b: break; // Fn+F1 > + case 0x3c: break; // Fn+F2 > + case 0x3d: break; // Fn+F3 > + case 0x3e: break; // Fn+F4 > + case 0x3f: break; // Fn+F5 > + case 0x40: // Fn+F6 > + reg8 = ec_read(0x17); > + reg8 = (reg8 > 8) ? (reg8 - 8) : 0; > + ec_write(0x17, reg8); > + return; Please add a short comment what "Fn+F6" does, just for info. > + case 0x41: // Fn+F7 > + reg8 = ec_read(0x17); > + reg8 += 8; > + reg8 = (reg8 >= MAX_LCD_BRIGHTNESS) ? MAX_LCD_BRIGHTNESS : > reg8; > + ec_write(0x17, reg8); > + return; This one is likely brigthness control, but a short comment would still be nice. > +#define OLD_ACPI 0 > +#if OLD_ACPI What's the reason for this? What's old vs. new ACPI? > + /* Pack GNVS into the ACPI table area */ > + for (i=0; i < dsdt->length; i++) { Minor issue: ^^ spaces before and after '='. > + if (*(u32*)(((u32)dsdt) + i) == 0xC0DEBABE) { > + printk(BIOS_DEBUG, "ACPI: Patching up global NVS in > DSDT at offset 0x%04x -> 0x%08x\n", i, (u32)current); > + *(u32*)(((u32)dsdt) + i) = current; // 0x92 bytes > + break; > + } > + } > + > + /* And fill it */ > + acpi_create_gnvs((global_nvs_t *)current); > + > + /* Keep pointer around */ > + gnvs = (void *)current; > + > + current += 0x100; > + ALIGN_CURRENT; > + > + for (i=0; i < dsdt->length; i++) { Ditto. > +#define DMI_TABLE_SIZE 0x55 > + > +static u8 dmi_table[DMI_TABLE_SIZE] = { No "const" here? > + 0x5f, 0x53, 0x4d, 0x5f, 0x29, 0x1f, 0x02, 0x03, 0x55, 0x00, 0x00, 0x00, > 0x00, 0x00, 0x00, 0x00, > + 0x5f, 0x44, 0x4d, 0x49, 0x5f, 0x61, 0x35, 0x00, 0xa0, 0xff, 0x0f, 0x00, > 0x01, 0x00, 0x23, 0x00, > + 0x00, 0x14, 0x00, 0x00, 0x01, 0x02, 0x00, 0xe0, 0x03, 0x07, 0x90, 0xde, > 0xcb, 0x7f, 0x00, 0x00, > + 0x00, 0x00, 0x37, 0x01, 0x63, 0x6f, 0x72, 0x65, 0x73, 0x79, 0x73, 0x74, > 0x65, 0x6d, 0x73, 0x20, > + 0x47, 0x6d, 0x62, 0x48, 0x00, 0x32, 0x2e, 0x30, 0x00, 0x30, 0x33, 0x2f, > 0x31, 0x33, 0x2f, 0x32, > + 0x30, 0x30, 0x38, 0x00, 0x00 > +}; > Index: mainboard/getac/p470/hda_verb.h > =================================================================== > --- mainboard/getac/p470/hda_verb.h (revision 0) > +++ mainboard/getac/p470/hda_verb.h (revision 0) > @@ -0,0 +1,100 @@ [...] > +static u32 mainboard_cim_verb_data[] = { No "const"? > + /* coreboot specific header */ > + 0x10ec0262, // Codec Vendor ID / Device ID > + 0x10714700, // Subsystem ID > + 0x0000000d, // Number of jacks [....] > +}; > +extern u32 * cim_verb_data; > +extern u32 cim_verb_data_size; What's the purpose of these? > Index: mainboard/getac/p470/romstage.c > =================================================================== > --- mainboard/getac/p470/romstage.c (revision 0) > +++ mainboard/getac/p470/romstage.c (revision 0) > @@ -0,0 +1,423 @@ > +/* This box has two superios, so enabling serial becomes slightly excessive. > + * We disable a lot of stuff to make sure that there are no conflicts between > + * the two. Also set up the GPIOs from the beginning. This is the "no > schematic > + * but safe anyways" method. > + */ > +static inline void pnp_enter_ext_func_mode(device_t dev) The "inline" is very likeley unneeded, can be dropped. > Index: mainboard/getac/p470/devicetree.cb > =================================================================== > --- mainboard/getac/p470/devicetree.cb (revision 0) > +++ mainboard/getac/p470/devicetree.cb (revision 0) > @@ -0,0 +1,145 @@ [...] > + #device pci 1f.4 off end # Realtek ID Codec Why commented out? Should it be dropped? > Index: mainboard/getac/p470/irq_tables.c > =================================================================== > --- mainboard/getac/p470/irq_tables.c (revision 0) > +++ mainboard/getac/p470/irq_tables.c (revision 0) > @@ -0,0 +1,63 @@ > +#include <arch/pirq_routing.h> > + > +const struct irq_routing_table intel_irq_routing_table = { > + PIRQ_SIGNATURE, /* u32 signature */ > + PIRQ_VERSION, /* u16 version */ > + 32+16*18, /* There can be total 18 devices on the bus */ 18 -> CONFIG_IRQ_SLOT_COUNT > Index: mainboard/getac/p470/acpi_slic.c > =================================================================== > --- mainboard/getac/p470/acpi_slic.c (revision 0) > +++ mainboard/getac/p470/acpi_slic.c (revision 0) > @@ -0,0 +1,38 @@ > +#include <string.h> > +#include <device/pci.h> > +#include <arch/acpi.h> > + > +static const char slic[0x4] = { > + 0x53, 0x4c, 0x49, 0x43 /* incomplete. */ > + /* If you wish to compile coreboot images with a windows license key > + * built in, you need to extract the ACPI SLIC from your machine and > + * add it here. > + */ Holy crap, ACPI has embedded Windows license keys? > Index: mainboard/getac/p470/rtl8168.c > =================================================================== > --- mainboard/getac/p470/rtl8168.c (revision 0) > +++ mainboard/getac/p470/rtl8168.c (revision 0) > @@ -0,0 +1,77 @@ > +#include <console/console.h> > +#include <device/device.h> > +#include <device/pci.h> > +#include <device/pci_ids.h> > +#include <delay.h> > + > +// #define RTL8168_DEBUG 1 Would be nice if this could also be in the Kconfig Debugging menu, as the other debug options. > Index: mainboard/getac/p470/dsdt.asl > =================================================================== > --- mainboard/getac/p470/dsdt.asl (revision 0) > +++ mainboard/getac/p470/dsdt.asl (revision 0) > @@ -0,0 +1,59 @@ > +#define ENABLE_TPM > +#undef ENABLE_FDC // There is no Floppy for this laptop > + > +DefinitionBlock( > + "dsdt.aml", > + "DSDT", > + 0x02, // DSDT revision: ACPI v2.0 > + "COREv2", // OEM id v4? > Index: mainboard/getac/p470/mainboard.c > =================================================================== > --- mainboard/getac/p470/mainboard.c (revision 0) > +++ mainboard/getac/p470/mainboard.c (revision 0) > @@ -0,0 +1,110 @@ > +#define MAX_LCD_BRIGHTNESS 0xd8 This is already defined in some other file above. Uwe. -- http://hermann-uwe.de | http://sigrok.org http://randomprojects.org | http://unmaintained-free-software.org -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
