On Wed, Oct 29, 2014 at 02:42:51PM +0100, Adam Hoka wrote: > Don't require configuration register write to be off a certain length, > as some PCI implementations always access them in 32bit only. This is > because it's in fact the only kind of access supported by the standard, > anything else is implementation dependent. > > Add support for reading back the configuration register values. > > Unify the MMIO register implementation into a common read and write > function. This makes driver testing in QEMU less surprising. > > Missing: interrupt register is still not implemented as interrupting > itself is absent. It's unclear from the 6300ESB ICH specs where > the IRQ line is connected in real hardware. > > Signed-off-by: Adam Hoka <adam.h...@gmail.com>
I don't really have any opinion on this patch. All I care is that it doesn't break the Linux device driver (the Intel-supplied 32 bit Windows device driver is unfortunately a lost cause). Did you test it against Linux? I wrote a small test harness that makes testing the qemu watchdog simple: http://git.annexia.org/?p=watchdog-test-framework.git;a=summary Rich. > hw/watchdog/wdt_i6300esb.c | 134 > ++++++++++++++++++--------------------------- > 1 file changed, 53 insertions(+), 81 deletions(-) > > diff --git a/hw/watchdog/wdt_i6300esb.c b/hw/watchdog/wdt_i6300esb.c > index 687c8b1..8512a91 100644 > --- a/hw/watchdog/wdt_i6300esb.c > +++ b/hw/watchdog/wdt_i6300esb.c > @@ -212,12 +212,12 @@ static void i6300esb_config_write(PCIDevice *dev, > uint32_t addr, > > i6300esb_debug("addr = %x, data = %x, len = %d\n", addr, data, len); > > - if (addr == ESB_CONFIG_REG && len == 2) { > + if (addr == ESB_CONFIG_REG) { > d->reboot_enabled = (data & ESB_WDT_REBOOT) == 0; > d->clock_scale = > (data & ESB_WDT_FREQ) != 0 ? CLOCK_SCALE_1MHZ : CLOCK_SCALE_1KHZ; > d->int_type = (data & ESB_WDT_INTTYPE); > - } else if (addr == ESB_LOCK_REG && len == 1) { > + } else if (addr == ESB_LOCK_REG) { > if (!d->locked) { > d->locked = (data & ESB_WDT_LOCK) != 0; > d->free_run = (data & ESB_WDT_FUNC) != 0; > @@ -240,13 +240,13 @@ static uint32_t i6300esb_config_read(PCIDevice *dev, > uint32_t addr, int len) > > i6300esb_debug ("addr = %x, len = %d\n", addr, len); > > - if (addr == ESB_CONFIG_REG && len == 2) { > + if (addr == ESB_CONFIG_REG) { > data = > (d->reboot_enabled ? 0 : ESB_WDT_REBOOT) | > (d->clock_scale == CLOCK_SCALE_1MHZ ? ESB_WDT_FREQ : 0) | > d->int_type; > return data; > - } else if (addr == ESB_LOCK_REG && len == 1) { > + } else if (addr == ESB_LOCK_REG) { > data = > (d->free_run ? ESB_WDT_FUNC : 0) | > (d->locked ? ESB_WDT_LOCK : 0) | > @@ -257,116 +257,88 @@ static uint32_t i6300esb_config_read(PCIDevice *dev, > uint32_t addr, int len) > } > } > > -static uint32_t i6300esb_mem_readb(void *vp, hwaddr addr) > +static uint32_t i6300esb_mem_read(void *vp, hwaddr addr) > { > - i6300esb_debug ("addr = %x\n", (int) addr); > - > - return 0; > -} > - > -static uint32_t i6300esb_mem_readw(void *vp, hwaddr addr) > -{ > - uint32_t data = 0; > I6300State *d = vp; > > - i6300esb_debug("addr = %x\n", (int) addr); > + i6300esb_debug("addr = %p\n", (void *)addr); > > - if (addr == 0xc) { > + switch (addr) { > + case 0x00: > + return d->timer1_preload; > + case 0x04: > + return d->timer2_preload; > + case 0x0c: > /* The previous reboot flag is really bit 9, but there is > * a bug in the Linux driver where it thinks it's bit 12. > * Set both. > */ > - data = d->previous_reboot_flag ? 0x1200 : 0; > + return d->previous_reboot_flag ? 0x1200 : 0; > } > > - return data; > -} > - > -static uint32_t i6300esb_mem_readl(void *vp, hwaddr addr) > -{ > - i6300esb_debug("addr = %x\n", (int) addr); > - > return 0; > } > > -static void i6300esb_mem_writeb(void *vp, hwaddr addr, uint32_t val) > +static void i6300esb_mem_write(void *vp, hwaddr addr, uint32_t val) > { > I6300State *d = vp; > > - i6300esb_debug("addr = %x, val = %x\n", (int) addr, val); > + i6300esb_debug("addr = %p, val = 0x%x\n", (void *)addr, val); > > - if (addr == 0xc && val == 0x80) > + /* register lock */ > + if (addr == 0xc && val == 0x80) { > d->unlock_state = 1; > - else if (addr == 0xc && val == 0x86 && d->unlock_state == 1) > + return; > + } else if (addr == 0xc && val == 0x86 && d->unlock_state == 1) { > d->unlock_state = 2; > -} > + return; > + } else if (d->unlock_state == 0) { > + return; > + } > > -static void i6300esb_mem_writew(void *vp, hwaddr addr, uint32_t val) > -{ > - I6300State *d = vp; > + switch (addr) { > + case 0x00: > + d->timer1_preload = val & 0xfffff; > + break; > > - i6300esb_debug("addr = %x, val = %x\n", (int) addr, val); > + case 0x04: > + d->timer2_preload = val & 0xfffff; > + break; > > - if (addr == 0xc && val == 0x80) > - d->unlock_state = 1; > - else if (addr == 0xc && val == 0x86 && d->unlock_state == 1) > - d->unlock_state = 2; > - else { > - if (d->unlock_state == 2) { > - if (addr == 0xc) { > - if ((val & 0x100) != 0) > - /* This is the "ping" from the userspace watchdog in > - * the guest ... > - */ > - i6300esb_restart_timer(d, 1); > - > - /* Setting bit 9 resets the previous reboot flag. > - * There's a bug in the Linux driver where it sets > - * bit 12 instead. > - */ > - if ((val & 0x200) != 0 || (val & 0x1000) != 0) { > - d->previous_reboot_flag = 0; > - } > - } > - > - d->unlock_state = 0; > + case 0x0c: > + if ((val & 0x100) != 0) { > + /* This is the "ping" from the userspace watchdog in > + * the guest ... > + */ > + i6300esb_restart_timer(d, 1); > } > - } > -} > - > -static void i6300esb_mem_writel(void *vp, hwaddr addr, uint32_t val) > -{ > - I6300State *d = vp; > > - i6300esb_debug ("addr = %x, val = %x\n", (int) addr, val); > - > - if (addr == 0xc && val == 0x80) > - d->unlock_state = 1; > - else if (addr == 0xc && val == 0x86 && d->unlock_state == 1) > - d->unlock_state = 2; > - else { > - if (d->unlock_state == 2) { > - if (addr == 0) > - d->timer1_preload = val & 0xfffff; > - else if (addr == 4) > - d->timer2_preload = val & 0xfffff; > - > - d->unlock_state = 0; > + /* Setting bit 9 resets the previous reboot flag. > + * There's a bug in the Linux driver where it sets > + * bit 12 instead. > + */ > + if ((val & 0x200) != 0 || (val & 0x1000) != 0) { > + d->previous_reboot_flag = 0; > } > + > + break; > } > + > + /* re-lock registers */ > + d->unlock_state = 0; > } > > static const MemoryRegionOps i6300esb_ops = { > .old_mmio = { > .read = { > - i6300esb_mem_readb, > - i6300esb_mem_readw, > - i6300esb_mem_readl, > + i6300esb_mem_read, > + i6300esb_mem_read, > + i6300esb_mem_read, > }, > .write = { > - i6300esb_mem_writeb, > - i6300esb_mem_writew, > - i6300esb_mem_writel, > + i6300esb_mem_write, > + i6300esb_mem_write, > + i6300esb_mem_write, > }, > }, > .endianness = DEVICE_NATIVE_ENDIAN, > -- > 2.1.1 > -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
