Re: [PATCH 3/7] macio: move heathrow PIC inside macio-oldworld device

[Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>]

On 28/12/2020 07:08, David Gibson wrote:

> On Sat, Dec 19, 2020 at 10:42:25AM +0000, Mark Cave-Ayland wrote:
> 
> Really needs a commit message.

This is currently explained in the cover letter: it's moving the PIC to the macio 
device as per real hardware (which also nicely removes the need for compulsory object 
property links which currently trip up some of the automated QOM introspection 
tests). I'll add something along these lines for the next revision.

>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/misc/macio/macio.c         | 20 +++++------
>>   hw/ppc/mac_oldworld.c         | 66 +++++++++++++++++------------------
>>   include/hw/misc/macio/macio.h |  2 +-
>>   3 files changed, 43 insertions(+), 45 deletions(-)
>>
>> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
>> index bb601f782c..cfb87da6c9 100644
>> --- a/hw/misc/macio/macio.c
>> +++ b/hw/misc/macio/macio.c
>> @@ -140,7 +140,7 @@ static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>>   {
>>       MacIOState *s = MACIO(d);
>>       OldWorldMacIOState *os = OLDWORLD_MACIO(d);
>> -    DeviceState *pic_dev = DEVICE(os->pic);
>> +    DeviceState *pic_dev = DEVICE(&os->pic);
>>       Error *err = NULL;
>>       SysBusDevice *sysbus_dev;
>>   
>> @@ -150,6 +150,14 @@ static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>>           return;
>>       }
>>   
>> +    /* Heathrow PIC */
>> +    if (!qdev_realize(DEVICE(&os->pic), BUS(&s->macio_bus), errp)) {
>> +        return;
>> +    }
>> +    sysbus_dev = SYS_BUS_DEVICE(&os->pic);
>> +    memory_region_add_subregion(&s->bar, 0x0,
>> +                                sysbus_mmio_get_region(sysbus_dev, 0));
>> +
>>       qdev_prop_set_uint64(DEVICE(&s->cuda), "timebase-frequency",
>>                            s->frequency);
>>       if (!qdev_realize(DEVICE(&s->cuda), BUS(&s->macio_bus), errp)) {
>> @@ -175,11 +183,6 @@ static void macio_oldworld_realize(PCIDevice *d, Error **errp)
>>                                   sysbus_mmio_get_region(sysbus_dev, 0));
>>       pmac_format_nvram_partition(&os->nvram, os->nvram.size);
>>   
>> -    /* Heathrow PIC */
>> -    sysbus_dev = SYS_BUS_DEVICE(os->pic);
>> -    memory_region_add_subregion(&s->bar, 0x0,
>> -                                sysbus_mmio_get_region(sysbus_dev, 0));
>> -
>>       /* IDE buses */
>>       macio_realize_ide(s, &os->ide[0],
>>                         qdev_get_gpio_in(pic_dev, OLDWORLD_IDE0_IRQ),
>> @@ -218,10 +221,7 @@ static void macio_oldworld_init(Object *obj)
>>       DeviceState *dev;
>>       int i;
>>   
>> -    object_property_add_link(obj, "pic", TYPE_HEATHROW,
>> -                             (Object **) &os->pic,
>> -                             qdev_prop_allow_set_link_before_realize,
>> -                             0);
>> +    object_initialize_child(OBJECT(s), "pic", &os->pic, TYPE_HEATHROW);
>>   
>>       object_initialize_child(OBJECT(s), "cuda", &s->cuda, TYPE_CUDA);
>>   
>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>> index e58e0525fe..44ee99be88 100644
>> --- a/hw/ppc/mac_oldworld.c
>> +++ b/hw/ppc/mac_oldworld.c
>> @@ -98,7 +98,7 @@ static void ppc_heathrow_init(MachineState *machine)
>>       MACIOIDEState *macio_ide;
>>       ESCCState *escc;
>>       SysBusDevice *s;
>> -    DeviceState *dev, *pic_dev;
>> +    DeviceState *dev, *pic_dev, *grackle_dev;
>>       BusState *adb_bus;
>>       uint64_t bios_addr;
>>       int bios_size;
>> @@ -227,10 +227,17 @@ static void ppc_heathrow_init(MachineState *machine)
>>           }
>>       }
>>   
>> +    /* Timebase Frequency */
>> +    if (kvm_enabled()) {
>> +        tbfreq = kvmppc_get_tbfreq();
>> +    } else {
>> +        tbfreq = TBFREQ;
>> +    }
>> +
>>       /* Grackle PCI host bridge */
>> -    dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
>> -    qdev_prop_set_uint32(dev, "ofw-addr", 0x80000000);
>> -    s = SYS_BUS_DEVICE(dev);
>> +    grackle_dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
>> +    qdev_prop_set_uint32(grackle_dev, "ofw-addr", 0x80000000);
>> +    s = SYS_BUS_DEVICE(grackle_dev);
>>       sysbus_realize_and_unref(s, &error_fatal);
>>   
>>       sysbus_mmio_map(s, 0, GRACKLE_BASE);
>> @@ -242,14 +249,30 @@ static void ppc_heathrow_init(MachineState *machine)
>>       memory_region_add_subregion(get_system_memory(), 0xfe000000,
>>                                   sysbus_mmio_get_region(s, 3));
>>   
>> -    /* XXX: we register only 1 output pin for heathrow PIC */
>> -    pic_dev = qdev_new(TYPE_HEATHROW);
>> -    sysbus_realize_and_unref(SYS_BUS_DEVICE(pic_dev), &error_fatal);
>> +    pci_bus = PCI_HOST_BRIDGE(grackle_dev)->bus;
>> +
>> +    /* MacIO */
>> +    macio = pci_new(PCI_DEVFN(16, 0), TYPE_OLDWORLD_MACIO);
>> +    dev = DEVICE(macio);
>> +    qdev_prop_set_uint64(dev, "frequency", tbfreq);
>> +
>> +    escc = ESCC(object_resolve_path_component(OBJECT(macio), "escc"));
>> +    qdev_prop_set_chr(DEVICE(escc), "chrA", serial_hd(0));
>> +    qdev_prop_set_chr(DEVICE(escc), "chrB", serial_hd(1));
>> +
>> +    pci_realize_and_unref(macio, pci_bus, &error_fatal);
>> +
>> +    pic_dev = DEVICE(object_resolve_path_component(OBJECT(macio), "pic"));
>> +    for (i = 0; i < 4; i++) {
>> +        qdev_connect_gpio_out(grackle_dev, i,
>> +                              qdev_get_gpio_in(pic_dev, 0x15 + i));
>> +    }
>>   
>>       /* Connect the heathrow PIC outputs to the 6xx bus */
>>       for (i = 0; i < smp_cpus; i++) {
>>           switch (PPC_INPUT(env)) {
>>           case PPC_FLAGS_INPUT_6xx:
>> +            /* XXX: we register only 1 output pin for heathrow PIC */
>>               qdev_connect_gpio_out(pic_dev, 0,
>>                   ((qemu_irq *)env->irq_inputs)[PPC6xx_INPUT_INT]);
>>               break;
>> @@ -259,40 +282,14 @@ static void ppc_heathrow_init(MachineState *machine)
>>           }
>>       }
>>   
>> -    /* Timebase Frequency */
>> -    if (kvm_enabled()) {
>> -        tbfreq = kvmppc_get_tbfreq();
>> -    } else {
>> -        tbfreq = TBFREQ;
>> -    }
>> -
>> -    for (i = 0; i < 4; i++) {
>> -        qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x15 + i));
>> -    }
>> -
>> -    pci_bus = PCI_HOST_BRIDGE(dev)->bus;
>> -
>>       pci_vga_init(pci_bus);
>>   
>>       for (i = 0; i < nb_nics; i++) {
>>           pci_nic_init_nofail(&nd_table[i], pci_bus, "ne2k_pci", NULL);
>>       }
>>   
>> +    /* MacIO IDE */
>>       ide_drive_get(hd, ARRAY_SIZE(hd));
>> -
>> -    /* MacIO */
>> -    macio = pci_new(PCI_DEVFN(16, 0), TYPE_OLDWORLD_MACIO);
>> -    dev = DEVICE(macio);
>> -    qdev_prop_set_uint64(dev, "frequency", tbfreq);
>> -    object_property_set_link(OBJECT(macio), "pic", OBJECT(pic_dev),
>> -                             &error_abort);
>> -
>> -    escc = ESCC(object_resolve_path_component(OBJECT(macio), "escc"));
>> -    qdev_prop_set_chr(DEVICE(escc), "chrA", serial_hd(0));
>> -    qdev_prop_set_chr(DEVICE(escc), "chrB", serial_hd(1));
>> -
>> -    pci_realize_and_unref(macio, pci_bus, &error_fatal);
>> -
>>       macio_ide = MACIO_IDE(object_resolve_path_component(OBJECT(macio),
>>                                                           "ide[0]"));
>>       macio_ide_init_drives(macio_ide, hd);
>> @@ -301,6 +298,7 @@ static void ppc_heathrow_init(MachineState *machine)
>>                                                           "ide[1]"));
>>       macio_ide_init_drives(macio_ide, &hd[MAX_IDE_DEVS]);
>>   
>> +    /* MacIO CUDA/ADB */
>>       dev = DEVICE(object_resolve_path_component(OBJECT(macio), "cuda"));
>>       adb_bus = qdev_get_child_bus(dev, "adb.0");
>>       dev = qdev_new(TYPE_ADB_KEYBOARD);
>> diff --git a/include/hw/misc/macio/macio.h b/include/hw/misc/macio/macio.h
>> index 22b4e64b2c..707dfab50c 100644
>> --- a/include/hw/misc/macio/macio.h
>> +++ b/include/hw/misc/macio/macio.h
>> @@ -99,7 +99,7 @@ struct OldWorldMacIOState {
>>       MacIOState parent_obj;
>>       /*< public >*/
>>   
>> -    HeathrowState *pic;
>> +    HeathrowState pic;
>>   
>>       MacIONVRAMState nvram;
>>       MACIOIDEState ide[2];

ATB,

Mark.