[Qemu-devel] [PATCH v3 0/8] Add system_powerdown support on ARM for ACPI and DT

[Qemu-devel] [PATCH v3 0/8] Add system_powerdown support on ARM for ACPI and DT

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

ACPI SPEC 5.0 defines GPIO-signaled ACPI Events for Hardware-reduced
platforms(like ARM). It uses GPIO pin to trigger an event to the guest.
For QEMU, here we add PL061 GPIO controller and use PIN 3 for
system_powerdown, reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and
memory hotplug.

This patchset adds system_powerdown support on ARM through both ACPI and
DT ways. It adds a GPIO controller(here is PL061) in machine virt and
uses GPIO-singled event for ACPI while gpio-keys for DT. It can be
fetched from [1] and has been tested for the guests starting by ACPI or
DT while guests use systemd or acpid.

a) ACPI way. Since Graeme send a patchset to make ACPI on ARM64 support
amba device[2], it could use PL061 directly without modification to its
kernel driver code. In addition, we should use ACPI to start VM,
referring to below script. QEMU_EFI.fd can be fetched from [3]. 

    qemu-system-aarch64 \
    -smp 1 -m 1024 -M virt \
    -cpu cortex-a57 -nographic \
    -monitor telnet::4444,server,nowait \
    -bios QEMU_EFI.fd \
    -kernel Image \
    -initrd guestfs.cpio.gz \
    -append "rdinit=/sbin/init console=ttyAMA0 mem=512M root=/dev/ram 
earlycon=pl011,0x9000000 rw acpi=force"

b) DT way. Start vm as usual.

    qemu-system-aarch64 \
    -smp 1 -m 1024 -M virt \
    -cpu cortex-a57 -nographic \
    -monitor telnet::4444,server,nowait \
    -kernel Image \
    -initrd guestfs.cpio.gz \
    -append "rdinit=/sbin/init console=ttyAMA0 mem=512M root=/dev/ram 
earlycon=pl011,0x9000000 rw"

Guest internal setup:
If your guest FS uses systemd, you should check file
/lib/udev/rules.d/70-power-switch.rules and add the following line in it
if it doesn't exist.

SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform",
ATTRS{keys}=="116", TAG+="power-switch"

If your guest FS uses acpid, you should check it has
/etc/acpi/powerbtn.sh and /etc/acpi/events/powerbtn. Refer to [4] for
the setup of acpid.

Thanks,
Shannon

[1] https://git.linaro.org/people/shannon.zhao/qemu.git/shortlog/refs/heads/PowerButton_v2
[2] https://lkml.org/lkml/2015/9/30/392
[3] http://people.linaro.org/~shannon.zhao/ACPI_ARM/QEMU_EFI.fd
[4] https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/GPIOPowerButton

Changes since v2:
* rebased on upstream QEMU
* adjust irq signalling for PowerButton of PATCH 7/8 (Peter Maydell)
* Add several Reviewed-bys and Tested-bys from Wei Huang, thanks!

Changes since v1:
* rewrite GPIO Connection Descriptor (Michael)

Shannon Zhao (8):
  hw/arm/virt: Add a GPIO controller
  hw/arm/virt-acpi-build: Add GPIO controller in ACPI DSDT table
  hw/arm/virt-acpi-build: Add power button device in ACPI DSDT table
  hw/acpi/aml-build: Add GPIO Connection Descriptor
  hw/acpi/aml-build: Add a wrapper for GPIO Interrupt Connection
  hw/arm/virt-acpi-build: Add _E03 for Power Button
  hw/arm/virt: Add QEMU powerdown notifier and hook it to GPIO Pin 3
  hw/arm/virt: Add gpio-keys node for Poweroff using DT

 hw/acpi/aml-build.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++
 hw/arm/virt-acpi-build.c    | 45 ++++++++++++++++++++++++++
 hw/arm/virt.c               | 59 +++++++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h | 26 +++++++++++++++
 include/hw/arm/virt.h       |  1 +
 5 files changed, 210 insertions(+)

-- 
2.1.0

[Qemu-devel] [PATCH v3 1/8] hw/arm/virt: Add a GPIO controller

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

ACPI 5.0 supports GPIO-signaled ACPI Events. This can be used for
powerdown, hotplug evnets. Add a GPIO controller in machine virt,
to support powerdown, maybe can be used for cpu hotplug. And
here we use pl061.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Reviewed-by: Wei Huang <wei@redhat.com>
Tested-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt.c         | 30 ++++++++++++++++++++++++++++++
 include/hw/arm/virt.h |  1 +
 2 files changed, 31 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 9c6792c..7977e3c 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -120,6 +120,7 @@ static const MemMapEntry a15memmap[] = {
     [VIRT_UART] =               { 0x09000000, 0x00001000 },
     [VIRT_RTC] =                { 0x09010000, 0x00001000 },
     [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
+    [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -135,6 +136,7 @@ static const int a15irqmap[] = {
     [VIRT_UART] = 1,
     [VIRT_RTC] = 2,
     [VIRT_PCIE] = 3, /* ... to 6 */
+    [VIRT_GPIO] = 7,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
     [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
@@ -538,6 +540,32 @@ static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
     g_free(nodename);
 }
 
+static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
+{
+    char *nodename;
+    hwaddr base = vbi->memmap[VIRT_GPIO].base;
+    hwaddr size = vbi->memmap[VIRT_GPIO].size;
+    int irq = vbi->irqmap[VIRT_GPIO];
+    const char compat[] = "arm,pl061\0arm,primecell";
+
+    sysbus_create_simple("pl061", base, pic[irq]);
+
+    nodename = g_strdup_printf("/pl061@%" PRIx64, base);
+    qemu_fdt_add_subnode(vbi->fdt, nodename);
+    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
+                                 2, base, 2, size);
+    qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat, sizeof(compat));
+    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#gpio-cells", 2);
+    qemu_fdt_setprop(vbi->fdt, nodename, "gpio-controller", NULL, 0);
+    qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupts",
+                           GIC_FDT_IRQ_TYPE_SPI, irq,
+                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
+    qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
+    qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
+
+    g_free(nodename);
+}
+
 static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
 {
     int i;
@@ -1041,6 +1069,8 @@ static void machvirt_init(MachineState *machine)
 
     create_pcie(vbi, pic, vms->highmem);
 
+    create_gpio(vbi, pic);
+
     /* Create mmio transports, so the user can create virtio backends
      * (which will be automatically plugged in to the transports). If
      * no backend is created the transport will just sit harmlessly idle.
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index f464586..925faa7 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -59,6 +59,7 @@ enum {
     VIRT_PCIE_ECAM,
     VIRT_PLATFORM_BUS,
     VIRT_PCIE_MMIO_HIGH,
+    VIRT_GPIO,
 };
 
 typedef struct MemMapEntry {
-- 
2.1.0

[Qemu-devel] [PATCH v3 2/8] hw/arm/virt-acpi-build: Add GPIO controller in ACPI DSDT table

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add GPIO controller in ACPI DSDT table. It can be used for gpio event.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Tested-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt-acpi-build.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 3c2c5d6..bf6b934 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -323,6 +323,23 @@ static void acpi_dsdt_add_pci(Aml *scope, const MemMapEntry *memmap, int irq,
     aml_append(scope, dev);
 }
 
+static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
+                                           int gpio_irq)
+{
+    Aml *dev = aml_device("GPO0");
+    aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061")));
+    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
+    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
+
+    Aml *crs = aml_resource_template();
+    aml_append(crs, aml_memory32_fixed(gpio_memmap->base, gpio_memmap->size,
+                                       AML_READ_WRITE));
+    aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
+                                  AML_EXCLUSIVE, gpio_irq));
+    aml_append(dev, aml_name_decl("_CRS", crs));
+    aml_append(scope, dev);
+}
+
 /* RSDP */
 static GArray *
 build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
@@ -539,6 +556,8 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
                     (irqmap[VIRT_MMIO] + ARM_SPI_BASE), NUM_VIRTIO_TRANSPORTS);
     acpi_dsdt_add_pci(scope, memmap, (irqmap[VIRT_PCIE] + ARM_SPI_BASE),
                       guest_info->use_highmem);
+    acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
+                       (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
 
     aml_append(dsdt, scope);
 
-- 
2.1.0

[Qemu-devel] [PATCH v3 3/8] hw/arm/virt-acpi-build: Add power button device in ACPI DSDT table

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add power button device in ACPI DSDT table.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Reviewed-by: Wei Huang <wei@redhat.com>
Tested-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt-acpi-build.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index bf6b934..b25c90b 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -340,6 +340,18 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
     aml_append(scope, dev);
 }
 
+static void acpi_dsdt_add_power_button(Aml *scope)
+{
+    Aml *dev = aml_device("PWRB");
+    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
+    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
+    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
+    Aml *method = aml_method("_STA", 0);
+    aml_append(method, aml_return(aml_int(0x0F)));
+    aml_append(dev, method);
+    aml_append(scope, dev);
+}
+
 /* RSDP */
 static GArray *
 build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
@@ -558,6 +570,7 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
                       guest_info->use_highmem);
     acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
                        (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
+    acpi_dsdt_add_power_button(scope);
 
     aml_append(dsdt, scope);
 
-- 
2.1.0

[Qemu-devel] [PATCH v3 4/8] hw/acpi/aml-build: Add GPIO Connection Descriptor

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Tested-by: Wei Huang <wei@redhat.com>
---
 hw/acpi/aml-build.c         | 61 +++++++++++++++++++++++++++++++++++++++++++++
 include/hw/acpi/aml-build.h | 20 +++++++++++++++
 2 files changed, 81 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index a00a0ab..60d4703 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -565,6 +565,67 @@ Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml *arg4)
 }
 
 /*
+ * ACPI 5.0: 6.4.3.8.1 GPIO Connection Descriptor
+ * Type 1, Large Item Name 0xC
+ */
+
+static Aml *aml_gpio_connection(AmlGpioConnectionType type,
+                                AmlConsumerAndProducer con_and_pro,
+                                uint8_t flags, AmlPinConfig pin_cfg,
+                                int16_t output_drive, int16_t debounce_timeout,
+                                int32_t pin_num[], int32_t pin_count,
+                                const char *name, const char *vendor_data)
+{
+    Aml *var = aml_alloc();
+    uint16_t name_len = name ? (strlen(name) + 1) : 0;
+    uint16_t vendor_data_len = vendor_data ? (strlen(vendor_data) + 1) : 0;
+    uint16_t length = 0x16 + name_len + vendor_data_len;
+    uint16_t name_offset = 0x17 + pin_count * 2;
+    uint16_t vendor_data_offset = name_offset + name_len;
+    int i;
+
+    build_append_byte(var->buf, 0x8C);  /* GpioInt Resource Descriptor */
+    build_append_int_noprefix(var->buf, length, 2); /* Length */
+    build_append_byte(var->buf, 1);     /* Revision ID */
+    build_append_byte(var->buf, type);  /* GPIO Connection Type */
+    /* General Flags (2 bytes) */
+    build_append_int_noprefix(var->buf, con_and_pro, 2);
+    /* Interrupt and IO Flags (2 bytes) */
+    build_append_int_noprefix(var->buf, flags, 2);
+    /* Pin Configuration 0 = Default 1 = Pull-up 2 = Pull-down 3 = No Pull */
+    build_append_byte(var->buf, pin_cfg);
+    /* Output Drive Strength (2 bytes) */
+    build_append_int_noprefix(var->buf, output_drive, 2);
+    /* Debounce Timeout (2 bytes) */
+    build_append_int_noprefix(var->buf, debounce_timeout, 2);
+    /* Pin Table Offset (2 bytes) */
+    build_append_int_noprefix(var->buf, 0x17, 2);
+    build_append_byte(var->buf, 0);     /* Resource Source Index */
+    /* Resource Source Name Offset (2 bytes) */
+    build_append_int_noprefix(var->buf, name_offset, 2);
+    /* Vendor Data Offset (2 bytes) */
+    build_append_int_noprefix(var->buf, vendor_data_offset, 2);
+    /* Vendor Data Length (2 bytes) */
+    build_append_int_noprefix(var->buf, vendor_data_len, 2);
+    /* Pin Number (2n bytes)*/
+    for (i = 0; i < pin_count; i++) {
+        build_append_int_noprefix(var->buf, pin_num[i], 2);
+    }
+    /* Resource Source Name */
+    if (name != NULL) {
+        build_append_namestring(var->buf, "%s", name);
+        build_append_byte(var->buf, '\0');
+    }
+    /* Vendor-defined Data */
+    if (vendor_data != NULL) {
+        build_append_namestring(var->buf, "%s", vendor_data);
+        build_append_byte(var->buf, '\0');
+    }
+
+    return var;
+}
+
+/*
  * ACPI 1.0b: 6.4.3.4 32-Bit Fixed Location Memory Range Descriptor
  * (Type 1, Large Item Name 0x6)
  */
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 1b632dc..4e88882 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -148,6 +148,26 @@ typedef enum {
     AML_SHARED_AND_WAKE = 3,
 } AmlShared;
 
+/*
+ * ACPI 5.0: Table 6-189 GPIO Connection Descriptor Definition
+ * GPIO Connection Type
+ */
+typedef enum {
+    AML_INTERRUPT_CONNECTION = 0,
+    AML_IO_CONNECTION = 1,
+} AmlGpioConnectionType;
+
+/*
+ * ACPI 5.0: Table 6-189 GPIO Connection Descriptor Definition
+ * _PPI field definition
+ */
+typedef enum {
+    AML_DEFAULT_CONFIG = 0,
+    AML_PULL_UP = 1,
+    AML_PULL_DOWN = 2,
+    AML_NO_PULL = 3,
+} AmlPinConfig;
+
 typedef
 struct AcpiBuildTables {
     GArray *table_data;
-- 
2.1.0

[Qemu-devel] [PATCH v3 5/8] hw/acpi/aml-build: Add a wrapper for GPIO Interrupt Connection

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Tested-by: Wei Huang <wei@redhat.com>
---
 hw/acpi/aml-build.c         | 18 ++++++++++++++++++
 include/hw/acpi/aml-build.h |  6 ++++++
 2 files changed, 24 insertions(+)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 60d4703..8e18b22 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -626,6 +626,24 @@ static Aml *aml_gpio_connection(AmlGpioConnectionType type,
 }
 
 /*
+ * ACPI 5.0: 19.5.53
+ * GpioInt(GPIO Interrupt Connection Resource Descriptor Macro)
+ */
+Aml *aml_gpio_int(AmlConsumerAndProducer con_and_pro,
+                  AmlLevelAndEdge level_and_edge,
+                  AmlActiveHighAndLow high_and_low, AmlShared shared,
+                  AmlPinConfig pin_cfg, int16_t debounce_timeout,
+                  int32_t pin_num[], int32_t pin_count, const char *name,
+                  const char *vendor_data)
+{
+    uint8_t flags = level_and_edge | (high_and_low << 1) | (shared << 3);
+
+    return aml_gpio_connection(AML_INTERRUPT_CONNECTION, con_and_pro, flags,
+                               pin_cfg, 0, debounce_timeout, pin_num, pin_count,
+                               name, vendor_data);
+}
+
+/*
  * ACPI 1.0b: 6.4.3.4 32-Bit Fixed Location Memory Range Descriptor
  * (Type 1, Large Item Name 0x6)
  */
diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
index 4e88882..bce813b 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.h
@@ -232,6 +232,12 @@ Aml *aml_call1(const char *method, Aml *arg1);
 Aml *aml_call2(const char *method, Aml *arg1, Aml *arg2);
 Aml *aml_call3(const char *method, Aml *arg1, Aml *arg2, Aml *arg3);
 Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml *arg4);
+Aml *aml_gpio_int(AmlConsumerAndProducer con_and_pro,
+                  AmlLevelAndEdge level_and_edge,
+                  AmlActiveHighAndLow high_and_low, AmlShared shared,
+                  AmlPinConfig pin_cfg, int16_t debounce_timeout,
+                  int32_t pin_num[], int32_t pin_count, const char *name,
+                  const char *vendor_data);
 Aml *aml_memory32_fixed(uint32_t addr, uint32_t size,
                         AmlReadAndWrite read_and_write);
 Aml *aml_interrupt(AmlConsumerAndProducer con_and_pro,
-- 
2.1.0

[Qemu-devel] [PATCH v3 6/8] hw/arm/virt-acpi-build: Add _E03 for Power Button

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Here GPIO pin 3 is used for Power Button, add _E03 in ACPI DSDT table.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Tested-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt-acpi-build.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index b25c90b..12c9e8b 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -337,6 +337,19 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
     aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
                                   AML_EXCLUSIVE, gpio_irq));
     aml_append(dev, aml_name_decl("_CRS", crs));
+
+    Aml *aei = aml_resource_template();
+    /* Pin 3 for power button */
+    int32_t pin_num[1] = {3};
+    aml_append(aei, aml_gpio_int(AML_CONSUMER_PRODUCER, AML_EDGE,
+                                 AML_ACTIVE_HIGH, AML_EXCLUSIVE, AML_PULL_UP, 0,
+                                 pin_num, 1, "GPO0", NULL));
+    aml_append(dev, aml_name_decl("_AEI", aei));
+
+    /* _E03 is handle for power button */
+    Aml *method = aml_method("_E03", 0);
+    aml_append(method, aml_notify(aml_name("PWRB"), aml_int(0x80)));
+    aml_append(dev, method);
     aml_append(scope, dev);
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH v3 7/8] hw/arm/virt: Add QEMU powerdown notifier and hook it to GPIO Pin 3

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Currently mach-virt model doesn't support powerdown request. Guest VM
doesn't react to system_powerdown from monitor console (or QMP) because
there is no communication mechanism for such requests. This patch registers
GPIO Pin 3 with powerdown notification. So guest VM can receive notification
when such powerdown request is triggered.

Signed-off-by: Wei Huang <wei@redhat.com>
Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Reviewed-by: Wei Huang <wei@redhat.com>
Tested-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7977e3c..cb1d45b 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -540,6 +540,17 @@ static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
     g_free(nodename);
 }
 
+static DeviceState *pl061_dev;
+static void virt_powerdown_req(Notifier *n, void *opaque)
+{
+    /* use gpio Pin 3 for power button event */
+    qemu_set_irq(qdev_get_gpio_in(pl061_dev, 3), 1);
+}
+
+static Notifier virt_system_powerdown_notifier = {
+    .notify = virt_powerdown_req
+};
+
 static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
 {
     char *nodename;
@@ -548,7 +559,7 @@ static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
     int irq = vbi->irqmap[VIRT_GPIO];
     const char compat[] = "arm,pl061\0arm,primecell";
 
-    sysbus_create_simple("pl061", base, pic[irq]);
+    pl061_dev = sysbus_create_simple("pl061", base, pic[irq]);
 
     nodename = g_strdup_printf("/pl061@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
@@ -563,6 +574,9 @@ static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
     qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
     qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
 
+    /* connect powerdown request */
+    qemu_register_powerdown_notifier(&virt_system_powerdown_notifier);
+
     g_free(nodename);
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH v3 8/8] hw/arm/virt: Add gpio-keys node for Poweroff using DT

[shannon.zhao]

From: Shannon Zhao <shannon.zhao@linaro.org>

Add a gpio-keys node. This is used for Poweroff for the systems which
use DT not ACPI.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Tested-by: Wei Huang <wei@redhat.com>
---
 hw/arm/virt.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index cb1d45b..3394262 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -561,6 +561,7 @@ static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
 
     pl061_dev = sysbus_create_simple("pl061", base, pic[irq]);
 
+    uint32_t phandle = qemu_fdt_alloc_phandle(vbi->fdt);
     nodename = g_strdup_printf("/pl061@%" PRIx64, base);
     qemu_fdt_add_subnode(vbi->fdt, nodename);
     qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
@@ -573,6 +574,20 @@ static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
                            GIC_FDT_IRQ_FLAGS_LEVEL_HI);
     qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
     qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
+    qemu_fdt_setprop_cell(vbi->fdt, nodename, "phandle", phandle);
+
+    qemu_fdt_add_subnode(vbi->fdt, "/gpio-keys");
+    qemu_fdt_setprop_string(vbi->fdt, "/gpio-keys", "compatible", "gpio-keys");
+    qemu_fdt_setprop_cell(vbi->fdt, "/gpio-keys", "#size-cells", 0);
+    qemu_fdt_setprop_cell(vbi->fdt, "/gpio-keys", "#address-cells", 1);
+    qemu_fdt_setprop(vbi->fdt, "/gpio-keys", "autorepeat", NULL, 0);
+
+    qemu_fdt_add_subnode(vbi->fdt, "/gpio-keys/poweroff");
+    qemu_fdt_setprop_string(vbi->fdt, "/gpio-keys/poweroff",
+                            "label", "GPIO Key Poweroff");
+    qemu_fdt_setprop_cell(vbi->fdt, "/gpio-keys/poweroff", "linux,code", 116);
+    qemu_fdt_setprop_cells(vbi->fdt, "/gpio-keys/poweroff",
+                           "gpios", phandle, 3, 0);
 
     /* connect powerdown request */
     qemu_register_powerdown_notifier(&virt_system_powerdown_notifier);
-- 
2.1.0

Re: [Qemu-devel] [PATCH v3 0/8] Add system_powerdown support on ARM for ACPI and DT

[Shannon Zhao]

Hi,

On 2015/11/16 21:23, shannon.zhao@linaro.org wrote:
> [1] https://git.linaro.org/people/shannon.zhao/qemu.git/shortlog/refs/heads/PowerButton_v2
Sorry, this link should be
https://git.linaro.org/people/shannon.zhao/qemu.git/shortlog/refs/heads/PowerButton_v3

> [2] https://lkml.org/lkml/2015/9/30/392
> [3] http://people.linaro.org/~shannon.zhao/ACPI_ARM/QEMU_EFI.fd
> [4] https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/GPIOPowerButton

-- 
Shannon

Re: [Qemu-devel] [PATCH v3 8/8] hw/arm/virt: Add gpio-keys node for Poweroff using DT

[Peter Maydell]

On 16 November 2015 at 13:23,  <shannon.zhao@linaro.org> wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
>
> Add a gpio-keys node. This is used for Poweroff for the systems which
> use DT not ACPI.
>
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Tested-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index cb1d45b..3394262 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -561,6 +561,7 @@ static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
>
>      pl061_dev = sysbus_create_simple("pl061", base, pic[irq]);
>
> +    uint32_t phandle = qemu_fdt_alloc_phandle(vbi->fdt);
>      nodename = g_strdup_printf("/pl061@%" PRIx64, base);
>      qemu_fdt_add_subnode(vbi->fdt, nodename);
>      qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
> @@ -573,6 +574,20 @@ static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
>                             GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>      qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
>      qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "phandle", phandle);
> +
> +    qemu_fdt_add_subnode(vbi->fdt, "/gpio-keys");
> +    qemu_fdt_setprop_string(vbi->fdt, "/gpio-keys", "compatible", "gpio-keys");
> +    qemu_fdt_setprop_cell(vbi->fdt, "/gpio-keys", "#size-cells", 0);
> +    qemu_fdt_setprop_cell(vbi->fdt, "/gpio-keys", "#address-cells", 1);
> +    qemu_fdt_setprop(vbi->fdt, "/gpio-keys", "autorepeat", NULL, 0);

