[PATCH v2 0/2] Reuse TYPE_GPIO_PWR in ppce500 machine

[PATCH v2 0/2] Reuse TYPE_GPIO_PWR in ppce500 machine

[Bernhard Beschow]

Just like the ARM virt machine, the ppce500 machine implements a
"gpio-poweroff"-compatible device tree node. So far, the implementations
aren't shared, and in addition, the ppce500 machine uses qemu_allocate_irq()
which leaks memory. This series fixes both by reusing TYPE_GPIO_PWR.

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.

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

Supersedes: 20240923101554.12900-1-shentey@gmail.com

Bernhard Beschow (2):
  MAINTAINERS: Add hw/gpio/gpio_pwr.c
  hw/ppc/e500: Reuse TYPE_GPIO_PWR

 MAINTAINERS    |  1 +
 hw/ppc/e500.c  | 16 ++++------------
 hw/ppc/Kconfig |  1 +
 3 files changed, 6 insertions(+), 12 deletions(-)

-- 
2.46.2

[PATCH v2 1/2] 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 34fffcb5be..104264f04f 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.2

[PATCH v2 2/2] hw/ppc/e500: Reuse TYPE_GPIO_PWR

[Bernhard Beschow]

Taking inspiration from the ARM virt machine, port away from
qemu_allocate_irq() by reusing TYPE_GPIO_PWR.

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

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 3bd12b54ab..7811c22e7b 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 *gpio_pwr_dev;
 
         dev = qdev_new("mpc8xxx_gpio");
         s = SYS_BUS_DEVICE(dev);
@@ -1096,8 +1087,9 @@ 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);
+        gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
+        qdev_connect_gpio_out(dev, 0, qdev_get_gpio_in_named(gpio_pwr_dev,
+                                                             "shutdown", 0));
     }
 
     /* Platform Bus Device */
diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index 5addad1124..89cabe5d53 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_PWR
     select OPENPIC
     select PFLASH_CFI01
     select PLATFORM_BUS
-- 
2.46.2

Re: [PATCH v2 2/2] hw/ppc/e500: Reuse TYPE_GPIO_PWR

[Philippe Mathieu-Daudé]

On 5/10/24 07:02, Bernhard Beschow wrote:
> Taking inspiration from the ARM virt machine, port away from
> qemu_allocate_irq() by reusing TYPE_GPIO_PWR.
> 
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/ppc/e500.c  | 16 ++++------------
>   hw/ppc/Kconfig |  1 +
>   2 files changed, 5 insertions(+), 12 deletions(-)


> @@ -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 *gpio_pwr_dev;

Can we keep a reference in PPCE500MachineState?

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

>   
>           dev = qdev_new("mpc8xxx_gpio");
>           s = SYS_BUS_DEVICE(dev);
> @@ -1096,8 +1087,9 @@ 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);
> +        gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
> +        qdev_connect_gpio_out(dev, 0, qdev_get_gpio_in_named(gpio_pwr_dev,
> +                                                             "shutdown", 0));
>       }

Re: [PATCH v2 2/2] hw/ppc/e500: Reuse TYPE_GPIO_PWR

[Bernhard Beschow]

Am 7. Oktober 2024 21:13:22 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 5/10/24 07:02, Bernhard Beschow wrote:
>> Taking inspiration from the ARM virt machine, port away from
>> qemu_allocate_irq() by reusing TYPE_GPIO_PWR.
>> 
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/ppc/e500.c  | 16 ++++------------
>>   hw/ppc/Kconfig |  1 +
>>   2 files changed, 5 insertions(+), 12 deletions(-)
>
>
>> @@ -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 *gpio_pwr_dev;
>
>Can we keep a reference in PPCE500MachineState?

I considered turning it into an embedded attribute, but decided against it, because 1/ the device isn't part of the SoC (and therefore ideally user-createable), 2/ only used by the ppce500 machine, 3/ would be inconsistent with surrounding code, and 4/ "gpio-pwr" would require exposing the struct first (ARM virt also has no embedded attribute). That all seemed like a lot of churn for what I want to achieve which is having the same solution for the same problem across two machines.

There is surely a lot of room for cleaning up e500 beyond my e500 cleanup series, but unless there is demand for it, I'd stop there (patches welcome). Now that this series doesn't touch ARM much it'd actually make sense to merge it there.

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

So, is creating an anonymous device okay for you? Does your R-b stand nevertheless?

Best regards,
Bernhard

>
>>             dev = qdev_new("mpc8xxx_gpio");
>>           s = SYS_BUS_DEVICE(dev);
>> @@ -1096,8 +1087,9 @@ 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);
>> +        gpio_pwr_dev = sysbus_create_simple("gpio-pwr", -1, NULL);
>> +        qdev_connect_gpio_out(dev, 0, qdev_get_gpio_in_named(gpio_pwr_dev,
>> +                                                             "shutdown", 0));
>>       }