1. Patchew
  2. QEMU
  3. [PATCH 0/2] m68k/q800: make the GLUE chip a QOM device
  4. View patch

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

Peter Maydell posted 2 patches 5 years, 3 months ago
Maintainers: Laurent Vivier <laurent@vivier.eu>
[PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input
Posted by Peter Maydell 5 years, 3 months ago 
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]);
+
+    /* 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
-- 
2.20.1
Re: [PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input
Posted by Philippe Mathieu-Daudé 5 years, 3 months ago 
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
>
Re: [PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input
Posted by Philippe Mathieu-Daudé 5 years, 3 months ago 
> 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(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Re: [PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input
Posted by Mark Cave-Ayland 5 years, 3 months ago 
On 07/11/2020 14:52, Philippe Mathieu-Daudé wrote:

> 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]);

Good spot. I'm fairly confident the code has been like this for a long time, so I 
don't see this is a being a critical fix for 5.2. I'll add it to my 6.0 TODO list.


ATB,

Mark.
Re: [PATCH 1/2] hw/m68k/q800: Don't connect two qemu_irqs directly to the same input
Posted by Laurent Vivier 5 years, 3 months ago 
Le 07/11/2020 à 00:51, Peter Maydell a écrit :
> 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(-)
...

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.