Why would we want the power button to autorepeat ?

> +
> +    qemu_fdt_add_subnode(vbi->fdt, "/gpio-keys/poweroff");
> +    qemu_fdt_setprop_string(vbi->fdt, "/gpio-keys/poweroff",
> +                            "label", "GPIO Key Poweroff");
> +    qemu_fdt_setprop_cell(vbi->fdt, "/gpio-keys/poweroff", "linux,code", 116);

This could use a comment /* KEY_POWER */ to indicate what 116 is.
(Alternatively, see whether we can #include "standard-headers/linux/input.h"
which #defines KEY_POWER.)

> +    qemu_fdt_setprop_cells(vbi->fdt, "/gpio-keys/poweroff",
> +                           "gpios", phandle, 3, 0);
>
>      /* connect powerdown request */
>      qemu_register_powerdown_notifier(&virt_system_powerdown_notifier);

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 8/8] hw/arm/virt: Add gpio-keys node for Poweroff using DT

[Shannon Zhao]

On 2015/11/28 1:16, Peter Maydell wrote:
> On 16 November 2015 at 13:23,  <shannon.zhao@linaro.org> wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Add a gpio-keys node. This is used for Poweroff for the systems which
>> use DT not ACPI.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> Tested-by: Wei Huang <wei@redhat.com>
>> ---
>>  hw/arm/virt.c | 15 +++++++++++++++
>>  1 file changed, 15 insertions(+)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index cb1d45b..3394262 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -561,6 +561,7 @@ static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
>>
>>      pl061_dev = sysbus_create_simple("pl061", base, pic[irq]);
>>
>> +    uint32_t phandle = qemu_fdt_alloc_phandle(vbi->fdt);
>>      nodename = g_strdup_printf("/pl061@%" PRIx64, base);
>>      qemu_fdt_add_subnode(vbi->fdt, nodename);
>>      qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
>> @@ -573,6 +574,20 @@ static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
>>                             GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>>      qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
>>      qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
>> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "phandle", phandle);
>> +
>> +    qemu_fdt_add_subnode(vbi->fdt, "/gpio-keys");
>> +    qemu_fdt_setprop_string(vbi->fdt, "/gpio-keys", "compatible", "gpio-keys");
>> +    qemu_fdt_setprop_cell(vbi->fdt, "/gpio-keys", "#size-cells", 0);
>> +    qemu_fdt_setprop_cell(vbi->fdt, "/gpio-keys", "#address-cells", 1);
>> +    qemu_fdt_setprop(vbi->fdt, "/gpio-keys", "autorepeat", NULL, 0);
> 
> Why would we want the power button to autorepeat ?
> 
Oh, it doesn't need. Will remove it.

>> +
>> +    qemu_fdt_add_subnode(vbi->fdt, "/gpio-keys/poweroff");
>> +    qemu_fdt_setprop_string(vbi->fdt, "/gpio-keys/poweroff",
>> +                            "label", "GPIO Key Poweroff");
>> +    qemu_fdt_setprop_cell(vbi->fdt, "/gpio-keys/poweroff", "linux,code", 116);
> 
> This could use a comment /* KEY_POWER */ to indicate what 116 is.
> (Alternatively, see whether we can #include "standard-headers/linux/input.h"
> which #defines KEY_POWER.)
> 
Ok, it could include the linux/input.h here.

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH v3 3/8] hw/arm/virt-acpi-build: Add power button device in ACPI DSDT table

[Igor Mammedov]

