Re: [LinuxBIOS] [PATCH]Fintek F71805f for LinuxBIOSv3

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