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 <[EMAIL PROTECTED]> > + * > + * 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 <[EMAIL PROTECTED]> > + * > + * 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 <[EMAIL PROTECTED]> > + * > + * 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 <[EMAIL PROTECTED]> > + * > + * 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. -- http://www.hermann-uwe.de | http://www.holsham-traders.de http://www.crazy-hacks.org | http://www.unmaintained-free-software.org -- coreboot mailing list [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
