[coreboot] patch: all superios in v3 are set up wrong

Uwe Hermann uwe at hermann-uwe.de
Fri Oct 3 18:54:08 CEST 2008


On Fri, Oct 03, 2008 at 09:15:37AM -0700, ron minnich wrote:
> Add the it8712f. 
> 
> Also, EVERY superio in v3 was set up wrong. The dts has to point to device
> operations (if there are any). 
> 
> Build tested on amd serengeti. 
> 
> Signed-off-by: Ronald G. Minnich <rminnich at gmail.com>
 

> Index: superio/ite/it8712f/stage1.c
> ===================================================================
> --- superio/ite/it8712f/stage1.c	(revision 0)
> +++ superio/ite/it8712f/stage1.c	(revision 0)
> @@ -0,0 +1,115 @@
> +/*
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2006 Uwe Hermann <uwe at hermann-uwe.de>
> + * Copyright (C) 2008 Ronald G. Minnich <rminnich at gmail.com>
--- Insert an empty line here, please (as in the other files) ----
> + * 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 <io.h>
> +#include <device/pnp.h>
> +#include "it8712f.h"
> +
> +/* The base address is 0x2e or 0x4e, depending on config bytes. */
> +#define SIO_BASE                     0x2e
> +#define SIO_INDEX                    SIO_BASE
> +#define SIO_DATA                     SIO_BASE+1

FYI, the IT8712F code is (like many other superios in v2) quite a mess right
now, I'll send patches to fix various issues...


> Index: superio/ite/it8712f/superio.c
> ===================================================================
> --- superio/ite/it8712f/superio.c	(revision 0)
> +++ superio/ite/it8712f/superio.c	(revision 0)
> @@ -0,0 +1,144 @@
> +/*
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2006 Uwe Hermann <uwe at hermann-uwe.de>
> + * Copyright (C) 2007 Philipp Degler <pdegler at rumms.uni-mannheim.de>

> + * Copyright (C) 2008 Ronald G. Minnich <rminnich at gmail.com>

Don't add the (C) this time I'd say, the changes are too small to
warrant adding it (IMHO).


> +static void it8712f_pnp_set_resources(struct device * dev)
> +{
> +	pnp_enter_ext_func_mode(dev);
> +	pnp_set_resources(dev);
> +	pnp_exit_ext_func_mode(dev);
> +}
> +
> +static void it8712f_pnp_enable_resources(struct device * dev)
> +{
> +	pnp_enter_ext_func_mode(dev);
> +	pnp_enable_resources(dev);
> +	pnp_exit_ext_func_mode(dev);
> +}
> +
> +static void it8712f_pnp_enable(struct device * dev)
> +{
> +	pnp_enter_ext_func_mode(dev);
> +	pnp_set_logical_device(dev);
> +	pnp_set_enable(dev, dev->enabled);
> +	pnp_exit_ext_func_mode(dev);
> +}
> +
> +static void it8712f_setup_scan_bus(struct device *dev)
> +{
> +	pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info);

I'm not sure about this -- what is 'ops'? Should it be it8712f_ops
here? I don't see any 'ops' being defined for this superio.

The IT8716F does this in v3:

  static struct device_operations ops;

right before the

static struct pnp_info pnp_dev_info[] = {
[...]


> +}
> +
> +struct device_operations it8712f_ops = {
> +	.phase2_setup_scan_bus	 = it8712f_setup_scan_bus,
> +	.phase4_read_resources	 = pnp_read_resources,
> +	.phase4_set_resources	 = it8712f_pnp_set_resources,
> +	.phase4_enable_disable	 = it8712f_pnp_enable_resources,
> +	.phase5_enable_resources = it8712f_pnp_enable,
> +	.phase6_init		 = it8712f_init,
> +};
> +
> +/* TODO: FDC, MIDI, GAME, IR. */
> +static struct pnp_info pnp_dev_info[] = {
> +	{&ops, IT8712F_SP1, PNP_IO0 | PNP_IRQ0, {0x7f8, 0}, },
> +	{&ops, IT8712F_SP2, PNP_IO0 | PNP_IRQ0 | PNP_DRQ0 | PNP_DRQ1, {0x7f8, 0}, },
> +	{&ops, IT8712F_PP, PNP_IO0 | PNP_IRQ0 | PNP_DRQ0, {0x07f8, 0},},
> +	{&ops, IT8712F_EC, PNP_IO0 | PNP_IO1 | PNP_IRQ0, {0x7f8, 0}, {0x7f8, 0x4},},
> +	{&ops, IT8712F_KBCK, PNP_IO0 | PNP_IO1 | PNP_IRQ0, {0x7f8, 0}, {0x7f8, 0x4}, },
> +	{&ops, IT8712F_KBCM, PNP_IRQ0, },
> +	{&ops, IT8712F_GPIO, },
> +};
> +
> +
> Index: superio/ite/it8712f/dts
> ===================================================================
> --- superio/ite/it8712f/dts	(revision 0)
> +++ superio/ite/it8712f/dts	(revision 0)
> @@ -0,0 +1,43 @@
> +/*
> + *
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2008 Peter Stuge <peter at stuge.se>
> + * Copyright (C) 2008 Ronald G. Minnich <rminnich at gmail.com>

(C) 2008 Ronald G. Minnich <rminnich at gmail.com> should be enough, this
is way too trivial to carry around other (C)'s...


> + * 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
> + *
> + */
> +{
> +	device_operations = "it8712f_ops";
> +	/* COM1 */
> +	com1dev = "1";
> +	com1enable = "0";
> +	com1io = "0x3f8";
> +	com1irq = "4";
> +
> +	/* COM2 */
> +	com2dev = "2";
> +	com2enable = "0";
> +	com2io = "0x2f8";
> +	com2irq = "3";
> +
> +	/* Keyboard */
> +	kbdev = "5";
> +	kbenable = "0";
> +	kbio = "0x60";

> +	kbio2 = "0x62";

I think this should be 0x64.


> +	kbirq = "1";

> +	kbirq2 = "12";

And this should probably be "2", not "12".


Various devices are missing, I can send a patch later to fill them in.
Does is pose a problem if they're not all listed in the dts?


> +};
> Index: superio/ite/it8712f/it8712f.h
> ===================================================================
> --- superio/ite/it8712f/it8712f.h	(revision 0)
> +++ superio/ite/it8712f/it8712f.h	(revision 0)
> @@ -0,0 +1,35 @@
> +/*
> + * This file is part of the coreboot project.
> + *
> + * Copyright (C) 2006 Uwe Hermann <uwe at hermann-uwe.de>
> + *
> + * 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
> + */
> +
> +/* Datasheet: http://www.ite.com.tw/product_info/PC/Brief-IT8712_2.asp */
> +/* Status: Com1 is tested and works. */
> +
> +#define IT8712F_FDC  0x00 /* Floppy */
> +#define IT8712F_SP1  0x01 /* Com1 */
> +#define IT8712F_SP2  0x02 /* Com2 */
> +#define IT8712F_PP   0x03 /* Parallel port */
> +#define IT8712F_EC   0x04 /* Environment controller */
> +#define IT8712F_KBCK 0x05 /* Keyboard */
> +#define IT8712F_KBCM 0x06 /* Mouse */
> +#define IT8712F_GPIO 0x07 /* GPIO */
> +#define IT8712F_MIDI 0x08 /* MIDI port */
> +#define IT8712F_GAME 0x09 /* GAME port */
> +#define IT8712F_IR   0x0a /* Consumer IR */
> +
> Index: superio/ite/it8712f/Makefile
> ===================================================================
> --- superio/ite/it8712f/Makefile	(revision 0)
> +++ superio/ite/it8712f/Makefile	(revision 0)
> @@ -0,0 +1,29 @@
> +##
> +## This file is part of the coreboot project.
> +##
> +## Copyright (C) 2008 Peter Stuge <peter at stuge.se>
> +##
> +## 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
> +##
> +
> +ifeq ($(CONFIG_SUPERIO_ITE_IT8712F),y)
> +
> +STAGE0_CHIPSET_SRC += $(src)/superio/ite/it8712f/stage1.c
> +STAGE0_CHIPSET_SRC += $(src)/device/pnp_raw.c
> +
> +# Always add to variables, as there could be more than one Super I/O.
> +STAGE2_CHIPSET_SRC += $(src)/superio/ite/it8712f/superio.c
> +
> +endif
> Index: superio/ite/it8716f/superio.c
> ===================================================================
> --- superio/ite/it8716f/superio.c	(revision 884)
> +++ superio/ite/it8716f/superio.c	(working copy)
> @@ -164,7 +164,7 @@
>  	}
>  }
>  
> -static struct device_operations ops = {
> +struct device_operations it8716f_ops = {
>  	.phase2_setup_scan_bus	 = it8716f_setup_scan_bus,
>  	.phase4_read_resources	 = pnp_read_resources,
>  	.phase4_set_resources	 = it8716f_pnp_set_resources,
> Index: superio/ite/it8716f/dts
> ===================================================================
> --- superio/ite/it8716f/dts	(revision 884)
> +++ superio/ite/it8716f/dts	(working copy)
> @@ -20,6 +20,7 @@
>   *
>   */
>  {
> +	device_operations = "it8716f_ops";
>  	/* Floppy */
>  	floppydev = "0x0";
>  	floppyenable = "0";


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org




More information about the coreboot mailing list