[PATCH 00/14] i.MX and SDHCI improvements

[PATCH 00/14] i.MX and SDHCI improvements

[Bernhard Beschow]

This series fixes some details in i.MX platform devices, improves SDHCI
compatibility with U-Boot and modernizes some code.

The first 6 patches are bugfixes 1/ resolving infinite loop in U-Boot esdhc
driver, 2/ fixing a character echoing issue in imx-serial, 3/ fixing IRQ sharing
issue in Designware PCIe emulation, and 4/ fixing GPIO level preservation across
resets in imx-gpio. The IRQ sharing issue is fixed using a new device type
"TYPE_SHARED_IRQ" which is inspired by TYPE_SPLIT_IRQ.

Patches 7 and 8 modernize SD card emulation by turning presence and
write-protect GPIOs into qdev GPIOs and then further allowing the GPIOs to be
inverted, just like device tree allows.

The rest of the series is cosmetics including turning DPRINTF() into trace
events which eases debugging.

Bernhard Beschow (14):
  hw/sd/sdhci: Set SDHC_NIS_DMA bit when appropriate
  hw/char/imx_serial: Fix reset value of UFCR register
  hw/char/imx_serial: Update all state before restarting ageing timer
  hw/core: Introduce TYPE_SHARED_IRQ
  hw/pci-host/designware: Expose MSI IRQ
  hw/gpio/imx_gpio: Don't clear input GPIO values upon reset
  hw/sd/sd: Remove legacy sd_set_cb() in favor of GPIOs
  hw/sd/sd: Allow for inverting polarities of presence and write-protect
    GPIOs
  hw/char/imx_serial: Turn some DPRINTF() statements into trace events
  hw/timer/imx_gpt: Remove unused define
  tests/qtest/libqos: Reuse TYPE_IMX_I2C define
  hw/i2c/imx_i2c: Convert DPRINTF() to trace events
  hw/misc/imx6_src: Convert DPRINTF() to trace events
  hw/gpio/imx_gpio: Turn DPRINTF() into trace events

 include/hw/arm/fsl-imx6.h                  |  4 +-
 include/hw/arm/fsl-imx7.h                  |  4 +-
 include/hw/char/imx_serial.h               |  2 +-
 include/hw/core/shared-irq.h               | 39 ++++++++++
 include/hw/pci-host/designware.h           |  1 +
 include/hw/sd/sdcard_legacy.h              |  1 -
 hw/arm/fsl-imx6.c                          | 13 +++-
 hw/arm/fsl-imx7.c                          | 13 +++-
 hw/char/imx_serial.c                       | 65 ++++++++++------
 hw/core/shared-irq.c                       | 88 ++++++++++++++++++++++
 hw/gpio/imx_gpio.c                         | 17 ++---
 hw/i2c/imx_i2c.c                           | 21 ++----
 hw/misc/imx6_src.c                         | 23 ++----
 hw/pci-host/designware.c                   |  7 +-
 hw/sd/sd.c                                 | 39 +++++-----
 hw/sd/sdhci.c                              | 11 ++-
 hw/timer/imx_gpt.c                         |  4 -
 tests/qtest/libqos/arm-imx25-pdk-machine.c |  5 +-
 tests/qtest/libqos/i2c-imx.c               |  4 +-
 hw/arm/Kconfig                             |  2 +
 hw/char/trace-events                       |  5 ++
 hw/core/Kconfig                            |  3 +
 hw/core/meson.build                        |  1 +
 hw/gpio/trace-events                       |  5 ++
 hw/i2c/trace-events                        |  5 ++
 hw/misc/trace-events                       |  6 ++
 26 files changed, 280 insertions(+), 108 deletions(-)
 create mode 100644 include/hw/core/shared-irq.h
 create mode 100644 hw/core/shared-irq.c

-- 
2.47.1

[PATCH 01/14] hw/sd/sdhci: Set SDHC_NIS_DMA bit when appropriate

[Bernhard Beschow]

In U-Boot, the fsl_esdhc[_imx] driver waits for both "transmit completed" and
"DMA" bits in esdhc_send_cmd_common() by means of DATA_COMPLETE constant. QEMU
currently misses to set the DMA bit which causes the driver to loop forever. Fix
that by setting the DMA bit if enabled when doing DMA block transfers.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/sd/sdhci.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 299cd4bc1b..a958c11497 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -665,12 +665,13 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
         }
     }
 
+    if (s->norintstsen & SDHC_NISEN_DMA) {
+        s->norintsts |= SDHC_NIS_DMA;
+    }
+
     if (s->blkcnt == 0) {
         sdhci_end_transfer(s);
     } else {
-        if (s->norintstsen & SDHC_NISEN_DMA) {
-            s->norintsts |= SDHC_NIS_DMA;
-        }
         sdhci_update_irq(s);
     }
 }
@@ -691,6 +692,10 @@ static void sdhci_sdma_transfer_single_block(SDHCIState *s)
     }
     s->blkcnt--;
 
+    if (s->norintstsen & SDHC_NISEN_DMA) {
+        s->norintsts |= SDHC_NIS_DMA;
+    }
+
     sdhci_end_transfer(s);
 }
 
-- 
2.47.1

Re: [PATCH 01/14] hw/sd/sdhci: Set SDHC_NIS_DMA bit when appropriate

[Philippe Mathieu-Daudé]

On 8/1/25 10:25, Bernhard Beschow wrote:
> In U-Boot, the fsl_esdhc[_imx] driver waits for both "transmit completed" and
> "DMA" bits in esdhc_send_cmd_common() by means of DATA_COMPLETE constant. QEMU
> currently misses to set the DMA bit which causes the driver to loop forever. Fix
> that by setting the DMA bit if enabled when doing DMA block transfers.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/sd/sdhci.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 02/14] hw/char/imx_serial: Fix reset value of UFCR register

[Bernhard Beschow]

The value of the UCFR register is respected when echoing characters to the
terminal, but its reset value is reserved. Fix the reset value to the one
documented in the datasheet.

While at it move the related attribute out of the section of unimplemented
registers since its value is actually respected.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/char/imx_serial.h | 2 +-
 hw/char/imx_serial.c         | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/hw/char/imx_serial.h b/include/hw/char/imx_serial.h
index 65f0e97c76..90ba3ff18c 100644
--- a/include/hw/char/imx_serial.h
+++ b/include/hw/char/imx_serial.h
@@ -109,13 +109,13 @@ struct IMXSerialState {
     uint32_t ucr1;
     uint32_t ucr2;
     uint32_t uts1;
+    uint32_t ufcr;
 
     /*
      * The registers below are implemented just so that the
      * guest OS sees what it has written
      */
     uint32_t onems;
-    uint32_t ufcr;
     uint32_t ubmr;
     uint32_t ubrc;
     uint32_t ucr3;
diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
index 12705a1337..f805da23ff 100644
--- a/hw/char/imx_serial.c
+++ b/hw/char/imx_serial.c
@@ -159,6 +159,7 @@ static void imx_serial_reset(IMXSerialState *s)
     s->ucr3 = 0x700;
     s->ubmr = 0;
     s->ubrc = 4;
+    s->ufcr = BIT(11) | BIT(0);
 
     fifo32_reset(&s->rx_fifo);
     timer_del(&s->ageing_timer);
-- 
2.47.1

[PATCH 03/14] hw/char/imx_serial: Update all state before restarting ageing timer

[Bernhard Beschow]

Fixes characters to be "echoed" after each keystroke rather than after every
other since imx_serial_rx_fifo_ageing_timer_restart() would see ~UTS1_RXEMPTY
only after every other keystroke.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/char/imx_serial.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
index f805da23ff..be06f39a4d 100644
--- a/hw/char/imx_serial.c
+++ b/hw/char/imx_serial.c
@@ -381,14 +381,14 @@ static void imx_put_data(void *opaque, uint32_t value)
     if (fifo32_num_used(&s->rx_fifo) >= rxtl) {
         s->usr1 |= USR1_RRDY;
     }
-
-    imx_serial_rx_fifo_ageing_timer_restart(s);
-
     s->usr2 |= USR2_RDR;
     s->uts1 &= ~UTS1_RXEMPTY;
     if (value & URXD_BRK) {
         s->usr2 |= USR2_BRCD;
     }
+
+    imx_serial_rx_fifo_ageing_timer_restart(s);
+
     imx_update(s);
 }
 
-- 
2.47.1

[PATCH 04/14] hw/core: Introduce TYPE_SHARED_IRQ

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/core/shared-irq.h | 39 ++++++++++++++++
 hw/core/shared-irq.c         | 88 ++++++++++++++++++++++++++++++++++++
 hw/core/Kconfig              |  3 ++
 hw/core/meson.build          |  1 +
 4 files changed, 131 insertions(+)
 create mode 100644 include/hw/core/shared-irq.h
 create mode 100644 hw/core/shared-irq.c

