On 13/05/2022 18:54, Bernhard Beschow wrote:
> Initialize the SM bus just like is done for piix3 which modernizes the
> code.
>
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> hw/isa/piix4.c | 15 +++------------
> hw/mips/malta.c | 7 ++++++-
> include/hw/southbridge/piix.h | 2 +-
> 3 files changed, 10 insertions(+), 14 deletions(-)
>
> diff --git a/hw/isa/piix4.c b/hw/isa/piix4.c
> index 4968c69da9..852e5c4db1 100644
> --- a/hw/isa/piix4.c
> +++ b/hw/isa/piix4.c
> @@ -301,21 +301,12 @@ static void piix4_register_types(void)
>
> type_init(piix4_register_types)
>
> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus)
> +PCIDevice *piix4_create(PCIBus *pci_bus)
> {
> PCIDevice *pci;
> - DeviceState *dev;
> - int devfn = PCI_DEVFN(10, 0);
>
> - pci = pci_create_simple_multifunction(pci_bus, devfn, true,
> + pci = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(10, 0), true,
> 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;
> + return pci;
> }
I don't think it makes sense to return PCIDevice here: when returning a QOM object
from a function, the general expectation is that for a device you would return a
DeviceState since then it can natively be used by the qdev API. So please keep the
original return type above.
Okay, will do.
I've been toying with moving piix4_pm_init() into piix4_realize(), such that it is created as part of TYPE_PIIX4_PCI_DEVICE - just as the real hardware. I think I like this solution much better.
> diff --git a/hw/mips/malta.c b/hw/mips/malta.c
> index e446b25ad0..d4bd3549d0 100644
> --- a/hw/mips/malta.c
> +++ b/hw/mips/malta.c
> @@ -1238,6 +1238,7 @@ void mips_malta_init(MachineState *machine)
> int be;
> MaltaState *s;
> DeviceState *dev;
> + PCIDevice *piix4;
>
> s = MIPS_MALTA(qdev_new(TYPE_MIPS_MALTA));
> sysbus_realize_and_unref(SYS_BUS_DEVICE(s), &error_fatal);
> @@ -1399,8 +1400,12 @@ void mips_malta_init(MachineState *machine)
> empty_slot_init("GT64120", 0, 0x20000000);
>
> /* Southbridge */
> - dev = piix4_create(pci_bus, &smbus);
> + piix4 = piix4_create(pci_bus);
> + dev = DEVICE(piix4);
> isa_bus = ISA_BUS(qdev_get_child_bus(dev, "isa.0"));
> + smbus = piix4_pm_init(pci_bus, piix4->devfn + 3, 0x1100,
> + qdev_get_gpio_in_named(dev, "isa", 9),
> + NULL, 0, NULL);
... then here you can do either "piix4 = PCI_DEVICE(dev)" or perhaps even inline it
directly as PCI_DEVICE(dev)->devfn if it isn't used elsewhere.
When instantiating the pm in TYPE_PIIX4_PCI_DEVICE this problem just disappears magically. So I'd roll with this in v2.
> /* 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 b768109f30..bea3b44551 100644
> --- a/include/hw/southbridge/piix.h
> +++ b/include/hw/southbridge/piix.h
> @@ -74,6 +74,6 @@ DECLARE_INSTANCE_CHECKER(PIIX3State, PIIX3_PCI_DEVICE,
>
> PIIX3State *piix3_create(PCIBus *pci_bus);
>
> -DeviceState *piix4_create(PCIBus *pci_bus, I2CBus **smbus);
> +PCIDevice *piix4_create(PCIBus *pci_bus);
>
> #endif
ATB,
Mark.