diff mbox series

[5/6] hw/isa/piix4: Factor out SM bus initialization from create() function

Message ID 20220513175445.89616-6-shentey@gmail.com (mailing list archive)
State New, archived
Headers show
Series QOM'ify PIIX southbridge creation | expand

Commit Message

Bernhard Beschow May 13, 2022, 5:54 p.m. UTC 
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(-)

Comments

Mark Cave-Ayland May 21, 2022, 8:38 a.m. UTC | #1 
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.

> 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.

>       /* 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.
Bernhard Beschow May 22, 2022, 9:21 a.m. UTC | #2 
On Sat, May 21, 2022 at 10:39 AM Mark Cave-Ayland <
mark.cave-ayland@ilande.co.uk> wrote:

> 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.
>
Mark Cave-Ayland May 22, 2022, 12:30 p.m. UTC | #3 
On 22/05/2022 10:21, Bernhard Beschow wrote:

> On Sat, May 21, 2022 at 10:39 AM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk 
> <mailto:mark.cave-ayland@ilande.co.uk>> wrote:
> 
>     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 <mailto: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.

(goes and looks)

Yes, that makes complete sense to me since it mirrors exactly how this is already 
done with the integrated IDE and USB devices. At some point I can see that 
piix4_pm_init() should also disappear but let's go one step at a time :)

>      >       /* 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.
diff mbox series

Patch

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;
 }
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);
 
     /* 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