[coreboot] [PATCH] Getac P470 notebook support
Uwe Hermann
uwe at hermann-uwe.de
Fri May 14 22:58:25 CEST 2010
On Fri, May 14, 2010 at 09:27:46PM +0200, Stefan Reinauer wrote:
> Add support for the Getac P470
>
> Signed-off-by: Stefan Reinauer <stepan at coresystems.de>
Great, thanks!
Acked-by: Uwe Hermann <uwe at hermann-uwe.de>
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
More information about the coreboot
mailing list