On Mon, 30 Jul 2018, Sebastian Bauer wrote:
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 <m...@sebastianbauer.info>
---
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 */
ppc460ex_pcie_init(env);
- /* 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.
Yes, you are right, different name is generated for each irq line. But all
this does not seem to matter because at the end irqs are raised/lowered
directly in the irq array through functions passed when calling
pci_register_root_bus so I'm not sure what these properties are used for
if they are used at all so their value probably don't matter.
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().
I think the number of irq lines could be set, the functions have an nirq
or num_irq parameters so the 4 lines is only assumed because PCI defines
that and this is what's modelled but we could use different value here.
Sam460ex seems to connect all four PCI irq lines to a single irq but I'm
not sure what's the best way to model this (implementing 1 line in
ppc440_pcix model or trying to merge these at some higher level). Using
wrong irq lines is definitely wrong and was only left there because I had
no clue about this when I've written that so your patch is definitely
closer to what the board does.
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?
I could not test it yet, I was trying to understand the code instead to
make sure it works than just verifying it fixes one particular problem
which I believe you've done.
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.
The ppc440_pcix model is only used on the sam460ex so I think we can
change it to model that and care about other cases when/if another SoC is
added that has this part with different irqs just to keep things simple.
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.
I was trying to read hw/pci/pci.c to find out what would specifying the
same irq multiple times do but I'm still not sure I could undestand it
completely. AFAIU it will call the map_irq and set_irq functions specified
in pci_register_root_bus. When we have the same irq in multiple lines
theoretically it may happen that one slot is raising the interrupt and
another is lowering another line which would change the same line here but
I'm not sure how this is synchronised and if this could cause any problem.
Probably we can just change the map function in ppc440_pcix.c to always
return the first line then what's specified for other lines should not
matter and the above problem is avoided. We could even get rid of those
additional irqs by changing ppc440_pcix.c to only model a single line but
I'd need someone with better understanding of this to confirm that I got
this right.
David, who should we ask to get advice on this?
Regards,
BALATON Zoltan