[PATCH QEMU v2 0/5] Add a GPIO backend

[PATCH QEMU v2 0/5] Add a GPIO backend

[Geert Uytterhoeven]

	Hi all,

This patch series adds a GPIO controller backend, to connect virtual
GPIOs on the guest to physical GPIOs on the host, and enables support
for this using user-creatable PL061 GPIO controller instances.  This
allows the guest to control any external device connected to the
physical GPIOs.

While this can be used with an upstream Linux kernel (e.g. using a
dedicated GPIO controller connected to an external bus), proper
isolation and assignment of GPIOs to virtual machines depends on the
GPIO Aggregator[1], which has not been accepted in Linux upstream yet.
Aggregating GPIOs and exposing them as a new gpiochip was suggested in
response to my proof-of-concept for GPIO virtualization with QEMU[2][3].

Features and limitations:
  - The backend uses libgpiod on Linux,
  - For now only GPIO outputs are supported,
  - The number of GPIO lines mapped is limited to the number of GPIO
    lines available on the virtual GPIO controller (i.e. 8 on PL061).

Future work:
  - GPIO inputs,
  - GPIO line configuration,
  - Optimizations for controlling multiple GPIO lines at once,
  - ...

This series contains 5 patches:
  - The first two patches refactor the existing code for reuse,
  - The third patch adds a gneric GPIO backend using libgpiod,
  - The fourth patch adds gpiodev support to the PL061 driver,
  - The fifth patch adds dynamic PL061 support to ARM virt.

Changes compared to v1[2]:
  - Drop vgpios and gpios parameters, and map the full gpiochip instead,
  - Replace the single hardcoded PL061 instance (created by ARM virt) by
    multiple dynamically created instances, one per imported GPIO
    controller.

For testing, I have pushed this series to the topic/gpio-backend-v2
branch of my git repository at https://github.com/geertu/qemu.git.

Sample session on the Renesas Salvator-XS development board:

  - Unbind keyboard (shared with LEDs) from gpio-keys driver:

        host$ echo keys > /sys/bus/platform/drivers/gpio-keys/unbind

  - Aggregate GPIO lines connected to LEDs into a new virtual GPIO chip:

        host$ echo e6055400.gpio 11-13 \
              > /sys/bus/platform/drivers/gpio-aggregator/new_device

        gpio-aggregator gpio-aggregator.0: 0 => gpio-371
        gpio-aggregator gpio-aggregator.0: 1 => gpio-372
        gpio-aggregator gpio-aggregator.0: 2 => gpio-373
        gpiochip_find_base: found new base at 343
        gpio gpiochip10: (gpio-aggregator.0): added GPIO chardev (254:10)
        gpiochip_setup_dev: registered GPIOs 343 to 345 on device: gpiochip10 (gpio-aggregator.0)

  - Adjust permissions on /dev/gpiochip10 (optional)

  - Launch QEMU:

        host$ aarch64-softmmu/qemu-system-aarch64 -enable-kvm -M virt \
              -cpu cortex-a57 -m 1024 -nographic -kernel /path/to/Image \
              -device pl061,host=gpio-aggregator.0

        ...
        pl061_gpio c000000.gpio: PL061 GPIO chip registered
        ...

  - Control LEDs:

        guest$ gpioset gpiochip0 0=0 1=1 # LED4 OFF, LED5 ON
        guest$ gpioset gpiochip0 0=1 1=0 # LED4 ON, LED5 OFF

Thanks for your comments!

[1] "[PATCH v6 0/8] gpio: Add GPIO Aggregator"
    (https://lore.kernel.org/linux-gpio/20200324135328.5796-1-geert+renesas@glider.be/)
[2] "[PATCH QEMU POC] Add a GPIO backend"
    (https://lore.kernel.org/linux-gpio/20181003152521.23144-1-geert+renesas@glider.be/)
[3] "Getting To Blinky: Virt Edition / Making device pass-through work
     on embedded ARM"
    (https://fosdem.org/2019/schedule/event/vai_getting_to_blinky/)

Geert Uytterhoeven (5):
  ARM: PL061: Move TYPE_PL061 to hw/gpio/pl061.h
  ARM: PL061: Extract pl061_create_fdt()
  Add a GPIO backend using libgpiod
  ARM: PL061: Add gpiodev support
  hw/arm/virt: Add dynamic PL061 GPIO support

 MAINTAINERS              |  7 +++
 backends/Makefile.objs   |  2 +
 backends/gpiodev.c       | 94 ++++++++++++++++++++++++++++++++++++++++
 configure                | 28 ++++++++++++
 hw/arm/sysbus-fdt.c      | 18 ++++++++
 hw/arm/virt.c            | 21 ++-------
 hw/gpio/pl061.c          | 79 ++++++++++++++++++++++++++++++++-
 include/hw/gpio/pl061.h  | 23 ++++++++++
 include/sysemu/gpiodev.h | 12 +++++
 qemu-options.hx          |  9 ++++
 10 files changed, 275 insertions(+), 18 deletions(-)
 create mode 100644 backends/gpiodev.c
 create mode 100644 include/hw/gpio/pl061.h
 create mode 100644 include/sysemu/gpiodev.h

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH QEMU v2 1/5] ARM: PL061: Move TYPE_PL061 to hw/gpio/pl061.h

[Geert Uytterhoeven]

Move the definition of TYPE_PL061 to a new header file, so it can be
used outside the driver.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - New.
---
 MAINTAINERS             |  1 +
 hw/gpio/pl061.c         |  2 +-
 include/hw/gpio/pl061.h | 16 ++++++++++++++++
 3 files changed, 18 insertions(+), 1 deletion(-)
 create mode 100644 include/hw/gpio/pl061.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 8cbc1fac2bfcec86..e760f65270d29d5d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -538,6 +538,7 @@ F: hw/dma/pl080.c
 F: include/hw/dma/pl080.h
 F: hw/dma/pl330.c
 F: hw/gpio/pl061.c
+F: include/hw/gpio/pl061.h
 F: hw/input/pl050.c
 F: hw/intc/pl190.c
 F: hw/sd/pl181.c
diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 2a828260bdb0b946..e776c09e474216ef 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -9,6 +9,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/gpio/pl061.h"
 #include "hw/irq.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
@@ -33,7 +34,6 @@ static const uint8_t pl061_id[12] =
 static const uint8_t pl061_id_luminary[12] =
   { 0x00, 0x00, 0x00, 0x00, 0x61, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1 };
 
-#define TYPE_PL061 "pl061"
 #define PL061(obj) OBJECT_CHECK(PL061State, (obj), TYPE_PL061)
 
 typedef struct PL061State {
diff --git a/include/hw/gpio/pl061.h b/include/hw/gpio/pl061.h
new file mode 100644
index 0000000000000000..78cc40c52679dc4e
--- /dev/null
+++ b/include/hw/gpio/pl061.h
@@ -0,0 +1,16 @@
+/*
+ * Arm PrimeCell PL061 General Purpose IO with additional Luminary Micro
+ * Stellaris bits.
+ *
+ * Copyright (c) 2007 CodeSourcery.
+ * Written by Paul Brook
+ *
+ * This code is licensed under the GPL.
+ */
+
+#ifndef PL061_GPIO_H
+#define PL061_GPIO_H
+
+#define TYPE_PL061 "pl061"
+
+#endif /* PL061_GPIO_H */
-- 
2.17.1

[PATCH QEMU v2 2/5] ARM: PL061: Extract pl061_create_fdt()

[Geert Uytterhoeven]

Move the code to create the DT node for the PL061 GPIO controller from
hw/arm/virt.c to the PL061 driver, so it can be reused.

While at it, make the created node comply with the PL061 Device Tree
bindings:
  - Use generic node name "gpio" instead of "pl061",
  - Add missing "#interrupt-cells" and "interrupt-controller"
    properties.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - New.
---
 hw/arm/virt.c           | 20 +++-----------------
 hw/gpio/pl061.c         | 42 +++++++++++++++++++++++++++++++++++++++++
 include/hw/gpio/pl061.h |  7 +++++++
 3 files changed, 52 insertions(+), 17 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7dc96abf72cf2b9a..c88c8850fbe00bdb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -40,6 +40,7 @@
 #include "hw/arm/primecell.h"
 #include "hw/arm/virt.h"
 #include "hw/block/flash.h"
+#include "hw/gpio/pl061.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
 #include "hw/vfio/vfio-amd-xgbe.h"
 #include "hw/display/ramfb.h"
@@ -807,30 +808,16 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
 
 static void create_gpio(const VirtMachineState *vms)
 {
-    char *nodename;
     DeviceState *pl061_dev;
     hwaddr base = vms->memmap[VIRT_GPIO].base;
     hwaddr size = vms->memmap[VIRT_GPIO].size;
     int irq = vms->irqmap[VIRT_GPIO];
-    const char compat[] = "arm,pl061\0arm,primecell";
 
     pl061_dev = sysbus_create_simple("pl061", base,
                                      qdev_get_gpio_in(vms->gic, irq));
 
-    uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
-    nodename = g_strdup_printf("/pl061@%" PRIx64, base);
-    qemu_fdt_add_subnode(vms->fdt, nodename);
-    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
-                                 2, base, 2, size);
-    qemu_fdt_setprop(vms->fdt, nodename, "compatible", compat, sizeof(compat));
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "#gpio-cells", 2);
-    qemu_fdt_setprop(vms->fdt, nodename, "gpio-controller", NULL, 0);
-    qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
-                           GIC_FDT_IRQ_TYPE_SPI, irq,
-                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "clocks", vms->clock_phandle);
-    qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
-    qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", phandle);
+    uint32_t phandle = pl061_create_fdt(vms->fdt, "", 2, base, size, irq,
+                                        vms->clock_phandle);
 
     gpio_key_dev = sysbus_create_simple("gpio-key", -1,
                                         qdev_get_gpio_in(pl061_dev, 3));
@@ -846,7 +833,6 @@ static void create_gpio(const VirtMachineState *vms)
                           KEY_POWER);
     qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
                            "gpios", phandle, 3, 0);
-    g_free(nodename);
 }
 
 static void create_virtio_devices(const VirtMachineState *vms)
diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index e776c09e474216ef..74ba733a8a5e8ca5 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -9,12 +9,14 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/arm/fdt.h"
 #include "hw/gpio/pl061.h"
 #include "hw/irq.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
+#include "sysemu/device_tree.h"
 
 //#define DEBUG_PL061 1
 
@@ -397,3 +399,43 @@ static void pl061_register_types(void)
 }
 
 type_init(pl061_register_types)
+
+/*
+ * pl061_create_fdt: Create a DT node for a PL061 GPIO controller
+ * @fdt: device tree blob
+ * @parent: name of the parent node
+ * @n_cells: value of #address-cells and #size-cells
+ * @base: base address of the controller's register block
+ * @size: size of the controller's register block
+ * @irq: interrupt number
+ * @clock: phandle of the apb-pclk clock
+ *
+ * Return value: a phandle referring to the created DT node.
+ *
+ * See the DT Binding Documentation in the Linux kernel source tree:
+ * Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
+ */
+uint32_t pl061_create_fdt(void *fdt, const char *parent, unsigned int n_cells,
+                          hwaddr base, hwaddr size, int irq, uint32_t clock)
+{
+    char *nodename = g_strdup_printf("%s/gpio@%" PRIx64, parent, base);
+    static const char compat[] = "arm,pl061\0arm,primecell";
+    uint32_t phandle = qemu_fdt_alloc_phandle(fdt);
+
+    qemu_fdt_add_subnode(fdt, nodename);
+    qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", n_cells, base, n_cells,
+                                 size);
+    qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_cell(fdt, nodename, "#gpio-cells", 2);
+    qemu_fdt_setprop(fdt, nodename, "gpio-controller", NULL, 0);
+    qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 2);
+    qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0);
+    qemu_fdt_setprop_cells(fdt, nodename, "interrupts", GIC_FDT_IRQ_TYPE_SPI,
+                           irq, GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+    qemu_fdt_setprop_cell(fdt, nodename, "clocks", clock);
+    qemu_fdt_setprop_string(fdt, nodename, "clock-names", "apb_pclk");
+    qemu_fdt_setprop_cell(fdt, nodename, "phandle", phandle);
+    g_free(nodename);
+
+    return phandle;
+}
diff --git a/include/hw/gpio/pl061.h b/include/hw/gpio/pl061.h
index 78cc40c52679dc4e..f98c6e24e0e68662 100644
--- a/include/hw/gpio/pl061.h
+++ b/include/hw/gpio/pl061.h
@@ -11,6 +11,13 @@
 #ifndef PL061_GPIO_H
 #define PL061_GPIO_H
 
+#include <stdint.h>
+
+#include "exec/hwaddr.h"
+
 #define TYPE_PL061 "pl061"
 
+uint32_t pl061_create_fdt(void *fdt, const char *parent, unsigned int n_cells,
+                          hwaddr addr, hwaddr size, int irq, uint32_t clock);
+
 #endif /* PL061_GPIO_H */
-- 
2.17.1

[PATCH QEMU v2 3/5] Add a GPIO backend using libgpiod

[Geert Uytterhoeven]

Add a GPIO controller backend, to connect virtual GPIOs on the guest to
physical GPIOs on the host.  This allows the guest to control any
external device connected to the physical GPIOs.

Features and limitations:
  - The backend uses libgpiod on Linux,
  - For now only GPIO outputs are supported,
  - The number of GPIO lines mapped is limited to the number of GPIO
    lines available on the virtual GPIO controller.

Future work:
  - GPIO inputs,
  - GPIO line configuration,
  - Optimizations for controlling multiple GPIO lines at once,
  - ...

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Drop vgpios and gpios parameters, and map the full gpiochip instead,
  - Replace hardcoded PL061 instance by multiple dynamic instances,
    registered through qemu_gpiodev_add().
---
 MAINTAINERS              |  6 +++
 backends/Makefile.objs   |  2 +
 backends/gpiodev.c       | 94 ++++++++++++++++++++++++++++++++++++++++
 configure                | 28 ++++++++++++
 include/sysemu/gpiodev.h | 12 +++++
 5 files changed, 142 insertions(+)
 create mode 100644 backends/gpiodev.c
 create mode 100644 include/sysemu/gpiodev.h

diff --git a/MAINTAINERS b/MAINTAINERS
index e760f65270d29d5d..a70af47430083d14 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -607,6 +607,12 @@ F: include/hw/arm/digic.h
 F: hw/*/digic*
 F: include/hw/*/digic*
 
+GPIO device backend
+M: Geert Uytterhoeven <geert+renesas@glider.be>
+S: Supported
+F: backends/gpiodev.c
+F: include/sysemu/gpiodev.h
+
 Goldfish RTC
 M: Anup Patel <anup.patel@wdc.com>
 M: Alistair Francis <Alistair.Francis@wdc.com>
diff --git a/backends/Makefile.objs b/backends/Makefile.objs
index 28a847cd571d96ed..ee658e797454119a 100644
--- a/backends/Makefile.objs
+++ b/backends/Makefile.objs
@@ -21,3 +21,5 @@ common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
 common-obj-$(CONFIG_GIO) += dbus-vmstate.o
 dbus-vmstate.o-cflags = $(GIO_CFLAGS)
 dbus-vmstate.o-libs = $(GIO_LIBS)
+
+common-obj-$(CONFIG_GPIODEV) += gpiodev.o
diff --git a/backends/gpiodev.c b/backends/gpiodev.c
new file mode 100644
index 0000000000000000..df1bd0113c7b2985
--- /dev/null
+++ b/backends/gpiodev.c
@@ -0,0 +1,94 @@
+/*
+ * QEMU GPIO Backend
+ *
+ * Copyright (C) 2018-2020 Glider bv
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include <errno.h>
+#include <gpiod.h>
+
+#include "qemu/osdep.h"
+#include "qemu/config-file.h"
+#include "qemu/cutils.h"
+#include "qemu/error-report.h"
+#include "qemu/module.h"
+#include "qemu/option.h"
+#include "qapi/error.h"
+
+#include "sysemu/gpiodev.h"
+
+#include "hw/irq.h"
+#include "hw/qdev-core.h"
+
+static void gpiodev_irq_handler(void *opaque, int n, int level)
+{
+    struct gpiod_line *line = opaque;
+    int status;
+
+    status = gpiod_line_set_value(line, level);
+    if (status < 0) {
+        struct gpiod_chip *chip = gpiod_line_get_chip(line);
+
+        error_report("%s/%s: Cannot set GPIO line %u: %s",
+                     gpiod_chip_name(chip), gpiod_chip_label(chip),
+                     gpiod_line_offset(line), strerror(errno));
+    }
+}
+
+static int gpiodev_map_line(DeviceState *dev, struct gpiod_chip *chip,
+                            unsigned int gpio, Error **errp)
+{
+    struct gpiod_line *line;
+    qemu_irq irq;
+    int status;
+
+    line = gpiod_chip_get_line(chip, gpio);
+    if (!line) {
+        error_setg(errp, "Cannot obtain GPIO line %u: %s", gpio,
+                   strerror(errno));
+        return -1;
+    }
+
+    status = gpiod_line_request_output(line, "qemu", 0);
+    if (status < 0) {
+        error_setg(errp, "Cannot request GPIO line %u for output: %s", gpio,
+                   strerror(errno));
+        return status;
+    }
+
+    irq = qemu_allocate_irq(gpiodev_irq_handler, line, 0);
+    qdev_connect_gpio_out(dev, gpio, irq);
+    return 0;
+}
+
+void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int maxgpio,
+                      Error **errp)
+{
+    struct gpiod_chip *chip;
+    unsigned int i, n;
+    int status;
+
+    chip = gpiod_chip_open_lookup(name);
+    if (!chip) {
+        error_setg(errp, "Cannot open GPIO chip %s: %s", name,
+                   strerror(errno));
+        return;
+    }
+
+    n = gpiod_chip_num_lines(chip);
+    if (n > maxgpio) {
+        warn_report("Last %u GPIO line(s) will not be mapped", n - maxgpio);
+        n = maxgpio;
+    }
+
+    for (i = 0; i < n; i++) {
+        status = gpiodev_map_line(dev, chip, i, errp);
+        if (status < 0) {
+            return;
+        }
+    }
+
+    info_report("Mapped %u GPIO lines", n);
+}
diff --git a/configure b/configure
index 23b5e93752b6a259..8b133402ef727c8e 100755
--- a/configure
+++ b/configure
@@ -509,6 +509,7 @@ libpmem=""
 default_devices="yes"
 plugins="no"
 fuzzing="no"
+gpio=""
 
 supported_cpu="no"
 supported_os="no"
@@ -1601,6 +1602,10 @@ for opt do
   ;;
   --gdb=*) gdb_bin="$optarg"
   ;;
+  --disable-gpio) gpio="no"
+  ;;
+  --enable-gpio) gpio="yes"
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -1894,6 +1899,7 @@ disabled with --disable-FEATURE, default is enabled if available:
   debug-mutex     mutex debugging support
   libpmem         libpmem support
   xkbcommon       xkbcommon support
+  gpio            gpio support
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -6250,6 +6256,23 @@ case "$slirp" in
     ;;
 esac
 
+##########################################
+# check for libgpiod
+
+if test "$gpio" != "no"; then
+    if $pkg_config --exists "libgpiod"; then
+        gpio="yes"
+        libgpiod_libs=$($pkg_config --libs libgpiod)
+        libgpiod_cflags=$($pkg_config --cflags libgpiod)
+        libs_softmmu="$libs_softmmu $libgpiod_libs"
+        QEMU_CFLAGS="$QEMU_CFLAGS $libgpiod_cflags"
+    else
+        if test "$gpio" = "yes" ; then
+            feature_not_found "gpio" "Install libgpiod"
+        fi
+        gpio="no"
+    fi
+fi
 
 ##########################################
 # End of CC checks
@@ -6733,6 +6756,7 @@ echo "default devices   $default_devices"
 echo "plugin support    $plugins"
 echo "fuzzing support   $fuzzing"
 echo "gdb               $gdb_bin"
+echo "gpio support      $gpio"
 
 if test "$supported_cpu" = "no"; then
     echo
@@ -7614,6 +7638,10 @@ if test -n "$gdb_bin" ; then
     echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
 fi
 
+if test "$gpio" = "yes" ; then
+  echo "CONFIG_GPIODEV=y" >> $config_host_mak
+fi
+
 if test "$tcg_interpreter" = "yes"; then
   QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
 elif test "$ARCH" = "sparc64" ; then
diff --git a/include/sysemu/gpiodev.h b/include/sysemu/gpiodev.h
new file mode 100644
index 0000000000000000..bedd141001245207
--- /dev/null
+++ b/include/sysemu/gpiodev.h
@@ -0,0 +1,12 @@
+/*
+ * QEMU GPIO Backend
+ *
+ * Copyright (C) 2018-2020 Glider bv
+ *
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/typedefs.h"
+
+void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int maxgpio,
+                      Error **errp);
-- 
2.17.1

[PATCH QEMU v2 4/5] ARM: PL061: Add gpiodev support

[Geert Uytterhoeven]

Make the PL061 GPIO controller user-creatable, and allow the user to tie
a newly created instance to a gpiochip on the host.

To create a new GPIO controller, the QEMU command line must be augmented
with:

    -device pl061,host=<gpiochip>

with <gpiochip> the name or label of the gpiochip on the host.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - New.
---
 hw/gpio/pl061.c | 35 +++++++++++++++++++++++++++++++++++
 qemu-options.hx |  9 +++++++++
 2 files changed, 44 insertions(+)

diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
index 74ba733a8a5e8ca5..98204f9a586ae8c8 100644
--- a/hw/gpio/pl061.c
+++ b/hw/gpio/pl061.c
@@ -12,11 +12,14 @@
 #include "hw/arm/fdt.h"
 #include "hw/gpio/pl061.h"
 #include "hw/irq.h"
+#include "hw/qdev-properties.h"
 #include "hw/sysbus.h"
 #include "migration/vmstate.h"
+#include "qapi/error.h"
 #include "qemu/log.h"
 #include "qemu/module.h"
 #include "sysemu/device_tree.h"
+#include "sysemu/gpiodev.h"
 
 //#define DEBUG_PL061 1
 
@@ -41,6 +44,9 @@ static const uint8_t pl061_id_luminary[12] =
 typedef struct PL061State {
     SysBusDevice parent_obj;
 
+#ifdef CONFIG_GPIODEV
+    char *host;
+#endif
     MemoryRegion iomem;
     uint32_t locked;
     uint32_t data;
@@ -370,10 +376,39 @@ static void pl061_init(Object *obj)
     qdev_init_gpio_out(dev, s->out, 8);
 }
 
+#ifdef CONFIG_GPIODEV
+static Property pl061_properties[] = {
+    DEFINE_PROP_STRING("host", PL061State, host),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
+static void pl061_realize(DeviceState *dev, Error **errp)
+{
+    PL061State *s = PL061(dev);
+
+    if (!dev->opts) {
+        /* Not created by user */
+        return;
+    }
+
+    if (!s->host) {
+        error_setg(errp, "'host' property is required");
+        return;
+    }
+
+    qemu_gpiodev_add(dev, s->host, 8, errp);
+}
+#endif /* CONFIG_GPIODEV */
+
 static void pl061_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
 
+#ifdef CONFIG_GPIODEV
+    device_class_set_props(dc, pl061_properties);
+    dc->realize = pl061_realize;
+    dc->user_creatable = true;
+#endif
     dc->vmsd = &vmstate_pl061;
     dc->reset = &pl061_reset;
 }
diff --git a/qemu-options.hx b/qemu-options.hx
index 292d4e7c0cef6097..182de7fb63923b38 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -875,6 +875,15 @@ SRST
 ``-device isa-ipmi-bt,bmc=id[,ioport=val][,irq=val]``
     Like the KCS interface, but defines a BT interface. The default port
     is 0xe4 and the default interrupt is 5.
+
+#ifdef CONFIG_GPIODEV
+``-device pl061,host=gpiochip``
+    Add a PL061 GPIO controller, and map its virtual GPIO lines to a GPIO
+    controller on the host.
+
+    ``host=gpiochip``
+        The name or label of the GPIO controller on the host.
+#endif
 ERST
 
 DEF("name", HAS_ARG, QEMU_OPTION_name,
-- 
2.17.1

[PATCH QEMU v2 5/5] hw/arm/virt: Add dynamic PL061 GPIO support

[Geert Uytterhoeven]

Add support for dynamically instantiating PL061 GPIO controller
instances in ARM virt.  Device tree nodes are created dynamically.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - New.
---
 hw/arm/sysbus-fdt.c | 18 ++++++++++++++++++
 hw/arm/virt.c       |  1 +
 2 files changed, 19 insertions(+)

diff --git a/hw/arm/sysbus-fdt.c b/hw/arm/sysbus-fdt.c
index 6b6906f4cfc36198..493583218d176d0a 100644
--- a/hw/arm/sysbus-fdt.c
+++ b/hw/arm/sysbus-fdt.c
@@ -32,6 +32,7 @@
 #include "sysemu/device_tree.h"
 #include "sysemu/tpm.h"
 #include "hw/platform-bus.h"
+#include "hw/gpio/pl061.h"
 #include "hw/vfio/vfio-platform.h"
 #include "hw/vfio/vfio-calxeda-xgmac.h"
 #include "hw/vfio/vfio-amd-xgbe.h"
@@ -468,6 +469,22 @@ static int add_tpm_tis_fdt_node(SysBusDevice *sbdev, void *opaque)
     return 0;
 }
 
+/*
+ * add_pl061_node: Create a DT node for a PL061 GPIO controller
+ */
+static int add_pl061_node(SysBusDevice *sbdev, void *opaque)
+{
+    PlatformBusFDTData *data = opaque;
+    PlatformBusDevice *pbus = data->pbus;
+    void *fdt = data->fdt;
+
+    pl061_create_fdt(fdt, data->pbus_node_name, 1,
+                     platform_bus_get_mmio_addr(pbus, sbdev, 0), 0x1000,
+                     platform_bus_get_irqn(pbus, sbdev, 0) + data->irq_start,
+                     qemu_fdt_get_phandle(fdt, "/apb-pclk"));
+    return 0;
+}
+
 static int no_fdt_node(SysBusDevice *sbdev, void *opaque)
 {
     return 0;
@@ -489,6 +506,7 @@ static const BindingEntry bindings[] = {
     VFIO_PLATFORM_BINDING("amd,xgbe-seattle-v1a", add_amd_xgbe_fdt_node),
 #endif
     TYPE_BINDING(TYPE_TPM_TIS_SYSBUS, add_tpm_tis_fdt_node),
+    TYPE_BINDING(TYPE_PL061, add_pl061_node),
     TYPE_BINDING(TYPE_RAMFB_DEVICE, no_fdt_node),
     TYPE_BINDING("", NULL), /* last element */
 };
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index c88c8850fbe00bdb..191594db09422d54 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2178,6 +2178,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_RAMFB_DEVICE);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_VFIO_PLATFORM);
     machine_class_allow_dynamic_sysbus_dev(mc, TYPE_TPM_TIS_SYSBUS);
