[PATCH v2 0/3] Clean up sam460ex irq mapping

[PATCH v2 0/3] Clean up sam460ex irq mapping

[BALATON Zoltan via]

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

[PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig

[BALATON Zoltan via]

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

Re: [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig

[Philippe Mathieu-Daudé]

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>

Re: [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig

[BALATON Zoltan via]

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

Re: [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig

[Peter Maydell]

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

Re: [PATCH v2 2/3] sam460ex: Remove FDT_PPC dependency from KConfig

[Paolo Bonzini]

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

[PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option

[BALATON Zoltan via]

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

Re: [PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option

[David Gibson]

[-- 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 --]

Re: [PATCH v2 1/3] ppc4xx: Move common dependency on serial to common option

[BALATON Zoltan via]

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
>
>

[PATCH v2 3/3] sam460ex: Clean up irq mapping

[BALATON Zoltan via]

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

Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping

[Peter Maydell]

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

Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping

[Peter Maydell]

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

Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping

[BALATON Zoltan via]

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

Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping

[BALATON Zoltan via]

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
>
>

Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping

[Peter Maydell]

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

Re: [PATCH v2 3/3] sam460ex: Clean up irq mapping

[BALATON Zoltan via]

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