diff mbox series

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

Message ID 29ab3c7737866916760f824547bd1beed0c6806b.1663368422.git.balaton@eik.bme.hu (mailing list archive)
State New, archived
Headers show
Series Misc ppc/mac machines clean up | expand

Commit Message

BALATON Zoltan Sept. 16, 2022, 11:07 p.m. UTC 
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 <balaton@eik.bme.hu>
---
 hw/ppc/mac_newworld.c | 50 ++++++++++++++++---------------------------
 1 file changed, 19 insertions(+), 31 deletions(-)

Comments

Mark Cave-Ayland Sept. 25, 2022, 8:57 a.m. UTC | #1 
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 <balaton@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?

>       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.

>       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.
BALATON Zoltan Sept. 25, 2022, 9:20 a.m. UTC | #2 
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 <balaton@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.
>
>
diff mbox series

Patch

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));
-
     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);
+
     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);
 
         /* 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;