From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by mx.groups.io with SMTP id smtpd.web12.7304.1643471539493051471 for ; Sat, 29 Jan 2022 07:52:20 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=sPvJfXc9; spf=pass (domain: kernel.org, ip: 145.40.68.75, mailfrom: ardb@kernel.org) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 6B2D9B827CE for ; Sat, 29 Jan 2022 15:52:17 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 0AD05C340E5 for ; Sat, 29 Jan 2022 15:52:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1643471536; bh=piVPHZI6pZ3/Qft89v18ba5Izk/5eVGTE1QnUnpjhIM=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=sPvJfXc9FjwqKWeEgCsus/V30N/zGLv1Nq754zHqf//wI+LkGmHNNhTpvZtCtUoSX /TfUo9L60PyNxYOCfeWkLPGHY7RRuxazP8k0FXMcs0mYXIprNTUPRaA8MoxfqzrraY 02JmgHynisfB3WpKETyONgq8IWUe6T3gGv/9DuzVHsZqzoX96Ms2QYlMTEIlGMbBfW dHJj6WjWKwOTxLwZs+gk6SMT0UPQxGF3bOc3zt6YsnnPXTuXkYCZ3VHH4eecw9V9lX uQTha2i21gm0aoVWM2TfAcO02zgv6+7gG5PqXroQJYl4UL9Eq8e8nqzt/nTUWw9FYH EWL3LEC+TkaOQ== Received: by mail-wr1-f45.google.com with SMTP id e2so16776150wra.2 for ; Sat, 29 Jan 2022 07:52:15 -0800 (PST) X-Gm-Message-State: AOAM531yznUAlnHlYaPI91RuEW+Yei3N2wpB/QcHtzQQKfbD1K6YP0BB puDwbLfTts735jIvy1EiX7v5aXngr07jVQ777cE= X-Google-Smtp-Source: ABdhPJxva4sSFncsUyAUFjZZotrU9q8fCRkTuAOB8muaXWHu9kO5t5zaOQg2SJZyMpLLMjda2//++DzSZo8Prry99Wg= X-Received: by 2002:a05:6000:1107:: with SMTP id z7mr10851216wrw.189.1643471534310; Sat, 29 Jan 2022 07:52:14 -0800 (PST) MIME-Version: 1.0 References: <20220128154103.20752-1-Pierre.Gondois@arm.com> <20220128154103.20752-4-Pierre.Gondois@arm.com> In-Reply-To: <20220128154103.20752-4-Pierre.Gondois@arm.com> From: "Ard Biesheuvel" Date: Sat, 29 Jan 2022 16:52:02 +0100 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3 3/8] DynamicTablesPkg: AcpiSsdtPcieLibArm: Fix _PRT description To: Pierre , Marc Zyngier Cc: edk2-devel-groups-io , Ard Biesheuvel , Sami Mujawar Content-Type: text/plain; charset="UTF-8" (+ Marc) On Fri, 28 Jan 2022 at 16:41, wrote: > > From: Pierre Gondois > > In ACPI 6.4, s6.2.13, _PRT objects describing PCI legacy interrupts > can be defined following 2 models. > In the first model, a _SRS object must be described to modify the PCI > interrupt. The _CRS and _PRS object allows to describe the PCI > interrupt (level/edge triggered, active high/low). > In the second model, the PCI interrupt cannot be described with a > similar granularity. PCI interrupts are by default level triggered, > active low. > > GicV2 SPI interrupts are level or edge triggered, active high. To > correctly describe PCI interrupts, the first model is used, even though > Arm Base Boot Requirements v1.0 requires to use the second mode. > There are two different issues here: - using separate 'interrupt link' device objects with an Interrupt() resource rather than a simple GSIV number - whether _PRS and _SRS need to be implemented on those link objects. The latter is simply not true - _PRS and _SRS are optional, and pointless if there is only a single possible value, so there is really no need to add them here. As for the choice between the link object or the GSIV number: I don't think INTx interrupts on ARM systems are actually level low, and the GSIV option is widely used, also in platforms that exist in edk2-platforms, without any reported issues. I've cc'ed Marc, perhaps he can shed some light on this, but I'd prefer to stick to the GSIV approach if we can, as it is much simpler. > Signed-off-by: Pierre Gondois > --- > > Notes: > v3: > - New patch. [Pierre] > > .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 47 +++++++++++++++++-- > 1 file changed, 44 insertions(+), 3 deletions(-) > > diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c > index 3e22587d4a25..6823a98795c8 100644 > --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c > +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c > @@ -1,7 +1,7 @@ > /** @file > SSDT Pcie Table Generator. > > - Copyright (c) 2021, Arm Limited. All rights reserved.
> + Copyright (c) 2021 - 2022, Arm Limited. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -295,6 +295,10 @@ GeneratePciDeviceInfo ( > Method (_CRS, 0) { > Return CRS0 > }) > + Method (_PRS, 0) { > + Return CRS0 > + }) > + Method (_SRS, 1) { } > Method (_DIS) { } > } > > @@ -302,9 +306,12 @@ GeneratePciDeviceInfo ( > PCI Firmware Specification - Revision 3.3, > s3.5. "Device State at Firmware/Operating System Handoff" > > - The _PRS and _SRS are not supported, cf Arm Base Boot Requirements v1.0: > + Warning: The Arm Base Boot Requirements v1.0 states the following: > "The _PRS (Possible Resource Settings) and _SRS (Set Resource Settings) > are not supported." > + However, the first model to describe PCI legacy interrupts is used (cf. ACPI > + 6.4 s6.2.13) as PCI defaults (Level Triggered, Active Low) are not compatible > + with GICv2 (must be Active High). > > @param [in] Irq Interrupt controller interrupt. > @param [in] IrqFlags Interrupt flags. > @@ -416,7 +423,41 @@ GenerateLinkDevice ( > return Status; > } > > - // ASL:Method (_DIS, 1) {} > + // ASL: > + // Method (_PRS, 0) { > + // Return (CRS0) > + // } > + Status = AmlCodeGenMethodRetNameString ( > + "_PRS", > + "CRS0", > + 0, > + FALSE, > + 0, > + LinkNode, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + return Status; > + } > + > + // ASL:Method (_SRS, 1) {} > + // Not possible to disable interrupts. > + Status = AmlCodeGenMethodRetNameString ( > + "_SRS", > + NULL, > + 1, > + FALSE, > + 0, > + LinkNode, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + ASSERT (0); > + return Status; > + } > + > + // ASL:Method (_DIS, 0) {} > // Not possible to disable interrupts. > Status = AmlCodeGenMethodRetNameString ( > "_DIS", > -- > 2.25.1 >