Export memory regions as sysbus mmio regions and let the board code
map them similar to how it is done in grackle.
Signed-off-by: BALATON Zoltan <balaton@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 ebf0c511dc..0c4eca04bb 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);
@@ -170,7 +168,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);
@@ -180,7 +178,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);
@@ -219,32 +227,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";
}
@@ -278,7 +266,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");
--
2.41.3On 18/09/2025 19:50, BALATON Zoltan wrote:
> Export memory regions as sysbus mmio regions and let the board code
> map them similar to how it is done in grackle.
>
> Signed-off-by: BALATON Zoltan <balaton@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 ebf0c511dc..0c4eca04bb 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);
> @@ -170,7 +168,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)
There is a function rename here not mentioned in the commit message.
> {
> SysBusDevice *dev = SYS_BUS_DEVICE(d);
> PCIHostState *h = PCI_HOST_BRIDGE(dev);
> @@ -180,7 +178,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);
> @@ -219,32 +227,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);
> -}
Normally if the MemoryRegion does not depend upon a property, it can be initialised
in an init() function, so these can stay where they are.
But certainly the device should not be mapping itself, so the sysbus-related changes
look correct.
> -
> 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";
> }
>
> @@ -278,7 +266,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);
Why not leave this within the device itself? It seems odd that something external to
the device is trying to set its initial state.
> + 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");
ATB,
Mark.
On Thu, 18 Sep 2025, Mark Cave-Ayland wrote:
> On 18/09/2025 19:50, BALATON Zoltan wrote:
>> Export memory regions as sysbus mmio regions and let the board code
>> map them similar to how it is done in grackle.
>>
>> Signed-off-by: BALATON Zoltan <balaton@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 ebf0c511dc..0c4eca04bb 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);
>> @@ -170,7 +168,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)
>
> There is a function rename here not mentioned in the commit message.
I thought this is trivial enough but can mention it in the commit message.
>> {
>> SysBusDevice *dev = SYS_BUS_DEVICE(d);
>> PCIHostState *h = PCI_HOST_BRIDGE(dev);
>> @@ -180,7 +178,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);
>> @@ -219,32 +227,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);
>> -}
>
> Normally if the MemoryRegion does not depend upon a property, it can be
> initialised in an init() function, so these can stay where they are.
There's no need to have an init method in the first place as these
also don't provide a property so they can be created in realize instead.
> But certainly the device should not be mapping itself, so the sysbus-related
> changes look correct.
>
>> -
>> 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";
>> }
>> @@ -278,7 +266,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);
>
> Why not leave this within the device itself? It seems odd that something
> external to the device is trying to set its initial state.
I don't remember but maybe this is to keep existing behaviour and can be
removed after next patch where it sets initial state based on property?
I'd have to check this again.
Regards,
BALATON Zoltan
>> + 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");
>
>
> ATB,
>
> Mark.
>
>
© 2016 - 2025 Red Hat, Inc.