diff --git a/include/hw/core/shared-irq.h b/include/hw/core/shared-irq.h
new file mode 100644
index 0000000000..803c303dd0
--- /dev/null
+++ b/include/hw/core/shared-irq.h
@@ -0,0 +1,39 @@
+/*
+ * IRQ sharing device.
+ *
+ * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+/*
+ * This is a simple device which has one GPIO output line and multiple GPIO
+ * input lines. The output line is active if at least one of the input lines is.
+ *
+ * QEMU interface:
+ *  + N unnamed GPIO inputs: the input lines
+ *  + one unnamed GPIO output: the output line
+ *  + QOM property "num-lines": sets the number of input lines
+ */
+#ifndef HW_SHARED_IRQ_H
+#define HW_SHARED_IRQ_H
+
+#include "hw/sysbus.h"
+#include "qom/object.h"
+
+#define TYPE_SHARED_IRQ "shared-irq"
+
+#define MAX_SHARED_LINES 16
+
+
+OBJECT_DECLARE_SIMPLE_TYPE(SharedIRQ, SHARED_IRQ)
+
+struct SharedIRQ {
+    DeviceState parent_obj;
+
+    qemu_irq out_irq;
+    uint16_t irq_states;
+    uint8_t num_lines;
+};
+
+#endif
diff --git a/hw/core/shared-irq.c b/hw/core/shared-irq.c
new file mode 100644
index 0000000000..b2a4ea4a66
--- /dev/null
+++ b/hw/core/shared-irq.c
@@ -0,0 +1,88 @@
+/*
+ * IRQ sharing device.
+ *
+ * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/core/shared-irq.h"
+#include "hw/irq.h"
+#include "hw/qdev-properties.h"
+#include "qapi/error.h"
+#include "migration/vmstate.h"
+
+static void shared_irq_handler(void *opaque, int n, int level)
+{
+    SharedIRQ *s = opaque;
+    uint16_t mask = BIT(n);
+
+    if (level) {
+        s->irq_states |= mask;
+    } else {
+        s->irq_states &= ~mask;
+    }
+
+    qemu_set_irq(s->out_irq, !!s->irq_states);
+}
+
+static void shared_irq_init(Object *obj)
+{
+    SharedIRQ *s = SHARED_IRQ(obj);
+
+    qdev_init_gpio_out(DEVICE(s), &s->out_irq, 1);
+}
+
+static void shared_irq_realize(DeviceState *dev, Error **errp)
+{
+    SharedIRQ *s = SHARED_IRQ(dev);
+
+    if (s->num_lines < 1 || s->num_lines >= MAX_SHARED_LINES) {
+        error_setg(errp,
+                   "IRQ shared number of lines %d must be between 1 and %d",
+                   s->num_lines, MAX_SHARED_LINES);
+        return;
+    }
+
+    qdev_init_gpio_in(dev, shared_irq_handler, s->num_lines);
+}
+
+static const Property shared_irq_properties[] = {
+    DEFINE_PROP_UINT8("num-lines", SharedIRQ, num_lines, 1),
+};
+
+static const VMStateDescription shared_irq_vmstate = {
+    .name = TYPE_SHARED_IRQ,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (const VMStateField[]) {
+        VMSTATE_UINT16(irq_states, SharedIRQ),
+        VMSTATE_END_OF_LIST()
+    },
+};
+
+static void shared_irq_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+
+    /* No state to reset */
+    device_class_set_props(dc, shared_irq_properties);
+    dc->vmsd = &shared_irq_vmstate;
+    dc->realize = shared_irq_realize;
+
+    /* Reason: Needs to be wired up to work */
+    dc->user_creatable = false;
+}
+
+static const TypeInfo shared_irq_types[] = {
+    {
+       .name = TYPE_SHARED_IRQ,
+       .parent = TYPE_DEVICE,
+       .instance_size = sizeof(SharedIRQ),
+       .instance_init = shared_irq_init,
+       .class_init = shared_irq_class_init,
+    },
+};
+
+DEFINE_TYPES(shared_irq_types)
diff --git a/hw/core/Kconfig b/hw/core/Kconfig
index d1bdf765ee..ddff977963 100644
--- a/hw/core/Kconfig
+++ b/hw/core/Kconfig
@@ -32,6 +32,9 @@ config PLATFORM_BUS
 config REGISTER
     bool
 
+config SHARED_IRQ
+    bool
+
 config SPLIT_IRQ
     bool
 
diff --git a/hw/core/meson.build b/hw/core/meson.build
index ce9dfa3f4b..6b5bdc8ec7 100644
--- a/hw/core/meson.build
+++ b/hw/core/meson.build
@@ -21,6 +21,7 @@ system_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
 system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
 system_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
 system_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
+system_ss.add(when: 'CONFIG_SHARED_IRQ', if_true: files('shared-irq.c'))
 system_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
 system_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
 system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('sysbus-fdt.c'))
-- 
2.47.1

Re: [PATCH 04/14] hw/core: Introduce TYPE_SHARED_IRQ

[BALATON Zoltan]

On Wed, 8 Jan 2025, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
> include/hw/core/shared-irq.h | 39 ++++++++++++++++
> hw/core/shared-irq.c         | 88 ++++++++++++++++++++++++++++++++++++
> hw/core/Kconfig              |  3 ++
> hw/core/meson.build          |  1 +
> 4 files changed, 131 insertions(+)
> create mode 100644 include/hw/core/shared-irq.h
> create mode 100644 hw/core/shared-irq.c
>
> diff --git a/include/hw/core/shared-irq.h b/include/hw/core/shared-irq.h
> new file mode 100644
> index 0000000000..803c303dd0
> --- /dev/null
> +++ b/include/hw/core/shared-irq.h
> @@ -0,0 +1,39 @@
> +/*
> + * IRQ sharing device.
> + *
> + * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +/*
> + * This is a simple device which has one GPIO output line and multiple GPIO
> + * input lines. The output line is active if at least one of the input lines is.

How is this different from TYPE_OR_IRQ. Also or-irq.h is in 
include/hw/or-irq.h not in include/hw/core/ where split-irq.h is so maybe 
these could all be moved to one place for consistency? Or-irq also has a 
reset method, do you need one in this device?

Regards,
BALATON Zoltan

> + *
> + * QEMU interface:
> + *  + N unnamed GPIO inputs: the input lines
> + *  + one unnamed GPIO output: the output line
> + *  + QOM property "num-lines": sets the number of input lines
> + */
> +#ifndef HW_SHARED_IRQ_H
> +#define HW_SHARED_IRQ_H
> +
> +#include "hw/sysbus.h"
> +#include "qom/object.h"
> +
> +#define TYPE_SHARED_IRQ "shared-irq"
> +
> +#define MAX_SHARED_LINES 16
> +
> +
> +OBJECT_DECLARE_SIMPLE_TYPE(SharedIRQ, SHARED_IRQ)
> +
> +struct SharedIRQ {
> +    DeviceState parent_obj;
> +
> +    qemu_irq out_irq;
> +    uint16_t irq_states;
> +    uint8_t num_lines;
> +};
> +
> +#endif
> diff --git a/hw/core/shared-irq.c b/hw/core/shared-irq.c
> new file mode 100644
> index 0000000000..b2a4ea4a66
> --- /dev/null
> +++ b/hw/core/shared-irq.c
> @@ -0,0 +1,88 @@
> +/*
> + * IRQ sharing device.
> + *
> + * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/core/shared-irq.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/error.h"
> +#include "migration/vmstate.h"
> +
> +static void shared_irq_handler(void *opaque, int n, int level)
> +{
> +    SharedIRQ *s = opaque;
> +    uint16_t mask = BIT(n);
> +
> +    if (level) {
> +        s->irq_states |= mask;
> +    } else {
> +        s->irq_states &= ~mask;
> +    }
> +
> +    qemu_set_irq(s->out_irq, !!s->irq_states);
> +}
> +
> +static void shared_irq_init(Object *obj)
> +{
> +    SharedIRQ *s = SHARED_IRQ(obj);
> +
> +    qdev_init_gpio_out(DEVICE(s), &s->out_irq, 1);
> +}
> +
> +static void shared_irq_realize(DeviceState *dev, Error **errp)
> +{
> +    SharedIRQ *s = SHARED_IRQ(dev);
> +
> +    if (s->num_lines < 1 || s->num_lines >= MAX_SHARED_LINES) {
> +        error_setg(errp,
> +                   "IRQ shared number of lines %d must be between 1 and %d",
> +                   s->num_lines, MAX_SHARED_LINES);
> +        return;
> +    }
> +
> +    qdev_init_gpio_in(dev, shared_irq_handler, s->num_lines);
> +}
> +
> +static const Property shared_irq_properties[] = {
> +    DEFINE_PROP_UINT8("num-lines", SharedIRQ, num_lines, 1),
> +};
> +
> +static const VMStateDescription shared_irq_vmstate = {
> +    .name = TYPE_SHARED_IRQ,
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT16(irq_states, SharedIRQ),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
> +static void shared_irq_class_init(ObjectClass *klass, void *data)
> +{
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    /* No state to reset */
> +    device_class_set_props(dc, shared_irq_properties);
> +    dc->vmsd = &shared_irq_vmstate;
> +    dc->realize = shared_irq_realize;
> +
> +    /* Reason: Needs to be wired up to work */
> +    dc->user_creatable = false;
> +}
> +
> +static const TypeInfo shared_irq_types[] = {
> +    {
> +       .name = TYPE_SHARED_IRQ,
> +       .parent = TYPE_DEVICE,
> +       .instance_size = sizeof(SharedIRQ),
> +       .instance_init = shared_irq_init,
> +       .class_init = shared_irq_class_init,
> +    },
> +};
> +
> +DEFINE_TYPES(shared_irq_types)
> diff --git a/hw/core/Kconfig b/hw/core/Kconfig
> index d1bdf765ee..ddff977963 100644
> --- a/hw/core/Kconfig
> +++ b/hw/core/Kconfig
> @@ -32,6 +32,9 @@ config PLATFORM_BUS
> config REGISTER
>     bool
>
> +config SHARED_IRQ
> +    bool
> +
> config SPLIT_IRQ
>     bool
>
> diff --git a/hw/core/meson.build b/hw/core/meson.build
> index ce9dfa3f4b..6b5bdc8ec7 100644
> --- a/hw/core/meson.build
> +++ b/hw/core/meson.build
> @@ -21,6 +21,7 @@ system_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
> system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
> system_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
> system_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
> +system_ss.add(when: 'CONFIG_SHARED_IRQ', if_true: files('shared-irq.c'))
> system_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
> system_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
> system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('sysbus-fdt.c'))
>

Re: [PATCH 04/14] hw/core: Introduce TYPE_SHARED_IRQ

[Bernhard Beschow]

Am 8. Januar 2025 13:53:02 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Wed, 8 Jan 2025, Bernhard Beschow wrote:
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>> include/hw/core/shared-irq.h | 39 ++++++++++++++++
>> hw/core/shared-irq.c         | 88 ++++++++++++++++++++++++++++++++++++
>> hw/core/Kconfig              |  3 ++
>> hw/core/meson.build          |  1 +
>> 4 files changed, 131 insertions(+)
>> create mode 100644 include/hw/core/shared-irq.h
>> create mode 100644 hw/core/shared-irq.c
>> 
>> diff --git a/include/hw/core/shared-irq.h b/include/hw/core/shared-irq.h
>> new file mode 100644
>> index 0000000000..803c303dd0
>> --- /dev/null
>> +++ b/include/hw/core/shared-irq.h
>> @@ -0,0 +1,39 @@
>> +/*
>> + * IRQ sharing device.
>> + *
>> + * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +/*
>> + * This is a simple device which has one GPIO output line and multiple GPIO
>> + * input lines. The output line is active if at least one of the input lines is.
>
>How is this different from TYPE_OR_IRQ. Also or-irq.h is in include/hw/or-irq.h not in include/hw/core/ where split-irq.h is

I missed exactly that.

> so maybe these could all be moved to one place for consistency? Or-irq also has a reset method, do you need one in this device?

TYPE_OR_IRQ should be used instead. The next iteration will use it and this patch will be dropped.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan
>
>> + *
>> + * QEMU interface:
>> + *  + N unnamed GPIO inputs: the input lines
>> + *  + one unnamed GPIO output: the output line
>> + *  + QOM property "num-lines": sets the number of input lines
>> + */
>> +#ifndef HW_SHARED_IRQ_H
>> +#define HW_SHARED_IRQ_H
>> +
>> +#include "hw/sysbus.h"
>> +#include "qom/object.h"
>> +
>> +#define TYPE_SHARED_IRQ "shared-irq"
>> +
>> +#define MAX_SHARED_LINES 16
>> +
>> +
>> +OBJECT_DECLARE_SIMPLE_TYPE(SharedIRQ, SHARED_IRQ)
>> +
>> +struct SharedIRQ {
>> +    DeviceState parent_obj;
>> +
>> +    qemu_irq out_irq;
>> +    uint16_t irq_states;
>> +    uint8_t num_lines;
>> +};
>> +
>> +#endif
>> diff --git a/hw/core/shared-irq.c b/hw/core/shared-irq.c
>> new file mode 100644
>> index 0000000000..b2a4ea4a66
>> --- /dev/null
>> +++ b/hw/core/shared-irq.c
>> @@ -0,0 +1,88 @@
>> +/*
>> + * IRQ sharing device.
>> + *
>> + * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
>> + *
>> + * SPDX-License-Identifier: GPL-2.0-or-later
>> + */
>> +
>> +#include "qemu/osdep.h"
>> +#include "hw/core/shared-irq.h"
>> +#include "hw/irq.h"
>> +#include "hw/qdev-properties.h"
>> +#include "qapi/error.h"
>> +#include "migration/vmstate.h"
>> +
>> +static void shared_irq_handler(void *opaque, int n, int level)
>> +{
>> +    SharedIRQ *s = opaque;
>> +    uint16_t mask = BIT(n);
>> +
>> +    if (level) {
>> +        s->irq_states |= mask;
>> +    } else {
>> +        s->irq_states &= ~mask;
>> +    }
>> +
>> +    qemu_set_irq(s->out_irq, !!s->irq_states);
>> +}
>> +
>> +static void shared_irq_init(Object *obj)
>> +{
>> +    SharedIRQ *s = SHARED_IRQ(obj);
>> +
>> +    qdev_init_gpio_out(DEVICE(s), &s->out_irq, 1);
>> +}
>> +
>> +static void shared_irq_realize(DeviceState *dev, Error **errp)
>> +{
>> +    SharedIRQ *s = SHARED_IRQ(dev);
>> +
>> +    if (s->num_lines < 1 || s->num_lines >= MAX_SHARED_LINES) {
>> +        error_setg(errp,
>> +                   "IRQ shared number of lines %d must be between 1 and %d",
>> +                   s->num_lines, MAX_SHARED_LINES);
>> +        return;
>> +    }
>> +
>> +    qdev_init_gpio_in(dev, shared_irq_handler, s->num_lines);
>> +}
>> +
>> +static const Property shared_irq_properties[] = {
>> +    DEFINE_PROP_UINT8("num-lines", SharedIRQ, num_lines, 1),
>> +};
>> +
>> +static const VMStateDescription shared_irq_vmstate = {
>> +    .name = TYPE_SHARED_IRQ,
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (const VMStateField[]) {
>> +        VMSTATE_UINT16(irq_states, SharedIRQ),
>> +        VMSTATE_END_OF_LIST()
>> +    },
>> +};
>> +
>> +static void shared_irq_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    /* No state to reset */
>> +    device_class_set_props(dc, shared_irq_properties);
>> +    dc->vmsd = &shared_irq_vmstate;
>> +    dc->realize = shared_irq_realize;
>> +
>> +    /* Reason: Needs to be wired up to work */
>> +    dc->user_creatable = false;
>> +}
>> +
>> +static const TypeInfo shared_irq_types[] = {
>> +    {
>> +       .name = TYPE_SHARED_IRQ,
>> +       .parent = TYPE_DEVICE,
>> +       .instance_size = sizeof(SharedIRQ),
>> +       .instance_init = shared_irq_init,
>> +       .class_init = shared_irq_class_init,
>> +    },
>> +};
>> +
>> +DEFINE_TYPES(shared_irq_types)
>> diff --git a/hw/core/Kconfig b/hw/core/Kconfig
>> index d1bdf765ee..ddff977963 100644
>> --- a/hw/core/Kconfig
>> +++ b/hw/core/Kconfig
>> @@ -32,6 +32,9 @@ config PLATFORM_BUS
>> config REGISTER
>>     bool
>> 
>> +config SHARED_IRQ
>> +    bool
>> +
>> config SPLIT_IRQ
>>     bool
>> 
>> diff --git a/hw/core/meson.build b/hw/core/meson.build
>> index ce9dfa3f4b..6b5bdc8ec7 100644
>> --- a/hw/core/meson.build
>> +++ b/hw/core/meson.build
>> @@ -21,6 +21,7 @@ system_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
>> system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
>> system_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
>> system_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
>> +system_ss.add(when: 'CONFIG_SHARED_IRQ', if_true: files('shared-irq.c'))
>> system_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
>> system_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
>> system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('sysbus-fdt.c'))
>> 

Re: [PATCH 04/14] hw/core: Introduce TYPE_SHARED_IRQ

[Bernhard Beschow]

Am 8. Januar 2025 09:25:28 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
>Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>---
> include/hw/core/shared-irq.h | 39 ++++++++++++++++
> hw/core/shared-irq.c         | 88 ++++++++++++++++++++++++++++++++++++
> hw/core/Kconfig              |  3 ++
> hw/core/meson.build          |  1 +
> 4 files changed, 131 insertions(+)
> create mode 100644 include/hw/core/shared-irq.h
> create mode 100644 hw/core/shared-irq.c
>
>diff --git a/include/hw/core/shared-irq.h b/include/hw/core/shared-irq.h
>new file mode 100644

As pointed out by David, this device is redundant to TYPE_OR_IRQ. I'll drop it in v2.

Best regards,
Bernhard

>index 0000000000..803c303dd0
>--- /dev/null
>+++ b/include/hw/core/shared-irq.h
>@@ -0,0 +1,39 @@
>+/*
>+ * IRQ sharing device.
>+ *
>+ * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
>+ *
>+ * SPDX-License-Identifier: GPL-2.0-or-later
>+ */
>+
>+/*
>+ * This is a simple device which has one GPIO output line and multiple GPIO
>+ * input lines. The output line is active if at least one of the input lines is.
>+ *
>+ * QEMU interface:
>+ *  + N unnamed GPIO inputs: the input lines
>+ *  + one unnamed GPIO output: the output line
>+ *  + QOM property "num-lines": sets the number of input lines
>+ */
>+#ifndef HW_SHARED_IRQ_H
>+#define HW_SHARED_IRQ_H
>+
>+#include "hw/sysbus.h"
>+#include "qom/object.h"
>+
>+#define TYPE_SHARED_IRQ "shared-irq"
>+
>+#define MAX_SHARED_LINES 16
>+
>+
>+OBJECT_DECLARE_SIMPLE_TYPE(SharedIRQ, SHARED_IRQ)
>+
>+struct SharedIRQ {
>+    DeviceState parent_obj;
>+
>+    qemu_irq out_irq;
>+    uint16_t irq_states;
>+    uint8_t num_lines;
>+};
>+
>+#endif
>diff --git a/hw/core/shared-irq.c b/hw/core/shared-irq.c
>new file mode 100644
>index 0000000000..b2a4ea4a66
>--- /dev/null
>+++ b/hw/core/shared-irq.c
>@@ -0,0 +1,88 @@
>+/*
>+ * IRQ sharing device.
>+ *
>+ * Copyright (c) 2025 Bernhard Beschow <shentey@gmail.com>
>+ *
>+ * SPDX-License-Identifier: GPL-2.0-or-later
>+ */
>+
>+#include "qemu/osdep.h"
>+#include "hw/core/shared-irq.h"
>+#include "hw/irq.h"
>+#include "hw/qdev-properties.h"
>+#include "qapi/error.h"
>+#include "migration/vmstate.h"
>+
>+static void shared_irq_handler(void *opaque, int n, int level)
>+{
>+    SharedIRQ *s = opaque;
>+    uint16_t mask = BIT(n);
>+
>+    if (level) {
>+        s->irq_states |= mask;
>+    } else {
>+        s->irq_states &= ~mask;
>+    }
>+
>+    qemu_set_irq(s->out_irq, !!s->irq_states);
>+}
>+
>+static void shared_irq_init(Object *obj)
>+{
>+    SharedIRQ *s = SHARED_IRQ(obj);
>+
>+    qdev_init_gpio_out(DEVICE(s), &s->out_irq, 1);
>+}
>+
>+static void shared_irq_realize(DeviceState *dev, Error **errp)
>+{
>+    SharedIRQ *s = SHARED_IRQ(dev);
>+
>+    if (s->num_lines < 1 || s->num_lines >= MAX_SHARED_LINES) {
>+        error_setg(errp,
>+                   "IRQ shared number of lines %d must be between 1 and %d",
>+                   s->num_lines, MAX_SHARED_LINES);
>+        return;
>+    }
>+
>+    qdev_init_gpio_in(dev, shared_irq_handler, s->num_lines);
>+}
>+
>+static const Property shared_irq_properties[] = {
>+    DEFINE_PROP_UINT8("num-lines", SharedIRQ, num_lines, 1),
>+};
>+
>+static const VMStateDescription shared_irq_vmstate = {
>+    .name = TYPE_SHARED_IRQ,
>+    .version_id = 1,
>+    .minimum_version_id = 1,
>+    .fields = (const VMStateField[]) {
>+        VMSTATE_UINT16(irq_states, SharedIRQ),
>+        VMSTATE_END_OF_LIST()
>+    },
>+};
>+
>+static void shared_irq_class_init(ObjectClass *klass, void *data)
>+{
>+    DeviceClass *dc = DEVICE_CLASS(klass);
>+
>+    /* No state to reset */
>+    device_class_set_props(dc, shared_irq_properties);
>+    dc->vmsd = &shared_irq_vmstate;
>+    dc->realize = shared_irq_realize;
>+
>+    /* Reason: Needs to be wired up to work */
>+    dc->user_creatable = false;
>+}
>+
>+static const TypeInfo shared_irq_types[] = {
>+    {
>+       .name = TYPE_SHARED_IRQ,
>+       .parent = TYPE_DEVICE,
>+       .instance_size = sizeof(SharedIRQ),
>+       .instance_init = shared_irq_init,
>+       .class_init = shared_irq_class_init,
>+    },
>+};
>+
>+DEFINE_TYPES(shared_irq_types)
>diff --git a/hw/core/Kconfig b/hw/core/Kconfig
>index d1bdf765ee..ddff977963 100644
>--- a/hw/core/Kconfig
>+++ b/hw/core/Kconfig
>@@ -32,6 +32,9 @@ config PLATFORM_BUS
> config REGISTER
>     bool
> 
>+config SHARED_IRQ
>+    bool
>+
> config SPLIT_IRQ
>     bool
> 
>diff --git a/hw/core/meson.build b/hw/core/meson.build
>index ce9dfa3f4b..6b5bdc8ec7 100644
>--- a/hw/core/meson.build
>+++ b/hw/core/meson.build
>@@ -21,6 +21,7 @@ system_ss.add(when: 'CONFIG_OR_IRQ', if_true: files('or-irq.c'))
> system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('platform-bus.c'))
> system_ss.add(when: 'CONFIG_PTIMER', if_true: files('ptimer.c'))
> system_ss.add(when: 'CONFIG_REGISTER', if_true: files('register.c'))
>+system_ss.add(when: 'CONFIG_SHARED_IRQ', if_true: files('shared-irq.c'))
> system_ss.add(when: 'CONFIG_SPLIT_IRQ', if_true: files('split-irq.c'))
> system_ss.add(when: 'CONFIG_XILINX_AXI', if_true: files('stream.c'))
> system_ss.add(when: 'CONFIG_PLATFORM_BUS', if_true: files('sysbus-fdt.c'))

