Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges

["Jeff Brasen via groups.io" <jbrasen=nvidia.com@groups.io>]

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_llindhol@quicinc.com>
> Sent: Tuesday, September 12, 2023 3:36 AM
> To: Jeff Brasen <jbrasen@nvidia.com>
> Cc: devel@edk2.groups.io; ardb+tianocore@kernel.org;
> Sami.Mujawar@arm.com; pierre.gondois@arm.com; Vidya Sagar
> <vidyas@nvidia.com>; Shanker Donthineni <sdonthineni@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 <vidyas@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 <sdonthineni@nvidia.com>
> 
> The above isn't really applicable to upstream.
> Although I feel less strongly about that than
> 
> > Signed-off-by: Vidya Sagar <vidyas@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 <jbrasen@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-