On Mon, 16 Nov 2015 21:23:04 +0800
shannon.zhao@linaro.org wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Add power button device in ACPI DSDT table.
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Reviewed-by: Wei Huang <wei@redhat.com>
> Tested-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index bf6b934..b25c90b 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -340,6 +340,18 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
>      aml_append(scope, dev);
>  }
>  
> +static void acpi_dsdt_add_power_button(Aml *scope)
> +{
> +    Aml *dev = aml_device("PWRB");
> +    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> +    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> +    Aml *method = aml_method("_STA", 0);
> +    aml_append(method, aml_return(aml_int(0x0F)));
> +    aml_append(dev, method);
since _STA always returns 0xF you can just drop it altogether,
as _STA == 0xF is implied if it's not present.

> +    aml_append(scope, dev);
> +}
> +
>  /* RSDP */
>  static GArray *
>  build_rsdp(GArray *rsdp_table, GArray *linker, unsigned rsdt)
> @@ -558,6 +570,7 @@ build_dsdt(GArray *table_data, GArray *linker, VirtGuestInfo *guest_info)
>                        guest_info->use_highmem);
>      acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
>                         (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
> +    acpi_dsdt_add_power_button(scope);
>  
>      aml_append(dsdt, scope);
>  

Re: [Qemu-devel] [PATCH v3 3/8] hw/arm/virt-acpi-build: Add power button device in ACPI DSDT table

[Shannon Zhao]

Hi Igor,

On 2015/12/1 19:41, Igor Mammedov wrote:
> On Mon, 16 Nov 2015 21:23:04 +0800
> shannon.zhao@linaro.org wrote:
> 
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > Add power button device in ACPI DSDT table.
>> > 
>> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > Reviewed-by: Wei Huang <wei@redhat.com>
>> > Tested-by: Wei Huang <wei@redhat.com>
>> > ---
>> >  hw/arm/virt-acpi-build.c | 13 +++++++++++++
>> >  1 file changed, 13 insertions(+)
>> > 
>> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> > index bf6b934..b25c90b 100644
>> > --- a/hw/arm/virt-acpi-build.c
>> > +++ b/hw/arm/virt-acpi-build.c
>> > @@ -340,6 +340,18 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
>> >      aml_append(scope, dev);
>> >  }
>> >  
>> > +static void acpi_dsdt_add_power_button(Aml *scope)
>> > +{
>> > +    Aml *dev = aml_device("PWRB");
>> > +    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
>> > +    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
>> > +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>> > +    Aml *method = aml_method("_STA", 0);
>> > +    aml_append(method, aml_return(aml_int(0x0F)));
>> > +    aml_append(dev, method);
> since _STA always returns 0xF you can just drop it altogether,
> as _STA == 0xF is implied if it's not present.
> 

Yes, but I think adding this is harmless and make the return value explicit.

Thanks,
-- 
Shannon

Re: [Qemu-devel] [PATCH v3 3/8] hw/arm/virt-acpi-build: Add power button device in ACPI DSDT table

[Igor Mammedov]

On Tue, 1 Dec 2015 20:43:36 +0800
Shannon Zhao <zhaoshenglong@huawei.com> wrote:

> Hi Igor,
> 
> On 2015/12/1 19:41, Igor Mammedov wrote:
> > On Mon, 16 Nov 2015 21:23:04 +0800
> > shannon.zhao@linaro.org wrote:
> > 
> >> > From: Shannon Zhao <shannon.zhao@linaro.org>
> >> > 
> >> > Add power button device in ACPI DSDT table.
> >> > 
> >> > Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> >> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> >> > Reviewed-by: Wei Huang <wei@redhat.com>
> >> > Tested-by: Wei Huang <wei@redhat.com>
> >> > ---
> >> >  hw/arm/virt-acpi-build.c | 13 +++++++++++++
> >> >  1 file changed, 13 insertions(+)
> >> > 
> >> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> > index bf6b934..b25c90b 100644
> >> > --- a/hw/arm/virt-acpi-build.c
> >> > +++ b/hw/arm/virt-acpi-build.c
> >> > @@ -340,6 +340,18 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
> >> >      aml_append(scope, dev);
> >> >  }
> >> >  
> >> > +static void acpi_dsdt_add_power_button(Aml *scope)
> >> > +{
> >> > +    Aml *dev = aml_device("PWRB");
> >> > +    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
> >> > +    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
> >> > +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
> >> > +    Aml *method = aml_method("_STA", 0);
> >> > +    aml_append(method, aml_return(aml_int(0x0F)));
> >> > +    aml_append(dev, method);
> > since _STA always returns 0xF you can just drop it altogether,
> > as _STA == 0xF is implied if it's not present.
> > 
> 
> Yes, but I think adding this is harmless and make the return value explicit.
It's useless and consumes several bytes, it's better to drop it unless you have to have it.


> Thanks,

Re: [Qemu-devel] [PATCH v3 3/8] hw/arm/virt-acpi-build: Add power button device in ACPI DSDT table

[Shannon Zhao]

Hi Igor,

On 2015/12/1 21:05, Igor Mammedov wrote:
>>>>> +static void acpi_dsdt_add_power_button(Aml *scope)
>>>>> > >> > +{
>>>>> > >> > +    Aml *dev = aml_device("PWRB");
>>>>> > >> > +    aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C0C")));
>>>>> > >> > +    aml_append(dev, aml_name_decl("_ADR", aml_int(0)));
>>>>> > >> > +    aml_append(dev, aml_name_decl("_UID", aml_int(0)));
>>>>> > >> > +    Aml *method = aml_method("_STA", 0);
>>>>> > >> > +    aml_append(method, aml_return(aml_int(0x0F)));
>>>>> > >> > +    aml_append(dev, method);
>>> > > since _STA always returns 0xF you can just drop it altogether,
>>> > > as _STA == 0xF is implied if it's not present.
>>> > > 
>> > 
>> > Yes, but I think adding this is harmless and make the return value explicit.
> It's useless and consumes several bytes, it's better to drop it unless you have to have it.
> 
Sure, will drop it. :)

BTW, could you have a look at other ACPI patches in this series? Thanks
in advance!

-- 
Shannon

Re: [Qemu-devel] [PATCH v3 1/8] hw/arm/virt: Add a GPIO controller

[Igor Mammedov]

On Mon, 16 Nov 2015 21:23:02 +0800
shannon.zhao@linaro.org wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> ACPI 5.0 supports GPIO-signaled ACPI Events. This can be used for
> powerdown, hotplug evnets. Add a GPIO controller in machine virt,
s/evnets/events/

> to support powerdown, maybe can be used for cpu hotplug. And
> here we use pl061.
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Reviewed-by: Wei Huang <wei@redhat.com>
> Tested-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt.c         | 30 ++++++++++++++++++++++++++++++
>  include/hw/arm/virt.h |  1 +
>  2 files changed, 31 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 9c6792c..7977e3c 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -120,6 +120,7 @@ static const MemMapEntry a15memmap[] = {
>      [VIRT_UART] =               { 0x09000000, 0x00001000 },
>      [VIRT_RTC] =                { 0x09010000, 0x00001000 },
>      [VIRT_FW_CFG] =             { 0x09020000, 0x00000018 },
> +    [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -135,6 +136,7 @@ static const int a15irqmap[] = {
>      [VIRT_UART] = 1,
>      [VIRT_RTC] = 2,
>      [VIRT_PCIE] = 3, /* ... to 6 */
> +    [VIRT_GPIO] = 7,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>      [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
>      [VIRT_PLATFORM_BUS] = 112, /* ...to 112 + PLATFORM_BUS_NUM_IRQS -1 */
> @@ -538,6 +540,32 @@ static void create_rtc(const VirtBoardInfo *vbi, qemu_irq *pic)
>      g_free(nodename);
>  }
>  
> +static void create_gpio(const VirtBoardInfo *vbi, qemu_irq *pic)
> +{
> +    char *nodename;
> +    hwaddr base = vbi->memmap[VIRT_GPIO].base;
> +    hwaddr size = vbi->memmap[VIRT_GPIO].size;
> +    int irq = vbi->irqmap[VIRT_GPIO];
> +    const char compat[] = "arm,pl061\0arm,primecell";
> +
> +    sysbus_create_simple("pl061", base, pic[irq]);
> +

vvvvv
> +    nodename = g_strdup_printf("/pl061@%" PRIx64, base);
> +    qemu_fdt_add_subnode(vbi->fdt, nodename);
> +    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
> +                                 2, base, 2, size);
> +    qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat, sizeof(compat));
> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#gpio-cells", 2);
> +    qemu_fdt_setprop(vbi->fdt, nodename, "gpio-controller", NULL, 0);
> +    qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupts",
> +                           GIC_FDT_IRQ_TYPE_SPI, irq,
> +                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
> +    qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
> +
> +    g_free(nodename);
could you split off DT parts into a separate patch pls.

> +}
> +
>  static void create_virtio_devices(const VirtBoardInfo *vbi, qemu_irq *pic)
>  {
>      int i;
> @@ -1041,6 +1069,8 @@ static void machvirt_init(MachineState *machine)
>  
>      create_pcie(vbi, pic, vms->highmem);
>  
> +    create_gpio(vbi, pic);
> +
>      /* Create mmio transports, so the user can create virtio backends
>       * (which will be automatically plugged in to the transports). If
>       * no backend is created the transport will just sit harmlessly idle.
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index f464586..925faa7 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -59,6 +59,7 @@ enum {
>      VIRT_PCIE_ECAM,
>      VIRT_PLATFORM_BUS,
>      VIRT_PCIE_MMIO_HIGH,
> +    VIRT_GPIO,
>  };
>  
>  typedef struct MemMapEntry {

Re: [Qemu-devel] [PATCH v3 0/8] Add system_powerdown support on ARM for ACPI and DT

[Igor Mammedov]

On Mon, 16 Nov 2015 21:23:01 +0800
shannon.zhao@linaro.org wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> ACPI SPEC 5.0 defines GPIO-signaled ACPI Events for Hardware-reduced
> platforms(like ARM). It uses GPIO pin to trigger an event to the guest.
> For QEMU, here we add PL061 GPIO controller and use PIN 3 for
> system_powerdown, reserving PIN 0, 1, 2 for PCI hotplug, CPU hotplug and
> memory hotplug.
> 
> This patchset adds system_powerdown support on ARM through both ACPI and
> DT ways. It adds a GPIO controller(here is PL061) in machine virt and
> uses GPIO-singled event for ACPI while gpio-keys for DT. It can be
> fetched from [1] and has been tested for the guests starting by ACPI or
> DT while guests use systemd or acpid.
> 
> a) ACPI way. Since Graeme send a patchset to make ACPI on ARM64 support
> amba device[2], it could use PL061 directly without modification to its
> kernel driver code. In addition, we should use ACPI to start VM,
> referring to below script. QEMU_EFI.fd can be fetched from [3]. 
I'd rearrange/merge ACPI related patches in following order:

1. put new aml_foo() patches before adding any DSDT bits
2. add PWRB device as it doesn't have any dependencies
3. merge all GP00 patches in one and add it to DSDT,
   that way it's easier to read what patches do


[...]
> Guest internal setup:
> If your guest FS uses systemd, you should check file
> /lib/udev/rules.d/70-power-switch.rules and add the following line in it
> if it doesn't exist.
> 
> SUBSYSTEM=="input", KERNEL=="event*", SUBSYSTEMS=="platform",
> ATTRS{keys}=="116", TAG+="power-switch"
Could you tell me, what method is used to power off QEMU?

From ACPI SPEC it should be possible to use 
SLEEP_{CONTROL|STATUS}_REG and \_S5 for hw-reduced mode to
implement shutdown in a standard way but QEMU doesn't implement it
for ARM target and I'm not sure if linux kernel would support it either.

> 
> If your guest FS uses acpid, you should check it has
> /etc/acpi/powerbtn.sh and /etc/acpi/events/powerbtn. Refer to [4] for
> the setup of acpid.
> 
> Thanks,
> Shannon
> 
> [1] https://git.linaro.org/people/shannon.zhao/qemu.git/shortlog/refs/heads/PowerButton_v2
> [2] https://lkml.org/lkml/2015/9/30/392
> [3] http://people.linaro.org/~shannon.zhao/ACPI_ARM/QEMU_EFI.fd
> [4] https://wiki.linaro.org/LEG/Engineering/Kernel/ACPI/GPIOPowerButton
> 
> Changes since v2:
> * rebased on upstream QEMU
> * adjust irq signalling for PowerButton of PATCH 7/8 (Peter Maydell)
> * Add several Reviewed-bys and Tested-bys from Wei Huang, thanks!
> 
> Changes since v1:
> * rewrite GPIO Connection Descriptor (Michael)
> 
> Shannon Zhao (8):
>   hw/arm/virt: Add a GPIO controller
>   hw/arm/virt-acpi-build: Add GPIO controller in ACPI DSDT table
>   hw/arm/virt-acpi-build: Add power button device in ACPI DSDT table
>   hw/acpi/aml-build: Add GPIO Connection Descriptor
>   hw/acpi/aml-build: Add a wrapper for GPIO Interrupt Connection
>   hw/arm/virt-acpi-build: Add _E03 for Power Button
>   hw/arm/virt: Add QEMU powerdown notifier and hook it to GPIO Pin 3
>   hw/arm/virt: Add gpio-keys node for Poweroff using DT
> 
>  hw/acpi/aml-build.c         | 79 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/arm/virt-acpi-build.c    | 45 ++++++++++++++++++++++++++
>  hw/arm/virt.c               | 59 +++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h | 26 +++++++++++++++
>  include/hw/arm/virt.h       |  1 +
>  5 files changed, 210 insertions(+)
> 

Re: [Qemu-devel] [PATCH v3 1/8] hw/arm/virt: Add a GPIO controller

[Peter Maydell]

On 1 December 2015 at 13:21, Igor Mammedov <imammedo@redhat.com> wrote:
> On Mon, 16 Nov 2015 21:23:02 +0800
> shannon.zhao@linaro.org wrote:
>
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> ACPI 5.0 supports GPIO-signaled ACPI Events. This can be used for
>> powerdown, hotplug evnets. Add a GPIO controller in machine virt,
> s/evnets/events/
>
>> to support powerdown, maybe can be used for cpu hotplug. And
>> here we use pl061.
>>


> vvvvv
>> +    nodename = g_strdup_printf("/pl061@%" PRIx64, base);
>> +    qemu_fdt_add_subnode(vbi->fdt, nodename);
>> +    qemu_fdt_setprop_sized_cells(vbi->fdt, nodename, "reg",
>> +                                 2, base, 2, size);
>> +    qemu_fdt_setprop(vbi->fdt, nodename, "compatible", compat, sizeof(compat));
>> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "#gpio-cells", 2);
>> +    qemu_fdt_setprop(vbi->fdt, nodename, "gpio-controller", NULL, 0);
>> +    qemu_fdt_setprop_cells(vbi->fdt, nodename, "interrupts",
>> +                           GIC_FDT_IRQ_TYPE_SPI, irq,
>> +                           GIC_FDT_IRQ_FLAGS_LEVEL_HI);
>> +    qemu_fdt_setprop_cell(vbi->fdt, nodename, "clocks", vbi->clock_phandle);
>> +    qemu_fdt_setprop_string(vbi->fdt, nodename, "clock-names", "apb_pclk");
>> +
>> +    g_free(nodename);
> could you split off DT parts into a separate patch pls.

I think they belong in this patch. We should never add a device
to the 'virt' board without having the DT binding for it too.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v3 1/8] hw/arm/virt: Add a GPIO controller

[Pavel Fedin]

 Hello!

> >> ACPI 5.0 supports GPIO-signaled ACPI Events. This can be used for
> >> powerdown, hotplug evnets. Add a GPIO controller in machine virt,
> > s/evnets/events/
> >
> >> to support powerdown, maybe can be used for cpu hotplug. And
> >> here we use pl061.

 Sorry for late jumping in, but this was the first message Cc'ed to me.
 With these devices virt machine IMHO goes farther and farther away from its initial goal: be a minimalistic virtual box, which ensures maximum possible compatibility and portability.
 virt machine already supports poweroff using PSCI interface. Why we need to add more hardware? Can't ACPI deal with PSCI?
 To tell the truth, i dislike ACPI + EFI thing at all. It looks like cramming PC-oriented firmware into architecture for which it was never meant to be written. Too much overcomplications, we drop already established things and reinvent a (triangular) wheel, but what's the purpose? Is it being done only because vendors want obscure proprietary firmware instead of old good u-boot?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v3 1/8] hw/arm/virt: Add a GPIO controller

[Shannon Zhao]

On 2015/12/1 22:48, Pavel Fedin wrote:
>   Hello!
>
>>>> ACPI 5.0 supports GPIO-signaled ACPI Events. This can be used for
>>>> powerdown, hotplug evnets. Add a GPIO controller in machine virt,
>>> s/evnets/events/
>>>
>>>> to support powerdown, maybe can be used for cpu hotplug. And
>>>> here we use pl061.
>
>   Sorry for late jumping in, but this was the first message Cc'ed to me.
>   With these devices virt machine IMHO goes farther and farther away from its initial goal: be a minimalistic virtual box, which ensures maximum possible compatibility and portability.
>   virt machine already supports poweroff using PSCI interface. Why we need to add more hardware? Can't ACPI deal with PSCI?
Hi Pavel,

IIUC, the poweroff using PSCI interface is the guest *internal* 
poweroff, that is you type "poweroff" inside guest. But this patch set 
is going to introduce a way to support *external* poweroff, like the 
power button on our desktop. This is something like you push a button to 
shutdown the guest.

>   To tell the truth, i dislike ACPI + EFI thing at all. It looks like cramming PC-oriented firmware into architecture for which it was never meant to be written. Too much overcomplications, we drop already established things and reinvent a (triangular) wheel, but what's the purpose? Is it being done only because vendors want obscure proprietary firmware instead of old good u-boot?
>
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
>

-- 
Shannon

Re: [Qemu-devel] [PATCH v3 1/8] hw/arm/virt: Add a GPIO controller

[Wei Huang]

On 12/01/2015 08:48 AM, Pavel Fedin wrote:
>  Hello!
> 
>>>> ACPI 5.0 supports GPIO-signaled ACPI Events. This can be used for
>>>> powerdown, hotplug evnets. Add a GPIO controller in machine virt,
>>> s/evnets/events/
>>>
>>>> to support powerdown, maybe can be used for cpu hotplug. And
>>>> here we use pl061.
> 
>  Sorry for late jumping in, but this was the first message Cc'ed to me.
>  With these devices virt machine IMHO goes farther and farther away from its initial goal: be a minimalistic virtual box, which ensures maximum possible compatibility and portability.
>  virt machine already supports poweroff using PSCI interface. Why we need to add more hardware? Can't ACPI deal with PSCI?

PSCI handles the actions initiated from the inside of OS. Examples
include system shutdown and hotplug (still inside OS). From this
perspective PSCI works well. However this communication is
one-direction: there isn't a way to communicate from the outside (e.g.
libvirt) to the guest OS. For instance, "system_powerdown" qmp command
won't work because guest OS can't receive the notification.

>  To tell the truth, i dislike ACPI + EFI thing at all. It looks like cramming PC-oriented firmware into architecture for which it was never meant to be written. Too much overcomplications, we drop already established things and reinvent a (triangular) wheel, but what's the purpose? Is it being done only because vendors want obscure proprietary firmware instead of old good u-boot?
> 
> Kind regards,
> Pavel Fedin
> Expert Engineer
> Samsung Electronics Research center Russia
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH v3 1/8] hw/arm/virt: Add a GPIO controller

[Pavel Fedin]

 Hello!

> PSCI handles the actions initiated from the inside of OS. Examples
> include system shutdown and hotplug (still inside OS). From this
> perspective PSCI works well. However this communication is
> one-direction: there isn't a way to communicate from the outside (e.g.
> libvirt) to the guest OS. For instance, "system_powerdown" qmp command
> won't work because guest OS can't receive the notification.

 Ah, so it is necessary for e.g. commanding a graceful shutdown using virt-manager's "shutdown" command? Good then, i've missed that.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [Qemu-devel] [PATCH v3 4/8] hw/acpi/aml-build: Add GPIO Connection Descriptor

[Igor Mammedov]

On Mon, 16 Nov 2015 21:23:05 +0800
shannon.zhao@linaro.org wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>

Subj can be shortened to:
 acpi: Add GPIO Connection Descriptor

> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Tested-by: Wei Huang <wei@redhat.com>
> ---
>  hw/acpi/aml-build.c         | 61 +++++++++++++++++++++++++++++++++++++++++++++
>  include/hw/acpi/aml-build.h | 20 +++++++++++++++
>  2 files changed, 81 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index a00a0ab..60d4703 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -565,6 +565,67 @@ Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml *arg4)
>  }
>  
>  /*
> + * ACPI 5.0: 6.4.3.8.1 GPIO Connection Descriptor
> + * Type 1, Large Item Name 0xC
> + */
> +
> +static Aml *aml_gpio_connection(AmlGpioConnectionType type,
> +                                AmlConsumerAndProducer con_and_pro,
> +                                uint8_t flags, AmlPinConfig pin_cfg,
> +                                int16_t output_drive, int16_t debounce_timeout,
> +                                int32_t pin_num[], int32_t pin_count,
Probably intFOO_t should be uintFOO_t.

s/pin_num/pin_list/
I've used a bit more complicated to make list of FOO integers, like this:
https://github.com/imammedo/qemu/commit/f6925e53aa2e0266a07dbb39ae17efbf13dba388

+    Aml *irqs = aml_interrupt_list();
+    aml_append_interrupt2list(irqs, uart_irq);
     aml_append(crs,
                aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
-                             AML_EXCLUSIVE, uart_irq));
+                             AML_EXCLUSIVE, irqs));

but I like simpler array way you're using here,

Michael do you have any objection to passing IRQ/PIN lists as arrays
like this patch does?

> +                                const char *name, const char *vendor_data)
s/name/resource_src_name/
s/const char *vendor_data/const uint8_t *vendor_data/

