Re: [PATCH] sun4m: don't connect two qemu_irqs directly to the same input

[Mark Cave-Ayland]

On 19/12/2020 11:19, Mark Cave-Ayland wrote:

> The sun4m board code connects both of the IRQ outputs of each ESCC to the
> same slavio input 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: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>   hw/sparc/Kconfig |  1 +
>   hw/sparc/sun4m.c | 23 ++++++++++++++++++-----
>   2 files changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sparc/Kconfig b/hw/sparc/Kconfig
> index 91805afab6..8dcb10086f 100644
> --- a/hw/sparc/Kconfig
> +++ b/hw/sparc/Kconfig
> @@ -14,6 +14,7 @@ config SUN4M
>       select M48T59
>       select STP2000
>       select CHRP_NVRAM
> +    select OR_IRQ
>   
>   config LEON3
>       bool
> diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 8686371318..c06c43be18 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -50,6 +50,7 @@
>   #include "hw/misc/empty_slot.h"
>   #include "hw/misc/unimp.h"
>   #include "hw/irq.h"
> +#include "hw/or-irq.h"
>   #include "hw/loader.h"
>   #include "elf.h"
>   #include "trace.h"
> @@ -848,7 +849,7 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>       uint32_t initrd_size;
>       DriveInfo *fd[MAX_FD];
>       FWCfgState *fw_cfg;
> -    DeviceState *dev;
> +    DeviceState *dev, *ms_kb_orgate, *serial_orgate;
>       SysBusDevice *s;
>       unsigned int smp_cpus = machine->smp.cpus;
>       unsigned int max_cpus = machine->smp.max_cpus;
> @@ -994,10 +995,16 @@ static void sun4m_hw_init(const struct sun4m_hwdef *hwdef,
>       qdev_prop_set_uint32(dev, "chnAtype", escc_kbd);
>       s = SYS_BUS_DEVICE(dev);
>       sysbus_realize_and_unref(s, &error_fatal);
> -    sysbus_connect_irq(s, 0, slavio_irq[14]);
> -    sysbus_connect_irq(s, 1, slavio_irq[14]);
>       sysbus_mmio_map(s, 0, hwdef->ms_kb_base);
>   
> +    /* Logically OR both its IRQs together */
> +    ms_kb_orgate = DEVICE(object_new(TYPE_OR_IRQ));
> +    object_property_set_int(OBJECT(ms_kb_orgate), "num-lines", 2, 
> &error_fatal);
> +    qdev_realize_and_unref(ms_kb_orgate, NULL, &error_fatal);
> +    sysbus_connect_irq(s, 0, qdev_get_gpio_in(ms_kb_orgate, 0));
> +    sysbus_connect_irq(s, 1, qdev_get_gpio_in(ms_kb_orgate, 1));
> +    qdev_connect_gpio_out(DEVICE(ms_kb_orgate), 0, slavio_irq[14]);
> +
>       dev = qdev_new(TYPE_ESCC);
>       qdev_prop_set_uint32(dev, "disabled", 0);
>       qdev_prop_set_uint32(dev, "frequency", ESCC_CLOCK);
> @@ -1009,10 +1016,16 @@ static void sun4m_hw_init(const struct sun4m_hwdef 
> *hwdef,
>   
>       s = SYS_BUS_DEVICE(dev);
>       sysbus_realize_and_unref(s, &error_fatal);
> -    sysbus_connect_irq(s, 0, slavio_irq[15]);
> -    sysbus_connect_irq(s, 1,  slavio_irq[15]);
>       sysbus_mmio_map(s, 0, hwdef->serial_base);
>   
> +    /* Logically OR both its IRQs together */
> +    serial_orgate = DEVICE(object_new(TYPE_OR_IRQ));
> +    object_property_set_int(OBJECT(serial_orgate), "num-lines", 2, 
> &error_fatal);
> +    qdev_realize_and_unref(serial_orgate, NULL, &error_fatal);
> +    sysbus_connect_irq(s, 0, qdev_get_gpio_in(serial_orgate, 0));
> +    sysbus_connect_irq(s, 1, qdev_get_gpio_in(serial_orgate, 1));
> +    qdev_connect_gpio_out(DEVICE(serial_orgate), 0, slavio_irq[15]);
> +
>       if (hwdef->apc_base) {
>           apc_init(hwdef->apc_base, qemu_allocate_irq(cpu_halt_signal, NULL, 
> 0));
>       }

I've applied this to my qemu-sparc branch, with a slight tweak to the second 
object_property_set_int() line as it was giving a warning on checkpatch.pl for being 
just over 80 characters in length.


ATB,

Mark.