[Qemu-devel] [PATCH 0/3] remove sysbus_init_mmio_cb2

[Qemu-devel] [PATCH 0/3] remove sysbus_init_mmio_cb2

[Benoît Canet]

Remove the last two users of this function then
remove the function's code from sysbus.

Tested with Renesas r2d image and buildroot for
the ppc board.

Benoît Canet (3):
  sh_pci: remove sysbus_init_mmio_cb2 usage
  ppce500_pci: remove sysbus_init_mmio_cb2 usage
  sysbus: remove sysbus_init_mmio_cb2

 hw/ppce500_mpc8544ds.c |   14 ++++++++++----
 hw/ppce500_pci.c       |   27 ++++++---------------------
 hw/r2d.c               |   14 ++++++++++++--
 hw/sh_pci.c            |   29 ++++-------------------------
 hw/sysbus.c            |   16 ----------------
 hw/sysbus.h            |    5 -----
 6 files changed, 32 insertions(+), 73 deletions(-)

-- 
1.7.7.3

[Qemu-devel] [PATCH 1/3] sh_pci: remove sysbus_init_mmio_cb2 usage

[Benoît Canet]

The isa region is not exposed as a sysbus region because the iobr
register contains its address and use it to remap dynamically
the region. (Peter Maydell's idea)

Signed-off-by: Benoît Canet <benoit.canet@gmail.com>
---
 hw/r2d.c    |   14 ++++++++++++--
 hw/sh_pci.c |   29 ++++-------------------------
 2 files changed, 16 insertions(+), 27 deletions(-)

diff --git a/hw/r2d.c b/hw/r2d.c
index 9b6fcba..6e1f71c 100644
--- a/hw/r2d.c
+++ b/hw/r2d.c
@@ -231,6 +231,8 @@ static void r2d_init(ram_addr_t ram_size,
     qemu_irq *irq;
     DriveInfo *dinfo;
     int i;
+    DeviceState *dev;
+    SysBusDevice *busdev;
     MemoryRegion *address_space_mem = get_system_memory();
 
     if (!cpu_model)
@@ -252,8 +254,16 @@ static void r2d_init(ram_addr_t ram_size,
     /* Register peripherals */
     s = sh7750_init(env, address_space_mem);
     irq = r2d_fpga_init(address_space_mem, 0x04000000, sh7750_irl(s));
-    sysbus_create_varargs("sh_pci", 0x1e200000, irq[PCI_INTA], irq[PCI_INTB],
-                          irq[PCI_INTC], irq[PCI_INTD], NULL);
+
+    dev = qdev_create(NULL, "sh_pci");
+    busdev = sysbus_from_qdev(dev);
+    qdev_init_nofail(dev);
+    sysbus_mmio_map(busdev, 0, P4ADDR(0x1e200000));
+    sysbus_mmio_map(busdev, 1, A7ADDR(0x1e200000));
+    sysbus_connect_irq(busdev, 0, irq[PCI_INTA]);
+    sysbus_connect_irq(busdev, 1, irq[PCI_INTB]);
+    sysbus_connect_irq(busdev, 2, irq[PCI_INTC]);
+    sysbus_connect_irq(busdev, 3, irq[PCI_INTD]);
 
     sm501_init(address_space_mem, 0x10000000, SM501_VRAM_SIZE,
                irq[SM501], serial_hds[2]);
diff --git a/hw/sh_pci.c b/hw/sh_pci.c
index 86f468e..d4d028d 100644
--- a/hw/sh_pci.c
+++ b/hw/sh_pci.c
@@ -110,29 +110,6 @@ static void sh_pci_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(pic[irq_num], level);
 }
 
-static void sh_pci_map(SysBusDevice *dev, target_phys_addr_t base)
-{
-    SHPCIState *s = FROM_SYSBUS(SHPCIState, dev);
-
-    memory_region_add_subregion(get_system_memory(),
-                                P4ADDR(base),
-                                &s->memconfig_p4);
-    memory_region_add_subregion(get_system_memory(),
-                                A7ADDR(base),
-                                &s->memconfig_a7);
-    s->iobr = 0xfe240000;
-    memory_region_add_subregion(get_system_memory(), s->iobr, &s->isa);
-}
-
-static void sh_pci_unmap(SysBusDevice *dev, target_phys_addr_t base)
-{
-    SHPCIState *s = FROM_SYSBUS(SHPCIState, dev);
-
-    memory_region_del_subregion(get_system_memory(), &s->memconfig_p4);
-    memory_region_del_subregion(get_system_memory(), &s->memconfig_a7);
-    memory_region_del_subregion(get_system_memory(), &s->isa);
-}
-
 static int sh_pci_init_device(SysBusDevice *dev)
 {
     SHPCIState *s;
@@ -153,9 +130,11 @@ static int sh_pci_init_device(SysBusDevice *dev)
     memory_region_init_alias(&s->memconfig_a7, "sh_pci.2", &s->memconfig_p4,
                              0, 0x224);
     isa_mmio_setup(&s->isa, 0x40000);
-    sysbus_init_mmio_cb2(dev, sh_pci_map, sh_pci_unmap);
+    sysbus_init_mmio(dev, &s->memconfig_p4);
     sysbus_init_mmio(dev, &s->memconfig_a7);
-    sysbus_init_mmio(dev, &s->isa);
+    s->iobr = 0xfe240000;
+    memory_region_add_subregion(get_system_memory(), s->iobr, &s->isa);
+
     s->dev = pci_create_simple(s->bus, PCI_DEVFN(0, 0), "sh_pci_host");
     return 0;
 }
-- 
1.7.7.3

[Qemu-devel] [PATCH 2/3] ppce500_pci: remove sysbus_init_mmio_cb2 usage

[Benoît Canet]

Expose only one container MemoryRegion to sysbus.
(Peter Maydell's idea)

Signed-off-by: Benoît Canet <benoit.canet@gmail.com>
---
 hw/ppce500_mpc8544ds.c |   14 ++++++++++----
 hw/ppce500_pci.c       |   27 ++++++---------------------
 2 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/hw/ppce500_mpc8544ds.c b/hw/ppce500_mpc8544ds.c
index 51b6abd..66c935e 100644
--- a/hw/ppce500_mpc8544ds.c
+++ b/hw/ppce500_mpc8544ds.c
@@ -244,6 +244,7 @@ static void mpc8544ds_init(ram_addr_t ram_size,
     unsigned int pci_irq_nrs[4] = {1, 2, 3, 4};
     qemu_irq **irqs, *mpic;
     DeviceState *dev;
+    SysBusDevice *busdev;
     CPUState *firstenv = NULL;
 
     /* Setup CPUs */
@@ -320,10 +321,15 @@ static void mpc8544ds_init(ram_addr_t ram_size,
     sysbus_create_simple("mpc8544-guts", MPC8544_UTIL_BASE, NULL);
 
     /* PCI */
-    dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
-                                mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
-                                mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
-                                NULL);
+    dev = qdev_create(NULL, "e500-pcihost");
+    busdev = sysbus_from_qdev(dev);
+    qdev_init_nofail(dev);
+    sysbus_mmio_map(busdev, 0, MPC8544_PCI_REGS_BASE);
+    sysbus_connect_irq(busdev, 0, mpic[pci_irq_nrs[0]]);
+    sysbus_connect_irq(busdev, 1, mpic[pci_irq_nrs[1]]);
+    sysbus_connect_irq(busdev, 2, mpic[pci_irq_nrs[2]]);
+    sysbus_connect_irq(busdev, 3, mpic[pci_irq_nrs[3]]);
+
     pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
     if (!pci_bus)
         printf("couldn't create PCI controller!\n");
diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
index 6232af1..b606206 100644
--- a/hw/ppce500_pci.c
+++ b/hw/ppce500_pci.c
@@ -79,6 +79,7 @@ struct PPCE500PCIState {
     uint32_t gasket_time;
     qemu_irq irq[4];
     /* mmio maps */
+    MemoryRegion container;
     MemoryRegion iomem;
 };
 
@@ -298,26 +299,6 @@ static const VMStateDescription vmstate_ppce500_pci = {
     }
 };
 
-static void e500_pci_map(SysBusDevice *dev, target_phys_addr_t base)
-{
-    PCIHostState *h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
-    PPCE500PCIState *s = DO_UPCAST(PPCE500PCIState, pci_state, h);
-
-    sysbus_add_memory(dev, base + PCIE500_CFGADDR, &h->conf_mem);
-    sysbus_add_memory(dev, base + PCIE500_CFGDATA, &h->data_mem);
-    sysbus_add_memory(dev, base + PCIE500_REG_BASE, &s->iomem);
-}
-
-static void e500_pci_unmap(SysBusDevice *dev, target_phys_addr_t base)
-{
-    PCIHostState *h = FROM_SYSBUS(PCIHostState, sysbus_from_qdev(dev));
-    PPCE500PCIState *s = DO_UPCAST(PPCE500PCIState, pci_state, h);
-
-    sysbus_del_memory(dev, &h->conf_mem);
-    sysbus_del_memory(dev, &h->data_mem);
-    sysbus_del_memory(dev, &s->iomem);
-}
-
 #include "exec-memory.h"
 
 static int e500_pcihost_initfn(SysBusDevice *dev)
@@ -343,13 +324,17 @@ static int e500_pcihost_initfn(SysBusDevice *dev)
 
     pci_create_simple(b, 0, "e500-host-bridge");
 
+    memory_region_init(&s->container, "pci-container", PCIE500_ALL_SIZE);
     memory_region_init_io(&h->conf_mem, &pci_host_conf_be_ops, h,
                           "pci-conf-idx", 4);
     memory_region_init_io(&h->data_mem, &pci_host_data_le_ops, h,
                           "pci-conf-data", 4);
     memory_region_init_io(&s->iomem, &e500_pci_reg_ops, s,
                           "pci.reg", PCIE500_REG_SIZE);
-    sysbus_init_mmio_cb2(dev, e500_pci_map, e500_pci_unmap);
+    memory_region_add_subregion(&s->container, PCIE500_CFGADDR, &h->conf_mem);
+    memory_region_add_subregion(&s->container, PCIE500_CFGDATA, &h->data_mem);
+    memory_region_add_subregion(&s->container, PCIE500_REG_BASE, &s->iomem);
+    sysbus_init_mmio(dev, &s->container);
 
     return 0;
 }
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH 2/3] ppce500_pci: remove sysbus_init_mmio_cb2 usage

[Peter Maydell]

On 2 December 2011 14:51, Benoît Canet <benoit.canet@gmail.com> wrote:
> @@ -320,10 +321,15 @@ static void mpc8544ds_init(ram_addr_t ram_size,
>     sysbus_create_simple("mpc8544-guts", MPC8544_UTIL_BASE, NULL);
>
>     /* PCI */
> -    dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
> -                                mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
> -                                mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
> -                                NULL);
> +    dev = qdev_create(NULL, "e500-pcihost");
> +    busdev = sysbus_from_qdev(dev);
> +    qdev_init_nofail(dev);
> +    sysbus_mmio_map(busdev, 0, MPC8544_PCI_REGS_BASE);
> +    sysbus_connect_irq(busdev, 0, mpic[pci_irq_nrs[0]]);
> +    sysbus_connect_irq(busdev, 1, mpic[pci_irq_nrs[1]]);
> +    sysbus_connect_irq(busdev, 2, mpic[pci_irq_nrs[2]]);
> +    sysbus_connect_irq(busdev, 3, mpic[pci_irq_nrs[3]]);
> +

Is this change actually necessary? If we only have one MMIO region
to map then sysbus_create_varargs() suffices.

How much did you test this patch, by the way? I've been running into
some odd behaviour with sub-page-sized memory regions which I'm
wondering if you hit here...

-- PMM

Re: [Qemu-devel] [PATCH 2/3] ppce500_pci: remove sysbus_init_mmio_cb2 usage

[Benoît Canet]

[-- Attachment #1: Type: text/plain, Size: 439 bytes --]

>
> Is this change actually necessary? If we only have one MMIO region
> to map then sysbus_create_varargs() suffices.
>
> Ack,


> How much did you test this patch, by the way? I've been running into
> some odd behaviour with sub-page-sized memory regions which I'm
> wondering if you hit here...
>
> I build a kernel and an initrd with buildroot and do some pci enumerations.
I'll check if the pci ethernet controller works.

> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 914 bytes --]

Re: [Qemu-devel] [PATCH 2/3] ppce500_pci: remove sysbus_init_mmio_cb2 usage

[Benoît Canet]

[-- Attachment #1: Type: text/plain, Size: 247 bytes --]

>
> How much did you test this patch, by the way? I've been running into
> some odd behaviour with sub-page-sized memory regions which I'm
> wondering if you hit here...


An e1000 attached as a pci card works fine. (ping and wget www.google.com)

[-- Attachment #2: Type: text/html, Size: 461 bytes --]

[Qemu-devel] [PATCH 3/3] sysbus: remove sysbus_init_mmio_cb2

[Benoît Canet]

This function is not longer in use so remove it.

Signed-off-by: Benoît Canet <benoit.canet@gmail.com>
---
 hw/sysbus.c |   16 ----------------
 hw/sysbus.h |    5 -----
 2 files changed, 0 insertions(+), 21 deletions(-)

diff --git a/hw/sysbus.c b/hw/sysbus.c
index b315a8c..81a57bd 100644
--- a/hw/sysbus.c
+++ b/hw/sysbus.c
@@ -53,8 +53,6 @@ void sysbus_mmio_map(SysBusDevice *dev, int n, target_phys_addr_t addr)
         if (dev->mmio[n].memory) {
             memory_region_del_subregion(get_system_memory(),
                                         dev->mmio[n].memory);
-        } else if (dev->mmio[n].unmap) {
-            dev->mmio[n].unmap(dev, dev->mmio[n].addr);
         }
     }
     dev->mmio[n].addr = addr;
@@ -62,8 +60,6 @@ void sysbus_mmio_map(SysBusDevice *dev, int n, target_phys_addr_t addr)
         memory_region_add_subregion(get_system_memory(),
                                     addr,
                                     dev->mmio[n].memory);
-    } else if (dev->mmio[n].cb) {
-        dev->mmio[n].cb(dev, addr);
     }
 }
 
