patch: all superios in v3 are set up wrong

[ron minnich]

attached.

ron

[Peter Stuge]

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).

This explains why keyboard didn't work on my alix1c with v3.

Good find Ron!
> Signed-off-by: Ronald G. Minnich

Acked-by: Peter Stuge

[ron minnich]

I hope I got all the points that Uwe mentioned, but it was pretty
urgent to get this commit in.

Committed revision 888.

[Uwe Hermann]

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
> 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
> Copyright (C) 2008 Ronald G. Minnich

--- 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
> Copyright (C) 2007 Philipp Degler
> Copyright (C) 2008 Ronald G. Minnich

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
> Copyright (C) 2008 Ronald G. Minnich

(C) 2008 Ronald G. Minnich  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
> 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 
> +##
> +## 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.

[ron minnich]

I appreciate the very high quality and fast review.

The ite8712f is wrong I am sure. I have tried to fix this patch to
address the reviews.

Let me know what you think.

Thanks

ron

v3 quality

[Peter Stuge]

Uwe Hermann wrote:
> 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?

A quality problem, definately. But I think getting something to work
is important, once something is working we'll do cleanup before
adding lots of new ports.

//Peter

[Vincent Legoll]

On Fri, Oct 3, 2008 at 6:54 PM, Uwe Hermann  wrote:
> > kbirq = "1";
> > kbirq2 = "12";
>
> And this should probably be "2", not "12".

if kbirq2 is for the second ps2 port, i.e. ps2 mouse, maybe irq 12 is right.