| Message ID | 6892fc8ac57283bf7ba27fe89ea9dbdd6a37f988.1609413115.git.balaton@eik.bme.hu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Clean up sam460ex irq mapping | expand |
On Thu, 31 Dec 2020 at 11:20, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > Avoid mapping multiple interrupts to the same irq. Instead map them to > the 4 PCI interrupts and use an or-gate in the board to connect them > to the interrupt controller. This does not fix any known problem but > does not seem to cause a new problem either and may be cleaner at least. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > Tested-by: Guenter Roeck <linux@roeck-us.net> So, this patch is a behavioural change, but I think it's probably a change to the right behaviour. The difference is relatively slight, but you would see it if there are two different PCI cards and they both assert a different PCI interrupt, and then one of them lowers the interrupt before the other: device A: _____|---------|________ device B: ________|----------|____ With the old code, the signal values as seen by the UIC look like this: UIC input: _____|---------|________ (this is because when device A says "interrupt line value 0 now" we just pass "interrupt line value 0" through to the output). and with an OR gate they look like: UIC input: _____|-------------|____ (which is probably the actual behaviour -- UIC line stays signalled until all PCI interrupts are quelled). For this to matter to a guest it would have to be doing something a bit odd in its interrupt handling code, though, I suspect (like saying "stop looking for device drivers which need to service their device once the UIC interrupt is no longer asserted"). Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
On Thu, 31 Dec 2020 at 15:11, Peter Maydell <peter.maydell@linaro.org> wrote: > > On Thu, 31 Dec 2020 at 11:20, BALATON Zoltan <balaton@eik.bme.hu> wrote: > > > > Avoid mapping multiple interrupts to the same irq. Instead map them to > > the 4 PCI interrupts and use an or-gate in the board to connect them > > to the interrupt controller. This does not fix any known problem but > > does not seem to cause a new problem either and may be cleaner at least. > > > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > Tested-by: Guenter Roeck <linux@roeck-us.net> > > So, this patch is a behavioural change, but I think it's > probably a change to the right behaviour. The difference > is relatively slight, but you would see it if there are two > different PCI cards and they both assert a different PCI > interrupt, and then one of them lowers the interrupt > before the other: This turns out to be wrong -- I hadn't looked at the QEMU PCI code, but it has an internal implementation of logic that gives the same behaviour as an explicit OR gate. Basically pci_change_irq_level() tracks how many assert/deasserts of the (mapped) IRQ lines have happened, so it only calls the controller's set_irq function when the count of asserted inputs goes down to 0. So both the current code and this patch's change are functionally correct. I'm not sure which would be nominally closer to the "real hardware": the 440ex CPU/SoC datasheet lists a single PCI0INT signal, but it says it is an output, not an input, so I'm pretty sure there's something I don't understand about PCI here. (Also, unlike the 440EP it provides PCI Express as well as PCI.) thanks -- PMM
On Thu, 31 Dec 2020, Peter Maydell wrote: > On Thu, 31 Dec 2020 at 15:11, Peter Maydell <peter.maydell@linaro.org> wrote: >> On Thu, 31 Dec 2020 at 11:20, BALATON Zoltan <balaton@eik.bme.hu> wrote: >>> >>> Avoid mapping multiple interrupts to the same irq. Instead map them to >>> the 4 PCI interrupts and use an or-gate in the board to connect them >>> to the interrupt controller. This does not fix any known problem but >>> does not seem to cause a new problem either and may be cleaner at least. >>> >>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>> Tested-by: Guenter Roeck <linux@roeck-us.net> >> >> So, this patch is a behavioural change, but I think it's >> probably a change to the right behaviour. The difference >> is relatively slight, but you would see it if there are two >> different PCI cards and they both assert a different PCI >> interrupt, and then one of them lowers the interrupt >> before the other: > > This turns out to be wrong -- I hadn't looked at the QEMU PCI > code, but it has an internal implementation of logic that > gives the same behaviour as an explicit OR gate. Basically > pci_change_irq_level() tracks how many assert/deasserts of > the (mapped) IRQ lines have happened, so it only calls the > controller's set_irq function when the count of asserted > inputs goes down to 0. So both the current code and this > patch's change are functionally correct. I've remembered we had this discussion before and arrived to the same conclusion that current code was equivalently working but could not recall the reason. > I'm not sure which would be nominally closer to the "real hardware": > the 440ex CPU/SoC datasheet lists a single PCI0INT signal, but > it says it is an output, not an input, so I'm pretty sure there's > something I don't understand about PCI here. (Also, unlike the > 440EP it provides PCI Express as well as PCI.) The SoC is called 460EX (despite having a PPC 440 core not 460 one) but I think you've looked at the right data sheet and it's just a typo. I also don't know how the board is wired so I think in this case I prefer dropping this patch and keeping the current code just for simplicity but to avoid going through this again maybe we should add a comment saying why it's working. Can you please suggest a test for such comment pointing to the relevant part of pci_change_irq_level() you refer to above? I don't think I understand it enough to document it. Thank you, BALATON Zoltan
On Thu, 31 Dec 2020, BALATON Zoltan via wrote: > On Thu, 31 Dec 2020, Peter Maydell wrote: >> On Thu, 31 Dec 2020 at 15:11, Peter Maydell <peter.maydell@linaro.org> >> wrote: >>> On Thu, 31 Dec 2020 at 11:20, BALATON Zoltan <balaton@eik.bme.hu> wrote: >>>> >>>> Avoid mapping multiple interrupts to the same irq. Instead map them to >>>> the 4 PCI interrupts and use an or-gate in the board to connect them >>>> to the interrupt controller. This does not fix any known problem but >>>> does not seem to cause a new problem either and may be cleaner at least. >>>> >>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >>>> Tested-by: Guenter Roeck <linux@roeck-us.net> >>> >>> So, this patch is a behavioural change, but I think it's >>> probably a change to the right behaviour. The difference >>> is relatively slight, but you would see it if there are two >>> different PCI cards and they both assert a different PCI >>> interrupt, and then one of them lowers the interrupt >>> before the other: >> >> This turns out to be wrong -- I hadn't looked at the QEMU PCI >> code, but it has an internal implementation of logic that >> gives the same behaviour as an explicit OR gate. Basically >> pci_change_irq_level() tracks how many assert/deasserts of >> the (mapped) IRQ lines have happened, so it only calls the >> controller's set_irq function when the count of asserted >> inputs goes down to 0. So both the current code and this >> patch's change are functionally correct. > > I've remembered we had this discussion before and arrived to the same > conclusion that current code was equivalently working but could not recall > the reason. > >> I'm not sure which would be nominally closer to the "real hardware": >> the 440ex CPU/SoC datasheet lists a single PCI0INT signal, but >> it says it is an output, not an input, so I'm pretty sure there's >> something I don't understand about PCI here. (Also, unlike the >> 440EP it provides PCI Express as well as PCI.) > > The SoC is called 460EX (despite having a PPC 440 core not 460 one) but I > think you've looked at the right data sheet and it's just a typo. I also > don't know how the board is wired so I think in this case I prefer dropping > this patch and keeping the current code just for simplicity but to avoid > going through this again maybe we should add a comment saying why it's > working. Can you please suggest a test for such comment pointing to the I mean "text" not "test" above. > relevant part of pci_change_irq_level() you refer to above? I don't think I > understand it enough to document it. > > Thank you, > BALATON Zoltan > >
On Thu, 31 Dec 2020 at 20:55, BALATON Zoltan <balaton@eik.bme.hu> wrote: > The SoC is called 460EX (despite having a PPC 440 core not 460 one) but I > think you've looked at the right data sheet and it's just a typo. I also > don't know how the board is wired so I think in this case I prefer > dropping this patch and keeping the current code just for simplicity but > to avoid going through this again maybe we should add a comment saying why > it's working. Can you please suggest a text for such comment pointing to > the relevant part of pci_change_irq_level() you refer to above? I don't > think I understand it enough to document it. How about: /* * All four IRQ[ABCD] pins from all slots are tied to a single board * IRQ, so our mapping function here maps everything to IRQ 0. * The code in pci_change_irq_level() tracks the number of times * the mapped IRQ is asserted and deasserted, so if multiple devices * assert an IRQ at the same time the behaviour is correct. */ ? thanks -- PMM
On Fri, 1 Jan 2021, Peter Maydell wrote: > On Thu, 31 Dec 2020 at 20:55, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> The SoC is called 460EX (despite having a PPC 440 core not 460 one) but I >> think you've looked at the right data sheet and it's just a typo. I also >> don't know how the board is wired so I think in this case I prefer >> dropping this patch and keeping the current code just for simplicity but >> to avoid going through this again maybe we should add a comment saying why >> it's working. Can you please suggest a text for such comment pointing to >> the relevant part of pci_change_irq_level() you refer to above? I don't >> think I understand it enough to document it. > > How about: > > /* > * All four IRQ[ABCD] pins from all slots are tied to a single board > * IRQ, so our mapping function here maps everything to IRQ 0. > * The code in pci_change_irq_level() tracks the number of times > * the mapped IRQ is asserted and deasserted, so if multiple devices > * assert an IRQ at the same time the behaviour is correct. > */ Very good, thank you. Leaving it as it is now also avoids needing to rebase your UIC series so that should still apply. I've sent a series with the above comment now, please add your Suggested-by, Reviewed-by as you see fit. Regards, BALATON Zoltan
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig index f1e1be208e..ebb70803c4 100644 --- a/hw/ppc/Kconfig +++ b/hw/ppc/Kconfig @@ -59,6 +59,7 @@ config SAM460EX select PFLASH_CFI01 select IDE_SII3112 select M41T80 + select OR_IRQ select PPC440 select SM501 select SMBUS_EEPROM diff --git a/hw/ppc/ppc440_pcix.c b/hw/ppc/ppc440_pcix.c index ee952314c8..504decbbc2 100644 --- a/hw/ppc/ppc440_pcix.c +++ b/hw/ppc/ppc440_pcix.c @@ -57,8 +57,8 @@ struct PPC440PCIXState { PCIDevice *dev; struct PLBOutMap pom[PPC440_PCIX_NR_POMS]; struct PLBInMap pim[PPC440_PCIX_NR_PIMS]; + qemu_irq irq[PCI_NUM_PINS]; uint32_t sts; - qemu_irq irq; AddressSpace bm_as; MemoryRegion bm; @@ -415,24 +415,20 @@ static void ppc440_pcix_reset(DeviceState *dev) s->sts = 0; } -/* All pins from each slot are tied to a single board IRQ. - * This may need further refactoring for other boards. */ static int ppc440_pcix_map_irq(PCIDevice *pci_dev, int irq_num) { - trace_ppc440_pcix_map_irq(pci_dev->devfn, irq_num, 0); - return 0; + int n = (irq_num + PCI_SLOT(pci_dev->devfn)) % PCI_NUM_PINS; + + trace_ppc440_pcix_map_irq(pci_dev->devfn, irq_num, n); + return n; } static void ppc440_pcix_set_irq(void *opaque, int irq_num, int level) { - qemu_irq *pci_irq = opaque; + qemu_irq *pci_irqs = opaque; trace_ppc440_pcix_set_irq(irq_num); - if (irq_num < 0) { - error_report("%s: PCI irq %d", __func__, irq_num); - return; - } - qemu_set_irq(*pci_irq, level); + qemu_set_irq(pci_irqs[irq_num], level); } static AddressSpace *ppc440_pcix_set_iommu(PCIBus *b, void *opaque, int devfn) @@ -472,15 +468,19 @@ static void ppc440_pcix_realize(DeviceState *dev, Error **errp) SysBusDevice *sbd = SYS_BUS_DEVICE(dev); PPC440PCIXState *s; PCIHostState *h; + int i; h = PCI_HOST_BRIDGE(dev); s = PPC440_PCIX_HOST_BRIDGE(dev); - sysbus_init_irq(sbd, &s->irq); + for (i = 0; i < ARRAY_SIZE(s->irq); i++) { + sysbus_init_irq(sbd, &s->irq[i]); + } memory_region_init(&s->busmem, OBJECT(dev), "pci bus memory", UINT64_MAX); h->bus = pci_register_root_bus(dev, NULL, ppc440_pcix_set_irq, - ppc440_pcix_map_irq, &s->irq, &s->busmem, - get_system_io(), PCI_DEVFN(0, 0), 1, TYPE_PCI_BUS); + ppc440_pcix_map_irq, s->irq, &s->busmem, + get_system_io(), PCI_DEVFN(0, 0), ARRAY_SIZE(s->irq), + TYPE_PCI_BUS); s->dev = pci_create_simple(h->bus, PCI_DEVFN(0, 0), "ppc4xx-host-bridge"); diff --git a/hw/ppc/sam460ex.c b/hw/ppc/sam460ex.c index 14e6583eb0..59b19fbca1 100644 --- a/hw/ppc/sam460ex.c +++ b/hw/ppc/sam460ex.c @@ -33,6 +33,7 @@ #include "sysemu/qtest.h" #include "sysemu/reset.h" #include "hw/sysbus.h" +#include "hw/or-irq.h" #include "hw/char/serial.h" #include "hw/i2c/ppc4xx_i2c.h" #include "hw/i2c/smbus_eeprom.h" @@ -292,7 +293,7 @@ static void sam460ex_init(MachineState *machine) SysBusDevice *sbdev; struct boot_info *boot_info; uint8_t *spd_data; - int success; + int i, success; cpu = POWERPC_CPU(cpu_create(machine->cpu_type)); env = &cpu->env; @@ -382,13 +383,22 @@ static void sam460ex_init(MachineState *machine) /* PCI bus */ ppc460ex_pcie_init(env); - /* All PCI irqs are connected to the same UIC pin (cf. UBoot source) */ - dev = sysbus_create_simple("ppc440-pcix-host", 0xc0ec00000, uic[1][0]); + dev = sysbus_create_simple("ppc440-pcix-host", 0xc0ec00000, NULL); pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0"); if (!pci_bus) { error_report("couldn't create PCI controller!"); exit(1); } + /* All PCI irqs are connected to the same UIC pin (cf. UBoot source) */ + sbdev = SYS_BUS_DEVICE(dev); + dev = qdev_new(TYPE_OR_IRQ); + object_property_set_int(OBJECT(dev), "num-lines", PCI_NUM_PINS, + &error_fatal); + qdev_realize_and_unref(dev, NULL, &error_fatal); + for (i = 0; i < PCI_NUM_PINS; i++) { + sysbus_connect_irq(sbdev, i, qdev_get_gpio_in(dev, i)); + } + qdev_connect_gpio_out(dev, 0, uic[1][0]); memory_region_init_alias(isa, NULL, "isa_mmio", get_system_io(), 0, 0x10000); memory_region_add_subregion(get_system_memory(), 0xc08000000, isa);