See patch.
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@coresystems.de
Great, thanks!
Acked-by: Uwe Hermann uwe@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.