[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Sebastian Bauer
Subject: Re: [Qemu-ppc] [PATCH] sam460ex: Fix PCI interrupt connections
Date: Mon, 30 Jul 2018 14:04:32 +0200
User-agent: Roundcube Webmail/1.3.6

Am 2018-07-30 13:06, schrieb BALATON Zoltan:
On Mon, 30 Jul 2018, Sebastian Bauer wrote:
The four interrupts of the PCI bus are connected to the same UIC pin on the real Sam460ex. Evidence for this can be found in the UBoot source for the Sam460ex in the Sam460ex.c file where PCI_INTERRUPT_LINE in written. This
change brings the connection in line with this.

This fixes the problem that can be observed when adding further PCI cards that get their interrupt rotated to other interrupts than PCI INT A. In
particular, the bug was observed and verified to be fixed (after this
change) with an additional OHCI PCI card.

Signed-off-by: Sebastian Bauer <address@hidden>
hw/ppc/sam460ex.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c
index 0999efcc1e..b2b22f280d 100644
--- a/hw/ppc/sam460ex.c
+++ b/hw/ppc/sam460ex.c
@@ -515,9 +515,9 @@ static void sam460ex_init(MachineState *machine)

    /* PCI bus */
-    /* FIXME: is this correct? */
+ /* All PCI ints are connected to the same UIC pin (cf. UBoot source) */
    dev = sysbus_create_varargs("ppc440-pcix-host", 0xc0ec00000,
- uic[1][0], uic[1][20], uic[1][21], uic[1][22], + uic[1][0], uic[1][0], uic[1][0], uic[1][0],

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

which will call for each of these

object_property_set_link(dev, uic[1][0], "some name", &errp);

I don't know QOM myself, but my research was that "some name" is different on each invocation. So you associate the same value to the same object (dev) under a property different name. What was not obvious to me was who is the owner of the value. But according to the doc QOM has the notation of strong references, but it is not created like that in qdev_connect_gpio_out_named(). So I assume that it is weak reference and the parent is not the owner.

Would this correctly add all device interrupts to the uic[1][0] irq or
will this replace it so only the last line will be connected instead
of the first after this patch? Could someone with more understanding
about QOM confirm this?

I don't know how this affects QOM, but as I see it the PCI bus has four interrupts and so you need to specify all of them (and specifying wrong ones seems to be not ideal either). The code assumes this throughout, e.g., see ppc440_pcix_initfn().

As I have written in the commit log, I tested this change. I used two cards, the (default) SATA and the OHCI controller and everything was working nicely (contrary to the previous state where only the SATA card worked because this was put into slot 1). Did you have a chance to test it yourself?

If this does not work this way would mapping all PCI interrupts to
first line in ppc440_pcix_map_irq at hw/ppc/ppc440_pcix.c:417 and
assign only that to uic[1][0] be a better fix?

I thought about this, but then it needs to passed that we are dealing with a SAM board, which seems to be more complicated than this solution. You also would need to adjust that ppc440_pcix_initfn() and I'm not sure if it is possible to register a root PCI bus with only one interrupt and how the remaining code deals with that. Overall, this solution seem to be much simpler. Of course, if it doesn't work the way I proposed it must be changed again.

But thinking more about it, what it is indeed not so clear to me is how the interrupt states are logical combined. The hardware most likely would do an "or" (but it is difficult to verify this without schematics), not sure what QEMU does. It may just forward the level change which would be not the right thing of course. I'll investigate it. The behaviour would still be better than the previous state as usually both interrupt handlers are invoked, so no interrupt will be missed.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]