> +{
> +    Aml *var = aml_alloc();
> +    uint16_t name_len = name ? (strlen(name) + 1) : 0;
name doesn't seem to optional so case 'name == NULL' should be invalid,
add assert(name) and drop condition

> +    uint16_t vendor_data_len = vendor_data ? (strlen(vendor_data) + 1) : 0;
vendor_data is binary blob so you can't use strlen() on it.

> +    uint16_t length = 0x16 + name_len + vendor_data_len;
s/0x16/const min_desc_len = 0x16/

> +    uint16_t name_offset = 0x17 + pin_count * 2;
and then use it here and below for calculating pin_table_offset

> +    uint16_t vendor_data_offset = name_offset + name_len;
> +    int i;
> +
> +    build_append_byte(var->buf, 0x8C);  /* GpioInt Resource Descriptor */
CpioInt is wrong, it should be "GPIO Connection Descriptor"

> +    build_append_int_noprefix(var->buf, length, 2); /* Length */
> +    build_append_byte(var->buf, 1);     /* Revision ID */
> +    build_append_byte(var->buf, type);  /* GPIO Connection Type */
> +    /* General Flags (2 bytes) */
> +    build_append_int_noprefix(var->buf, con_and_pro, 2);
> +    /* Interrupt and IO Flags (2 bytes) */
> +    build_append_int_noprefix(var->buf, flags, 2);
> +    /* Pin Configuration 0 = Default 1 = Pull-up 2 = Pull-down 3 = No Pull */
> +    build_append_byte(var->buf, pin_cfg);
> +    /* Output Drive Strength (2 bytes) */
> +    build_append_int_noprefix(var->buf, output_drive, 2);
> +    /* Debounce Timeout (2 bytes) */
> +    build_append_int_noprefix(var->buf, debounce_timeout, 2);
> +    /* Pin Table Offset (2 bytes) */
> +    build_append_int_noprefix(var->buf, 0x17, 2);
> +    build_append_byte(var->buf, 0);     /* Resource Source Index */
> +    /* Resource Source Name Offset (2 bytes) */
> +    build_append_int_noprefix(var->buf, name_offset, 2);
> +    /* Vendor Data Offset (2 bytes) */
> +    build_append_int_noprefix(var->buf, vendor_data_offset, 2);
> +    /* Vendor Data Length (2 bytes) */
> +    build_append_int_noprefix(var->buf, vendor_data_len, 2);
> +    /* Pin Number (2n bytes)*/
> +    for (i = 0; i < pin_count; i++) {
> +        build_append_int_noprefix(var->buf, pin_num[i], 2);
> +    }
> +    /* Resource Source Name */
> +    if (name != NULL) {
name shouldn't be NULL ever, so drop it

> +        build_append_namestring(var->buf, "%s", name);
> +        build_append_byte(var->buf, '\0');
> +    }
> +    /* Vendor-defined Data */
> +    if (vendor_data != NULL) {
> +        build_append_namestring(var->buf, "%s", vendor_data);
> +        build_append_byte(var->buf, '\0');
> +    }
that's wrong, vendor_data is RawDataBuffer and not NameString
following would do the right thing:
  g_array_append_vals(var->buf, vendor_data, vendor_data_len);

> +
> +    return var;
> +}
> +
> +/*
>   * ACPI 1.0b: 6.4.3.4 32-Bit Fixed Location Memory Range Descriptor
>   * (Type 1, Large Item Name 0x6)
>   */
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 1b632dc..4e88882 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -148,6 +148,26 @@ typedef enum {
>      AML_SHARED_AND_WAKE = 3,
>  } AmlShared;
>  
> +/*
> + * ACPI 5.0: Table 6-189 GPIO Connection Descriptor Definition
> + * GPIO Connection Type
> + */
> +typedef enum {
> +    AML_INTERRUPT_CONNECTION = 0,
> +    AML_IO_CONNECTION = 1,
> +} AmlGpioConnectionType;
> +
> +/*
> + * ACPI 5.0: Table 6-189 GPIO Connection Descriptor Definition
> + * _PPI field definition
> + */
> +typedef enum {
> +    AML_DEFAULT_CONFIG = 0,
I'd suggest to use AML_PULL_DEFAULT as it's mentioned in spec (see GpioInt/GpioIO)

> +    AML_PULL_UP = 1,
> +    AML_PULL_DOWN = 2,
> +    AML_NO_PULL = 3,
Likewise AML_PULL_NONE

> +} AmlPinConfig;
> +
>  typedef
>  struct AcpiBuildTables {
>      GArray *table_data;

Re: [Qemu-devel] [PATCH v3 5/8] hw/acpi/aml-build: Add a wrapper for GPIO Interrupt Connection

[Igor Mammedov]

On Mon, 16 Nov 2015 21:23:06 +0800
shannon.zhao@linaro.org wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>

s/
hw/acpi/aml-build: Add a wrapper for GPIO Interrupt Connection
/
acpi: add aml_gpio_int() wrapper
/

> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Tested-by: Wei Huang <wei@redhat.com>
> ---
>  hw/acpi/aml-build.c         | 18 ++++++++++++++++++
>  include/hw/acpi/aml-build.h |  6 ++++++
>  2 files changed, 24 insertions(+)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 60d4703..8e18b22 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -626,6 +626,24 @@ static Aml *aml_gpio_connection(AmlGpioConnectionType type,
>  }
>  
>  /*
> + * ACPI 5.0: 19.5.53
> + * GpioInt(GPIO Interrupt Connection Resource Descriptor Macro)
> + */
> +Aml *aml_gpio_int(AmlConsumerAndProducer con_and_pro,
> +                  AmlLevelAndEdge level_and_edge,
to be closer to GpioInt descrition in spec:

s/level_and_edge/edge_level/

> +                  AmlActiveHighAndLow high_and_low, AmlShared shared,
s/high_and_low/active_level/
> +                  AmlPinConfig pin_cfg, int16_t debounce_timeout,
s/pin_cfg/pin_config/

> +                  int32_t pin_num[], int32_t pin_count, const char *name,
s/pin_num/pin_list/
s/name/resource_source{_name}/

> +                  const char *vendor_data)
> +{
> +    uint8_t flags = level_and_edge | (high_and_low << 1) | (shared << 3);
> +
> +    return aml_gpio_connection(AML_INTERRUPT_CONNECTION, con_and_pro, flags,
> +                               pin_cfg, 0, debounce_timeout, pin_num, pin_count,
> +                               name, vendor_data);
> +}
> +
> +/*
>   * ACPI 1.0b: 6.4.3.4 32-Bit Fixed Location Memory Range Descriptor
>   * (Type 1, Large Item Name 0x6)
>   */
> diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> index 4e88882..bce813b 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -232,6 +232,12 @@ Aml *aml_call1(const char *method, Aml *arg1);
>  Aml *aml_call2(const char *method, Aml *arg1, Aml *arg2);
>  Aml *aml_call3(const char *method, Aml *arg1, Aml *arg2, Aml *arg3);
>  Aml *aml_call4(const char *method, Aml *arg1, Aml *arg2, Aml *arg3, Aml *arg4);
> +Aml *aml_gpio_int(AmlConsumerAndProducer con_and_pro,
> +                  AmlLevelAndEdge level_and_edge,
> +                  AmlActiveHighAndLow high_and_low, AmlShared shared,
> +                  AmlPinConfig pin_cfg, int16_t debounce_timeout,
> +                  int32_t pin_num[], int32_t pin_count, const char *name,
> +                  const char *vendor_data);
>  Aml *aml_memory32_fixed(uint32_t addr, uint32_t size,
>                          AmlReadAndWrite read_and_write);
>  Aml *aml_interrupt(AmlConsumerAndProducer con_and_pro,

Re: [Qemu-devel] [PATCH v3 5/8] hw/acpi/aml-build: Add a wrapper for GPIO Interrupt Connection

[Igor Mammedov]

On Mon, 16 Nov 2015 21:23:06 +0800
shannon.zhao@linaro.org wrote:

[...]
> + * ACPI 5.0: 19.5.53
> + * GpioInt(GPIO Interrupt Connection Resource Descriptor Macro)
> + */
> +Aml *aml_gpio_int(AmlConsumerAndProducer con_and_pro,
> +                  AmlLevelAndEdge level_and_edge,
> +                  AmlActiveHighAndLow high_and_low, AmlShared shared,
> +                  AmlPinConfig pin_cfg, int16_t debounce_timeout,
> +                  int32_t pin_num[], int32_t pin_count, const char *name,
also we never going to change pin_num[], so could be const

[...]

Re: [Qemu-devel] [PATCH v3 6/8] hw/arm/virt-acpi-build: Add _E03 for Power Button

[Igor Mammedov]

On Mon, 16 Nov 2015 21:23:07 +0800
shannon.zhao@linaro.org wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Here GPIO pin 3 is used for Power Button, add _E03 in ACPI DSDT table.
> 
> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> Tested-by: Wei Huang <wei@redhat.com>
> ---
>  hw/arm/virt-acpi-build.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index b25c90b..12c9e8b 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -337,6 +337,19 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
>      aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
>                                    AML_EXCLUSIVE, gpio_irq));
>      aml_append(dev, aml_name_decl("_CRS", crs));
> +
> +    Aml *aei = aml_resource_template();
> +    /* Pin 3 for power button */
> +    int32_t pin_num[1] = {3};
> +    aml_append(aei, aml_gpio_int(AML_CONSUMER_PRODUCER, AML_EDGE,
> +                                 AML_ACTIVE_HIGH, AML_EXCLUSIVE, AML_PULL_UP, 0,
> +                                 pin_num, 1, "GPO0", NULL));
> +    aml_append(dev, aml_name_decl("_AEI", aei));
> +
> +    /* _E03 is handle for power button */
> +    Aml *method = aml_method("_E03", 0);
> +    aml_append(method, aml_notify(aml_name("PWRB"), aml_int(0x80)));
you use "PWRB" here the second time, perhaps it would be better to use macro
#define ACPI_POWER_BUTTON_DEVICE "PWRB"

> +    aml_append(dev, method);
>      aml_append(scope, dev);
>  }
>  

Re: [Qemu-devel] [PATCH v3 6/8] hw/arm/virt-acpi-build: Add _E03 for Power Button

[Shannon Zhao]

On 2015/12/4 22:10, Igor Mammedov wrote:
> On Mon, 16 Nov 2015 21:23:07 +0800
> shannon.zhao@linaro.org wrote:
> 
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Here GPIO pin 3 is used for Power Button, add _E03 in ACPI DSDT table.
>>
>> Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> Tested-by: Wei Huang <wei@redhat.com>
>> ---
>>  hw/arm/virt-acpi-build.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
>> index b25c90b..12c9e8b 100644
>> --- a/hw/arm/virt-acpi-build.c
>> +++ b/hw/arm/virt-acpi-build.c
>> @@ -337,6 +337,19 @@ static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap,
>>      aml_append(crs, aml_interrupt(AML_CONSUMER, AML_LEVEL, AML_ACTIVE_HIGH,
>>                                    AML_EXCLUSIVE, gpio_irq));
>>      aml_append(dev, aml_name_decl("_CRS", crs));
>> +
>> +    Aml *aei = aml_resource_template();
>> +    /* Pin 3 for power button */
>> +    int32_t pin_num[1] = {3};
>> +    aml_append(aei, aml_gpio_int(AML_CONSUMER_PRODUCER, AML_EDGE,
>> +                                 AML_ACTIVE_HIGH, AML_EXCLUSIVE, AML_PULL_UP, 0,
>> +                                 pin_num, 1, "GPO0", NULL));
>> +    aml_append(dev, aml_name_decl("_AEI", aei));
>> +
>> +    /* _E03 is handle for power button */
>> +    Aml *method = aml_method("_E03", 0);
>> +    aml_append(method, aml_notify(aml_name("PWRB"), aml_int(0x80)));
> you use "PWRB" here the second time, perhaps it would be better to use macro
> #define ACPI_POWER_BUTTON_DEVICE "PWRB"
> 
Sure, thanks!

-- 
Shannon