* [PATCH v2 0/3] Clean up sam460ex irq mapping @ 2020-12-31 11:11 BALATON Zoltan via 2020-12-25 20:12 ` [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig BALATON Zoltan via ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: BALATON Zoltan via @ 2020-12-31 11:11 UTC (permalink / raw) To: qemu-devel, qemu-ppc Cc: Peter Maydell, Mark Cave-Ayland, Guenter Roeck, David Gibson v2 of https://patchew.org/QEMU/cover.1608937677.git.balaton@eik.bme.hu/ with added Tested-by from Guenter and revised patch 2 as suggested by Philippe. Regards, BALATON Zoltan BALATON Zoltan (3): ppc4xx: Move common dependency on serial to common option sam460ex: Remove FDT_PPC dependency from KConfig sam460ex: Clean up irq mapping hw/ppc/Kconfig | 7 ++----- hw/ppc/ppc440_pcix.c | 28 ++++++++++++++-------------- hw/ppc/sam460ex.c | 16 +++++++++++++--- 3 files changed, 29 insertions(+), 22 deletions(-) -- 2.21.3 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig 2020-12-31 11:11 [PATCH v2 0/3] Clean up sam460ex irq mapping BALATON Zoltan via @ 2020-12-25 20:12 ` BALATON Zoltan via 2020-12-31 16:08 ` Philippe Mathieu-Daudé 2021-01-04 1:51 ` BALATON Zoltan via 2020-12-31 11:11 ` [PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option BALATON Zoltan via 2020-12-31 11:11 ` [PATCH v2 3/3] sam460ex: Clean up irq mapping BALATON Zoltan via 2 siblings, 2 replies; 16+ messages in thread From: BALATON Zoltan via @ 2020-12-25 20:12 UTC (permalink / raw) To: qemu-devel, qemu-ppc Cc: Peter Maydell, Mark Cave-Ayland, Guenter Roeck, David Gibson Dependency on FDT_PPC was added in commit b0048f76095 ("hw/ppc/Kconfig: Only select FDT helper for machines using it") but it does not seem to be really necessary so remove it again. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- v2: Do not remove PPC405, reworded commit message hw/ppc/Kconfig | 1 - 1 file changed, 1 deletion(-) diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig index 8548f42b0d..f1e1be208e 100644 --- a/hw/ppc/Kconfig +++ b/hw/ppc/Kconfig @@ -64,7 +64,6 @@ config SAM460EX select SMBUS_EEPROM select USB_EHCI_SYSBUS select USB_OHCI - select FDT_PPC config PREP bool -- 2.21.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig 2020-12-25 20:12 ` [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig BALATON Zoltan via @ 2020-12-31 16:08 ` Philippe Mathieu-Daudé 2021-01-04 1:51 ` BALATON Zoltan via 1 sibling, 0 replies; 16+ messages in thread From: Philippe Mathieu-Daudé @ 2020-12-31 16:08 UTC (permalink / raw) To: BALATON Zoltan, qemu-devel, qemu-ppc Cc: Peter Maydell, Mark Cave-Ayland, Guenter Roeck, David Gibson On 12/31/20 12:11 PM, BALATON Zoltan via wrote: > Dependency on FDT_PPC was added in commit b0048f76095 > ("hw/ppc/Kconfig: Only select FDT helper for machines using it") but > it does not seem to be really necessary so remove it again. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > v2: Do not remove PPC405, reworded commit message > > hw/ppc/Kconfig | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig > index 8548f42b0d..f1e1be208e 100644 > --- a/hw/ppc/Kconfig > +++ b/hw/ppc/Kconfig > @@ -64,7 +64,6 @@ config SAM460EX > select SMBUS_EEPROM > select USB_EHCI_SYSBUS > select USB_OHCI > - select FDT_PPC > > config PREP > bool > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig 2020-12-25 20:12 ` [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig BALATON Zoltan via 2020-12-31 16:08 ` Philippe Mathieu-Daudé @ 2021-01-04 1:51 ` BALATON Zoltan via 2021-01-04 11:48 ` Peter Maydell 2021-01-05 12:50 ` Paolo Bonzini 1 sibling, 2 replies; 16+ messages in thread From: BALATON Zoltan via @ 2021-01-04 1:51 UTC (permalink / raw) To: qemu-devel, qemu-ppc; +Cc: Peter Maydell, Paolo Bonzini, David Gibson On Thu, 31 Dec 2020, BALATON Zoltan via wrote: > Dependency on FDT_PPC was added in commit b0048f76095 > ("hw/ppc/Kconfig: Only select FDT helper for machines using it") but > it does not seem to be really necessary so remove it again. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > --- > v2: Do not remove PPC405, reworded commit message > > hw/ppc/Kconfig | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig > index 8548f42b0d..f1e1be208e 100644 > --- a/hw/ppc/Kconfig > +++ b/hw/ppc/Kconfig > @@ -64,7 +64,6 @@ config SAM460EX > select SMBUS_EEPROM > select USB_EHCI_SYSBUS > select USB_OHCI > - select FDT_PPC > > config PREP > bool Something is still not right with this, I've noticed that a few other boards also have this option selected but they don't need fdt.o that's gated by this option in meson.build. That fdt.o is only needed by PSERIES and POWERNV but removing FDT_PPC from other boards I get: ../hw/ppc/sam460ex.c:43:10: fatal error: libfdt.h: No such file or directory #include <libfdt.h> so apparently this switch also pulls in the necessary CPPFLAGS or libfdt dependency. Is there a separate switch for that or we can only get it with fdt.o. Not a big deal just not trivial why we need an option that at first sight select a source file which we don't need. I think I'll drop this patch for now when resending the series. Regards, BALATON Zoltan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig 2021-01-04 1:51 ` BALATON Zoltan via @ 2021-01-04 11:48 ` Peter Maydell 2021-01-05 12:50 ` Paolo Bonzini 1 sibling, 0 replies; 16+ messages in thread From: Peter Maydell @ 2021-01-04 11:48 UTC (permalink / raw) To: BALATON Zoltan; +Cc: Paolo Bonzini, qemu-ppc, QEMU Developers, David Gibson On Mon, 4 Jan 2021 at 01:51, BALATON Zoltan <balaton@eik.bme.hu> wrote: > Something is still not right with this, I've noticed that a few other > boards also have this option selected but they don't need fdt.o that's > gated by this option in meson.build. That fdt.o is only needed by PSERIES > and POWERNV but removing FDT_PPC from other boards I get: > > ../hw/ppc/sam460ex.c:43:10: fatal error: libfdt.h: No such file or directory > #include <libfdt.h> > > so apparently this switch also pulls in the necessary CPPFLAGS or libfdt > dependency. Is there a separate switch for that or we can only get it with > fdt.o. Not a big deal just not trivial why we need an option that at first > sight select a source file which we don't need. I think I'll drop this > patch for now when resending the series. This happens because hw/ppc/meson.build does: ppc_ss.add(when: 'CONFIG_FDT_PPC', if_true: [files( 'fdt.c', ), fdt]) ie if CONFIG_FDT_PPC is set then both * add fdt.c to the build * add the 'fdt' dependency (which brings in the CFLAGS and LDFLAGS necessary for libfdt). So yes, at the moment for PPC there are only two options: * board doesn't use FDT at all * board uses FDT and gets hw/ppc/fdt.c linked in even if it doesn't use the functions there Since fdt.c currently has just one not very large function (which is only even present if TARGET_PPC64) this doesn't seem like a big deal, but in theory the "need libfdt" and "need fdt.c" parts could be decoupled. thanks -- PMM ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig 2021-01-04 1:51 ` BALATON Zoltan via 2021-01-04 11:48 ` Peter Maydell @ 2021-01-05 12:50 ` Paolo Bonzini 1 sibling, 0 replies; 16+ messages in thread From: Paolo Bonzini @ 2021-01-05 12:50 UTC (permalink / raw) To: BALATON Zoltan, qemu-devel, qemu-ppc; +Cc: Peter Maydell, David Gibson On 04/01/21 02:51, BALATON Zoltan wrote: > On Thu, 31 Dec 2020, BALATON Zoltan via wrote: >> Dependency on FDT_PPC was added in commit b0048f76095 >> ("hw/ppc/Kconfig: Only select FDT helper for machines using it") but >> it does not seem to be really necessary so remove it again. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> >> --- >> v2: Do not remove PPC405, reworded commit message >> >> hw/ppc/Kconfig | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig >> index 8548f42b0d..f1e1be208e 100644 >> --- a/hw/ppc/Kconfig >> +++ b/hw/ppc/Kconfig >> @@ -64,7 +64,6 @@ config SAM460EX >> select SMBUS_EEPROM >> select USB_EHCI_SYSBUS >> select USB_OHCI >> - select FDT_PPC >> >> config PREP >> bool > > Something is still not right with this, I've noticed that a few other > boards also have this option selected but they don't need fdt.o that's > gated by this option in meson.build. That fdt.o is only needed by > PSERIES and POWERNV but removing FDT_PPC from other boards I get: > > ../hw/ppc/sam460ex.c:43:10: fatal error: libfdt.h: No such file or > directory > #include <libfdt.h> > > so apparently this switch also pulls in the necessary CPPFLAGS or libfdt > dependency. Is there a separate switch for that or we can only get it > with fdt.o. Not a big deal just not trivial why we need an option that > at first sight select a source file which we don't need. I think I'll > drop this patch for now when resending the series. You can always do ppc_ss.add(when: 'CONFIG_SAM460EX', if_true: [files('sam460ex.c'), fdt]) if you want to get rid of the flag. Paolo ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option 2020-12-31 11:11 [PATCH v2 0/3] Clean up sam460ex irq mapping BALATON Zoltan via 2020-12-25 20:12 ` [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig BALATON Zoltan via @ 2020-12-31 11:11 ` BALATON Zoltan via 2021-01-04 0:31 ` David Gibson 2020-12-31 11:11 ` [PATCH v2 3/3] sam460ex: Clean up irq mapping BALATON Zoltan via 2 siblings, 1 reply; 16+ messages in thread From: BALATON Zoltan via @ 2020-12-31 11:11 UTC (permalink / raw) To: qemu-devel, qemu-ppc Cc: Peter Maydell, Mark Cave-Ayland, Guenter Roeck, David Gibson All machines that select SERIAL also select PPC4XX so we can just add this common dependency there once. Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> --- hw/ppc/Kconfig | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig index dd86e664d2..8548f42b0d 100644 --- a/hw/ppc/Kconfig +++ b/hw/ppc/Kconfig @@ -37,7 +37,6 @@ config PPC405 select M48T59 select PFLASH_CFI02 select PPC4XX - select SERIAL config PPC440 bool @@ -46,13 +45,13 @@ config PPC440 imply E1000_PCI select PCI_EXPRESS select PPC4XX - select SERIAL select FDT_PPC config PPC4XX bool select BITBANG_I2C select PCI + select SERIAL config SAM460EX bool @@ -61,7 +60,6 @@ config SAM460EX select IDE_SII3112 select M41T80 select PPC440 - select SERIAL select SM501 select SMBUS_EEPROM select USB_EHCI_SYSBUS @@ -123,7 +121,6 @@ config VIRTEX bool select PPC4XX select PFLASH_CFI01 - select SERIAL select XILINX select XILINX_ETHLITE select FDT_PPC -- 2.21.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option 2020-12-31 11:11 ` [PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option BALATON Zoltan via @ 2021-01-04 0:31 ` David Gibson 2021-01-04 1:25 ` BALATON Zoltan via 0 siblings, 1 reply; 16+ messages in thread From: David Gibson @ 2021-01-04 0:31 UTC (permalink / raw) To: BALATON Zoltan; +Cc: Peter Maydell, qemu-ppc, qemu-devel, Guenter Roeck [-- Attachment #1: Type: text/plain, Size: 1558 bytes --] On Thu, Dec 31, 2020 at 12:11:55PM +0100, BALATON Zoltan via wrote: > All machines that select SERIAL also select PPC4XX so we can just add > this common dependency there once. > > Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> Applied to ppc-for-6.0. > --- > hw/ppc/Kconfig | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig > index dd86e664d2..8548f42b0d 100644 > --- a/hw/ppc/Kconfig > +++ b/hw/ppc/Kconfig > @@ -37,7 +37,6 @@ config PPC405 > select M48T59 > select PFLASH_CFI02 > select PPC4XX > - select SERIAL > > config PPC440 > bool > @@ -46,13 +45,13 @@ config PPC440 > imply E1000_PCI > select PCI_EXPRESS > select PPC4XX > - select SERIAL > select FDT_PPC > > config PPC4XX > bool > select BITBANG_I2C > select PCI > + select SERIAL > > config SAM460EX > bool > @@ -61,7 +60,6 @@ config SAM460EX > select IDE_SII3112 > select M41T80 > select PPC440 > - select SERIAL > select SM501 > select SMBUS_EEPROM > select USB_EHCI_SYSBUS > @@ -123,7 +121,6 @@ config VIRTEX > bool > select PPC4XX > select PFLASH_CFI01 > - select SERIAL > select XILINX > select XILINX_ETHLITE > select FDT_PPC -- 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 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option 2021-01-04 0:31 ` David Gibson @ 2021-01-04 1:25 ` BALATON Zoltan via 0 siblings, 0 replies; 16+ messages in thread From: BALATON Zoltan via @ 2021-01-04 1:25 UTC (permalink / raw) To: David Gibson; +Cc: Peter Maydell, qemu-ppc, qemu-devel, Guenter Roeck On Mon, 4 Jan 2021, David Gibson wrote: > On Thu, Dec 31, 2020 at 12:11:55PM +0100, BALATON Zoltan via wrote: >> All machines that select SERIAL also select PPC4XX so we can just add >> this common dependency there once. >> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu> > > Applied to ppc-for-6.0. Not yet please, I'll send another version with changes suggested by Thomas. and also noticed another patch in this series needs update so I'll wait until tomorrow to see if anybody else has anything to say then will send and updated series. Regards, BALATON Zoltan. >> --- >> hw/ppc/Kconfig | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig >> index dd86e664d2..8548f42b0d 100644 >> --- a/hw/ppc/Kconfig >> +++ b/hw/ppc/Kconfig >> @@ -37,7 +37,6 @@ config PPC405 >> select M48T59 >> select PFLASH_CFI02 >> select PPC4XX >> - select SERIAL >> >> config PPC440 >> bool >> @@ -46,13 +45,13 @@ config PPC440 >> imply E1000_PCI >> select PCI_EXPRESS >> select PPC4XX >> - select SERIAL >> select FDT_PPC >> >> config PPC4XX >> bool >> select BITBANG_I2C >> select PCI >> + select SERIAL >> >> config SAM460EX >> bool >> @@ -61,7 +60,6 @@ config SAM460EX >> select IDE_SII3112 >> select M41T80 >> select PPC440 >> - select SERIAL >> select SM501 >> select SMBUS_EEPROM >> select USB_EHCI_SYSBUS >> @@ -123,7 +121,6 @@ config VIRTEX >> bool >> select PPC4XX >> select PFLASH_CFI01 >> - select SERIAL >> select XILINX >> select XILINX_ETHLITE >> select FDT_PPC > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 3/3] sam460ex: Clean up irq mapping 2020-12-31 11:11 [PATCH v2 0/3] Clean up sam460ex irq mapping BALATON Zoltan via 2020-12-25 20:12 ` [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig BALATON Zoltan via 2020-12-31 11:11 ` [PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option BALATON Zoltan via @ 2020-12-31 11:11 ` BALATON Zoltan via 2020-12-31 15:11 ` Peter Maydell 2 siblings, 1 reply; 16+ messages in thread From: BALATON Zoltan via @ 2020-12-31 11:11 UTC (permalink / raw) To: qemu-devel, qemu-ppc Cc: Peter Maydell, Mark Cave-Ayland, Guenter Roeck, David Gibson 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> --- hw/ppc/Kconfig | 1 + hw/ppc/ppc440_pcix.c | 28 ++++++++++++++-------------- hw/ppc/sam460ex.c | 16 +++++++++++++--- 3 files changed, 28 insertions(+), 17 deletions(-) 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); -- 2.21.3 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping 2020-12-31 11:11 ` [PATCH v2 3/3] sam460ex: Clean up irq mapping BALATON Zoltan via @ 2020-12-31 15:11 ` Peter Maydell 2020-12-31 15:51 ` Peter Maydell 0 siblings, 1 reply; 16+ messages in thread From: Peter Maydell @ 2020-12-31 15:11 UTC (permalink / raw) To: BALATON Zoltan Cc: Mark Cave-Ayland, qemu-ppc, QEMU Developers, Guenter Roeck, David Gibson 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping 2020-12-31 15:11 ` Peter Maydell @ 2020-12-31 15:51 ` Peter Maydell 2020-12-31 20:54 ` BALATON Zoltan via 0 siblings, 1 reply; 16+ messages in thread From: Peter Maydell @ 2020-12-31 15:51 UTC (permalink / raw) To: BALATON Zoltan Cc: Mark Cave-Ayland, qemu-ppc, QEMU Developers, Guenter Roeck, David Gibson 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping 2020-12-31 15:51 ` Peter Maydell @ 2020-12-31 20:54 ` BALATON Zoltan via 2020-12-31 20:58 ` BALATON Zoltan via 2021-01-01 12:03 ` Peter Maydell 0 siblings, 2 replies; 16+ messages in thread From: BALATON Zoltan via @ 2020-12-31 20:54 UTC (permalink / raw) To: Peter Maydell Cc: Mark Cave-Ayland, qemu-ppc, QEMU Developers, Guenter Roeck, David Gibson 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping 2020-12-31 20:54 ` BALATON Zoltan via @ 2020-12-31 20:58 ` BALATON Zoltan via 2021-01-01 12:03 ` Peter Maydell 1 sibling, 0 replies; 16+ messages in thread From: BALATON Zoltan via @ 2020-12-31 20:58 UTC (permalink / raw) To: Peter Maydell; +Cc: qemu-ppc, QEMU Developers, Guenter Roeck, David Gibson 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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping 2020-12-31 20:54 ` BALATON Zoltan via 2020-12-31 20:58 ` BALATON Zoltan via @ 2021-01-01 12:03 ` Peter Maydell 2021-01-03 1:46 ` BALATON Zoltan via 1 sibling, 1 reply; 16+ messages in thread From: Peter Maydell @ 2021-01-01 12:03 UTC (permalink / raw) To: BALATON Zoltan Cc: Mark Cave-Ayland, qemu-ppc, QEMU Developers, Guenter Roeck, David Gibson 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping 2021-01-01 12:03 ` Peter Maydell @ 2021-01-03 1:46 ` BALATON Zoltan via 0 siblings, 0 replies; 16+ messages in thread From: BALATON Zoltan via @ 2021-01-03 1:46 UTC (permalink / raw) To: Peter Maydell Cc: Mark Cave-Ayland, qemu-ppc, QEMU Developers, Guenter Roeck, David Gibson 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-01-05 12:52 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-31 11:11 [PATCH v2 0/3] Clean up sam460ex irq mapping BALATON Zoltan via 2020-12-25 20:12 ` [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig BALATON Zoltan via 2020-12-31 16:08 ` Philippe Mathieu-Daudé 2021-01-04 1:51 ` BALATON Zoltan via 2021-01-04 11:48 ` Peter Maydell 2021-01-05 12:50 ` Paolo Bonzini 2020-12-31 11:11 ` [PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option BALATON Zoltan via 2021-01-04 0:31 ` David Gibson 2021-01-04 1:25 ` BALATON Zoltan via 2020-12-31 11:11 ` [PATCH v2 3/3] sam460ex: Clean up irq mapping BALATON Zoltan via 2020-12-31 15:11 ` Peter Maydell 2020-12-31 15:51 ` Peter Maydell 2020-12-31 20:54 ` BALATON Zoltan via 2020-12-31 20:58 ` BALATON Zoltan via 2021-01-01 12:03 ` Peter Maydell 2021-01-03 1:46 ` BALATON Zoltan via
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.