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 <hpous...@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)
>