@@ -89,18 +85,6 @@ void sysbus_pass_irq(SysBusDevice *dev, SysBusDevice *target)
     }
 }
 
-void sysbus_init_mmio_cb2(SysBusDevice *dev,
-                          mmio_mapfunc cb, mmio_mapfunc unmap)
-{
-    int n;
-
-    assert(dev->num_mmio < QDEV_MAX_MMIO);
-    n = dev->num_mmio++;
-    dev->mmio[n].addr = -1;
-    dev->mmio[n].cb = cb;
-    dev->mmio[n].unmap = unmap;
-}
-
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory)
 {
     int n;
diff --git a/hw/sysbus.h b/hw/sysbus.h
index 9bac582..2f4025b 100644
--- a/hw/sysbus.h
+++ b/hw/sysbus.h
@@ -11,7 +11,6 @@
 #define QDEV_MAX_IRQ 256
 
 typedef struct SysBusDevice SysBusDevice;
-typedef void (*mmio_mapfunc)(SysBusDevice *dev, target_phys_addr_t addr);
 
 struct SysBusDevice {
     DeviceState qdev;
@@ -21,8 +20,6 @@ struct SysBusDevice {
     int num_mmio;
     struct {
         target_phys_addr_t addr;
-        mmio_mapfunc cb;
-        mmio_mapfunc unmap;
         MemoryRegion *memory;
     } mmio[QDEV_MAX_MMIO];
     int num_pio;
@@ -43,8 +40,6 @@ typedef struct {
 void sysbus_register_dev(const char *name, size_t size, sysbus_initfn init);
 void sysbus_register_withprop(SysBusDeviceInfo *info);
 void *sysbus_new(void);
-void sysbus_init_mmio_cb2(SysBusDevice *dev,
-                          mmio_mapfunc cb, mmio_mapfunc unmap);
 void sysbus_init_mmio(SysBusDevice *dev, MemoryRegion *memory);
 MemoryRegion *sysbus_mmio_get_region(SysBusDevice *dev, int n);
 void sysbus_init_irq(SysBusDevice *dev, qemu_irq *p);
-- 
1.7.7.3