[LinuxBIOS] [PATCH]Fintek F71805f for LinuxBIOSv3

[Corey Osgood]

See patch.

-Corey

[Peter Stuge]

On Sun, Oct 28, 2007 at 08:04:26PM -0400, Corey Osgood wrote:
> +void f71805f_pnp_enable(struct device *dev)
> +{
> pnp_enter_conf_state(dev);
> pnp_set_logical_device(dev);
> if(dev->enabled) {
> pnp_set_enable(dev, 1);
> }
> else {
> pnp_set_enable(dev, 0);
> }
> pnp_exit_conf_state(dev);
>
> +}

How about just pnp_set_enable(dev, dev->enabled) ?

Or !!dev->enabled if it needs to be 1 specifically.
> +static void f71805f_init(struct device *dev)
> +{
> struct superio_smsc_f71805f_config *conf = dev->chip_info;
> struct resource *res0, *res1;
> if (!dev->enabled)
> return;
> switch(dev->path.u.pnp.device) {

Since switch isn't a function I think the style is to have a space
before ( though I don't care much for all the spaces myself.
> case F71805F_SP1:
> res0 = find_resource(dev, PNP_IDX_IO0);
> //init_uart8250(res0->base, &conf->com1);

Why commented out? Can it be removed?
> /* COM1 */
> com1dev = "2";

Are the ports called COM1 and 2 in the data sheet? They're SP1/2
above. I don't particularly like the COMn names.
> +++ superio/fintek/f71805f/Makefile	(revision 0)
> @@ -0,0 +1,30 @@
> +##
> +## This file is part of the LinuxBIOS project.
> +##
> +## Copyright (C) 2007 coresystems GmbH
> +## (Written by Stefan Reinauer  for coresystems GmbH)

Did Stefan really write this?

//Peter

[Corey Osgood]

Peter Stuge wrote:
> On Sun, Oct 28, 2007 at 08:04:26PM -0400, Corey Osgood wrote:
> > +void f71805f_pnp_enable(struct device *dev)
> > +{
> > pnp_enter_conf_state(dev);
> > pnp_set_logical_device(dev);
> > if(dev->enabled) {
> > pnp_set_enable(dev, 1);
> > }
> > else {
> > pnp_set_enable(dev, 0);
> > }
> > pnp_exit_conf_state(dev);
> >
> > +}
>
> How about just pnp_set_enable(dev, dev->enabled) ?
>
> Or !!dev->enabled if it needs to be 1 specifically.

Ok.
> > +static void f71805f_init(struct device *dev)
> > +{
> > struct superio_smsc_f71805f_config *conf = dev->chip_info;
> > struct resource *res0, *res1;
> > if (!dev->enabled)
> > return;
> > switch(dev->path.u.pnp.device) {
>
> Since switch isn't a function I think the style is to have a space
> before ( though I don't care much for all the spaces myself.

Sounds right to me.
> > case F71805F_SP1:
> > res0 = find_resource(dev, PNP_IDX_IO0);
> > //init_uart8250(res0->base, &conf->com1);
>
> Why commented out? Can it be removed?

Good question, wish I could give you a good answer. It was commented in
the v2 code, and is also commented in the winbond. I think it's
something that should work in the future, but doesn't at the moment.
I've put in a comment to that effect.
> > /* COM1 */
> > com1dev = "2";
>
> Are the ports called COM1 and 2 in the data sheet? They're SP1/2
> above. I don't particularly like the COMn names.

OK. But does renaming them to spn break the dts? Guess we'll find out later.
> > +++ superio/fintek/f71805f/Makefile	(revision 0)
> > @@ -0,0 +1,30 @@
> > +##
> > +## This file is part of the LinuxBIOS project.
> > +##
> > +## Copyright (C) 2007 coresystems GmbH
> > +## (Written by Stefan Reinauer  for coresystems GmbH)
>
> Did Stefan really write this?

Nope, my bad, copy-paste error. Updated patch attached.

-Corey

[Peter Stuge]

On Sun, Oct 28, 2007 at 08:49:04PM -0400, Corey Osgood wrote:
> +++ superio/fintek/f71805f/Makefile	(revision 0)

..
> +STAGE0_CHIPSET_OBJ += $(obj)/superio/fintek/f71805f/stage1.o
> +STAGE0_CHIPSET_OBJ += $(obj)/device/pnp_raw.o
>
> +# Always add to variables, as there could be more than one Super I/O.
> +STAGE2_CHIPSET_OBJ += $(obj)/superio/fintek/f71805f/superio.o

The path seems redundant when it's the very directory the Makefile is
stored in. Can we get rid of the path within the Makefile somehow?

Maybe by setting a variable to the path of each Makefile, before it
is processed. Maybe Kconfig does this already?

//Peter

[Peter Stuge]

On Sun, Oct 28, 2007 at 08:49:04PM -0400, Corey Osgood wrote:
> +void f71805f_pnp_set_resources(struct device *dev);
> +void f71805f_pnp_set_resources(struct device *dev);

Twice?
> +static void pnp_enter_conf_state(struct device *dev);
> +static void pnp_exit_conf_state(struct device *dev);
>
> +static void pnp_enter_conf_state(struct device *dev) 
> +{
> outb(0x87, dev->path.u.pnp.port);
>
> +}
>
> +static void pnp_exit_conf_state(struct device *dev) 
> +{
> outb(0xaa, dev->path.u.pnp.port);
>
> +}

Are the declarations really needed when the definition is right after
them?
> switch (dev->path.u.pnp.device) {
> case F71805F_SP1:
> res0 = find_resource(dev, PNP_IDX_IO0);
> //TODO: needed? fix or remove?
> //init_uart8250(res0->base, &conf->sp1);
> break;
> case F71805F_SP2:
> res1 = find_resource(dev, PNP_IDX_IO0);
> //init_uart8250(res0->base, &conf->sp2);
> break;

PNP_IDX_IO0 in both cases?
Plus the commented SP2 call uses res0.
> +static struct device_operations ops;
> +static struct pnp_info pnp_dev_info[] = {
> { &ops, F71805F_SP1,  PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, },
> { &ops, F71805F_SP2,  PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, },
> /* TODO: Everything else */
>
> +};

You could combine the declaration and definition of ops.
> +static struct device_operations ops = {
> .phase2_setup_scan_bus   = phase2_setup_scan_bus,
> .phase4_read_resources   = pnp_read_resources,
> .phase4_set_resources    = f71805f_pnp_set_resources,
> .phase4_enable_disable   = f71805f_pnp_enable_resources,
> .phase5_enable_resources = f71805f_pnp_enable,

Seems these two have been mixed up.
> .phase6_init             = f71805f_init,

Using ops it's of course very easy to have generic pnp functions that
do what most of the chips want/need. We want that. :)

//Peter

[Stefan Reinauer]

* Corey Osgood  [071029 01:49]:
> Add support for the Fintek F71805f to LinuxBIOSv3. It hasn't been tested yet
> because something's failing to build elsewhere, but the stage1 does build, and
> for the moment that's what matters. It's based on working v2 code so it should
> work fine.
>
> Signed-off-by: Corey Osgood

I tried to follow the rest of the discussion.

I think that super-components like "fintek-all" are slightly overrated,
do not decrease the workload, but rather readability and can even be
dangerous -- think workarounds for chip revisions.

If someone happens to offer a super-component driver, we will have
something to discuss about. But keeping an outstanding patch unapplied
is definitely not an option.

r508.

[Uwe Hermann]

On Sun, Oct 28, 2007 at 08:04:26PM -0400, Corey Osgood wrote:
> Add support for the Fintek F71805f to LinuxBIOSv3. It hasn't been tested yet
> because something's failing to build elsewhere, but the stage1 does build, and
> for the moment that's what matters. It's based on working v2 code so it should
> work fine.
>
> Signed-off-by: Corey Osgood

Looks nice, but I'd prefer to make it a generic driver for most Fintek
chips while we're at it.

Please look at 'smscsuperio' in v2 (which supports 10-15 Super I/Os),
ideally this would be a similar generic Fintek driver code, which supports
most of the Fintek chips.

Please model the code in a similar way as in 'smscsuperio' and repost.

No need to add support for _all_ or multiple Fintek chips, but
please create the framework code in a way that other Fintek chips
can be added easily later. Ideally such an addition would only require
you to add the device IDs in a table, and the LDNs to another table.

Feel free to check superiotool for hints on detection of chips,
and how to keep them apart / probe for them etc.
> Index: Kconfig
>
> --- Kconfig	(revision 507)
> +++ Kconfig	(working copy)
> @@ -84,6 +84,8 @@
>  # Super I/Os:
>  config SUPERIO_WINBOND_W83627HF
>    boolean
> +config SUPERIO_FINTEK_F71805F
> boolean
>
> # Source all northbridge/southbridge/superio Kconfig files:
>  source northbridge/intel/i440bxemulation/Kconfig
> Index: superio/fintek/f71805f/stage1.c
> ===================================================================
> --- superio/fintek/f71805f/stage1.c	(revision 0)
> +++ superio/fintek/f71805f/stage1.c	(revision 0)
> @@ -0,0 +1,41 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright 2007 Corey Osgood
> 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 "f71805f.h"
>
> +static inline void f71805f_rawpnp_enter_ext_func_mode(u8 dev)
> +{
> /* Fintek F71805f needs this only once, but Winbond needs it twice.
> * Perhaps modify rawpnp_enter_ext_func_mode() to only do it once,
> * then modify the winbond to call it twice? */
> outb(0x87, dev);

If dev is the same as in v2 this won't work. dev is not just the config
port (e.g. 0x2e) but a struct which contains other stuff, I think.

Either way, I think you can safely apply 0x87 twice, even if it's only
needed once (see superiotool). Please test this on hardware, and if
it works (I'm pretty sure it will), just use the already existing
0x87 0x87 function.
> +}
>
> +void f71805f_enable_serial(u8 dev, u8 serial, u16 iobase)

enable_serial() only? Hardcoding the device name in function names was
an ugly workaround in v2, AFAIK. It's not really needed in v3 (?)

I'd rather like to see some common code which calls an _enable_serial
function which is "registered" to the framewrok via structs like we
use in most of the other code, e.g.

static void enable_serial(u8 dev, u8 serial, u16 iobase)
  {
    // ...
  }

static struct foo bar = {
          .init = &enable_serial,
  };

or something along those lines...
> +{
> f71805f_rawpnp_enter_ext_func_mode(dev);
> rawpnp_set_logical_device(dev, serial);
> rawpnp_set_enable(dev, 0);
> rawpnp_set_iobase(dev, PNP_IDX_IO0, iobase);
> rawpnp_set_enable(dev, 1);
> rawpnp_exit_ext_func_mode(dev);
>
> +}

Pretty generic, will be the same for 95% of all Super I/Os, so it
belongs in the generic Super I/O framework code (which might or might
not yet exist ;-), not here.
> Index: superio/fintek/f71805f/f71805f.h
>
> --- superio/fintek/f71805f/f71805f.h	(revision 0)
> +++ superio/fintek/f71805f/f71805f.h	(revision 0)
> @@ -0,0 +1,36 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Corey Osgood
> 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:
> Name: F71805F/FG Super H/W Monitor + LPC IO
> URL: http://www.fintek.com.tw/eng/products.asp?BID=1&SID=17
> PDF: http://www.fintek.com.tw/files/productfiles/F71805F_V025.pdf
> Revision: V0.25P
> */

Yep, but should be moved to some other file, because...
> +/* Logical Device Numbers (LDN). */
> +#define	F71805F_FDC	0x00	/* Floppy */
> +#define	F71805F_SP1	0x01	/* UART1 */
> +#define	F71805F_SP2	0x02	/* UART2 */
> +#define	F71805F_PP	0x03	/* Parallel Port */
> +#define	F71805F_HWM	0x04	/* Hardware Monitor */
> +#define	F71805F_GPIO	0x06	/* General Purpose I/O (GPIO) */
> +#define	F71805F_PME	0x0a	/* Power Management Events (PME) */

...NAK. No such files in v3 please. Kill this file completely.

The LDNs are listed in the dts anyway, and probably also in the
superio.c file (if modeled similar as the 'smscsuperio' code in v2).
> Index: superio/fintek/f71805f/superio.c
>
> --- superio/fintek/f71805f/superio.c	(revision 0)
> +++ superio/fintek/f71805f/superio.c	(revision 0)
> @@ -0,0 +1,122 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright 2007 Corey Osgood
> 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
> */
>
> +/* Ported from v2 driver */

Drop this, noone cares. Could be in the commit message, but not here.
> +#include <io.h>
> +#include <lib.h>
> +#include <device/device.h>
> +#include <device/pnp.h>
> +#include <console.h>
> +#include <string.h>
> +#include <uart8250.h>
> +#include <statictree.h>
> +#include "f71805f.h"
> +static void enable_dev(struct device *dev);
> +void f71805f_pnp_set_resources(struct device *dev);
> +void f71805f_pnp_set_resources(struct device *dev);
> +void f71805f_pnp_enable_resources(struct device *dev);
> +void f71805f_pnp_enable(struct device *dev);
> +static void f71805f_init(struct device *dev);
>
> +static void pnp_enter_conf_state(struct device *dev);
> +static void pnp_exit_conf_state(struct device *dev);

Why? Needed?
> +static void pnp_enter_conf_state(struct device *dev) 
> +{
> outb(0x87, dev->path.u.pnp.port);
>
> +}
>
> +static void pnp_exit_conf_state(struct device *dev) 
> +{
> outb(0xaa, dev->path.u.pnp.port);
>
> +}
>
> +void f71805f_pnp_set_resources(struct device *dev)
> +{
> pnp_enter_conf_state(dev);
> pnp_set_resources(dev);
> pnp_exit_conf_state(dev);
>
> +}
>
> +void f71805f_pnp_enable_resources(struct device *dev)
> +{
> pnp_enter_conf_state(dev);
> pnp_enable_resources(dev);
> pnp_exit_conf_state(dev);
>
> +}
>
> +void f71805f_pnp_enable(struct device *dev)
> +{
> pnp_enter_conf_state(dev);
> pnp_set_logical_device(dev);
> if(dev->enabled) {
> pnp_set_enable(dev, 1);
> }
> else {
> pnp_set_enable(dev, 0);
> }
> pnp_exit_conf_state(dev);
>
> +}

Nope, completely generic. This part should go into the "framework" (to
be defined, might not yet exist completely).

IMO defining a new Super I/O should look something like this:

static struct foo bar = {
          .enter = &enter_conf_state,
          .exit = &exit_conf_state,
  };

Those are mostly the specific parts, everything else above is generic and
applies to 95% of the Super I/Os.

Maybe a file lib/superio.c or something which contains the common
framework? Or superio/framework/*? Dunno...
> +static void f71805f_init(struct device *dev)
> +{
> struct superio_smsc_f71805f_config *conf = dev->chip_info;
> struct resource *res0, *res1;
> if (!dev->enabled)
> return;
> switch(dev->path.u.pnp.device) {
> case F71805F_SP1:
> res0 = find_resource(dev, PNP_IDX_IO0);
> //init_uart8250(res0->base, &conf->com1);
> break;
> case F71805F_SP2:
> res1 = find_resource(dev, PNP_IDX_IO0);
> //init_uart8250(res0->base, &conf->com2);
> break;
> /* No KBC on F71805f */
> }
>
> +}
>
> +static struct device_operations ops;
> +static struct pnp_info pnp_dev_info[] = {
> { &ops, F71805F_SP1,  PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, },
> { &ops, F71805F_SP2,  PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, },
> /* TODO: Everything else */
>
> +};
>
> +static void phase2_setup_scan_bus(struct device *dev)
> +{
> pnp_enable_devices(dev, &ops, ARRAY_SIZE(pnp_dev_info), pnp_dev_info);
>
> +}
>
> +static struct device_operations ops = {
> .phase2_setup_scan_bus   = phase2_setup_scan_bus,
> .phase4_read_resources   = pnp_read_resources,
> .phase4_set_resources    = f71805f_pnp_set_resources,
> .phase4_enable_disable   = f71805f_pnp_enable_resources,
> .phase5_enable_resources = f71805f_pnp_enable,
> .phase6_init             = f71805f_init,
>
> +};
> Index: superio/fintek/f71805f/dts
> ===================================================================
> --- superio/fintek/f71805f/dts	(revision 0)
> +++ superio/fintek/f71805f/dts	(revision 0)
> @@ -0,0 +1,51 @@
> +/*
> This file is part of the LinuxBIOS project.
> Copyright (C) 2007 Corey Osgood
> 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
> */
>
> +{
> /* Floppy */
> floppydev = "0x0";
> floppyenable = "0";
> floppyio = "0x3f0";
> floppyirq = "6";
> floppydrq = "2";
> /* Parallel port */
> ppdev = "1";
> ppenable = "1";
> ppio = "0x378";
> ppirq = "7";
> /* COM1 */
> com1dev = "2";
> com1enable = "1";
> com1io = "0x3f8";
> com1irq = "4";
> /* COM2 */
> com2dev = "3";
> com2enable = "1";
> com2io = "0x2f8";
> com2irq = "3";
> /* Hardware Monitor */
> hwmdev = "0xb";
> hwmenable = "0";
> hwmio = "0xec00";
>
> +};

Yep, this is great stuff. If the majority of the v3 "code" looks like this,
we have won. Generic framework for handling code, all the chip-specific
stuff is encoded in a pure datafile (dts).
> Index: superio/fintek/f71805f/Makefile
>
> --- superio/fintek/f71805f/Makefile	(revision 0)
> +++ superio/fintek/f71805f/Makefile	(revision 0)
> @@ -0,0 +1,30 @@
> +##
> +## This file is part of the LinuxBIOS project.
> +##
> +## Copyright (C) 2007 coresystems GmbH
> +## (Written by Stefan Reinauer  for 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; 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_FINTEK_F71805F),y)
>
> +STAGE0_CHIPSET_OBJ += $(obj)/superio/fintek/f71805f/stage1.o
> +STAGE0_CHIPSET_OBJ += $(obj)/device/pnp_raw.o
>
> +# Always add to variables, as there could be more than one Super I/O.
> +STAGE2_CHIPSET_OBJ += $(obj)/superio/fintek/f71805f/superio.o
>
> +endif

OK for now, but I think we can obsolete 90% of these almost-empty
Makefiles later (we already did away with quite a bunch of them,
which is good).

Uwe.

[Corey Osgood]

Uwe Hermann wrote:
> On Sun, Oct 28, 2007 at 08:04:26PM -0400, Corey Osgood wrote:
> > Add support for the Fintek F71805f to LinuxBIOSv3. It hasn't been tested yet
> > because something's failing to build elsewhere, but the stage1 does build, and
> > for the moment that's what matters. It's based on working v2 code so it should
> > work fine.
> >
> > Signed-off-by: Corey Osgood
>
> Looks nice, but I'd prefer to make it a generic driver for most Fintek
> chips while we're at it.
>
> Please look at 'smscsuperio' in v2 (which supports 10-15 Super I/Os),
> ideally this would be a similar generic Fintek driver code, which supports
> most of the Fintek chips.
>
> Please model the code in a similar way as in 'smscsuperio' and repost.
>
> No need to add support for _all_ or multiple Fintek chips, but
> please create the framework code in a way that other Fintek chips
> can be added easily later. Ideally such an addition would only require
> you to add the device IDs in a table, and the LDNs to another table.
>
> Feel free to check superiotool for hints on detection of chips,
> and how to keep them apart / probe for them etc.

Then forget the patch. I'll work on that once the rest of the hardware
is working. Getting ram init working is about a million times more
important to me than getting the rest of the fintek lineup working. Not
to mention that I have no other fintek chips, so no way to test.

-Corey

[Corey Osgood]

Uwe Hermann wrote:
> On Sun, Oct 28, 2007 at 08:04:26PM -0400, Corey Osgood wrote:
> > Add support for the Fintek F71805f to LinuxBIOSv3. It hasn't been tested yet
> > because something's failing to build elsewhere, but the stage1 does build, and
> > for the moment that's what matters. It's based on working v2 code so it should
> > work fine.
> >
> > Signed-off-by: Corey Osgood
>
> Looks nice, but I'd prefer to make it a generic driver for most Fintek
> chips while we're at it.
>
> Please look at 'smscsuperio' in v2 (which supports 10-15 Super I/Os),
> ideally this would be a similar generic Fintek driver code, which supports
> most of the Fintek chips.
>
> Please model the code in a similar way as in 'smscsuperio' and repost.
>
> No need to add support for _all_ or multiple Fintek chips, but
> please create the framework code in a way that other Fintek chips
> can be added easily later. Ideally such an addition would only require
> you to add the device IDs in a table, and the LDNs to another table.
>
> Feel free to check superiotool for hints on detection of chips,
> and how to keep them apart / probe for them etc.

Ok, sorry for the small outburst, but in truth I am focusing on trying
to learn the dts and how to make the best use of it, and also how to
port from v2 using it. So my primary goal for the moment is to get the
rest of the board working, it can be made generic later. More comments
inline below
> > Index: Kconfig
> >
> > --- Kconfig	(revision 507)
> > +++ Kconfig	(working copy)
> > @@ -84,6 +84,8 @@
> >  # Super I/Os:
> >  config SUPERIO_WINBOND_W83627HF
> >    boolean
> > +config SUPERIO_FINTEK_F71805F
> > boolean
> >
> > # Source all northbridge/southbridge/superio Kconfig files:
> >  source northbridge/intel/i440bxemulation/Kconfig
> > Index: superio/fintek/f71805f/stage1.c
> > ===================================================================
> > --- superio/fintek/f71805f/stage1.c	(revision 0)
> > +++ superio/fintek/f71805f/stage1.c	(revision 0)
> > @@ -0,0 +1,41 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright 2007 Corey Osgood
> > 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 "f71805f.h"
> >
> > +static inline void f71805f_rawpnp_enter_ext_func_mode(u8 dev)
> > +{
> > /* Fintek F71805f needs this only once, but Winbond needs it twice.
> > * Perhaps modify rawpnp_enter_ext_func_mode() to only do it once,
> > * then modify the winbond to call it twice? */
> > outb(0x87, dev);
>
> If dev is the same as in v2 this won't work. dev is not just the config
> port (e.g. 0x2e) but a struct which contains other stuff, I think.

dev is just the port in this case, it's not like v2.
> Either way, I think you can safely apply 0x87 twice, even if it's only
> needed once (see superiotool). Please test this on hardware, and if
> it works (I'm pretty sure it will), just use the already existing
> 0x87 0x87 function.

I would much, MUCH rather apply it once and have it succeed then do it
twice and have it fail, especially if by that point it slips my mind why
it might have failed. I'll test it out on hardware once the rest of the
code builds (and if I don't, poke me a reminder).
> > +}
> >
> > +void f71805f_enable_serial(u8 dev, u8 serial, u16 iobase)
>
> enable_serial() only? Hardcoding the device name in function names was
> an ugly workaround in v2, AFAIK. It's not really needed in v3 (?)

Hmm, very true, but we also have to remember the case of multiple
superios, where one might be connected and the other not, such as my
pcchips board with the vt8235 (integrated superio) and a separate
superio. Even though we don't care about the other one, we still have to
handle it, since it is there for some purpose, be it sensors or southbridge.
> I'd rather like to see some common code which calls an _enable_serial
> function which is "registered" to the framewrok via structs like we
> use in most of the other code, e.g.
>
> static void enable_serial(u8 dev, u8 serial, u16 iobase)
>   {
>    // ...
>   }
>
> static struct foo bar = {
>           .init = &enable_serial,
>   };
>
> or something along those lines...

Good suggestion. Perhaps super ios should work something like pci
devices, where if no .xxx is explicitly provided, a default routine is
used. Patches are always welcome ;)
> > +{
> > f71805f_rawpnp_enter_ext_func_mode(dev);
> > rawpnp_set_logical_device(dev, serial);
> > rawpnp_set_enable(dev, 0);
> > rawpnp_set_iobase(dev, PNP_IDX_IO0, iobase);
> > rawpnp_set_enable(dev, 1);
> > rawpnp_exit_ext_func_mode(dev);
> >
> > +}
>
> Pretty generic, will be the same for 95% of all Super I/Os, so it
> belongs in the generic Super I/O framework code (which might or might
> not yet exist ;-), not here.

Good point, I'll work on it later, or again patches welcome.
> > Index: superio/fintek/f71805f/f71805f.h
> >
> > --- superio/fintek/f71805f/f71805f.h	(revision 0)
> > +++ superio/fintek/f71805f/f71805f.h	(revision 0)
> > @@ -0,0 +1,36 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright (C) 2007 Corey Osgood
> > 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:
> > Name: F71805F/FG Super H/W Monitor + LPC IO
> > URL: http://www.fintek.com.tw/eng/products.asp?BID=1&SID=17
> > PDF: http://www.fintek.com.tw/files/productfiles/F71805F_V025.pdf
> > Revision: V0.25P
> > */
>
> Yep, but should be moved to some other file, because...
> > +/* Logical Device Numbers (LDN). */
> > +#define	F71805F_FDC	0x00	/* Floppy */
> > +#define	F71805F_SP1	0x01	/* UART1 */
> > +#define	F71805F_SP2	0x02	/* UART2 */
> > +#define	F71805F_PP	0x03	/* Parallel Port */
> > +#define	F71805F_HWM	0x04	/* Hardware Monitor */
> > +#define	F71805F_GPIO	0x06	/* General Purpose I/O (GPIO) */
> > +#define	F71805F_PME	0x0a	/* Power Management Events (PME) */
>
> ...NAK. No such files in v3 please. Kill this file completely.
>
> The LDNs are listed in the dts anyway, and probably also in the
> superio.c file (if modeled similar as the 'smscsuperio' code in v2).

Just have to figure out how to USE them from the dts, and we'll be golden ;)
> > Index: superio/fintek/f71805f/superio.c
> >
> > --- superio/fintek/f71805f/superio.c	(revision 0)
> > +++ superio/fintek/f71805f/superio.c	(revision 0)
> > @@ -0,0 +1,122 @@
> > +/*
> > This file is part of the LinuxBIOS project.
> > Copyright 2007 Corey Osgood
> > 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
> > */
> >
> > +/* Ported from v2 driver */
>
> Drop this, noone cares. Could be in the commit message, but not here.

Good point. It was supposed to be a reminder for me to put in a commit
message, at a later date.
> > +static void enable_dev(struct device *dev);
> > +void f71805f_pnp_set_resources(struct device *dev);
> > +void f71805f_pnp_set_resources(struct device *dev);
> > +void f71805f_pnp_enable_resources(struct device *dev);
> > +void f71805f_pnp_enable(struct device *dev);
> > +static void f71805f_init(struct device *dev);
> >
> > +static void pnp_enter_conf_state(struct device *dev);
> > +static void pnp_exit_conf_state(struct device *dev);
>
> Why? Needed?

Not really, but clears up a lot of gcc warnings about implicit function
declarations. Should go into a header though.
> > +static void pnp_enter_conf_state(struct device *dev) 
> > +{
> > outb(0x87, dev->path.u.pnp.port);
> >
> > +}
> >
> > +static void pnp_exit_conf_state(struct device *dev) 
> > +{
> > outb(0xaa, dev->path.u.pnp.port);
> >
> > +}
> >
> > +void f71805f_pnp_set_resources(struct device *dev)
> > +{
> > pnp_enter_conf_state(dev);
> > pnp_set_resources(dev);
> > pnp_exit_conf_state(dev);
> >
> > +}
> >
> > +void f71805f_pnp_enable_resources(struct device *dev)
> > +{
> > pnp_enter_conf_state(dev);
> > pnp_enable_resources(dev);
> > pnp_exit_conf_state(dev);
> >
> > +}
> >
> > +void f71805f_pnp_enable(struct device *dev)
> > +{
> > pnp_enter_conf_state(dev);
> > pnp_set_logical_device(dev);
> > if(dev->enabled) {
> > pnp_set_enable(dev, 1);
> > }
> > else {
> > pnp_set_enable(dev, 0);
> > }
> > pnp_exit_conf_state(dev);
> >
> > +}
>
> Nope, completely generic. This part should go into the "framework" (to
> be defined, might not yet exist completely).
>
> IMO defining a new Super I/O should look something like this:
>
> static struct foo bar = {
>           .enter = &enter_conf_state,
>           .exit = &exit_conf_state,
>   };
>
> Those are mostly the specific parts, everything else above is generic and
> applies to 95% of the Super I/Os.
>
> Maybe a file lib/superio.c or something which contains the common
> framework? Or superio/framework/*? Dunno...

Again, I think we can reuse some parts of the pci device framework. I'll
toy with it once I can get the rest of ram init building/working, if no
one else does.

-Corey

[Peter Stuge]

On Sun, Oct 28, 2007 at 10:00:51PM -0400, Corey Osgood wrote:
> Ok, sorry for the small outburst,

Don't worry.
> but in truth I am focusing on trying to learn the dts and how to
> make the best use of it, and also how to port from v2 using it.

Yep.
> So my primary goal for the moment is to get the rest of the board
> working, it can be made generic later.

I agree. First get it working, then get fancy. :)
> Just have to figure out how to USE them from the dts, and we'll be
> golden ;)

I think I've asked about this before. There should be code to do it.

//Peter

[Uwe Hermann]

On Sun, Oct 28, 2007 at 10:00:51PM -0400, Corey Osgood wrote:
> > If dev is the same as in v2 this won't work. dev is not just the config
> > port (e.g. 0x2e) but a struct which contains other stuff, I think.
>
> dev is just the port in this case, it's not like v2.

OK, we should rename the variable 'port' then maybe. It's confusing
otherwise.
> > Either way, I think you can safely apply 0x87 twice, even if it's only
> > needed once (see superiotool). Please test this on hardware, and if
> > it works (I'm pretty sure it will), just use the already existing
> > 0x87 0x87 function.
>
> I would much, MUCH rather apply it once and have it succeed then do it
> twice and have it fail, especially if by that point it slips my mind why
> it might have failed. I'll test it out on hardware once the rest of the
> code builds (and if I don't, poke me a reminder).

Sure, the safe way is to do it only once. FWIW, with some other Super
I/Os I have tested that applying the sequence twice is non-critical
(forgetting the exit sequence _does_ cause problems though!)
> > enable_serial() only? Hardcoding the device name in function names was
> > an ugly workaround in v2, AFAIK. It's not really needed in v3 (?)
>
> Hmm, very true, but we also have to remember the case of multiple
> superios, where one might be connected and the other not, such as my

Yeah.
> pcchips board with the vt8235 (integrated superio) and a separate
> superio. Even though we don't care about the other one, we still have to
> handle it, since it is there for some purpose, be it sensors or southbridge.

Maybe we should have a _list_ of Super I/O init functions which are
called? That way we could even handle 3 or 4 Super I/Os...
> > I'd rather like to see some common code which calls an _enable_serial
> > function which is "registered" to the framewrok via structs like we
> > use in most of the other code, e.g.
> >
> > static void enable_serial(u8 dev, u8 serial, u16 iobase)
> >   {
> >    // ...
> >   }
> >
> > static struct foo bar = {
> >           .init = &enable_serial,
> >   };
> >
> > or something along those lines...
>
> Good suggestion. Perhaps super ios should work something like pci
> devices, where if no .xxx is explicitly provided, a default routine is
> used.

Yeah, that would be perfect!
> > The LDNs are listed in the dts anyway, and probably also in the
> > superio.c file (if modeled similar as the 'smscsuperio' code in v2).
>
> Just have to figure out how to USE them from the dts, and we'll be golden ;)

Yeah, that's a bit of a problem. Haven't tried if it's possible, yet.
> > Maybe a file lib/superio.c or something which contains the common
> > framework? Or superio/framework/*? Dunno...
>
> Again, I think we can reuse some parts of the pci device framework. I'll
> toy with it once I can get the rest of ram init building/working, if no
> one else does.

I'll see what I can come up with. Expect a patch Soon (tm).

Uwe.