* [edk2-devel] [PATCH 0/2] Add support for PCI IO using Qword resources
@ 2023-09-11 23:48 Jeff Brasen via groups.io
2023-09-11 23:48 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges Jeff Brasen via groups.io
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Jeff Brasen via groups.io @ 2023-09-11 23:48 UTC (permalink / raw)
To: devel
Cc: quic_llindhol, ardb+tianocore, Sami.Mujawar, pierre.gondois,
Jeff Brasen
Use AmlCodeGenRdQWordIo() to generate the I/O range in _CRS instead of
AmlCodeGenRdDWordIo() to cater to the scenarios where 64-bit addresses
can be used to generate I/O packets over the PCIe bus.
Vidya Sagar (2):
DynamicTablesPkg: AML Code generation for I/O ranges
DynamicTablesPkg: AcpiSsdtPcieLibArm: Use QWord to describe I/O range
.../Include/Library/AmlLib/AmlLib.h | 67 ++++++++++++++
.../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 2 +-
.../AmlLib/CodeGen/AmlResourceDataCodeGen.c | 90 +++++++++++++++++++
3 files changed, 158 insertions(+), 1 deletion(-)
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108507): https://edk2.groups.io/g/devel/message/108507
Mute This Topic: https://groups.io/mt/101305537/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges
2023-09-11 23:48 [edk2-devel] [PATCH 0/2] Add support for PCI IO using Qword resources Jeff Brasen via groups.io
@ 2023-09-11 23:48 ` Jeff Brasen via groups.io
2023-09-12 9:35 ` Leif Lindholm
2023-09-11 23:48 ` [edk2-devel] [PATCH 2/2] DynamicTablesPkg: AcpiSsdtPcieLibArm: Use QWord to describe I/O range Jeff Brasen via groups.io
2023-09-12 8:54 ` [edk2-devel] [PATCH 0/2] Add support for PCI IO using Qword resources PierreGondois
2 siblings, 1 reply; 9+ messages in thread
From: Jeff Brasen via groups.io @ 2023-09-11 23:48 UTC (permalink / raw)
To: devel
Cc: quic_llindhol, ardb+tianocore, Sami.Mujawar, pierre.gondois,
Vidya Sagar, Shanker Donthineni, Jeff Brasen
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>
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
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
+ @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/AmlResourceDataCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
index 4ca63ccd2396..9c6700b9e08c 100644
--- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
+++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.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 (#108506): https://edk2.groups.io/g/devel/message/108506
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [edk2-devel] [PATCH 2/2] DynamicTablesPkg: AcpiSsdtPcieLibArm: Use QWord to describe I/O range
2023-09-11 23:48 [edk2-devel] [PATCH 0/2] Add support for PCI IO using Qword resources Jeff Brasen via groups.io
2023-09-11 23:48 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges Jeff Brasen via groups.io
@ 2023-09-11 23:48 ` Jeff Brasen via groups.io
2023-09-12 8:54 ` [edk2-devel] [PATCH 0/2] Add support for PCI IO using Qword resources PierreGondois
2 siblings, 0 replies; 9+ messages in thread
From: Jeff Brasen via groups.io @ 2023-09-11 23:48 UTC (permalink / raw)
To: devel
Cc: quic_llindhol, ardb+tianocore, Sami.Mujawar, pierre.gondois,
Vidya Sagar, Shanker Donthineni, Jeff Brasen
From: Vidya Sagar <vidyas@nvidia.com>
Use AmlCodeGenRdQWordIo() to generate the I/O range in _CRS instead of
AmlCodeGenRdDWordIo() to cater to the scenarios where 64-bit addresses
can be used to generate I/O packets over the PCIe bus.
Reviewed-by: Shanker Donthineni <sdonthineni@nvidia.com>
Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
---
.../Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index c54ae6f551f6..9ddaddc198fa 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -540,7 +540,7 @@ GeneratePciCrs (
switch (AddrMapInfo->SpaceCode) {
case PCI_SS_IO:
- Status = AmlCodeGenRdDWordIo (
+ Status = AmlCodeGenRdQWordIo (
FALSE,
TRUE,
TRUE,
--
2.25.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108508): https://edk2.groups.io/g/devel/message/108508
Mute This Topic: https://groups.io/mt/101305538/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 0/2] Add support for PCI IO using Qword resources
2023-09-11 23:48 [edk2-devel] [PATCH 0/2] Add support for PCI IO using Qword resources Jeff Brasen via groups.io
2023-09-11 23:48 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges Jeff Brasen via groups.io
2023-09-11 23:48 ` [edk2-devel] [PATCH 2/2] DynamicTablesPkg: AcpiSsdtPcieLibArm: Use QWord to describe I/O range Jeff Brasen via groups.io
@ 2023-09-12 8:54 ` PierreGondois
2 siblings, 0 replies; 9+ messages in thread
From: PierreGondois @ 2023-09-12 8:54 UTC (permalink / raw)
To: Jeff Brasen, devel; +Cc: quic_llindhol, ardb+tianocore, Sami.Mujawar
Hello Jeff,
I had some trouble applying PATCH [1/2], but the changes look good
to me otherwise:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
Regards,
Pierre
On 9/12/23 01:48, Jeff Brasen wrote:
> Use AmlCodeGenRdQWordIo() to generate the I/O range in _CRS instead of
> AmlCodeGenRdDWordIo() to cater to the scenarios where 64-bit addresses
> can be used to generate I/O packets over the PCIe bus.
>
> Vidya Sagar (2):
> DynamicTablesPkg: AML Code generation for I/O ranges
> DynamicTablesPkg: AcpiSsdtPcieLibArm: Use QWord to describe I/O range
>
> .../Include/Library/AmlLib/AmlLib.h | 67 ++++++++++++++
> .../AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 2 +-
> .../AmlLib/CodeGen/AmlResourceDataCodeGen.c | 90 +++++++++++++++++++
> 3 files changed, 158 insertions(+), 1 deletion(-)
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108521): https://edk2.groups.io/g/devel/message/108521
Mute This Topic: https://groups.io/mt/101305537/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges
2023-09-11 23:48 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges Jeff Brasen via groups.io
@ 2023-09-12 9:35 ` Leif Lindholm
2023-09-12 9:55 ` PierreGondois
2023-09-12 19:04 ` Jeff Brasen via groups.io
0 siblings, 2 replies; 9+ messages in thread
From: Leif Lindholm @ 2023-09-12 9:35 UTC (permalink / raw)
To: Jeff Brasen
Cc: devel, ardb+tianocore, Sami.Mujawar, pierre.gondois, Vidya Sagar,
Shanker Donthineni
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/AmlResourceDataCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> index 4ca63ccd2396..9c6700b9e08c 100644
> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.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 (#108523): https://edk2.groups.io/g/devel/message/108523
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges
2023-09-12 9:35 ` Leif Lindholm
@ 2023-09-12 9:55 ` PierreGondois
2023-09-12 19:32 ` Leif Lindholm
2023-09-12 19:04 ` Jeff Brasen via groups.io
1 sibling, 1 reply; 9+ messages in thread
From: PierreGondois @ 2023-09-12 9:55 UTC (permalink / raw)
To: Leif Lindholm, Jeff Brasen
Cc: devel, ardb+tianocore, Sami.Mujawar, Vidya Sagar,
Shanker Donthineni
Hello Leif,
On 9/12/23 11:35, Leif Lindholm wrote:
> 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.
Ok yes, I'll make the modifications where required,
Thanks for the review,
Regards,
Pierre
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108525): https://edk2.groups.io/g/devel/message/108525
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges
2023-09-12 9:35 ` Leif Lindholm
2023-09-12 9:55 ` PierreGondois
@ 2023-09-12 19:04 ` Jeff Brasen via groups.io
2023-09-12 19:31 ` Leif Lindholm
1 sibling, 1 reply; 9+ messages in thread
From: Jeff Brasen via groups.io @ 2023-09-12 19:04 UTC (permalink / raw)
To: Leif Lindholm
Cc: devel@edk2.groups.io, ardb+tianocore@kernel.org,
Sami.Mujawar@arm.com, pierre.gondois@arm.com, Vidya Sagar,
Shanker Donthineni
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges
2023-09-12 19:04 ` Jeff Brasen via groups.io
@ 2023-09-12 19:31 ` Leif Lindholm
0 siblings, 0 replies; 9+ messages in thread
From: Leif Lindholm @ 2023-09-12 19:31 UTC (permalink / raw)
To: Jeff Brasen
Cc: devel@edk2.groups.io, ardb+tianocore@kernel.org,
Sami.Mujawar@arm.com, pierre.gondois@arm.com, Vidya Sagar,
Shanker Donthineni
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_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 (#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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges
2023-09-12 9:55 ` PierreGondois
@ 2023-09-12 19:32 ` Leif Lindholm
0 siblings, 0 replies; 9+ messages in thread
From: Leif Lindholm @ 2023-09-12 19:32 UTC (permalink / raw)
To: Pierre Gondois
Cc: Jeff Brasen, devel, ardb+tianocore, Sami.Mujawar, Vidya Sagar,
Shanker Donthineni
On Tue, Sep 12, 2023 at 11:55:18 +0200, Pierre Gondois wrote:
> Hello Leif,
>
> On 9/12/23 11:35, Leif Lindholm wrote:
> > > 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.
>
> Ok yes, I'll make the modifications where required,
> Thanks for the review,
Thanks, I appreciate that.
Best Regards,
Leif
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108553): https://edk2.groups.io/g/devel/message/108553
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-09-12 19:32 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-11 23:48 [edk2-devel] [PATCH 0/2] Add support for PCI IO using Qword resources Jeff Brasen via groups.io
2023-09-11 23:48 ` [edk2-devel] [PATCH 1/2] DynamicTablesPkg: AML Code generation for I/O ranges Jeff Brasen via groups.io
2023-09-12 9:35 ` Leif Lindholm
2023-09-12 9:55 ` PierreGondois
2023-09-12 19:32 ` Leif Lindholm
2023-09-12 19:04 ` Jeff Brasen via groups.io
2023-09-12 19:31 ` Leif Lindholm
2023-09-11 23:48 ` [edk2-devel] [PATCH 2/2] DynamicTablesPkg: AcpiSsdtPcieLibArm: Use QWord to describe I/O range Jeff Brasen via groups.io
2023-09-12 8:54 ` [edk2-devel] [PATCH 0/2] Add support for PCI IO using Qword resources PierreGondois
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox