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]
-=-=-=-=-=-=-=-=-=-=-=-