From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 0206A339AB for ; Tue, 30 Jul 2024 08:36:22 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=170.10.133.124 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722328584; cv=none; b=ETwaS+9V3OrB7l7a8FAYPo9CitdgkzgaZIW8ktrMOmpPdLIqZC0lBZICZvJ4AmSFogSVj/V9n3/JCVX6naDfK3P12GRmNr46Y2ULTZ3QzaBJda7p/TRS+EXof4+amGG1wTaqaHmn3Tz0fWOHiU+973fxDXeNRmRdlKT20m9cH/w= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1722328584; c=relaxed/simple; bh=/7lViDMvdXX/FqTeQJ4UtTX1jkoluWxs7QAJQdZtMDY=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=YjdUh80Af6u8hedcp8wEDgTTLSrtvbR2lzXli72gilfD6v33NU+NQyIdodwlWDSGfX2Mz4TUtgEWQUO4yN2SVm13K0IaG4cuVvBh6rR7V2bcIaV7VJNckVqr/LIFHrdV6dc0q1wa4sGM+Htt+gHFLWFfqCIZdVb2+plH2EvYQ1o= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com; spf=pass smtp.mailfrom=redhat.com; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b=M6f++keO; arc=none smtp.client-ip=170.10.133.124 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=redhat.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (1024-bit key) header.d=redhat.com header.i=@redhat.com header.b="M6f++keO" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1722328581; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=jZvxrDQmClMXIYrFtS427ByaFNgahNTrWr4i/x1An7w=; b=M6f++keO4P3+U3G+zZcQxCfsJJLinaIV7YOnCa3fBtVuV+wUjw2+5YKrEHyBchkqqgKg7k Y22SwbHMre5XetOQt8hyB5NzWXSwQiD3ojESn09uGMRmPv6lUiQRdGtm0wEEp76d4Skzs/ D6ajvvkEFBXIiOuHELHXDcdP3hSZ6q4= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-664-Dw8gv7nxN9ihr3HeVfzn0Q-1; Tue, 30 Jul 2024 04:36:19 -0400 X-MC-Unique: Dw8gv7nxN9ihr3HeVfzn0Q-1 Received: by mail-wm1-f69.google.com with SMTP id 5b1f17b1804b1-4281b7196bbso17528475e9.0 for ; Tue, 30 Jul 2024 01:36:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1722328578; x=1722933378; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=jZvxrDQmClMXIYrFtS427ByaFNgahNTrWr4i/x1An7w=; b=vpWx8LJb/CZCCtinm7GlLIbw/yp8rlA3pnb8S9uFDUPKoA1vINuyHMIgKBkXGxl33r na1SGUeS0nnNvJU9bdiiwp95dR0DlcJHBxLp//vo8knzXm0m/9o4uCNfxnpu5NQyfnn0 4tF50t/6H6i1PG6jyJ17TaYJePsW5P51pPW7zkcPGQIksuW4XOrQI2X5GYiP3j19ocXq dMC1lifLS3Zl5i5s6nMAMgxwPDfiuy0/4i9d4TiAbyaPagM9TzJEsyArEBRBcClg3HVk 2iLZNDl7gx0isWD4cy67CZjz46UGwNQUTK6lAql6pLIq+XQf+CF53bz9i2L5HEr4FONF OFzA== X-Forwarded-Encrypted: i=1; AJvYcCXG5SYaIcmN/IHd0zN2XI767C+lj215S0jYBWL4bnLt5Lepw6DpqQHl0qrXM7l9K/txvUt9sfiYiPD5t253QVSR8ZXY7RZWWUS3i5gC X-Gm-Message-State: AOJu0Yyypw6Ohk0ETYP9vRpCtD4yXtFH1E9lc4oItD1gLYJ/jbw2zZqZ WYk53MXly7c2plwnX3QVLldkicO0QCF8qP+dC8POm4BfR4A8RAmEMqhk9uY8qCGTWD1StLaqv3z HnmHHV0WNXdT2i/WXGysaq3JtlAE5ERpDgdMdUljEm38xYodvukX/pczTdojJHQ== X-Received: by 2002:adf:e707:0:b0:367:9088:fecc with SMTP id ffacd0b85a97d-36b5d0b04a6mr7637661f8f.7.1722328578269; Tue, 30 Jul 2024 01:36:18 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFX4uoGu0ltyAIFOKyUsPySdzb1wHut0ofsSOLJt5xD+DWRL85w43RjRJ8JmCYg6vsfKakvPQ== X-Received: by 2002:adf:e707:0:b0:367:9088:fecc with SMTP id ffacd0b85a97d-36b5d0b04a6mr7637636f8f.7.1722328577802; Tue, 30 Jul 2024 01:36:17 -0700 (PDT) Received: from imammedo.users.ipa.redhat.com (nat-pool-brq-t.redhat.com. [213.175.37.10]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-36b36861b11sm14072497f8f.96.2024.07.30.01.36.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 30 Jul 2024 01:36:17 -0700 (PDT) Date: Tue, 30 Jul 2024 10:36:15 +0200 From: Igor Mammedov To: Mauro Carvalho Chehab Cc: Jonathan Cameron , Shiju Jose , "Michael S. Tsirkin" , Philippe =?UTF-8?B?TWF0aGlldS1EYXVkw6k=?= , Ani Sinha , Eduardo Habkost , Marcel Apfelbaum , Peter Maydell , Shannon Zhao , Yanan Wang , linux-kernel@vger.kernel.org, qemu-arm@nongnu.org, qemu-devel@nongnu.org, shameerali.kolothum.thodi@huawei.com Subject: Re: [PATCH v3 2/7] arm/virt: Wire up GPIO error source for ACPI / GHES Message-ID: <20240730103615.5bb7613a@imammedo.users.ipa.redhat.com> In-Reply-To: <034a7e86761e09996001394c98ffb8201ac52cd2.1721630625.git.mchehab+huawei@kernel.org> References: <034a7e86761e09996001394c98ffb8201ac52cd2.1721630625.git.mchehab+huawei@kernel.org> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.43; x86_64-redhat-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 22 Jul 2024 08:45:54 +0200 Mauro Carvalho Chehab wrote: > From: Jonathan Cameron > > 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. 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 > Signed-off-by: Mauro Carvalho Chehab > --- > 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); > }; > > /**