[LinuxBIOS] [PATCH]Fintek F71805f for LinuxBIOSv3

Corey Osgood corey.osgood at gmail.com
Mon Oct 29 03:00:51 CET 2007


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 <corey.osgood at gmail.com>
>>     
>  
> 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 <corey.osgood at gmail.com>
>> + *
>> + * 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 <corey at slightlyhackish.com>
>> + *
>> + * 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 <corey.osgood at gmail.com>
>> + *
>> + * 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




More information about the coreboot mailing list