diff mbox series

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

Message ID 20201013114922.2946-3-mark.cave-ayland@ilande.co.uk
State Superseded
Headers show
Series ppc: Mac machine updates | expand

Commit Message

Mark Cave-Ayland Oct. 13, 2020, 11:49 a.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Oct. 13, 2020, 1:37 p.m. UTC | #1
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);

>
Mark Cave-Ayland Oct. 13, 2020, 4:51 p.m. UTC | #2
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.
Xingtao Yao (Fujitsu)" via Oct. 13, 2020, 5:05 p.m. UTC | #3
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
Mark Cave-Ayland Oct. 15, 2020, 7:42 p.m. UTC | #4
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.
David Gibson Oct. 16, 2020, 12:18 a.m. UTC | #5
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
Xingtao Yao (Fujitsu)" via Oct. 16, 2020, 12:26 a.m. UTC | #6
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
Howard Spoelstra Oct. 16, 2020, 6:45 a.m. UTC | #7
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
<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Oct 16, 2020 at 2:30 AM David Gibson &lt;<a href="mailto:david@gibson.dropbear.id.au">david@gibson.dropbear.id.au</a>&gt; wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Tue, Oct 13, 2020 at 12:49:21PM +0100, Mark Cave-Ayland wrote:<br>
&gt; Currently an object link property is used to pass a reference to the Heathrow<br>
&gt; PIC into the PCI host bridge so that grackle_init_irqs() can connect the PCI<br>
&gt; IRQs to the PIC itself.<br>
&gt; <br>
&gt; This can be simplified by defining the PCI IRQs as qdev gpios and then wiring<br>
&gt; up the PCI IRQs to the PIC in the Old World machine init function.<br>
&gt; <br>
&gt; Signed-off-by: Mark Cave-Ayland &lt;<a href="mailto:mark.cave-ayland@ilande.co.uk" target="_blank">mark.cave-ayland@ilande.co.uk</a>&gt;<br>
<br>
Applied to ppc-for-5.2.<br>
<br>
&gt; ---<br>
&gt;  hw/pci-host/grackle.c | 19 ++-----------------<br>
&gt;  hw/ppc/mac_oldworld.c |  7 +++++--<br>
&gt;  2 files changed, 7 insertions(+), 19 deletions(-)<br>
&gt; <br>
&gt; diff --git a/hw/pci-host/grackle.c b/hw/pci-host/grackle.c<br>
&gt; index 57c29b20af..b05facf463 100644<br>
&gt; --- a/hw/pci-host/grackle.c<br>
&gt; +++ b/hw/pci-host/grackle.c<br>
&gt; @@ -28,7 +28,6 @@<br>
&gt;  #include &quot;hw/ppc/mac.h&quot;<br>
&gt;  #include &quot;hw/qdev-properties.h&quot;<br>
&gt;  #include &quot;hw/pci/pci.h&quot;<br>
&gt; -#include &quot;hw/intc/heathrow_pic.h&quot;<br>
&gt;  #include &quot;hw/irq.h&quot;<br>
&gt;  #include &quot;qapi/error.h&quot;<br>
&gt;  #include &quot;qemu/module.h&quot;<br>
&gt; @@ -41,7 +40,6 @@ struct GrackleState {<br>
&gt;      PCIHostState parent_obj;<br>
&gt;  <br>
&gt;      uint32_t ofw_addr;<br>
&gt; -    HeathrowState *pic;<br>
&gt;      qemu_irq irqs[4];<br>
&gt;      MemoryRegion pci_mmio;<br>
&gt;      MemoryRegion pci_hole;<br>
&gt; @@ -62,15 +60,6 @@ static void pci_grackle_set_irq(void *opaque, int irq_num, int level)<br>
&gt;      qemu_set_irq(s-&gt;irqs[irq_num], level);<br>
&gt;  }<br>
&gt;  <br>
&gt; -static void grackle_init_irqs(GrackleState *s)<br>
&gt; -{<br>
&gt; -    int i;<br>
&gt; -<br>
&gt; -    for (i = 0; i &lt; ARRAY_SIZE(s-&gt;irqs); i++) {<br>
&gt; -        s-&gt;irqs[i] = qdev_get_gpio_in(DEVICE(s-&gt;pic), 0x15 + i);<br>
&gt; -    }<br>
&gt; -}<br>
&gt; -<br>
&gt;  static void grackle_realize(DeviceState *dev, Error **errp)<br>
&gt;  {<br>
&gt;      GrackleState *s = GRACKLE_PCI_HOST_BRIDGE(dev);<br>
&gt; @@ -85,7 +74,6 @@ static void grackle_realize(DeviceState *dev, Error **errp)<br>
&gt;                                       0, 4, TYPE_PCI_BUS);<br>
&gt;  <br>
&gt;      pci_create_simple(phb-&gt;bus, 0, &quot;grackle&quot;);<br>
&gt; -    grackle_init_irqs(s);<br>
&gt;  }<br>
&gt;  <br>
&gt;  static void grackle_init(Object *obj)<br>
&gt; @@ -106,15 +94,12 @@ static void grackle_init(Object *obj)<br>
&gt;      memory_region_init_io(&amp;phb-&gt;data_mem, obj, &amp;pci_host_data_le_ops,<br>
&gt;                            DEVICE(obj), &quot;pci-data-idx&quot;, 0x1000);<br>
&gt;  <br>
&gt; -    object_property_add_link(obj, &quot;pic&quot;, TYPE_HEATHROW,<br>
&gt; -                             (Object **) &amp;s-&gt;pic,<br>
&gt; -                             qdev_prop_allow_set_link_before_realize,<br>
&gt; -                             0);<br>
&gt; -<br>
&gt;      sysbus_init_mmio(sbd, &amp;phb-&gt;conf_mem);<br>
&gt;      sysbus_init_mmio(sbd, &amp;phb-&gt;data_mem);<br>
&gt;      sysbus_init_mmio(sbd, &amp;s-&gt;pci_hole);<br>
&gt;      sysbus_init_mmio(sbd, &amp;s-&gt;pci_io);<br>
&gt; +<br>
&gt; +    qdev_init_gpio_out(DEVICE(obj), s-&gt;irqs, ARRAY_SIZE(s-&gt;irqs));<br>
&gt;  }<br>
&gt;  <br>
&gt;  static void grackle_pci_realize(PCIDevice *d, Error **errp)<br>
&gt; diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c<br>
&gt; index d6a76d06dc..05e46ee6fe 100644<br>
&gt; --- a/hw/ppc/mac_oldworld.c<br>
&gt; +++ b/hw/ppc/mac_oldworld.c<br>
&gt; @@ -253,10 +253,9 @@ static void ppc_heathrow_init(MachineState *machine)<br>
&gt;      /* Grackle PCI host bridge */<br>
&gt;      dev = qdev_new(TYPE_GRACKLE_PCI_HOST_BRIDGE);<br>
&gt;      qdev_prop_set_uint32(dev, &quot;ofw-addr&quot;, 0x80000000);<br>
&gt; -    object_property_set_link(OBJECT(dev), &quot;pic&quot;, OBJECT(pic_dev),<br>
&gt; -                             &amp;error_abort);<br>
&gt;      s = SYS_BUS_DEVICE(dev);<br>
&gt;      sysbus_realize_and_unref(s, &amp;error_fatal);<br>
&gt; +<br>
&gt;      sysbus_mmio_map(s, 0, GRACKLE_BASE);<br>
&gt;      sysbus_mmio_map(s, 1, GRACKLE_BASE + 0x200000);<br>
&gt;      /* PCI hole */<br>
&gt; @@ -266,6 +265,10 @@ static void ppc_heathrow_init(MachineState *machine)<br>
&gt;      memory_region_add_subregion(get_system_memory(), 0xfe000000,<br>
&gt;                                  sysbus_mmio_get_region(s, 3));<br>
&gt;  <br>
&gt; +    for (i = 0; i &lt; 4; i++) {<br>
&gt; +        qdev_connect_gpio_out(dev, i, qdev_get_gpio_in(pic_dev, 0x15 + i));<br>
&gt; +    }<br>
&gt; +<br>
&gt;      pci_bus = PCI_HOST_BRIDGE(dev)-&gt;bus;<br>
&gt;  <br>
&gt;      pci_vga_init(pci_bus);<br>
<br><br></blockquote><div>Hi,</div><div><br></div><div>I see compilation of the current ppc-for-5.2 branch fail with:</div><div><br></div><div>../hw/pci-host/grackle.c: In function ‘grackle_realize’:<br>../hw/pci-host/grackle.c:68:11: error: ‘GrackleState’ has no member named ‘pic’<br>   68 |     if (!s-&gt;pic) {<br>      |           ^~<br>make: *** [Makefile.ninja:1741: libcommon.fa.p/hw_pci-host_grackle.c.o] Error 1</div><div><br></div><div>Best,</div><div>Howard<br></div></div></div>
Mark Cave-Ayland Oct. 16, 2020, 6:53 a.m. UTC | #8
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.
David Gibson Oct. 17, 2020, 6:20 a.m. UTC | #9
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
Mark Cave-Ayland Oct. 17, 2020, 1:03 p.m. UTC | #10
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.
diff mbox series

Patch

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);