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.
Here is the copy of the patch on our edk2 fork as well. https://github.com/NVIDIA/edk2/commit/0171b6c1f60500c5e5178ef3521fa14bcacd3488 > -----Original Message----- > From: Leif Lindholm <quic_llind...@quicinc.com> > Sent: Tuesday, September 12, 2023 3:36 AM > To: Jeff Brasen <jbra...@nvidia.com> > Cc: devel@edk2.groups.io; ardb+tianoc...@kernel.org; > sami.muja...@arm.com; pierre.gond...@arm.com; Vidya Sagar > <vid...@nvidia.com>; Shanker Donthineni <sdonthin...@nvidia.com> > 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 <vid...@nvidia.com> > > > > Add helper functions to generate AML Resource Data describing I/O > > ranges of four words long. API AmlCodeGenRdQWordIo () is exposed. > > > > Reviewed-by: Shanker Donthineni <sdonthin...@nvidia.com> > > The above isn't really applicable to upstream. > Although I feel less strongly about that than > > > Signed-off-by: Vidya Sagar <vid...@nvidia.com> > > 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 <jbra...@nvidia.com> > > --- > > .../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 (#108550): https://edk2.groups.io/g/devel/message/108550 Mute This Topic: https://groups.io/mt/101305535/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
