From: "Sami Mujawar" <sami.mujawar@arm.com>
To: Himanshu Sharma <Himanshu.Sharma@arm.com>, devel@edk2.groups.io
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>,
Leif Lindholm <quic_llindhol@quicinc.com>,
Pierre Gondois <Pierre.Gondois@arm.com>,
"nd@arm.com" <nd@arm.com>
Subject: Re: [edk2-devel] [edk2][PATCH V2 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only
Date: Thu, 4 Jan 2024 16:03:15 +0000 [thread overview]
Message-ID: <746a5922-4e75-4150-9e53-2c9c660055e4@arm.com> (raw)
In-Reply-To: <20240104080257.319631-3-Himanshu.Sharma@arm.com>
[-- Attachment #1: Type: text/plain, Size: 10124 bytes --]
Hi Himanshu,
There are some minor comments marked inline as [SAMI], otherwise this
patch looks good to me.
I can fix those up before merging the patch.
With that,
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Regards,
Sami Mujawar
On 04/01/2024 08:02 am, Himanshu Sharma wrote:
> Add interrupt node to the AML description of the serial-port only if the
> IRQ ID from the Configuration Manager is a valid SPI (shared processor
> interrupt) or an extended SPI. So, for DBG2 UART ports where interrupt
> is not mandatory, adding of an interrupt node in the AML description
> using Serial Port Fixup Library can be ignored if the UART is not
> defined with a valid SPI, like in N1SDP.
>
> This update generates the interrupt node for the valid SPI range using
> the AML Codegen API instead of updating it using the AML Fixup API.
>
> Signed-off-by: Himanshu Sharma<Himanshu.Sharma@arm.com>
> ---
> DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf | 3 +-
> DynamicTablesPkg/Include/ArmNameSpaceObjects.h | 6 ++-
> DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c | 49 ++++++++++++++------
> DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl | 29 +++++++-----
> 4 files changed, 58 insertions(+), 29 deletions(-)
>
> diff --git a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf
> index 965167bdc4e1..5e2615c961ad 100644
> --- a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf
> +++ b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf
> @@ -1,7 +1,7 @@
> ## @file
>
> # SSDT Serial Port fixup Library
>
> #
>
> -# Copyright (c) 2020 - 2021, Arm Limited. All rights reserved.<BR>
>
> +# Copyright (c) 2020 - 2021, 2024, Arm Limited. All rights reserved.<BR>
[SAMI] I missed this in my previous review, but we really do not need to
extend the copyright here.
>
> #
>
> # SPDX-License-Identifier: BSD-2-Clause-Patent
>
> ##
>
> @@ -19,6 +19,7 @@
> SsdtSerialPortTemplate.asl
>
>
>
> [Packages]
>
> + ArmPkg/ArmPkg.dec
>
> MdePkg/MdePkg.dec
>
> MdeModulePkg/MdeModulePkg.dec
>
> EmbeddedPkg/EmbeddedPkg.dec
>
> diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> index 8c00bdac20bb..e9df0ec94808 100644
> --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> @@ -1,6 +1,6 @@
> /** @file
>
>
>
> - Copyright (c) 2017 - 2023, Arm Limited. All rights reserved.<BR>
>
> + Copyright (c) 2017 - 2024, Arm Limited. All rights reserved.<BR>
>
>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -311,7 +311,9 @@ typedef struct CmArmSerialPortInfo {
> /// The physical base address for the serial port
>
> UINT64 BaseAddress;
>
>
>
> - /// The serial port interrupt
>
> + /** The serial port interrupt
>
> + to be speciifed 0 if the serial port does not have an interrupt wired.
[SAMI] There is a typo here. Also would it be better to say
' 0 indicates that the serial port does not
have an interrupt wired.
'
[/SAMI]
>
> + */
>
> UINT32 Interrupt;
>
>
>
> /// The serial port baud rate
>
> diff --git a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c
> index a65c1fe7e30d..d0b1f61fdf85 100644
> --- a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c
> +++ b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c
> @@ -1,7 +1,7 @@
> /** @file
>
> SSDT Serial Port Fixup Library.
>
>
>
> - Copyright (c) 2019 - 2021, Arm Limited. All rights reserved.<BR>
>
> + Copyright (c) 2019 - 2021, 2024, Arm Limited. All rights reserved.<BR>
[SAMI] the end year needs to be extended, i.e. Copyright (c) 2019 -
2024, Arm Limited. All rights reserved.<BR>
>
>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -9,10 +9,14 @@
> - Arm Server Base Boot Requirements (SBBR), s4.2.1.8 "SPCR".
>
> - Microsoft Debug Port Table 2 (DBG2) Specification - December 10, 2015.
>
> - ACPI for Arm Components 1.0 - 2020
>
> + - Arm Generic Interrupt Controller Architecture Specification,
>
> + Issue H, January 2022.
>
> + (https://developer.arm.com/documentation/ihi0069/)
>
> **/
>
>
>
> #include <IndustryStandard/DebugPort2Table.h>
>
> #include <Library/AcpiLib.h>
>
> +#include <Library/ArmGicArchLib.h>
>
> #include <Library/BaseLib.h>
>
> #include <Library/BaseMemoryLib.h>
>
> #include <Library/DebugLib.h>
>
> @@ -270,7 +274,6 @@ FixupCrs (
> EFI_STATUS Status;
>
> AML_OBJECT_NODE_HANDLE NameOpCrsNode;
>
> AML_DATA_NODE_HANDLE QWordRdNode;
>
> - AML_DATA_NODE_HANDLE InterruptRdNode;
>
>
>
> // Get the "_CRS" object defined by the "Name ()" statement.
>
> Status = AmlFindNode (
>
> @@ -303,20 +306,38 @@ FixupCrs (
> return Status;
>
> }
>
>
>
> - // Get the Interrupt node.
>
> - // It is the second Resource Data element in the NameOpCrsNode's
>
> - // variable list of arguments.
>
> - Status = AmlNameOpGetNextRdNode (QWordRdNode, &InterruptRdNode);
>
> - if (EFI_ERROR (Status)) {
>
> - return Status;
>
> + // Generate an interrupt node as the second Resource Data element in the
>
> + // NameOpCrsNode, if the interrupt for the serial-port is a valid SPI from
>
> + // Table 2-1 in Arm Generic Interrupt Controller Architecture Specification.
>
> + if (((SerialPortInfo->Interrupt >= ARM_GIC_ARCH_SPI_MIN) &&
>
> + (SerialPortInfo->Interrupt <= ARM_GIC_ARCH_SPI_MAX)) ||
>
> + ((SerialPortInfo->Interrupt >= ARM_GIC_ARCH_EXT_SPI_MIN) &&
>
> + (SerialPortInfo->Interrupt <= ARM_GIC_ARCH_EXT_SPI_MAX)))
>
> + {
>
> + Status = AmlCodeGenRdInterrupt (
>
> + TRUE, // Resource Consumer
>
> + FALSE, // Level Triggered
>
> + FALSE, // Active High
>
> + FALSE, // Exclusive
>
> + (UINT32 *)&SerialPortInfo->Interrupt,
>
> + 1,
>
> + NameOpCrsNode,
>
> + NULL
>
> + );
>
> + ASSERT_EFI_ERROR (Status);
>
> + } else if (SerialPortInfo->Interrupt != 0) {
>
> + // If an interrupt is not wired to the serial port, the
>
> + // Configuration Manager specifies the interrupt as 0.
>
> + // Any other value must be treated as an error.
>
> + DEBUG ((
>
> + DEBUG_ERROR,
>
> + "ERROR: SSDT-SERIAL-PORT-FIXUP: Invalid interrupt ID for Serial Port\n"
>
> + ));
>
> + ASSERT (0);
>
> + Status = EFI_INVALID_PARAMETER;
>
> }
>
>
>
> - if (InterruptRdNode == NULL) {
>
> - return EFI_INVALID_PARAMETER;
>
> - }
>
> -
>
> - // Update the interrupt number.
>
> - return AmlUpdateRdInterrupt (InterruptRdNode, SerialPortInfo->Interrupt);
>
> + return Status;
>
> }
>
>
>
> /** Fixup the Serial Port device name.
>
> diff --git a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl
> index fcae2160ac3d..22e6c04e020c 100644
> --- a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl
> +++ b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl
> @@ -1,7 +1,7 @@
> /** @file
>
> SSDT Serial Template
>
>
>
> - Copyright (c) 2019 - 2020, Arm Limited. All rights reserved.<BR>
>
> + Copyright (c) 2019 - 2020, 2024, Arm Limited. All rights reserved.<BR>
[SAMI] Same comment as above.
>
>
>
> SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
>
> @@ -10,6 +10,7 @@
>
>
> @par Glossary:
>
> - {template} - Data fixed up using AML Fixup APIs.
>
> + - {codegen} - Data generated using AML Codegen APIs.
>
> **/
>
>
>
> DefinitionBlock ("SsdtSerialPortTemplate.aml", "SSDT", 2, "ARMLTD", "SERIAL", 1) {
>
> @@ -43,17 +44,21 @@ DefinitionBlock ("SsdtSerialPortTemplate.aml", "SSDT", 2, "ARMLTD", "SERIAL", 1)
> , // MemoryRangeType
>
> // TranslationType
>
> ) // QWordMemory
>
> - Interrupt (
>
> - ResourceConsumer, // ResourceUsage
>
> - Level, // EdgeLevel
>
> - ActiveHigh, // ActiveLevel
>
> - Exclusive, // Shared
>
> - , // ResourceSourceIndex
>
> - , // ResourceSource
>
> - // DescriptorName
>
> - ) {
>
> - 0xA5 // {template}
>
> - } // Interrupt
>
> +
>
> + // The Interrupt information is generated using AmlCodegen.
>
> + //
>
> + // Interrupt ( // {codegen}
>
> + // ResourceConsumer, // ResourceUsage
>
> + // Level, // EdgeLevel
>
> + // ActiveHigh, // ActiveLevel
>
> + // Exclusive, // Shared
>
> + // , // ResourceSourceIndex
>
> + // , // ResourceSource
>
> + // // DescriptorName
>
> + // ) {
>
> + // <IRQ> // <spi>
>
> + // } // Interrupt
>
> +
>
> }) // Name
>
> } // Device
>
> } // Scope (_SB)
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113186): https://edk2.groups.io/g/devel/message/113186
Mute This Topic: https://groups.io/mt/103518973/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
[-- Attachment #2: Type: text/html, Size: 11974 bytes --]
next prev parent reply other threads:[~2024-01-04 16:03 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-01-04 8:02 [edk2-devel] [edk2][PATCH V2 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library Himanshu Sharma
2024-01-04 8:02 ` [edk2-devel] [edk2][PATCH V2 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges Himanshu Sharma
2024-01-04 15:59 ` Sami Mujawar
2024-02-28 14:45 ` Sami Mujawar
2024-02-28 16:59 ` Sami Mujawar
2024-03-01 14:34 ` Ard Biesheuvel
2024-01-04 8:02 ` [edk2-devel] [edk2][PATCH V2 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only Himanshu Sharma
2024-01-04 16:03 ` Sami Mujawar [this message]
2024-02-05 10:54 ` [edk2-devel] [edk2][PATCH V2 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library PierreGondois
2024-03-04 10:34 ` Sami Mujawar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=746a5922-4e75-4150-9e53-2c9c660055e4@arm.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox