[PATCH v2 0/3] ppc: Mac machine updates

[PATCH v2 0/3] ppc: Mac machine updates

[Mark Cave-Ayland]

Here are a few Mac machine updates from my pending patch queue. Patch 1 is
split out from the QOM series I posted a few weeks ago (the other patches
are all SPARC-related).

Patches 2 and 3 move the IRQ wiring of the PCI host bridge from the host
bridge device to the Old World and New World machines as discussed with
Philippe yesterday:

David - patch 2 is a replacement for Philippe's original patch at
https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02988.html whilst
patch 3 makes the same change for uninorth.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


v2:
- Rebase onto master
- Remove PIC header includes from grackle.c and uninorth.h which are no longer required


Mark Cave-Ayland (3):
  macio: don't reference serial_hd() directly within the device
  grackle: use qdev gpios for PCI IRQs
  uninorth: use qdev gpios for PCI IRQs

 hw/misc/macio/macio.c          |  4 ---
 hw/pci-host/grackle.c          | 19 ++------------
 hw/pci-host/uninorth.c         | 45 +++++++---------------------------
 hw/ppc/mac_newworld.c          | 30 +++++++++++++++++------
 hw/ppc/mac_oldworld.c          | 13 ++++++++--
 include/hw/pci-host/uninorth.h |  2 --
 6 files changed, 44 insertions(+), 69 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] macio: don't reference serial_hd() directly within the device

[Mark Cave-Ayland]

Instead use qdev_prop_set_chr() to configure the ESCC serial chardevs at the
Mac Old World and New World machine level.

Also remove the now obsolete comment referring to the use of serial_hd() and
the setting of user_creatable to false accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/macio/macio.c | 4 ----
 hw/ppc/mac_newworld.c | 6 ++++++
 hw/ppc/mac_oldworld.c | 6 ++++++
 3 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
index 679722628e..51368884d0 100644
--- a/hw/misc/macio/macio.c
+++ b/hw/misc/macio/macio.c
@@ -109,8 +109,6 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
     qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
     qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
     qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
-    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hd(0));
-    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hd(1));
     qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
     qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
     if (!qdev_realize(DEVICE(&s->escc), BUS(&s->macio_bus), errp)) {
@@ -458,8 +456,6 @@ static void macio_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_OTHERS << 8;
     device_class_set_props(dc, macio_properties);
     set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
-    /* Reason: Uses serial_hds in macio_instance_init */
-    dc->user_creatable = false;
 }
 
 static const TypeInfo macio_bus_info = {
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 4dfbeec0ca..6f5ef2e782 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -123,6 +123,7 @@ static void ppc_core99_init(MachineState *machine)
     UNINHostState *uninorth_pci;
     PCIBus *pci_bus;
     PCIDevice *macio;
+    ESCCState *escc;
     bool has_pmu, has_adb;
     MACIOIDEState *macio_ide;
     BusState *adb_bus;
@@ -380,6 +381,11 @@ static void ppc_core99_init(MachineState *machine)
     qdev_prop_set_bit(dev, "has-adb", has_adb);
     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);
 
     /* We only emulate 2 out of 3 IDE controllers for now */
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index f8173934a2..d6a76d06dc 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -96,6 +96,7 @@ static void ppc_heathrow_init(MachineState *machine)
     PCIBus *pci_bus;
     PCIDevice *macio;
     MACIOIDEState *macio_ide;
+    ESCCState *escc;
     SysBusDevice *s;
     DeviceState *dev, *pic_dev;
     BusState *adb_bus;
@@ -281,6 +282,11 @@ static void ppc_heathrow_init(MachineState *machine)
     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),
-- 
2.20.1

Re: [PATCH v2 1/3] macio: don't reference serial_hd() directly within the device

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4073 bytes --]

On Tue, Oct 13, 2020 at 12:49:20PM +0100, Mark Cave-Ayland wrote:
> Instead use qdev_prop_set_chr() to configure the ESCC serial chardevs at the
> Mac Old World and New World machine level.
> 
> Also remove the now obsolete comment referring to the use of serial_hd() and
> the setting of user_creatable to false accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied to ppc-for-5.2, thanks.

> ---
>  hw/misc/macio/macio.c | 4 ----
>  hw/ppc/mac_newworld.c | 6 ++++++
>  hw/ppc/mac_oldworld.c | 6 ++++++
>  3 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/misc/macio/macio.c b/hw/misc/macio/macio.c
> index 679722628e..51368884d0 100644
> --- a/hw/misc/macio/macio.c
> +++ b/hw/misc/macio/macio.c
> @@ -109,8 +109,6 @@ static void macio_common_realize(PCIDevice *d, Error **errp)
>      qdev_prop_set_uint32(DEVICE(&s->escc), "disabled", 0);
>      qdev_prop_set_uint32(DEVICE(&s->escc), "frequency", ESCC_CLOCK);
>      qdev_prop_set_uint32(DEVICE(&s->escc), "it_shift", 4);
> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrA", serial_hd(0));
> -    qdev_prop_set_chr(DEVICE(&s->escc), "chrB", serial_hd(1));
>      qdev_prop_set_uint32(DEVICE(&s->escc), "chnBtype", escc_serial);
>      qdev_prop_set_uint32(DEVICE(&s->escc), "chnAtype", escc_serial);
>      if (!qdev_realize(DEVICE(&s->escc), BUS(&s->macio_bus), errp)) {
> @@ -458,8 +456,6 @@ static void macio_class_init(ObjectClass *klass, void *data)
>      k->class_id = PCI_CLASS_OTHERS << 8;
>      device_class_set_props(dc, macio_properties);
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
> -    /* Reason: Uses serial_hds in macio_instance_init */
> -    dc->user_creatable = false;
>  }
>  
>  static const TypeInfo macio_bus_info = {
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 4dfbeec0ca..6f5ef2e782 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -123,6 +123,7 @@ static void ppc_core99_init(MachineState *machine)
>      UNINHostState *uninorth_pci;
>      PCIBus *pci_bus;
>      PCIDevice *macio;
> +    ESCCState *escc;
>      bool has_pmu, has_adb;
>      MACIOIDEState *macio_ide;
>      BusState *adb_bus;
> @@ -380,6 +381,11 @@ static void ppc_core99_init(MachineState *machine)
>      qdev_prop_set_bit(dev, "has-adb", has_adb);
>      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);
>  
>      /* We only emulate 2 out of 3 IDE controllers for now */
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index f8173934a2..d6a76d06dc 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -96,6 +96,7 @@ static void ppc_heathrow_init(MachineState *machine)
>      PCIBus *pci_bus;
>      PCIDevice *macio;
>      MACIOIDEState *macio_ide;
> +    ESCCState *escc;
>      SysBusDevice *s;
>      DeviceState *dev, *pic_dev;
>      BusState *adb_bus;
> @@ -281,6 +282,11 @@ static void ppc_heathrow_init(MachineState *machine)
>      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),

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/3] macio: don't reference serial_hd() directly within the device

[Mark Cave-Ayland]

On 16/10/2020 01:16, David Gibson wrote:

> On Tue, Oct 13, 2020 at 12:49:20PM +0100, Mark Cave-Ayland wrote:
>> Instead use qdev_prop_set_chr() to configure the ESCC serial chardevs at the
>> Mac Old World and New World machine level.
>>
>> Also remove the now obsolete comment referring to the use of serial_hd() and
>> the setting of user_creatable to false accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> Applied to ppc-for-5.2, thanks.

Ah okay, I was planning to send a separate qemu-macppc pull request myself with these 
patches plus some cherry-picks from Zoltan's patchset after some more testing. Does 
this mean you would prefer to take the patches directly yourself?


ATB,

Mark.

Re: [PATCH v2 1/3] macio: don't reference serial_hd() directly within the device

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1199 bytes --]

On Fri, Oct 16, 2020 at 08:00:06AM +0100, Mark Cave-Ayland wrote:
> On 16/10/2020 01:16, David Gibson wrote:
> 
> > On Tue, Oct 13, 2020 at 12:49:20PM +0100, Mark Cave-Ayland wrote:
> > > Instead use qdev_prop_set_chr() to configure the ESCC serial chardevs at the
> > > Mac Old World and New World machine level.
> > > 
> > > Also remove the now obsolete comment referring to the use of serial_hd() and
> > > the setting of user_creatable to false accordingly.
> > > 
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > 
> > Applied to ppc-for-5.2, thanks.
> 
> Ah okay, I was planning to send a separate qemu-macppc pull request myself
> with these patches plus some cherry-picks from Zoltan's patchset after some
> more testing. Does this mean you would prefer to take the patches directly
> yourself?

Not really.  I sent a PR recently, so I probably won't do another for
a little while.  So go ahead and send yours, and mine should rebase
easily enough.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[Mark Cave-Ayland]

Currently an object link property is used to pass a reference to the Heathrow
PIC into the PCI host bridge so that grackle_init_irqs() can connect the PCI
IRQs to the PIC itself.

This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
up the PCI IRQs to the PIC in the Old World machine init function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci-host/grackle.c | 19 ++-----------------
 hw/ppc/mac_oldworld.c |  7 +++++--
 2 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
index 57c29b20af..b05facf463 100644
--- a/hw/pci-host/grackle.c
+++ b/hw/pci-host/grackle.c
@@ -28,7 +28,6 @@
 #include "hw/ppc/mac.h"
 #include "hw/qdev-properties.h"
 #include "hw/pci/pci.h"
-#include "hw/intc/heathrow_pic.h"
 #include "hw/irq.h"
 #include "qapi/error.h"
 #include "qemu/module.h"
@@ -41,7 +40,6 @@ struct GrackleState {
     PCIHostState parent_obj;
 
     uint32_t ofw_addr;
-    HeathrowState *pic;
     qemu_irq irqs[4];
     MemoryRegion pci_mmio;
     MemoryRegion pci_hole;
@@ -62,15 +60,6 @@ static void pci_grackle_set_irq(void *opaque, int irq_num, int level)
     qemu_set_irq(s->irqs[irq_num], level);
 }
 
-static void grackle_init_irqs(GrackleState *s)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
-        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), 0x15 + i);
-    }
-}
-
 static void grackle_realize(DeviceState *dev, Error **errp)
 {
     GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev);
@@ -85,7 +74,6 @@ static void grackle_realize(DeviceState *dev, Error **errp)
                                      0, 4, TYPE_PCI_BUS);
 
     pci_create_simple(phb->bus, 0, "grackle");
-    grackle_init_irqs(s);
 }
 
 static void grackle_init(Object *obj)
@@ -106,15 +94,12 @@ static void grackle_init(Object *obj)
     memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops,
                           DEVICE(obj), "pci-data-idx", 0x1000);
 
-    object_property_add_link(obj, "pic", TYPE_HEATHROW,
-                             (Object **) &s->pic,
-                             qdev_prop_allow_set_link_before_realize,
-                             0);
-
     sysbus_init_mmio(sbd, &phb->conf_mem);
     sysbus_init_mmio(sbd, &phb->data_mem);
     sysbus_init_mmio(sbd, &s->pci_hole);
     sysbus_init_mmio(sbd, &s->pci_io);
