qemu-ppc
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v7 2/8] vt82c686: QOM-ify superio related functionality

From: BALATON Zoltan
Subject: Re: [PATCH v7 2/8] vt82c686: QOM-ify superio related functionality
Date: Fri, 12 Mar 2021 01:32:25 +0100 (CET) 
On Fri, 12 Mar 2021, Philippe Mathieu-Daudé wrote:

On 3/10/21 3:58 AM, BALATON Zoltan wrote:

Collect superio functionality and its controlling config registers
handling in an abstract VIA_SUPERIO class that is a subclass of
ISA_SUPERIO and put vt82c686b specific parts in a subclass of this
abstract class.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 242 ++++++++++++++++++++++++--------------
 include/hw/isa/vt82c686.h |   1 -
 2 files changed, 152 insertions(+), 91 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index a3353ec5db..e89dbf43da 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -249,12 +249,21 @@ static const TypeInfo vt8231_pm_info = {
 };


-typedef struct SuperIOConfig {
+#define TYPE_VIA_SUPERIO "via-superio"
+OBJECT_DECLARE_SIMPLE_TYPE(ViaSuperIOState, VIA_SUPERIO)
+
+struct ViaSuperIOState {
+    ISASuperIODevice superio;
     uint8_t regs[0x100];
+    const MemoryRegionOps *io_ops;
     MemoryRegion io;
-    ISASuperIODevice *superio;
     MemoryRegion *serial_io[SUPERIO_MAX_SERIAL_PORTS];
-} SuperIOConfig;
+};
+
+static inline void via_superio_io_enable(ViaSuperIOState *s, bool enable)
+{
+    memory_region_set_enabled(&s->io, enable);
+}

 static MemoryRegion *find_subregion(ISADevice *d, MemoryRegion *parent,
                                     int offs)
@@ -270,10 +279,78 @@ static MemoryRegion *find_subregion(ISADevice *d, 
MemoryRegion *parent,
     return mr;
 }

-static void superio_cfg_write(void *opaque, hwaddr addr, uint64_t data,
-                              unsigned size)
+static void via_superio_realize(DeviceState *d, Error **errp)
+{
+    ViaSuperIOState *s = VIA_SUPERIO(d);
+    ISASuperIOClass *ic = ISA_SUPERIO_GET_CLASS(s);
+    Error *local_err = NULL;
+    int i;
+
+    assert(s->io_ops);
+    ic->parent_realize(d, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        return;
+    }
+    /* Grab io regions of serial devices so we can control them */
+    for (i = 0; i < ic->serial.count; i++) {
+        ISADevice *sd = s->superio.serial[i];
+        MemoryRegion *io = isa_address_space_io(sd);
+        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
+        if (!mr) {
+            error_setg(errp, "Could not get io region for serial %d", i);
+            return;
+        }
+        s->serial_io[i] = mr;


Don't we need to memory_region_ref(mr) here?
I don't think so. The ISA device still owns the memory region, we just snatch it to be able to enable/disable and move it around but otherwise don't take it over from the device. The memory region would be destroyed with the device and after that we don't want to keep the memory region as that's not useful without the device. (This is all theoretical though as in practice the device is created at startup and never destroyed until exit.) If we increased ref count here we would also have to decrease it somewhere but since the serial device is child of this south bridge it should not go away before via-superio so no separate reference should be needed here. We own ISASuperIO which owns the isa-serial which owns the memory region so it should stay there during the lifetime of this object without increasing ref count. 

Regards,
BALATON Zoltan


+    }
+
+    memory_region_init_io(&s->io, OBJECT(d), s->io_ops, s, "via-superio", 2);
+    memory_region_set_enabled(&s->io, false);
+    /* The floppy also uses 0x3f0 and 0x3f1 but this seems to work anyway */
+    memory_region_add_subregion(isa_address_space_io(ISA_DEVICE(s)), 0x3f0,
+                                &s->io);
+}
+
+static uint64_t via_superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
+{
+    ViaSuperIOState *sc = opaque;
+    uint8_t idx = sc->regs[0];
+    uint8_t val = sc->regs[idx];
+
+    if (addr == 0) {
+        return idx;
+    }
+    if (addr == 1 && idx == 0) {
+        val = 0; /* reading reg 0 where we store index value */
+    }
+    trace_via_superio_read(idx, val);
+    return val;
+}
+
+static void via_superio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
+
+    sc->parent_realize = dc->realize;
+    dc->realize = via_superio_realize;
+}
+
+static const TypeInfo via_superio_info = {
+    .name          = TYPE_VIA_SUPERIO,
+    .parent        = TYPE_ISA_SUPERIO,
+    .instance_size = sizeof(ViaSuperIOState),
+    .class_size    = sizeof(ISASuperIOClass),
+    .class_init    = via_superio_class_init,
+    .abstract      = true,
+};
+
+#define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
+
+static void vt82c686b_superio_cfg_write(void *opaque, hwaddr addr,
+                                        uint64_t data, unsigned size)
 {
-    SuperIOConfig *sc = opaque;
+    ViaSuperIOState *sc = opaque;
     uint8_t idx = sc->regs[0];

     if (addr == 0) { /* config index register */
@@ -295,29 +372,29 @@ static void superio_cfg_write(void *opaque, hwaddr addr, 
uint64_t data,
     case 0xfd ... 0xff:
         /* ignore write to read only registers */
         return;
-    case 0xe2:
+    case 0xe2: /* Function select */
     {
         data &= 0x1f;
         if (data & BIT(2)) { /* Serial port 1 enable */
-            ISADevice *dev = sc->superio->serial[0];
+            ISADevice *dev = sc->superio.serial[0];
             if (!memory_region_is_mapped(sc->serial_io[0])) {
                 memory_region_add_subregion(isa_address_space_io(dev),
                                             dev->ioport_id, sc->serial_io[0]);
             }
         } else {
-            MemoryRegion *io = isa_address_space_io(sc->superio->serial[0]);
+            MemoryRegion *io = isa_address_space_io(sc->superio.serial[0]);
             if (memory_region_is_mapped(sc->serial_io[0])) {
                 memory_region_del_subregion(io, sc->serial_io[0]);
             }
         }
         if (data & BIT(3)) { /* Serial port 2 enable */
-            ISADevice *dev = sc->superio->serial[1];
+            ISADevice *dev = sc->superio.serial[1];
             if (!memory_region_is_mapped(sc->serial_io[1])) {
                 memory_region_add_subregion(isa_address_space_io(dev),
                                             dev->ioport_id, sc->serial_io[1]);
             }
         } else {
-            MemoryRegion *io = isa_address_space_io(sc->superio->serial[1]);
+            MemoryRegion *io = isa_address_space_io(sc->superio.serial[1]);
             if (memory_region_is_mapped(sc->serial_io[1])) {
                 memory_region_del_subregion(io, sc->serial_io[1]);
             }
@@ -327,7 +404,7 @@ static void superio_cfg_write(void *opaque, hwaddr addr, 
uint64_t data,
     case 0xe7: /* Serial port 1 io base address */
     {
         data &= 0xfe;
-        sc->superio->serial[0]->ioport_id = data << 2;
+        sc->superio.serial[0]->ioport_id = data << 2;
         if (memory_region_is_mapped(sc->serial_io[0])) {
             memory_region_set_address(sc->serial_io[0], data << 2);
         }
@@ -336,7 +413,7 @@ static void superio_cfg_write(void *opaque, hwaddr addr, 
uint64_t data,
     case 0xe8: /* Serial port 2 io base address */
     {
         data &= 0xfe;
-        sc->superio->serial[1]->ioport_id = data << 2;
+        sc->superio.serial[1]->ioport_id = data << 2;
         if (memory_region_is_mapped(sc->serial_io[1])) {
             memory_region_set_address(sc->serial_io[1], data << 2);
         }
@@ -350,25 +427,9 @@ static void superio_cfg_write(void *opaque, hwaddr addr, 
uint64_t data,
     sc->regs[idx] = data;
 }

-static uint64_t superio_cfg_read(void *opaque, hwaddr addr, unsigned size)
-{
-    SuperIOConfig *sc = opaque;
-    uint8_t idx = sc->regs[0];
-    uint8_t val = sc->regs[idx];
-
-    if (addr == 0) {
-        return idx;
-    }
-    if (addr == 1 && idx == 0) {
-        val = 0; /* reading reg 0 where we store index value */
-    }
-    trace_via_superio_read(idx, val);
-    return val;
-}
-
-static const MemoryRegionOps superio_cfg_ops = {
-    .read = superio_cfg_read,
-    .write = superio_cfg_write,
+static const MemoryRegionOps vt82c686b_superio_cfg_ops = {
+    .read = via_superio_cfg_read,
+    .write = vt82c686b_superio_cfg_write,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .impl = {
         .min_access_size = 1,
@@ -376,13 +437,66 @@ static const MemoryRegionOps superio_cfg_ops = {
     },
 };

+static void vt82c686b_superio_reset(DeviceState *dev)
+{
+    ViaSuperIOState *s = VIA_SUPERIO(dev);
+
+    memset(s->regs, 0, sizeof(s->regs));
+    /* Device ID */
+    vt82c686b_superio_cfg_write(s, 0, 0xe0, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0x3c, 1);
+    /* Function select - all disabled */
+    vt82c686b_superio_cfg_write(s, 0, 0xe2, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0x03, 1);
+    /* Floppy ctrl base addr */
+    vt82c686b_superio_cfg_write(s, 0, 0xe3, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0xfc, 1);
+    /* Parallel port base addr */
+    vt82c686b_superio_cfg_write(s, 0, 0xe6, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0xde, 1);
+    /* Serial port 1 base addr */
+    vt82c686b_superio_cfg_write(s, 0, 0xe7, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0xfe, 1);
+    /* Serial port 2 base addr */
+    vt82c686b_superio_cfg_write(s, 0, 0xe8, 1);
+    vt82c686b_superio_cfg_write(s, 1, 0xbe, 1);
+
+    vt82c686b_superio_cfg_write(s, 0, 0, 1);
+}
+
+static void vt82c686b_superio_init(Object *obj)
+{
+    VIA_SUPERIO(obj)->io_ops = &vt82c686b_superio_cfg_ops;
+}
+
+static void vt82c686b_superio_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
+
+    dc->reset = vt82c686b_superio_reset;
+    sc->serial.count = 2;
+    sc->parallel.count = 1;
+    sc->ide.count = 0; /* emulated by via-ide */
+    sc->floppy.count = 1;
+}
+
+static const TypeInfo vt82c686b_superio_info = {
+    .name          = TYPE_VT82C686B_SUPERIO,
+    .parent        = TYPE_VIA_SUPERIO,
+    .instance_size = sizeof(ViaSuperIOState),
+    .instance_init = vt82c686b_superio_init,
+    .class_size    = sizeof(ISASuperIOClass),
+    .class_init    = vt82c686b_superio_class_init,
+};
+

 OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, VT82C686B_ISA)

 struct VT82C686BISAState {
     PCIDevice dev;
     qemu_irq cpu_intr;
-    SuperIOConfig superio_cfg;
+    ViaSuperIOState *via_sio;
 };

 static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
@@ -400,7 +514,7 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t 
addr,
     pci_default_write_config(d, addr, val, len);
     if (addr == 0x85) {
         /* BIT(1): enable or disable superio config io ports */
-        memory_region_set_enabled(&s->superio_cfg.io, val & BIT(1));
+        via_superio_io_enable(s->via_sio, val & BIT(1));
     }
 }

@@ -432,13 +546,6 @@ static void vt82c686b_isa_reset(DeviceState *dev)
     pci_conf[0x5a] = 0x04; /* KBC/RTC Control*/
     pci_conf[0x5f] = 0x04;
     pci_conf[0x77] = 0x10; /* GPIO Control 1/2/3/4 */
-
-    s->superio_cfg.regs[0xe0] = 0x3c; /* Device ID */
-    s->superio_cfg.regs[0xe2] = 0x03; /* Function select */
-    s->superio_cfg.regs[0xe3] = 0xfc; /* Floppy ctrl base addr */
-    s->superio_cfg.regs[0xe6] = 0xde; /* Parallel port base addr */
-    s->superio_cfg.regs[0xe7] = 0xfe; /* Serial port 1 base addr */
-    s->superio_cfg.regs[0xe8] = 0xbe; /* Serial port 2 base addr */
 }

 static void vt82c686b_realize(PCIDevice *d, Error **errp)
@@ -447,7 +554,6 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     DeviceState *dev = DEVICE(d);
     ISABus *isa_bus;
     qemu_irq *isa_irq;
-    ISASuperIOClass *ic;
     int i;

     qdev_init_gpio_out(dev, &s->cpu_intr, 1);
@@ -457,9 +563,8 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
     isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
     i8254_pit_init(isa_bus, 0x40, 0, NULL);
     i8257_dma_init(isa_bus, 0);
-    s->superio_cfg.superio = ISA_SUPERIO(isa_create_simple(isa_bus,
-                                                      TYPE_VT82C686B_SUPERIO));
-    ic = ISA_SUPERIO_GET_CLASS(s->superio_cfg.superio);
+    s->via_sio = VIA_SUPERIO(isa_create_simple(isa_bus,
+                                               TYPE_VT82C686B_SUPERIO));
     mc146818_rtc_init(isa_bus, 2000, NULL);

     for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
@@ -467,31 +572,6 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
             d->wmask[i] = 0;
         }
     }
-
-    memory_region_init_io(&s->superio_cfg.io, OBJECT(d), &superio_cfg_ops,
-                          &s->superio_cfg, "superio_cfg", 2);
-    memory_region_set_enabled(&s->superio_cfg.io, false);
-    /*
-     * The floppy also uses 0x3f0 and 0x3f1.
-     * But we do not emulate a floppy, so just set it here.
-     */
-    memory_region_add_subregion(isa_bus->address_space_io, 0x3f0,
-                                &s->superio_cfg.io);
-
-    /* Grab io regions of serial devices so we can control them */
-    for (i = 0; i < ic->serial.count; i++) {
-        ISADevice *sd = s->superio_cfg.superio->serial[i];
-        MemoryRegion *io = isa_address_space_io(sd);
-        MemoryRegion *mr = find_subregion(sd, io, sd->ioport_id);
-        if (!mr) {
-            error_setg(errp, "Could not get io region for serial %d", i);
-            return;
-        }
-        s->superio_cfg.serial_io[i] = mr;
-        if (memory_region_is_mapped(mr)) {
-            memory_region_del_subregion(io, mr);
-        }
-    }
 }

 static void via_class_init(ObjectClass *klass, void *data)
@@ -527,32 +607,14 @@ static const TypeInfo via_info = {
 };


-static void vt82c686b_superio_class_init(ObjectClass *klass, void *data)
-{
-    ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
-
-    sc->serial.count = 2;
-    sc->parallel.count = 1;
-    sc->ide.count = 0;
-    sc->floppy.count = 1;
-}
-
-static const TypeInfo via_superio_info = {
-    .name          = TYPE_VT82C686B_SUPERIO,
-    .parent        = TYPE_ISA_SUPERIO,
-    .instance_size = sizeof(ISASuperIODevice),
-    .class_size    = sizeof(ISASuperIOClass),
-    .class_init    = vt82c686b_superio_class_init,
-};
-
-
 static void vt82c686b_register_types(void)
 {
     type_register_static(&via_pm_info);
     type_register_static(&vt82c686b_pm_info);
     type_register_static(&vt8231_pm_info);
-    type_register_static(&via_info);
     type_register_static(&via_superio_info);
+    type_register_static(&vt82c686b_superio_info);
+    type_register_static(&via_info);
 }

 type_init(vt82c686b_register_types)
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 9b6d610e83..0692b9a527 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -2,7 +2,6 @@
 #define HW_VT82C686_H

 #define TYPE_VT82C686B_ISA "vt82c686b-isa"
-#define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
 #define TYPE_VT82C686B_PM "vt82c686b-pm"
 #define TYPE_VT8231_PM "vt8231-pm"
 #define TYPE_VIA_AC97 "via-ac97"
reply via email to
[Prev in Thread] Current Thread [Next in Thread]