| Message ID | 1374614206-9368-3-git-send-email-hpoussin@reactos.org |
|---|---|
| State | New |
| Headers | show |
Hi, Am 23.07.2013 23:16, schrieb Hervé Poussineau: > - i82378 only exists on PCI bus ; do not split implementation in 2 structures > - remove BARs, which are not specified in datasheet > - replace custom isa_mmio implementation by PCI bus IO region usage > - use QOM casts when required > > Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> Thanks for adopting some of the latest patterns without being asked to! I've queued this with some style changes, but apart from testing issues as CC'ed, I have a major question/concern in the bottom... > --- > hw/isa/i82378.c | 220 ++++++++++++------------------------------------------- > 1 file changed, 48 insertions(+), 172 deletions(-) > > diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c > index de71d81..f2045de 100644 > --- a/hw/isa/i82378.c > +++ b/hw/isa/i82378.c > @@ -22,134 +22,27 @@ > #include "hw/timer/i8254.h" > #include "hw/audio/pcspk.h" > > -//#define DEBUG_I82378 > - > -#ifdef DEBUG_I82378 > -#define DPRINTF(fmt, ...) \ > -do { fprintf(stderr, "i82378: " fmt , ## __VA_ARGS__); } while (0) > -#else > -#define DPRINTF(fmt, ...) \ > -do {} while (0) > -#endif > - > -#define BADF(fmt, ...) \ > -do { fprintf(stderr, "i82378 ERROR: " fmt , ## __VA_ARGS__); } while (0) > +#define TYPE_I82378 "i82378" > +#define I82378(obj) \ > + OBJECT_CHECK(I82378State, (obj), TYPE_I82378) > > typedef struct I82378State { > + PCIDevice parent_obj; > qemu_irq out[2]; > qemu_irq *i8259; > MemoryRegion io; > - MemoryRegion mem; > } I82378State; > > -typedef struct PCIi82378State { > - PCIDevice pci_dev; > - uint32_t isa_io_base; > - I82378State state; > -} PCIi82378State; > - > -static const VMStateDescription vmstate_pci_i82378 = { > - .name = "pci-i82378", > +static const VMStateDescription vmstate_i82378 = { > + .name = "i82378", > .version_id = 0, > .minimum_version_id = 0, > .fields = (VMStateField[]) { > - VMSTATE_PCI_DEVICE(pci_dev, PCIi82378State), > + VMSTATE_PCI_DEVICE(parent_obj, I82378State), > VMSTATE_END_OF_LIST() > }, > }; > > -static void i82378_io_write(void *opaque, hwaddr addr, > - uint64_t value, unsigned int size) > -{ > - switch (size) { > - case 1: > - DPRINTF("%s: " TARGET_FMT_plx "=%02" PRIx64 "\n", __func__, > - addr, value); > - cpu_outb(addr, value); > - break; > - case 2: > - DPRINTF("%s: " TARGET_FMT_plx "=%04" PRIx64 "\n", __func__, > - addr, value); > - cpu_outw(addr, value); > - break; > - case 4: > - DPRINTF("%s: " TARGET_FMT_plx "=%08" PRIx64 "\n", __func__, > - addr, value); > - cpu_outl(addr, value); > - break; > - default: > - abort(); > - } > -} > - > -static uint64_t i82378_io_read(void *opaque, hwaddr addr, > - unsigned int size) > -{ > - DPRINTF("%s: " TARGET_FMT_plx "\n", __func__, addr); > - switch (size) { > - case 1: > - return cpu_inb(addr); > - case 2: > - return cpu_inw(addr); > - case 4: > - return cpu_inl(addr); > - default: > - abort(); > - } > -} > - > -static const MemoryRegionOps i82378_io_ops = { > - .read = i82378_io_read, > - .write = i82378_io_write, > - .endianness = DEVICE_LITTLE_ENDIAN, > -}; > - > -static void i82378_mem_write(void *opaque, hwaddr addr, > - uint64_t value, unsigned int size) > -{ > - switch (size) { > - case 1: > - DPRINTF("%s: " TARGET_FMT_plx "=%02" PRIx64 "\n", __func__, > - addr, value); > - cpu_outb(addr, value); > - break; > - case 2: > - DPRINTF("%s: " TARGET_FMT_plx "=%04" PRIx64 "\n", __func__, > - addr, value); > - cpu_outw(addr, value); > - break; > - case 4: > - DPRINTF("%s: " TARGET_FMT_plx "=%08" PRIx64 "\n", __func__, > - addr, value); > - cpu_outl(addr, value); > - break; > - default: > - abort(); > - } > -} > - > -static uint64_t i82378_mem_read(void *opaque, hwaddr addr, > - unsigned int size) > -{ > - DPRINTF("%s: " TARGET_FMT_plx "\n", __func__, addr); > - switch (size) { > - case 1: > - return cpu_inb(addr); > - case 2: > - return cpu_inw(addr); > - case 4: > - return cpu_inl(addr); > - default: > - abort(); > - } > -} > - > -static const MemoryRegionOps i82378_mem_ops = { > - .read = i82378_mem_read, > - .write = i82378_mem_write, > - .endianness = DEVICE_LITTLE_ENDIAN, > -}; > - > static void i82378_request_out0_irq(void *opaque, int irq, int level) > { > I82378State *s = opaque; > @@ -159,19 +52,36 @@ static void i82378_request_out0_irq(void *opaque, int irq, int level) > static void i82378_request_pic_irq(void *opaque, int irq, int level) > { > DeviceState *dev = opaque; > - PCIDevice *pci = DO_UPCAST(PCIDevice, qdev, dev); > - PCIi82378State *s = DO_UPCAST(PCIi82378State, pci_dev, pci); > - > - qemu_set_irq(s->state.i8259[irq], level); > + qemu_set_irq(I82378(dev)->i8259[irq], level); Changed that back to a variable s - FOO(bar)->baz is undesired. > } > > -static void i82378_init(DeviceState *dev, I82378State *s) > +static void i82378_realize(DeviceState *dev, Error **errp) > { > - ISABus *isabus = ISA_BUS(qdev_get_child_bus(dev, "isa.0")); > - ISADevice *pit; > + PCIDevice *pci = PCI_DEVICE(dev); > + I82378State *s = I82378(dev); > + DeviceClass *dc; > + uint8_t *pci_conf; > + ISABus *isabus; > ISADevice *isa; > qemu_irq *out0_irq; > > + dc = DEVICE_CLASS(object_class_get_parent(object_get_class(OBJECT(dev)))); This is going into uncharted territories. ;) I consider it wrong to use object_get_class() - we should use object_class_by_name() to allow for derived types and I'll put it into a macro that I'll try to align with Peter C.'s and my QOM work. > + assert(dc); This shouldn't be necessary? > + dc->realize(dev, errp); > + if (error_is_set(errp)) { > + return; This doesn't do what you want. You need a local err variable to return here, errp might be NULL => !error_is_set(errp). > + } > + > + pci_conf = pci->config; > + pci_set_word(pci_conf + PCI_COMMAND, > + PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER); > + pci_set_word(pci_conf + PCI_STATUS, > + PCI_STATUS_DEVSEL_MEDIUM); > + > + pci_config_set_interrupt_pin(pci_conf, 1); /* interrupt pin 0 */ > + > + isabus = isa_bus_new(dev, pci_address_space_io(pci)); > + > /* This device has: > 2 82C59 (irq) > 1 82C54 (pit) > @@ -182,9 +92,6 @@ static void i82378_init(DeviceState *dev, I82378State *s) > All devices accept byte access only, except timer > */ > > - qdev_init_gpio_out(dev, s->out, 2); > - qdev_init_gpio_in(dev, i82378_request_pic_irq, 16); > - > /* Workaround the fact that i8259 is not qdev'ified... */ > out0_irq = qemu_allocate_irqs(i82378_request_out0_irq, s, 1); > > @@ -193,10 +100,10 @@ static void i82378_init(DeviceState *dev, I82378State *s) > isa_bus_irqs(isabus, s->i8259); > > /* 1 82C54 (pit) */ > - pit = pit_init(isabus, 0x40, 0, NULL); > + isa = pit_init(isabus, 0x40, 0, NULL); > > /* speaker */ > - pcspk_init(isabus, pit); > + pcspk_init(isabus, isa); > > /* 2 82C37 (dma) */ > isa = isa_create_simple(isabus, "i82374"); > @@ -206,71 +113,40 @@ static void i82378_init(DeviceState *dev, I82378State *s) > isa_create_simple(isabus, "mc146818rtc"); > } > > -static int pci_i82378_init(PCIDevice *dev) > +static void i82378_instance_init(Object *obj) > { > - PCIi82378State *pci = DO_UPCAST(PCIi82378State, pci_dev, dev); > - I82378State *s = &pci->state; > - uint8_t *pci_conf; > - > - pci_conf = dev->config; > - pci_set_word(pci_conf + PCI_COMMAND, > - PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER); > - pci_set_word(pci_conf + PCI_STATUS, > - PCI_STATUS_DEVSEL_MEDIUM); > - > - pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin 0 */ > - > - memory_region_init_io(&s->io, OBJECT(pci), &i82378_io_ops, s, > - "i82378-io", 0x00010000); > - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->io); > - > - memory_region_init_io(&s->mem, OBJECT(pci), &i82378_mem_ops, s, > - "i82378-mem", 0x01000000); > - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem); > - > - /* Make I/O address read only */ > - pci_set_word(dev->wmask + PCI_COMMAND, PCI_COMMAND_SPECIAL); > - pci_set_long(dev->wmask + PCI_BASE_ADDRESS_0, 0); > - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, pci->isa_io_base); > + DeviceState *dev = DEVICE(obj); > + I82378State *s = I82378(obj); > > - isa_bus_new(&dev->qdev, pci_address_space_io(dev)); > - > - i82378_init(&dev->qdev, s); > - > - return 0; > + qdev_init_gpio_out(dev, s->out, 2); > + qdev_init_gpio_in(dev, i82378_request_pic_irq, 16); > } > > -static Property i82378_properties[] = { > - DEFINE_PROP_HEX32("iobase", PCIi82378State, isa_io_base, 0x80000000), > - DEFINE_PROP_END_OF_LIST() > -}; > - > -static void pci_i82378_class_init(ObjectClass *klass, void *data) > +static void i82378_class_init(ObjectClass *klass, void *data) > { > PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); > DeviceClass *dc = DEVICE_CLASS(klass); > > - k->init = pci_i82378_init; > k->vendor_id = PCI_VENDOR_ID_INTEL; > k->device_id = PCI_DEVICE_ID_INTEL_82378; > k->revision = 0x03; > k->class_id = PCI_CLASS_BRIDGE_ISA; > - k->subsystem_vendor_id = 0x0; > - k->subsystem_id = 0x0; > - dc->vmsd = &vmstate_pci_i82378; > - dc->props = i82378_properties; > + dc->realize = i82378_realize; > + dc->vmsd = &vmstate_i82378; (FWIW dc->categories has been merged here.) > + dc->no_user = 1; Why do you do this? For one, according to Anthony it should no longer be used, and for another, Paolo's endianness-test (make check) is using -device i82378 for various other ppc and sh4 machines IIUC. make check still succeeds for ppc with this patch though, so that might be due tot -device ignoring DeviceClass::no_user? Hoping to get this in shape for -rc1. Regards, Andreas > } > > -static const TypeInfo pci_i82378_info = { > - .name = "i82378", > +static const TypeInfo i82378_info = { > + .name = TYPE_I82378, > .parent = TYPE_PCI_DEVICE, > - .instance_size = sizeof(PCIi82378State), > - .class_init = pci_i82378_class_init, > + .instance_size = sizeof(I82378State), > + .instance_init = i82378_instance_init, > + .class_init = i82378_class_init, > }; > > static void i82378_register_types(void) > { > - type_register_static(&pci_i82378_info); > + type_register_static(&i82378_info); > } > > type_init(i82378_register_types) >
Andreas Färber a écrit : > Hi, > > Am 23.07.2013 23:16, schrieb Hervé Poussineau: >> - i82378 only exists on PCI bus ; do not split implementation in 2 structures >> - remove BARs, which are not specified in datasheet >> - replace custom isa_mmio implementation by PCI bus IO region usage >> - use QOM casts when required >> >> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> > > Thanks for adopting some of the latest patterns without being asked to! > > I've queued this with some style changes, but apart from testing issues > as CC'ed, I have a major question/concern in the bottom... > >> --- >> hw/isa/i82378.c | 220 ++++++++++++------------------------------------------- >> 1 file changed, 48 insertions(+), 172 deletions(-) >> >> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c >> index de71d81..f2045de 100644 >> --- a/hw/isa/i82378.c >> +++ b/hw/isa/i82378.c [...] >> @@ -159,19 +52,36 @@ static void i82378_request_out0_irq(void *opaque, int irq, int level) >> static void i82378_request_pic_irq(void *opaque, int irq, int level) >> { >> DeviceState *dev = opaque; >> - PCIDevice *pci = DO_UPCAST(PCIDevice, qdev, dev); >> - PCIi82378State *s = DO_UPCAST(PCIi82378State, pci_dev, pci); >> - >> - qemu_set_irq(s->state.i8259[irq], level); >> + qemu_set_irq(I82378(dev)->i8259[irq], level); > > Changed that back to a variable s - FOO(bar)->baz is undesired. OK > >> } >> >> -static void i82378_init(DeviceState *dev, I82378State *s) >> +static void i82378_realize(DeviceState *dev, Error **errp) >> { >> - ISABus *isabus = ISA_BUS(qdev_get_child_bus(dev, "isa.0")); >> - ISADevice *pit; >> + PCIDevice *pci = PCI_DEVICE(dev); >> + I82378State *s = I82378(dev); >> + DeviceClass *dc; >> + uint8_t *pci_conf; >> + ISABus *isabus; >> ISADevice *isa; >> qemu_irq *out0_irq; >> >> + dc = DEVICE_CLASS(object_class_get_parent(object_get_class(OBJECT(dev)))); > > This is going into uncharted territories. ;) I consider it wrong to use > object_get_class() - we should use object_class_by_name() to allow for > derived types and I'll put it into a macro that I'll try to align with > Peter C.'s and my QOM work. OK >> + assert(dc); > > This shouldn't be necessary? OK. It can be removed. > >> + dc->realize(dev, errp); >> + if (error_is_set(errp)) { >> + return; > > This doesn't do what you want. You need a local err variable to return > here, errp might be NULL => !error_is_set(errp). OK [...] >> >> - k->init = pci_i82378_init; >> k->vendor_id = PCI_VENDOR_ID_INTEL; >> k->device_id = PCI_DEVICE_ID_INTEL_82378; >> k->revision = 0x03; >> k->class_id = PCI_CLASS_BRIDGE_ISA; >> - k->subsystem_vendor_id = 0x0; >> - k->subsystem_id = 0x0; >> - dc->vmsd = &vmstate_pci_i82378; >> - dc->props = i82378_properties; >> + dc->realize = i82378_realize; >> + dc->vmsd = &vmstate_i82378; > > (FWIW dc->categories has been merged here.) Yes, but it has been merged after I sent this patch... > >> + dc->no_user = 1; > > Why do you do this? For one, according to Anthony it should no longer be > used, and for another, Paolo's endianness-test (make check) is using > -device i82378 for various other ppc and sh4 machines IIUC. make check > still succeeds for ppc with this patch though, so that might be due tot > -device ignoring DeviceClass::no_user? I probably copied it from another chipset device, maybe i440fx. I don't really mind removing it. Yes, I double-checked that make check still works for all architectures. > Hoping to get this in shape for -rc1. Sure. Should I send a v2, as it seems you already queued it? Hervé
Am 30.07.2013 22:06, schrieb Hervé Poussineau: > Andreas Färber a écrit : >> Am 23.07.2013 23:16, schrieb Hervé Poussineau: >>> } >>> >>> -static void i82378_init(DeviceState *dev, I82378State *s) >>> +static void i82378_realize(DeviceState *dev, Error **errp) >>> { >>> - ISABus *isabus = ISA_BUS(qdev_get_child_bus(dev, "isa.0")); >>> - ISADevice *pit; >>> + PCIDevice *pci = PCI_DEVICE(dev); >>> + I82378State *s = I82378(dev); >>> + DeviceClass *dc; >>> + uint8_t *pci_conf; >>> + ISABus *isabus; >>> ISADevice *isa; >>> qemu_irq *out0_irq; >>> >>> + dc = >>> DEVICE_CLASS(object_class_get_parent(object_get_class(OBJECT(dev)))); >> >> This is going into uncharted territories. ;) I consider it wrong to use >> object_get_class() - we should use object_class_by_name() to allow for >> derived types and I'll put it into a macro that I'll try to align with >> Peter C.'s and my QOM work. > > OK While this gave me an inspiration for my virtio refactoring (it is possible to convert device by device by calling the parent's DeviceClass::realize as done here, as long as the *Class::init is called conditionally through the DeviceClass::init implementation), I am concerned about a single device deviating from initialization order here (hw/pci/pci.c:pci_qdev_init() does things after calling PCIDevice::init, namely ROM handling and bus hotplug) and will revert this to an old-style qdev initfn for 1.6 bugfix. [...] >>> + dc->no_user = 1; >> >> Why do you do this? For one, according to Anthony it should no longer be >> used, and for another, Paolo's endianness-test (make check) is using >> -device i82378 for various other ppc and sh4 machines IIUC. make check >> still succeeds for ppc with this patch though, so that might be due to >> -device ignoring DeviceClass::no_user? > > I probably copied it from another chipset device, maybe i440fx. > I don't really mind removing it. Good. > Yes, I double-checked that make check still works for all architectures. > >> Hoping to get this in shape for -rc1. > > Sure. Should I send a v2, as it seems you already queued it? Since -rc1 is due tomorrow, I'll put together a pull myself tonight. Andreas
Am 23.07.2013 23:16, schrieb Hervé Poussineau: > diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c > index de71d81..f2045de 100644 > --- a/hw/isa/i82378.c > +++ b/hw/isa/i82378.c [...] > -static const VMStateDescription vmstate_pci_i82378 = { > - .name = "pci-i82378", > +static const VMStateDescription vmstate_i82378 = { > + .name = "i82378", > .version_id = 0, > .minimum_version_id = 0, > .fields = (VMStateField[]) { > - VMSTATE_PCI_DEVICE(pci_dev, PCIi82378State), > + VMSTATE_PCI_DEVICE(parent_obj, I82378State), > VMSTATE_END_OF_LIST() > }, > }; [snip] Renaming the section would break backwards compatibility, we'll need to live with that name. Andreas
Andreas Färber a écrit : > Am 23.07.2013 23:16, schrieb Hervé Poussineau: >> diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c >> index de71d81..f2045de 100644 >> --- a/hw/isa/i82378.c >> +++ b/hw/isa/i82378.c > [...] >> -static const VMStateDescription vmstate_pci_i82378 = { >> - .name = "pci-i82378", >> +static const VMStateDescription vmstate_i82378 = { >> + .name = "i82378", >> .version_id = 0, >> .minimum_version_id = 0, >> .fields = (VMStateField[]) { >> - VMSTATE_PCI_DEVICE(pci_dev, PCIi82378State), >> + VMSTATE_PCI_DEVICE(parent_obj, I82378State), >> VMSTATE_END_OF_LIST() >> }, >> }; > [snip] > > Renaming the section would break backwards compatibility, we'll need to > live with that name. Not a problem. Or, you can update version_id and minimum_version_id to 1, and ignore backward compatibility problems, as i82378 is only used in PPC machine by default. Do as you want. Hervé
diff --git a/hw/isa/i82378.c b/hw/isa/i82378.c index de71d81..f2045de 100644 --- a/hw/isa/i82378.c +++ b/hw/isa/i82378.c @@ -22,134 +22,27 @@ #include "hw/timer/i8254.h" #include "hw/audio/pcspk.h" -//#define DEBUG_I82378 - -#ifdef DEBUG_I82378 -#define DPRINTF(fmt, ...) \ -do { fprintf(stderr, "i82378: " fmt , ## __VA_ARGS__); } while (0) -#else -#define DPRINTF(fmt, ...) \ -do {} while (0) -#endif - -#define BADF(fmt, ...) \ -do { fprintf(stderr, "i82378 ERROR: " fmt , ## __VA_ARGS__); } while (0) +#define TYPE_I82378 "i82378" +#define I82378(obj) \ + OBJECT_CHECK(I82378State, (obj), TYPE_I82378) typedef struct I82378State { + PCIDevice parent_obj; qemu_irq out[2]; qemu_irq *i8259; MemoryRegion io; - MemoryRegion mem; } I82378State; -typedef struct PCIi82378State { - PCIDevice pci_dev; - uint32_t isa_io_base; - I82378State state; -} PCIi82378State; - -static const VMStateDescription vmstate_pci_i82378 = { - .name = "pci-i82378", +static const VMStateDescription vmstate_i82378 = { + .name = "i82378", .version_id = 0, .minimum_version_id = 0, .fields = (VMStateField[]) { - VMSTATE_PCI_DEVICE(pci_dev, PCIi82378State), + VMSTATE_PCI_DEVICE(parent_obj, I82378State), VMSTATE_END_OF_LIST() }, }; -static void i82378_io_write(void *opaque, hwaddr addr, - uint64_t value, unsigned int size) -{ - switch (size) { - case 1: - DPRINTF("%s: " TARGET_FMT_plx "=%02" PRIx64 "\n", __func__, - addr, value); - cpu_outb(addr, value); - break; - case 2: - DPRINTF("%s: " TARGET_FMT_plx "=%04" PRIx64 "\n", __func__, - addr, value); - cpu_outw(addr, value); - break; - case 4: - DPRINTF("%s: " TARGET_FMT_plx "=%08" PRIx64 "\n", __func__, - addr, value); - cpu_outl(addr, value); - break; - default: - abort(); - } -} - -static uint64_t i82378_io_read(void *opaque, hwaddr addr, - unsigned int size) -{ - DPRINTF("%s: " TARGET_FMT_plx "\n", __func__, addr); - switch (size) { - case 1: - return cpu_inb(addr); - case 2: - return cpu_inw(addr); - case 4: - return cpu_inl(addr); - default: - abort(); - } -} - -static const MemoryRegionOps i82378_io_ops = { - .read = i82378_io_read, - .write = i82378_io_write, - .endianness = DEVICE_LITTLE_ENDIAN, -}; - -static void i82378_mem_write(void *opaque, hwaddr addr, - uint64_t value, unsigned int size) -{ - switch (size) { - case 1: - DPRINTF("%s: " TARGET_FMT_plx "=%02" PRIx64 "\n", __func__, - addr, value); - cpu_outb(addr, value); - break; - case 2: - DPRINTF("%s: " TARGET_FMT_plx "=%04" PRIx64 "\n", __func__, - addr, value); - cpu_outw(addr, value); - break; - case 4: - DPRINTF("%s: " TARGET_FMT_plx "=%08" PRIx64 "\n", __func__, - addr, value); - cpu_outl(addr, value); - break; - default: - abort(); - } -} - -static uint64_t i82378_mem_read(void *opaque, hwaddr addr, - unsigned int size) -{ - DPRINTF("%s: " TARGET_FMT_plx "\n", __func__, addr); - switch (size) { - case 1: - return cpu_inb(addr); - case 2: - return cpu_inw(addr); - case 4: - return cpu_inl(addr); - default: - abort(); - } -} - -static const MemoryRegionOps i82378_mem_ops = { - .read = i82378_mem_read, - .write = i82378_mem_write, - .endianness = DEVICE_LITTLE_ENDIAN, -}; - static void i82378_request_out0_irq(void *opaque, int irq, int level) { I82378State *s = opaque; @@ -159,19 +52,36 @@ static void i82378_request_out0_irq(void *opaque, int irq, int level) static void i82378_request_pic_irq(void *opaque, int irq, int level) { DeviceState *dev = opaque; - PCIDevice *pci = DO_UPCAST(PCIDevice, qdev, dev); - PCIi82378State *s = DO_UPCAST(PCIi82378State, pci_dev, pci); - - qemu_set_irq(s->state.i8259[irq], level); + qemu_set_irq(I82378(dev)->i8259[irq], level); } -static void i82378_init(DeviceState *dev, I82378State *s) +static void i82378_realize(DeviceState *dev, Error **errp) { - ISABus *isabus = ISA_BUS(qdev_get_child_bus(dev, "isa.0")); - ISADevice *pit; + PCIDevice *pci = PCI_DEVICE(dev); + I82378State *s = I82378(dev); + DeviceClass *dc; + uint8_t *pci_conf; + ISABus *isabus; ISADevice *isa; qemu_irq *out0_irq; + dc = DEVICE_CLASS(object_class_get_parent(object_get_class(OBJECT(dev)))); + assert(dc); + dc->realize(dev, errp); + if (error_is_set(errp)) { + return; + } + + pci_conf = pci->config; + pci_set_word(pci_conf + PCI_COMMAND, + PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER); + pci_set_word(pci_conf + PCI_STATUS, + PCI_STATUS_DEVSEL_MEDIUM); + + pci_config_set_interrupt_pin(pci_conf, 1); /* interrupt pin 0 */ + + isabus = isa_bus_new(dev, pci_address_space_io(pci)); + /* This device has: 2 82C59 (irq) 1 82C54 (pit) @@ -182,9 +92,6 @@ static void i82378_init(DeviceState *dev, I82378State *s) All devices accept byte access only, except timer */ - qdev_init_gpio_out(dev, s->out, 2); - qdev_init_gpio_in(dev, i82378_request_pic_irq, 16); - /* Workaround the fact that i8259 is not qdev'ified... */ out0_irq = qemu_allocate_irqs(i82378_request_out0_irq, s, 1); @@ -193,10 +100,10 @@ static void i82378_init(DeviceState *dev, I82378State *s) isa_bus_irqs(isabus, s->i8259); /* 1 82C54 (pit) */ - pit = pit_init(isabus, 0x40, 0, NULL); + isa = pit_init(isabus, 0x40, 0, NULL); /* speaker */ - pcspk_init(isabus, pit); + pcspk_init(isabus, isa); /* 2 82C37 (dma) */ isa = isa_create_simple(isabus, "i82374"); @@ -206,71 +113,40 @@ static void i82378_init(DeviceState *dev, I82378State *s) isa_create_simple(isabus, "mc146818rtc"); } -static int pci_i82378_init(PCIDevice *dev) +static void i82378_instance_init(Object *obj) { - PCIi82378State *pci = DO_UPCAST(PCIi82378State, pci_dev, dev); - I82378State *s = &pci->state; - uint8_t *pci_conf; - - pci_conf = dev->config; - pci_set_word(pci_conf + PCI_COMMAND, - PCI_COMMAND_IO | PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER); - pci_set_word(pci_conf + PCI_STATUS, - PCI_STATUS_DEVSEL_MEDIUM); - - pci_conf[PCI_INTERRUPT_PIN] = 1; /* interrupt pin 0 */ - - memory_region_init_io(&s->io, OBJECT(pci), &i82378_io_ops, s, - "i82378-io", 0x00010000); - pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->io); - - memory_region_init_io(&s->mem, OBJECT(pci), &i82378_mem_ops, s, - "i82378-mem", 0x01000000); - pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem); - - /* Make I/O address read only */ - pci_set_word(dev->wmask + PCI_COMMAND, PCI_COMMAND_SPECIAL); - pci_set_long(dev->wmask + PCI_BASE_ADDRESS_0, 0); - pci_set_long(pci_conf + PCI_BASE_ADDRESS_0, pci->isa_io_base); + DeviceState *dev = DEVICE(obj); + I82378State *s = I82378(obj); - isa_bus_new(&dev->qdev, pci_address_space_io(dev)); - - i82378_init(&dev->qdev, s); - - return 0; + qdev_init_gpio_out(dev, s->out, 2); + qdev_init_gpio_in(dev, i82378_request_pic_irq, 16); } -static Property i82378_properties[] = { - DEFINE_PROP_HEX32("iobase", PCIi82378State, isa_io_base, 0x80000000), - DEFINE_PROP_END_OF_LIST() -}; - -static void pci_i82378_class_init(ObjectClass *klass, void *data) +static void i82378_class_init(ObjectClass *klass, void *data) { PCIDeviceClass *k = PCI_DEVICE_CLASS(klass); DeviceClass *dc = DEVICE_CLASS(klass); - k->init = pci_i82378_init; k->vendor_id = PCI_VENDOR_ID_INTEL; k->device_id = PCI_DEVICE_ID_INTEL_82378; k->revision = 0x03; k->class_id = PCI_CLASS_BRIDGE_ISA; - k->subsystem_vendor_id = 0x0; - k->subsystem_id = 0x0; - dc->vmsd = &vmstate_pci_i82378; - dc->props = i82378_properties; + dc->realize = i82378_realize; + dc->vmsd = &vmstate_i82378; + dc->no_user = 1; } -static const TypeInfo pci_i82378_info = { - .name = "i82378", +static const TypeInfo i82378_info = { + .name = TYPE_I82378, .parent = TYPE_PCI_DEVICE, - .instance_size = sizeof(PCIi82378State), - .class_init = pci_i82378_class_init, + .instance_size = sizeof(I82378State), + .instance_init = i82378_instance_init, + .class_init = i82378_class_init, }; static void i82378_register_types(void) { - type_register_static(&pci_i82378_info); + type_register_static(&i82378_info); } type_init(i82378_register_types)
- i82378 only exists on PCI bus ; do not split implementation in 2 structures - remove BARs, which are not specified in datasheet - replace custom isa_mmio implementation by PCI bus IO region usage - use QOM casts when required Signed-off-by: Hervé Poussineau <hpoussin@reactos.org> --- hw/isa/i82378.c | 220 ++++++++++++------------------------------------------- 1 file changed, 48 insertions(+), 172 deletions(-)