Re: [coreboot] Super IO: Winbond W83697HF Support

[Uwe Hermann]

Hi,

thanks for you patch!

On Thu, Jul 17, 2008 at 07:51:56PM -0700, Sean Nelson wrote:
> Here's a patch for coreboot v2 for Winbond W83697HF Support.
>
> Patch Attached.

Please add a Signed-off-by to every patch, otherwise we cannot commit,
see http://www.coreboot.org/Development_Guidelines#Sign-off_Procedure.

Quick review below.
> Index: src/superio/winbond/w83697hf/w83697hf_early_init.c
>
> --- src/superio/winbond/w83697hf/w83697hf_early_init.c	(revision 0)
> +++ src/superio/winbond/w83697hf/w83697hf_early_init.c	(revision 0)
> @@ -0,0 +1,35 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Sean Nelson
> 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 <arch/romcc_io.h>
> +#include "w83697hf.h"
>
> +static void w83697hf_disable_dev(device_t dev)
> +{
> pnp_set_logical_device(dev);
> pnp_set_enable(dev, 0);
>
> +}
> +static void w83697hf_enable_dev(device_t dev, unsigned iobase)
> +{
> pnp_set_logical_device(dev);
> pnp_set_enable(dev, 0);
> pnp_set_iobase(dev, PNP_IDX_IO0, iobase);
> pnp_set_enable(dev, 1);
>
> +}

This file will probably not be required in the usual case. I'd drop
it for now unless you explicitly need it or use it in practice.
> Index: src/superio/winbond/w83697hf/superio.c
>
> --- src/superio/winbond/w83697hf/superio.c	(revision 0)
> +++ src/superio/winbond/w83697hf/superio.c	(revision 0)
> @@ -0,0 +1,205 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Sean Nelson
> 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 <arch/io.h>
> +#include <device/device.h>
> +#include <device/pnp.h>
> +#include <console/console.h>
> +#include <string.h>
> +#include <bitops.h>
> +#include <uart8250.h>
> +#include <pc80/keyboard.h>

See above, keyboard.h is likely not needed.
> +#include <pc80/mc146818rtc.h>
> +#include "chip.h"
> +#include "w83697hf.h"
>
> +static void pnp_enter_ext_func_mode(device_t dev) 
> +{
>    outb(0x87, dev->path.u.pnp.port);
>    outb(0x87, dev->path.u.pnp.port);
>
> +}
> +static void pnp_exit_ext_func_mode(device_t dev) 
> +{
>    outb(0xaa, dev->path.u.pnp.port);
>
> +}
> +static void pnp_write_index(unsigned long port_base, uint8_t reg, uint8_t value)
> +{
>    outb(reg, port_base);
>    outb(value, port_base + 1);
>
> +}
>
> +static uint8_t pnp_read_index(unsigned long port_base, uint8_t reg)
> +{
>    outb(reg, port_base);
>    return inb(port_base + 1);
>
> +}

These two are likely not needed if you drop the HWM code (see below).
> +static void enable_hwm_smbus(device_t dev) {
> /* set the pin 91,92 as I2C bus */
> uint8_t reg, value;
> reg = 0x2b;
> value = pnp_read_config(dev, reg);
> value &= 0x3f;
> pnp_write_config(dev, reg, value);
>
> +}

This will not work on this Super I/O, there's no 0x2b register.
I suggest to drop HWM init completely for now and/or (if needed)
fix this with another patch when it is tested on hardware...
> +static void init_acpi(device_t dev)
> +{
> uint8_t  value = 0x20;
> int power_on = 1;
> get_option(&power_on, "power_on_after_fail");
> pnp_enter_ext_func_mode(dev);
> pnp_write_index(dev->path.u.pnp.port,7,0x0a);
> value = pnp_read_config(dev, 0xE4);
> value &= ~(3<<5);
> if(power_on) {
> value |= (1<<5);
> }
> pnp_write_config(dev, 0xE4, value);
>    pnp_exit_ext_func_mode(dev);  
>
> +}

Can also be dropped, there's no such feature on this Super I/O it seems,
and there's no 0xe4 register on LDN 0xa (ACPI).
> +static void init_hwm(unsigned long base)
> +{
> uint8_t  reg, value;
> int i;
> unsigned  hwm_reg_values[] = {
>
> +/*	      reg  mask  data */
>          0x40, 0xff, 0x81,  /* start HWM */
>          //0x48, 0xaa, 0x2a,  /* set SMBus base to 0x54>>1	*/
>          //0x4a, 0x21, 0x21,  /* set T2 SMBus base to 0x92>>1 and T3 SMBus base to 0x94>>1 */
>          0x4e, 0x80, 0x00,  
>          0x43, 0x00, 0xfd,  /* disable some SMI interrupts */
>          0x44, 0x00, 0x17,  /* disable more SMI interrupts */
>          0x4c, 0xbf, 0x04
>          //0x4d, 0xff, 0x80   /* turn off beep */
> };
> for(i = 0; i<  sizeof(hwm_reg_values)/sizeof(hwm_reg_values[0]); i+=3 ) {
> reg = hwm_reg_values[i];	
> 	value = pnp_read_index(base, reg);		
> value &= 0xff & hwm_reg_values[i+1];
> value |= 0xff & hwm_reg_values[i+2];
>
> +#if 0
> printk_debug("base = 0x%04x, reg = 0x%02x, value = 0x%02x\r\n", base, reg,value);
>
> +#endif
> pnp_write_index(base, reg, value);
> }
>
> +}

