Re: [PATCH 04/10] mac_newworld: Simplify creation of Uninorth devices

[BALATON Zoltan]

On Sun, 25 Sep 2022, Mark Cave-Ayland wrote:
On 17/09/2022 00:07, BALATON Zoltan wrote:
Avoid open coding sysbus_create_simple where not necessary and
reorganise code a bit to avoid some casts to make the code more
readable.
Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
---
  hw/ppc/mac_newworld.c | 50 ++++++++++++++++---------------------------
  1 file changed, 19 insertions(+), 31 deletions(-)

diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 6bc3bd19be..1038477793 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -228,13 +228,6 @@ static void ppc_core99_init(MachineState *machine)
          }
      }
  -    /* UniN init */
-    dev = qdev_new(TYPE_UNI_NORTH);
-    s = SYS_BUS_DEVICE(dev);
-    sysbus_realize_and_unref(s, &error_fatal);
-    memory_region_add_subregion(get_system_memory(), 0xf8000000,
-                                sysbus_mmio_get_region(s, 0));

Curious - is there a reason that the initialisation of UniNorth is moved to 
later in the file?

To move it togerher with the other parts of the uninorth. Creating this 
one here maybe was needed before for some reason but apparently nothing 
needs it until later so creating all the pci stuff together is more 
logical.

      openpic_irqs = g_new0(IrqLines, machine->smp.cpus);
      for (i = 0; i < machine->smp.cpus; i++) {
          /* Mac99 IRQ connection between OpenPIC outputs pins
@@ -275,56 +268,51 @@ static void ppc_core99_init(MachineState *machine)
          }
      }
  +    /* UniN init */
+    sysbus_create_simple(TYPE_UNI_NORTH, 0xf8000000, NULL);
+

I've had a look at sysbus_create_simple() as I'm not overly familiar with it, 
but this is one to add to the legacy functions we really shouldn't be using 
these days.

Obvious flaws from looking at the code are i) it attempts to map/wire devices 
in a _simple() function in contrast to all the other _simple() functions and 
ii) it assumes that properties are ordered (we can't guarantee this, as per 
the current array property breakage). So please keep this as-is.

I don't think creating devices with qdev calls is easier to read than 
sysbud_create_simple but OK, I'll drop this part then.

Regards,
BALATON Zoltan

      if (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) {
+        machine_arch = ARCH_MAC99_U3;
          /* 970 gets a U3 bus */
          /* Uninorth AGP bus */
-        dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
-        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
-        uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
-        s = SYS_BUS_DEVICE(dev);
+        s = SYS_BUS_DEVICE(sysbus_create_simple(TYPE_U3_AGP_HOST_BRIDGE,
+                                                0xf0800000, NULL));
+        uninorth_pci = U3_AGP_HOST_BRIDGE(s);
+        sysbus_mmio_map(s, 1, 0xf0c00000);
          /* PCI hole */
          memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
                                      sysbus_mmio_get_region(s, 2));
          /* Register 8 MB of ISA IO space */
          memory_region_add_subregion(get_system_memory(), 0xf2000000,
                                      sysbus_mmio_get_region(s, 3));
-        sysbus_mmio_map(s, 0, 0xf0800000);
-        sysbus_mmio_map(s, 1, 0xf0c00000);
-
-        machine_arch = ARCH_MAC99_U3;
      } else {
+        machine_arch = ARCH_MAC99;
          /* Use values found on a real PowerMac */
          /* Uninorth AGP bus */
-        uninorth_agp_dev = qdev_new(TYPE_UNI_NORTH_AGP_HOST_BRIDGE);
-        s = SYS_BUS_DEVICE(uninorth_agp_dev);
-        sysbus_realize_and_unref(s, &error_fatal);
-        sysbus_mmio_map(s, 0, 0xf0800000);
-        sysbus_mmio_map(s, 1, 0xf0c00000);
+        uninorth_agp_dev = 
sysbus_create_simple(TYPE_UNI_NORTH_AGP_HOST_BRIDGE,
+                                                0xf0800000, NULL);
+        sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_agp_dev), 1, 0xf0c00000);

Yeah sysbus_create_simple() makes this uglier.

          /* Uninorth internal bus */
-        uninorth_internal_dev = qdev_new(
-                                TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE);
-        s = SYS_BUS_DEVICE(uninorth_internal_dev);
-        sysbus_realize_and_unref(s, &error_fatal);
-        sysbus_mmio_map(s, 0, 0xf4800000);
-        sysbus_mmio_map(s, 1, 0xf4c00000);
+        uninorth_internal_dev = sysbus_create_simple(
+ 
TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE,
+                                                     0xf4800000, NULL);
+        sysbus_mmio_map(SYS_BUS_DEVICE(uninorth_internal_dev), 1, 
0xf4c00000);
            /* Uninorth main bus */
          dev = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
          qdev_prop_set_uint32(dev, "ofw-addr", 0xf2000000);
-        sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
          uninorth_pci = UNI_NORTH_PCI_HOST_BRIDGE(dev);
          s = SYS_BUS_DEVICE(dev);
+        sysbus_realize_and_unref(s, &error_fatal);
+        sysbus_mmio_map(s, 0, 0xf2800000);
+        sysbus_mmio_map(s, 1, 0xf2c00000);
          /* PCI hole */
          memory_region_add_subregion(get_system_memory(), 0x80000000ULL,
                                      sysbus_mmio_get_region(s, 2));
          /* Register 8 MB of ISA IO space */
          memory_region_add_subregion(get_system_memory(), 0xf2000000,
                                      sysbus_mmio_get_region(s, 3));
-        sysbus_mmio_map(s, 0, 0xf2800000);
-        sysbus_mmio_map(s, 1, 0xf2c00000);
-
-        machine_arch = ARCH_MAC99;
      }
        machine->usb |= defaults_enabled() && !machine->usb_disabled;

ATB,

Mark.