+
+    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
 }
 
 static void grackle_pci_realize(PCIDevice *d, Error **errp)
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index d6a76d06dc..05e46ee6fe 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -253,10 +253,9 @@ static void ppc_heathrow_init(MachineState *machine)
     /* Grackle PCI host bridge */
     dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
     qdev_prop_set_uint32(dev, "ofw-addr", 0x80000000);
-    object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
-                             &error_abort);
     s = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(s, &error_fatal);
+
     sysbus_mmio_map(s, 0, GRACKLE_BASE);
     sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000);
     /* PCI hole */
@@ -266,6 +265,10 @@ static void ppc_heathrow_init(MachineState *machine)
     memory_region_add_subregion(get_system_memory(), 0xfe000000,
                                 sysbus_mmio_get_region(s, 3));
 
+    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);
-- 
2.20.1

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[Philippe Mathieu-Daudé]

On 10/13/20 1:49 PM, Mark Cave-Ayland wrote:
> Currently an object link property is used to pass a reference to the Heathrow
> PIC into the PCI host bridge so that grackle_init_irqs() can connect the PCI
> IRQs to the PIC itself.
> 
> This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
> up the PCI IRQs to the PIC in the Old World machine init function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/pci-host/grackle.c | 19 ++-----------------
>   hw/ppc/mac_oldworld.c |  7 +++++--
>   2 files changed, 7 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> index 57c29b20af..b05facf463 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -28,7 +28,6 @@
>   #include "hw/ppc/mac.h"
>   #include "hw/qdev-properties.h"
>   #include "hw/pci/pci.h"
> -#include "hw/intc/heathrow_pic.h"
>   #include "hw/irq.h"
>   #include "qapi/error.h"
>   #include "qemu/module.h"
> @@ -41,7 +40,6 @@ struct GrackleState {
>       PCIHostState parent_obj;
>   
>       uint32_t ofw_addr;
> -    HeathrowState *pic;
>       qemu_irq irqs[4];
>       MemoryRegion pci_mmio;
>       MemoryRegion pci_hole;
> @@ -62,15 +60,6 @@ static void pci_grackle_set_irq(void *opaque, int irq_num, int level)
>       qemu_set_irq(s->irqs[irq_num], level);
>   }
>   
> -static void grackle_init_irqs(GrackleState *s)
> -{
> -    int i;
> -
> -    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
> -        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), 0x15 + i);
> -    }
> -}
> -
>   static void grackle_realize(DeviceState *dev, Error **errp)
>   {
>       GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev);
> @@ -85,7 +74,6 @@ static void grackle_realize(DeviceState *dev, Error **errp)
>                                        0, 4, TYPE_PCI_BUS);
>   
>       pci_create_simple(phb->bus, 0, "grackle");
> -    grackle_init_irqs(s);
>   }
>   
>   static void grackle_init(Object *obj)
> @@ -106,15 +94,12 @@ static void grackle_init(Object *obj)
>       memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops,
>                             DEVICE(obj), "pci-data-idx", 0x1000);
>   
> -    object_property_add_link(obj, "pic", TYPE_HEATHROW,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>       sysbus_init_mmio(sbd, &phb->conf_mem);
>       sysbus_init_mmio(sbd, &phb->data_mem);
>       sysbus_init_mmio(sbd, &s->pci_hole);
>       sysbus_init_mmio(sbd, &s->pci_io);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>   }
>   
>   static void grackle_pci_realize(PCIDevice *d, Error **errp)
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index d6a76d06dc..05e46ee6fe 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -253,10 +253,9 @@ static void ppc_heathrow_init(MachineState *machine)
>       /* Grackle PCI host bridge */
>       dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
>       qdev_prop_set_uint32(dev, "ofw-addr", 0x80000000);
> -    object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                             &error_abort);
>       s = SYS_BUS_DEVICE(dev);
>       sysbus_realize_and_unref(s, &error_fatal);
> +
>       sysbus_mmio_map(s, 0, GRACKLE_BASE);
>       sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000);
>       /* PCI hole */
> @@ -266,6 +265,10 @@ static void ppc_heathrow_init(MachineState *machine)
>       memory_region_add_subregion(get_system_memory(), 0xfe000000,
>                                   sysbus_mmio_get_region(s, 3));
>   
> +    for (i = 0; i < 4; i++) {
> +        qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x15 + i));

If possible (follow up patch) please describe this 0x15 magic value.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +    }
> +
>       pci_bus = PCI_HOST_BRIDGE(dev)->bus;
>   
>       pci_vga_init(pci_bus);
> 

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[Mark Cave-Ayland]

On 13/10/2020 14:37, Philippe Mathieu-Daudé wrote:

> On 10/13/20 1:49 PM, Mark Cave-Ayland wrote:
>> Currently an object link property is used to pass a reference to the Heathrow
>> PIC into the PCI host bridge so that grackle_init_irqs() can connect the PCI
>> IRQs to the PIC itself.
>>
>> This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
>> up the PCI IRQs to the PIC in the Old World machine init function.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/pci-host/grackle.c | 19 ++-----------------
>>   hw/ppc/mac_oldworld.c |  7 +++++--
>>   2 files changed, 7 insertions(+), 19 deletions(-)
>>
>> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
>> index 57c29b20af..b05facf463 100644
>> --- a/hw/pci-host/grackle.c
>> +++ b/hw/pci-host/grackle.c
>> @@ -28,7 +28,6 @@
>>   #include "hw/ppc/mac.h"
>>   #include "hw/qdev-properties.h"
>>   #include "hw/pci/pci.h"
>> -#include "hw/intc/heathrow_pic.h"
>>   #include "hw/irq.h"
>>   #include "qapi/error.h"
>>   #include "qemu/module.h"
>> @@ -41,7 +40,6 @@ struct GrackleState {
>>       PCIHostState parent_obj;
>>       uint32_t ofw_addr;
>> -    HeathrowState *pic;
>>       qemu_irq irqs[4];
>>       MemoryRegion pci_mmio;
>>       MemoryRegion pci_hole;
>> @@ -62,15 +60,6 @@ static void pci_grackle_set_irq(void *opaque, int irq_num, int 
>> level)
>>       qemu_set_irq(s->irqs[irq_num], level);
>>   }
>> -static void grackle_init_irqs(GrackleState *s)
>> -{
>> -    int i;
>> -
>> -    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
>> -        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), 0x15 + i);
>> -    }
>> -}
>> -
>>   static void grackle_realize(DeviceState *dev, Error **errp)
>>   {
>>       GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev);
>> @@ -85,7 +74,6 @@ static void grackle_realize(DeviceState *dev, Error **errp)
>>                                        0, 4, TYPE_PCI_BUS);
>>       pci_create_simple(phb->bus, 0, "grackle");
>> -    grackle_init_irqs(s);
>>   }
>>   static void grackle_init(Object *obj)
>> @@ -106,15 +94,12 @@ static void grackle_init(Object *obj)
>>       memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops,
>>                             DEVICE(obj), "pci-data-idx", 0x1000);
>> -    object_property_add_link(obj, "pic", TYPE_HEATHROW,
>> -                             (Object **) &s->pic,
>> -                             qdev_prop_allow_set_link_before_realize,
>> -                             0);
>> -
>>       sysbus_init_mmio(sbd, &phb->conf_mem);
>>       sysbus_init_mmio(sbd, &phb->data_mem);
>>       sysbus_init_mmio(sbd, &s->pci_hole);
>>       sysbus_init_mmio(sbd, &s->pci_io);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>>   }
>>   static void grackle_pci_realize(PCIDevice *d, Error **errp)
>> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
>> index d6a76d06dc..05e46ee6fe 100644
>> --- a/hw/ppc/mac_oldworld.c
>> +++ b/hw/ppc/mac_oldworld.c
>> @@ -253,10 +253,9 @@ static void ppc_heathrow_init(MachineState *machine)
>>       /* Grackle PCI host bridge */
>>       dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
>>       qdev_prop_set_uint32(dev, "ofw-addr", 0x80000000);
>> -    object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
>> -                             &error_abort);
>>       s = SYS_BUS_DEVICE(dev);
>>       sysbus_realize_and_unref(s, &error_fatal);
>> +
>>       sysbus_mmio_map(s, 0, GRACKLE_BASE);
>>       sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000);
>>       /* PCI hole */
>> @@ -266,6 +265,10 @@ static void ppc_heathrow_init(MachineState *machine)
>>       memory_region_add_subregion(get_system_memory(), 0xfe000000,
>>                                   sysbus_mmio_get_region(s, 3));
>> +    for (i = 0; i < 4; i++) {
>> +        qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x15 + i));
> 
> If possible (follow up patch) please describe this 0x15 magic value.
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks! Unfortunately I don't have any information about the source of the value, it 
is currently just taken from grackle_init_irqs() above :(


ATB,

Mark.

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[BALATON Zoltan via]

Hello,

Not related to this patch but while you're at it could you please take 
those patches that are already reviewed by you from this series as well?

http://patchwork.ozlabs.org/project/qemu-devel/list/?series=186439

That would help cleaning up my tree and see which patches still need 
changes. Let me know if these need any rebasing and point me to the tree 
on which I should rebase them. Currently they apply to your screamer 
branch I think.

Regards,
BALATON Zoltan

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[Mark Cave-Ayland]

On 13/10/2020 18:05, BALATON Zoltan via wrote:

> Hello,
> 
> Not related to this patch but while you're at it could you please take those patches 
> that are already reviewed by you from this series as well?
> 
> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=186439
> 
> That would help cleaning up my tree and see which patches still need changes. Let me 
> know if these need any rebasing and point me to the tree on which I should rebase 
> them. Currently they apply to your screamer branch I think.

I've queued the grackle/uninorth patches to my qemu-macppc branch, however when I try 
to apply patches from the above series git fails with the following message:

Applying: mac_oldworld: Drop a variable, use get_system_memory() directly
error: sha1 information is lacking or useless (hw/ppc/mac_oldworld.c).
error: could not build fake ancestor

Any chance you can rebase and repost? I'm happy to take patches 3 and 4, and if my 
suggestion of casting the return address via target_ulong works then I think 1 and 2 
are also fine. The I2C stuff I can't really review, and weren't there still issues 
with the SPD data in patch 8 reporting the wrong RAM size?


ATB,

Mark.

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[BALATON Zoltan via]

On Thu, 15 Oct 2020, Mark Cave-Ayland wrote:
> I've queued the grackle/uninorth patches to my qemu-macppc branch, however 
> when I try to apply patches from the above series git fails with the 
> following message:
>
> Applying: mac_oldworld: Drop a variable, use get_system_memory() directly
> error: sha1 information is lacking or useless (hw/ppc/mac_oldworld.c).
> error: could not build fake ancestor

Maybe because these were based on your screamer branch but I could 
cherry-pick them from there to your qemu-macppc branch without issue.

> Any chance you can rebase and repost? I'm happy to take patches 3 and 4, and

I've just posted the rebased series.

> if my suggestion of casting the return address via target_ulong works then I 
> think 1 and 2 are also fine.

Your original comment was:

"Given that this needs to work with both qemu-system-ppc and 
qemu-system-ppc64 would casting bios_addr to target_ulong work?"

This cast only appears in mac_oldworld.c which is qemu-system-ppc only (or 
not different in qemu-system-ppc64 unlike mac_newworld.c) so target_ulong 
there is basically uint32_t. I've changed the cast accordingly but I think 
it does not really matter.

> The I2C stuff I can't really review, and weren't 
> there still issues with the SPD data in patch 8 reporting the wrong RAM size?

As said in previous message the i2c and SPD patches are not quite ready 
yet so I've omitted those from this series, I may rework them later once 
this part is merged and can rebase the rest on top of that. We would also 
need your screamer patches to get the Mac ROM working, what is still 
missing for those?

Regards,
BALATON Zoltan

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[Mark Cave-Ayland]

On 16/10/2020 01:26, BALATON Zoltan via wrote:

> As said in previous message the i2c and SPD patches are not quite ready yet so I've 
> omitted those from this series, I may rework them later once this part is merged and 
> can rebase the rest on top of that. We would also need your screamer patches to get 
> the Mac ROM working, what is still missing for those?

The 2 main reasons for not merging the screamer patches so far are:

1) Hangs in MacOS 9.0 and 9.1 on startup

Probably related to DBDMA interrupts, but I haven't had time to dig into this in much 
detail.

2) Reduced OS X emulation speed

When OS X detects the sound hardware it enables its internal sound engine which does 
2 things: firstly it constantly runs DBDMA requests which execute in the bottom-half 
even if no sound is being generated, so you end up reducing the raw emulation speed 
and secondly the OS X sound engine is floating point based so you end up running a 
lot more background floating point arithmetic in the OS.

I'm open to further ideas as to how this can be improved. The DBDMA overhead could be 
reduced by running DBDMA in the iothread if that is possible but that would be a fair 
bit of work.


ATB,

Mark.

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4179 bytes --]

On Tue, Oct 13, 2020 at 12:49:21PM +0100, Mark Cave-Ayland wrote:
> Currently an object link property is used to pass a reference to the Heathrow
> PIC into the PCI host bridge so that grackle_init_irqs() can connect the PCI
> IRQs to the PIC itself.
> 
> This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
> up the PCI IRQs to the PIC in the Old World machine init function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied to ppc-for-5.2.

> ---
>  hw/pci-host/grackle.c | 19 ++-----------------
>  hw/ppc/mac_oldworld.c |  7 +++++--
>  2 files changed, 7 insertions(+), 19 deletions(-)
> 
> diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> index 57c29b20af..b05facf463 100644
> --- a/hw/pci-host/grackle.c
> +++ b/hw/pci-host/grackle.c
> @@ -28,7 +28,6 @@
>  #include "hw/ppc/mac.h"
>  #include "hw/qdev-properties.h"
>  #include "hw/pci/pci.h"
> -#include "hw/intc/heathrow_pic.h"
>  #include "hw/irq.h"
>  #include "qapi/error.h"
>  #include "qemu/module.h"
> @@ -41,7 +40,6 @@ struct GrackleState {
>      PCIHostState parent_obj;
>  
>      uint32_t ofw_addr;
> -    HeathrowState *pic;
>      qemu_irq irqs[4];
>      MemoryRegion pci_mmio;
>      MemoryRegion pci_hole;
> @@ -62,15 +60,6 @@ static void pci_grackle_set_irq(void *opaque, int irq_num, int level)
>      qemu_set_irq(s->irqs[irq_num], level);
>  }
>  
> -static void grackle_init_irqs(GrackleState *s)
> -{
> -    int i;
> -
> -    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
> -        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), 0x15 + i);
> -    }
> -}
> -
>  static void grackle_realize(DeviceState *dev, Error **errp)
>  {
>      GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev);
> @@ -85,7 +74,6 @@ static void grackle_realize(DeviceState *dev, Error **errp)
>                                       0, 4, TYPE_PCI_BUS);
>  
>      pci_create_simple(phb->bus, 0, "grackle");
> -    grackle_init_irqs(s);
>  }
>  
>  static void grackle_init(Object *obj)
> @@ -106,15 +94,12 @@ static void grackle_init(Object *obj)
>      memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops,
>                            DEVICE(obj), "pci-data-idx", 0x1000);
>  
> -    object_property_add_link(obj, "pic", TYPE_HEATHROW,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>      sysbus_init_mmio(sbd, &phb->conf_mem);
>      sysbus_init_mmio(sbd, &phb->data_mem);
>      sysbus_init_mmio(sbd, &s->pci_hole);
>      sysbus_init_mmio(sbd, &s->pci_io);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>  }
>  
>  static void grackle_pci_realize(PCIDevice *d, Error **errp)
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index d6a76d06dc..05e46ee6fe 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -253,10 +253,9 @@ static void ppc_heathrow_init(MachineState *machine)
>      /* Grackle PCI host bridge */
>      dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
>      qdev_prop_set_uint32(dev, "ofw-addr", 0x80000000);
> -    object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                             &error_abort);
>      s = SYS_BUS_DEVICE(dev);
>      sysbus_realize_and_unref(s, &error_fatal);
> +
>      sysbus_mmio_map(s, 0, GRACKLE_BASE);
>      sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000);
>      /* PCI hole */
> @@ -266,6 +265,10 @@ static void ppc_heathrow_init(MachineState *machine)
>      memory_region_add_subregion(get_system_memory(), 0xfe000000,
>                                  sysbus_mmio_get_region(s, 3));
>  
> +    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);

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[Howard Spoelstra]

[-- Attachment #1: Type: text/plain, Size: 4660 bytes --]

On Fri, Oct 16, 2020 at 2:30 AM David Gibson <david@gibson.dropbear.id.au>
wrote:

> On Tue, Oct 13, 2020 at 12:49:21PM +0100, Mark Cave-Ayland wrote:
> > Currently an object link property is used to pass a reference to the
> Heathrow
> > PIC into the PCI host bridge so that grackle_init_irqs() can connect the
> PCI
> > IRQs to the PIC itself.
> >
> > This can be simplified by defining the PCI IRQs as qdev gpios and then
> wiring
> > up the PCI IRQs to the PIC in the Old World machine init function.
> >
> > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>
> Applied to ppc-for-5.2.
>
> > ---
> >  hw/pci-host/grackle.c | 19 ++-----------------
> >  hw/ppc/mac_oldworld.c |  7 +++++--
> >  2 files changed, 7 insertions(+), 19 deletions(-)
> >
> > diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c
> > index 57c29b20af..b05facf463 100644
> > --- a/hw/pci-host/grackle.c
> > +++ b/hw/pci-host/grackle.c
> > @@ -28,7 +28,6 @@
> >  #include "hw/ppc/mac.h"
> >  #include "hw/qdev-properties.h"
> >  #include "hw/pci/pci.h"
> > -#include "hw/intc/heathrow_pic.h"
> >  #include "hw/irq.h"
> >  #include "qapi/error.h"
> >  #include "qemu/module.h"
> > @@ -41,7 +40,6 @@ struct GrackleState {
> >      PCIHostState parent_obj;
> >
> >      uint32_t ofw_addr;
> > -    HeathrowState *pic;
> >      qemu_irq irqs[4];
> >      MemoryRegion pci_mmio;
> >      MemoryRegion pci_hole;
> > @@ -62,15 +60,6 @@ static void pci_grackle_set_irq(void *opaque, int
> irq_num, int level)
> >      qemu_set_irq(s->irqs[irq_num], level);
> >  }
> >
> > -static void grackle_init_irqs(GrackleState *s)
> > -{
> > -    int i;
> > -
> > -    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
> > -        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), 0x15 + i);
> > -    }
> > -}
> > -
> >  static void grackle_realize(DeviceState *dev, Error **errp)
> >  {
> >      GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev);
> > @@ -85,7 +74,6 @@ static void grackle_realize(DeviceState *dev, Error
> **errp)
> >                                       0, 4, TYPE_PCI_BUS);
> >
> >      pci_create_simple(phb->bus, 0, "grackle");
> > -    grackle_init_irqs(s);
> >  }
> >
> >  static void grackle_init(Object *obj)
> > @@ -106,15 +94,12 @@ static void grackle_init(Object *obj)
> >      memory_region_init_io(&phb->data_mem, obj, &pci_host_data_le_ops,
> >                            DEVICE(obj), "pci-data-idx", 0x1000);
> >
> > -    object_property_add_link(obj, "pic", TYPE_HEATHROW,
> > -                             (Object **) &s->pic,
> > -                             qdev_prop_allow_set_link_before_realize,
> > -                             0);
> > -
> >      sysbus_init_mmio(sbd, &phb->conf_mem);
> >      sysbus_init_mmio(sbd, &phb->data_mem);
> >      sysbus_init_mmio(sbd, &s->pci_hole);
> >      sysbus_init_mmio(sbd, &s->pci_io);
> > +
> > +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
> >  }
> >
> >  static void grackle_pci_realize(PCIDevice *d, Error **errp)
> > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> > index d6a76d06dc..05e46ee6fe 100644
> > --- a/hw/ppc/mac_oldworld.c
> > +++ b/hw/ppc/mac_oldworld.c
> > @@ -253,10 +253,9 @@ static void ppc_heathrow_init(MachineState *machine)
> >      /* Grackle PCI host bridge */
> >      dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);
> >      qdev_prop_set_uint32(dev, "ofw-addr", 0x80000000);
> > -    object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> > -                             &error_abort);
> >      s = SYS_BUS_DEVICE(dev);
> >      sysbus_realize_and_unref(s, &error_fatal);
> > +
> >      sysbus_mmio_map(s, 0, GRACKLE_BASE);
> >      sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000);
> >      /* PCI hole */
> > @@ -266,6 +265,10 @@ static void ppc_heathrow_init(MachineState *machine)
> >      memory_region_add_subregion(get_system_memory(), 0xfe000000,
> >                                  sysbus_mmio_get_region(s, 3));
> >
> > +    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);
>
>
> Hi,

I see compilation of the current ppc-for-5.2 branch fail with:

../hw/pci-host/grackle.c: In function ‘grackle_realize’:
../hw/pci-host/grackle.c:68:11: error: ‘GrackleState’ has no member named
‘pic’
   68 |     if (!s->pic) {
      |           ^~
make: *** [Makefile.ninja:1741: libcommon.fa.p/hw_pci-host_grackle.c.o]
Error 1

Best,
Howard

[-- Attachment #2: Type: text/html, Size: 6114 bytes --]

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[Mark Cave-Ayland]

On 16/10/2020 07:45, Howard Spoelstra wrote:

> Hi,
> 
> I see compilation of the current ppc-for-5.2 branch fail with:
> 
> ../hw/pci-host/grackle.c: In function ‘grackle_realize’:
> ../hw/pci-host/grackle.c:68:11: error: ‘GrackleState’ has no member named ‘pic’
>     68 |     if (!s->pic) {
>        |           ^~
> make: *** [Makefile.ninja:1741: libcommon.fa.p/hw_pci-host_grackle.c.o] Error 1
> 
> Best,
> Howard

I see - as per the cover letter, my series is a replacement for Phil's original patch 
at https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02988.html (the PIC link 
is now completely removed), so the solution here is to drop patch 7daac97 
"hw/pci-host/grackle: Verify PIC link is properly set".


ATB,

Mark.

Re: [PATCH v2 2/3] grackle: use qdev gpios for PCI IRQs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

On Fri, Oct 16, 2020 at 07:53:10AM +0100, Mark Cave-Ayland wrote:
> On 16/10/2020 07:45, Howard Spoelstra wrote:
> 
> > Hi,
> > 
> > I see compilation of the current ppc-for-5.2 branch fail with:
> > 
> > ../hw/pci-host/grackle.c: In function ‘grackle_realize’:
> > ../hw/pci-host/grackle.c:68:11: error: ‘GrackleState’ has no member named ‘pic’
> >     68 |     if (!s->pic) {
> >        |           ^~
> > make: *** [Makefile.ninja:1741: libcommon.fa.p/hw_pci-host_grackle.c.o] Error 1
> > 
> > Best,
> > Howard
> 
> I see - as per the cover letter, my series is a replacement for Phil's
> original patch at
> https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg02988.html (the PIC
> link is now completely removed), so the solution here is to drop patch
> 7daac97 "hw/pci-host/grackle: Verify PIC link is properly set".

Ok, I've removed that from my ppc-for-5.2 tree.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/3] uninorth: use qdev gpios for PCI IRQs

[Mark Cave-Ayland]

Currently an object link property is used to pass a reference to the OpenPIC
into the PCI host bridge so that pci_unin_init_irqs() can connect the PCI
IRQs to the PIC itself.

This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
up the PCI IRQs to the PIC in the New World machine init function.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/pci-host/uninorth.c         | 45 +++++++---------------------------
 hw/ppc/mac_newworld.c          | 24 ++++++++++++------
 include/hw/pci-host/uninorth.h |  2 --
 3 files changed, 25 insertions(+), 46 deletions(-)

diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
index 1ed1072eeb..0c0a9ecee1 100644
--- a/hw/pci-host/uninorth.c
+++ b/hw/pci-host/uninorth.c
@@ -32,8 +32,6 @@
 #include "hw/pci-host/uninorth.h"
 #include "trace.h"
 
-static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
-
 static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
 {
     return (irq_num + (pci_dev->devfn >> 3)) & 3;
@@ -43,7 +41,7 @@ static void pci_unin_set_irq(void *opaque, int irq_num, int level)
 {
     UNINHostState *s = opaque;
 
-    trace_unin_set_irq(unin_irq_line[irq_num], level);
+    trace_unin_set_irq(irq_num, level);
     qemu_set_irq(s->irqs[irq_num], level);
 }
 
@@ -112,15 +110,6 @@ static const MemoryRegionOps unin_data_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static void pci_unin_init_irqs(UNINHostState *s)
-{
-    int i;
-
-    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
-        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), unin_irq_line[i]);
-    }
-}
-
 static char *pci_unin_main_ofw_unit_address(const SysBusDevice *dev)
 {
     UNINHostState *s = UNI_NORTH_PCI_HOST_BRIDGE(dev);
@@ -141,7 +130,6 @@ static void pci_unin_main_realize(DeviceState *dev, Error **errp)
                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
 
     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-pci");
-    pci_unin_init_irqs(s);
 
     /* DEC 21154 bridge */
 #if 0
@@ -172,15 +160,12 @@ static void pci_unin_main_init(Object *obj)
                              "unin-pci-hole", &s->pci_mmio,
                              0x80000000ULL, 0x10000000ULL);
 
-    object_property_add_link(obj, "pic", TYPE_OPENPIC,
-                             (Object **) &s->pic,
-                             qdev_prop_allow_set_link_before_realize,
-                             0);
-
     sysbus_init_mmio(sbd, &h->conf_mem);
     sysbus_init_mmio(sbd, &h->data_mem);
     sysbus_init_mmio(sbd, &s->pci_hole);
     sysbus_init_mmio(sbd, &s->pci_io);
+
+    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
 }
 
 static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
@@ -196,7 +181,6 @@ static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
 
     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "u3-agp");
-    pci_unin_init_irqs(s);
 }
 
 static void pci_u3_agp_init(Object *obj)
@@ -220,15 +204,12 @@ static void pci_u3_agp_init(Object *obj)
                              "unin-pci-hole", &s->pci_mmio,
                              0x80000000ULL, 0x70000000ULL);
 
-    object_property_add_link(obj, "pic", TYPE_OPENPIC,
-                             (Object **) &s->pic,
-                             qdev_prop_allow_set_link_before_realize,
-                             0);
-
     sysbus_init_mmio(sbd, &h->conf_mem);
     sysbus_init_mmio(sbd, &h->data_mem);
     sysbus_init_mmio(sbd, &s->pci_hole);
     sysbus_init_mmio(sbd, &s->pci_io);
+
+    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
 }
 
 static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
@@ -244,7 +225,6 @@ static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
 
     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-agp");
-    pci_unin_init_irqs(s);
 }
 
 static void pci_unin_agp_init(Object *obj)
@@ -259,13 +239,10 @@ static void pci_unin_agp_init(Object *obj)
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
                           obj, "unin-agp-conf-data", 0x1000);
 
-    object_property_add_link(obj, "pic", TYPE_OPENPIC,
-                             (Object **) &s->pic,
-                             qdev_prop_allow_set_link_before_realize,
-                             0);
-
     sysbus_init_mmio(sbd, &h->conf_mem);
     sysbus_init_mmio(sbd, &h->data_mem);
+
+    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
 }
 
 static void pci_unin_internal_realize(DeviceState *dev, Error **errp)
@@ -281,7 +258,6 @@ static void pci_unin_internal_realize(DeviceState *dev, Error **errp)
                                    PCI_DEVFN(14, 0), 4, TYPE_PCI_BUS);
 
     pci_create_simple(h->bus, PCI_DEVFN(14, 0), "uni-north-internal-pci");
-    pci_unin_init_irqs(s);
 }
 
 static void pci_unin_internal_init(Object *obj)
@@ -296,13 +272,10 @@ static void pci_unin_internal_init(Object *obj)
     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
                           obj, "unin-pci-conf-data", 0x1000);
 
-    object_property_add_link(obj, "pic", TYPE_OPENPIC,
-                             (Object **) &s->pic,
-                             qdev_prop_allow_set_link_before_realize,
-                             0);
-
     sysbus_init_mmio(sbd, &h->conf_mem);
     sysbus_init_mmio(sbd, &h->data_mem);
+
+    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
 }
 
 static void unin_main_pci_host_realize(PCIDevice *d, Error **errp)
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 6f5ef2e782..7a8dc09c8d 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -303,8 +303,6 @@ static void ppc_core99_init(MachineState *machine)
         /* 970 gets a U3 bus */
         /* Uninorth AGP bus */
         dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
-        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
-                                 &error_abort);
         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
         uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
         s = SYS_BUS_DEVICE(dev);
@@ -317,32 +315,38 @@ static void ppc_core99_init(MachineState *machine)
         sysbus_mmio_map(s, 0, 0xf0800000);
         sysbus_mmio_map(s, 1, 0xf0c00000);
 
+        for (i = 0; i < 4; i++) {
+            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
+        }
+
         machine_arch = ARCH_MAC99_U3;
     } else {
         /* Use values found on a real PowerMac */
         /* Uninorth AGP bus */
         dev = qdev_new(TYPE_UNI_NORTH_AGP_HOST_BRIDGE);
-        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
-                                 &error_abort);
         s = SYS_BUS_DEVICE(dev);
         sysbus_realize_and_unref(s, &error_fatal);
         sysbus_mmio_map(s, 0, 0xf0800000);
         sysbus_mmio_map(s, 1, 0xf0c00000);
 
+        for (i = 0; i < 4; i++) {
+            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
+        }
+
         /* Uninorth internal bus */
         dev = qdev_new(TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE);
-        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
-                                 &error_abort);
         s = SYS_BUS_DEVICE(dev);
         sysbus_realize_and_unref(s, &error_fatal);
         sysbus_mmio_map(s, 0, 0xf4800000);
         sysbus_mmio_map(s, 1, 0xf4c00000);
 
+        for (i = 0; i < 4; i++) {
+            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
+        }
+
         /* Uninorth main bus */
         dev = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
         qdev_prop_set_uint32(dev, "ofw-addr", 0xf2000000);
-        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
-                                 &error_abort);
         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
         uninorth_pci = UNI_NORTH_PCI_HOST_BRIDGE(dev);
         s = SYS_BUS_DEVICE(dev);
@@ -355,6 +359,10 @@ static void ppc_core99_init(MachineState *machine)
         sysbus_mmio_map(s, 0, 0xf2800000);
         sysbus_mmio_map(s, 1, 0xf2c00000);
 
+        for (i = 0; i < 4; i++) {
+            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
+        }
+
         machine_arch = ARCH_MAC99;
     }
 
diff --git a/include/hw/pci-host/uninorth.h b/include/hw/pci-host/uninorth.h
index a6ba5f21a8..62bd81e721 100644
--- a/include/hw/pci-host/uninorth.h
+++ b/include/hw/pci-host/uninorth.h
@@ -26,7 +26,6 @@
 #define UNINORTH_H
 
 #include "hw/pci/pci_host.h"
-#include "hw/ppc/openpic.h"
 #include "qom/object.h"
 
 /* UniNorth version */