Re: [PATCH 04/14] hw/core: Introduce TYPE_SHARED_IRQ

[David Woodhouse]

[-- Attachment #1: Type: text/plain, Size: 1032 bytes --]

On Wed, 2025-01-08 at 14:26 +0000, Bernhard Beschow wrote:
> 
> 
> Am 8. Januar 2025 09:25:28 UTC schrieb Bernhard Beschow <shentey@gmail.com>:
> > Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> > ---
> > include/hw/core/shared-irq.h | 39 ++++++++++++++++
> > hw/core/shared-irq.c         | 88 ++++++++++++++++++++++++++++++++++++
> > hw/core/Kconfig              |  3 ++
> > hw/core/meson.build          |  1 +
> > 4 files changed, 131 insertions(+)
> > create mode 100644 include/hw/core/shared-irq.h
> > create mode 100644 hw/core/shared-irq.c
> > 
> > diff --git a/include/hw/core/shared-irq.h b/include/hw/core/shared-irq.h
> > new file mode 100644
> 
> As pointed out by David, this device is redundant to TYPE_OR_IRQ. I'll drop it in v2.

Of course, I'd much rather *fix* it to do a reverse callback up the
chain when the line is deasserted, to check whether any sources want it
to be asserted again. That's the only way we can do VFIO level
interrupts properly.

[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 5069 bytes --]

[PATCH 05/14] hw/pci-host/designware: Expose MSI IRQ

[Bernhard Beschow]

Fixes INTD and MSI interrupts poking the same IRQ line without keeping track of
each other's IRQ level. Furthermore, SoCs such as the i.MX 8M Plus don't share
the MSI IRQ with the INTx lines, so expose it as a dedicated pin.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/arm/fsl-imx6.h        |  4 +++-
 include/hw/arm/fsl-imx7.h        |  4 +++-
 include/hw/pci-host/designware.h |  1 +
 hw/arm/fsl-imx6.c                | 13 ++++++++++++-
 hw/arm/fsl-imx7.c                | 13 ++++++++++++-
 hw/pci-host/designware.c         |  7 +++----
 hw/arm/Kconfig                   |  2 ++
 7 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/include/hw/arm/fsl-imx6.h b/include/hw/arm/fsl-imx6.h
index 61c593ffd2..4395b2ae5e 100644
--- a/include/hw/arm/fsl-imx6.h
+++ b/include/hw/arm/fsl-imx6.h
@@ -23,6 +23,7 @@
 #include "hw/misc/imx7_snvs.h"
 #include "hw/watchdog/wdt_imx2.h"
 #include "hw/char/imx_serial.h"
+#include "hw/core/shared-irq.h"
 #include "hw/timer/imx_gpt.h"
 #include "hw/timer/imx_epit.h"
 #include "hw/i2c/imx_i2c.h"
@@ -73,6 +74,7 @@ struct FslIMX6State {
     ChipideaState      usb[FSL_IMX6_NUM_USBS];
     IMXFECState        eth;
     DesignwarePCIEHost pcie;
+    SharedIRQ          pcie4_msi_irq;
     MemoryRegion       rom;
     MemoryRegion       caam;
     MemoryRegion       ocram;
@@ -457,7 +459,7 @@ struct FslIMX6State {
 #define FSL_IMX6_PCIE1_IRQ 120
 #define FSL_IMX6_PCIE2_IRQ 121
 #define FSL_IMX6_PCIE3_IRQ 122
-#define FSL_IMX6_PCIE4_IRQ 123
+#define FSL_IMX6_PCIE4_MSI_IRQ 123
 #define FSL_IMX6_DCIC1_IRQ 124
 #define FSL_IMX6_DCIC2_IRQ 125
 #define FSL_IMX6_MLB150_HIGH_IRQ 126
diff --git a/include/hw/arm/fsl-imx7.h b/include/hw/arm/fsl-imx7.h
index 411fa1c2e3..2dccc4db67 100644
--- a/include/hw/arm/fsl-imx7.h
+++ b/include/hw/arm/fsl-imx7.h
@@ -28,6 +28,7 @@
 #include "hw/watchdog/wdt_imx2.h"
 #include "hw/gpio/imx_gpio.h"
 #include "hw/char/imx_serial.h"
+#include "hw/core/shared-irq.h"
 #include "hw/timer/imx_gpt.h"
 #include "hw/timer/imx_epit.h"
 #include "hw/i2c/imx_i2c.h"
@@ -85,6 +86,7 @@ struct FslIMX7State {
     IMX7GPRState       gpr;
     ChipideaState      usb[FSL_IMX7_NUM_USBS];
     DesignwarePCIEHost pcie;
+    SharedIRQ          pcie4_msi_irq;
     MemoryRegion       rom;
     MemoryRegion       caam;
     MemoryRegion       ocram;
@@ -428,7 +430,7 @@ enum FslIMX7IRQs {
     FSL_IMX7_PCI_INTA_IRQ = 125,
     FSL_IMX7_PCI_INTB_IRQ = 124,
     FSL_IMX7_PCI_INTC_IRQ = 123,
-    FSL_IMX7_PCI_INTD_IRQ = 122,
+    FSL_IMX7_PCI_INTD_MSI_IRQ = 122,
 
     FSL_IMX7_UART7_IRQ    = 126,
 
diff --git a/include/hw/pci-host/designware.h b/include/hw/pci-host/designware.h
index c484e377a8..bf8b278978 100644
--- a/include/hw/pci-host/designware.h
+++ b/include/hw/pci-host/designware.h
@@ -86,6 +86,7 @@ struct DesignwarePCIEHost {
         MemoryRegion io;
 
         qemu_irq     irqs[4];
+        qemu_irq     msi;
     } pci;
 
     MemoryRegion mmio;
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index ac8c66e242..d8c6685bac 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -106,6 +106,8 @@ static void fsl_imx6_init(Object *obj)
     object_initialize_child(obj, "eth", &s->eth, TYPE_IMX_ENET);
 
     object_initialize_child(obj, "pcie", &s->pcie, TYPE_DESIGNWARE_PCIE_HOST);
+    object_initialize_child(obj, "pcie4-msi-irq", &s->pcie4_msi_irq,
+                            TYPE_SHARED_IRQ);
 }
 
 static void fsl_imx6_realize(DeviceState *dev, Error **errp)
@@ -435,14 +437,23 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
     sysbus_realize(SYS_BUS_DEVICE(&s->pcie), &error_abort);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->pcie), 0, FSL_IMX6_PCIe_REG_ADDR);
 
+    object_property_set_int(OBJECT(&s->pcie4_msi_irq), "num-lines", 2,
+                            &error_abort);
+    qdev_realize(DEVICE(&s->pcie4_msi_irq), NULL, &error_abort);
+
+    irq = qdev_get_gpio_in(DEVICE(&s->a9mpcore), FSL_IMX6_PCIE4_MSI_IRQ);
+    qdev_connect_gpio_out(DEVICE(&s->pcie4_msi_irq), 0, irq);
+
     irq = qdev_get_gpio_in(DEVICE(&s->a9mpcore), FSL_IMX6_PCIE1_IRQ);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 0, irq);
     irq = qdev_get_gpio_in(DEVICE(&s->a9mpcore), FSL_IMX6_PCIE2_IRQ);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 1, irq);
     irq = qdev_get_gpio_in(DEVICE(&s->a9mpcore), FSL_IMX6_PCIE3_IRQ);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 2, irq);
-    irq = qdev_get_gpio_in(DEVICE(&s->a9mpcore), FSL_IMX6_PCIE4_IRQ);
+    irq = qdev_get_gpio_in(DEVICE(&s->pcie4_msi_irq), 0);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 3, irq);
+    irq = qdev_get_gpio_in(DEVICE(&s->pcie4_msi_irq), 1);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 4, irq);
 
     /*
      * PCIe PHY
diff --git a/hw/arm/fsl-imx7.c b/hw/arm/fsl-imx7.c
index 05e3389fbe..801f49c94a 100644
--- a/hw/arm/fsl-imx7.c
+++ b/hw/arm/fsl-imx7.c
@@ -150,6 +150,8 @@ static void fsl_imx7_init(Object *obj)
      * PCIE
      */
     object_initialize_child(obj, "pcie", &s->pcie, TYPE_DESIGNWARE_PCIE_HOST);
+    object_initialize_child(obj, "pcie4-msi-irq", &s->pcie4_msi_irq,
+                            TYPE_SHARED_IRQ);
 
     /*
      * USBs
@@ -597,14 +599,23 @@ static void fsl_imx7_realize(DeviceState *dev, Error **errp)
     sysbus_realize(SYS_BUS_DEVICE(&s->pcie), &error_abort);
     sysbus_mmio_map(SYS_BUS_DEVICE(&s->pcie), 0, FSL_IMX7_PCIE_REG_ADDR);
 
+    object_property_set_int(OBJECT(&s->pcie4_msi_irq), "num-lines", 2,
+                            &error_abort);
+    qdev_realize(DEVICE(&s->pcie4_msi_irq), NULL, &error_abort);
+
+    irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_PCI_INTD_MSI_IRQ);
+    qdev_connect_gpio_out(DEVICE(&s->pcie4_msi_irq), 0, irq);
+
     irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_PCI_INTA_IRQ);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 0, irq);
     irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_PCI_INTB_IRQ);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 1, irq);
     irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_PCI_INTC_IRQ);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 2, irq);
-    irq = qdev_get_gpio_in(DEVICE(&s->a7mpcore), FSL_IMX7_PCI_INTD_IRQ);
+    irq = qdev_get_gpio_in(DEVICE(&s->pcie4_msi_irq), 0);
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 3, irq);
+    irq = qdev_get_gpio_in(DEVICE(&s->pcie4_msi_irq), 1);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->pcie), 4, irq);
 
     /*
      * USBs
diff --git a/hw/pci-host/designware.c b/hw/pci-host/designware.c
index c3fc37b904..3e8c36e6a7 100644
--- a/hw/pci-host/designware.c
+++ b/hw/pci-host/designware.c
@@ -55,8 +55,6 @@
 #define DESIGNWARE_PCIE_ATU_DEVFN(x)               (((x) >> 16) & 0xff)
 #define DESIGNWARE_PCIE_ATU_UPPER_TARGET           0x91C
 
-#define DESIGNWARE_PCIE_IRQ_MSI                    3
-
 static DesignwarePCIEHost *
 designware_pcie_root_to_host(DesignwarePCIERoot *root)
 {
@@ -90,7 +88,7 @@ static void designware_pcie_root_msi_write(void *opaque, hwaddr addr,
     root->msi.intr[0].status |= BIT(val) & root->msi.intr[0].enable;
 
     if (root->msi.intr[0].status & ~root->msi.intr[0].mask) {
-        qemu_set_irq(host->pci.irqs[DESIGNWARE_PCIE_IRQ_MSI], 1);
+        qemu_set_irq(host->pci.msi, 1);
     }
 }
 
@@ -335,7 +333,7 @@ static void designware_pcie_root_config_write(PCIDevice *d, uint32_t address,
     case DESIGNWARE_PCIE_MSI_INTR0_STATUS:
         root->msi.intr[0].status ^= val;
         if (!root->msi.intr[0].status) {
-            qemu_set_irq(host->pci.irqs[DESIGNWARE_PCIE_IRQ_MSI], 0);
+            qemu_set_irq(host->pci.msi, 0);
         }
         break;
 
@@ -680,6 +678,7 @@ static void designware_pcie_host_realize(DeviceState *dev, Error **errp)
     for (i = 0; i < ARRAY_SIZE(s->pci.irqs); i++) {
         sysbus_init_irq(sbd, &s->pci.irqs[i]);
     }
+    sysbus_init_irq(sbd, &s->pci.msi);
 
     memory_region_init_io(&s->mmio,
                           OBJECT(s),
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index e779b5af95..0c9ccd850d 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -516,6 +516,7 @@ config FSL_IMX6
     select PL310  # cache controller
     select PCI_EXPRESS_DESIGNWARE
     select SDHCI
+    select SHARED_IRQ
 
 config ASPEED_SOC
     bool
@@ -573,6 +574,7 @@ config FSL_IMX7
     select WDT_IMX2
     select PCI_EXPRESS_DESIGNWARE
     select SDHCI
+    select SHARED_IRQ
     select UNIMP
 
 config ARM_SMMUV3
-- 
2.47.1

[PATCH 06/14] hw/gpio/imx_gpio: Don't clear input GPIO values upon reset

[Bernhard Beschow]

Input GPIO values such as a present SD card may get notified before the GPIO
controller itself gets reset. Claring the input values thus loses data. Assuming
that input GPIO events are only fired when the state changes, the input values
shouldn't be reset.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/gpio/imx_gpio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/gpio/imx_gpio.c b/hw/gpio/imx_gpio.c
index 898f80f8c8..67c47a7280 100644
--- a/hw/gpio/imx_gpio.c
+++ b/hw/gpio/imx_gpio.c
@@ -302,7 +302,6 @@ static void imx_gpio_reset(DeviceState *dev)
 
     s->dr       = 0;
     s->gdir     = 0;
-    s->psr      = 0;
     s->icr      = 0;
     s->imr      = 0;
     s->isr      = 0;
-- 
2.47.1

[PATCH 07/14] hw/sd/sd: Remove legacy sd_set_cb() in favor of GPIOs

[Bernhard Beschow]

Commit ce5dd27534b0 "hw/sd: Remove omap2_mmc device" removed the last user of
sd_set_cb(). Rework this functionality into GPIOs.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 include/hw/sd/sdcard_legacy.h |  1 -
 hw/sd/sd.c                    | 31 ++++++++++++++-----------------
 2 files changed, 14 insertions(+), 18 deletions(-)

diff --git a/include/hw/sd/sdcard_legacy.h b/include/hw/sd/sdcard_legacy.h
index 0dc3889555..a121232560 100644
--- a/include/hw/sd/sdcard_legacy.h
+++ b/include/hw/sd/sdcard_legacy.h
@@ -36,7 +36,6 @@ SDState *sd_init(BlockBackend *blk, bool is_spi);
 int sd_do_command(SDState *card, SDRequest *request, uint8_t *response);
 void sd_write_byte(SDState *card, uint8_t value);
 uint8_t sd_read_byte(SDState *card);
-void sd_set_cb(SDState *card, qemu_irq readonly, qemu_irq insert);
 
 /* sd_enable should not be used -- it is only used on the nseries boards,
  * where it is part of a broken implementation of the MMC card slot switch
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 0330d432fd..aa8d86e1af 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -820,6 +820,16 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
     return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
 }
 
+static bool sd_get_inserted(SDState *sd)
+{
+    return sd->blk && blk_is_inserted(sd->blk);
+}
+
+static bool sd_get_readonly(SDState *sd)
+{
+    return sd->wp_switch;
+}
+
 static void sd_reset(DeviceState *dev)
 {
     SDState *sd = SDMMC_COMMON(dev);
@@ -865,16 +875,9 @@ static void sd_reset(DeviceState *dev)
     sd->dat_lines = 0xf;
     sd->cmd_line = true;
     sd->multi_blk_cnt = 0;
-}
 
-static bool sd_get_inserted(SDState *sd)
-{
-    return sd->blk && blk_is_inserted(sd->blk);
-}
-
-static bool sd_get_readonly(SDState *sd)
-{
-    return sd->wp_switch;
+    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
+    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
 }
 
 static void sd_cardchange(void *opaque, bool load, Error **errp)
@@ -1034,14 +1037,6 @@ SDState *sd_init(BlockBackend *blk, bool is_spi)
     return sd;
 }
 
-void sd_set_cb(SDState *sd, qemu_irq readonly, qemu_irq insert)
-{
-    sd->readonly_cb = readonly;
-    sd->inserted_cb = insert;
-    qemu_set_irq(readonly, sd->blk ? !blk_is_writable(sd->blk) : 0);
-    qemu_set_irq(insert, sd->blk ? blk_is_inserted(sd->blk) : 0);
-}
-
 static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len)
 {
     trace_sdcard_read_block(addr, len);
@@ -2727,6 +2722,8 @@ static void sd_instance_init(Object *obj)
     sd->last_cmd_name = "UNSET";
     sd->enable = true;
     sd->ocr_power_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sd_ocr_powerup, sd);
+    qdev_init_gpio_out_named(DEVICE(sd), &sd->inserted_cb, "cd", 1);
+    qdev_init_gpio_out_named(DEVICE(sd), &sd->readonly_cb, "wp", 1);
 }
 
 static void sd_instance_finalize(Object *obj)
-- 
2.47.1

Re: [PATCH 07/14] hw/sd/sd: Remove legacy sd_set_cb() in favor of GPIOs

[Philippe Mathieu-Daudé]

On 8/1/25 10:25, Bernhard Beschow wrote:
> Commit ce5dd27534b0 "hw/sd: Remove omap2_mmc device" removed the last user of
> sd_set_cb(). Rework this functionality into GPIOs.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   include/hw/sd/sdcard_legacy.h |  1 -
>   hw/sd/sd.c                    | 31 ++++++++++++++-----------------
>   2 files changed, 14 insertions(+), 18 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/sd/sd.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index aa8d86e1af..a50e5c20c8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -181,6 +181,8 @@ struct SDState {
     qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
     bool enable;
+    bool readonly_active_low;
+    bool inserted_active_low;
     uint8_t dat_lines;
     bool cmd_line;
 };
@@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
     sd->cmd_line = true;
     sd->multi_blk_cnt = 0;
 
-    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
-    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
+    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
+    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
 }
 
 static void sd_cardchange(void *opaque, bool load, Error **errp)
@@ -896,9 +898,9 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
     }
 
     if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
-        qemu_set_irq(sd->inserted_cb, inserted);
+        qemu_set_irq(sd->inserted_cb, inserted ^ sd->inserted_active_low);
         if (inserted) {
-            qemu_set_irq(sd->readonly_cb, readonly);
+            qemu_set_irq(sd->readonly_cb, readonly ^ sd->readonly_active_low);
         }
     } else {
         sdbus = SD_BUS(qdev_get_parent_bus(dev));
@@ -2797,6 +2799,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
 
 static const Property sdmmc_common_properties[] = {
     DEFINE_PROP_DRIVE("drive", SDState, blk),
+    DEFINE_PROP_BOOL("cd-active-low", SDState, inserted_active_low, false),
+    DEFINE_PROP_BOOL("wp-active-low", SDState, readonly_active_low, false),
 };
 
 static const Property sd_properties[] = {
-- 
2.47.1

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

[Philippe Mathieu-Daudé]

Hi Bernhard,

On 8/1/25 10:25, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/sd/sd.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)


> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>       sd->cmd_line = true;
>       sd->multi_blk_cnt = 0;
>   
> -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);

Please embed in sd_get_readonly(),

> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);

and sd_get_inserted().

>   }
>   
>   static void sd_cardchange(void *opaque, bool load, Error **errp)
> @@ -896,9 +898,9 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
>       }
>   
>       if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
> -        qemu_set_irq(sd->inserted_cb, inserted);
> +        qemu_set_irq(sd->inserted_cb, inserted ^ sd->inserted_active_low);

Use sd_get_inserted(),

>           if (inserted) {
> -            qemu_set_irq(sd->readonly_cb, readonly);
> +            qemu_set_irq(sd->readonly_cb, readonly ^ sd->readonly_active_low);

and sd_get_readonly() here.

>           }
>       } else {
>           sdbus = SD_BUS(qdev_get_parent_bus(dev));
> @@ -2797,6 +2799,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
>   
>   static const Property sdmmc_common_properties[] = {
>       DEFINE_PROP_DRIVE("drive", SDState, blk),
> +    DEFINE_PROP_BOOL("cd-active-low", SDState, inserted_active_low, false),
> +    DEFINE_PROP_BOOL("wp-active-low", SDState, readonly_active_low, false),
>   };
With the requested changes:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

[Bernhard Beschow]

Am 9. Januar 2025 11:40:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Hi Bernhard,
>
>On 8/1/25 10:25, Bernhard Beschow wrote:
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/sd/sd.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>
>
>> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>>       sd->cmd_line = true;
>>       sd->multi_blk_cnt = 0;
>>   -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
>> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
>> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
>
>Please embed in sd_get_readonly(),
>
>> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
>
>and sd_get_inserted().

Are you sure? I deliberately implemented it as is because embedding would change the internal logic of the device as well as SDCardClass::{get_inserted, get_readonly}.

Best regards,
Bernhard

>
>>   }
>>     static void sd_cardchange(void *opaque, bool load, Error **errp)
>> @@ -896,9 +898,9 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
>>       }
>>         if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
>> -        qemu_set_irq(sd->inserted_cb, inserted);
>> +        qemu_set_irq(sd->inserted_cb, inserted ^ sd->inserted_active_low);
>
>Use sd_get_inserted(),
>
>>           if (inserted) {
>> -            qemu_set_irq(sd->readonly_cb, readonly);
>> +            qemu_set_irq(sd->readonly_cb, readonly ^ sd->readonly_active_low);
>
>and sd_get_readonly() here.
>
>>           }
>>       } else {
>>           sdbus = SD_BUS(qdev_get_parent_bus(dev));
>> @@ -2797,6 +2799,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
>>     static const Property sdmmc_common_properties[] = {
>>       DEFINE_PROP_DRIVE("drive", SDState, blk),
>> +    DEFINE_PROP_BOOL("cd-active-low", SDState, inserted_active_low, false),
>> +    DEFINE_PROP_BOOL("wp-active-low", SDState, readonly_active_low, false),
>>   };
>With the requested changes:
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

[Philippe Mathieu-Daudé]

On 9/1/25 17:20, Bernhard Beschow wrote:
> 
> 
> Am 9. Januar 2025 11:40:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> Hi Bernhard,
>>
>> On 8/1/25 10:25, Bernhard Beschow wrote:
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>    hw/sd/sd.c | 12 ++++++++----
>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>
>>
>>> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>>>        sd->cmd_line = true;
>>>        sd->multi_blk_cnt = 0;
>>>    -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
>>> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
>>> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
>>
>> Please embed in sd_get_readonly(),
>>
>>> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
>>
>> and sd_get_inserted().
> 
> Are you sure? I deliberately implemented it as is because embedding would change the internal logic of the device as well as SDCardClass::{get_inserted, get_readonly}.

Yes, this is why I requested that change. Why don't you think it is correct?

> 
> Best regards,
> Bernhard

>> With the requested changes:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

[Bernhard Beschow]

Am 12. Januar 2025 18:06:04 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 9/1/25 17:20, Bernhard Beschow wrote:
>> 
>> 
>> Am 9. Januar 2025 11:40:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>> Hi Bernhard,
>>> 
>>> On 8/1/25 10:25, Bernhard Beschow wrote:
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>    hw/sd/sd.c | 12 ++++++++----
>>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>> 
>>> 
>>>> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>>>>        sd->cmd_line = true;
>>>>        sd->multi_blk_cnt = 0;
>>>>    -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
>>>> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
>>>> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
>>> 
>>> Please embed in sd_get_readonly(),
>>> 
>>>> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
>>> 
>>> and sd_get_inserted().
>> 
>> Are you sure? I deliberately implemented it as is because embedding would change the internal logic of the device as well as SDCardClass::{get_inserted, get_readonly}.
>
>Yes, this is why I requested that change. Why don't you think it is correct?

I'm asking because I think that moving the xor inside the methods would break the device model.

The goal of the active_low attributes is to invert the polarity of the GPIOs only which allows to model boards where these are inverted. IOW they are intended to influence the wiring. That is in contrast to the two methods which determine the internal logic of the device model. They are also used as virtual method implementations of SDCardClass::{get_inserted, get_readonly} which determine the logic of the sd bus. Moving the xor inside inverts their return values if s->*_active_low are true, effectively flipping every if statement, which seems wrong to me. What do I miss?

Best regards,
Bernhard

>
>> 
>> Best regards,
>> Bernhard
>
>>> With the requested changes:
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> 
>

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

[Philippe Mathieu-Daudé]

On 17/1/25 00:20, Bernhard Beschow wrote:
> 
> 
> Am 12. Januar 2025 18:06:04 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> On 9/1/25 17:20, Bernhard Beschow wrote:
>>>
>>>
>>> Am 9. Januar 2025 11:40:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>>> Hi Bernhard,
>>>>
>>>> On 8/1/25 10:25, Bernhard Beschow wrote:
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>>     hw/sd/sd.c | 12 ++++++++----
>>>>>     1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>>
>>>>> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>>>>>         sd->cmd_line = true;
>>>>>         sd->multi_blk_cnt = 0;
>>>>>     -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
>>>>> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
>>>>> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
>>>>
>>>> Please embed in sd_get_readonly(),
>>>>
>>>>> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
>>>>
>>>> and sd_get_inserted().
>>>
>>> Are you sure? I deliberately implemented it as is because embedding would change the internal logic of the device as well as SDCardClass::{get_inserted, get_readonly}.
>>
>> Yes, this is why I requested that change. Why don't you think it is correct?
> 
> I'm asking because I think that moving the xor inside the methods would break the device model.
> 
> The goal of the active_low attributes is to invert the polarity of the GPIOs only which allows to model boards where these are inverted. IOW they are intended to influence the wiring. That is in contrast to the two methods which determine the internal logic of the device model. They are also used as virtual method implementations of SDCardClass::{get_inserted, get_readonly} which determine the logic of the sd bus. Moving the xor inside inverts their return values if s->*_active_low are true, effectively flipping every if statement, which seems wrong to me. What do I miss?

Right... Then maybe we should model polarity in qemu_irq.

Patches 7 & 8 queued, thanks!

[PATCH 09/14] hw/char/imx_serial: Turn some DPRINTF() statements into trace events

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/char/imx_serial.c | 58 +++++++++++++++++++++++++++++---------------
 hw/char/trace-events |  5 ++++
 2 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
index be06f39a4d..38b4865157 100644
--- a/hw/char/imx_serial.c
+++ b/hw/char/imx_serial.c
@@ -27,6 +27,7 @@
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "qemu/fifo32.h"
+#include "trace.h"
 
 #ifndef DEBUG_IMX_UART
 #define DEBUG_IMX_UART 0
@@ -185,10 +186,10 @@ static uint64_t imx_serial_read(void *opaque, hwaddr offset,
                                 unsigned size)
 {
     IMXSerialState *s = (IMXSerialState *)opaque;
+    Chardev *chr = qemu_chr_fe_get_driver(&s->chr);
     uint32_t c, rx_used;
     uint8_t rxtl = s->ufcr & TL_MASK;
-
-    DPRINTF("read(offset=0x%" HWADDR_PRIx ")\n", offset);
+    uint64_t value;
 
     switch (offset >> 2) {
     case 0x0: /* URXD */
@@ -209,49 +210,67 @@ static uint64_t imx_serial_read(void *opaque, hwaddr offset,
             imx_serial_rx_fifo_ageing_timer_restart(s);
             qemu_chr_fe_accept_input(&s->chr);
         }
-        return c;
+        value = c;
+        break;
 
     case 0x20: /* UCR1 */
-        return s->ucr1;
+        value = s->ucr1;
+        break;
 
     case 0x21: /* UCR2 */
-        return s->ucr2;
+        value = s->ucr2;
+        break;
 
     case 0x25: /* USR1 */
-        return s->usr1;
+        value = s->usr1;
+        break;
 
     case 0x26: /* USR2 */
-        return s->usr2;
+        value = s->usr2;
+        break;
 
     case 0x2A: /* BRM Modulator */
-        return s->ubmr;
+        value = s->ubmr;
+        break;
 
     case 0x2B: /* Baud Rate Count */
-        return s->ubrc;
+        value = s->ubrc;
+        break;
 
     case 0x2d: /* Test register */
-        return s->uts1;
+        value = s->uts1;
+        break;
 
     case 0x24: /* UFCR */
-        return s->ufcr;
+        value = s->ufcr;
+        break;
 
     case 0x2c:
-        return s->onems;
+        value = s->onems;
+        break;
 
     case 0x22: /* UCR3 */
-        return s->ucr3;
+        value = s->ucr3;
+        break;
 
     case 0x23: /* UCR4 */
-        return s->ucr4;
+        value = s->ucr4;
+        break;
 
     case 0x29: /* BRM Incremental */
-        return 0x0; /* TODO */
+        value = 0x0; /* TODO */
+        break;
 
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
                       HWADDR_PRIx "\n", TYPE_IMX_SERIAL, __func__, offset);
-        return 0;
+        value = 0;
+        break;
     }
