[Qemu-devel] [PATCH 0/5] Refactor common PCI IDE functions from CMD646

[Qemu-devel] [PATCH 0/5] Refactor common PCI IDE functions from CMD646

[BALATON Zoltan]

Hello,

This series is a small refactoring that moves some common PCI IDE io
mem ops functions from the CMD646 model to the PCI IDE model so a
CMD646 specific type can be dropped from PCIIDEState, removes code
duplication from SiI3112 model (also fixing a bug) and allows these
functions to be used by future PCI IDE implementations.

Also cc'd Mark and Richard because the CMD646 seems to be used by
Sparc, HPPA and Alpha machines.

Regards,
BALATON Zoltan

BALATON Zoltan (5):
  cmd646: Remove unused variable
  cmd646: Remove IDEBus from CMD646BAR
  cmd646: Move PCI IDE specific functions to ide/pci.c
  ide: Get rid of CMD646BAR struct
  sii3112: Remove duplicated code and use PCI IDE ops instead

 hw/ide/cmd646.c      | 102 ++++++++-------------------------------------------
 hw/ide/pci.c         |  65 ++++++++++++++++++++++++++++++++
 hw/ide/sii3112.c     |  52 ++++----------------------
 include/hw/ide/pci.h |  14 ++-----
 4 files changed, 93 insertions(+), 140 deletions(-)

-- 
2.13.7

[Qemu-devel] [PATCH 4/5] ide: Get rid of CMD646BAR struct

[BALATON Zoltan]

Now that no CMD646 specific parts are left in CMD646BAR (all remaining
members are really PCI IDE specific) this struct can be deleted moving
the memory regions for PCI IDE BARs to PCIIDEState where they better
belong. The CMD646 PCI IDE model is adjusted accordingly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/cmd646.c      | 33 ++++++++++++++++-----------------
 include/hw/ide/pci.h | 10 ++--------
 2 files changed, 18 insertions(+), 25 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 95f0df9742..5a5679134a 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -50,17 +50,6 @@
 
 static void cmd646_update_irq(PCIDevice *pd);
 
-static void setup_cmd646_bar(PCIIDEState *d, int bus_num)
-{
-    IDEBus *bus = &d->bus[bus_num];
-    CMD646BAR *bar = &d->cmd646_bar[bus_num];
-
-    memory_region_init_io(&bar->cmd, OBJECT(d), &pci_ide_cmd_le_ops, bus,
-                          "cmd646-cmd", 4);
-    memory_region_init_io(&bar->data, OBJECT(d), &pci_ide_data_le_ops, bus,
-                          "cmd646-data", 8);
-}
-
 static void cmd646_update_dma_interrupts(PCIDevice *pd)
 {
     /* Sync DMA interrupt status from UDMA interrupt status */
@@ -277,12 +266,22 @@ static void pci_cmd646_ide_realize(PCIDevice *dev, Error **errp)
     dev->wmask[MRDMODE] = 0x0;
     dev->w1cmask[MRDMODE] = MRDMODE_INTR_CH0 | MRDMODE_INTR_CH1;
 
-    setup_cmd646_bar(d, 0);
-    setup_cmd646_bar(d, 1);
-    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd646_bar[0].data);
-    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd646_bar[0].cmd);
-    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd646_bar[1].data);
-    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd646_bar[1].cmd);
+    memory_region_init_io(&d->data_bar[0], OBJECT(d), &pci_ide_data_le_ops,
+                          &d->bus[0], "cmd646-data0", 8);
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[0]);
+
+    memory_region_init_io(&d->cmd_bar[0], OBJECT(d), &pci_ide_cmd_le_ops,
+                          &d->bus[0], "cmd646-cmd0", 4);
+    pci_register_bar(dev, 1, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[0]);
+
+    memory_region_init_io(&d->data_bar[1], OBJECT(d), &pci_ide_data_le_ops,
+                          &d->bus[1], "cmd646-data1", 8);
+    pci_register_bar(dev, 2, PCI_BASE_ADDRESS_SPACE_IO, &d->data_bar[1]);
+
+    memory_region_init_io(&d->cmd_bar[1], OBJECT(d), &pci_ide_cmd_le_ops,
+                          &d->bus[1], "cmd646-cmd1", 4);
+    pci_register_bar(dev, 3, PCI_BASE_ADDRESS_SPACE_IO, &d->cmd_bar[1]);
+
     bmdma_setup_bar(d);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 3110633e4c..a9f2c33e68 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -37,11 +37,6 @@ typedef struct BMDMAState {
     struct PCIIDEState *pci_dev;
 } BMDMAState;
 
-typedef struct CMD646BAR {
-    MemoryRegion cmd;
-    MemoryRegion data;
-} CMD646BAR;
-
 #define TYPE_PCI_IDE "pci-ide"
 #define PCI_IDE(obj) OBJECT_CHECK(PCIIDEState, (obj), TYPE_PCI_IDE)
 
@@ -54,17 +49,16 @@ typedef struct PCIIDEState {
     BMDMAState bmdma[2];
     uint32_t secondary; /* used only for cmd646 */
     MemoryRegion bmdma_bar;
-    CMD646BAR cmd646_bar[2]; /* used only for cmd646 */
+    MemoryRegion cmd_bar[2];
+    MemoryRegion data_bar[2];
 } PCIIDEState;
 
-
 static inline IDEState *bmdma_active_if(BMDMAState *bmdma)
 {
     assert(bmdma->bus->retry_unit != (uint8_t)-1);
     return bmdma->bus->ifs + bmdma->bus->retry_unit;
 }
 
-
 void bmdma_init(IDEBus *bus, BMDMAState *bm, PCIIDEState *d);
 void bmdma_cmd_writeb(BMDMAState *bm, uint32_t val);
 extern MemoryRegionOps bmdma_addr_ioport_ops;
-- 
2.13.7

[Qemu-devel] [PATCH 1/5] cmd646: Remove unused variable

[BALATON Zoltan]

There was a pointer to PCIIDEState in CMD646BAR which was set but
not used afterwards. Get rid of this unused variable.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/cmd646.c      | 1 -
 include/hw/ide/pci.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 6bb92d717f..41c1831f9a 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -123,7 +123,6 @@ static void setup_cmd646_bar(PCIIDEState *d, int bus_num)
     CMD646BAR *bar = &d->cmd646_bar[bus_num];
 
     bar->bus = bus;
-    bar->pci_dev = d;
     memory_region_init_io(&bar->cmd, OBJECT(d), &cmd646_cmd_ops, bar,
                           "cmd646-cmd", 4);
     memory_region_init_io(&bar->data, OBJECT(d), &cmd646_data_ops, bar,
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index dbc6a0383d..ed723acfb4 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -41,7 +41,6 @@ typedef struct CMD646BAR {
     MemoryRegion cmd;
     MemoryRegion data;
     IDEBus *bus;
-    struct PCIIDEState *pci_dev;
 } CMD646BAR;
 
 #define TYPE_PCI_IDE "pci-ide"
-- 
2.13.7

[Qemu-devel] [PATCH 2/5] cmd646: Remove IDEBus from CMD646BAR

[BALATON Zoltan]

The cmd646 io mem ops callbacks only need the IDEBus which is
currently passed via a CMD646BAR struct. No need to wrap it up like
that, we can pass it directly to these callbacks which then allows to
drop the IDEBus from the CMD646BAR.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/cmd646.c      | 29 ++++++++++++++---------------
 include/hw/ide/pci.h |  1 -
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index 41c1831f9a..c24f71e219 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -53,23 +53,23 @@ static void cmd646_update_irq(PCIDevice *pd);
 static uint64_t cmd646_cmd_read(void *opaque, hwaddr addr,
                                 unsigned size)
 {
-    CMD646BAR *cmd646bar = opaque;
+    IDEBus *bus = opaque;
 
     if (addr != 2 || size != 1) {
         return ((uint64_t)1 << (size * 8)) - 1;
     }
-    return ide_status_read(cmd646bar->bus, addr + 2);
+    return ide_status_read(bus, addr + 2);
 }
 
 static void cmd646_cmd_write(void *opaque, hwaddr addr,
                              uint64_t data, unsigned size)
 {
-    CMD646BAR *cmd646bar = opaque;
+    IDEBus *bus = opaque;
 
     if (addr != 2 || size != 1) {
         return;
     }
-    ide_cmd_write(cmd646bar->bus, addr + 2, data);
+    ide_cmd_write(bus, addr + 2, data);
 }
 
 static const MemoryRegionOps cmd646_cmd_ops = {
@@ -81,15 +81,15 @@ static const MemoryRegionOps cmd646_cmd_ops = {
 static uint64_t cmd646_data_read(void *opaque, hwaddr addr,
                                  unsigned size)
 {
-    CMD646BAR *cmd646bar = opaque;
+    IDEBus *bus = opaque;
 
     if (size == 1) {
-        return ide_ioport_read(cmd646bar->bus, addr);
+        return ide_ioport_read(bus, addr);
     } else if (addr == 0) {
         if (size == 2) {
-            return ide_data_readw(cmd646bar->bus, addr);
+            return ide_data_readw(bus, addr);
         } else {
-            return ide_data_readl(cmd646bar->bus, addr);
+            return ide_data_readl(bus, addr);
         }
     }
     return ((uint64_t)1 << (size * 8)) - 1;
@@ -98,15 +98,15 @@ static uint64_t cmd646_data_read(void *opaque, hwaddr addr,
 static void cmd646_data_write(void *opaque, hwaddr addr,
                              uint64_t data, unsigned size)
 {
-    CMD646BAR *cmd646bar = opaque;
+    IDEBus *bus = opaque;
 
     if (size == 1) {
-        ide_ioport_write(cmd646bar->bus, addr, data);
+        ide_ioport_write(bus, addr, data);
     } else if (addr == 0) {
         if (size == 2) {
-            ide_data_writew(cmd646bar->bus, addr, data);
+            ide_data_writew(bus, addr, data);
         } else {
-            ide_data_writel(cmd646bar->bus, addr, data);
+            ide_data_writel(bus, addr, data);
         }
     }
 }
@@ -122,10 +122,9 @@ static void setup_cmd646_bar(PCIIDEState *d, int bus_num)
     IDEBus *bus = &d->bus[bus_num];
     CMD646BAR *bar = &d->cmd646_bar[bus_num];
 
-    bar->bus = bus;
-    memory_region_init_io(&bar->cmd, OBJECT(d), &cmd646_cmd_ops, bar,
+    memory_region_init_io(&bar->cmd, OBJECT(d), &cmd646_cmd_ops, bus,
                           "cmd646-cmd", 4);
-    memory_region_init_io(&bar->data, OBJECT(d), &cmd646_data_ops, bar,
+    memory_region_init_io(&bar->data, OBJECT(d), &cmd646_data_ops, bus,
                           "cmd646-data", 8);
 }
 
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index ed723acfb4..013d7937d2 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -40,7 +40,6 @@ typedef struct BMDMAState {
 typedef struct CMD646BAR {
     MemoryRegion cmd;
     MemoryRegion data;
-    IDEBus *bus;
 } CMD646BAR;
 
 #define TYPE_PCI_IDE "pci-ide"
-- 
2.13.7

[Qemu-devel] [PATCH 3/5] cmd646: Move PCI IDE specific functions to ide/pci.c

[BALATON Zoltan]

The io mem ops callbacks are not specific to CMD646 but really follow
the PCI IDE spec so move these from cmd646.c to pci.c to allow other
PCI IDE implementations to use them.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/cmd646.c      | 71 ++--------------------------------------------------
 hw/ide/pci.c         | 65 +++++++++++++++++++++++++++++++++++++++++++++++
 include/hw/ide/pci.h |  2 ++
 3 files changed, 69 insertions(+), 69 deletions(-)

diff --git a/hw/ide/cmd646.c b/hw/ide/cmd646.c
index c24f71e219..95f0df9742 100644
--- a/hw/ide/cmd646.c
+++ b/hw/ide/cmd646.c
@@ -50,81 +50,14 @@
 
 static void cmd646_update_irq(PCIDevice *pd);
 
-static uint64_t cmd646_cmd_read(void *opaque, hwaddr addr,
-                                unsigned size)
-{
-    IDEBus *bus = opaque;
-
-    if (addr != 2 || size != 1) {
-        return ((uint64_t)1 << (size * 8)) - 1;
-    }
-    return ide_status_read(bus, addr + 2);
-}
-
-static void cmd646_cmd_write(void *opaque, hwaddr addr,
-                             uint64_t data, unsigned size)
-{
-    IDEBus *bus = opaque;
-
-    if (addr != 2 || size != 1) {
-        return;
-    }
-    ide_cmd_write(bus, addr + 2, data);
-}
-
-static const MemoryRegionOps cmd646_cmd_ops = {
-    .read = cmd646_cmd_read,
-    .write = cmd646_cmd_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
-static uint64_t cmd646_data_read(void *opaque, hwaddr addr,
-                                 unsigned size)
-{
-    IDEBus *bus = opaque;
-
-    if (size == 1) {
-        return ide_ioport_read(bus, addr);
-    } else if (addr == 0) {
-        if (size == 2) {
-            return ide_data_readw(bus, addr);
-        } else {
-            return ide_data_readl(bus, addr);
-        }
-    }
-    return ((uint64_t)1 << (size * 8)) - 1;
-}
-
-static void cmd646_data_write(void *opaque, hwaddr addr,
-                             uint64_t data, unsigned size)
-{
-    IDEBus *bus = opaque;
-
-    if (size == 1) {
-        ide_ioport_write(bus, addr, data);
-    } else if (addr == 0) {
-        if (size == 2) {
-            ide_data_writew(bus, addr, data);
-        } else {
-            ide_data_writel(bus, addr, data);
-        }
-    }
-}
-
-static const MemoryRegionOps cmd646_data_ops = {
-    .read = cmd646_data_read,
-    .write = cmd646_data_write,
-    .endianness = DEVICE_LITTLE_ENDIAN,
-};
-
 static void setup_cmd646_bar(PCIIDEState *d, int bus_num)
 {
     IDEBus *bus = &d->bus[bus_num];
     CMD646BAR *bar = &d->cmd646_bar[bus_num];
 
-    memory_region_init_io(&bar->cmd, OBJECT(d), &cmd646_cmd_ops, bus,
+    memory_region_init_io(&bar->cmd, OBJECT(d), &pci_ide_cmd_le_ops, bus,
                           "cmd646-cmd", 4);
-    memory_region_init_io(&bar->data, OBJECT(d), &cmd646_data_ops, bus,
+    memory_region_init_io(&bar->data, OBJECT(d), &pci_ide_data_le_ops, bus,
                           "cmd646-data", 8);
 }
 
diff --git a/hw/ide/pci.c b/hw/ide/pci.c
index b75154f99f..942613a9a9 100644
--- a/hw/ide/pci.c
+++ b/hw/ide/pci.c
@@ -36,6 +36,71 @@
         (IDE_RETRY_DMA | IDE_RETRY_PIO | \
         IDE_RETRY_READ | IDE_RETRY_FLUSH)
 
+static uint64_t pci_ide_cmd_read(void *opaque, hwaddr addr, unsigned size)
+{
+    IDEBus *bus = opaque;
+
+    if (addr != 2 || size != 1) {
+        return ((uint64_t)1 << (size * 8)) - 1;
+    }
+    return ide_status_read(bus, addr + 2);
+}
+
+static void pci_ide_cmd_write(void *opaque, hwaddr addr,
+                              uint64_t data, unsigned size)
+{
+    IDEBus *bus = opaque;
+
+    if (addr != 2 || size != 1) {
+        return;
+    }
+    ide_cmd_write(bus, addr + 2, data);
+}
+
+const MemoryRegionOps pci_ide_cmd_le_ops = {
+    .read = pci_ide_cmd_read,
+    .write = pci_ide_cmd_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
+static uint64_t pci_ide_data_read(void *opaque, hwaddr addr, unsigned size)
+{
+    IDEBus *bus = opaque;
+
+    if (size == 1) {
+        return ide_ioport_read(bus, addr);
+    } else if (addr == 0) {
+        if (size == 2) {
+            return ide_data_readw(bus, addr);
+        } else {
+            return ide_data_readl(bus, addr);
+        }
+    }
+    return ((uint64_t)1 << (size * 8)) - 1;
+}
+
+static void pci_ide_data_write(void *opaque, hwaddr addr,
+                               uint64_t data, unsigned size)
+{
+    IDEBus *bus = opaque;
+
+    if (size == 1) {
+        ide_ioport_write(bus, addr, data);
+    } else if (addr == 0) {
+        if (size == 2) {
+            ide_data_writew(bus, addr, data);
+        } else {
+            ide_data_writel(bus, addr, data);
+        }
+    }
+}
+
+const MemoryRegionOps pci_ide_data_le_ops = {
+    .read = pci_ide_data_read,
+    .write = pci_ide_data_write,
+    .endianness = DEVICE_LITTLE_ENDIAN,
+};
+
 static void bmdma_start_dma(IDEDMA *dma, IDEState *s,
                             BlockCompletionFunc *dma_cb)
 {
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index 013d7937d2..3110633e4c 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -71,4 +71,6 @@ extern MemoryRegionOps bmdma_addr_ioport_ops;
 void pci_ide_create_devs(PCIDevice *dev, DriveInfo **hd_table);
 
 extern const VMStateDescription vmstate_ide_pci;
+extern const MemoryRegionOps pci_ide_cmd_le_ops;
+extern const MemoryRegionOps pci_ide_data_le_ops;
 #endif
-- 
2.13.7

[Qemu-devel] [PATCH 5/5] sii3112: Remove duplicated code and use PCI IDE ops instead

[BALATON Zoltan]

Parts of the SiI3112 mmio are identical to PCI IDE registers so we can
use the corresponding functions that were factored out into ide/pci.c.
This removes code duplication and simplifies the SiI3112 model which
also helped to spot a copy paste error where reading status of the
2nd channel read the 1st channel instead. This is also fixed here.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/sii3112.c | 52 ++++++++--------------------------------------------
 1 file changed, 8 insertions(+), 44 deletions(-)

diff --git a/hw/ide/sii3112.c b/hw/ide/sii3112.c
index 743a50ed51..59db09cfe4 100644
--- a/hw/ide/sii3112.c
+++ b/hw/ide/sii3112.c
@@ -88,35 +88,19 @@ static uint64_t sii3112_reg_read(void *opaque, hwaddr addr,
         val |= (uint32_t)d->i.bmdma[1].status << 16;
         break;
     case 0x80 ... 0x87:
-        if (size == 1) {
-            val = ide_ioport_read(&d->i.bus[0], addr - 0x80);
-        } else if (addr == 0x80) {
-            val = (size == 2) ? ide_data_readw(&d->i.bus[0], 0) :
-                                ide_data_readl(&d->i.bus[0], 0);
-        } else {
-            val = (1ULL << (size * 8)) - 1;
-        }
+        val = pci_ide_data_le_ops.read(&d->i.bus[0], addr - 0x80, size);
         break;
     case 0x8a:
-        val = (size == 1) ? ide_status_read(&d->i.bus[0], 4) :
-                            (1ULL << (size * 8)) - 1;
+        val = pci_ide_cmd_le_ops.read(&d->i.bus[0], 2, size);
         break;
     case 0xa0:
         val = d->regs[0].confstat;
         break;
     case 0xc0 ... 0xc7:
-        if (size == 1) {
-            val = ide_ioport_read(&d->i.bus[1], addr - 0xc0);
-        } else if (addr == 0xc0) {
-            val = (size == 2) ? ide_data_readw(&d->i.bus[1], 0) :
-                                ide_data_readl(&d->i.bus[1], 0);
-        } else {
-            val = (1ULL << (size * 8)) - 1;
-        }
+        val = pci_ide_data_le_ops.read(&d->i.bus[1], addr - 0xc0, size);
         break;
     case 0xca:
-        val = (size == 1) ? ide_status_read(&d->i.bus[0], 4) :
-                            (1ULL << (size * 8)) - 1;
+        val = pci_ide_cmd_le_ops.read(&d->i.bus[1], 2, size);
         break;
     case 0xe0:
         val = d->regs[1].confstat;
@@ -186,36 +170,16 @@ static void sii3112_reg_write(void *opaque, hwaddr addr,
         bmdma_addr_ioport_ops.write(&d->i.bmdma[1], addr - 12, val, size);
         break;
     case 0x80 ... 0x87:
-        if (size == 1) {
-            ide_ioport_write(&d->i.bus[0], addr - 0x80, val);
-        } else if (addr == 0x80) {
-            if (size == 2) {
-                ide_data_writew(&d->i.bus[0], 0, val);
-            } else {
-                ide_data_writel(&d->i.bus[0], 0, val);
-            }
-        }
+        pci_ide_data_le_ops.write(&d->i.bus[0], addr - 0x80, val, size);
         break;
     case 0x8a:
-        if (size == 1) {
-            ide_cmd_write(&d->i.bus[0], 4, val);
-        }
+        pci_ide_cmd_le_ops.write(&d->i.bus[0], 2, val, size);
         break;
     case 0xc0 ... 0xc7:
-        if (size == 1) {
-            ide_ioport_write(&d->i.bus[1], addr - 0xc0, val);
-        } else if (addr == 0xc0) {
-            if (size == 2) {
-                ide_data_writew(&d->i.bus[1], 0, val);
-            } else {
-                ide_data_writel(&d->i.bus[1], 0, val);
-            }
-        }
+        pci_ide_data_le_ops.write(&d->i.bus[1], addr - 0xc0, val, size);
         break;
     case 0xca:
-        if (size == 1) {
-            ide_cmd_write(&d->i.bus[1], 4, val);
-        }
+        pci_ide_cmd_le_ops.write(&d->i.bus[1], 2, val, size);
         break;
     case 0x100:
         d->regs[0].scontrol = val & 0xfff;
-- 
2.13.7

Re: [Qemu-devel] [PATCH 0/5] Refactor common PCI IDE functions from CMD646

[John Snow]

On 1/10/19 7:36 PM, BALATON Zoltan wrote:
> Hello,
> 
> This series is a small refactoring that moves some common PCI IDE io
> mem ops functions from the CMD646 model to the PCI IDE model so a
> CMD646 specific type can be dropped from PCIIDEState, removes code
> duplication from SiI3112 model (also fixing a bug) and allows these
> functions to be used by future PCI IDE implementations.
> 
> Also cc'd Mark and Richard because the CMD646 seems to be used by
> Sparc, HPPA and Alpha machines.
> 
> Regards,
> BALATON Zoltan
> 
> BALATON Zoltan (5):
>   cmd646: Remove unused variable
>   cmd646: Remove IDEBus from CMD646BAR
>   cmd646: Move PCI IDE specific functions to ide/pci.c
>   ide: Get rid of CMD646BAR struct
>   sii3112: Remove duplicated code and use PCI IDE ops instead
> 
>  hw/ide/cmd646.c      | 102 ++++++++-------------------------------------------
>  hw/ide/pci.c         |  65 ++++++++++++++++++++++++++++++++
>  hw/ide/sii3112.c     |  52 ++++----------------------
>  include/hw/ide/pci.h |  14 ++-----
>  4 files changed, 93 insertions(+), 140 deletions(-)
> 

Hi, just a note to let you know I just got back from vacation and I will
take a little while to get to this as I unbury.

--js

Re: [Qemu-devel] [PATCH 0/5] Refactor common PCI IDE functions from CMD646

[Mark Cave-Ayland]

On 11/01/2019 00:36, BALATON Zoltan wrote:

> Hello,
> 
> This series is a small refactoring that moves some common PCI IDE io
> mem ops functions from the CMD646 model to the PCI IDE model so a
> CMD646 specific type can be dropped from PCIIDEState, removes code
> duplication from SiI3112 model (also fixing a bug) and allows these
> functions to be used by future PCI IDE implementations.
> 
> Also cc'd Mark and Richard because the CMD646 seems to be used by
> Sparc, HPPA and Alpha machines.
> 
> Regards,
> BALATON Zoltan
> 
> BALATON Zoltan (5):
>   cmd646: Remove unused variable
>   cmd646: Remove IDEBus from CMD646BAR
>   cmd646: Move PCI IDE specific functions to ide/pci.c
>   ide: Get rid of CMD646BAR struct
>   sii3112: Remove duplicated code and use PCI IDE ops instead
> 
>  hw/ide/cmd646.c      | 102 ++++++++-------------------------------------------
>  hw/ide/pci.c         |  65 ++++++++++++++++++++++++++++++++
>  hw/ide/sii3112.c     |  52 ++++----------------------
>  include/hw/ide/pci.h |  14 ++-----
>  4 files changed, 93 insertions(+), 140 deletions(-)

This looks like a good tidy-up to me. I've applied these patches locally and tried a
quick boot test under qemu-system-sparc64 and it seems to work, so:

Tested-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


ATB,

Mark.

Re: [Qemu-devel] [PATCH 0/5] Refactor common PCI IDE functions from CMD646

[John Snow]

On 1/10/19 7:36 PM, BALATON Zoltan wrote:
> Hello,
> 
> This series is a small refactoring that moves some common PCI IDE io
> mem ops functions from the CMD646 model to the PCI IDE model so a
> CMD646 specific type can be dropped from PCIIDEState, removes code
> duplication from SiI3112 model (also fixing a bug) and allows these
> functions to be used by future PCI IDE implementations.
> 
> Also cc'd Mark and Richard because the CMD646 seems to be used by
> Sparc, HPPA and Alpha machines.
> 
> Regards,
> BALATON Zoltan
> 
> BALATON Zoltan (5):
>   cmd646: Remove unused variable
>   cmd646: Remove IDEBus from CMD646BAR
>   cmd646: Move PCI IDE specific functions to ide/pci.c
>   ide: Get rid of CMD646BAR struct
>   sii3112: Remove duplicated code and use PCI IDE ops instead
> 
>  hw/ide/cmd646.c      | 102 ++++++++-------------------------------------------
>  hw/ide/pci.c         |  65 ++++++++++++++++++++++++++++++++
>  hw/ide/sii3112.c     |  52 ++++----------------------
>  include/hw/ide/pci.h |  14 ++-----
>  4 files changed, 93 insertions(+), 140 deletions(-)
> 

Richard, any strong feelings on this? I'm inclined to pull it this weekend.

--js

Re: [Qemu-devel] [PATCH 0/5] Refactor common PCI IDE functions from CMD646

[Richard Henderson]

On 1/21/19 5:44 PM, John Snow wrote:
>> BALATON Zoltan (5):
>>   cmd646: Remove unused variable
>>   cmd646: Remove IDEBus from CMD646BAR
>>   cmd646: Move PCI IDE specific functions to ide/pci.c
>>   ide: Get rid of CMD646BAR struct
>>   sii3112: Remove duplicated code and use PCI IDE ops instead
>>
>>  hw/ide/cmd646.c      | 102 ++++++++-------------------------------------------
>>  hw/ide/pci.c         |  65 ++++++++++++++++++++++++++++++++
>>  hw/ide/sii3112.c     |  52 ++++----------------------
>>  include/hw/ide/pci.h |  14 ++-----
>>  4 files changed, 93 insertions(+), 140 deletions(-)
>>
> 
> Richard, any strong feelings on this? I'm inclined to pull it this weekend.

Please do.


r~

Re: [Qemu-devel] [PATCH 0/5] Refactor common PCI IDE functions from CMD646

[John Snow]

On 1/10/19 7:36 PM, BALATON Zoltan wrote:
> Hello,
> 
> This series is a small refactoring that moves some common PCI IDE io
> mem ops functions from the CMD646 model to the PCI IDE model so a
> CMD646 specific type can be dropped from PCIIDEState, removes code
> duplication from SiI3112 model (also fixing a bug) and allows these
> functions to be used by future PCI IDE implementations.
> 
> Also cc'd Mark and Richard because the CMD646 seems to be used by
> Sparc, HPPA and Alpha machines.
> 
> Regards,
> BALATON Zoltan
> 
> BALATON Zoltan (5):
>   cmd646: Remove unused variable
>   cmd646: Remove IDEBus from CMD646BAR
>   cmd646: Move PCI IDE specific functions to ide/pci.c
>   ide: Get rid of CMD646BAR struct
>   sii3112: Remove duplicated code and use PCI IDE ops instead
> 
>  hw/ide/cmd646.c      | 102 ++++++++-------------------------------------------
>  hw/ide/pci.c         |  65 ++++++++++++++++++++++++++++++++
>  hw/ide/sii3112.c     |  52 ++++----------------------
>  include/hw/ide/pci.h |  14 ++-----
>  4 files changed, 93 insertions(+), 140 deletions(-)
> 

Pushed to my staging branch, thanks.