@@ -51,7 +50,6 @@ struct UNINHostState {
     PCIHostState parent_obj;
 
     uint32_t ofw_addr;
-    OpenPICState *pic;
     qemu_irq irqs[4];
     MemoryRegion pci_mmio;
     MemoryRegion pci_hole;
-- 
2.20.1

Re: [PATCH v2 3/3] uninorth: use qdev gpios for PCI IRQs

[BALATON Zoltan via]

On Tue, 13 Oct 2020, Mark Cave-Ayland wrote:
> Currently an object link property is used to pass a reference to the OpenPIC
> into the PCI host bridge so that pci_unin_init_irqs() can connect the PCI
> IRQs to the PIC itself.
>
> This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
> up the PCI IRQs to the PIC in the New World machine init function.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/pci-host/uninorth.c         | 45 +++++++---------------------------
> hw/ppc/mac_newworld.c          | 24 ++++++++++++------
> include/hw/pci-host/uninorth.h |  2 --
> 3 files changed, 25 insertions(+), 46 deletions(-)
>
> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
> index 1ed1072eeb..0c0a9ecee1 100644
> --- a/hw/pci-host/uninorth.c
> +++ b/hw/pci-host/uninorth.c
> @@ -32,8 +32,6 @@
> #include "hw/pci-host/uninorth.h"
> #include "trace.h"
>
> -static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
> -
> static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
> {
>     return (irq_num + (pci_dev->devfn >> 3)) & 3;
> @@ -43,7 +41,7 @@ static void pci_unin_set_irq(void *opaque, int irq_num, int level)
> {
>     UNINHostState *s = opaque;
>
> -    trace_unin_set_irq(unin_irq_line[irq_num], level);
> +    trace_unin_set_irq(irq_num, level);
>     qemu_set_irq(s->irqs[irq_num], level);
> }
>
> @@ -112,15 +110,6 @@ static const MemoryRegionOps unin_data_ops = {
>     .endianness = DEVICE_LITTLE_ENDIAN,
> };
>
> -static void pci_unin_init_irqs(UNINHostState *s)
> -{
> -    int i;
> -
> -    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
> -        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), unin_irq_line[i]);
> -    }
> -}
> -
> static char *pci_unin_main_ofw_unit_address(const SysBusDevice *dev)
> {
>     UNINHostState *s = UNI_NORTH_PCI_HOST_BRIDGE(dev);
> @@ -141,7 +130,6 @@ static void pci_unin_main_realize(DeviceState *dev, Error **errp)
>                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>
>     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-pci");
> -    pci_unin_init_irqs(s);
>
>     /* DEC 21154 bridge */
> #if 0
> @@ -172,15 +160,12 @@ static void pci_unin_main_init(Object *obj)
>                              "unin-pci-hole", &s->pci_mmio,
>                              0x80000000ULL, 0x10000000ULL);
>
> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>     sysbus_init_mmio(sbd, &h->conf_mem);
>     sysbus_init_mmio(sbd, &h->data_mem);
>     sysbus_init_mmio(sbd, &s->pci_hole);
>     sysbus_init_mmio(sbd, &s->pci_io);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
> }
>
> static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
> @@ -196,7 +181,6 @@ static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
>                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>
>     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "u3-agp");
> -    pci_unin_init_irqs(s);
> }
>
> static void pci_u3_agp_init(Object *obj)
> @@ -220,15 +204,12 @@ static void pci_u3_agp_init(Object *obj)
>                              "unin-pci-hole", &s->pci_mmio,
>                              0x80000000ULL, 0x70000000ULL);
>
> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>     sysbus_init_mmio(sbd, &h->conf_mem);
>     sysbus_init_mmio(sbd, &h->data_mem);
>     sysbus_init_mmio(sbd, &s->pci_hole);
>     sysbus_init_mmio(sbd, &s->pci_io);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
> }
>
> static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
> @@ -244,7 +225,6 @@ static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
>                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>
>     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-agp");
> -    pci_unin_init_irqs(s);
> }
>
> static void pci_unin_agp_init(Object *obj)
> @@ -259,13 +239,10 @@ static void pci_unin_agp_init(Object *obj)
>     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
>                           obj, "unin-agp-conf-data", 0x1000);
>
> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>     sysbus_init_mmio(sbd, &h->conf_mem);
>     sysbus_init_mmio(sbd, &h->data_mem);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
> }
>
> static void pci_unin_internal_realize(DeviceState *dev, Error **errp)
> @@ -281,7 +258,6 @@ static void pci_unin_internal_realize(DeviceState *dev, Error **errp)
>                                    PCI_DEVFN(14, 0), 4, TYPE_PCI_BUS);
>
>     pci_create_simple(h->bus, PCI_DEVFN(14, 0), "uni-north-internal-pci");
> -    pci_unin_init_irqs(s);
> }
>
> static void pci_unin_internal_init(Object *obj)
> @@ -296,13 +272,10 @@ static void pci_unin_internal_init(Object *obj)
>     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
>                           obj, "unin-pci-conf-data", 0x1000);
>
> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>     sysbus_init_mmio(sbd, &h->conf_mem);
>     sysbus_init_mmio(sbd, &h->data_mem);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
> }
>
> static void unin_main_pci_host_realize(PCIDevice *d, Error **errp)
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 6f5ef2e782..7a8dc09c8d 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -303,8 +303,6 @@ static void ppc_core99_init(MachineState *machine)
>         /* 970 gets a U3 bus */
>         /* Uninorth AGP bus */
>         dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                                 &error_abort);
>         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>         uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
>         s = SYS_BUS_DEVICE(dev);
> @@ -317,32 +315,38 @@ static void ppc_core99_init(MachineState *machine)
>         sysbus_mmio_map(s, 0, 0xf0800000);
>         sysbus_mmio_map(s, 1, 0xf0c00000);
>
> +        for (i = 0; i < 4; i++) {

Philippe had a series that replaced all these with PCI_NUM_IRQS or similar 
constant. Should you use that instead in new additions?

This is probably not modelled faithfuly (the whole mac99 machine is a hack 
to get the most OSes run, not emulating an actual machine) but I wonder if 
it's correct to connect all these irqs for all the different busses to the 
same pic lines? Is that how it is on a real PowerMac3,1? (This is just a 
question not something that should be fixed in this patch which is a nice 
clean up anyway, this just made it more obvious to me what's happening so 
maybe we can find out a difference in emulation compared to real hardware 
that could be fixed in the future.)

Regards,
BALATON Zoltan

> +            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
> +        }
> +
>         machine_arch = ARCH_MAC99_U3;
>     } else {
>         /* Use values found on a real PowerMac */
>         /* Uninorth AGP bus */
>         dev = qdev_new(TYPE_UNI_NORTH_AGP_HOST_BRIDGE);
> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                                 &error_abort);
>         s = SYS_BUS_DEVICE(dev);
>         sysbus_realize_and_unref(s, &error_fatal);
>         sysbus_mmio_map(s, 0, 0xf0800000);
>         sysbus_mmio_map(s, 1, 0xf0c00000);
>
> +        for (i = 0; i < 4; i++) {
> +            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
> +        }
> +
>         /* Uninorth internal bus */
>         dev = qdev_new(TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE);
> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                                 &error_abort);
>         s = SYS_BUS_DEVICE(dev);
>         sysbus_realize_and_unref(s, &error_fatal);
>         sysbus_mmio_map(s, 0, 0xf4800000);
>         sysbus_mmio_map(s, 1, 0xf4c00000);
>
> +        for (i = 0; i < 4; i++) {
> +            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
> +        }
> +
>         /* Uninorth main bus */
>         dev = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
>         qdev_prop_set_uint32(dev, "ofw-addr", 0xf2000000);
> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                                 &error_abort);
>         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>         uninorth_pci = UNI_NORTH_PCI_HOST_BRIDGE(dev);
>         s = SYS_BUS_DEVICE(dev);
> @@ -355,6 +359,10 @@ static void ppc_core99_init(MachineState *machine)
>         sysbus_mmio_map(s, 0, 0xf2800000);
>         sysbus_mmio_map(s, 1, 0xf2c00000);
>
> +        for (i = 0; i < 4; i++) {
> +            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
> +        }
> +
>         machine_arch = ARCH_MAC99;
>     }
>
> diff --git a/include/hw/pci-host/uninorth.h b/include/hw/pci-host/uninorth.h
> index a6ba5f21a8..62bd81e721 100644
> --- a/include/hw/pci-host/uninorth.h
> +++ b/include/hw/pci-host/uninorth.h
> @@ -26,7 +26,6 @@
> #define UNINORTH_H
>
> #include "hw/pci/pci_host.h"
> -#include "hw/ppc/openpic.h"
> #include "qom/object.h"
>
> /* UniNorth version */
> @@ -51,7 +50,6 @@ struct UNINHostState {
>     PCIHostState parent_obj;
>
>     uint32_t ofw_addr;
> -    OpenPICState *pic;
>     qemu_irq irqs[4];
>     MemoryRegion pci_mmio;
>     MemoryRegion pci_hole;
>

Re: [PATCH v2 3/3] uninorth: use qdev gpios for PCI IRQs

[Mark Cave-Ayland]

On 13/10/2020 14:38, BALATON Zoltan via wrote:

> On Tue, 13 Oct 2020, Mark Cave-Ayland wrote:
>> Currently an object link property is used to pass a reference to the OpenPIC
>> into the PCI host bridge so that pci_unin_init_irqs() can connect the PCI
>> IRQs to the PIC itself.
>>
>> This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
>> up the PCI IRQs to the PIC in the New World machine init function.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/pci-host/uninorth.c         | 45 +++++++---------------------------
>> hw/ppc/mac_newworld.c          | 24 ++++++++++++------
>> include/hw/pci-host/uninorth.h |  2 --
>> 3 files changed, 25 insertions(+), 46 deletions(-)
>>
>> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
>> index 1ed1072eeb..0c0a9ecee1 100644
>> --- a/hw/pci-host/uninorth.c
>> +++ b/hw/pci-host/uninorth.c
>> @@ -32,8 +32,6 @@
>> #include "hw/pci-host/uninorth.h"
>> #include "trace.h"
>>
>> -static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
>> -
>> static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
>> {
>>     return (irq_num + (pci_dev->devfn >> 3)) & 3;
>> @@ -43,7 +41,7 @@ static void pci_unin_set_irq(void *opaque, int irq_num, int level)
>> {
>>     UNINHostState *s = opaque;
>>
>> -    trace_unin_set_irq(unin_irq_line[irq_num], level);
>> +    trace_unin_set_irq(irq_num, level);
>>     qemu_set_irq(s->irqs[irq_num], level);
>> }
>>
>> @@ -112,15 +110,6 @@ static const MemoryRegionOps unin_data_ops = {
>>     .endianness = DEVICE_LITTLE_ENDIAN,
>> };
>>
>> -static void pci_unin_init_irqs(UNINHostState *s)
>> -{
>> -    int i;
>> -
>> -    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
>> -        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), unin_irq_line[i]);
>> -    }
>> -}
>> -
>> static char *pci_unin_main_ofw_unit_address(const SysBusDevice *dev)
>> {
>>     UNINHostState *s = UNI_NORTH_PCI_HOST_BRIDGE(dev);
>> @@ -141,7 +130,6 @@ static void pci_unin_main_realize(DeviceState *dev, Error **errp)
>>                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>>
>>     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-pci");
>> -    pci_unin_init_irqs(s);
>>
>>     /* DEC 21154 bridge */
>> #if 0
>> @@ -172,15 +160,12 @@ static void pci_unin_main_init(Object *obj)
>>                              "unin-pci-hole", &s->pci_mmio,
>>                              0x80000000ULL, 0x10000000ULL);
>>
>> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
>> -                             (Object **) &s->pic,
>> -                             qdev_prop_allow_set_link_before_realize,
>> -                             0);
>> -
>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>     sysbus_init_mmio(sbd, &h->data_mem);
>>     sysbus_init_mmio(sbd, &s->pci_hole);
>>     sysbus_init_mmio(sbd, &s->pci_io);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>> }
>>
>> static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
>> @@ -196,7 +181,6 @@ static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
>>                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>>
>>     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "u3-agp");
>> -    pci_unin_init_irqs(s);
>> }
>>
>> static void pci_u3_agp_init(Object *obj)
>> @@ -220,15 +204,12 @@ static void pci_u3_agp_init(Object *obj)
>>                              "unin-pci-hole", &s->pci_mmio,
>>                              0x80000000ULL, 0x70000000ULL);
>>
>> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
>> -                             (Object **) &s->pic,
>> -                             qdev_prop_allow_set_link_before_realize,
>> -                             0);
>> -
>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>     sysbus_init_mmio(sbd, &h->data_mem);
>>     sysbus_init_mmio(sbd, &s->pci_hole);
>>     sysbus_init_mmio(sbd, &s->pci_io);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>> }
>>
>> static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
>> @@ -244,7 +225,6 @@ static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
>>                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>>
>>     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-agp");
>> -    pci_unin_init_irqs(s);
>> }
>>
>> static void pci_unin_agp_init(Object *obj)
>> @@ -259,13 +239,10 @@ static void pci_unin_agp_init(Object *obj)
>>     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
>>                           obj, "unin-agp-conf-data", 0x1000);
>>
>> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
>> -                             (Object **) &s->pic,
>> -                             qdev_prop_allow_set_link_before_realize,
>> -                             0);
>> -
>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>     sysbus_init_mmio(sbd, &h->data_mem);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>> }
>>
>> static void pci_unin_internal_realize(DeviceState *dev, Error **errp)
>> @@ -281,7 +258,6 @@ static void pci_unin_internal_realize(DeviceState *dev, Error 
>> **errp)
>>                                    PCI_DEVFN(14, 0), 4, TYPE_PCI_BUS);
>>
>>     pci_create_simple(h->bus, PCI_DEVFN(14, 0), "uni-north-internal-pci");
>> -    pci_unin_init_irqs(s);
>> }
>>
>> static void pci_unin_internal_init(Object *obj)
>> @@ -296,13 +272,10 @@ static void pci_unin_internal_init(Object *obj)
>>     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
>>                           obj, "unin-pci-conf-data", 0x1000);
>>
>> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
>> -                             (Object **) &s->pic,
>> -                             qdev_prop_allow_set_link_before_realize,
>> -                             0);
>> -
>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>     sysbus_init_mmio(sbd, &h->data_mem);
>> +
>> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>> }
>>
>> static void unin_main_pci_host_realize(PCIDevice *d, Error **errp)
>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>> index 6f5ef2e782..7a8dc09c8d 100644
>> --- a/hw/ppc/mac_newworld.c
>> +++ b/hw/ppc/mac_newworld.c
>> @@ -303,8 +303,6 @@ static void ppc_core99_init(MachineState *machine)
>>         /* 970 gets a U3 bus */
>>         /* Uninorth AGP bus */
>>         dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
>> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
>> -                                 &error_abort);
>>         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>         uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
>>         s = SYS_BUS_DEVICE(dev);
>> @@ -317,32 +315,38 @@ static void ppc_core99_init(MachineState *machine)
>>         sysbus_mmio_map(s, 0, 0xf0800000);
>>         sysbus_mmio_map(s, 1, 0xf0c00000);
>>
>> +        for (i = 0; i < 4; i++) {
> 
> Philippe had a series that replaced all these with PCI_NUM_IRQS or similar constant. 
> Should you use that instead in new additions?

I'm not sure that's directly relevant here - my understanding was that PCI_NUM_IRQS 
represents the number of IRQs on the device, not the PCI host bridge. Certainly it 
could be argued that there is a 1:1 correspondence in this case, however I don't have 
any documentation to support this so it doesn't feel right to make these values 
directly equivalent.

> This is probably not modelled faithfuly (the whole mac99 machine is a hack to get the 
> most OSes run, not emulating an actual machine) but I wonder if it's correct to 
> connect all these irqs for all the different busses to the same pic lines? Is that 
> how it is on a real PowerMac3,1? (This is just a question not something that should 
> be fixed in this patch which is a nice clean up anyway, this just made it more 
> obvious to me what's happening so maybe we can find out a difference in emulation 
> compared to real hardware that could be fixed in the future.)

Yeah I spotted that too when I was writing the patch. In its current form it keeps 
the existing behaviour as-is, but this is something that may be worth investigating 
later. I'm not aware of any documentation explaining how the PCI/AGP bridges are 
wired up on a Mac, so it might end up being a case of trawling through driver source 
code to try and get some answers...


ATB,

Mark.

Re: [PATCH v2 3/3] uninorth: use qdev gpios for PCI IRQs

[BALATON Zoltan via]

[-- Attachment #1: Type: text/plain, Size: 9885 bytes --]

On Tue, 13 Oct 2020, Mark Cave-Ayland wrote:
> On 13/10/2020 14:38, BALATON Zoltan via wrote:
>> On Tue, 13 Oct 2020, Mark Cave-Ayland wrote:
>>> Currently an object link property is used to pass a reference to the 
>>> OpenPIC
>>> into the PCI host bridge so that pci_unin_init_irqs() can connect the PCI
>>> IRQs to the PIC itself.
>>> 
>>> This can be simplified by defining the PCI IRQs as qdev gpios and then 
>>> wiring
>>> up the PCI IRQs to the PIC in the New World machine init function.
>>> 
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> hw/pci-host/uninorth.c         | 45 +++++++---------------------------
>>> hw/ppc/mac_newworld.c          | 24 ++++++++++++------
>>> include/hw/pci-host/uninorth.h |  2 --
>>> 3 files changed, 25 insertions(+), 46 deletions(-)
>>> 
>>> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
>>> index 1ed1072eeb..0c0a9ecee1 100644
>>> --- a/hw/pci-host/uninorth.c
>>> +++ b/hw/pci-host/uninorth.c
>>> @@ -32,8 +32,6 @@
>>> #include "hw/pci-host/uninorth.h"
>>> #include "trace.h"
>>> 
>>> -static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
>>> -
>>> static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
>>> {
>>>     return (irq_num + (pci_dev->devfn >> 3)) & 3;
>>> @@ -43,7 +41,7 @@ static void pci_unin_set_irq(void *opaque, int irq_num, 
>>> int level)
>>> {
>>>     UNINHostState *s = opaque;
>>> 
>>> -    trace_unin_set_irq(unin_irq_line[irq_num], level);
>>> +    trace_unin_set_irq(irq_num, level);
>>>     qemu_set_irq(s->irqs[irq_num], level);
>>> }
>>> 
>>> @@ -112,15 +110,6 @@ static const MemoryRegionOps unin_data_ops = {
>>>     .endianness = DEVICE_LITTLE_ENDIAN,
>>> };
>>> 
>>> -static void pci_unin_init_irqs(UNINHostState *s)
>>> -{
>>> -    int i;
>>> -
>>> -    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
>>> -        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), unin_irq_line[i]);
>>> -    }
>>> -}
>>> -
>>> static char *pci_unin_main_ofw_unit_address(const SysBusDevice *dev)
>>> {
>>>     UNINHostState *s = UNI_NORTH_PCI_HOST_BRIDGE(dev);
>>> @@ -141,7 +130,6 @@ static void pci_unin_main_realize(DeviceState *dev, 
>>> Error **errp)
>>>                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>>> 
>>>     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-pci");
>>> -    pci_unin_init_irqs(s);
>>> 
>>>     /* DEC 21154 bridge */
>>> #if 0
>>> @@ -172,15 +160,12 @@ static void pci_unin_main_init(Object *obj)
>>>                              "unin-pci-hole", &s->pci_mmio,
>>>                              0x80000000ULL, 0x10000000ULL);
>>> 
>>> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
>>> -                             (Object **) &s->pic,
>>> -                             qdev_prop_allow_set_link_before_realize,
>>> -                             0);
>>> -
>>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>>     sysbus_init_mmio(sbd, &h->data_mem);
>>>     sysbus_init_mmio(sbd, &s->pci_hole);
>>>     sysbus_init_mmio(sbd, &s->pci_io);
>>> +
>>> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>>> }
>>> 
>>> static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
>>> @@ -196,7 +181,6 @@ static void pci_u3_agp_realize(DeviceState *dev, Error 
>>> **errp)
>>>                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>>> 
>>>     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "u3-agp");
>>> -    pci_unin_init_irqs(s);
>>> }
>>> 
>>> static void pci_u3_agp_init(Object *obj)
>>> @@ -220,15 +204,12 @@ static void pci_u3_agp_init(Object *obj)
>>>                              "unin-pci-hole", &s->pci_mmio,
>>>                              0x80000000ULL, 0x70000000ULL);
>>> 
>>> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
>>> -                             (Object **) &s->pic,
>>> -                             qdev_prop_allow_set_link_before_realize,
>>> -                             0);
>>> -
>>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>>     sysbus_init_mmio(sbd, &h->data_mem);
>>>     sysbus_init_mmio(sbd, &s->pci_hole);
>>>     sysbus_init_mmio(sbd, &s->pci_io);
>>> +
>>> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>>> }
>>> 
>>> static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
>>> @@ -244,7 +225,6 @@ static void pci_unin_agp_realize(DeviceState *dev, 
>>> Error **errp)
>>>                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>>> 
>>>     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-agp");
>>> -    pci_unin_init_irqs(s);
>>> }
>>> 
>>> static void pci_unin_agp_init(Object *obj)
>>> @@ -259,13 +239,10 @@ static void pci_unin_agp_init(Object *obj)
>>>     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
>>>                           obj, "unin-agp-conf-data", 0x1000);
>>> 
>>> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
>>> -                             (Object **) &s->pic,
>>> -                             qdev_prop_allow_set_link_before_realize,
>>> -                             0);
>>> -
>>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>>     sysbus_init_mmio(sbd, &h->data_mem);
>>> +
>>> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>>> }
>>> 
>>> static void pci_unin_internal_realize(DeviceState *dev, Error **errp)
>>> @@ -281,7 +258,6 @@ static void pci_unin_internal_realize(DeviceState 
>>> *dev, Error **errp)
>>>                                    PCI_DEVFN(14, 0), 4, TYPE_PCI_BUS);
>>> 
>>>     pci_create_simple(h->bus, PCI_DEVFN(14, 0), "uni-north-internal-pci");
>>> -    pci_unin_init_irqs(s);
>>> }
>>> 
>>> static void pci_unin_internal_init(Object *obj)
>>> @@ -296,13 +272,10 @@ static void pci_unin_internal_init(Object *obj)
>>>     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
>>>                           obj, "unin-pci-conf-data", 0x1000);
>>> 
>>> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
>>> -                             (Object **) &s->pic,
>>> -                             qdev_prop_allow_set_link_before_realize,
>>> -                             0);
>>> -
>>>     sysbus_init_mmio(sbd, &h->conf_mem);
>>>     sysbus_init_mmio(sbd, &h->data_mem);
>>> +
>>> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>>> }
>>> 
>>> static void unin_main_pci_host_realize(PCIDevice *d, Error **errp)
>>> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
>>> index 6f5ef2e782..7a8dc09c8d 100644
>>> --- a/hw/ppc/mac_newworld.c
>>> +++ b/hw/ppc/mac_newworld.c
>>> @@ -303,8 +303,6 @@ static void ppc_core99_init(MachineState *machine)
>>>         /* 970 gets a U3 bus */
>>>         /* Uninorth AGP bus */
>>>         dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
>>> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
>>> -                                 &error_abort);
>>>         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>>>         uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
>>>         s = SYS_BUS_DEVICE(dev);
>>> @@ -317,32 +315,38 @@ static void ppc_core99_init(MachineState *machine)
>>>         sysbus_mmio_map(s, 0, 0xf0800000);
>>>         sysbus_mmio_map(s, 1, 0xf0c00000);
>>> 
>>> +        for (i = 0; i < 4; i++) {
>> 
>> Philippe had a series that replaced all these with PCI_NUM_IRQS or similar 
>> constant. Should you use that instead in new additions?
>
> I'm not sure that's directly relevant here - my understanding was that 
> PCI_NUM_IRQS represents the number of IRQs on the device, not the PCI host 
> bridge. Certainly it could be argued that there is a 1:1 correspondence in 
> this case, however I don't have any documentation to support this so it 
> doesn't feel right to make these values directly equivalent.

OK with me either way, just noted this might be something Philippe would 
suggest anyway :-)

