[edk2-devel] [edk2][PATCH V2 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library

[edk2-devel] [edk2][PATCH V2 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library

[Himanshu Sharma]

Currently in the Dynamic Tables Framework, the interrupt node for the
AML description of the serial-ports is populated using the template
and so is mandatorily added even if the serial-port is enumerated as
a DBG2 port in the platform's configuration manager where the
interrupt is not mandatory. The proposed implementation adds the
interrupt node only if the interrupt defined for the serial-port is a
valid SPI or a valid extended SPI. So, in case of DBG2 ports, the
platforms with interrupt defined as SPI (like Morello) can have the
interrupt node added to the description and the platforms where it is
not defined (like N1SDP) can ignore the addition of the interrupt node.

The changes include adding the SPI range macros in ArmGicArchLib
(ArmPkg) which can be used by the SSDTSerialPortFixupLib
(DynamicTablesPkg) to put a check for generating the interrupt node
using AML Codegen API.

Change log:

V2:
  - Fix comments on V1
    - Add link to Arm GIC Specification.
    - Fix uncrustify errors.
    - Add a check more if the interrupt is 0,
      to be used when an interrupt is not wired to the serial port.
  - Update copyright year to 2024.
  - Link to branch with the patches in this series
    https://github.com/himsha01/edk2/tree/ssdt_serial_port_interrupt_v2

V1:
  - Link to branch with the patches in this series
    https://github.com/himsha01/edk2/tree/ssdt_serial_port_interrupt 

Himanshu Sharma (2):
  ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges
  DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only

 DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf |  3 +-
 ArmPkg/Include/Library/ArmGicArchLib.h                                            | 14 ++++++
 DynamicTablesPkg/Include/ArmNameSpaceObjects.h                                    |  6 ++-
 DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c   | 49 ++++++++++++++------
 DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl | 29 +++++++-----
 5 files changed, 72 insertions(+), 29 deletions(-)

-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113140): https://edk2.groups.io/g/devel/message/113140
Mute This Topic: https://groups.io/mt/103518971/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2][PATCH V2 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges

[Himanshu Sharma]

Taking reference from Table 2-1 of the Arm Generic Interrupt Controller
Architecture Specification, Issue H, January 2022, add macros for the
SPI and extended SPI ranges with the purpose of reusability on including
the ArmPkg.

Signed-off-by: Himanshu Sharma <Himanshu.Sharma@arm.com>
---
 ArmPkg/Include/Library/ArmGicArchLib.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/ArmPkg/Include/Library/ArmGicArchLib.h b/ArmPkg/Include/Library/ArmGicArchLib.h
index 72ac17e13b5a..ed6fe6fecb09 100644
--- a/ArmPkg/Include/Library/ArmGicArchLib.h
+++ b/ArmPkg/Include/Library/ArmGicArchLib.h
@@ -1,9 +1,15 @@
 /** @file
 *
 *  Copyright (c) 2015, Linaro Ltd. All rights reserved.
+*  Copyright (c) 2024, Arm Limited. All rights reserved.
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
+*  @par Reference(s):
+*  - Arm Generic Interrupt Controller Architecture Specification,
+*    Issue H, January 2022.
+*    (https://developer.arm.com/documentation/ihi0069/)
+*
 **/
 
 #ifndef ARM_GIC_ARCH_LIB_H_
@@ -23,4 +29,12 @@ ArmGicGetSupportedArchRevision (
   VOID
   );
 
+//
+// GIC SPI and extended SPI ranges
+//
+#define ARM_GIC_ARCH_SPI_MIN      32
+#define ARM_GIC_ARCH_SPI_MAX      1019
+#define ARM_GIC_ARCH_EXT_SPI_MIN  4096
+#define ARM_GIC_ARCH_EXT_SPI_MAX  5119
+
 #endif // ARM_GIC_ARCH_LIB_H_
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113141): https://edk2.groups.io/g/devel/message/113141
Mute This Topic: https://groups.io/mt/103518972/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2][PATCH V2 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges

[Sami Mujawar]

Hi Himanshu,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 04/01/2024 08:02 am, Himanshu Sharma wrote:
> Taking reference from Table 2-1 of the Arm Generic Interrupt Controller
> Architecture Specification, Issue H, January 2022, add macros for the
> SPI and extended SPI ranges with the purpose of reusability on including
> the ArmPkg.
>
> Signed-off-by: Himanshu Sharma <Himanshu.Sharma@arm.com>
> ---
>   ArmPkg/Include/Library/ArmGicArchLib.h | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/ArmPkg/Include/Library/ArmGicArchLib.h b/ArmPkg/Include/Library/ArmGicArchLib.h
> index 72ac17e13b5a..ed6fe6fecb09 100644
> --- a/ArmPkg/Include/Library/ArmGicArchLib.h
> +++ b/ArmPkg/Include/Library/ArmGicArchLib.h
> @@ -1,9 +1,15 @@
>   /** @file
>
>   *
>
>   *  Copyright (c) 2015, Linaro Ltd. All rights reserved.
>
> +*  Copyright (c) 2024, Arm Limited. All rights reserved.
>
>   *
>
>   *  SPDX-License-Identifier: BSD-2-Clause-Patent
>
>   *
>
> +*  @par Reference(s):
>
> +*  - Arm Generic Interrupt Controller Architecture Specification,
>
> +*    Issue H, January 2022.
>
> +*    (https://developer.arm.com/documentation/ihi0069/)
>
> +*
>
>   **/
>
>   
>
>   #ifndef ARM_GIC_ARCH_LIB_H_
>
> @@ -23,4 +29,12 @@ ArmGicGetSupportedArchRevision (
>     VOID
>
>     );
>
>   
>
> +//
>
> +// GIC SPI and extended SPI ranges
>
> +//
>
> +#define ARM_GIC_ARCH_SPI_MIN      32
>
> +#define ARM_GIC_ARCH_SPI_MAX      1019
>
> +#define ARM_GIC_ARCH_EXT_SPI_MIN  4096
>
> +#define ARM_GIC_ARCH_EXT_SPI_MAX  5119
>
> +
>
>   #endif // ARM_GIC_ARCH_LIB_H_
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113185): https://edk2.groups.io/g/devel/message/113185
Mute This Topic: https://groups.io/mt/103518972/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2][PATCH V2 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 633 bytes --]

Hi Ard, Leif,

This patch adds macros that can be used to validate that the SPI ranges are valid.
These have been define here so that we do not duplicate it at multiple places.

Can you let me know if I can merge this patch, please?

Regards,

Sami Mujawar


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116118): https://edk2.groups.io/g/devel/message/116118
Mute This Topic: https://groups.io/mt/103518972/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: 1082 bytes --]

Re: [edk2-devel] [edk2][PATCH V2 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

+Resending with email address for maintainers.

Hi Ard, Leif,

This patch adds macros that can be used to validate that the SPI ranges are valid.
These have been define here so that we do not duplicate it at multiple places.

Can you let me know if I can merge this patch, please?

Regards,

Sami Mujawar

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116123): https://edk2.groups.io/g/devel/message/116123
Mute This Topic: https://groups.io/mt/103518972/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: 3348 bytes --]

Re: [edk2-devel] [edk2][PATCH V2 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges

[Ard Biesheuvel]

On Wed, 28 Feb 2024 at 17:59, Sami Mujawar <Sami.Mujawar@arm.com> wrote:
>
> +Resending with email address for maintainers.
>
>
>
> Hi Ard, Leif,
>
> This patch adds macros that can be used to validate that the SPI ranges are valid.
> These have been define here so that we do not duplicate it at multiple places.
>
> Can you let me know if I can merge this patch, please?
>

I have no problems with that, please go ahead.

Acked-by: Ard Biesheuvel <ardb@kernel.org>

I will note that we will have to clean this up in the future, though:
the architected GIC pieces should not be part of a library header, but
a separate IndustryStandard/ header that is separate from the GIC
library code. But that can wait until later.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116259): https://edk2.groups.io/g/devel/message/116259
Mute This Topic: https://groups.io/mt/103518972/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [edk2][PATCH V2 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only

[Himanshu Sharma]

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>
 #
 #  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.
+  */
   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>
 
   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>
 
   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)
-- 
2.25.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113142): https://edk2.groups.io/g/devel/message/113142
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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2][PATCH V2 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only

[Sami Mujawar]

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

Re: [edk2-devel] [edk2][PATCH V2 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library

[PierreGondois]

Hello Himanshu,
I don't have other comments than what Sami had:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

Regards,
Pierre

On 1/4/24 09:02, Himanshu Sharma wrote:
> Currently in the Dynamic Tables Framework, the interrupt node for the
> AML description of the serial-ports is populated using the template
> and so is mandatorily added even if the serial-port is enumerated as
> a DBG2 port in the platform's configuration manager where the
> interrupt is not mandatory. The proposed implementation adds the
> interrupt node only if the interrupt defined for the serial-port is a
> valid SPI or a valid extended SPI. So, in case of DBG2 ports, the
> platforms with interrupt defined as SPI (like Morello) can have the
> interrupt node added to the description and the platforms where it is
> not defined (like N1SDP) can ignore the addition of the interrupt node.
> 
> The changes include adding the SPI range macros in ArmGicArchLib
> (ArmPkg) which can be used by the SSDTSerialPortFixupLib
> (DynamicTablesPkg) to put a check for generating the interrupt node
> using AML Codegen API.
> 
> Change log:
> 
> V2:
>    - Fix comments on V1
>      - Add link to Arm GIC Specification.
>      - Fix uncrustify errors.
>      - Add a check more if the interrupt is 0,
>        to be used when an interrupt is not wired to the serial port.
>    - Update copyright year to 2024.
>    - Link to branch with the patches in this series
>      https://github.com/himsha01/edk2/tree/ssdt_serial_port_interrupt_v2
> 
> V1:
>    - Link to branch with the patches in this series
>      https://github.com/himsha01/edk2/tree/ssdt_serial_port_interrupt
> 
> Himanshu Sharma (2):
>    ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges
>    DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only
> 
>   DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf |  3 +-
>   ArmPkg/Include/Library/ArmGicArchLib.h                                            | 14 ++++++
>   DynamicTablesPkg/Include/ArmNameSpaceObjects.h                                    |  6 ++-
>   DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c   | 49 ++++++++++++++------
>   DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl | 29 +++++++-----
>   5 files changed, 72 insertions(+), 29 deletions(-)
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115113): https://edk2.groups.io/g/devel/message/115113
Mute This Topic: https://groups.io/mt/103518971/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [edk2][PATCH V2 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library

[Sami Mujawar]

[-- Attachment #1: Type: text/plain, Size: 443 bytes --]

Merged as 970aacd191eb..1ae5bee967bf

Thanks.

Regards,

Sami Mujawar


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116307): https://edk2.groups.io/g/devel/message/116307
Mute This Topic: https://groups.io/mt/103518971/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: 879 bytes --]