Re: [PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input

[Philippe Mathieu-Daudé]

Cc'ing SPARC maintainers ...

On 11/7/20 12:51 AM, Peter Maydell wrote:
> The q800 board code connects both of the IRQ outputs of the ESCC
> to the same pic[3] qemu_irq. Connecting two qemu_irqs outputs directly
> to the same input is not valid as it produces subtly wrong behaviour
> (for instance if both the IRQ lines are high, and then one goes
> low, the PIC input will see this as a high-to-low transition
> even though the second IRQ line should still be holding it high).
> 
> This kind of wiring needs an explicitly created OR gate; add one.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/m68k/q800.c  | 12 ++++++++++--
>  hw/m68k/Kconfig |  1 +
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index ce4b47c3e34..dc13007aaf2 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -28,6 +28,7 @@
>  #include "hw/hw.h"
>  #include "hw/boards.h"
>  #include "hw/irq.h"
> +#include "hw/or-irq.h"
>  #include "elf.h"
>  #include "hw/loader.h"
>  #include "ui/console.h"
> @@ -171,6 +172,7 @@ static void q800_init(MachineState *machine)
>      CPUState *cs;
>      DeviceState *dev;
>      DeviceState *via_dev;
> +    DeviceState *escc_orgate;
>      SysBusESPState *sysbus_esp;
>      ESPState *esp;
>      SysBusDevice *sysbus;
> @@ -283,8 +285,14 @@ static void q800_init(MachineState *machine)
>      qdev_prop_set_uint32(dev, "chnAtype", 0);
>      sysbus = SYS_BUS_DEVICE(dev);
>      sysbus_realize_and_unref(sysbus, &error_fatal);
> -    sysbus_connect_irq(sysbus, 0, pic[3]);
> -    sysbus_connect_irq(sysbus, 1, pic[3]);

... because sun4m_hw_init() has the same issue:

 986     dev = qdev_new(TYPE_ESCC);
...
 996     sysbus_connect_irq(s, 0, slavio_irq[14]);
 997     sysbus_connect_irq(s, 1, slavio_irq[14]);
...
1011     sysbus_connect_irq(s, 0, slavio_irq[15]);
1012     sysbus_connect_irq(s, 1,  slavio_irq[15]);

> +
> +    /* Logically OR both its IRQs together */
> +    escc_orgate = DEVICE(object_new(TYPE_OR_IRQ));
> +    object_property_set_int(OBJECT(escc_orgate), "num-lines", 2, 
> &error_fatal);
> +    qdev_realize_and_unref(escc_orgate, NULL, &error_fatal);
> +    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(escc_orgate, 0));
> +    sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(escc_orgate, 1));
> +    qdev_connect_gpio_out(DEVICE(escc_orgate), 0, pic[3]);
>      sysbus_mmio_map(sysbus, 0, SCC_BASE);
>  
>      /* SCSI */
> diff --git a/hw/m68k/Kconfig b/hw/m68k/Kconfig
> index c757e7dfa48..60d7bcfb8f2 100644
> --- a/hw/m68k/Kconfig
> +++ b/hw/m68k/Kconfig
> @@ -22,3 +22,4 @@ config Q800
>      select ESCC
>      select ESP
>      select DP8393X
> +    select OR_IRQ
>