Hi, On 11/30/2014 11:31 PM, Richard W.M. Jones wrote: > 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 > 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. Thanks for the feedback. I guess I could test it on Linux. Should I just run this utility? What's wrong with the Intel/Win driver? BTW, could you ever find some documentation on where the hell is the IRQ line is connected on real HW? >> 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 >> Regards, Adam