Re: [Qemu-devel] [PATCH] sam460ex: Fix PCI interrupt connections

[Peter Maydell]

On 31 July 2018 at 00:00, BALATON Zoltan <address@hidden> wrote:
> On Mon, 30 Jul 2018, Peter Maydell wrote:
>>
>> On 30 July 2018 at 12:06, BALATON Zoltan <address@hidden> wrote:
>>> I don't understand QOM. Does this really work? It will ultimately do
>>>
>>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 0, uic[1][0]);
>>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 1, uic[1][0]);
>>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 2, uic[1][0]);
>>> qdev_connect_gpio_out_named(dev, SYSBUS_DEVICE_GPIO_IRQ, 3, uic[1][0]);
>>
>>
>> You are correct; this will not do the intended thing. If you
>> want to wire up multiple outputs which are logically ORed
>> together into a single input, you need to instantiate an
>> OR gate for that (we have the TYPE_OR_IRQ for this).
>
>
> Thanks for confirming it. However after reading hw/pci/pci.c I still don't
> see where these properties are used at all. It looks like it will just call
> the map_irq and set_irq functions specified in pci_register_root_bus passing
> them the irq array given in opaque and these will change irq lines directly.

When ppc4xx_pci_set_irq() calls qemu_set_irq() it passes it a
qemu_irq which is one of the s->irq[] which was initialized
via sysbus_init_irq(). The qdev_connect_gpio_out_named() calls
above are what connect those to the interrupt controller at the
other end of the line. Without them qemu_set_irq() does nothing
(because an initialized-but-never-connected qemu_irq is actually
a NULL pointer, and qemu_set_irq() exits doing nothing for NULL).

On the output end of a device, a qemu_irq connection is a QOM property
which is a link (pointer) property (a qemu_irq is a pointer to an
opaque struct). On the input end of a device, initializing an input
GPIO creates those opaque structs and sets the callback function
which will be invoked for a level change. The various functions for
wiring up gpio connections get a pointer to the input device's struct
and pass it to the output device by setting its QOM property.

> So I think we could just change the map function to map everything to the
> first line and then the other lines don't matter at all. We could even get
> rid of them and change ppc440_pcix to have a single line as used in the
> sam460ex (which is the only board using this model currently so we can add
> more complexity when/if anything else needs it to be different). Is there
> anything why this simple solution would not work that justifies adding more
> complexity now?

It would work. But creating an OR gate is half a dozen lines or so of
code, so it's not much more work than changing the PCI controller.
So we should prefer whichever is closest to what the real hardware
does, assuming we can determine that.

thanks
-- PMM