Re: [coreboot] [PATCH] Getac P470 notebook support

[Uwe Hermann]

On Fri, May 14, 2010 at 09:27:46PM +0200, Stefan Reinauer wrote:
> Add support for the Getac P470
>
> Signed-off-by: Stefan Reinauer

Great, thanks!

Acked-by: Uwe Hermann

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.