Drop this, see above.
> +static void w83697hf_init(device_t dev)
> +{
> struct superio_winbond_w83697hf_config *conf;
> struct resource *res0, *res1;
> if (!dev->enabled) {
> return;
> }
> conf = dev->chip_info;
> switch(dev->path.u.pnp.device) {
> case W83697HF_SP1:
> res0 = find_resource(dev, PNP_IDX_IO0);
> init_uart8250(res0->base, &conf->com1);
> break;
> case W83697HF_SP2:
> res0 = find_resource(dev, PNP_IDX_IO0);
> init_uart8250(res0->base, &conf->com2);
> break;

This is ok...
> case W83697HF_HWM:
> res0 = find_resource(dev, PNP_IDX_IO0);
>
> +#define HWM_INDEX_PORT 5
> init_hwm(res0->base + HWM_INDEX_PORT);
> break;
> //case W83697HF_ACPI:
> //	init_acpi(dev);
> //	break;

... these should be dropped.
> }
>
> +}
>
> +void w83697hf_pnp_set_resources(device_t dev)
> +{
> pnp_enter_ext_func_mode(dev);
> pnp_set_resources(dev);
> pnp_exit_ext_func_mode(dev);
>
> +}
>
> +void w83697hf_pnp_enable_resources(device_t dev)
> +{
> pnp_enter_ext_func_mode(dev);
> pnp_enable_resources(dev);
> switch(dev->path.u.pnp.device) {
> case W83697HF_HWM:
> printk_debug("w83697hf hwm smbus enabled\n");
> enable_hwm_smbus(dev);
> break;
> }

Drop.
> pnp_exit_ext_func_mode(dev);
>
> +}
>
> +void w83697hf_pnp_enable(device_t dev)
> +{
> if (!dev->enabled) {
> pnp_enter_ext_func_mode(dev);   
> pnp_set_logical_device(dev);
> pnp_set_enable(dev, 0);
> pnp_exit_ext_func_mode(dev);  
> }
>
> +}
>
> +static struct device_operations ops = {
> .read_resources   = pnp_read_resources,
> .set_resources    = w83697hf_pnp_set_resources,
> .enable_resources = pnp_enable_resources,
> .enable           = w83697hf_pnp_enable,
> .init             = w83697hf_init,
>
> +};
>
> +static struct pnp_info pnp_dev_info[] = {
>    { &ops, W83697HF_FDC,  PNP_IO0 | PNP_IRQ0 | PNP_DRQ0, { 0x07f8, 0}, },
>    { &ops, W83697HF_PP,   PNP_IO0 | PNP_IRQ0 | PNP_DRQ0, { 0x07f8, 0}, },
>    { &ops, W83697HF_SP1,  PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, },
>    { &ops, W83697HF_SP2,  PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, },
>    // No 4 { 0,},
>    // No 5 { 0,},
>    { &ops, W83697HF_CIR, PNP_IO0 | PNP_IRQ0, { 0x7f8, 0 }, },
>    { &ops, W83697HF_GAME_GPIO1, PNP_IO0 | PNP_IO1 | PNP_IRQ0, { 0x7ff, 0 }, {0x7fe, 0x4}, },
>    { &ops, W83697HF_MIDI_GPIO5, },
>    { &ops, W83697HF_GPIO2, },
>    { &ops, W83697HF_ACPI, },
>    { &ops, W83697HF_HWM,  PNP_IO0 | PNP_IRQ0, { 0xff8, 0 }, },
>
> +};
>
> +static void enable_dev(struct device *dev)
> +{
> pnp_enable_devices(dev, &ops,
> sizeof(pnp_dev_info)/sizeof(pnp_dev_info[0]), pnp_dev_info);

Use ARRAY_SIZE here please (stdlib.h).
> +}
>
> +struct chip_operations superio_winbond_w83697hf_ops = {
> CHIP_NAME("Winbond W83697HF Super I/O")
> .enable_dev = enable_dev,
>
> +};
> Index: src/superio/winbond/w83697hf/chip.h
>
> --- src/superio/winbond/w83697hf/chip.h	(revision 0)
> +++ src/superio/winbond/w83697hf/chip.h	(revision 0)
> @@ -0,0 +1,28 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Sean Nelson
> 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 <pc80/keyboard.h>

This chip doesn't seem to have keyboard support so this is #include is
not needed.
> +#include <uart8250.h>
>
> +extern struct chip_operations superio_winbond_w83697hf_ops;
>
> +struct superio_winbond_w83697hf_config {
> struct uart8250 com1, com2;
>
> +};
> Index: src/superio/winbond/w83697hf/w83697hf.h
> ===================================================================
> --- src/superio/winbond/w83697hf/w83697hf.h	(revision 0)
> +++ src/superio/winbond/w83697hf/w83697hf.h	(revision 0)
> @@ -0,0 +1,30 @@
> +/*
> This file is part of the coreboot project.
> Copyright (C) 2008 Sean Nelson
> 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
> */
>
> +#define W83697HF_FDC            0   /* Floppy */
> +#define W83697HF_PP             1   /* Parallel Port */
> +#define W83697HF_SP1            2   /* Com1 */
> +#define W83697HF_SP2            3   /* Com2 */
> +#define W83697HF_CIR            6
> +#define W83697HF_GAME_GPIO1     7
> +#define W83697HF_MIDI_GPIO5     8
> +#define W83697HF_GPIO2          9

Maybe W83697HF_GPIO234, as this is for GPIOs 2-4.
> +#define W83697HF_ACPI           10
> +#define W83697HF_HWM            11   /* Hardware Monitor */

HTH, Uwe.