sun4m: don't connect two qemu_irqs directly to the same input

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

Posted by Mark Cave-Ayland 4 years, 11 months ago

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));
     }
-- 
2.20.1

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

Posted by Mark Cave-Ayland 4 years, 10 months ago

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.

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

Posted by Artyom Tarasenko 4 years, 11 months ago

сб, 19 дек. 2020 г., 12:19 Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>:

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

Reviewed-by: Artyom Tarasenko <atar4qemu@gmail.com>

---
>  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));
>      }
> --
> 2.20.1
>
>