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

[Mark Cave-Ayland]

On 22/05/2022 22:24, 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                | 14 +++++++-------
>   hw/mips/malta.c               |  3 ++-
>   include/hw/southbridge/piix.h |  2 +-
>   3 files changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 4968c69da9..1645f63450 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -206,6 +206,7 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>       PIIX4State *s = PIIX4_PCI_DEVICE(dev);
>       PCIDevice *pci;
>       PCIBus *pci_bus = pci_get_bus(dev);
> +    I2CBus *smbus;
>       ISABus *isa_bus;
>       qemu_irq *i8259_out_irq;
>   
> @@ -252,6 +253,11 @@ static void piix4_realize(PCIDevice *dev, Error **errp)
>       /* USB */
>       pci_create_simple(pci_bus, dev->devfn + 2, "piix4-usb-uhci");
>   
> +    /* ACPI controller */
> +    smbus = piix4_pm_init(pci_bus, pci->devfn + 3, 0x1100, s->isa[9],
> +                          NULL, 0, NULL);
> +    object_property_add_const_link(OBJECT(s), "smbus", OBJECT(smbus));
> +

Interesting hack here to expose the smbus so it is available to qdev_get_child_bus(), 
but really this is still really working around the fact that piix4_pm_init() itself 
should be removed first. Once that is done, you can then use a standard QOM pattern 
to initialise the "internal" PCI devices via object_initialize_child() and realize 
them in piix4_realize() instead of using pci_create_simple().

Is that something you could take a look at? If not, I may be able to put something 
together towards the end of the week. Other than that I think the rest of the series 
looks good.

>       pci_bus_irqs(pci_bus, piix4_set_irq, pci_slot_get_pirq, s, 
> PIIX_NUM_PIRQS);
>   }
>   
> @@ -301,7 +307,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;
> @@ -311,11 +317,5 @@ DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
>                                             TYPE_PIIX4_PCI_DEVICE);
>       dev = DEVICE(pci);
>   
> -    if (smbus) {
> -        *smbus = piix4_pm_init(pci_bus, devfn + 3, 0x1100,
> -                               qdev_get_gpio_in_named(dev, "isa", 9),
> -                               NULL, 0, NULL);
> -    }
> -
>       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"));
>   
>       /* 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 0bec7f8ca3..2c21359efa 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -76,6 +76,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.