Re: [PATCH 15/16] hw/pci-host/raven: Do not map regions in init method

[BALATON Zoltan]

On Tue, 3 Jun 2025, Philippe Mathieu-Daudé wrote:

On 4/5/25 18:01, BALATON Zoltan wrote:

Export memory regions as sysbus mmio regions and let the board code
map them.

Why? The mapping belong to the host bridge, not the board...

I took inspiration from grackle that does it the same way.

Regards,
BALATON Zoltan

Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
---
  hw/pci-host/raven.c | 37 ++++++++++++-------------------------
  hw/ppc/prep.c       | 11 +++++++++--
  2 files changed, 21 insertions(+), 27 deletions(-)

diff --git a/hw/pci-host/raven.c b/hw/pci-host/raven.c
index 68d64e3a97..c9df3db401 100644
--- a/hw/pci-host/raven.c
+++ b/hw/pci-host/raven.c
@@ -49,8 +49,6 @@ struct PREPPCIState {
      AddressSpace bm_as;
  };

-#define PCI_IO_BASE_ADDR 0x80000000 /* Physical address on main bus */

-
  static inline uint32_t raven_idsel_to_addr(hwaddr addr)
  {
      return (ctz16(addr >> 11) << 11) | (addr & 0x7ff);

@@ -166,7 +164,7 @@ static void raven_change_gpio(void *opaque, int n, int level)

      memory_region_set_enabled(&s->pci_discontiguous_io, !!level);
  }
  -static void raven_pcihost_realizefn(DeviceState *d, Error **errp)
+static void raven_pcihost_realize(DeviceState *d, Error **errp)
  {
      SysBusDevice *dev = SYS_BUS_DEVICE(d);
      PCIHostState *h = PCI_HOST_BRIDGE(dev);

@@ -176,7 +174,17 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)

        qdev_init_gpio_in(d, raven_change_gpio, 1);
  +    memory_region_init(&s->pci_io, o, "pci-io", 0x3f800000);
+    memory_region_init_io(&s->pci_discontiguous_io, o,
+                          &raven_io_ops, &s->pci_io,
+                          "pci-discontiguous-io", 8 * MiB);
+    memory_region_init(&s->pci_memory, o, "pci-memory", 0x3f000000);
+
+    sysbus_init_mmio(dev, &s->pci_io);
+    sysbus_init_mmio(dev, &s->pci_discontiguous_io);
+    sysbus_init_mmio(dev, &s->pci_memory);
      sysbus_init_irq(dev, &s->irq);
+
      h->bus = pci_register_root_bus(d, NULL, raven_set_irq, raven_map_irq,

&s->irq, &s->pci_memory, &s->pci_io, 0, 1,

                                     TYPE_PCI_BUS);

@@ -215,32 +223,12 @@ static void raven_pcihost_realizefn(DeviceState *d, Error **errp)

      pci_setup_iommu(h->bus, &raven_iommu_ops, s);
  }
  -static void raven_pcihost_initfn(Object *obj)
-{
-    PREPPCIState *s = RAVEN_PCI_HOST_BRIDGE(obj);
-    MemoryRegion *address_space_mem = get_system_memory();
-
-    memory_region_init(&s->pci_io, obj, "pci-io", 0x3f800000);
-    memory_region_init_io(&s->pci_discontiguous_io, obj,
-                          &raven_io_ops, &s->pci_io,
-                          "pci-discontiguous-io", 8 * MiB);
-    memory_region_init(&s->pci_memory, obj, "pci-memory", 0x3f000000);
-
-    /* CPU address space */
-    memory_region_add_subregion(address_space_mem, PCI_IO_BASE_ADDR,
-                                &s->pci_io);

- memory_region_add_subregion_overlap(address_space_mem, PCI_IO_BASE_ADDR,

-                                        &s->pci_discontiguous_io, 1);
-    memory_region_set_enabled(&s->pci_discontiguous_io, false);

- memory_region_add_subregion(address_space_mem, 0xc0000000, &s->pci_memory);

-}
-

static void raven_pcihost_class_init(ObjectClass *klass, const void *data)

  {
      DeviceClass *dc = DEVICE_CLASS(klass);
        set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    dc->realize = raven_pcihost_realizefn;
+    dc->realize = raven_pcihost_realize;
      dc->fw_name = "pci";
  }
  @@ -274,7 +262,6 @@ static const TypeInfo raven_types[] = {
          .name = TYPE_RAVEN_PCI_HOST_BRIDGE,
          .parent = TYPE_PCI_HOST_BRIDGE,
          .instance_size = sizeof(PREPPCIState),
-        .instance_init = raven_pcihost_initfn,
          .class_init = raven_pcihost_class_init,
      },
      {
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index d3365414d2..23d0e1eeaa 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -53,8 +53,11 @@
    #define CFG_ADDR 0xf0000510
  -#define KERNEL_LOAD_ADDR 0x01000000
-#define INITRD_LOAD_ADDR 0x01800000
+#define KERNEL_LOAD_ADDR  0x01000000
+#define INITRD_LOAD_ADDR  0x01800000
+
+#define PCI_IO_BASE_ADDR  0x80000000
+#define PCI_MEM_BASE_ADDR 0xc0000000
    #define BIOS_ADDR         0xfff00000
  #define BIOS_SIZE         (1 * MiB)
@@ -293,6 +296,10 @@ static void ibm_40p_init(MachineState *machine)
      pcihost = SYS_BUS_DEVICE(dev);
      object_property_add_child(qdev_get_machine(), "raven", OBJECT(dev));
      sysbus_realize_and_unref(pcihost, &error_fatal);
+    sysbus_mmio_map(pcihost, 0, PCI_IO_BASE_ADDR);
+    sysbus_mmio_map_overlap(pcihost, 1, PCI_IO_BASE_ADDR, 1);
+    memory_region_set_enabled(sysbus_mmio_get_region(pcihost, 1), false);
+    sysbus_mmio_map(pcihost, 2, PCI_MEM_BASE_ADDR);
      pci_bus = PCI_BUS(qdev_get_child_bus(dev, "pci.0"));
      if (!pci_bus) {
          error_report("could not create PCI host controller");