| Message ID | 28d9455165261a9cfd2cc4a650b25f987e60295c.1609121293.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Misc vt82c686b clean ups | expand |
On 12/28/20 3:08 AM, BALATON Zoltan via wrote: > Drop DPRINTF and use trace functions instead. Two debug messages about > unimplemented registers could be converted to qemu_log_mask() but in > reality all registers are currently unimplemented (we just store and > return values of writable regs but do nothing with them). As we > already trace register access there's no need for additional debug > messages so these are just removed and a comment is added as a reminder. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > v2: Extended commit message > > hw/isa/trace-events | 6 ++++++ > hw/isa/vt82c686.c | 51 +++++++++++++-------------------------------- > 2 files changed, 21 insertions(+), 36 deletions(-) > > diff --git a/hw/isa/trace-events b/hw/isa/trace-events > index 3544c6213c..d267d3e652 100644 > --- a/hw/isa/trace-events > +++ b/hw/isa/trace-events > @@ -13,3 +13,9 @@ pc87312_io_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x" > # apm.c > apm_io_read(uint8_t addr, uint8_t val) "read addr=0x%x val=0x%02x" > apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x" > + > +# vt82c686.c > +via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x" > +via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x" > +via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x" > +via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x" > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index cd87ec0103..d7ce15bf9f 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -27,14 +27,7 @@ > #include "qemu/timer.h" > #include "exec/address-spaces.h" > #include "qom/object.h" > - > -/* #define DEBUG_VT82C686B */ > - > -#ifdef DEBUG_VT82C686B > -#define DPRINTF(fmt, ...) fprintf(stderr, "%s: " fmt, __func__, ##__VA_ARGS__) > -#else > -#define DPRINTF(fmt, ...) > -#endif > +#include "trace.h" > > typedef struct SuperIOConfig { > uint8_t config[0x100]; > @@ -55,12 +48,12 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, > { > SuperIOConfig *superio_conf = opaque; > > - DPRINTF("superio_ioport_writeb address 0x%x val 0x%x\n", addr, data); > - if (addr == 0x3f0) { > + if (addr == 0x3f0) { /* config index register */ > superio_conf->index = data & 0xff; > } else { > bool can_write = true; > - /* 0x3f1 */ > + /* 0x3f1, config data register */ > + trace_via_superio_write(superio_conf->index, data & 0xff); > switch (superio_conf->index) { > case 0x00 ... 0xdf: > case 0xe4: > @@ -73,18 +66,7 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, > case 0xfd ... 0xff: > can_write = false; > break; > - case 0xe7: > - if ((data & 0xff) != 0xfe) { > - DPRINTF("change uart 1 base. unsupported yet\n"); > - can_write = false; > - } > - break; > - case 0xe8: > - if ((data & 0xff) != 0xbe) { > - DPRINTF("change uart 2 base. unsupported yet\n"); > - can_write = false; > - } > - break; > + /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */ If you don't mind I'll prepend this patch: -- >8 -- diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index cd87ec01039..23b4deaac93 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -25,6 +25,7 @@ #include "qapi/error.h" #include "qemu/module.h" #include "qemu/timer.h" +#include "qemu/log.h" #include "exec/address-spaces.h" #include "qom/object.h" @@ -73,17 +74,9 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, case 0xfd ... 0xff: can_write = false; break; - case 0xe7: - if ((data & 0xff) != 0xfe) { - DPRINTF("change uart 1 base. unsupported yet\n"); - can_write = false; - } - break; - case 0xe8: - if ((data & 0xff) != 0xbe) { - DPRINTF("change uart 2 base. unsupported yet\n"); - can_write = false; - } + case 0xe6 ... 0xe8: /* set base port of parallel and serial */ + qemu_log_mask(LOG_UNIMP, "change base port not implemented\n"); + can_write = false; break; default: break; ---
On Fri, 1 Jan 2021, Philippe Mathieu-Daudé wrote: > On 12/28/20 3:08 AM, BALATON Zoltan via wrote: >> Drop DPRINTF and use trace functions instead. Two debug messages about >> unimplemented registers could be converted to qemu_log_mask() but in >> reality all registers are currently unimplemented (we just store and >> return values of writable regs but do nothing with them). As we >> already trace register access there's no need for additional debug >> messages so these are just removed and a comment is added as a reminder. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> --- >> v2: Extended commit message >> >> hw/isa/trace-events | 6 ++++++ >> hw/isa/vt82c686.c | 51 +++++++++++++-------------------------------- >> 2 files changed, 21 insertions(+), 36 deletions(-) >> >> diff --git a/hw/isa/trace-events b/hw/isa/trace-events >> index 3544c6213c..d267d3e652 100644 >> --- a/hw/isa/trace-events >> +++ b/hw/isa/trace-events >> @@ -13,3 +13,9 @@ pc87312_io_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x" >> # apm.c >> apm_io_read(uint8_t addr, uint8_t val) "read addr=0x%x val=0x%02x" >> apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x" >> + >> +# vt82c686.c >> +via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x" >> +via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x" >> +via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x" >> +via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x" >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index cd87ec0103..d7ce15bf9f 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -27,14 +27,7 @@ >> #include "qemu/timer.h" >> #include "exec/address-spaces.h" >> #include "qom/object.h" >> - >> -/* #define DEBUG_VT82C686B */ >> - >> -#ifdef DEBUG_VT82C686B >> -#define DPRINTF(fmt, ...) fprintf(stderr, "%s: " fmt, __func__, ##__VA_ARGS__) >> -#else >> -#define DPRINTF(fmt, ...) >> -#endif >> +#include "trace.h" >> >> typedef struct SuperIOConfig { >> uint8_t config[0x100]; >> @@ -55,12 +48,12 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, >> { >> SuperIOConfig *superio_conf = opaque; >> >> - DPRINTF("superio_ioport_writeb address 0x%x val 0x%x\n", addr, data); >> - if (addr == 0x3f0) { >> + if (addr == 0x3f0) { /* config index register */ >> superio_conf->index = data & 0xff; >> } else { >> bool can_write = true; >> - /* 0x3f1 */ >> + /* 0x3f1, config data register */ >> + trace_via_superio_write(superio_conf->index, data & 0xff); >> switch (superio_conf->index) { >> case 0x00 ... 0xdf: >> case 0xe4: >> @@ -73,18 +66,7 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, >> case 0xfd ... 0xff: >> can_write = false; >> break; >> - case 0xe7: >> - if ((data & 0xff) != 0xfe) { >> - DPRINTF("change uart 1 base. unsupported yet\n"); >> - can_write = false; >> - } >> - break; >> - case 0xe8: >> - if ((data & 0xff) != 0xbe) { >> - DPRINTF("change uart 2 base. unsupported yet\n"); >> - can_write = false; >> - } >> - break; >> + /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */ > > If you don't mind I'll prepend this patch: I'll split it. I've finished rewriting vt82c686b to add vt8231 emulation and ended up with a 24 patch series. This'll make it 25. This includes all of part I and part II I've already posted and some more. Fuloong2e still seems to work (with the Bonito fix) at least with PMON and pegasos2 firmware also runs and starts to boot but MorphOS does not fully boot for some reason but I'm not sure if it's because of some other change or a bug somewhere in this series. I need to do more testing but I think I'll submit this series now anyway so it can be reviewed in the meantime and I can make changes in next iteration and don't need too many versions because I'll have less time for it now. Regards and happy new year, BALATON Zoltan > -- >8 -- > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index cd87ec01039..23b4deaac93 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -25,6 +25,7 @@ > #include "qapi/error.h" > #include "qemu/module.h" > #include "qemu/timer.h" > +#include "qemu/log.h" > #include "exec/address-spaces.h" > #include "qom/object.h" > > @@ -73,17 +74,9 @@ static void superio_ioport_writeb(void *opaque, > hwaddr addr, uint64_t data, > case 0xfd ... 0xff: > can_write = false; > break; > - case 0xe7: > - if ((data & 0xff) != 0xfe) { > - DPRINTF("change uart 1 base. unsupported yet\n"); > - can_write = false; > - } > - break; > - case 0xe8: > - if ((data & 0xff) != 0xbe) { > - DPRINTF("change uart 2 base. unsupported yet\n"); > - can_write = false; > - } > + case 0xe6 ... 0xe8: /* set base port of parallel and serial */ > + qemu_log_mask(LOG_UNIMP, "change base port not implemented\n"); > + can_write = false; > break; > default: > break; > --- > >
On Fri, 1 Jan 2021, Philippe Mathieu-Daudé wrote: > On 12/28/20 3:08 AM, BALATON Zoltan via wrote: >> Drop DPRINTF and use trace functions instead. Two debug messages about >> unimplemented registers could be converted to qemu_log_mask() but in >> reality all registers are currently unimplemented (we just store and >> return values of writable regs but do nothing with them). As we >> already trace register access there's no need for additional debug >> messages so these are just removed and a comment is added as a reminder. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > >> --- >> v2: Extended commit message >> >> hw/isa/trace-events | 6 ++++++ >> hw/isa/vt82c686.c | 51 +++++++++++++-------------------------------- >> 2 files changed, 21 insertions(+), 36 deletions(-) >> >> diff --git a/hw/isa/trace-events b/hw/isa/trace-events >> index 3544c6213c..d267d3e652 100644 >> --- a/hw/isa/trace-events >> +++ b/hw/isa/trace-events >> @@ -13,3 +13,9 @@ pc87312_io_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x" >> # apm.c >> apm_io_read(uint8_t addr, uint8_t val) "read addr=0x%x val=0x%02x" >> apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x" >> + >> +# vt82c686.c >> +via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x" >> +via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x" >> +via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x" >> +via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x" >> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c >> index cd87ec0103..d7ce15bf9f 100644 >> --- a/hw/isa/vt82c686.c >> +++ b/hw/isa/vt82c686.c >> @@ -27,14 +27,7 @@ >> #include "qemu/timer.h" >> #include "exec/address-spaces.h" >> #include "qom/object.h" >> - >> -/* #define DEBUG_VT82C686B */ >> - >> -#ifdef DEBUG_VT82C686B >> -#define DPRINTF(fmt, ...) fprintf(stderr, "%s: " fmt, __func__, ##__VA_ARGS__) >> -#else >> -#define DPRINTF(fmt, ...) >> -#endif >> +#include "trace.h" >> >> typedef struct SuperIOConfig { >> uint8_t config[0x100]; >> @@ -55,12 +48,12 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, >> { >> SuperIOConfig *superio_conf = opaque; >> >> - DPRINTF("superio_ioport_writeb address 0x%x val 0x%x\n", addr, data); >> - if (addr == 0x3f0) { >> + if (addr == 0x3f0) { /* config index register */ >> superio_conf->index = data & 0xff; >> } else { >> bool can_write = true; >> - /* 0x3f1 */ >> + /* 0x3f1, config data register */ >> + trace_via_superio_write(superio_conf->index, data & 0xff); >> switch (superio_conf->index) { >> case 0x00 ... 0xdf: >> case 0xe4: >> @@ -73,18 +66,7 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, >> case 0xfd ... 0xff: >> can_write = false; >> break; >> - case 0xe7: >> - if ((data & 0xff) != 0xfe) { >> - DPRINTF("change uart 1 base. unsupported yet\n"); >> - can_write = false; >> - } >> - break; >> - case 0xe8: >> - if ((data & 0xff) != 0xbe) { >> - DPRINTF("change uart 2 base. unsupported yet\n"); >> - can_write = false; >> - } >> - break; >> + /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */ > > If you don't mind I'll prepend this patch: > -- >8 -- > diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c > index cd87ec01039..23b4deaac93 100644 > --- a/hw/isa/vt82c686.c > +++ b/hw/isa/vt82c686.c > @@ -25,6 +25,7 @@ > #include "qapi/error.h" > #include "qemu/module.h" > #include "qemu/timer.h" > +#include "qemu/log.h" > #include "exec/address-spaces.h" > #include "qom/object.h" > > @@ -73,17 +74,9 @@ static void superio_ioport_writeb(void *opaque, > hwaddr addr, uint64_t data, > case 0xfd ... 0xff: > can_write = false; > break; > - case 0xe7: > - if ((data & 0xff) != 0xfe) { > - DPRINTF("change uart 1 base. unsupported yet\n"); > - can_write = false; > - } > - break; > - case 0xe8: > - if ((data & 0xff) != 0xbe) { > - DPRINTF("change uart 2 base. unsupported yet\n"); > - can_write = false; > - } > + case 0xe6 ... 0xe8: /* set base port of parallel and serial */ > + qemu_log_mask(LOG_UNIMP, "change base port not implemented\n"); > + can_write = false; Actually I won't add this log becuse in later patches I've added logging for all unimplemented regs here and implemented changing base for serial (needed for pegasos2 which puts the single serial port at 0x2f8). Still want this patch spit into too? Regards, BALATON Zoltan
diff --git a/hw/isa/trace-events b/hw/isa/trace-events index 3544c6213c..d267d3e652 100644 --- a/hw/isa/trace-events +++ b/hw/isa/trace-events @@ -13,3 +13,9 @@ pc87312_io_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x" # apm.c apm_io_read(uint8_t addr, uint8_t val) "read addr=0x%x val=0x%02x" apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x" + +# vt82c686.c +via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x" +via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x" +via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x" +via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x" diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c index cd87ec0103..d7ce15bf9f 100644 --- a/hw/isa/vt82c686.c +++ b/hw/isa/vt82c686.c @@ -27,14 +27,7 @@ #include "qemu/timer.h" #include "exec/address-spaces.h" #include "qom/object.h" - -/* #define DEBUG_VT82C686B */ - -#ifdef DEBUG_VT82C686B -#define DPRINTF(fmt, ...) fprintf(stderr, "%s: " fmt, __func__, ##__VA_ARGS__) -#else -#define DPRINTF(fmt, ...) -#endif +#include "trace.h" typedef struct SuperIOConfig { uint8_t config[0x100]; @@ -55,12 +48,12 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, { SuperIOConfig *superio_conf = opaque; - DPRINTF("superio_ioport_writeb address 0x%x val 0x%x\n", addr, data); - if (addr == 0x3f0) { + if (addr == 0x3f0) { /* config index register */ superio_conf->index = data & 0xff; } else { bool can_write = true; - /* 0x3f1 */ + /* 0x3f1, config data register */ + trace_via_superio_write(superio_conf->index, data & 0xff); switch (superio_conf->index) { case 0x00 ... 0xdf: case 0xe4: @@ -73,18 +66,7 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, case 0xfd ... 0xff: can_write = false; break; - case 0xe7: - if ((data & 0xff) != 0xfe) { - DPRINTF("change uart 1 base. unsupported yet\n"); - can_write = false; - } - break; - case 0xe8: - if ((data & 0xff) != 0xbe) { - DPRINTF("change uart 2 base. unsupported yet\n"); - can_write = false; - } - break; + /* case 0xe6 ... 0xe8: Should set base port of parallel and serial */ default: break; @@ -98,9 +80,10 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data, static uint64_t superio_ioport_readb(void *opaque, hwaddr addr, unsigned size) { SuperIOConfig *superio_conf = opaque; + uint8_t val = superio_conf->config[superio_conf->index]; - DPRINTF("superio_ioport_readb address 0x%x\n", addr); - return superio_conf->config[superio_conf->index]; + trace_via_superio_read(superio_conf->index, val); + return val; } static const MemoryRegionOps superio_ops = { @@ -141,16 +124,14 @@ static void vt82c686b_isa_reset(DeviceState *dev) } /* write config pci function0 registers. PCI-ISA bridge */ -static void vt82c686b_write_config(PCIDevice *d, uint32_t address, +static void vt82c686b_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len) { VT82C686BISAState *vt686 = VT82C686B_ISA(d); - DPRINTF("vt82c686b_write_config address 0x%x val 0x%x len 0x%x\n", - address, val, len); - - pci_default_write_config(d, address, val, len); - if (address == 0x85) { /* enable or disable super IO configure */ + trace_via_isa_write(addr, val, len); + pci_default_write_config(d, addr, val, len); + if (addr == 0x85) { /* enable or disable super IO configure */ memory_region_set_enabled(&vt686->superio, val & 0x2); } } @@ -203,12 +184,10 @@ static void pm_io_space_update(VT686PMState *s) memory_region_transaction_commit(); } -static void pm_write_config(PCIDevice *d, - uint32_t address, uint32_t val, int len) +static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len) { - DPRINTF("pm_write_config address 0x%x val 0x%x len 0x%x\n", - address, val, len); - pci_default_write_config(d, address, val, len); + trace_via_pm_write(addr, val, len); + pci_default_write_config(d, addr, val, len); } static int vmstate_acpi_post_load(void *opaque, int version_id)
Drop DPRINTF and use trace functions instead. Two debug messages about unimplemented registers could be converted to qemu_log_mask() but in reality all registers are currently unimplemented (we just store and return values of writable regs but do nothing with them). As we already trace register access there's no need for additional debug messages so these are just removed and a comment is added as a reminder. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- v2: Extended commit message hw/isa/trace-events | 6 ++++++ hw/isa/vt82c686.c | 51 +++++++++++++-------------------------------- 2 files changed, 21 insertions(+), 36 deletions(-)