>> This is probably not modelled faithfuly (the whole mac99 machine is a hack 
>> to get the most OSes run, not emulating an actual machine) but I wonder if 
>> it's correct to connect all these irqs for all the different busses to the 
>> same pic lines? Is that how it is on a real PowerMac3,1? (This is just a 
>> question not something that should be fixed in this patch which is a nice 
>> clean up anyway, this just made it more obvious to me what's happening so 
>> maybe we can find out a difference in emulation compared to real hardware 
>> that could be fixed in the future.)
>
> Yeah I spotted that too when I was writing the patch. In its current form it 
> keeps the existing behaviour as-is, but this is something that may be worth 
> investigating later. I'm not aware of any documentation explaining how the 
> PCI/AGP bridges are wired up on a Mac, so it might end up being a case of 
> trawling through driver source code to try and get some answers...

As usual... Maybe there's someone on the list reading this who has an 
answer and could save reading the sources. Maybe asking BenH and Alex Graf 
worth a try.

Regards,
BALATON Zoltan

Re: [PATCH v2 3/3] uninorth: use qdev gpios for PCI IRQs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 10182 bytes --]

On Tue, Oct 13, 2020 at 05:58:52PM +0100, Mark Cave-Ayland wrote:
> On 13/10/2020 14:38, BALATON Zoltan via wrote:
> 
> > On Tue, 13 Oct 2020, Mark Cave-Ayland wrote:
> > > Currently an object link property is used to pass a reference to the OpenPIC
> > > into the PCI host bridge so that pci_unin_init_irqs() can connect the PCI
> > > IRQs to the PIC itself.
> > > 
> > > This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
> > > up the PCI IRQs to the PIC in the New World machine init function.
> > > 
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > ---
> > > hw/pci-host/uninorth.c         | 45 +++++++---------------------------
> > > hw/ppc/mac_newworld.c          | 24 ++++++++++++------
> > > include/hw/pci-host/uninorth.h |  2 --
> > > 3 files changed, 25 insertions(+), 46 deletions(-)
> > > 
> > > diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
> > > index 1ed1072eeb..0c0a9ecee1 100644
> > > --- a/hw/pci-host/uninorth.c
> > > +++ b/hw/pci-host/uninorth.c
> > > @@ -32,8 +32,6 @@
> > > #include "hw/pci-host/uninorth.h"
> > > #include "trace.h"
> > > 
> > > -static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
> > > -
> > > static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
> > > {
> > >     return (irq_num + (pci_dev->devfn >> 3)) & 3;
> > > @@ -43,7 +41,7 @@ static void pci_unin_set_irq(void *opaque, int irq_num, int level)
> > > {
> > >     UNINHostState *s = opaque;
> > > 
> > > -    trace_unin_set_irq(unin_irq_line[irq_num], level);
> > > +    trace_unin_set_irq(irq_num, level);
> > >     qemu_set_irq(s->irqs[irq_num], level);
> > > }
> > > 
> > > @@ -112,15 +110,6 @@ static const MemoryRegionOps unin_data_ops = {
> > >     .endianness = DEVICE_LITTLE_ENDIAN,
> > > };
> > > 
> > > -static void pci_unin_init_irqs(UNINHostState *s)
> > > -{
> > > -    int i;
> > > -
> > > -    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
> > > -        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), unin_irq_line[i]);
> > > -    }
> > > -}
> > > -
> > > static char *pci_unin_main_ofw_unit_address(const SysBusDevice *dev)
> > > {
> > >     UNINHostState *s = UNI_NORTH_PCI_HOST_BRIDGE(dev);
> > > @@ -141,7 +130,6 @@ static void pci_unin_main_realize(DeviceState *dev, Error **errp)
> > >                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
> > > 
> > >     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-pci");
> > > -    pci_unin_init_irqs(s);
> > > 
> > >     /* DEC 21154 bridge */
> > > #if 0
> > > @@ -172,15 +160,12 @@ static void pci_unin_main_init(Object *obj)
> > >                              "unin-pci-hole", &s->pci_mmio,
> > >                              0x80000000ULL, 0x10000000ULL);
> > > 
> > > -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> > > -                             (Object **) &s->pic,
> > > -                             qdev_prop_allow_set_link_before_realize,
> > > -                             0);
> > > -
> > >     sysbus_init_mmio(sbd, &h->conf_mem);
> > >     sysbus_init_mmio(sbd, &h->data_mem);
> > >     sysbus_init_mmio(sbd, &s->pci_hole);
> > >     sysbus_init_mmio(sbd, &s->pci_io);
> > > +
> > > +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
> > > }
> > > 
> > > static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
> > > @@ -196,7 +181,6 @@ static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
> > >                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
> > > 
> > >     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "u3-agp");
> > > -    pci_unin_init_irqs(s);
> > > }
> > > 
> > > static void pci_u3_agp_init(Object *obj)
> > > @@ -220,15 +204,12 @@ static void pci_u3_agp_init(Object *obj)
> > >                              "unin-pci-hole", &s->pci_mmio,
> > >                              0x80000000ULL, 0x70000000ULL);
> > > 
> > > -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> > > -                             (Object **) &s->pic,
> > > -                             qdev_prop_allow_set_link_before_realize,
> > > -                             0);
> > > -
> > >     sysbus_init_mmio(sbd, &h->conf_mem);
> > >     sysbus_init_mmio(sbd, &h->data_mem);
> > >     sysbus_init_mmio(sbd, &s->pci_hole);
> > >     sysbus_init_mmio(sbd, &s->pci_io);
> > > +
> > > +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
> > > }
> > > 
> > > static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
> > > @@ -244,7 +225,6 @@ static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
> > >                                    PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
> > > 
> > >     pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-agp");
> > > -    pci_unin_init_irqs(s);
> > > }
> > > 
> > > static void pci_unin_agp_init(Object *obj)
> > > @@ -259,13 +239,10 @@ static void pci_unin_agp_init(Object *obj)
> > >     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
> > >                           obj, "unin-agp-conf-data", 0x1000);
> > > 
> > > -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> > > -                             (Object **) &s->pic,
> > > -                             qdev_prop_allow_set_link_before_realize,
> > > -                             0);
> > > -
> > >     sysbus_init_mmio(sbd, &h->conf_mem);
> > >     sysbus_init_mmio(sbd, &h->data_mem);
> > > +
> > > +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
> > > }
> > > 
> > > static void pci_unin_internal_realize(DeviceState *dev, Error **errp)
> > > @@ -281,7 +258,6 @@ static void
> > > pci_unin_internal_realize(DeviceState *dev, Error **errp)
> > >                                    PCI_DEVFN(14, 0), 4, TYPE_PCI_BUS);
> > > 
> > >     pci_create_simple(h->bus, PCI_DEVFN(14, 0), "uni-north-internal-pci");
> > > -    pci_unin_init_irqs(s);
> > > }
> > > 
> > > static void pci_unin_internal_init(Object *obj)
> > > @@ -296,13 +272,10 @@ static void pci_unin_internal_init(Object *obj)
> > >     memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
> > >                           obj, "unin-pci-conf-data", 0x1000);
> > > 
> > > -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> > > -                             (Object **) &s->pic,
> > > -                             qdev_prop_allow_set_link_before_realize,
> > > -                             0);
> > > -
> > >     sysbus_init_mmio(sbd, &h->conf_mem);
> > >     sysbus_init_mmio(sbd, &h->data_mem);
> > > +
> > > +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
> > > }
> > > 
> > > static void unin_main_pci_host_realize(PCIDevice *d, Error **errp)
> > > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> > > index 6f5ef2e782..7a8dc09c8d 100644
> > > --- a/hw/ppc/mac_newworld.c
> > > +++ b/hw/ppc/mac_newworld.c
> > > @@ -303,8 +303,6 @@ static void ppc_core99_init(MachineState *machine)
> > >         /* 970 gets a U3 bus */
> > >         /* Uninorth AGP bus */
> > >         dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
> > > -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> > > -                                 &error_abort);
> > >         sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > >         uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
> > >         s = SYS_BUS_DEVICE(dev);
> > > @@ -317,32 +315,38 @@ static void ppc_core99_init(MachineState *machine)
> > >         sysbus_mmio_map(s, 0, 0xf0800000);
> > >         sysbus_mmio_map(s, 1, 0xf0c00000);
> > > 
> > > +        for (i = 0; i < 4; i++) {
> > 
> > Philippe had a series that replaced all these with PCI_NUM_IRQS or
> > similar constant. Should you use that instead in new additions?
> 
> I'm not sure that's directly relevant here - my understanding was that
> PCI_NUM_IRQS represents the number of IRQs on the device, not the PCI host
> bridge. Certainly it could be argued that there is a 1:1 correspondence in
> this case, however I don't have any documentation to support this so it
> doesn't feel right to make these values directly equivalent.

I think it's the same thing.  On legacy PCI there are 4 interrupt
lines (INTA..INTD).  Each device can use any or all of those (though
they usually only use one per function in practice).  Bridges swizzle
which pins on the downstream bus are wired to which pins on the
upstream bus, but there's still just 4 of them all the way up to the
host bridge, which will then wire those 4 up to system PIC lines one
way or another.

On-board devices often have (had?) a sideband interrupt wired directly
to the PIC, rather than using one of the bus lines.

> > This is probably not modelled faithfuly (the whole mac99 machine is a
> > hack to get the most OSes run, not emulating an actual machine) but I
> > wonder if it's correct to connect all these irqs for all the different
> > busses to the same pic lines? Is that how it is on a real PowerMac3,1?
> > (This is just a question not something that should be fixed in this
> > patch which is a nice clean up anyway, this just made it more obvious to
> > me what's happening so maybe we can find out a difference in emulation
> > compared to real hardware that could be fixed in the future.)
> 
> Yeah I spotted that too when I was writing the patch. In its current form it
> keeps the existing behaviour as-is, but this is something that may be worth
> investigating later. I'm not aware of any documentation explaining how the
> PCI/AGP bridges are wired up on a Mac, so it might end up being a case of
> trawling through driver source code to try and get some answers...

It's worth checking, but having them all wired together isn't that
implausible.  In that pre-MSI era some weird wirings and heavily
shared irq lines are pretty common.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] uninorth: use qdev gpios for PCI IRQs

