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

[Mark Cave-Ayland]

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.