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 <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 (#108552): https://edk2.groups.io/g/devel/message/108552 Mute This Topic: https://groups.io/mt/101305535/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
