[PATCH 0/3] Split TYPE_GPIOPWR and reuse in E500 machines

[PATCH 0/3] Split TYPE_GPIOPWR and reuse in E500 machines

[Bernhard Beschow]

This series is part of a bigger series exploring data-driven machine creation
using device tree blobs on top of the e500 machines [1]. The idea is to
instantiate a QEMU device model for each device tree node containing a
compatible property. [1] achieves feature-parity with the hardcoded machines
when supplied the same device tree blob that the hardcoded machine would
generate.

Just like the ARM virt machine, the ppce500 machine implements a
"gpio-poweroff"-compatible device tree node. Unfortunately, the implementation
isn't shared which this series fixes. In order to reflect device tree which has
separate bindings for gpio-poweroff and gpio-reset, and to prepare for the
above, the gpio-pwr device model is split.

Note: If the split seems too fine-grained, the existing gpio-pwr device model
could probably be reused in ppce500, too.

Testing done:
* Build qemu_ppc64_e5500_defconfig in Buildroot, run it in the ppce500 machine
  and issue the `poweroff` command. Observe that QEMU is shut down cleanly.
* ARM virt: How to test the secure path?

[1] https://github.com/shentok/qemu/tree/e500-fdt

Bernhard Beschow (3):
  MAINTAINERS: Add hw/gpio/gpio_pwr.c
  hw/gpio/gpio_pwr: Split into separate gpio_poweroff and gpio_restart
    devices
  hw/ppc/e500: Reuse TYPE_GPIO_POWEROFF

 MAINTAINERS             |  2 ++
 hw/arm/virt.c           | 32 +++++++++++++------
 hw/gpio/gpio_poweroff.c | 51 ++++++++++++++++++++++++++++++
 hw/gpio/gpio_pwr.c      | 70 -----------------------------------------
 hw/gpio/gpio_restart.c  | 51 ++++++++++++++++++++++++++++++
 hw/ppc/e500.c           | 15 ++-------
 hw/arm/Kconfig          |  3 +-
 hw/gpio/Kconfig         |  5 ++-
 hw/gpio/meson.build     |  3 +-
 hw/ppc/Kconfig          |  1 +
 10 files changed, 138 insertions(+), 95 deletions(-)
 create mode 100644 hw/gpio/gpio_poweroff.c
 delete mode 100644 hw/gpio/gpio_pwr.c
 create mode 100644 hw/gpio/gpio_restart.c

-- 
2.46.1

[PATCH 1/3] MAINTAINERS: Add hw/gpio/gpio_pwr.c

[Bernhard Beschow]

The device is only used in the ARM virt machine and designed to be used on top
of pl061 for use cases such as ARM Trusted Firmware. Add it to the same section
as hw/gpio/pl061.c.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index ffacd60f40..7e0d3509e8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -656,6 +656,7 @@ F: hw/display/pl110*
 F: hw/dma/pl080.c
 F: include/hw/dma/pl080.h
 F: hw/dma/pl330.c
+F: hw/gpio/gpio_pwr.c
 F: hw/gpio/pl061.c
 F: hw/input/pl050.c
 F: include/hw/input/pl050.h
-- 
2.46.1

[PATCH 2/3] hw/gpio/gpio_pwr: Split into separate gpio_poweroff and gpio_restart devices

[Bernhard Beschow]

Take inspiration from Linux which has separate device tree bindings for the two
GPIO lines. The naming of the two device models matches Linux' compatible
properties.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 MAINTAINERS             |  3 +-
 hw/arm/virt.c           | 32 +++++++++++++------
 hw/gpio/gpio_poweroff.c | 51 ++++++++++++++++++++++++++++++
 hw/gpio/gpio_pwr.c      | 70 -----------------------------------------
 hw/gpio/gpio_restart.c  | 51 ++++++++++++++++++++++++++++++
 hw/arm/Kconfig          |  3 +-
 hw/gpio/Kconfig         |  5 ++-
 hw/gpio/meson.build     |  3 +-
 8 files changed, 134 insertions(+), 84 deletions(-)
 create mode 100644 hw/gpio/gpio_poweroff.c
 delete mode 100644 hw/gpio/gpio_pwr.c
 create mode 100644 hw/gpio/gpio_restart.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7e0d3509e8..8c17d68821 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -656,7 +656,8 @@ F: hw/display/pl110*
 F: hw/dma/pl080.c
 F: include/hw/dma/pl080.h
 F: hw/dma/pl330.c
