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 05EF9D80041 for ; Tue, 12 Sep 2023 19:31:32 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=GMyDYBUB36PSwCO9zJqqtOeuUnbVfRKUpJeUfp2VfGc=; c=relaxed/simple; d=groups.io; h=Date:From:To:CC:Subject:Message-ID:References:MIME-Version:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Disposition; s=20140610; t=1694547091; v=1; b=LNYS9iXMJpb/zBTpJN8lYtVAN4PD4I2sqEZ3Hjp/4HJHKVCvoxoa8W+0YaBoLKHsJZUfLjzx sFakjPhwmuSYDFEBn/Tg1GC9P2XKT+PfVps7b8Vpz1FlVXe9F6EhvbN79RBPhJ12xajWTytIIH2 keG0KwGaX3eihrA/2CGzhgO4= X-Received: by 127.0.0.2 with SMTP id yLmbYY7687511xcDa4FoR46v; Tue, 12 Sep 2023 12:31:31 -0700 X-Received: from mx0b-0031df01.pphosted.com (mx0b-0031df01.pphosted.com [205.220.180.131]) by mx.groups.io with SMTP id smtpd.web10.4217.1694547091010387318 for ; Tue, 12 Sep 2023 12:31:31 -0700 X-Received: from pps.filterd (m0279871.ppops.net [127.0.0.1]) by mx0a-0031df01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 38CJTxCc007115; Tue, 12 Sep 2023 19:31:27 GMT X-Received: from nasanppmta02.qualcomm.com (i-global254.qualcomm.com [199.106.103.254]) by mx0a-0031df01.pphosted.com (PPS) with ESMTPS id 3t2qefhadk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 12 Sep 2023 19:31:27 +0000 X-Received: from nasanex01c.na.qualcomm.com (nasanex01c.na.qualcomm.com [10.45.79.139]) by NASANPPMTA02.qualcomm.com (8.17.1.5/8.17.1.5) with ESMTPS id 38CJVQYd028988 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 12 Sep 2023 19:31:26 GMT X-Received: from qc-i7.hemma.eciton.net (10.80.80.8) by nasanex01c.na.qualcomm.com (10.45.79.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.2.1118.36; Tue, 12 Sep 2023 12:31:23 -0700 Date: Tue, 12 Sep 2023 20:31:20 +0100 From: "Leif Lindholm" To: Jeff Brasen CC: "devel@edk2.groups.io" , "ardb+tianocore@kernel.org" , "Sami.Mujawar@arm.com" , "pierre.gondois@arm.com" , Vidya Sagar , Shanker Donthineni Subject: Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges Message-ID: References: <64a84c1fedd66114f46ddd5d47ff922d68169d4c.1694475927.git.jbrasen@nvidia.com> MIME-Version: 1.0 In-Reply-To: X-Originating-IP: [10.80.80.8] X-ClientProxiedBy: nasanex01a.na.qualcomm.com (10.52.223.231) To nasanex01c.na.qualcomm.com (10.45.79.139) X-QCInternal: smtphost X-Proofpoint-GUID: 3M8WVqGoe6JIgviW2VGwsNUWekMKP5cI X-Proofpoint-ORIG-GUID: 3M8WVqGoe6JIgviW2VGwsNUWekMKP5cI 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,quic_llindhol@quicinc.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: BwsDO70VaSyG2Vz1CygtPnqwx7686176AA= Content-Type: text/plain; charset="us-ascii" Content-Disposition: inline X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=LNYS9iXM; 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=quicinc.com (policy=none) Hi Jeff, On Tue, Sep 12, 2023 at 19:04:26 +0000, Jeff Brasen wrote: > Regarding the signed-off-by I wasn't sure the right way to handle > this. Vidya was the author of this patch and applied the signed off > on our internal repo during development. I was upstreaming it on his > behalf. I was unsure if I should just replace his as I assumed just > leaving his from this wouldn't be ideal as I figured we would want > the signed-off by the submitter to the devel list. If I was sending this patch out, I would drop reviewed-by and signed-off-by both. Git format-patch sets the From: tag, so authorship is preserved regardless. But I try to separate what i prefer from what I think is absolutely needed. And yes, you're 100% right we need your signed-off-by in any case. > Here is the copy of the patch on our edk2 fork as > well. https://github.com/NVIDIA/edk2/commit/0171b6c1f60500c5e5178ef3521fa14bcacd3488 Yeah, so with that providence, I'm OK with keeping both tags - *but* I'd then appreciate having the statement "this is me contributing https://github.com/NVIDIA/edk2/commit/0171b6c1f60500c5e5178ef3521fa14bcacd3488 from our tree" to the commit message. / Leif > > -----Original Message----- > > From: Leif Lindholm > > Sent: Tuesday, September 12, 2023 3:36 AM > > To: Jeff Brasen > > Cc: devel@edk2.groups.io; ardb+tianocore@kernel.org; > > Sami.Mujawar@arm.com; pierre.gondois@arm.com; Vidya Sagar > > ; Shanker Donthineni > > Subject: Re: [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O > > ranges > > > > External email: Use caution opening links or attachments > > > > > > Hi Jeff, > > > > On Mon, Sep 11, 2023 at 23:48:57 +0000, Jeff Brasen wrote: > > > From: Vidya Sagar > > > > > > Add helper functions to generate AML Resource Data describing I/O > > > ranges of four words long. API AmlCodeGenRdQWordIo () is exposed. > > > > > > Reviewed-by: Shanker Donthineni > > > > The above isn't really applicable to upstream. > > Although I feel less strongly about that than > > > > > Signed-off-by: Vidya Sagar > > > > The DCO is a statement that you have performed basic legal due diligence on > > the provenance of the change. I'm uncomfortable with people making such > > statements on behalf of others. > > > > If this is being upstreamed from a downstream repository, such that the > > review trail is available there, then both of these could be fine. > > But I think it would be useful to include a link to the patch in that repository in > > the commit message in that case. > > > > One technical, but not necessarily for this set (it just made me spot it), note > > below. > > > > > Signed-off-by: Jeff Brasen > > > --- > > > .../Include/Library/AmlLib/AmlLib.h | 67 ++++++++++++++ > > > .../AmlLib/CodeGen/AmlResourceDataCodeGen.c | 90 > > +++++++++++++++++++ > > > 2 files changed, 157 insertions(+) > > > > > > diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > > b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > > index 9210c5091548..8e24cecdd77b 100644 > > > --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > > +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > > @@ -683,6 +683,73 @@ AmlCodeGenRdWordBusNumber ( > > > OUT AML_DATA_NODE_HANDLE *NewRdNode OPTIONAL > > > ); > > > > > > +/** Code generation for the "QWordIO ()" ASL function. > > > + > > > + The Resource Data effectively created is a QWord Address Space > > > + Resource Data. Cf ACPI 6.4: > > > + - s6.4.3.5.1 "QWord Address Space Descriptor". > > > + - s19.6.109 "QWordIO". > > > + > > > + The created resource data node can be: > > > + - appended to the list of resource data elements of the NameOpNode. > > > + In such case NameOpNode must be defined by a the "Name ()" ASL > > statement > > > + and initially contain a "ResourceTemplate ()". > > > + - returned through the NewRdNode parameter. > > > + > > > + See ACPI 6.4 spec, s19.6.109 for more. > > > + > > > + @param [in] IsResourceConsumer ResourceUsage parameter. > > > + @param [in] IsMinFixed Minimum address is fixed. > > > + @param [in] IsMaxFixed Maximum address is fixed. > > > + @param [in] IsPosDecode Decode parameter > > > + @param [in] IsaRanges Possible values are: > > > + 0-Reserved > > > + 1-NonISAOnly > > > + 2-ISAOnly > > > + 3-EntireRange > > > > This is an existing antipattern which this patch (rightly) adheres to when > > adding an additional variant of an existing API. But this also pushes the count > > to three functions in the same file where we're doing enum-but-in-doxygen > > and then keep magic values in the code. > > > > I think someone should rewrite this as an enum and get rid of the magic values > > in the callers. > > > > An additional antipattern is that because the doxygen stanza becomes > > exessively bulky, it leaves out actually documenting the parameter at all. > > > > But as I said, that's not the fault of this set, and does not need to be fixed by it. > > > > / > > Leif > > > > > + @param [in] AddressGranularity Address granularity. > > > + @param [in] AddressMinimum Minimum address. > > > + @param [in] AddressMaximum Maximum address. > > > + @param [in] AddressTranslation Address translation. > > > + @param [in] RangeLength Range length. > > > + @param [in] ResourceSourceIndex Resource Source index. > > > + Unused. Must be 0. > > > + @param [in] ResourceSource Resource Source. > > > + Unused. Must be NULL. > > > + @param [in] IsDenseTranslation TranslationDensity parameter. > > > + @param [in] IsTypeStatic TranslationType parameter. > > > + @param [in] NameOpNode NameOp object node defining a named > > object. > > > + If provided, append the new resource data > > > + node to the list of resource data elements > > > + of this node. > > > + @param [out] NewRdNode If provided and success, > > > + contain the created node. > > > + > > > + @retval EFI_SUCCESS The function completed successfully. > > > + @retval EFI_INVALID_PARAMETER Invalid parameter. > > > + @retval EFI_OUT_OF_RESOURCES Could not allocate memory. > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +AmlCodeGenRdQWordIo ( > > > + IN BOOLEAN IsResourceConsumer, > > > + IN BOOLEAN IsMinFixed, > > > + IN BOOLEAN IsMaxFixed, > > > + IN BOOLEAN IsPosDecode, > > > + IN UINT8 IsaRanges, > > > + IN UINT64 AddressGranularity, > > > + IN UINT64 AddressMinimum, > > > + IN UINT64 AddressMaximum, > > > + IN UINT64 AddressTranslation, > > > + IN UINT64 RangeLength, > > > + IN UINT8 ResourceSourceIndex, > > > + IN CONST CHAR8 *ResourceSource, > > > + IN BOOLEAN IsDenseTranslation, > > > + IN BOOLEAN IsTypeStatic, > > > + IN AML_OBJECT_NODE_HANDLE NameOpNode, OPTIONAL > > > + OUT AML_DATA_NODE_HANDLE *NewRdNode OPTIONAL > > > + ); > > > + > > > /** Code generation for the "QWordMemory ()" ASL function. > > > > > > The Resource Data effectively created is a QWord Address Space > > > Resource diff --git > > > > > a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCo > > deGe > > > n.c > > > > > b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCo > > deGe > > > n.c index 4ca63ccd2396..9c6700b9e08c 100644 > > > --- > > > > > a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCo > > deGe > > > n.c > > > +++ > > b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCo > > > +++ deGen.c > > > @@ -1012,6 +1012,96 @@ AmlCodeGenRdQWordSpace ( > > > return LinkRdNode (RdNode, NameOpNode, NewRdNode); } > > > > > > +/** Code generation for the "QWordIO ()" ASL function. > > > + > > > + The Resource Data effectively created is a QWord Address Space > > > + Resource Data. Cf ACPI 6.4: > > > + - s6.4.3.5.1 "QWord Address Space Descriptor". > > > + - s19.6.109 "QWordIO". > > > + > > > + The created resource data node can be: > > > + - appended to the list of resource data elements of the NameOpNode. > > > + In such case NameOpNode must be defined by a the "Name ()" ASL > > statement > > > + and initially contain a "ResourceTemplate ()". > > > + - returned through the NewRdNode parameter. > > > + > > > + See ACPI 6.4 spec, s19.6.109 for more. > > > + > > > + @param [in] IsResourceConsumer ResourceUsage parameter. > > > + @param [in] IsMinFixed Minimum address is fixed. > > > + @param [in] IsMaxFixed Maximum address is fixed. > > > + @param [in] IsPosDecode Decode parameter > > > + @param [in] IsaRanges Possible values are: > > > + 0-Reserved > > > + 1-NonISAOnly > > > + 2-ISAOnly > > > + 3-EntireRange > > > + @param [in] AddressGranularity Address granularity. > > > + @param [in] AddressMinimum Minimum address. > > > + @param [in] AddressMaximum Maximum address. > > > + @param [in] AddressTranslation Address translation. > > > + @param [in] RangeLength Range length. > > > + @param [in] ResourceSourceIndex Resource Source index. > > > + Unused. Must be 0. > > > + @param [in] ResourceSource Resource Source. > > > + Unused. Must be NULL. > > > + @param [in] IsDenseTranslation TranslationDensity parameter. > > > + @param [in] IsTypeStatic TranslationType parameter. > > > + @param [in] NameOpNode NameOp object node defining a named > > object. > > > + If provided, append the new resource data > > > + node to the list of resource data elements > > > + of this node. > > > + @param [out] NewRdNode If provided and success, > > > + contain the created node. > > > + > > > + @retval EFI_SUCCESS The function completed successfully. > > > + @retval EFI_INVALID_PARAMETER Invalid parameter. > > > + @retval EFI_OUT_OF_RESOURCES Could not allocate memory. > > > +**/ > > > +EFI_STATUS > > > +EFIAPI > > > +AmlCodeGenRdQWordIo ( > > > + IN BOOLEAN IsResourceConsumer, > > > + IN BOOLEAN IsMinFixed, > > > + IN BOOLEAN IsMaxFixed, > > > + IN BOOLEAN IsPosDecode, > > > + IN UINT8 IsaRanges, > > > + IN UINT64 AddressGranularity, > > > + IN UINT64 AddressMinimum, > > > + IN UINT64 AddressMaximum, > > > + IN UINT64 AddressTranslation, > > > + IN UINT64 RangeLength, > > > + IN UINT8 ResourceSourceIndex, > > > + IN CONST CHAR8 *ResourceSource, > > > + IN BOOLEAN IsDenseTranslation, > > > + IN BOOLEAN IsTypeStatic, > > > + IN AML_OBJECT_NODE_HANDLE NameOpNode, OPTIONAL > > > + OUT AML_DATA_NODE_HANDLE *NewRdNode OPTIONAL > > > + ) > > > +{ > > > + return AmlCodeGenRdQWordSpace ( > > > + ACPI_ADDRESS_SPACE_TYPE_IO, > > > + IsResourceConsumer, > > > + IsPosDecode, > > > + IsMinFixed, > > > + IsMaxFixed, > > > + RdIoRangeSpecificFlags ( > > > + IsaRanges, > > > + IsDenseTranslation, > > > + IsTypeStatic > > > + ), > > > + AddressGranularity, > > > + AddressMinimum, > > > + AddressMaximum, > > > + AddressTranslation, > > > + RangeLength, > > > + ResourceSourceIndex, > > > + ResourceSource, > > > + NameOpNode, > > > + NewRdNode > > > + ); > > > +} > > > + > > > /** Code generation for the "QWordMemory ()" ASL function. > > > > > > The Resource Data effectively created is a QWord Address Space > > > Resource > > > -- > > > 2.25.1 > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108552): https://edk2.groups.io/g/devel/message/108552 Mute This Topic: https://groups.io/mt/101305535/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-