diff mbox series

[06/23] hw/ppc/e500: Use SysBusDevice API to access TYPE_CCSR's internal resources

Message ID 20240923093016.66437-7-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series E500 Cleanup | expand

Commit Message

Bernhard Beschow Sept. 23, 2024, 9:29 a.m. UTC 
Rather than accessing the attributes of TYPE_CCSR directly, use the SysBusDevice
API which exists exactly for that purpose. Furthermore, registering the memory
region with the SysBusDevice API makes it show up in QMP's `info qom-tree`
command.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/pci-host/ppce500.c | 10 +++++-----
 hw/ppc/e500.c         |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

Comments

BALATON Zoltan Sept. 23, 2024, 10:28 a.m. UTC | #1 
On Mon, 23 Sep 2024, Bernhard Beschow wrote:
> Rather than accessing the attributes of TYPE_CCSR directly, use the SysBusDevice
> API which exists exactly for that purpose. Furthermore, registering the memory
> region with the SysBusDevice API makes it show up in QMP's `info qom-tree`
> command.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/pci-host/ppce500.c | 10 +++++-----
> hw/ppc/e500.c         |  8 ++++----
> 2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
> index 95b983b2b3..97e5d47cec 100644
> --- a/hw/pci-host/ppce500.c
> +++ b/hw/pci-host/ppce500.c
> @@ -16,7 +16,6 @@
>
> #include "qemu/osdep.h"
> #include "hw/irq.h"
> -#include "hw/ppc/e500-ccsr.h"
> #include "hw/qdev-properties.h"
> #include "migration/vmstate.h"
> #include "hw/pci/pci_device.h"
> @@ -419,11 +418,12 @@ static const VMStateDescription vmstate_ppce500_pci = {
> static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
> {
>     PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
> -    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
> -                                  "/e500-ccsr"));
> +    SysBusDevice *ccsr = SYS_BUS_DEVICE(container_get(qdev_get_machine(),
> +                                                      "/e500-ccsr"));
> +    MemoryRegion *ccsr_space = sysbus_mmio_get_region(ccsr, 0);
>
> -    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
> -                             0, int128_get64(ccsr->ccsr_space.size));
> +    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0",
> +                             ccsr_space, 0, int128_get64(ccsr_space->size));

I wonder if this really needs an alias or could the original memory region 
be registered as the PCI BAR region? Otherwise:

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

Regards,
BALATON Zoltan

>     pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
> }
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index e2a4f265a5..2225533e33 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -924,7 +924,6 @@ void ppce500_init(MachineState *machine)
>     DriveInfo *dinfo;
>     MemoryRegion *ccsr_addr_space;
>     SysBusDevice *s;
> -    PPCE500CCSRState *ccsr;
>     I2CBus *i2c;
>
>     irqs = g_new0(IrqLines, smp_cpus);
> @@ -980,10 +979,10 @@ void ppce500_init(MachineState *machine)
>     memory_region_add_subregion(address_space_mem, 0, machine->ram);
>
>     dev = qdev_new("e500-ccsr");
> +    s = SYS_BUS_DEVICE(dev);
>     object_property_add_child(OBJECT(machine), "e500-ccsr", OBJECT(dev));
> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> -    ccsr = CCSR(dev);
> -    ccsr_addr_space = &ccsr->ccsr_space;
> +    sysbus_realize_and_unref(s, &error_fatal);
> +    ccsr_addr_space = sysbus_mmio_get_region(s, 0);
>     memory_region_add_subregion(address_space_mem, pmc->ccsrbar_base,
>                                 ccsr_addr_space);
>
> @@ -1270,6 +1269,7 @@ static void e500_ccsr_initfn(Object *obj)
>     PPCE500CCSRState *ccsr = CCSR(obj);
>     memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>                        MPC8544_CCSRBAR_SIZE);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
> }
>
> static const TypeInfo e500_ccsr_info = {
>
Bernhard Beschow Sept. 27, 2024, 4:57 p.m. UTC | #2 
Am 23. September 2024 10:28:35 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 23 Sep 2024, Bernhard Beschow wrote:
>> Rather than accessing the attributes of TYPE_CCSR directly, use the SysBusDevice
>> API which exists exactly for that purpose. Furthermore, registering the memory
>> region with the SysBusDevice API makes it show up in QMP's `info qom-tree`
>> command.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> hw/pci-host/ppce500.c | 10 +++++-----
>> hw/ppc/e500.c         |  8 ++++----
>> 2 files changed, 9 insertions(+), 9 deletions(-)
>> 
>> diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
>> index 95b983b2b3..97e5d47cec 100644
>> --- a/hw/pci-host/ppce500.c
>> +++ b/hw/pci-host/ppce500.c
>> @@ -16,7 +16,6 @@
>> 
>> #include "qemu/osdep.h"
>> #include "hw/irq.h"
>> -#include "hw/ppc/e500-ccsr.h"
>> #include "hw/qdev-properties.h"
>> #include "migration/vmstate.h"
>> #include "hw/pci/pci_device.h"
>> @@ -419,11 +418,12 @@ static const VMStateDescription vmstate_ppce500_pci = {
>> static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
>> {
>>     PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
>> -    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
>> -                                  "/e500-ccsr"));
>> +    SysBusDevice *ccsr = SYS_BUS_DEVICE(container_get(qdev_get_machine(),
>> +                                                      "/e500-ccsr"));
>> +    MemoryRegion *ccsr_space = sysbus_mmio_get_region(ccsr, 0);
>> 
>> -    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
>> -                             0, int128_get64(ccsr->ccsr_space.size));
>> +    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0",
>> +                             ccsr_space, 0, int128_get64(ccsr_space->size));
>
>I wonder if this really needs an alias or could the original memory region be registered as the PCI BAR region? Otherwise:

This hits an assertion when running Buildroot's qemu_ppc64_e5500_defconfig. Therefore I'll keep the alias.

Best regards,
Bernhard

>
>Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>
>
>Regards,
>BALATON Zoltan
>
>>     pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
>> }
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index e2a4f265a5..2225533e33 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -924,7 +924,6 @@ void ppce500_init(MachineState *machine)
>>     DriveInfo *dinfo;
>>     MemoryRegion *ccsr_addr_space;
>>     SysBusDevice *s;
>> -    PPCE500CCSRState *ccsr;
>>     I2CBus *i2c;
>> 
>>     irqs = g_new0(IrqLines, smp_cpus);
>> @@ -980,10 +979,10 @@ void ppce500_init(MachineState *machine)
>>     memory_region_add_subregion(address_space_mem, 0, machine->ram);
>> 
>>     dev = qdev_new("e500-ccsr");
>> +    s = SYS_BUS_DEVICE(dev);
>>     object_property_add_child(OBJECT(machine), "e500-ccsr", OBJECT(dev));
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>> -    ccsr = CCSR(dev);
>> -    ccsr_addr_space = &ccsr->ccsr_space;
>> +    sysbus_realize_and_unref(s, &error_fatal);
>> +    ccsr_addr_space = sysbus_mmio_get_region(s, 0);
>>     memory_region_add_subregion(address_space_mem, pmc->ccsrbar_base,
>>                                 ccsr_addr_space);
>> 
>> @@ -1270,6 +1269,7 @@ static void e500_ccsr_initfn(Object *obj)
>>     PPCE500CCSRState *ccsr = CCSR(obj);
>>     memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
>>                        MPC8544_CCSRBAR_SIZE);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
>> }
>> 
>> static const TypeInfo e500_ccsr_info = {
>>
diff mbox series

Patch

diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c
index 95b983b2b3..97e5d47cec 100644
--- a/hw/pci-host/ppce500.c
+++ b/hw/pci-host/ppce500.c
@@ -16,7 +16,6 @@ 
 
 #include "qemu/osdep.h"
 #include "hw/irq.h"
-#include "hw/ppc/e500-ccsr.h"
 #include "hw/qdev-properties.h"
 #include "migration/vmstate.h"
 #include "hw/pci/pci_device.h"
@@ -419,11 +418,12 @@  static const VMStateDescription vmstate_ppce500_pci = {
 static void e500_pcihost_bridge_realize(PCIDevice *d, Error **errp)
 {
     PPCE500PCIBridgeState *b = PPC_E500_PCI_BRIDGE(d);
-    PPCE500CCSRState *ccsr = CCSR(container_get(qdev_get_machine(),
-                                  "/e500-ccsr"));
+    SysBusDevice *ccsr = SYS_BUS_DEVICE(container_get(qdev_get_machine(),
+                                                      "/e500-ccsr"));
+    MemoryRegion *ccsr_space = sysbus_mmio_get_region(ccsr, 0);
 
-    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0", &ccsr->ccsr_space,
-                             0, int128_get64(ccsr->ccsr_space.size));
+    memory_region_init_alias(&b->bar0, OBJECT(ccsr), "e500-pci-bar0",
+                             ccsr_space, 0, int128_get64(ccsr_space->size));
     pci_register_bar(d, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &b->bar0);
 }
 
diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index e2a4f265a5..2225533e33 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -924,7 +924,6 @@  void ppce500_init(MachineState *machine)
     DriveInfo *dinfo;
     MemoryRegion *ccsr_addr_space;
     SysBusDevice *s;
-    PPCE500CCSRState *ccsr;
     I2CBus *i2c;
 
     irqs = g_new0(IrqLines, smp_cpus);
@@ -980,10 +979,10 @@  void ppce500_init(MachineState *machine)
     memory_region_add_subregion(address_space_mem, 0, machine->ram);
 
     dev = qdev_new("e500-ccsr");
+    s = SYS_BUS_DEVICE(dev);
     object_property_add_child(OBJECT(machine), "e500-ccsr", OBJECT(dev));
-    sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-    ccsr = CCSR(dev);
-    ccsr_addr_space = &ccsr->ccsr_space;
+    sysbus_realize_and_unref(s, &error_fatal);
+    ccsr_addr_space = sysbus_mmio_get_region(s, 0);
     memory_region_add_subregion(address_space_mem, pmc->ccsrbar_base,
                                 ccsr_addr_space);
 
@@ -1270,6 +1269,7 @@  static void e500_ccsr_initfn(Object *obj)
     PPCE500CCSRState *ccsr = CCSR(obj);
     memory_region_init(&ccsr->ccsr_space, obj, "e500-ccsr",
                        MPC8544_CCSRBAR_SIZE);
+    sysbus_init_mmio(SYS_BUS_DEVICE(ccsr), &ccsr->ccsr_space);
 }
 
 static const TypeInfo e500_ccsr_info = {