[Philippe Mathieu-Daudé]

On 10/13/20 1:49 PM, Mark Cave-Ayland wrote:
> Currently an object link property is used to pass a reference to the OpenPIC
> into the PCI host bridge so that pci_unin_init_irqs() can connect the PCI
> IRQs to the PIC itself.
> 
> This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
> up the PCI IRQs to the PIC in the New World machine init function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/pci-host/uninorth.c         | 45 +++++++---------------------------
>   hw/ppc/mac_newworld.c          | 24 ++++++++++++------
>   include/hw/pci-host/uninorth.h |  2 --
>   3 files changed, 25 insertions(+), 46 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 3/3] uninorth: use qdev gpios for PCI IRQs

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 10037 bytes --]

On Tue, Oct 13, 2020 at 12:49:22PM +0100, Mark Cave-Ayland wrote:
> Currently an object link property is used to pass a reference to the OpenPIC
> into the PCI host bridge so that pci_unin_init_irqs() can connect the PCI
> IRQs to the PIC itself.
> 
> This can be simplified by defining the PCI IRQs as qdev gpios and then wiring
> up the PCI IRQs to the PIC in the New World machine init function.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>

Applied to ppc-for-5.2, thanks.

> ---
>  hw/pci-host/uninorth.c         | 45 +++++++---------------------------
>  hw/ppc/mac_newworld.c          | 24 ++++++++++++------
>  include/hw/pci-host/uninorth.h |  2 --
>  3 files changed, 25 insertions(+), 46 deletions(-)
> 
> diff --git a/hw/pci-host/uninorth.c b/hw/pci-host/uninorth.c
> index 1ed1072eeb..0c0a9ecee1 100644
> --- a/hw/pci-host/uninorth.c
> +++ b/hw/pci-host/uninorth.c
> @@ -32,8 +32,6 @@
>  #include "hw/pci-host/uninorth.h"
>  #include "trace.h"
>  
> -static const int unin_irq_line[] = { 0x1b, 0x1c, 0x1d, 0x1e };
> -
>  static int pci_unin_map_irq(PCIDevice *pci_dev, int irq_num)
>  {
>      return (irq_num + (pci_dev->devfn >> 3)) & 3;
> @@ -43,7 +41,7 @@ static void pci_unin_set_irq(void *opaque, int irq_num, int level)
>  {
>      UNINHostState *s = opaque;
>  
> -    trace_unin_set_irq(unin_irq_line[irq_num], level);
> +    trace_unin_set_irq(irq_num, level);
>      qemu_set_irq(s->irqs[irq_num], level);
>  }
>  
> @@ -112,15 +110,6 @@ static const MemoryRegionOps unin_data_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> -static void pci_unin_init_irqs(UNINHostState *s)
> -{
> -    int i;
> -
> -    for (i = 0; i < ARRAY_SIZE(s->irqs); i++) {
> -        s->irqs[i] = qdev_get_gpio_in(DEVICE(s->pic), unin_irq_line[i]);
> -    }
> -}
> -
>  static char *pci_unin_main_ofw_unit_address(const SysBusDevice *dev)
>  {
>      UNINHostState *s = UNI_NORTH_PCI_HOST_BRIDGE(dev);
> @@ -141,7 +130,6 @@ static void pci_unin_main_realize(DeviceState *dev, Error **errp)
>                                     PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>  
>      pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-pci");
> -    pci_unin_init_irqs(s);
>  
>      /* DEC 21154 bridge */
>  #if 0
> @@ -172,15 +160,12 @@ static void pci_unin_main_init(Object *obj)
>                               "unin-pci-hole", &s->pci_mmio,
>                               0x80000000ULL, 0x10000000ULL);
>  
> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>      sysbus_init_mmio(sbd, &h->conf_mem);
>      sysbus_init_mmio(sbd, &h->data_mem);
>      sysbus_init_mmio(sbd, &s->pci_hole);
>      sysbus_init_mmio(sbd, &s->pci_io);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>  }
>  
>  static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
> @@ -196,7 +181,6 @@ static void pci_u3_agp_realize(DeviceState *dev, Error **errp)
>                                     PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>  
>      pci_create_simple(h->bus, PCI_DEVFN(11, 0), "u3-agp");
> -    pci_unin_init_irqs(s);
>  }
>  
>  static void pci_u3_agp_init(Object *obj)
> @@ -220,15 +204,12 @@ static void pci_u3_agp_init(Object *obj)
>                               "unin-pci-hole", &s->pci_mmio,
>                               0x80000000ULL, 0x70000000ULL);
>  
> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>      sysbus_init_mmio(sbd, &h->conf_mem);
>      sysbus_init_mmio(sbd, &h->data_mem);
>      sysbus_init_mmio(sbd, &s->pci_hole);
>      sysbus_init_mmio(sbd, &s->pci_io);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>  }
>  
>  static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
> @@ -244,7 +225,6 @@ static void pci_unin_agp_realize(DeviceState *dev, Error **errp)
>                                     PCI_DEVFN(11, 0), 4, TYPE_PCI_BUS);
>  
>      pci_create_simple(h->bus, PCI_DEVFN(11, 0), "uni-north-agp");
> -    pci_unin_init_irqs(s);
>  }
>  
>  static void pci_unin_agp_init(Object *obj)
> @@ -259,13 +239,10 @@ static void pci_unin_agp_init(Object *obj)
>      memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
>                            obj, "unin-agp-conf-data", 0x1000);
>  
> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>      sysbus_init_mmio(sbd, &h->conf_mem);
>      sysbus_init_mmio(sbd, &h->data_mem);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>  }
>  
>  static void pci_unin_internal_realize(DeviceState *dev, Error **errp)
> @@ -281,7 +258,6 @@ static void pci_unin_internal_realize(DeviceState *dev, Error **errp)
>                                     PCI_DEVFN(14, 0), 4, TYPE_PCI_BUS);
>  
>      pci_create_simple(h->bus, PCI_DEVFN(14, 0), "uni-north-internal-pci");
> -    pci_unin_init_irqs(s);
>  }
>  
>  static void pci_unin_internal_init(Object *obj)
> @@ -296,13 +272,10 @@ static void pci_unin_internal_init(Object *obj)
>      memory_region_init_io(&h->data_mem, OBJECT(h), &pci_host_data_le_ops,
>                            obj, "unin-pci-conf-data", 0x1000);
>  
> -    object_property_add_link(obj, "pic", TYPE_OPENPIC,
> -                             (Object **) &s->pic,
> -                             qdev_prop_allow_set_link_before_realize,
> -                             0);
> -
>      sysbus_init_mmio(sbd, &h->conf_mem);
>      sysbus_init_mmio(sbd, &h->data_mem);
> +
> +    qdev_init_gpio_out(DEVICE(obj), s->irqs, ARRAY_SIZE(s->irqs));
>  }
>  
>  static void unin_main_pci_host_realize(PCIDevice *d, Error **errp)
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 6f5ef2e782..7a8dc09c8d 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -303,8 +303,6 @@ static void ppc_core99_init(MachineState *machine)
>          /* 970 gets a U3 bus */
>          /* Uninorth AGP bus */
>          dev = qdev_new(TYPE_U3_AGP_HOST_BRIDGE);
> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                                 &error_abort);
>          sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>          uninorth_pci = U3_AGP_HOST_BRIDGE(dev);
>          s = SYS_BUS_DEVICE(dev);
> @@ -317,32 +315,38 @@ static void ppc_core99_init(MachineState *machine)
>          sysbus_mmio_map(s, 0, 0xf0800000);
>          sysbus_mmio_map(s, 1, 0xf0c00000);
>  
> +        for (i = 0; i < 4; i++) {
> +            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
> +        }
> +
>          machine_arch = ARCH_MAC99_U3;
>      } else {
>          /* Use values found on a real PowerMac */
>          /* Uninorth AGP bus */
>          dev = qdev_new(TYPE_UNI_NORTH_AGP_HOST_BRIDGE);
> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                                 &error_abort);
>          s = SYS_BUS_DEVICE(dev);
>          sysbus_realize_and_unref(s, &error_fatal);
>          sysbus_mmio_map(s, 0, 0xf0800000);
>          sysbus_mmio_map(s, 1, 0xf0c00000);
>  
> +        for (i = 0; i < 4; i++) {
> +            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
> +        }
> +
>          /* Uninorth internal bus */
>          dev = qdev_new(TYPE_UNI_NORTH_INTERNAL_PCI_HOST_BRIDGE);
> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                                 &error_abort);
>          s = SYS_BUS_DEVICE(dev);
>          sysbus_realize_and_unref(s, &error_fatal);
>          sysbus_mmio_map(s, 0, 0xf4800000);
>          sysbus_mmio_map(s, 1, 0xf4c00000);
>  
> +        for (i = 0; i < 4; i++) {
> +            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
> +        }
> +
>          /* Uninorth main bus */
>          dev = qdev_new(TYPE_UNI_NORTH_PCI_HOST_BRIDGE);
>          qdev_prop_set_uint32(dev, "ofw-addr", 0xf2000000);
> -        object_property_set_link(OBJECT(dev), "pic", OBJECT(pic_dev),
> -                                 &error_abort);
>          sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>          uninorth_pci = UNI_NORTH_PCI_HOST_BRIDGE(dev);
>          s = SYS_BUS_DEVICE(dev);
> @@ -355,6 +359,10 @@ static void ppc_core99_init(MachineState *machine)
>          sysbus_mmio_map(s, 0, 0xf2800000);
>          sysbus_mmio_map(s, 1, 0xf2c00000);
>  
> +        for (i = 0; i < 4; i++) {
> +            qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x1b + i));
> +        }
> +
>          machine_arch = ARCH_MAC99;
>      }
>  
> diff --git a/include/hw/pci-host/uninorth.h b/include/hw/pci-host/uninorth.h
> index a6ba5f21a8..62bd81e721 100644
> --- a/include/hw/pci-host/uninorth.h
> +++ b/include/hw/pci-host/uninorth.h
> @@ -26,7 +26,6 @@
>  #define UNINORTH_H
>  
>  #include "hw/pci/pci_host.h"
> -#include "hw/ppc/openpic.h"
>  #include "qom/object.h"
>  
>  /* UniNorth version */
> @@ -51,7 +50,6 @@ struct UNINHostState {
>      PCIHostState parent_obj;
>  
>      uint32_t ofw_addr;
> -    OpenPICState *pic;
>      qemu_irq irqs[4];
>      MemoryRegion pci_mmio;
>      MemoryRegion pci_hole;

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]