Em Tue, 30 Jul 2024 10:36:15 +0200 Igor Mammedov <[email protected]> escreveu:
> On Mon, 22 Jul 2024 08:45:54 +0200 > Mauro Carvalho Chehab <[email protected]> wrote: > > > From: Jonathan Cameron <[email protected]> > > > > Creates a GED - Generic Event Device and set a GPIO to > > be used or error injection. > > QEMU already has GED device, so question is why it wasn't > used for event delivery? > I nutshell, I'd really prefer this series being rewritten > to reuse exiting GED instead of adding ad hoc GPIO and ACPI > plumbing. Makes sense. I'll split this one on two patches, the first one adding the error device PNP to acpi/generic_event_device, and the second one with ghes and arm virt changes to support it, using a notifier list inside ghes to signalize the error events. Jonathan, As the logic will be different, I'm placing you as co-author, and adding you as Cc on the patches. If you're ok with that, please reply with your SoB to them when I submit the next patch series. > PS: > as side effect of that, error injection could be used no only for > ARM but other machines that use GED (providing they implement GHES) > > Also CCing Shameer wrt touched power button code > > > [mchehab: use a define for the generic event pin number and do some > > cleanups] > > Signed-off-by: Jonathan Cameron <[email protected]> > > Signed-off-by: Mauro Carvalho Chehab <[email protected]> > > --- > > hw/arm/virt-acpi-build.c | 30 ++++++++++++++++++++++++++---- > > hw/arm/virt.c | 14 ++++++++++++-- > > include/hw/arm/virt.h | 1 + > > include/hw/boards.h | 1 + > > 4 files changed, 40 insertions(+), 6 deletions(-) > > > > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c > > index f76fb117adff..c502ccf40909 100644 > > --- a/hw/arm/virt-acpi-build.c > > +++ b/hw/arm/virt-acpi-build.c > > @@ -63,6 +63,7 @@ > > > > #define ARM_SPI_BASE 32 > > > > +#define ACPI_GENERIC_EVENT_DEVICE "GEDD" > > #define ACPI_BUILD_TABLE_SIZE 0x20000 > > > > static void acpi_dsdt_add_cpus(Aml *scope, VirtMachineState *vms) > > @@ -142,6 +143,8 @@ static void acpi_dsdt_add_pci(Aml *scope, const > > MemMapEntry *memmap, > > static void acpi_dsdt_add_gpio(Aml *scope, const MemMapEntry *gpio_memmap, > > uint32_t gpio_irq) > > this function supposed to be called when acpi_dev is not present (exiting GED > device) > and run on old machines only, so it should not be called for recent machine > types. > I'd avoid adding anything to it. > > see more comment about it below > > > { > > + uint32_t pin; > > + > > Aml *dev = aml_device("GPO0"); > > aml_append(dev, aml_name_decl("_HID", aml_string("ARMH0061"))); > > aml_append(dev, aml_name_decl("_UID", aml_int(0))); > > @@ -155,7 +158,12 @@ static void acpi_dsdt_add_gpio(Aml *scope, const > > MemMapEntry *gpio_memmap, > > > > Aml *aei = aml_resource_template(); > > > > - const uint32_t pin = GPIO_PIN_POWER_BUTTON; > > + pin = GPIO_PIN_POWER_BUTTON; > > + aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH, > > + AML_EXCLUSIVE, AML_PULL_UP, 0, &pin, 1, > > + "GPO0", NULL, 0)); > > + /* Pin for generic error */ > > + pin = GPIO_PIN_GENERIC_ERROR; > > aml_append(aei, aml_gpio_int(AML_CONSUMER, AML_EDGE, AML_ACTIVE_HIGH, > > AML_EXCLUSIVE, AML_PULL_UP, 0, &pin, 1, > > "GPO0", NULL, 0)); > > @@ -166,6 +174,11 @@ static void acpi_dsdt_add_gpio(Aml *scope, const > > MemMapEntry *gpio_memmap, > > aml_append(method, aml_notify(aml_name(ACPI_POWER_BUTTON_DEVICE), > > aml_int(0x80))); > > aml_append(dev, method); > > + method = aml_method("_E06", 0, AML_NOTSERIALIZED); > > + aml_append(method, aml_notify(aml_name(ACPI_GENERIC_EVENT_DEVICE), > > + aml_int(0x80))); > > + aml_append(dev, method); > > + > > aml_append(scope, dev); > > } > > > > @@ -800,6 +813,15 @@ static void build_fadt_rev6(GArray *table_data, > > BIOSLinker *linker, > > build_fadt(table_data, linker, &fadt, vms->oem_id, vms->oem_table_id); > > } > > > > +static void acpi_dsdt_add_generic_event_device(Aml *scope) > > +{ > > + Aml *dev = aml_device(ACPI_GENERIC_EVENT_DEVICE); > > + aml_append(dev, aml_name_decl("_HID", aml_string("PNP0C33"))); > this is not _event_ device, it's referred as _error_ device in spec. > > PS: > please properly document new ACPI primitives/devices, > see comment above aml_notify() for example. > Use earliest APIC spec where the device was defined for the 1st time. > > > + aml_append(dev, aml_name_decl("_UID", aml_int(0))); > > + aml_append(dev, aml_name_decl("_STA", aml_int(0xF))); > > + aml_append(scope, dev); > > +} > > + > > /* DSDT */ > > static void > > build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms) > > @@ -841,10 +863,9 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > VirtMachineState *vms) > > HOTPLUG_HANDLER(vms->acpi_dev), > > irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, > > AML_SYSTEM_MEMORY, > > memmap[VIRT_ACPI_GED].base); > > - } else { > > - acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO], > > - (irqmap[VIRT_GPIO] + ARM_SPI_BASE)); > > } > > + acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO], > > + (irqmap[VIRT_GPIO] + ARM_SPI_BASE)); > > wouldn't that create double/conflicting power button handlers > (GPIO and GED one), on recent machine types GED should be used > and power button in acpi_dsdt_add_gpio() is used only if > machine doesn't have GED. > > > > > if (vms->acpi_dev) { > > uint32_t event = object_property_get_uint(OBJECT(vms->acpi_dev), > > @@ -858,6 +879,7 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, > > VirtMachineState *vms) > > } > > > > acpi_dsdt_add_power_button(scope); > > + acpi_dsdt_add_generic_event_device(scope); > > #ifdef CONFIG_TPM > > acpi_dsdt_add_tpm(scope, vms); > > #endif > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c > > index c99c8b1713c6..f81cf3a69961 100644 > > --- a/hw/arm/virt.c > > +++ b/hw/arm/virt.c > > @@ -997,6 +997,13 @@ static void create_rtc(const VirtMachineState *vms) > > } > > > > static DeviceState *gpio_key_dev; > > + > > +static DeviceState *gpio_error_dev; > > +static void virt_set_error(void) > > +{ > > + qemu_set_irq(qdev_get_gpio_in(gpio_error_dev, 0), 1); > > +} > > + > > static void virt_powerdown_req(Notifier *n, void *opaque) > > { > > VirtMachineState *s = container_of(n, VirtMachineState, > > powerdown_notifier); > > @@ -1015,6 +1022,9 @@ static void create_gpio_keys(char *fdt, DeviceState > > *pl061_dev, > > gpio_key_dev = sysbus_create_simple("gpio-key", -1, > > qdev_get_gpio_in(pl061_dev, > > > > GPIO_PIN_POWER_BUTTON)); > > + gpio_error_dev = sysbus_create_simple("gpio-key", -1, > > + qdev_get_gpio_in(pl061_dev, > > + > > GPIO_PIN_GENERIC_ERROR)); > > > > qemu_fdt_add_subnode(fdt, "/gpio-keys"); > > qemu_fdt_setprop_string(fdt, "/gpio-keys", "compatible", "gpio-keys"); > > @@ -2385,9 +2395,8 @@ static void machvirt_init(MachineState *machine) > > > > if (has_ged && aarch64 && firmware_loaded && > > virt_is_acpi_enabled(vms)) { > > vms->acpi_dev = create_acpi_ged(vms); > > - } else { > > - create_gpio_devices(vms, VIRT_GPIO, sysmem); > > } > > + create_gpio_devices(vms, VIRT_GPIO, sysmem); > > again, this create duplicate/conflicting power button source > > > > > if (vms->secure && !vmc->no_secure_gpio) { > > create_gpio_devices(vms, VIRT_SECURE_GPIO, secure_sysmem); > > @@ -3101,6 +3110,7 @@ static void virt_machine_class_init(ObjectClass *oc, > > void *data) > > mc->default_ram_id = "mach-virt.ram"; > > mc->default_nic = "virtio-net-pci"; > > > > + mc->set_error = virt_set_error; > > object_class_property_add(oc, "acpi", "OnOffAuto", > > virt_get_acpi, virt_set_acpi, > > NULL, NULL); > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h > > index a4d937ed45ac..c9769d7d4d7f 100644 > > --- a/include/hw/arm/virt.h > > +++ b/include/hw/arm/virt.h > > @@ -49,6 +49,7 @@ > > > > /* GPIO pins */ > > #define GPIO_PIN_POWER_BUTTON 3 > > +#define GPIO_PIN_GENERIC_ERROR 6 > > > > enum { > > VIRT_FLASH, > > diff --git a/include/hw/boards.h b/include/hw/boards.h > > index ef6f18f2c1a7..6cf01f3934ae 100644 > > --- a/include/hw/boards.h > > +++ b/include/hw/boards.h > > @@ -304,6 +304,7 @@ struct MachineClass { > > const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine); > > int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx); > > ram_addr_t (*fixup_ram_size)(ram_addr_t size); > > + void (*set_error)(void); > > }; > > > > /** > Thanks, Mauro
