Re: [PATCH v1 02/13] DynamicTablesPkg: AML Code generation for Register()

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Pierre,

Thank you for this patch.

I have a minor suggestion marked inline as [SAMI].

With that changed,

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar


On 23/06/2021 12:40 PM, Pierre.Gondois@arm.com wrote:
> From: Pierre Gondois <Pierre.Gondois@arm.com>
>
> Add AmlCodeGenRegister() to generate AML code for the
> Generic Register Resource Descriptor. This function is
> equivalent to the ASL macro Register().
>
> Signed-off-by: Pierre Gondois <Pierre.Gondois@arm.com>
> ---
>   .../AmlLib/CodeGen/AmlResourceDataCodeGen.c   | 87 +++++++++++++++++++
>   .../AmlLib/CodeGen/AmlResourceDataCodeGen.h   | 49 +++++++++++
>   2 files changed, 136 insertions(+)
>
> diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> index 089597a6c906..07a96725a4ef 100644
> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> @@ -187,6 +187,93 @@ AmlCodeGenRdInterrupt (
>     return LinkRdNode (RdNode, NameOpNode, NewRdNode);
>   }
>   
> +/** Code generation for the "Register ()" ASL function.
> +
> +  The Resource Data effectively created is a Generic Register Descriptor.
> +  Data. Cf ACPI 6.4:
> +   - s6.4.3.7 "Generic Register Descriptor".
> +   - s19.6.114 "Register".
> +
> +  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.
> +
> +  @param [in]  AddressSpace    Address space where the register exists.
> +                               Can be one of I/O space, System Memory, etc.
> +  @param [in]  BitWidth        Number of bits in the register.
> +  @param [in]  BitOffset       Offset in bits from the start of the register
> +                               indicated by the Address.
> +  @param [in]  Address         Register address.
> +  @param [in]  AccessSize      Size of data values used when accessing the
> +                               address space. Can be one of:
> +                                 0 - Undefined, legacy (EFI_ACPI_6_3_UNDEFINED)
> +                                 1 - Byte access (EFI_ACPI_6_3_BYTE)
> +                                 2 - Word access (EFI_ACPI_6_3_WORD)
> +                                 3 - DWord access (EFI_ACPI_6_3_DWORD)
> +                                 4 - QWord access (EFI_ACPI_6_3_QWORD)
> +  @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
> +AmlCodeGenRdRegister (
> +  IN  UINT8                   AddressSpace,
> +  IN  UINT8                   BitWidth,
> +  IN  UINT8                   BitOffset,
> +  IN  UINT64                  Address,
> +  IN  UINT8                   AccessSize,
> +  IN  AML_OBJECT_NODE_HANDLE  NameOpNode, OPTIONAL
> +  OUT AML_DATA_NODE_HANDLE    *NewRdNode  OPTIONAL
> +  )
> +{
> +  EFI_STATUS                             Status;
> +  AML_DATA_NODE                        * RdNode;
> +  EFI_ACPI_GENERIC_REGISTER_DESCRIPTOR   RdRegister;
> +
> +  if ((AccessSize > EFI_ACPI_6_3_QWORD)  ||
[SAMI] Can we use the EFI_ACPI_6_4* defines, please? Corresponding 
changes are also needed at other places in this patch.
> +      ((NameOpNode == NULL) && (NewRdNode == NULL))) {
> +    ASSERT (0);
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  // Header
> +  RdRegister.Header.Header.Bits.Name =
> +    ACPI_LARGE_GENERIC_REGISTER_DESCRIPTOR_NAME;
> +  RdRegister.Header.Header.Bits.Type = ACPI_LARGE_ITEM_FLAG;
> +  RdRegister.Header.Length = sizeof (EFI_ACPI_GENERIC_REGISTER_DESCRIPTOR) -
> +                               sizeof (ACPI_LARGE_RESOURCE_HEADER);
> +
> +  // Body
> +  RdRegister.AddressSpaceId = AddressSpace;
> +  RdRegister.RegisterBitWidth = BitWidth;
> +  RdRegister.RegisterBitOffset = BitOffset;
> +  RdRegister.AddressSize = AccessSize;
> +  RdRegister.RegisterAddress = Address;
> +
> +  Status = AmlCreateDataNode (
> +             EAmlNodeDataTypeResourceData,
> +             (UINT8*)&RdRegister,
> +             sizeof (EFI_ACPI_GENERIC_REGISTER_DESCRIPTOR),
> +             &RdNode
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    return Status;
> +  }
> +
> +  return LinkRdNode (RdNode, NameOpNode, NewRdNode);
> +}
> +
>   // DEPRECATED APIS
>   #ifndef DISABLE_NEW_DEPRECATED_INTERFACES
>   
> diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.h b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.h
> index 764051e3d7c9..3c9217d9ddab 100644
> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.h
> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.h
> @@ -55,4 +55,53 @@ AmlCodeGenRdInterrupt (
>     OUT AML_DATA_NODE_HANDLE    *NewRdNode  OPTIONAL
>     );
>   
> +/** Code generation for the "Register ()" ASL function.
> +
> +  The Resource Data effectively created is a Generic Register Descriptor.
> +  Data. Cf ACPI 6.4:
> +   - s6.4.3.7 "Generic Register Descriptor".
> +   - s19.6.114 "Register".
> +
> +  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.
> +
> +  @param [in]  AddressSpace    Address space where the register exists.
> +                               Can be one of I/O space, System Memory, etc.
> +  @param [in]  BitWidth        Number of bits in the register.
> +  @param [in]  BitOffset       Offset in bits from the start of the register
> +                               indicated by the Address.
> +  @param [in]  Address         Register address.
> +  @param [in]  AccessSize      Size of data values used when accessing the
> +                               address space. Can be one of:
> +                                 0 - Undefined, legacy (EFI_ACPI_6_3_UNDEFINED)
> +                                 1 - Byte access (EFI_ACPI_6_3_BYTE)
> +                                 2 - Word access (EFI_ACPI_6_3_WORD)
> +                                 3 - DWord access (EFI_ACPI_6_3_DWORD)
> +                                 4 - QWord access (EFI_ACPI_6_3_QWORD)
> +  @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
> +AmlCodeGenRdRegister (
> +  IN  UINT8                   AddressSpace,
> +  IN  UINT8                   BitWidth,
> +  IN  UINT8                   BitOffset,
> +  IN  UINT64                  Address,
> +  IN  UINT8                   AccessSize,
> +  IN  AML_OBJECT_NODE_HANDLE  NameOpNode, OPTIONAL
> +  OUT AML_DATA_NODE_HANDLE    *NewRdNode  OPTIONAL
> +  );
> +
>   #endif // AML_RESOURCE_DATA_CODE_GEN_H_