+
+    trace_imx_serial_read(chr ? chr->label : "NODEV", offset, value);
+
+    return value;
 }
 
 static void imx_serial_write(void *opaque, hwaddr offset,
@@ -261,8 +280,7 @@ static void imx_serial_write(void *opaque, hwaddr offset,
     Chardev *chr = qemu_chr_fe_get_driver(&s->chr);
     unsigned char ch;
 
-    DPRINTF("write(offset=0x%" HWADDR_PRIx ", value = 0x%x) to %s\n",
-            offset, (unsigned int)value, chr ? chr->label : "NODEV");
+    trace_imx_serial_write(chr ? chr->label : "NODEV", offset, value);
 
     switch (offset >> 2) {
     case 0x10: /* UTXD */
@@ -374,9 +392,11 @@ static int imx_can_receive(void *opaque)
 static void imx_put_data(void *opaque, uint32_t value)
 {
     IMXSerialState *s = (IMXSerialState *)opaque;
+    Chardev *chr = qemu_chr_fe_get_driver(&s->chr);
     uint8_t rxtl = s->ufcr & TL_MASK;
 
-    DPRINTF("received char\n");
+    trace_imx_serial_put_data(chr ? chr->label : "NODEV", value);
+
     imx_serial_rx_fifo_push(s, value);
     if (fifo32_num_used(&s->rx_fifo) >= rxtl) {
         s->usr1 |= USR1_RRDY;
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 59e1f734a7..4c6d401b4b 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -52,6 +52,11 @@ escc_sunkbd_event_out(int ch) "Translated keycode 0x%2.2x"
 escc_kbd_command(int val) "Command %d"
 escc_sunmouse_event(int dx, int dy, int buttons_state) "dx=%d dy=%d buttons=0x%01x"
 
+# imx_serial.c
+imx_serial_read(const char *chrname, uint64_t addr, uint64_t value) "%s:[0x%03" PRIu64 "] -> 0x%08" PRIu64
+imx_serial_write(const char *chrname, uint64_t addr, uint64_t value) "%s:[0x%03" PRIu64 "] <- 0x%08" PRIu64
+imx_serial_put_data(const char *chrname, uint32_t value) "%s: 0x%" PRIu32
+
 # pl011.c
 pl011_irq_state(int level) "irq state %d"
 pl011_read(uint32_t addr, uint32_t value, const char *regname) "addr 0x%03x value 0x%08x reg %s"
-- 
2.47.1

Re: [PATCH 09/14] hw/char/imx_serial: Turn some DPRINTF() statements into trace events

[Philippe Mathieu-Daudé]

On 8/1/25 10:25, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/char/imx_serial.c | 58 +++++++++++++++++++++++++++++---------------
>   hw/char/trace-events |  5 ++++
>   2 files changed, 44 insertions(+), 19 deletions(-)


> diff --git a/hw/char/trace-events b/hw/char/trace-events
> index 59e1f734a7..4c6d401b4b 100644
> --- a/hw/char/trace-events
> +++ b/hw/char/trace-events
> @@ -52,6 +52,11 @@ escc_sunkbd_event_out(int ch) "Translated keycode 0x%2.2x"
>   escc_kbd_command(int val) "Command %d"
>   escc_sunmouse_event(int dx, int dy, int buttons_state) "dx=%d dy=%d buttons=0x%01x"
>   
> +# imx_serial.c
> +imx_serial_read(const char *chrname, uint64_t addr, uint64_t value) "%s:[0x%03" PRIu64 "] -> 0x%08" PRIu64
> +imx_serial_write(const char *chrname, uint64_t addr, uint64_t value) "%s:[0x%03" PRIu64 "] <- 0x%08" PRIu64
> +imx_serial_put_data(const char *chrname, uint32_t value) "%s: 0x%" PRIu32

0x%08" PRIx64 and 0x%" PRIx32.

With that changed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 10/14] hw/timer/imx_gpt: Remove unused define

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/timer/imx_gpt.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/timer/imx_gpt.c b/hw/timer/imx_gpt.c
index 2663a9d9ef..11eca9fa4d 100644
--- a/hw/timer/imx_gpt.c
+++ b/hw/timer/imx_gpt.c
@@ -20,10 +20,6 @@
 #include "qemu/log.h"
 #include "trace.h"
 
-#ifndef DEBUG_IMX_GPT
-#define DEBUG_IMX_GPT 0
-#endif
-
 static const char *imx_gpt_reg_name(uint32_t reg)
 {
     switch (reg) {
-- 
2.47.1

Re: [PATCH 10/14] hw/timer/imx_gpt: Remove unused define

[Philippe Mathieu-Daudé]

On 8/1/25 10:25, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/timer/imx_gpt.c | 4 ----
>   1 file changed, 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 11/14] tests/qtest/libqos: Reuse TYPE_IMX_I2C define

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 tests/qtest/libqos/arm-imx25-pdk-machine.c | 5 +++--
 tests/qtest/libqos/i2c-imx.c               | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/tests/qtest/libqos/arm-imx25-pdk-machine.c b/tests/qtest/libqos/arm-imx25-pdk-machine.c
index 8fe128fae8..2d8b754343 100644
--- a/tests/qtest/libqos/arm-imx25-pdk-machine.c
+++ b/tests/qtest/libqos/arm-imx25-pdk-machine.c
@@ -23,6 +23,7 @@
 #include "libqos-malloc.h"
 #include "qgraph.h"
 #include "i2c.h"
+#include "hw/i2c/imx_i2c.h"
 
 #define ARM_PAGE_SIZE            4096
 #define IMX25_PDK_RAM_START      0x80000000
@@ -50,7 +51,7 @@ static void *imx25_pdk_get_driver(void *object, const char *interface)
 static QOSGraphObject *imx25_pdk_get_device(void *obj, const char *device)
 {
     QIMX25PDKMachine *machine = obj;
-    if (!g_strcmp0(device, "imx.i2c")) {
+    if (!g_strcmp0(device, TYPE_IMX_I2C)) {
         return &machine->i2c_1.obj;
     }
 
@@ -86,7 +87,7 @@ static void imx25_pdk_register_nodes(void)
         .extra_device_opts = "bus=i2c-bus.0"
     };
     qos_node_create_machine("arm/imx25-pdk", qos_create_machine_arm_imx25_pdk);
-    qos_node_contains("arm/imx25-pdk", "imx.i2c", &edge, NULL);
+    qos_node_contains("arm/imx25-pdk", TYPE_IMX_I2C, &edge, NULL);
 }
 
 libqos_init(imx25_pdk_register_nodes);
diff --git a/tests/qtest/libqos/i2c-imx.c b/tests/qtest/libqos/i2c-imx.c
index 710cb926d6..6d868e4cc4 100644
--- a/tests/qtest/libqos/i2c-imx.c
+++ b/tests/qtest/libqos/i2c-imx.c
@@ -209,8 +209,8 @@ void imx_i2c_init(IMXI2C *s, QTestState *qts, uint64_t addr)
 
 static void imx_i2c_register_nodes(void)
 {
-    qos_node_create_driver("imx.i2c", NULL);
-    qos_node_produces("imx.i2c", "i2c-bus");
+    qos_node_create_driver(TYPE_IMX_I2C, NULL);
+    qos_node_produces(TYPE_IMX_I2C, "i2c-bus");
 }
 
 libqos_init(imx_i2c_register_nodes);
-- 
2.47.1

Re: [PATCH 11/14] tests/qtest/libqos: Reuse TYPE_IMX_I2C define

[Philippe Mathieu-Daudé]

On 8/1/25 10:25, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   tests/qtest/libqos/arm-imx25-pdk-machine.c | 5 +++--
>   tests/qtest/libqos/i2c-imx.c               | 4 ++--
>   2 files changed, 5 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 11/14] tests/qtest/libqos: Reuse TYPE_IMX_I2C define

[Fabiano Rosas]

Bernhard Beschow <shentey@gmail.com> writes:

> Signed-off-by: Bernhard Beschow <shentey@gmail.com>

Reviewed-by: Fabiano Rosas <farosas@suse.de>

[PATCH 12/14] hw/i2c/imx_i2c: Convert DPRINTF() to trace events

[Bernhard Beschow]

Also print the MMIO address when tracing. This allows to distinguishing the
many instances a typical i.MX SoC has.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/i2c/imx_i2c.c    | 21 +++++----------------
 hw/i2c/trace-events |  5 +++++
 2 files changed, 10 insertions(+), 16 deletions(-)

diff --git a/hw/i2c/imx_i2c.c b/hw/i2c/imx_i2c.c
index c565fd5b8a..be1688c064 100644
--- a/hw/i2c/imx_i2c.c
+++ b/hw/i2c/imx_i2c.c
@@ -25,18 +25,7 @@
 #include "hw/i2c/i2c.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
-
-#ifndef DEBUG_IMX_I2C
-#define DEBUG_IMX_I2C 0
-#endif
-
-#define DPRINTF(fmt, args...) \
-    do { \
-        if (DEBUG_IMX_I2C) { \
-            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_I2C, \
-                                             __func__, ##args); \
-        } \
-    } while (0)
+#include "trace.h"
 
 static const char *imx_i2c_get_regname(unsigned offset)
 {
@@ -152,8 +141,8 @@ static uint64_t imx_i2c_read(void *opaque, hwaddr offset,
         break;
     }
 
-    DPRINTF("read %s [0x%" HWADDR_PRIx "] -> 0x%02x\n",
-            imx_i2c_get_regname(offset), offset, value);
+    trace_imx_i2c_read(s->iomem.addr, imx_i2c_get_regname(offset), offset,
+                       value);
 
     return (uint64_t)value;
 }
@@ -163,8 +152,8 @@ static void imx_i2c_write(void *opaque, hwaddr offset,
 {
     IMXI2CState *s = IMX_I2C(opaque);
 
-    DPRINTF("write %s [0x%" HWADDR_PRIx "] <- 0x%02x\n",
-            imx_i2c_get_regname(offset), offset, (int)value);
+    trace_imx_i2c_read(s->iomem.addr, imx_i2c_get_regname(offset), offset,
+                       value);
 
     value &= 0xff;
 
diff --git a/hw/i2c/trace-events b/hw/i2c/trace-events
index f708a7ace1..c6cba1ecf6 100644
--- a/hw/i2c/trace-events
+++ b/hw/i2c/trace-events
@@ -56,3 +56,8 @@ npcm7xx_smbus_recv_fifo(const char *id, uint8_t received, uint8_t expected) "%s
 
 pca954x_write_bytes(uint8_t value) "PCA954X write data: 0x%02x"
 pca954x_read_data(uint8_t value) "PCA954X read data: 0x%02x"
+
+# imx_i2c.c
+
+imx_i2c_read(uint64_t addr, const char *reg, uint64_t ofs, uint64_t value) "0x%" PRIx64 ":[%s (0x%" PRIx64 ")] -> 0x%02" PRIx64
+imx_i2c_write(uint64_t addr, const char *reg, uint64_t ofs, uint64_t value) "0x%" PRIx64 ":[%s (0x%" PRIx64 ")] <- 0x%02" PRIx64
-- 
2.47.1

Re: [PATCH 12/14] hw/i2c/imx_i2c: Convert DPRINTF() to trace events

[Philippe Mathieu-Daudé]

On 8/1/25 10:25, Bernhard Beschow wrote:
> Also print the MMIO address when tracing. This allows to distinguishing the
> many instances a typical i.MX SoC has.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/i2c/imx_i2c.c    | 21 +++++----------------
>   hw/i2c/trace-events |  5 +++++
>   2 files changed, 10 insertions(+), 16 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 12/14] hw/i2c/imx_i2c: Convert DPRINTF() to trace events

[Philippe Mathieu-Daudé]

On 9/1/25 12:43, Philippe Mathieu-Daudé wrote:
> On 8/1/25 10:25, Bernhard Beschow wrote:
>> Also print the MMIO address when tracing. This allows to 
>> distinguishing the
>> many instances a typical i.MX SoC has.

I'm not a fan of using peripheral address access, because it
can change i.e. when a vCPU is accessing it from secure or
non-secure mode.

I'd rather use an 'id', a 'name' or even the QOM (canonical?)
path.

Maybe we should directly cache that as Device::qom_path, so
all devices can use it for tracing, and we don't need to set
an id/name property when creating the device...

>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/i2c/imx_i2c.c    | 21 +++++----------------
>>   hw/i2c/trace-events |  5 +++++
>>   2 files changed, 10 insertions(+), 16 deletions(-)
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 12/14] hw/i2c/imx_i2c: Convert DPRINTF() to trace events

[Philippe Mathieu-Daudé]

On 9/1/25 12:56, Philippe Mathieu-Daudé wrote:
> On 9/1/25 12:43, Philippe Mathieu-Daudé wrote:
>> On 8/1/25 10:25, Bernhard Beschow wrote:
>>> Also print the MMIO address when tracing. This allows to 
>>> distinguishing the
>>> many instances a typical i.MX SoC has.
> 
> I'm not a fan of using peripheral address access, because it
> can change i.e. when a vCPU is accessing it from secure or
> non-secure mode.
> 
> I'd rather use an 'id', a 'name' or even the QOM (canonical?)
> path.
> 
> Maybe we should directly cache that as Device::qom_path, so
> all devices can use it for tracing, and we don't need to set
> an id/name property when creating the device...

We already have that, it is Device::canonical_path :)

> 
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>   hw/i2c/imx_i2c.c    | 21 +++++----------------
>>>   hw/i2c/trace-events |  5 +++++
>>>   2 files changed, 10 insertions(+), 16 deletions(-)
>>
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

Re: [PATCH 12/14] hw/i2c/imx_i2c: Convert DPRINTF() to trace events

[Bernhard Beschow]

Am 9. Januar 2025 12:38:11 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 9/1/25 12:56, Philippe Mathieu-Daudé wrote:
>> On 9/1/25 12:43, Philippe Mathieu-Daudé wrote:
>>> On 8/1/25 10:25, Bernhard Beschow wrote:
>>>> Also print the MMIO address when tracing. This allows to distinguishing the
>>>> many instances a typical i.MX SoC has.
>> 
>> I'm not a fan of using peripheral address access, because it
>> can change i.e. when a vCPU is accessing it from secure or
>> non-secure mode.
>> 
>> I'd rather use an 'id', a 'name' or even the QOM (canonical?)
>> path.
>> 
>> Maybe we should directly cache that as Device::qom_path, so
>> all devices can use it for tracing, and we don't need to set
>> an id/name property when creating the device...
>
>We already have that, it is Device::canonical_path :)

Will do!

>
>> 
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>   hw/i2c/imx_i2c.c    | 21 +++++----------------
>>>>   hw/i2c/trace-events |  5 +++++
>>>>   2 files changed, 10 insertions(+), 16 deletions(-)
>>> 
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> 
>

[PATCH 13/14] hw/misc/imx6_src: Convert DPRINTF() to trace events

[Bernhard Beschow]

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/misc/imx6_src.c   | 23 +++++------------------
 hw/misc/trace-events |  6 ++++++
 2 files changed, 11 insertions(+), 18 deletions(-)

diff --git a/hw/misc/imx6_src.c b/hw/misc/imx6_src.c
index dc6a2b92ba..06cc46292e 100644
--- a/hw/misc/imx6_src.c
+++ b/hw/misc/imx6_src.c
@@ -17,18 +17,7 @@
 #include "qemu/module.h"
 #include "target/arm/arm-powerctl.h"
 #include "hw/core/cpu.h"
-
-#ifndef DEBUG_IMX6_SRC
-#define DEBUG_IMX6_SRC 0
-#endif
-
-#define DPRINTF(fmt, args...) \
-    do { \
-        if (DEBUG_IMX6_SRC) { \
-            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX6_SRC, \
-                                             __func__, ##args); \
-        } \
-    } while (0)
+#include "trace.h"
 
 static const char *imx6_src_reg_name(uint32_t reg)
 {
@@ -87,7 +76,7 @@ static void imx6_src_reset(DeviceState *dev)
 {
     IMX6SRCState *s = IMX6_SRC(dev);
 
-    DPRINTF("\n");
+    trace_imx6_src_reset();
 
     memset(s->regs, 0, sizeof(s->regs));
 
@@ -111,7 +100,7 @@ static uint64_t imx6_src_read(void *opaque, hwaddr offset, unsigned size)
 
     }
 
-    DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx6_src_reg_name(index), value);
+    trace_imx6_src_read(imx6_src_reg_name(index), value);
 
     return value;
 }
@@ -134,8 +123,7 @@ static void imx6_clear_reset_bit(CPUState *cpu, run_on_cpu_data data)
     assert(bql_locked());
 
     s->regs[SRC_SCR] = deposit32(s->regs[SRC_SCR], ri->reset_bit, 1, 0);
-    DPRINTF("reg[%s] <= 0x%" PRIx32 "\n",
-            imx6_src_reg_name(SRC_SCR), s->regs[SRC_SCR]);
+    trace_imx6_clear_reset_bit(imx6_src_reg_name(SRC_SCR), s->regs[SRC_SCR]);
 
     g_free(ri);
 }
@@ -173,8 +161,7 @@ static void imx6_src_write(void *opaque, hwaddr offset, uint64_t value,
         return;
     }
 
-    DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx6_src_reg_name(index),
-            (uint32_t)current_value);
+    trace_imx6_src_write(imx6_src_reg_name(index), value);
 
     change_mask = s->regs[index] ^ (uint32_t)current_value;
 
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index 0f5d2b5666..cf1abe6928 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -253,6 +253,12 @@ ccm_clock_freq(uint32_t clock, uint32_t freq) "(Clock = %d) = %d"
 ccm_read_reg(const char *reg_name, uint32_t value) "reg[%s] <= 0x%" PRIx32
 ccm_write_reg(const char *reg_name, uint32_t value) "reg[%s] => 0x%" PRIx32
 
+# imx6_src.c
+imx6_src_read(const char *reg_name, uint32_t value) "reg[%s] => 0x%" PRIx32
+imx6_src_write(const char *reg_name, uint64_t value) "reg[%s] <= 0x%" PRIx64
+imx6_clear_reset_bit(const char *reg_name, uint32_t value) "reg[%s] <= 0x%" PRIx32
+imx6_src_reset(void) ""
+
 # imx7_src.c
 imx7_src_read(const char *reg_name, uint32_t value) "reg[%s] => 0x%" PRIx32
 imx7_src_write(const char *reg_name, uint32_t value) "reg[%s] <= 0x%" PRIx32
-- 
2.47.1

Re: [PATCH 13/14] hw/misc/imx6_src: Convert DPRINTF() to trace events

[Philippe Mathieu-Daudé]

On 8/1/25 10:25, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/misc/imx6_src.c   | 23 +++++------------------
>   hw/misc/trace-events |  6 ++++++
>   2 files changed, 11 insertions(+), 18 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

[PATCH 14/14] hw/gpio/imx_gpio: Turn DPRINTF() into trace events

[Bernhard Beschow]

While at it add a trace event for input GPIO events.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/gpio/imx_gpio.c   | 16 +++++-----------
 hw/gpio/trace-events |  5 +++++
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/hw/gpio/imx_gpio.c b/hw/gpio/imx_gpio.c
index 67c47a7280..f77132fe04 100644
--- a/hw/gpio/imx_gpio.c
+++ b/hw/gpio/imx_gpio.c
@@ -24,6 +24,7 @@
 #include "migration/vmstate.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "trace.h"
 
 #ifndef DEBUG_IMX_GPIO
 #define DEBUG_IMX_GPIO 0
@@ -34,14 +35,6 @@ typedef enum IMXGPIOLevel {
     IMX_GPIO_LEVEL_HIGH = 1,
 } IMXGPIOLevel;
 
-#define DPRINTF(fmt, args...) \
-    do { \
-        if (DEBUG_IMX_GPIO) { \
-            fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX_GPIO, \
-                                             __func__, ##args); \
-        } \
-    } while (0)
-
 static const char *imx_gpio_reg_name(uint32_t reg)
 {
     switch (reg) {
@@ -111,6 +104,8 @@ static void imx_gpio_set(void *opaque, int line, int level)
     IMXGPIOState *s = IMX_GPIO(opaque);
     IMXGPIOLevel imx_level = level ? IMX_GPIO_LEVEL_HIGH : IMX_GPIO_LEVEL_LOW;
 
+    trace_imx_gpio_set(s->iomem.addr, line, imx_level);
+
     imx_gpio_set_int_line(s, line, imx_level);
 
     /* this is an input signal, so set PSR */
@@ -200,7 +195,7 @@ static uint64_t imx_gpio_read(void *opaque, hwaddr offset, unsigned size)
         break;
     }
 
-    DPRINTF("(%s) = 0x%" PRIx32 "\n", imx_gpio_reg_name(offset), reg_value);
+    trace_imx_gpio_read(s->iomem.addr, imx_gpio_reg_name(offset), reg_value);
 
     return reg_value;
 }
@@ -210,8 +205,7 @@ static void imx_gpio_write(void *opaque, hwaddr offset, uint64_t value,
 {
     IMXGPIOState *s = IMX_GPIO(opaque);
 
-    DPRINTF("(%s, value = 0x%" PRIx32 ")\n", imx_gpio_reg_name(offset),
-            (uint32_t)value);
+    trace_imx_gpio_write(s->iomem.addr, imx_gpio_reg_name(offset), value);
 
     switch (offset) {
     case DR_ADDR:
diff --git a/hw/gpio/trace-events b/hw/gpio/trace-events
index b91cc7e9a4..9ddacc12e3 100644
--- a/hw/gpio/trace-events
+++ b/hw/gpio/trace-events
@@ -1,5 +1,10 @@
 # See docs/devel/tracing.rst for syntax documentation.
 
+# imx_gpio.c
+imx_gpio_read(uint64_t base, const char *reg, uint32_t value) "0x%" PRIx64 ":[%s] -> 0x%" PRIx32
+imx_gpio_write(uint64_t base, const char *reg, uint32_t value) "0x%" PRIx64 ":[%s] <- 0x%" PRIx32
+imx_gpio_set(uint64_t base, int line, int level) "0x%" PRIx64 ":[%d] <- %d"
+
 # npcm7xx_gpio.c
 npcm7xx_gpio_read(const char *id, uint64_t offset, uint64_t value) " %s offset: 0x%04" PRIx64 " value 0x%08" PRIx64
 npcm7xx_gpio_write(const char *id, uint64_t offset, uint64_t value) "%s offset: 0x%04" PRIx64 " value 0x%08" PRIx64
-- 
2.47.1

Re: [PATCH 14/14] hw/gpio/imx_gpio: Turn DPRINTF() into trace events

[Philippe Mathieu-Daudé]

On 8/1/25 10:25, Bernhard Beschow wrote:
> While at it add a trace event for input GPIO events.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/gpio/imx_gpio.c   | 16 +++++-----------
>   hw/gpio/trace-events |  5 +++++
>   2 files changed, 10 insertions(+), 11 deletions(-)


> @@ -210,8 +205,7 @@ static void imx_gpio_write(void *opaque, hwaddr offset, uint64_t value,
>   {
>       IMXGPIOState *s = IMX_GPIO(opaque);
>   
> -    DPRINTF("(%s, value = 0x%" PRIx32 ")\n", imx_gpio_reg_name(offset),
> -            (uint32_t)value);
> +    trace_imx_gpio_write(s->iomem.addr, imx_gpio_reg_name(offset), value);

Similar comment from patch 12 (iomem.addr -> qom_path?), otherwise:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>