[PATCH-for-5.2 0/5] hw/arm: Fix various incorrect IRQ handling

[PATCH-for-5.2 0/5] hw/arm: Fix various incorrect IRQ handling

[Philippe Mathieu-Daudé]

This series is inspired by Peter's following patch:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg758178.html

I started to audit the IRQ uses and fixed the easy problems.

Unresolved ones:
- stellaris_init() connects different TYPE_STELLARIS_GPTM
  to the same ADC input (seems some weird kludge)
- platform_bus_link_device() uses sysbus_has_irq() to check
  if an device has IRQ mapped but it doesn't seem to work.

Anyway enough audit for the day.

Regards,

Phil.

Philippe Mathieu-Daudé (5):
  hw/arm/armsse: Correct expansion MPC interrupt lines
  hw/misc/stm32f2xx_syscfg: Remove extraneous IRQ
  hw/arm/nseries: Remove invalid/unnecessary n8x0_uart_setup()
  hw/arm/musicpal: Don't connect two qemu_irqs directly to the same
    input
  hw/arm/musicpal: Only use qdev_get_gpio_in() when necessary

 include/hw/misc/stm32f2xx_syscfg.h |  2 --
 hw/arm/armsse.c                    |  3 ++-
 hw/arm/musicpal.c                  | 40 +++++++++++++++++++-----------
 hw/arm/nseries.c                   | 11 --------
 hw/arm/stm32f205_soc.c             |  1 -
 hw/misc/stm32f2xx_syscfg.c         |  2 --
 hw/arm/Kconfig                     |  1 +
 7 files changed, 28 insertions(+), 32 deletions(-)

-- 
2.26.2

[PATCH-for-5.2 1/5] hw/arm/armsse: Correct expansion MPC interrupt lines

[Philippe Mathieu-Daudé]

We can use one MPC per SRAM bank, but we currently only wire the
IRQ from the first expansion MPC to the IRQ splitter. Fix that.

Fixes: bb75e16d5e6 ("hw/arm/iotkit: Wire up MPC interrupt lines")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/armsse.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/arm/armsse.c b/hw/arm/armsse.c
index a93da37dcbb..baac027659d 100644
--- a/hw/arm/armsse.c
+++ b/hw/arm/armsse.c
@@ -1074,7 +1074,8 @@ static void armsse_realize(DeviceState *dev, Error **errp)
                                         qdev_get_gpio_in(dev_splitter, 0));
             qdev_connect_gpio_out(dev_splitter, 0,
                                   qdev_get_gpio_in_named(dev_secctl,
-                                                         "mpc_status", 0));
+                                                         "mpc_status",
+                                                         i - IOTS_NUM_EXP_MPC));
         }
 
         qdev_connect_gpio_out(dev_splitter, 1,
-- 
2.26.2

[PATCH-for-5.2 2/5] hw/misc/stm32f2xx_syscfg: Remove extraneous IRQ

[Philippe Mathieu-Daudé]

The system configuration controller (SYSCFG) doesn't have
any output IRQ (and the INTC input #71 belongs to the UART6).
Remove the invalid code.

Fixes: db635521a02 ("stm32f205: Add the stm32f205 SoC")
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/misc/stm32f2xx_syscfg.h | 2 --
 hw/arm/stm32f205_soc.c             | 1 -
 hw/misc/stm32f2xx_syscfg.c         | 2 --
 3 files changed, 5 deletions(-)

diff --git a/include/hw/misc/stm32f2xx_syscfg.h b/include/hw/misc/stm32f2xx_syscfg.h
index 57a98c533db..8595a3b31b0 100644
--- a/include/hw/misc/stm32f2xx_syscfg.h
+++ b/include/hw/misc/stm32f2xx_syscfg.h
@@ -53,8 +53,6 @@ struct STM32F2XXSyscfgState {
     uint32_t syscfg_exticr3;
     uint32_t syscfg_exticr4;
     uint32_t syscfg_cmpcr;
-
-    qemu_irq irq;
 };
 
 #endif /* HW_STM32F2XX_SYSCFG_H */
diff --git a/hw/arm/stm32f205_soc.c b/hw/arm/stm32f205_soc.c
index a4f3344db25..9cd41bf56da 100644
--- a/hw/arm/stm32f205_soc.c
+++ b/hw/arm/stm32f205_soc.c
@@ -117,7 +117,6 @@ static void stm32f205_soc_realize(DeviceState *dev_soc, Error **errp)
     }
     busdev = SYS_BUS_DEVICE(dev);
     sysbus_mmio_map(busdev, 0, 0x40013800);
-    sysbus_connect_irq(busdev, 0, qdev_get_gpio_in(armv7m, 71));
 
     /* Attach UART (uses USART registers) and USART controllers */
     for (i = 0; i < STM_NUM_USARTS; i++) {
diff --git a/hw/misc/stm32f2xx_syscfg.c b/hw/misc/stm32f2xx_syscfg.c
index aa59b435498..04c22c28502 100644
--- a/hw/misc/stm32f2xx_syscfg.c
+++ b/hw/misc/stm32f2xx_syscfg.c
@@ -133,8 +133,6 @@ static void stm32f2xx_syscfg_init(Object *obj)
 {
     STM32F2XXSyscfgState *s = STM32F2XX_SYSCFG(obj);
 
-    sysbus_init_irq(SYS_BUS_DEVICE(obj), &s->irq);
-
     memory_region_init_io(&s->mmio, obj, &stm32f2xx_syscfg_ops, s,
                           TYPE_STM32F2XX_SYSCFG, 0x400);
     sysbus_init_mmio(SYS_BUS_DEVICE(obj), &s->mmio);
-- 
2.26.2

[PATCH-for-5.2 3/5] hw/arm/nseries: Remove invalid/unnecessary n8x0_uart_setup()

[Philippe Mathieu-Daudé]

omap2420_mpu_init() introduced in commit 827df9f3c5f ("Add basic
OMAP2 chip support") takes care of creating the 3 UARTs.

Then commit 58a26b477e9 ("Emulate a serial bluetooth HCI with H4+
extensions and attach to n8x0's UART") added n8x0_uart_setup()
which create the UART and connects it to an IRQ output,
overwritting the existing peripheral and its IRQ connection.
This is incorrect.

Fortunately we don't need to fix this, because commit 6da68df7f9b
("hw/arm/nseries: Replace the bluetooth chardev with a "null"
chardev") removed the use of this peripheral. We can simply
remove the code.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/nseries.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
index 76fd7fe9854..6215c18d627 100644
--- a/hw/arm/nseries.c
+++ b/hw/arm/nseries.c
@@ -789,16 +789,6 @@ static void n8x0_cbus_setup(struct n800_s *s)
     cbus_attach(cbus, s->tahvo = tahvo_init(tahvo_irq, 1));
 }
 
-static void n8x0_uart_setup(struct n800_s *s)
-{
-    Chardev *radio = qemu_chr_new("bt-dummy-uart", "null", NULL);
-    /*
-     * Note: We used to connect N8X0_BT_RESET_GPIO and N8X0_BT_WKUP_GPIO
-     * here, but this code has been removed with the bluetooth backend.
-     */
-    omap_uart_attach(s->mpu->uart[BT_UART], radio);
-}
-
 static void n8x0_usb_setup(struct n800_s *s)
 {
     SysBusDevice *dev;
@@ -1362,7 +1352,6 @@ static void n8x0_init(MachineState *machine,
     n8x0_spi_setup(s);
     n8x0_dss_setup(s);
     n8x0_cbus_setup(s);
-    n8x0_uart_setup(s);
     if (machine_usb(machine)) {
         n8x0_usb_setup(s);
     }
-- 
2.26.2

Re: [PATCH-for-5.2 3/5] hw/arm/nseries: Remove invalid/unnecessary n8x0_uart_setup()

[Peter Maydell]

On Sat, 7 Nov 2020 at 19:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> omap2420_mpu_init() introduced in commit 827df9f3c5f ("Add basic
> OMAP2 chip support") takes care of creating the 3 UARTs.
>
> Then commit 58a26b477e9 ("Emulate a serial bluetooth HCI with H4+
> extensions and attach to n8x0's UART") added n8x0_uart_setup()
> which create the UART and connects it to an IRQ output,
> overwritting the existing peripheral and its IRQ connection.
> This is incorrect.
>
> Fortunately we don't need to fix this, because commit 6da68df7f9b
> ("hw/arm/nseries: Replace the bluetooth chardev with a "null"
> chardev") removed the use of this peripheral. We can simply
> remove the code.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/arm/nseries.c | 11 -----------
>  1 file changed, 11 deletions(-)
>
> diff --git a/hw/arm/nseries.c b/hw/arm/nseries.c
> index 76fd7fe9854..6215c18d627 100644
> --- a/hw/arm/nseries.c
> +++ b/hw/arm/nseries.c
> @@ -789,16 +789,6 @@ static void n8x0_cbus_setup(struct n800_s *s)
>      cbus_attach(cbus, s->tahvo = tahvo_init(tahvo_irq, 1));
>  }
>
> -static void n8x0_uart_setup(struct n800_s *s)
> -{
> -    Chardev *radio = qemu_chr_new("bt-dummy-uart", "null", NULL);
> -    /*
> -     * Note: We used to connect N8X0_BT_RESET_GPIO and N8X0_BT_WKUP_GPIO
> -     * here, but this code has been removed with the bluetooth backend.
> -     */
> -    omap_uart_attach(s->mpu->uart[BT_UART], radio);
> -}

This deletes the only use of omap_uart_attach(), so as a
follow-up patch you could remove that function entirely.

thanks
-- PMM

[PATCH-for-5.2? 4/5] hw/arm/musicpal: Don't connect two qemu_irqs directly to the same input

[Philippe Mathieu-Daudé]

The MusicPal board code connects both of the IRQ outputs of the UART
to the same INTC 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 INTC 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.

Inspired-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/musicpal.c | 17 +++++++++++++----
 hw/arm/Kconfig    |  1 +
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index 5eb3f969fb4..ebc3ec24ef7 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -27,6 +27,7 @@
 #include "ui/console.h"
 #include "hw/i2c/i2c.h"
 #include "hw/irq.h"
+#include "hw/or-irq.h"
 #include "hw/audio/wm8750.h"
 #include "sysemu/block-backend.h"
 #include "sysemu/runstate.h"
@@ -77,8 +78,7 @@
 #define MP_TIMER4_IRQ           7
 #define MP_EHCI_IRQ             8
 #define MP_ETH_IRQ              9
-#define MP_UART1_IRQ            11
-#define MP_UART2_IRQ            11
+#define MP_UART_SHARED_IRQ      11
 #define MP_GPIO_IRQ             12
 #define MP_RTC_IRQ              28
 #define MP_AUDIO_IRQ            30
@@ -1589,6 +1589,7 @@ static void musicpal_init(MachineState *machine)
     ARMCPU *cpu;
     qemu_irq pic[32];
     DeviceState *dev;
+    DeviceState *uart_orgate;
     DeviceState *i2c_dev;
     DeviceState *lcd_dev;
     DeviceState *key_dev;
@@ -1627,9 +1628,17 @@ static void musicpal_init(MachineState *machine)
                           pic[MP_TIMER2_IRQ], pic[MP_TIMER3_IRQ],
                           pic[MP_TIMER4_IRQ], NULL);
 
-    serial_mm_init(address_space_mem, MP_UART1_BASE, 2, pic[MP_UART1_IRQ],
+    /* Logically OR both UART IRQs together */
+    uart_orgate = DEVICE(object_new(TYPE_OR_IRQ));
+    object_property_set_int(OBJECT(uart_orgate), "num-lines", 2, &error_fatal);
+    qdev_realize_and_unref(uart_orgate, NULL, &error_fatal);
+    qdev_connect_gpio_out(DEVICE(uart_orgate), 0, pic[MP_UART_SHARED_IRQ]);
+
+    serial_mm_init(address_space_mem, MP_UART1_BASE, 2,
+                   qdev_get_gpio_in(uart_orgate, 0),
                    1825000, serial_hd(0), DEVICE_NATIVE_ENDIAN);
-    serial_mm_init(address_space_mem, MP_UART2_BASE, 2, pic[MP_UART2_IRQ],
+    serial_mm_init(address_space_mem, MP_UART2_BASE, 2,
+                   qdev_get_gpio_in(uart_orgate, 1),
                    1825000, serial_hd(1), DEVICE_NATIVE_ENDIAN);
 
     /* Register flash */
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index fdf4464b948..319916c310e 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -94,6 +94,7 @@ config MUSCA
 
 config MUSICPAL
     bool
+    select OR_IRQ
     select BITBANG_I2C
     select MARVELL_88W8618
     select PTIMER
-- 
2.26.2

[PATCH-for-6.0 5/5] hw/arm/musicpal: Only use qdev_get_gpio_in() when necessary

[Philippe Mathieu-Daudé]

We don't need to fill the full pic[] array if we only use
few of the interrupt lines. Directly call qdev_get_gpio_in()
when necessary.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/musicpal.c | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/hw/arm/musicpal.c b/hw/arm/musicpal.c
index ebc3ec24ef7..458b1cbeb76 100644
--- a/hw/arm/musicpal.c
+++ b/hw/arm/musicpal.c
@@ -1587,8 +1587,8 @@ static struct arm_boot_info musicpal_binfo = {
 static void musicpal_init(MachineState *machine)
 {
     ARMCPU *cpu;
-    qemu_irq pic[32];
     DeviceState *dev;
+    DeviceState *pic;
     DeviceState *uart_orgate;
     DeviceState *i2c_dev;
     DeviceState *lcd_dev;
@@ -1619,20 +1619,20 @@ static void musicpal_init(MachineState *machine)
                            &error_fatal);
     memory_region_add_subregion(address_space_mem, MP_SRAM_BASE, sram);
 
-    dev = sysbus_create_simple(TYPE_MV88W8618_PIC, MP_PIC_BASE,
+    pic = sysbus_create_simple(TYPE_MV88W8618_PIC, MP_PIC_BASE,
                                qdev_get_gpio_in(DEVICE(cpu), ARM_CPU_IRQ));
-    for (i = 0; i < 32; i++) {
-        pic[i] = qdev_get_gpio_in(dev, i);
-    }
-    sysbus_create_varargs(TYPE_MV88W8618_PIT, MP_PIT_BASE, pic[MP_TIMER1_IRQ],
-                          pic[MP_TIMER2_IRQ], pic[MP_TIMER3_IRQ],
-                          pic[MP_TIMER4_IRQ], NULL);
+    sysbus_create_varargs(TYPE_MV88W8618_PIT, MP_PIT_BASE,
+                          qdev_get_gpio_in(pic, MP_TIMER1_IRQ),
+                          qdev_get_gpio_in(pic, MP_TIMER2_IRQ),
+                          qdev_get_gpio_in(pic, MP_TIMER3_IRQ),
+                          qdev_get_gpio_in(pic, MP_TIMER4_IRQ), NULL);
 
     /* Logically OR both UART IRQs together */
     uart_orgate = DEVICE(object_new(TYPE_OR_IRQ));
     object_property_set_int(OBJECT(uart_orgate), "num-lines", 2, &error_fatal);
     qdev_realize_and_unref(uart_orgate, NULL, &error_fatal);
-    qdev_connect_gpio_out(DEVICE(uart_orgate), 0, pic[MP_UART_SHARED_IRQ]);
+    qdev_connect_gpio_out(DEVICE(uart_orgate), 0,
+                          qdev_get_gpio_in(pic, MP_UART_SHARED_IRQ));
 
     serial_mm_init(address_space_mem, MP_UART1_BASE, 2,
                    qdev_get_gpio_in(uart_orgate, 0),
@@ -1674,14 +1674,15 @@ static void musicpal_init(MachineState *machine)
                              OBJECT(get_system_memory()), &error_fatal);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
     sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, MP_ETH_BASE);
-    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[MP_ETH_IRQ]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0,
+                       qdev_get_gpio_in(pic, MP_ETH_IRQ));
 
     sysbus_create_simple("mv88w8618_wlan", MP_WLAN_BASE, NULL);
 
     sysbus_create_simple(TYPE_MUSICPAL_MISC, MP_MISC_BASE, NULL);
 
     dev = sysbus_create_simple(TYPE_MUSICPAL_GPIO, MP_GPIO_BASE,
-                               pic[MP_GPIO_IRQ]);
+                               qdev_get_gpio_in(pic, MP_GPIO_IRQ));
     i2c_dev = sysbus_create_simple("gpio_i2c", -1, NULL);
     i2c = (I2CBus *)qdev_get_child_bus(i2c_dev, "i2c");
 
@@ -1713,7 +1714,7 @@ static void musicpal_init(MachineState *machine)
                              NULL);
     sysbus_realize_and_unref(s, &error_fatal);
     sysbus_mmio_map(s, 0, MP_AUDIO_BASE);
-    sysbus_connect_irq(s, 0, pic[MP_AUDIO_IRQ]);
+    sysbus_connect_irq(s, 0, qdev_get_gpio_in(pic, MP_AUDIO_IRQ));
 
     musicpal_binfo.ram_size = MP_RAM_DEFAULT_SIZE;
     arm_load_kernel(cpu, machine, &musicpal_binfo);
-- 
2.26.2

Re: [PATCH-for-5.2 0/5] hw/arm: Fix various incorrect IRQ handling

[Peter Maydell]

On Sat, 7 Nov 2020 at 19:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> This series is inspired by Peter's following patch:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg758178.html
>
> I started to audit the IRQ uses and fixed the easy problems.
>
> Unresolved ones:
> - stellaris_init() connects different TYPE_STELLARIS_GPTM
>   to the same ADC input (seems some weird kludge)
> - platform_bus_link_device() uses sysbus_has_irq() to check
>   if an device has IRQ mapped but it doesn't seem to work.

Were you finding these by inspection, or did you add some
kind of assert or other check for double-irq-line-connections?

Applied to target-arm.next, thanks (deletion of omap_uart_attach()
can be a separate followup cleanup).

-- PMM

Re: [PATCH-for-5.2 0/5] hw/arm: Fix various incorrect IRQ handling

[Philippe Mathieu-Daudé]

On 11/9/20 3:23 PM, Peter Maydell wrote:
> On Sat, 7 Nov 2020 at 19:34, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> This series is inspired by Peter's following patch:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg758178.html
>>
>> I started to audit the IRQ uses and fixed the easy problems.
>>
>> Unresolved ones:
>> - stellaris_init() connects different TYPE_STELLARIS_GPTM
>>   to the same ADC input (seems some weird kludge)
>> - platform_bus_link_device() uses sysbus_has_irq() to check
>>   if an device has IRQ mapped but it doesn't seem to work.
> 
> Were you finding these by inspection, or did you add some
> kind of assert or other check for double-irq-line-connections?

I was not sure adding a field to IRQState would be accepted,
so I added a trace event in qdev_connect_gpio_out_named():

 trace_qdev_connect_gpio_out(pin, n, name,
                             object_get_typename(OBJECT(dev)));

And lookup up when 'pin' pointer value appears more than 1 time.

For 6.0 I am wondering about replacing --enable-qom-cast-debug
by --maintainer-mode and add a field in IRQState guarded for
that mode. Maybe it is not too bad and we can use it normally.

> 
> Applied to target-arm.next, thanks (deletion of omap_uart_attach()
> can be a separate followup cleanup).

Yes, I have *some* OMAP patches for 6.0 ;)

> 
> -- PMM
>