-F: hw/gpio/gpio_pwr.c
+F: hw/gpio/gpio_poweroff.c
+F: hw/gpio/gpio_restart.c
 F: hw/gpio/pl061.c
 F: hw/input/pl050.c
 F: include/hw/input/pl050.h
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8b2b991d97..1ce1a1ebc1 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1032,19 +1032,17 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
 #define SECURE_GPIO_POWEROFF 0
 #define SECURE_GPIO_RESET    1
 
-static void create_secure_gpio_pwr(char *fdt, DeviceState *pl061_dev,
-                                   uint32_t phandle)
+static void create_secure_gpio_poweroff(char *fdt, DeviceState *pl061_dev,
+                                        uint32_t phandle)
 {
-    DeviceState *gpio_pwr_dev;
+    DeviceState *gpio_poweroff_dev;
 
-    /* gpio-pwr */
-    gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
+    /* gpio-poweroff */
+    gpio_poweroff_dev = sysbus_create_simple("gpio-poweroff", -1, NULL);
 
-    /* connect secure pl061 to gpio-pwr */
-    qdev_connect_gpio_out(pl061_dev, SECURE_GPIO_RESET,
-                          qdev_get_gpio_in_named(gpio_pwr_dev, "reset", 0));
+    /* connect secure pl061 to gpio-poweroff */
     qdev_connect_gpio_out(pl061_dev, SECURE_GPIO_POWEROFF,
-                          qdev_get_gpio_in_named(gpio_pwr_dev, "shutdown", 0));
+                          qdev_get_gpio_in(gpio_poweroff_dev, 0));
 
     qemu_fdt_add_subnode(fdt, "/gpio-poweroff");
     qemu_fdt_setprop_string(fdt, "/gpio-poweroff", "compatible",
@@ -1054,6 +1052,19 @@ static void create_secure_gpio_pwr(char *fdt, DeviceState *pl061_dev,
     qemu_fdt_setprop_string(fdt, "/gpio-poweroff", "status", "disabled");
     qemu_fdt_setprop_string(fdt, "/gpio-poweroff", "secure-status",
                             "okay");
+}
+
+static void create_secure_gpio_restart(char *fdt, DeviceState *pl061_dev,
+                                       uint32_t phandle)
+{
+    DeviceState *gpio_restart_dev;
+
+    /* gpio-restart */
+    gpio_restart_dev = sysbus_create_simple("gpio-restart", -1, NULL);
+
+    /* connect secure pl061 to gpio-restart */
+    qdev_connect_gpio_out(pl061_dev, SECURE_GPIO_RESET,
+                          qdev_get_gpio_in(gpio_restart_dev, 0));
 
     qemu_fdt_add_subnode(fdt, "/gpio-restart");
     qemu_fdt_setprop_string(fdt, "/gpio-restart", "compatible",
@@ -1112,7 +1123,8 @@ static void create_gpio_devices(const VirtMachineState *vms, int gpio,
     if (gpio == VIRT_GPIO) {
         create_gpio_keys(ms->fdt, pl061_dev, phandle);
     } else {
-        create_secure_gpio_pwr(ms->fdt, pl061_dev, phandle);
+        create_secure_gpio_poweroff(ms->fdt, pl061_dev, phandle);
+        create_secure_gpio_restart(ms->fdt, pl061_dev, phandle);
     }
 }
 
diff --git a/hw/gpio/gpio_poweroff.c b/hw/gpio/gpio_poweroff.c
new file mode 100644
index 0000000000..9c5fd0cc4d
--- /dev/null
+++ b/hw/gpio/gpio_poweroff.c
@@ -0,0 +1,51 @@
+/*
+ * GPIO qemu poweroff controller
+ *
+ * Copyright (c) 2020 Linaro Limited
+ *
+ * Author: Maxim Uvarov <maxim.uvarov@linaro.org>
+ *
+ * Virtual gpio driver which can be used on top of pl061 to reboot qemu virtual
+ * machine. One of use case is gpio driver for secure world application (ARM
+ * Trusted Firmware).
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "sysemu/runstate.h"
+
+#define TYPE_GPIO_POWEROFF "gpio-poweroff"
+OBJECT_DECLARE_SIMPLE_TYPE(GpioPoweroffState, GPIO_POWEROFF)
+
+struct GpioPoweroffState {
+    SysBusDevice parent_obj;
+};
+
+static void gpio_poweroff(void *opaque, int n, int level)
+{
+    if (level) {
+        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
+    }
+}
+
+static void gpio_poweroff_init(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    qdev_init_gpio_in(dev, gpio_poweroff, 1);
+}
+
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_GPIO_POWEROFF,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(GpioPoweroffState),
+        .instance_init = gpio_poweroff_init,
+    },
+};
+
+DEFINE_TYPES(types)
diff --git a/hw/gpio/gpio_pwr.c b/hw/gpio/gpio_pwr.c
deleted file mode 100644
index dbaf1c70c8..0000000000
--- a/hw/gpio/gpio_pwr.c
+++ /dev/null
@@ -1,70 +0,0 @@
-/*
- * GPIO qemu power controller
- *
- * Copyright (c) 2020 Linaro Limited
- *
- * Author: Maxim Uvarov <maxim.uvarov@linaro.org>
- *
- * Virtual gpio driver which can be used on top of pl061
- * to reboot and shutdown qemu virtual machine. One of use
- * case is gpio driver for secure world application (ARM
- * Trusted Firmware.).
- *
- * This work is licensed under the terms of the GNU GPL, version 2 or later.
- * See the COPYING file in the top-level directory.
- * SPDX-License-Identifier: GPL-2.0-or-later
- */
-
-/*
- * QEMU interface:
- * two named input GPIO lines:
- *   'reset' : when asserted, trigger system reset
- *   'shutdown' : when asserted, trigger system shutdown
- */
-
-#include "qemu/osdep.h"
-#include "hw/sysbus.h"
-#include "sysemu/runstate.h"
-
-#define TYPE_GPIOPWR "gpio-pwr"
-OBJECT_DECLARE_SIMPLE_TYPE(GPIO_PWR_State, GPIOPWR)
-
-struct GPIO_PWR_State {
-    SysBusDevice parent_obj;
-};
-
-static void gpio_pwr_reset(void *opaque, int n, int level)
-{
-    if (level) {
-        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
-    }
-}
-
-static void gpio_pwr_shutdown(void *opaque, int n, int level)
-{
-    if (level) {
-        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
-    }
-}
-
-static void gpio_pwr_init(Object *obj)
-{
-    DeviceState *dev = DEVICE(obj);
-
-    qdev_init_gpio_in_named(dev, gpio_pwr_reset, "reset", 1);
-    qdev_init_gpio_in_named(dev, gpio_pwr_shutdown, "shutdown", 1);
-}
-
-static const TypeInfo gpio_pwr_info = {
-    .name          = TYPE_GPIOPWR,
-    .parent        = TYPE_SYS_BUS_DEVICE,
-    .instance_size = sizeof(GPIO_PWR_State),
-    .instance_init = gpio_pwr_init,
-};
-
-static void gpio_pwr_register_types(void)
-{
-    type_register_static(&gpio_pwr_info);
-}
-
-type_init(gpio_pwr_register_types)
diff --git a/hw/gpio/gpio_restart.c b/hw/gpio/gpio_restart.c
new file mode 100644
index 0000000000..045d8cfda8
--- /dev/null
+++ b/hw/gpio/gpio_restart.c
@@ -0,0 +1,51 @@
+/*
+ * GPIO qemu restart controller
+ *
+ * Copyright (c) 2020 Linaro Limited
+ *
+ * Author: Maxim Uvarov <maxim.uvarov@linaro.org>
+ *
+ * Virtual gpio driver which can be used on top of pl061 to reboot qemu virtual
+ * machine. One of use case is gpio driver for secure world application (ARM
+ * Trusted Firmware).
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ */
+
+#include "qemu/osdep.h"
+#include "hw/sysbus.h"
+#include "sysemu/runstate.h"
+
+#define TYPE_GPIO_RESTART "gpio-restart"
+OBJECT_DECLARE_SIMPLE_TYPE(GpioRestartState, GPIO_RESTART)
+
+struct GpioRestartState {
+    SysBusDevice parent_obj;
+};
+
+static void gpio_restart(void *opaque, int n, int level)
+{
+    if (level) {
+        qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
+    }
+}
+
+static void gpio_restart_init(Object *obj)
+{
+    DeviceState *dev = DEVICE(obj);
+
+    qdev_init_gpio_in(dev, gpio_restart, 1);
+}
+
+static const TypeInfo types[] = {
+    {
+        .name          = TYPE_GPIO_RESTART,
+        .parent        = TYPE_SYS_BUS_DEVICE,
+        .instance_size = sizeof(GpioRestartState),
+        .instance_init = gpio_restart_init,
+    },
+};
+
+DEFINE_TYPES(types)
diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 1ad60da7aa..2592f8c60b 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -23,7 +23,8 @@ config ARM_VIRT
     select PL011 # UART
     select PL031 # RTC
     select PL061 # GPIO
-    select GPIO_PWR
+    select GPIO_POWEROFF
+    select GPIO_RESTART
     select PLATFORM_BUS
     select SMBIOS
     select VIRTIO_MMIO
diff --git a/hw/gpio/Kconfig b/hw/gpio/Kconfig
index 19c97cc823..49cb25400c 100644
--- a/hw/gpio/Kconfig
+++ b/hw/gpio/Kconfig
@@ -11,7 +11,10 @@ config GPIO_KEY
 config GPIO_MPC8XXX
     bool
 
-config GPIO_PWR
+config GPIO_POWEROFF
+    bool
+
+config GPIO_RESTART
     bool
 
 config SIFIVE_GPIO
diff --git a/hw/gpio/meson.build b/hw/gpio/meson.build
index a7495d196a..0e12f47ce1 100644
--- a/hw/gpio/meson.build
+++ b/hw/gpio/meson.build
@@ -1,6 +1,7 @@
 system_ss.add(when: 'CONFIG_GPIO_KEY', if_true: files('gpio_key.c'))
 system_ss.add(when: 'CONFIG_GPIO_MPC8XXX', if_true: files('mpc8xxx.c'))
-system_ss.add(when: 'CONFIG_GPIO_PWR', if_true: files('gpio_pwr.c'))
+system_ss.add(when: 'CONFIG_GPIO_POWEROFF', if_true: files('gpio_poweroff.c'))
+system_ss.add(when: 'CONFIG_GPIO_RESTART', if_true: files('gpio_restart.c'))
 system_ss.add(when: 'CONFIG_MAX7310', if_true: files('max7310.c'))
 system_ss.add(when: 'CONFIG_PCA9552', if_true: files('pca9552.c'))
 system_ss.add(when: 'CONFIG_PCA9554', if_true: files('pca9554.c'))
-- 
2.46.1

[PATCH 3/3] hw/ppc/e500: Reuse TYPE_GPIO_POWEROFF

[Bernhard Beschow]

The ppce500 machine provides a device tree node whose compatible property is
"gpio-poweroff". This matches TYPE_GPIO_POWEROFF like used in the ARM virt
machine, so reuse it.

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/ppc/e500.c  | 15 +++------------
 hw/ppc/Kconfig |  1 +
 2 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 3bd12b54ab..af74b32c13 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -30,7 +30,6 @@
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "sysemu/reset.h"
-#include "sysemu/runstate.h"
 #include "kvm_ppc.h"
 #include "sysemu/device_tree.h"
 #include "hw/ppc/openpic.h"
@@ -47,7 +46,6 @@
 #include "hw/platform-bus.h"
 #include "hw/net/fsl_etsec/etsec.h"
 #include "hw/i2c/i2c.h"
-#include "hw/irq.h"
 #include "hw/sd/sdhci.h"
 #include "hw/misc/unimp.h"
 
@@ -892,13 +890,6 @@ static DeviceState *ppce500_init_mpic(PPCE500MachineState *pms,
     return dev;
 }
 
-static void ppce500_power_off(void *opaque, int line, int on)
-{
-    if (on) {
-        qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
-    }
-}
-
 void ppce500_init(MachineState *machine)
 {
     MemoryRegion *address_space_mem = get_system_memory();
@@ -1086,7 +1077,7 @@ void ppce500_init(MachineState *machine)
     sysbus_create_simple("e500-spin", pmc->spin_base, NULL);
 
     if (pmc->has_mpc8xxx_gpio) {
-        qemu_irq poweroff_irq;
+        DeviceState *poweroff_dev;
 
         dev = qdev_new("mpc8xxx_gpio");
         s = SYS_BUS_DEVICE(dev);
@@ -1096,8 +1087,8 @@ void ppce500_init(MachineState *machine)
                                     sysbus_mmio_get_region(s, 0));
 
         /* Power Off GPIO at Pin 0 */
-        poweroff_irq = qemu_allocate_irq(ppce500_power_off, NULL, 0);
-        qdev_connect_gpio_out(dev, 0, poweroff_irq);
+        poweroff_dev = sysbus_create_simple("gpio-poweroff", -1, NULL);
+        qdev_connect_gpio_out(dev, 0, qdev_get_gpio_in(poweroff_dev, 0));
     }
 
     /* Platform Bus Device */
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 5addad1124..31764ef909 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -158,6 +158,7 @@ config E500
     imply VIRTIO_PCI
     select ETSEC
     select GPIO_MPC8XXX
+    select GPIO_POWEROFF
     select OPENPIC
     select PFLASH_CFI01
     select PLATFORM_BUS
-- 
2.46.1

Re: [PATCH 0/3] Split TYPE_GPIOPWR and reuse in E500 machines

[Peter Maydell]

On Mon, 23 Sept 2024 at 11:16, Bernhard Beschow <shentey@gmail.com> wrote:
>
> This series is part of a bigger series exploring data-driven machine creation
> using device tree blobs on top of the e500 machines [1]. The idea is to
> instantiate a QEMU device model for each device tree node containing a
> compatible property. [1] achieves feature-parity with the hardcoded machines
> when supplied the same device tree blob that the hardcoded machine would
> generate.

FWIW, on Arm I have generally pushed back against the idea
of "create a QEMU machine from a device tree", because the
device tree does not in general contain sufficient information
to create a QEMU machine. It only has the information that
Linux needs to use the devices, which is a subset of the
total "how do all these devices exist and get wired together".
(There are some special cases like some Xilinx FPGA systems,
where both the device tree and the hardware were autogenerated
from a common source definition, and so there's enough of
a constraint on what the hardware can be to make it workable.)

> Just like the ARM virt machine, the ppce500 machine implements a
> "gpio-poweroff"-compatible device tree node. Unfortunately, the implementation
> isn't shared which this series fixes. In order to reflect device tree which has
> separate bindings for gpio-poweroff and gpio-reset, and to prepare for the
> above, the gpio-pwr device model is split.
>
> Note: If the split seems too fine-grained, the existing gpio-pwr device model
> could probably be reused in ppce500, too.

I definitely like getting rid of the hand-coded
qemu_allocate_irq() in e500.c. But I don't really see
the benefit of splitting gpio_pwr into two devices.
If you only need the power-off behaviour, you can
leave the restart gpio input not connected to anything.

thanks
-- PMM

Re: [PATCH 0/3] Split TYPE_GPIOPWR and reuse in E500 machines

[Bernhard Beschow]

Am 30. September 2024 12:57:17 UTC schrieb Peter Maydell <peter.maydell@linaro.org>:
>On Mon, 23 Sept 2024 at 11:16, Bernhard Beschow <shentey@gmail.com> wrote:
>>
>> This series is part of a bigger series exploring data-driven machine creation
>> using device tree blobs on top of the e500 machines [1]. The idea is to
>> instantiate a QEMU device model for each device tree node containing a
>> compatible property. [1] achieves feature-parity with the hardcoded machines
>> when supplied the same device tree blob that the hardcoded machine would
>> generate.
>
>FWIW, on Arm I have generally pushed back against the idea
>of "create a QEMU machine from a device tree", because the
>device tree does not in general contain sufficient information
>to create a QEMU machine. It only has the information that
>Linux needs to use the devices, which is a subset of the
>total "how do all these devices exist and get wired together".
>(There are some special cases like some Xilinx FPGA systems,
>where both the device tree and the hardware were autogenerated
>from a common source definition, and so there's enough of
>a constraint on what the hardware can be to make it workable.)
>
>> Just like the ARM virt machine, the ppce500 machine implements a
>> "gpio-poweroff"-compatible device tree node. Unfortunately, the implementation
>> isn't shared which this series fixes. In order to reflect device tree which has
>> separate bindings for gpio-poweroff and gpio-reset, and to prepare for the
>> above, the gpio-pwr device model is split.
>>
>> Note: If the split seems too fine-grained, the existing gpio-pwr device model
>> could probably be reused in ppce500, too.
>
>I definitely like getting rid of the hand-coded
>qemu_allocate_irq() in e500.c. But I don't really see
>the benefit of splitting gpio_pwr into two devices.
>If you only need the power-off behaviour, you can
>leave the restart gpio input not connected to anything.

New series sent which removes qemu_allocate_irq() usage in e500.c: https://lore.kernel.org/qemu-devel/20241005100228.28094-1-shentey@gmail.com/

Best regards,
Bernhard

>
>thanks
>-- PMM