Re: [PATCH v3 6/7] hw/isa/piix4: QOM'ify PIIX4 PM creation

[Mark Cave-Ayland]

On 28/05/2022 20:20, Bernhard Beschow wrote:

> Just like the real hardware, create the PIIX4 ACPI controller as part of
> the PIIX4 southbridge. This also mirrors how the IDE and USB functions
> are already created.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/isa/piix4.c                | 25 ++++++++++++++-----------
>   hw/mips/malta.c               |  3 ++-
>   include/hw/southbridge/piix.h |  2 +-
>   3 files changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 96df21a610..217989227d 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -49,6 +49,7 @@ struct PIIX4State {
>       RTCState rtc;
>       PCIIDEState ide;
>       UHCIState uhci;
> +    PIIX4PMState pm;
>       /* Reset Control Register */
>       MemoryRegion rcr_mem;
>       uint8_t rcr;
> @@ -261,6 +262,14 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>           return;
>       }
>   
> +    /* ACPI controller */
> +    qdev_prop_set_int32(DEVICE(&s->pm), "addr", dev->devfn + 3);
> +    if (!qdev_realize(DEVICE(&s->pm), BUS(pci_bus), errp)) {
> +        return;
> +    }
> +    qdev_connect_gpio_out(DEVICE(&s->pm), 0, s->isa[9]);
> +    object_property_add_alias(OBJECT(s), "smbus", OBJECT(&s->pm), "i2c");

Now that the PM device is QOMified you don't actually need this alias anymore (see 
below). In general aliasing parts of contained devices onto the container isn't 
recommended, since it starts to blur the line between individual devices and then you 
find some wiring gets done to the container, some directly to the contained device 
and then it starts to get confusing quite quickly.

>       pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 
> PIIX_NUM_PIRQS);
>   }
>   
> @@ -271,6 +280,10 @@ static void piix4_init(Object *obj)
>       object_initialize_child(obj, "rtc", &s->rtc, TYPE_MC146818_RTC);
>       object_initialize_child(obj, "ide", &s->ide, "piix4-ide");
>       object_initialize_child(obj, "uhci", &s->uhci, "piix4-usb-uhci");
> +
> +    object_initialize_child(obj, "pm", &s->pm, TYPE_PIIX4_PM);
> +    qdev_prop_set_uint32(DEVICE(&s->pm), "smb_io_base", 0x1100);
> +    qdev_prop_set_bit(DEVICE(&s->pm), "smm-enabled", 0);
>   }
>   
>   static void piix4_class_init(ObjectClass *klass, void *data)
> @@ -312,7 +325,7 @@ static void piix4_register_types(void)
>   
>   type_init(piix4_register_types)
>   
> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
> +DeviceState *piix4_create(PCIBus *pci_bus)
>   {
>       PCIDevice *pci;
>       DeviceState *dev;
> @@ -322,15 +335,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
>                                             TYPE_PIIX4_PCI_DEVICE);
>       dev = DEVICE(pci);
>   
> -    if (smbus) {
> -        pci = pci_new(devfn + 3, TYPE_PIIX4_PM);
> -        qdev_prop_set_uint32(DEVICE(pci), "smb_io_base", 0x1100);
> -        qdev_prop_set_bit(DEVICE(pci), "smm-enabled", 0);
> -        pci_realize_and_unref(pci, pci_bus, &error_fatal);
> -        qdev_connect_gpio_out(DEVICE(pci), 0,
> -                              qdev_get_gpio_in_named(dev, "isa", 9));
> -        *smbus = I2C_BUS(qdev_get_child_bus(DEVICE(pci), "i2c"));
> -    }
> -
>       return dev;
>   }
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index e446b25ad0..b0fc84ccbb 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -1399,8 +1399,9 @@ void mips_malta_init(MachineState *machine)
>       empty_slot_init("GT64120", 0, 0x20000000);
>   
>       /* Southbridge */
> -    dev = piix4_create(pci_bus, &smbus);
> +    dev = piix4_create(pci_bus);
>       isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> +    smbus = I2C_BUS(qdev_get_child_bus(dev, "smbus"));

It should now be possible to do something like this:

    pm_dev = DEVICE(object_resolve_path_component(OBJECT(dev), "pm"));
    smbus = I2C_BUS(qdev_get_child_bus(pm_dev, "i2c"));

whereby we grab the reference to the PIIX4_PM device by resolving the "pm" child 
object and then use that to obtain the reference to smbus.

>       /* Interrupt controller */
>       qdev_connect_gpio_out_named(dev, "intr", 0, i8259_irq);
> diff --git a/include/hw/southbridge/piix.h b/include/hw/southbridge/piix.h
> index 0a2ef0c7b6..e1f5d6d5c8 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -70,6 +70,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>   
>   PIIX3State *piix3_create(PCIBus *pci_bus);
>   
> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
> +DeviceState *piix4_create(PCIBus *pci_bus);
>   
>   #endif


ATB,

Mark.