From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 67BC7D8119E for ; Thu, 14 Dec 2023 09:47:51 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=Sr+H0QbqdzwW8pVD/IqzUX566fuLBVIh+aIbONPj5aU=; c=relaxed/simple; d=groups.io; h=ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:Received-SPF:ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:Authentication-Results-Original:Message-ID:Date:User-Agent:Subject:To:Cc:References:From:In-Reply-To:MIME-Version:NoDisclaimer:Original-Authentication-Results:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Language; s=20140610; t=1702547269; v=1; b=poYP16OqdB57clDS0yDsgfmxQU+SWQd6wWYWY9EpRIjqtNo684gGIyJj9bthtgBpR7xflVax nDteFfq8cYpVKhTAnebnOjR0jst3aLQUTVKbSnm7KFNa6qDvEM6tq91Yc3H0IVlTFQedS6TIg/8 k61BTsKebJX2UYRcj0zJvO/8= X-Received: by 127.0.0.2 with SMTP id C1mVYY7687511xMBdX9XNP3o; Thu, 14 Dec 2023 01:47:49 -0800 X-Received: from EUR04-DB3-obe.outbound.protection.outlook.com (EUR04-DB3-obe.outbound.protection.outlook.com [40.107.6.47]) by mx.groups.io with SMTP id smtpd.web10.18512.1702547268484556869 for ; Thu, 14 Dec 2023 01:47:49 -0800 ARC-Seal: i=2; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=pass; b=avQamatEqEpfcVjzui6J00Ea45MCnK6LIWX5nTdbGwLPlcvsue8sDKliPhCMvXoObOBX1qkuvWZzhcnuoWEH51XQBvj3jv+Cq8o36yv03sbb0p8rmFj0XmVb3jJoIdP+OAAcixyXQwhRIV/a/5p86rdm53d0x/W1w9R8nLrOafjhg4EMv4tepyGsjlypR74A5Zq7rQUqWN67HS1+Ric7yLvIYZvAp1JGvy52kFhT6mtDBPIkQTIa4UQbpDNVKTF6bG6eQ6PyNFaEDxYWsWNrS/XCAHc9D2IcnfQveoFtxiEBo2lZjMMD8jCZunnVPkVkIOlTjinHM6kVLNKk0WG+Vg== ARC-Message-Signature: i=2; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=7lBwkgKqNysskykHNeds1axkn4h91w/tvlkLRvsDNSg=; b=TeDoY7dBNLirSgPODA3+iVvSg/9pA0WxRcFsyN3U9pM10nphbhwn1WQjHvRIbCTUp5E+Gm9pAUQVH09kQwzSK3gGknJsehASqwrzWB0Cn+8f6Y52H8Ya33uloROSktOkBgixh5KmaXlbG9iTSLeq/ghXrGweGPKvuxM2IWLFqLB1Zo4HoXp93EBmhnCn1t381mji0TnsSGpnYt7s5O44O1j3hwJpm3iE8veg5758TLQiCmRSGEEk7MAVtz8MfYynT3gKoxRo5d/+we6tEiE6f2Ha9IcBuYj3fDkrefGw0Z1/dl1QlPPxWiNX8Y1ndPPXvVHl/7hiil6+HoWCTvIPJw== ARC-Authentication-Results: i=2; mx.microsoft.com 1; spf=pass (sender ip is 63.35.35.123) smtp.rcpttodomain=edk2.groups.io smtp.mailfrom=arm.com; dmarc=pass (p=none sp=none pct=100) action=none header.from=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com; arc=pass (0 oda=1 ltdi=1 spf=[1,1,smtp.mailfrom=arm.com] dkim=[1,1,header.d=arm.com] dmarc=[1,1,header.from=arm.com]) X-Received: from AM0PR02CA0182.eurprd02.prod.outlook.com (2603:10a6:20b:28e::19) by AS8PR08MB9043.eurprd08.prod.outlook.com (2603:10a6:20b:5c1::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7091.28; Thu, 14 Dec 2023 09:47:44 +0000 X-Received: from AMS1EPF0000004B.eurprd04.prod.outlook.com (2603:10a6:20b:28e:cafe::6c) by AM0PR02CA0182.outlook.office365.com (2603:10a6:20b:28e::19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7091.26 via Frontend Transport; Thu, 14 Dec 2023 09:47:44 +0000 X-MS-Exchange-Authentication-Results: spf=pass (sender IP is 63.35.35.123) smtp.mailfrom=arm.com; dkim=pass (signature was verified) header.d=armh.onmicrosoft.com;dmarc=pass action=none header.from=arm.com; Received-SPF: Pass (protection.outlook.com: domain of arm.com designates 63.35.35.123 as permitted sender) receiver=protection.outlook.com; client-ip=63.35.35.123; helo=64aa7808-outbound-1.mta.getcheckrecipient.com; pr=C X-Received: from 64aa7808-outbound-1.mta.getcheckrecipient.com (63.35.35.123) by AMS1EPF0000004B.mail.protection.outlook.com (10.167.16.136) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7091.26 via Frontend Transport; Thu, 14 Dec 2023 09:47:44 +0000 X-Received: ("Tessian outbound 7671e7ddc218:v228"); Thu, 14 Dec 2023 09:47:44 +0000 X-CheckRecipientChecked: true X-CR-MTA-CID: b134d92cbb0db303 X-CR-MTA-TID: 64aa7808 X-Received: from f2228fac45a8.2 by 64aa7808-outbound-1.mta.getcheckrecipient.com id 3BA8C083-3526-4399-ACDE-C303FBB9F997.1; Thu, 14 Dec 2023 09:47:37 +0000 X-Received: from EUR04-HE1-obe.outbound.protection.outlook.com by 64aa7808-outbound-1.mta.getcheckrecipient.com with ESMTPS id f2228fac45a8.2 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384); Thu, 14 Dec 2023 09:47:37 +0000 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=ixTg/K86gYFD1FDGxBt63Eg3vqZ5/bMccL8MEX9SiNhBVQpLKok4S52VReIHPXUcoxAQKXiKUqbJyqJRxz6tJ0kiPpFz7jfvfVuE4vzRNrzFaUbUt4viIeHPcEaXdEpjMRs7CmDD1LSIjAp2Rb8NqWrGBKL07Y5F5j5OI4F0j17+/sfk96pbW60OLPWYmlBy11bhV3kuLUPnjMaNGAdTu/0ovs+fxaSFqs4hnEZZU0Gc8QeUfhyR1sJuYCgcxkWK9oaFo1JkXH5z9y1BGuFL4DTN3u9tUmlHj640gdTxho4hVqpBIaT62G9WvHWEnCHamQg4d6zrZHb/VzIjHj+IcQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=7lBwkgKqNysskykHNeds1axkn4h91w/tvlkLRvsDNSg=; b=ce/iKYhW7YikdaFhrhheqdW6TVGYV+Iao3rSRwAvljB3Yex+AgQTXp40zUZVRVHGZQ5SW/2gfAS28ZfFgqpkLGBdFWYSnzPQgIzFDoDulNYnCb6+SNNM2IO2htSkcmuUsvY9H/SCs9CVYO9JVOCiCXtAYBdgEgq2e04h0tiU62v1jWxIwyCXw9GSqhLw3GVQDuzbGEBx4U/4h+yM4zVrwMQbKaOKhJl2y1baG8OKjGdMFuwS4ktGEaoiZCKxipOU9L+8oUKC6yJZIwPLVjX58+BBhOWNmzRXeYTC++btQfSKLMRxQeI8w+ixcaByDtKSA3smrHL1WWKDI2w9+BfzLw== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=arm.com; dmarc=pass action=none header.from=arm.com; dkim=pass header.d=arm.com; arc=none Authentication-Results-Original: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; X-Received: from AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) by AS8PR08MB6038.eurprd08.prod.outlook.com (2603:10a6:20b:23f::24) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.7091.28; Thu, 14 Dec 2023 09:47:35 +0000 X-Received: from AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::f36e:3882:2fce:d775]) by AS8PR08MB6806.eurprd08.prod.outlook.com ([fe80::f36e:3882:2fce:d775%4]) with mapi id 15.20.7091.028; Thu, 14 Dec 2023 09:47:35 +0000 Message-ID: Date: Thu, 14 Dec 2023 09:47:35 +0000 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [edk2][PATCH V1 2/2] DynamicTablesPkg/SsdtSerialPortFixupLib: Add Interrupt node for SPIs only To: Himanshu Sharma , devel@edk2.groups.io Cc: Ard Biesheuvel , Leif Lindholm , Pierre Gondois , "nd@arm.com" References: <20231206101146.1309296-1-Himanshu.Sharma@arm.com> <20231206101146.1309296-3-Himanshu.Sharma@arm.com> From: "Sami Mujawar" In-Reply-To: <20231206101146.1309296-3-Himanshu.Sharma@arm.com> X-ClientProxiedBy: LO4P123CA0190.GBRP123.PROD.OUTLOOK.COM (2603:10a6:600:1a4::15) To AS8PR08MB6806.eurprd08.prod.outlook.com (2603:10a6:20b:39b::12) MIME-Version: 1.0 X-MS-TrafficTypeDiagnostic: AS8PR08MB6806:EE_|AS8PR08MB6038:EE_|AMS1EPF0000004B:EE_|AS8PR08MB9043:EE_ X-MS-Office365-Filtering-Correlation-Id: c281cb57-fe4f-4783-ff5b-08dbfc89b8ef x-checkrecipientrouted: true NoDisclaimer: true X-MS-Exchange-SenderADCheck: 1 X-MS-Exchange-AntiSpam-Relay: 0 X-Microsoft-Antispam-Untrusted: BCL:0; X-Microsoft-Antispam-Message-Info-Original: mXBL7n8yWf9bOMib1waegGAaI34VLoXARQc8l1L3SYAcV3OVK7lORteR9mrpFVL6bRZmMycDu4ETUSMXLl+PnstwHvzjrqaBIH5ZGecF39H7/FdPPxdV8llUf1dlepIl4ZO8ZMQKaroV4twW1NNM/O4XYqNsossHD147SVZrgUSRIbkjVY2b0nA947TMVcLHp9/gl2rYPS8oFe6VMbnMgmoK8DsQd3u9ghDbBbjXUh3Z2PEl5ilEH6ZVvHXqlra9Efy+H/YlZcad6irCgxHaChy0mgFrG7lVqnYO7fARfYkbnfGjFYLYQL0+LhDfLzE9G1WQuK0g8aP3c9Ate/I1EXTgBOTxYWz3+/GvxC4vJfl1dq2DcX2YFb9wLj1bLB30THAP7xHloMf/4PN4uVi2ikSUNs0lGLkfyVBGC2tqxzAWJG7vQceAw8BRSF1OigqZqOdce/PkKgxPNbvljiPXO/Jwq5wSn9Vjj/r4PGg2oX/aP06kRAGu/BW1fjCY5I26qEdVKGXv0LDQkEDBkThiJGNPssyIecpn1KCNjG+d5CnQOvh2kOWu/L8iSaOSbbIa76J1rAZNqczgVeizoBvvIL3C5tr2UQWT2+qRASe4hNn4wjBrCCYJ6dB3P9e4gM23//p3hGCJNRfPocA2CdByMKAsCH58eNuFt0TtDw2UByjvYyL7c1dRm7Qsdz6JovHdqtYK0YUa3VwQzb7lLMGyRw== X-Forefront-Antispam-Report-Untrusted: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:AS8PR08MB6806.eurprd08.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230031)(39860400002)(376002)(136003)(366004)(396003)(346002)(230922051799003)(451199024)(64100799003)(1800799012)(186009)(41300700001)(83380400001)(38100700002)(33964004)(5660300002)(45080400002)(44832011)(478600001)(6486002)(966005)(6512007)(2906002)(30864003)(53546011)(54906003)(316002)(66476007)(66556008)(8676002)(66946007)(31686004)(4326008)(8936002)(166002)(86362001)(2616005)(26005)(6506007)(31696002)(36756003)(213903007)(45980500001)(43740500002);DIR:OUT;SFP:1101; X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR08MB6038 Original-Authentication-Results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=arm.com; X-EOPAttributedMessage: 0 X-MS-Exchange-Transport-CrossTenantHeadersStripped: AMS1EPF0000004B.eurprd04.prod.outlook.com X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-Correlation-Id-Prvs: 748707fc-d3c9-4112-e0c3-08dbfc89b33e X-Microsoft-Antispam-Message-Info: b/3WbSHF6iMNq6CEc7xhURu9sWViVSCH/b3YyytH1DwqaLVJiOxU21hC0XcFMUyAPVwoxraIxk+GOczTbodN1/0/Dxwow6A1PdAhxe0G24MfQWKPW7bfclQBpf89wOGUS312zgIZ+tNIcMAOORPC0i/SLwu+jrr6EP1LvlgG19Q86IzbI0E5xoxPP0Pdv1Zo1AhiZU04hKPn+cocrjuNuh9fV0s7PmNH2Orf2FuGRljo3bzIJLZ67QqaJ425wfYvi10YQPs+l1kOAy3jPwS8F7DA7jAIUzP99QxhEG8M6vMkHkOGeadfMTPnwc+5ZdPgQUaykhnreqP6b5f4t7qruOPy1ISAmmFebg//VALFS6xf66qJx0fY2mhEVGuIv/pMuXaEhxTyEUd9TtlQ8MQMt3EYZeQkjn79KNp/qAPOQyjYhSfq9lh36mVbe+7+tE6UiRUEUUHoebKK7ZiBz9LyFknECNaMqSn1P0rxJNLMn9eO0QpgZ6v3xy5lEnsEN1iagG8V/zfKYzAHa9KWYV6qK2AhRVDhenLwwbTHgEdDVRcAUVDagBN2VQnExpqMJkcPDnH9wAkYtmKMF8K9PKn2pNAWACUepE64yZmd6j70dt9JTqV7jMwy99dT24Fv8I1+cPMiuVF7VVZb+a10pF/wg/lh9Io9fK5YgoHuHektcozLzy60Q6mdQAnI/W70OVcl52tiQAlKoXKE9c8do3xJk3cqMI2UsmrKnqDyze3Tw3bDPpr3E+YuXkNitiuhbVPXktmgjhC1mImUKLttch2n5ZKFWZ31hcDp9CojcVbhAH5YDaasQP/NU6ev5nn0ghDk X-OriginatorOrg: arm.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Dec 2023 09:47:44.5887 (UTC) X-MS-Exchange-CrossTenant-Network-Message-Id: c281cb57-fe4f-4783-ff5b-08dbfc89b8ef X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=f34e5979-57d9-4aaa-ad4d-b122a662184d;Ip=[63.35.35.123];Helo=[64aa7808-outbound-1.mta.getcheckrecipient.com] X-MS-Exchange-CrossTenant-AuthSource: AMS1EPF0000004B.eurprd04.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Anonymous X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR08MB9043 Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,sami.mujawar@arm.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: t8Qp0xKWw1C5IzReTlLO6hP5x7686176AA= Content-Type: multipart/alternative; boundary="------------jx2VVFaANYdnNoS2iy0gPDBv" Content-Language: en-GB X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=poYP16Oq; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=arm.com (policy=none); arc=reject ("signature check failed: fail, {[1] = sig:microsoft.com:reject}") --------------jx2VVFaANYdnNoS2iy0gPDBv Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > --- > 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.
> > +# Copyright (c) 2020 - 2021, 2023, Arm Limited. All rights reserved.
> > # > > # 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.
> > + Copyright (c) 2019 - 2021, 2023, Arm Limited. All rights reserved.
> > > > 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 > > #include > > +#include > > #include > > #include > > #include > > @@ -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.
> > + Copyright (c) 2019 - 2020, 2023, Arm Limited. All rights reserved.
> > > > 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 > > + // ) { > > + // // > > + // } // 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] -=-=-=-=-=-=-=-=-=-=-=- --------------jx2VVFaANYdnNoS2iy0gPDBv Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit

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) | | Mute This Topic | New Topic
Your Subscription | Contact Group Owner | Unsubscribe [rebecca@openfw.io]

_._,_._,_
--------------jx2VVFaANYdnNoS2iy0gPDBv--