+    machine_class_allow_dynamic_sysbus_dev(mc, TYPE_PL061);
     mc->block_default_type = IF_VIRTIO;
     mc->no_cdrom = 1;
     mc->pci_allow_0_address = true;
-- 
2.17.1

Re: [PATCH QEMU v2 0/5] Add a GPIO backend

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200423090118.11199-1-geert+renesas@glider.be/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      chardev/char-pipe.o
  CC      chardev/char-pty.o

Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:881:Unexpected indentation.
  CC      chardev/char-ringbuf.o
  CC      chardev/char-serial.o

Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:881:Unexpected indentation.
  CC      chardev/char-socket.o
  CC      chardev/char-stdio.o
---
  CC      blockdev-nbd.o
  CC      iothread.o
  CC      job-qmp.o
make: *** [Makefile:1115: .docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.] Error 2
make: *** Deleting file '.docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.'
make: *** Waiting for unfinished jobs....
make: *** [Makefile:1104: docs/system/index.html] Error 2
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=583b10e192d841ea985fe9474e9cce5b', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-41xb7be_/src/docker-src.2020-04-23-05.09.56.10248:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=583b10e192d841ea985fe9474e9cce5b
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-41xb7be_/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    3m30.232s
user    0m8.457s


The full log is available at
http://patchew.org/logs/20200423090118.11199-1-geert+renesas@glider.be/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH QEMU v2 1/5] ARM: PL061: Move TYPE_PL061 to hw/gpio/pl061.h

[Philippe Mathieu-Daudé]

On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
> Move the definition of TYPE_PL061 to a new header file, so it can be
> used outside the driver.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - New.
> ---
>  MAINTAINERS             |  1 +
>  hw/gpio/pl061.c         |  2 +-
>  include/hw/gpio/pl061.h | 16 ++++++++++++++++
>  3 files changed, 18 insertions(+), 1 deletion(-)
>  create mode 100644 include/hw/gpio/pl061.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 8cbc1fac2bfcec86..e760f65270d29d5d 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -538,6 +538,7 @@ F: hw/dma/pl080.c
>  F: include/hw/dma/pl080.h
>  F: hw/dma/pl330.c
>  F: hw/gpio/pl061.c
> +F: include/hw/gpio/pl061.h
>  F: hw/input/pl050.c
>  F: hw/intc/pl190.c
>  F: hw/sd/pl181.c
> diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
> index 2a828260bdb0b946..e776c09e474216ef 100644
> --- a/hw/gpio/pl061.c
> +++ b/hw/gpio/pl061.c
> @@ -9,6 +9,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "hw/gpio/pl061.h"
>  #include "hw/irq.h"
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
> @@ -33,7 +34,6 @@ static const uint8_t pl061_id[12] =
>  static const uint8_t pl061_id_luminary[12] =
>    { 0x00, 0x00, 0x00, 0x00, 0x61, 0x00, 0x18, 0x01, 0x0d, 0xf0, 0x05, 0xb1 };
>  
> -#define TYPE_PL061 "pl061"
>  #define PL061(obj) OBJECT_CHECK(PL061State, (obj), TYPE_PL061)
>  
>  typedef struct PL061State {
> diff --git a/include/hw/gpio/pl061.h b/include/hw/gpio/pl061.h
> new file mode 100644
> index 0000000000000000..78cc40c52679dc4e
> --- /dev/null
> +++ b/include/hw/gpio/pl061.h
> @@ -0,0 +1,16 @@
> +/*
> + * Arm PrimeCell PL061 General Purpose IO with additional Luminary Micro
> + * Stellaris bits.
> + *
> + * Copyright (c) 2007 CodeSourcery.
> + * Written by Paul Brook
> + *
> + * This code is licensed under the GPL.
> + */
> +
> +#ifndef PL061_GPIO_H
> +#define PL061_GPIO_H
> +
> +#define TYPE_PL061 "pl061"
> +
> +#endif /* PL061_GPIO_H */
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH QEMU v2 2/5] ARM: PL061: Extract pl061_create_fdt()

[Philippe Mathieu-Daudé]

On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
> Move the code to create the DT node for the PL061 GPIO controller from
> hw/arm/virt.c to the PL061 driver, so it can be reused.
> 
> While at it, make the created node comply with the PL061 Device Tree
> bindings:
>   - Use generic node name "gpio" instead of "pl061",

I'd split this patch in 2 (first one being fixing missing properties).

>   - Add missing "#interrupt-cells" and "interrupt-controller"
>     properties.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - New.
> ---
>  hw/arm/virt.c           | 20 +++-----------------
>  hw/gpio/pl061.c         | 42 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/gpio/pl061.h |  7 +++++++
>  3 files changed, 52 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 7dc96abf72cf2b9a..c88c8850fbe00bdb 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -40,6 +40,7 @@
>  #include "hw/arm/primecell.h"
>  #include "hw/arm/virt.h"
>  #include "hw/block/flash.h"
> +#include "hw/gpio/pl061.h"
>  #include "hw/vfio/vfio-calxeda-xgmac.h"
>  #include "hw/vfio/vfio-amd-xgbe.h"
>  #include "hw/display/ramfb.h"
> @@ -807,30 +808,16 @@ static void virt_powerdown_req(Notifier *n, void *opaque)
>  
>  static void create_gpio(const VirtMachineState *vms)
>  {
> -    char *nodename;
>      DeviceState *pl061_dev;
>      hwaddr base = vms->memmap[VIRT_GPIO].base;
>      hwaddr size = vms->memmap[VIRT_GPIO].size;
>      int irq = vms->irqmap[VIRT_GPIO];
> -    const char compat[] = "arm,pl061\0arm,primecell";
>  
>      pl061_dev = sysbus_create_simple("pl061", base,
>                                       qdev_get_gpio_in(vms->gic, irq));
>  
> -    uint32_t phandle = qemu_fdt_alloc_phandle(vms->fdt);
> -    nodename = g_strdup_printf("/pl061@%" PRIx64, base);
> -    qemu_fdt_add_subnode(vms->fdt, nodename);
> -    qemu_fdt_setprop_sized_cells(vms->fdt, nodename, "reg",
> -                                 2, base, 2, size);
> -    qemu_fdt_setprop(vms->fdt, nodename, "compatible", compat, sizeof(compat));
> -    qemu_fdt_setprop_cell(vms->fdt, nodename, "#gpio-cells", 2);
> -    qemu_fdt_setprop(vms->fdt, nodename, "gpio-controller", NULL, 0);
> -    qemu_fdt_setprop_cells(vms->fdt, nodename, "interrupts",
> -                           GIC_FDT_IRQ_TYPE_SPI, irq,
> -                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> -    qemu_fdt_setprop_cell(vms->fdt, nodename, "clocks", vms->clock_phandle);
> -    qemu_fdt_setprop_string(vms->fdt, nodename, "clock-names", "apb_pclk");
> -    qemu_fdt_setprop_cell(vms->fdt, nodename, "phandle", phandle);
> +    uint32_t phandle = pl061_create_fdt(vms->fdt, "", 2, base, size, irq,
> +                                        vms->clock_phandle);
>  
>      gpio_key_dev = sysbus_create_simple("gpio-key", -1,
>                                          qdev_get_gpio_in(pl061_dev, 3));
> @@ -846,7 +833,6 @@ static void create_gpio(const VirtMachineState *vms)
>                            KEY_POWER);
>      qemu_fdt_setprop_cells(vms->fdt, "/gpio-keys/poweroff",
>                             "gpios", phandle, 3, 0);
> -    g_free(nodename);
>  }
>  
>  static void create_virtio_devices(const VirtMachineState *vms)
> diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
> index e776c09e474216ef..74ba733a8a5e8ca5 100644
> --- a/hw/gpio/pl061.c
> +++ b/hw/gpio/pl061.c
> @@ -9,12 +9,14 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "hw/arm/fdt.h"
>  #include "hw/gpio/pl061.h"
>  #include "hw/irq.h"
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
> +#include "sysemu/device_tree.h"
>  
>  //#define DEBUG_PL061 1
>  
> @@ -397,3 +399,43 @@ static void pl061_register_types(void)
>  }
>  
>  type_init(pl061_register_types)
> +
> +/*
> + * pl061_create_fdt: Create a DT node for a PL061 GPIO controller
> + * @fdt: device tree blob
> + * @parent: name of the parent node
> + * @n_cells: value of #address-cells and #size-cells
> + * @base: base address of the controller's register block
> + * @size: size of the controller's register block
> + * @irq: interrupt number
> + * @clock: phandle of the apb-pclk clock
> + *
> + * Return value: a phandle referring to the created DT node.
> + *
> + * See the DT Binding Documentation in the Linux kernel source tree:
> + * Documentation/devicetree/bindings/gpio/pl061-gpio.yaml
> + */
> +uint32_t pl061_create_fdt(void *fdt, const char *parent, unsigned int n_cells,
> +                          hwaddr base, hwaddr size, int irq, uint32_t clock)
> +{
> +    char *nodename = g_strdup_printf("%s/gpio@%" PRIx64, parent, base);
> +    static const char compat[] = "arm,pl061\0arm,primecell";
> +    uint32_t phandle = qemu_fdt_alloc_phandle(fdt);
> +
> +    qemu_fdt_add_subnode(fdt, nodename);
> +    qemu_fdt_setprop_sized_cells(fdt, nodename, "reg", n_cells, base, n_cells,
> +                                 size);
> +    qemu_fdt_setprop(fdt, nodename, "compatible", compat, sizeof(compat));
> +    qemu_fdt_setprop_cell(fdt, nodename, "#gpio-cells", 2);
> +    qemu_fdt_setprop(fdt, nodename, "gpio-controller", NULL, 0);
> +    qemu_fdt_setprop_cell(fdt, nodename, "#interrupt-cells", 2);
> +    qemu_fdt_setprop(fdt, nodename, "interrupt-controller", NULL, 0);
> +    qemu_fdt_setprop_cells(fdt, nodename, "interrupts", GIC_FDT_IRQ_TYPE_SPI,
> +                           irq, GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +    qemu_fdt_setprop_cell(fdt, nodename, "clocks", clock);
> +    qemu_fdt_setprop_string(fdt, nodename, "clock-names", "apb_pclk");
> +    qemu_fdt_setprop_cell(fdt, nodename, "phandle", phandle);
> +    g_free(nodename);
> +
> +    return phandle;
> +}
> diff --git a/include/hw/gpio/pl061.h b/include/hw/gpio/pl061.h
> index 78cc40c52679dc4e..f98c6e24e0e68662 100644
> --- a/include/hw/gpio/pl061.h
> +++ b/include/hw/gpio/pl061.h
> @@ -11,6 +11,13 @@
>  #ifndef PL061_GPIO_H
>  #define PL061_GPIO_H
>  
> +#include <stdint.h>
> +
> +#include "exec/hwaddr.h"
> +
>  #define TYPE_PL061 "pl061"
>  
> +uint32_t pl061_create_fdt(void *fdt, const char *parent, unsigned int n_cells,
> +                          hwaddr addr, hwaddr size, int irq, uint32_t clock);
> +
>  #endif /* PL061_GPIO_H */
> 

Re: [PATCH QEMU v2 3/5] Add a GPIO backend using libgpiod

[Philippe Mathieu-Daudé]

On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
> Add a GPIO controller backend, to connect virtual GPIOs on the guest to
> physical GPIOs on the host.  This allows the guest to control any
> external device connected to the physical GPIOs.
> 
> Features and limitations:
>   - The backend uses libgpiod on Linux,
>   - For now only GPIO outputs are supported,
>   - The number of GPIO lines mapped is limited to the number of GPIO
>     lines available on the virtual GPIO controller.
> 
> Future work:
>   - GPIO inputs,
>   - GPIO line configuration,
>   - Optimizations for controlling multiple GPIO lines at once,
>   - ...
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - Drop vgpios and gpios parameters, and map the full gpiochip instead,
>   - Replace hardcoded PL061 instance by multiple dynamic instances,
>     registered through qemu_gpiodev_add().
> ---
>  MAINTAINERS              |  6 +++
>  backends/Makefile.objs   |  2 +
>  backends/gpiodev.c       | 94 ++++++++++++++++++++++++++++++++++++++++
>  configure                | 28 ++++++++++++
>  include/sysemu/gpiodev.h | 12 +++++
>  5 files changed, 142 insertions(+)
>  create mode 100644 backends/gpiodev.c
>  create mode 100644 include/sysemu/gpiodev.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e760f65270d29d5d..a70af47430083d14 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -607,6 +607,12 @@ F: include/hw/arm/digic.h
>  F: hw/*/digic*
>  F: include/hw/*/digic*
>  
> +GPIO device backend
> +M: Geert Uytterhoeven <geert+renesas@glider.be>
> +S: Supported
> +F: backends/gpiodev.c
> +F: include/sysemu/gpiodev.h
> +
>  Goldfish RTC
>  M: Anup Patel <anup.patel@wdc.com>
>  M: Alistair Francis <Alistair.Francis@wdc.com>
> diff --git a/backends/Makefile.objs b/backends/Makefile.objs
> index 28a847cd571d96ed..ee658e797454119a 100644
> --- a/backends/Makefile.objs
> +++ b/backends/Makefile.objs
> @@ -21,3 +21,5 @@ common-obj-$(CONFIG_LINUX) += hostmem-memfd.o
>  common-obj-$(CONFIG_GIO) += dbus-vmstate.o
>  dbus-vmstate.o-cflags = $(GIO_CFLAGS)
>  dbus-vmstate.o-libs = $(GIO_LIBS)
> +
> +common-obj-$(CONFIG_GPIODEV) += gpiodev.o
> diff --git a/backends/gpiodev.c b/backends/gpiodev.c
> new file mode 100644
> index 0000000000000000..df1bd0113c7b2985
> --- /dev/null
> +++ b/backends/gpiodev.c
> @@ -0,0 +1,94 @@
> +/*
> + * QEMU GPIO Backend
> + *
> + * Copyright (C) 2018-2020 Glider bv
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include <errno.h>

<errno.h> probably not needed.

> +#include <gpiod.h>

Please move this one...

> +
> +#include "qemu/osdep.h"
> +#include "qemu/config-file.h"
> +#include "qemu/cutils.h"
> +#include "qemu/error-report.h"
> +#include "qemu/module.h"
> +#include "qemu/option.h"
> +#include "qapi/error.h"
> +
> +#include "sysemu/gpiodev.h"
> +
> +#include "hw/irq.h"
> +#include "hw/qdev-core.h"

... here:

#include <gpiod.h>

> +
> +static void gpiodev_irq_handler(void *opaque, int n, int level)
> +{
> +    struct gpiod_line *line = opaque;
> +    int status;
> +
> +    status = gpiod_line_set_value(line, level);
> +    if (status < 0) {
> +        struct gpiod_chip *chip = gpiod_line_get_chip(line);
> +
> +        error_report("%s/%s: Cannot set GPIO line %u: %s",
> +                     gpiod_chip_name(chip), gpiod_chip_label(chip),
> +                     gpiod_line_offset(line), strerror(errno));
> +    }
> +}
> +
> +static int gpiodev_map_line(DeviceState *dev, struct gpiod_chip *chip,
> +                            unsigned int gpio, Error **errp)
> +{
> +    struct gpiod_line *line;
> +    qemu_irq irq;
> +    int status;
> +
> +    line = gpiod_chip_get_line(chip, gpio);
> +    if (!line) {
> +        error_setg(errp, "Cannot obtain GPIO line %u: %s", gpio,
> +                   strerror(errno));
> +        return -1;
> +    }
> +
> +    status = gpiod_line_request_output(line, "qemu", 0);
> +    if (status < 0) {
> +        error_setg(errp, "Cannot request GPIO line %u for output: %s", gpio,
> +                   strerror(errno));
> +        return status;
> +    }
> +
> +    irq = qemu_allocate_irq(gpiodev_irq_handler, line, 0);
> +    qdev_connect_gpio_out(dev, gpio, irq);
> +    return 0;
> +}
> +
> +void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int maxgpio,
> +                      Error **errp)
> +{
> +    struct gpiod_chip *chip;
> +    unsigned int i, n;
> +    int status;
> +
> +    chip = gpiod_chip_open_lookup(name);
> +    if (!chip) {
> +        error_setg(errp, "Cannot open GPIO chip %s: %s", name,
> +                   strerror(errno));
> +        return;
> +    }
> +
> +    n = gpiod_chip_num_lines(chip);
> +    if (n > maxgpio) {
> +        warn_report("Last %u GPIO line(s) will not be mapped", n - maxgpio);
> +        n = maxgpio;
> +    }
> +
> +    for (i = 0; i < n; i++) {
> +        status = gpiodev_map_line(dev, chip, i, errp);
> +        if (status < 0) {
> +            return;
> +        }
> +    }
> +
> +    info_report("Mapped %u GPIO lines", n);
> +}
> diff --git a/configure b/configure
> index 23b5e93752b6a259..8b133402ef727c8e 100755
> --- a/configure
> +++ b/configure
> @@ -509,6 +509,7 @@ libpmem=""
>  default_devices="yes"
>  plugins="no"
>  fuzzing="no"
> +gpio=""

Maybe name this feature 'libgpiod'?

>  
>  supported_cpu="no"
>  supported_os="no"
> @@ -1601,6 +1602,10 @@ for opt do
>    ;;
>    --gdb=*) gdb_bin="$optarg"
>    ;;
> +  --disable-gpio) gpio="no"
> +  ;;
> +  --enable-gpio) gpio="yes"

Ditto: --enable-libgpiod, because else it seems rather confusing.

> +  ;;
>    *)
>        echo "ERROR: unknown option $opt"
>        echo "Try '$0 --help' for more information"
> @@ -1894,6 +1899,7 @@ disabled with --disable-FEATURE, default is enabled if available:
>    debug-mutex     mutex debugging support
>    libpmem         libpmem support
>    xkbcommon       xkbcommon support
> +  gpio            gpio support
>  
>  NOTE: The object files are built at the place where configure is launched
>  EOF
> @@ -6250,6 +6256,23 @@ case "$slirp" in
>      ;;
>  esac
>  
> +##########################################
> +# check for libgpiod
> +
> +if test "$gpio" != "no"; then
> +    if $pkg_config --exists "libgpiod"; then
> +        gpio="yes"
> +        libgpiod_libs=$($pkg_config --libs libgpiod)
> +        libgpiod_cflags=$($pkg_config --cflags libgpiod)
> +        libs_softmmu="$libs_softmmu $libgpiod_libs"
> +        QEMU_CFLAGS="$QEMU_CFLAGS $libgpiod_cflags"
> +    else
> +        if test "$gpio" = "yes" ; then
> +            feature_not_found "gpio" "Install libgpiod"
> +        fi
> +        gpio="no"
> +    fi
> +fi
>  
>  ##########################################
>  # End of CC checks
> @@ -6733,6 +6756,7 @@ echo "default devices   $default_devices"
>  echo "plugin support    $plugins"
>  echo "fuzzing support   $fuzzing"
>  echo "gdb               $gdb_bin"
> +echo "gpio support      $gpio"
>  
>  if test "$supported_cpu" = "no"; then
>      echo
> @@ -7614,6 +7638,10 @@ if test -n "$gdb_bin" ; then
>      echo "HAVE_GDB_BIN=$gdb_bin" >> $config_host_mak
>  fi
>  
> +if test "$gpio" = "yes" ; then
> +  echo "CONFIG_GPIODEV=y" >> $config_host_mak
> +fi
> +
>  if test "$tcg_interpreter" = "yes"; then
>    QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
>  elif test "$ARCH" = "sparc64" ; then
> diff --git a/include/sysemu/gpiodev.h b/include/sysemu/gpiodev.h
> new file mode 100644
> index 0000000000000000..bedd141001245207
> --- /dev/null
> +++ b/include/sysemu/gpiodev.h
> @@ -0,0 +1,12 @@
> +/*
> + * QEMU GPIO Backend
> + *
> + * Copyright (C) 2018-2020 Glider bv
> + *
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + */
> +
> +#include "qemu/typedefs.h"

"qemu/typedefs.h" not needed in includes.

> +
> +void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int maxgpio,
> +                      Error **errp);
> 

Re: [PATCH QEMU v2 4/5] ARM: PL061: Add gpiodev support

[Philippe Mathieu-Daudé]

On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
> Make the PL061 GPIO controller user-creatable, and allow the user to tie
> a newly created instance to a gpiochip on the host.
> 
> To create a new GPIO controller, the QEMU command line must be augmented
> with:
> 
>     -device pl061,host=<gpiochip>
> 
> with <gpiochip> the name or label of the gpiochip on the host.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> v2:
>   - New.
> ---
>  hw/gpio/pl061.c | 35 +++++++++++++++++++++++++++++++++++
>  qemu-options.hx |  9 +++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
> index 74ba733a8a5e8ca5..98204f9a586ae8c8 100644
> --- a/hw/gpio/pl061.c
> +++ b/hw/gpio/pl061.c
> @@ -12,11 +12,14 @@
>  #include "hw/arm/fdt.h"
>  #include "hw/gpio/pl061.h"
>  #include "hw/irq.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/sysbus.h"
>  #include "migration/vmstate.h"
> +#include "qapi/error.h"
>  #include "qemu/log.h"
>  #include "qemu/module.h"
>  #include "sysemu/device_tree.h"
> +#include "sysemu/gpiodev.h"
>  
>  //#define DEBUG_PL061 1
>  
> @@ -41,6 +44,9 @@ static const uint8_t pl061_id_luminary[12] =
>  typedef struct PL061State {
>      SysBusDevice parent_obj;
>  
> +#ifdef CONFIG_GPIODEV
> +    char *host;
> +#endif
>      MemoryRegion iomem;
>      uint32_t locked;
>      uint32_t data;
> @@ -370,10 +376,39 @@ static void pl061_init(Object *obj)
>      qdev_init_gpio_out(dev, s->out, 8);

Not related to this patch, but we should replace this 8 magic value by a
proper definition...

>  }
>  
> +#ifdef CONFIG_GPIODEV
> +static Property pl061_properties[] = {
> +    DEFINE_PROP_STRING("host", PL061State, host),
> +    DEFINE_PROP_END_OF_LIST(),
> +};
> +
> +static void pl061_realize(DeviceState *dev, Error **errp)
> +{
> +    PL061State *s = PL061(dev);
> +
> +    if (!dev->opts) {
> +        /* Not created by user */
> +        return;
> +    }
> +
> +    if (!s->host) {
> +        error_setg(errp, "'host' property is required");
> +        return;
> +    }
> +
> +    qemu_gpiodev_add(dev, s->host, 8, errp);
> +}
> +#endif /* CONFIG_GPIODEV */
> +
>  static void pl061_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
> +#ifdef CONFIG_GPIODEV
> +    device_class_set_props(dc, pl061_properties);
> +    dc->realize = pl061_realize;
> +    dc->user_creatable = true;
> +#endif
>      dc->vmsd = &vmstate_pl061;
>      dc->reset = &pl061_reset;
>  }
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 292d4e7c0cef6097..182de7fb63923b38 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -875,6 +875,15 @@ SRST
>  ``-device isa-ipmi-bt,bmc=id[,ioport=val][,irq=val]``
>      Like the KCS interface, but defines a BT interface. The default port
>      is 0xe4 and the default interrupt is 5.
> +
> +#ifdef CONFIG_GPIODEV
> +``-device pl061,host=gpiochip``
> +    Add a PL061 GPIO controller, and map its virtual GPIO lines to a GPIO
> +    controller on the host.
> +
> +    ``host=gpiochip``
> +        The name or label of the GPIO controller on the host.
> +#endif
>  ERST
>  
>  DEF("name", HAS_ARG, QEMU_OPTION_name,
> 

Instead of restricting this to the pl061, it would be cleaner you add a
GPIO_PLUGGABLE_INTERFACE (or GPIO_BINDABLE_INTERFACE or better name),
and have TYPE_PL061 implement it.

Re: [PATCH QEMU v2 0/5] Add a GPIO backend

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200423090118.11199-1-geert+renesas@glider.be/



Hi,

This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/9pfs/9p-synth.o
  CC      hw/9pfs/9p-proxy.o

Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:881:Unexpected indentation.
  CC      hw/9pfs/xen-9p-backend.o

Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:881:Unexpected indentation.
  CC      hw/acpi/core.o
  CC      hw/acpi/piix4.o
  CC      hw/acpi/pcihp.o
  CC      hw/acpi/ich9.o
  CC      hw/acpi/tco.o
make: *** [Makefile:1104: docs/system/index.html] Error 2
make: *** Waiting for unfinished jobs....
make: *** [Makefile:1115: .docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.] Error 2
make: *** Deleting file '.docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.'
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=79991128d80240f382279e2c059f43f2', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=x86_64-softmmu', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-amlfs24e/src/docker-src.2020-04-23-05.29.38.22661:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-debug']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=79991128d80240f382279e2c059f43f2
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-amlfs24e/src'
make: *** [docker-run-test-debug@fedora] Error 2

real    4m43.053s
user    0m5.106s


The full log is available at
http://patchew.org/logs/20200423090118.11199-1-geert+renesas@glider.be/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH QEMU v2 0/5] Add a GPIO backend

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200423090118.11199-1-geert+renesas@glider.be/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      hw/acpi/generic_event_device.o
  CC      hw/acpi/hmat.o

Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:881:Unexpected indentation.
  CC      hw/acpi/acpi_interface.o
  CC      hw/acpi/bios-linker-loader.o
---
  CC      hw/acpi/pci.o
  CC      hw/acpi/ipmi.o
  CC      hw/acpi/acpi-stub.o
make: *** [Makefile:1115: .docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.] Error 2
make: *** Deleting file '.docs_system_qemu.1_docs_system_qemu-block-drivers.7_docs_system_qemu-cpu-models.7.sentinel.'
make: *** Waiting for unfinished jobs....

Warning, treated as error:
/tmp/qemu-test/src/docs/../qemu-options.hx:881:Unexpected indentation.
make: *** [Makefile:1104: docs/system/index.html] Error 2
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
    sys.exit(main())
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=db085d5fd0c44c1fa6016be6e323530d', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-ds0rnrhj/src/docker-src.2020-04-23-05.37.44.31543:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=db085d5fd0c44c1fa6016be6e323530d
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ds0rnrhj/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m45.699s
user    0m7.476s


The full log is available at
http://patchew.org/logs/20200423090118.11199-1-geert+renesas@glider.be/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH QEMU v2 3/5] Add a GPIO backend using libgpiod

[Geert Uytterhoeven]

Hi Philippe,

Thanks for your comments!

On Thu, Apr 23, 2020 at 11:28 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
> > Add a GPIO controller backend, to connect virtual GPIOs on the guest to
> > physical GPIOs on the host.  This allows the guest to control any
> > external device connected to the physical GPIOs.
> >
> > Features and limitations:
> >   - The backend uses libgpiod on Linux,
> >   - For now only GPIO outputs are supported,
> >   - The number of GPIO lines mapped is limited to the number of GPIO
> >     lines available on the virtual GPIO controller.
> >
> > Future work:
> >   - GPIO inputs,
> >   - GPIO line configuration,
> >   - Optimizations for controlling multiple GPIO lines at once,
> >   - ...
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- /dev/null
> > +++ b/backends/gpiodev.c
> > @@ -0,0 +1,94 @@
> > +/*
> > + * QEMU GPIO Backend
> > + *
> > + * Copyright (C) 2018-2020 Glider bv
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include <errno.h>
>
> <errno.h> probably not needed.

It is indeed included by one of the other header files.
What is the QEMU policy w.r.t. that?

>
> > +#include <gpiod.h>
>
> Please move this one...
>
> > +
> > +#include "qemu/osdep.h"
> > +#include "qemu/config-file.h"
> > +#include "qemu/cutils.h"

I forgot to remove the two above...

> > +#include "qemu/error-report.h"
> > +#include "qemu/module.h"
> > +#include "qemu/option.h"

... and the two above.

> > +#include "qapi/error.h"
> > +
> > +#include "sysemu/gpiodev.h"
> > +
> > +#include "hw/irq.h"
> > +#include "hw/qdev-core.h"
>
> ... here:
>
> #include <gpiod.h>

OK.

> > --- a/configure
> > +++ b/configure
> > @@ -509,6 +509,7 @@ libpmem=""
> >  default_devices="yes"
> >  plugins="no"
> >  fuzzing="no"
> > +gpio=""
>
> Maybe name this feature 'libgpiod'?

Makes sense.

> >
> >  supported_cpu="no"
> >  supported_os="no"
> > @@ -1601,6 +1602,10 @@ for opt do
> >    ;;
> >    --gdb=*) gdb_bin="$optarg"
> >    ;;
> > +  --disable-gpio) gpio="no"
> > +  ;;
> > +  --enable-gpio) gpio="yes"
>
> Ditto: --enable-libgpiod, because else it seems rather confusing.

OK.

> > --- /dev/null
> > +++ b/include/sysemu/gpiodev.h
> > @@ -0,0 +1,12 @@
> > +/*
> > + * QEMU GPIO Backend
> > + *
> > + * Copyright (C) 2018-2020 Glider bv
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include "qemu/typedefs.h"
>
> "qemu/typedefs.h" not needed in includes.

While removing that works, it does mean the header file is no longer
self-contained:

include/sysemu/gpiodev.h:10:23: error: unknown type name ‘DeviceState’

> > +
> > +void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int maxgpio,
> > +                      Error **errp);

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH QEMU v2 3/5] Add a GPIO backend using libgpiod

[Philippe Mathieu-Daudé]

On 4/23/20 11:41 AM, Geert Uytterhoeven wrote:
> Hi Philippe,
> 
> Thanks for your comments!
> 
> On Thu, Apr 23, 2020 at 11:28 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
>>> Add a GPIO controller backend, to connect virtual GPIOs on the guest to
>>> physical GPIOs on the host.  This allows the guest to control any
>>> external device connected to the physical GPIOs.
>>>
>>> Features and limitations:
>>>   - The backend uses libgpiod on Linux,
>>>   - For now only GPIO outputs are supported,
>>>   - The number of GPIO lines mapped is limited to the number of GPIO
>>>     lines available on the virtual GPIO controller.
>>>
>>> Future work:
>>>   - GPIO inputs,
>>>   - GPIO line configuration,
>>>   - Optimizations for controlling multiple GPIO lines at once,
>>>   - ...
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
>>> --- /dev/null
>>> +++ b/backends/gpiodev.c
>>> @@ -0,0 +1,94 @@
>>> +/*
>>> + * QEMU GPIO Backend
>>> + *
>>> + * Copyright (C) 2018-2020 Glider bv
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#include <errno.h>
>>
>> <errno.h> probably not needed.
> 
> It is indeed included by one of the other header files.
> What is the QEMU policy w.r.t. that?

See CODING_STYLE.rst:

Include directives
------------------

Order include directives as follows:

.. code-block:: c

    #include "qemu/osdep.h"  /* Always first... */
    #include <...>           /* then system headers... */
    #include "..."           /* and finally QEMU headers. */

The "qemu/osdep.h" header contains preprocessor macros that affect the
behavior
of core system headers like <stdint.h>.  It must be the first include so
that
core system headers included by external libraries get the preprocessor
macros
that QEMU depends on.

> 
>>
>>> +#include <gpiod.h>
>>
>> Please move this one...
>>
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/config-file.h"
>>> +#include "qemu/cutils.h"
> 
> I forgot to remove the two above...
> 
>>> +#include "qemu/error-report.h"
>>> +#include "qemu/module.h"
>>> +#include "qemu/option.h"
> 
> ... and the two above.
> 
>>> +#include "qapi/error.h"
>>> +
>>> +#include "sysemu/gpiodev.h"
>>> +
>>> +#include "hw/irq.h"
>>> +#include "hw/qdev-core.h"
>>
>> ... here:
>>
>> #include <gpiod.h>
> 
> OK.
> 
>>> --- a/configure
>>> +++ b/configure
>>> @@ -509,6 +509,7 @@ libpmem=""
>>>  default_devices="yes"
>>>  plugins="no"
>>>  fuzzing="no"
>>> +gpio=""
>>
>> Maybe name this feature 'libgpiod'?
> 
> Makes sense.
> 
>>>
>>>  supported_cpu="no"
>>>  supported_os="no"
>>> @@ -1601,6 +1602,10 @@ for opt do
>>>    ;;
>>>    --gdb=*) gdb_bin="$optarg"
>>>    ;;
>>> +  --disable-gpio) gpio="no"
>>> +  ;;
>>> +  --enable-gpio) gpio="yes"
>>
>> Ditto: --enable-libgpiod, because else it seems rather confusing.
> 
> OK.
> 
>>> --- /dev/null
>>> +++ b/include/sysemu/gpiodev.h
>>> @@ -0,0 +1,12 @@
>>> +/*
>>> + * QEMU GPIO Backend
>>> + *
>>> + * Copyright (C) 2018-2020 Glider bv
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#include "qemu/typedefs.h"
>>
>> "qemu/typedefs.h" not needed in includes.
> 
> While removing that works, it does mean the header file is no longer
> self-contained:
> 
> include/sysemu/gpiodev.h:10:23: error: unknown type name ‘DeviceState’

Odd, because your backends/gpiodev.c already has:

#include "hw/qdev-core.h"

> 
>>> +
>>> +void qemu_gpiodev_add(DeviceState *dev, const char *name, unsigned int maxgpio,
>>> +                      Error **errp);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH QEMU v2 4/5] ARM: PL061: Add gpiodev support

[Philippe Mathieu-Daudé]

On 4/23/20 11:33 AM, Philippe Mathieu-Daudé wrote:
> On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
>> Make the PL061 GPIO controller user-creatable, and allow the user to tie
>> a newly created instance to a gpiochip on the host.
>>
>> To create a new GPIO controller, the QEMU command line must be augmented
>> with:
>>
>>     -device pl061,host=<gpiochip>
>>
>> with <gpiochip> the name or label of the gpiochip on the host.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>> v2:
>>   - New.
>> ---
>>  hw/gpio/pl061.c | 35 +++++++++++++++++++++++++++++++++++
>>  qemu-options.hx |  9 +++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
>> index 74ba733a8a5e8ca5..98204f9a586ae8c8 100644
>> --- a/hw/gpio/pl061.c
>> +++ b/hw/gpio/pl061.c
>> @@ -12,11 +12,14 @@
>>  #include "hw/arm/fdt.h"
>>  #include "hw/gpio/pl061.h"
>>  #include "hw/irq.h"
>> +#include "hw/qdev-properties.h"
>>  #include "hw/sysbus.h"
>>  #include "migration/vmstate.h"
>> +#include "qapi/error.h"
>>  #include "qemu/log.h"
>>  #include "qemu/module.h"
>>  #include "sysemu/device_tree.h"
>> +#include "sysemu/gpiodev.h"
>>  
>>  //#define DEBUG_PL061 1
>>  
>> @@ -41,6 +44,9 @@ static const uint8_t pl061_id_luminary[12] =
>>  typedef struct PL061State {
>>      SysBusDevice parent_obj;
>>  
>> +#ifdef CONFIG_GPIODEV
>> +    char *host;
>> +#endif
>>      MemoryRegion iomem;
>>      uint32_t locked;
>>      uint32_t data;
>> @@ -370,10 +376,39 @@ static void pl061_init(Object *obj)
>>      qdev_init_gpio_out(dev, s->out, 8);
> 
> Not related to this patch, but we should replace this 8 magic value by a
> proper definition...
> 
>>  }
>>  
>> +#ifdef CONFIG_GPIODEV
>> +static Property pl061_properties[] = {
>> +    DEFINE_PROP_STRING("host", PL061State, host),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
>> +static void pl061_realize(DeviceState *dev, Error **errp)
>> +{
>> +    PL061State *s = PL061(dev);
>> +
>> +    if (!dev->opts) {
>> +        /* Not created by user */
>> +        return;
>> +    }
>> +
>> +    if (!s->host) {
>> +        error_setg(errp, "'host' property is required");
>> +        return;
>> +    }
>> +
>> +    qemu_gpiodev_add(dev, s->host, 8, errp);
>> +}
>> +#endif /* CONFIG_GPIODEV */
>> +
>>  static void pl061_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>>  
>> +#ifdef CONFIG_GPIODEV
>> +    device_class_set_props(dc, pl061_properties);
>> +    dc->realize = pl061_realize;
>> +    dc->user_creatable = true;
>> +#endif
>>      dc->vmsd = &vmstate_pl061;
>>      dc->reset = &pl061_reset;
>>  }
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 292d4e7c0cef6097..182de7fb63923b38 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -875,6 +875,15 @@ SRST
>>  ``-device isa-ipmi-bt,bmc=id[,ioport=val][,irq=val]``
>>      Like the KCS interface, but defines a BT interface. The default port
>>      is 0xe4 and the default interrupt is 5.
>> +
>> +#ifdef CONFIG_GPIODEV
>> +``-device pl061,host=gpiochip``
>> +    Add a PL061 GPIO controller, and map its virtual GPIO lines to a GPIO
>> +    controller on the host.
>> +
>> +    ``host=gpiochip``
>> +        The name or label of the GPIO controller on the host.
>> +#endif
>>  ERST
>>  
>>  DEF("name", HAS_ARG, QEMU_OPTION_name,
>>
> 
> Instead of restricting this to the pl061, it would be cleaner you add a
> GPIO_PLUGGABLE_INTERFACE (or GPIO_BINDABLE_INTERFACE or better name),
> and have TYPE_PL061 implement it.

IOW your backend should consume devices implementing this generic interface.

Re: [PATCH QEMU v2 4/5] ARM: PL061: Add gpiodev support

[Geert Uytterhoeven]

Hi Philippe,

On Thu, Apr 23, 2020 at 12:08 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 4/23/20 11:33 AM, Philippe Mathieu-Daudé wrote:
> > On 4/23/20 11:01 AM, Geert Uytterhoeven wrote:
> >> Make the PL061 GPIO controller user-creatable, and allow the user to tie
> >> a newly created instance to a gpiochip on the host.
> >>
> >> To create a new GPIO controller, the QEMU command line must be augmented
> >> with:
> >>
> >>     -device pl061,host=<gpiochip>
> >>
> >> with <gpiochip> the name or label of the gpiochip on the host.
> >>
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> >> --- a/hw/gpio/pl061.c
> >> +++ b/hw/gpio/pl061.c
> >> @@ -12,11 +12,14 @@
> >>  #include "hw/arm/fdt.h"
> >>  #include "hw/gpio/pl061.h"
> >>  #include "hw/irq.h"
> >> +#include "hw/qdev-properties.h"
> >>  #include "hw/sysbus.h"
> >>  #include "migration/vmstate.h"
> >> +#include "qapi/error.h"
> >>  #include "qemu/log.h"
> >>  #include "qemu/module.h"
> >>  #include "sysemu/device_tree.h"
> >> +#include "sysemu/gpiodev.h"
> >>
> >>  //#define DEBUG_PL061 1
> >>
> >> @@ -41,6 +44,9 @@ static const uint8_t pl061_id_luminary[12] =
> >>  typedef struct PL061State {
> >>      SysBusDevice parent_obj;
> >>
> >> +#ifdef CONFIG_GPIODEV
> >> +    char *host;
> >> +#endif
> >>      MemoryRegion iomem;
> >>      uint32_t locked;
> >>      uint32_t data;
> >> @@ -370,10 +376,39 @@ static void pl061_init(Object *obj)
> >>      qdev_init_gpio_out(dev, s->out, 8);
> >>
> >> +#ifdef CONFIG_GPIODEV
> >> +static Property pl061_properties[] = {
> >> +    DEFINE_PROP_STRING("host", PL061State, host),
> >> +    DEFINE_PROP_END_OF_LIST(),
> >> +};
> >> +
> >> +static void pl061_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    PL061State *s = PL061(dev);
> >> +
> >> +    if (!dev->opts) {
> >> +        /* Not created by user */
> >> +        return;
> >> +    }
> >> +
> >> +    if (!s->host) {
> >> +        error_setg(errp, "'host' property is required");
> >> +        return;
> >> +    }
> >> +
> >> +    qemu_gpiodev_add(dev, s->host, 8, errp);
> >> +}
> >> +#endif /* CONFIG_GPIODEV */
> >> +
> >>  static void pl061_class_init(ObjectClass *klass, void *data)
> >>  {
> >>      DeviceClass *dc = DEVICE_CLASS(klass);
> >>
> >> +#ifdef CONFIG_GPIODEV
> >> +    device_class_set_props(dc, pl061_properties);
> >> +    dc->realize = pl061_realize;
> >> +    dc->user_creatable = true;
> >> +#endif
> >>      dc->vmsd = &vmstate_pl061;
> >>      dc->reset = &pl061_reset;
> >>  }
> >> diff --git a/qemu-options.hx b/qemu-options.hx
> >> index 292d4e7c0cef6097..182de7fb63923b38 100644
> >> --- a/qemu-options.hx
> >> +++ b/qemu-options.hx
> >> @@ -875,6 +875,15 @@ SRST
> >>  ``-device isa-ipmi-bt,bmc=id[,ioport=val][,irq=val]``
> >>      Like the KCS interface, but defines a BT interface. The default port
> >>      is 0xe4 and the default interrupt is 5.
> >> +
> >> +#ifdef CONFIG_GPIODEV
> >> +``-device pl061,host=gpiochip``
> >> +    Add a PL061 GPIO controller, and map its virtual GPIO lines to a GPIO
> >> +    controller on the host.
> >> +
> >> +    ``host=gpiochip``
> >> +        The name or label of the GPIO controller on the host.
> >> +#endif
> >>  ERST
> >>
> >>  DEF("name", HAS_ARG, QEMU_OPTION_name,
> >>
> >
> > Instead of restricting this to the pl061, it would be cleaner you add a
> > GPIO_PLUGGABLE_INTERFACE (or GPIO_BINDABLE_INTERFACE or better name),
> > and have TYPE_PL061 implement it.
>
> IOW your backend should consume devices implementing this generic interface.

Thanks for the suggestion! I had a look into implementing this.

Please pardon my QEMU illiteracy, but I fail to see how adding an
interface, and letting frontends like pl061.c implement it, will help:
  - The backend never has to call directly into the frontend: all
    communication is done indirectly, using existing core qemu irq and
    qdev gpio calls.
  - The frontend has to call into the backend once, at initialization
    time, to pass the host= parameter, and the number of GPIOs supported
    by the frontend (through qemu_gpiodev_add()).
  - Generalizing host= parsing in the backend would be nice, to avoid
    duplicating this in each and every frontend, but AFAIU, an interface
    cannot use device_class_set_props(), as InterfaceClass is not
    derived from DeviceClass.

Note that when adding more features later (input support, and e.g.
pull-up/down support), the frontend will have to call into the backend
to change GPIO line configuration at runtime, but this is from frontend
to backend again, not the other way around.

I do agree that if we ever want to support multiple backends,
implementing that through an interface (for the backend, not for the
frontend) would be needed.

What am I missing?
Thanks again!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds