* [edk2-devel] [edk2][PATCH V1 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library @ 2023-12-06 10:11 Himanshu Sharma 2023-12-06 10:11 ` [edk2-devel] [edk2][PATCH V1 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges Himanshu Sharma 2023-12-06 10:11 ` [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only Himanshu Sharma 0 siblings, 2 replies; 6+ messages in thread From: Himanshu Sharma @ 2023-12-06 10:11 UTC (permalink / raw) To: devel Cc: Ard Biesheuvel, Leif Lindholm, Sami Mujawar, Pierre Gondois, 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, he 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. 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 | 9 +++++ DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c | 38 ++++++++++++-------- DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl | 29 ++++++++------- 4 files changed, 51 insertions(+), 28 deletions(-) -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112116): https://edk2.groups.io/g/devel/message/112116 Mute This Topic: https://groups.io/mt/103010239/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply [flat|nested] 6+ messages in thread
* [edk2-devel] [edk2][PATCH V1 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges 2023-12-06 10:11 [edk2-devel] [edk2][PATCH V1 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library Himanshu Sharma @ 2023-12-06 10:11 ` Himanshu Sharma 2023-12-14 9:47 ` Sami Mujawar 2023-12-06 10:11 ` [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only Himanshu Sharma 1 sibling, 1 reply; 6+ messages in thread From: Himanshu Sharma @ 2023-12-06 10:11 UTC (permalink / raw) To: devel Cc: Ard Biesheuvel, Leif Lindholm, Sami Mujawar, Pierre Gondois, 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 | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ArmPkg/Include/Library/ArmGicArchLib.h b/ArmPkg/Include/Library/ArmGicArchLib.h index 72ac17e13b5a..1b90b354f785 100644 --- a/ArmPkg/Include/Library/ArmGicArchLib.h +++ b/ArmPkg/Include/Library/ArmGicArchLib.h @@ -1,6 +1,7 @@ /** @file * * Copyright (c) 2015, Linaro Ltd. All rights reserved. +* Copyright (c) 2023, Arm Limited. All rights reserved. * * SPDX-License-Identifier: BSD-2-Clause-Patent * @@ -23,4 +24,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 (#112117): https://edk2.groups.io/g/devel/message/112117 Mute This Topic: https://groups.io/mt/103010240/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [edk2][PATCH V1 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges 2023-12-06 10:11 ` [edk2-devel] [edk2][PATCH V1 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges Himanshu Sharma @ 2023-12-14 9:47 ` Sami Mujawar 0 siblings, 0 replies; 6+ messages in thread From: Sami Mujawar @ 2023-12-14 9:47 UTC (permalink / raw) To: Himanshu Sharma, devel Cc: Ard Biesheuvel, Leif Lindholm, Pierre Gondois, nd@arm.com [-- Attachment #1: Type: text/plain, Size: 2114 bytes --] Hi Himanshu, Thank you for this patch. I have a minor suggestion marked inline as [SAMI]. Otherwise this patch looks good to me. With that fixed, Reviewed-by: Sami Mujawar <sami.mujawar@arm.com> Regards, Sami Mujawar On 06/12/2023 10:11 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 | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/ArmPkg/Include/Library/ArmGicArchLib.h b/ArmPkg/Include/Library/ArmGicArchLib.h > index 72ac17e13b5a..1b90b354f785 100644 > --- a/ArmPkg/Include/Library/ArmGicArchLib.h > +++ b/ArmPkg/Include/Library/ArmGicArchLib.h > @@ -1,6 +1,7 @@ > /** @file > > * > > * Copyright (c) 2015, Linaro Ltd. All rights reserved. > > +* Copyright (c) 2023, Arm Limited. All rights reserved. > > * > > * SPDX-License-Identifier: BSD-2-Clause-Patent > > * [SAMI] It may be useful to include a reference to the spec. e.g. @parReference(s): - Arm Generic Interrupt Controller Architecture Specification, Issue H, January 2022. (https://developer.arm.com/documentation/ihi0069/) [/SAMI] > @@ -23,4 +24,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 (#112517): https://edk2.groups.io/g/devel/message/112517 Mute This Topic: https://groups.io/mt/103010240/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: 3690 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only 2023-12-06 10:11 [edk2-devel] [edk2][PATCH V1 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library Himanshu Sharma 2023-12-06 10:11 ` [edk2-devel] [edk2][PATCH V1 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges Himanshu Sharma @ 2023-12-06 10:11 ` Himanshu Sharma 2023-12-06 10:25 ` Himanshu Sharma 2023-12-14 9:47 ` Sami Mujawar 1 sibling, 2 replies; 6+ messages in thread From: Himanshu Sharma @ 2023-12-06 10:11 UTC (permalink / raw) To: devel Cc: Ard Biesheuvel, Leif Lindholm, Sami Mujawar, Pierre Gondois, 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/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c | 38 ++++++++++++-------- DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl | 29 ++++++++------- 3 files changed, 42 insertions(+), 28 deletions(-) diff --git a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf index 965167bdc4e1..2d16a22aeb41 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, 2023, Arm Limited. All rights reserved.<BR> # # SPDX-License-Identifier: BSD-2-Clause-Patent ## @@ -23,6 +23,7 @@ MdeModulePkg/MdeModulePkg.dec EmbeddedPkg/EmbeddedPkg.dec DynamicTablesPkg/DynamicTablesPkg.dec + ArmPkg/ArmPkg.dec [LibraryClasses] AcpiHelperLib diff --git a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c index a65c1fe7e30d..fb77136aa844 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, 2023, Arm Limited. All rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-Patent @@ -9,10 +9,12 @@ - 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. **/ #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 +272,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 +304,27 @@ 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 if the interrupt for the serial-port is a valid SPI. + // SPI ranges 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 ( + 1, // Resource Consumer + 0, // Level Triggered + 0, // Active High + 0, // Exclusive + (UINT32 *)&SerialPortInfo->Interrupt, + 1, + NameOpCrsNode, + NULL + ); + if (EFI_ERROR (Status)) { + return Status; + } } - - if (InterruptRdNode == NULL) { - return EFI_INVALID_PARAMETER; - } - - // Update the interrupt number. - return AmlUpdateRdInterrupt (InterruptRdNode, SerialPortInfo->Interrupt); + return EFI_SUCCESS; } /** Fixup the Serial Port device name. diff --git a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl index fcae2160ac3d..46f800b0cdad 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, 2023, 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 (#112118): https://edk2.groups.io/g/devel/message/112118 Mute This Topic: https://groups.io/mt/103010241/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only 2023-12-06 10:11 ` [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only Himanshu Sharma @ 2023-12-06 10:25 ` Himanshu Sharma 2023-12-14 9:47 ` Sami Mujawar 1 sibling, 0 replies; 6+ messages in thread From: Himanshu Sharma @ 2023-12-06 10:25 UTC (permalink / raw) To: Himanshu Sharma, devel [-- Attachment #1: Type: text/plain, Size: 452 bytes --] Tested on N1SDP and Morello. Tested-by: Himanshu Sharma <Himanshu.Sharma@arm.com> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112120): https://edk2.groups.io/g/devel/message/112120 Mute This Topic: https://groups.io/mt/103010241/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: 894 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only 2023-12-06 10:11 ` [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only Himanshu Sharma 2023-12-06 10:25 ` Himanshu Sharma @ 2023-12-14 9:47 ` Sami Mujawar 1 sibling, 0 replies; 6+ messages in thread From: Sami Mujawar @ 2023-12-14 9:47 UTC (permalink / raw) To: Himanshu Sharma, devel Cc: Ard Biesheuvel, Leif Lindholm, Pierre Gondois, nd@arm.com [-- Attachment #1: Type: text/plain, Size: 9724 bytes --] Hi Himanshu, Thank you for this patch. Please see my feedback marked inline as [SAMI]. Regards, Sami Mujawar On 06/12/2023 10:11 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/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c | 38 ++++++++++++-------- > DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl | 29 ++++++++------- > 3 files changed, 42 insertions(+), 28 deletions(-) > > diff --git a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.inf > index 965167bdc4e1..2d16a22aeb41 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, 2023, Arm Limited. All rights reserved.<BR> > > # > > # SPDX-License-Identifier: BSD-2-Clause-Patent > > ## > > @@ -23,6 +23,7 @@ > MdeModulePkg/MdeModulePkg.dec > > EmbeddedPkg/EmbeddedPkg.dec > > DynamicTablesPkg/DynamicTablesPkg.dec > > + ArmPkg/ArmPkg.dec [SAMI] I understand that this section was not ordered alphabetically, but can you add the above line at the begining of this section, please? > > > > [LibraryClasses] > > AcpiHelperLib > > diff --git a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortFixupLib.c > index a65c1fe7e30d..fb77136aa844 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, 2023, Arm Limited. All rights reserved.<BR> > > > > SPDX-License-Identifier: BSD-2-Clause-Patent > > > > @@ -9,10 +9,12 @@ > - 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. [SAMI] Please limit line length to 80 chars. Also it may be useful to include the link to the spec. e.g. - Arm Generic Interrupt Controller Architecture Specification, Issue H, January 2022. (https://developer.arm.com/documentation/ihi0069/) [/SAMI] > > **/ > > > > #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 +272,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 +304,27 @@ 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 if the interrupt for the serial-port is a valid SPI. [SAMI] Can you modify the above comment to say 'Generate an interrupt node as the second Resource Data element in the NameOpCrsNode, if the interrupt ...'. Also, limit the line length to 80 characters. [/SAMI] > > + // SPI ranges 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 ( > > + 1, // Resource Consumer > > + 0, // Level Triggered > > + 0, // Active High > > + 0, // Exclusive [SAMI] Use BOOLEAN types TRUE/FALSE instead of 1/0. > > + (UINT32 *)&SerialPortInfo->Interrupt, > > + 1, > > + NameOpCrsNode, > > + NULL > > + ); [SAMI] Run uncrustify and patch check script before submitting the patches. > > + if (EFI_ERROR (Status)) { > > + return Status; > > + } [SAMI] Replace this 'if block' with ASSERT_EFI_ERROR (Status) and return 'Status' at the end of the function. > > } [SAMI] I think you should document at https://github.com/tianocore/edk2/blob/master/DynamicTablesPkg/Include/ArmNameSpaceObjects.h#L314 that if the Serial port does not have an interrupt wired, then zero must be specified for the CM_ARM_SERIAL_PORT_INFO.Interrupt field. Then there should be an additional check here as shown below: 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, "Invalid interrupt ID for Serial Port\n")); Status = EFI_INVALID_PARAMETER; } [/SAMI] > > - > > - if (InterruptRdNode == NULL) { > > - return EFI_INVALID_PARAMETER; > > - } > > - > > - // Update the interrupt number. > > - return AmlUpdateRdInterrupt (InterruptRdNode, SerialPortInfo->Interrupt); > > + return EFI_SUCCESS; > > } > > > > /** Fixup the Serial Port device name. > > diff --git a/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl b/DynamicTablesPkg/Library/Common/SsdtSerialPortFixupLib/SsdtSerialPortTemplate.asl > index fcae2160ac3d..46f800b0cdad 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, 2023, 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) > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#112516): https://edk2.groups.io/g/devel/message/112516 Mute This Topic: https://groups.io/mt/103010241/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: 12387 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2023-12-14 9:47 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-12-06 10:11 [edk2-devel] [edk2][PATCH V1 0/2] Update handling of interrupt node for SSDT Serial Port Fixup Library Himanshu Sharma 2023-12-06 10:11 ` [edk2-devel] [edk2][PATCH V1 1/2] ArmPkg/ArmGicArchLib: Add macros for SPI and extended SPI ranges Himanshu Sharma 2023-12-14 9:47 ` Sami Mujawar 2023-12-06 10:11 ` [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only Himanshu Sharma 2023-12-06 10:25 ` Himanshu Sharma 2023-12-14 9:47 ` Sami Mujawar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox