Re: [PATCH v2 1/3] DynamicTablesPkg: Add Memory32Fixed function

["PierreGondois" <pierre.gondois@arm.com>]

Hello Rebecca,

Just 2 minor comments. With that changed:

Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>


On 1/11/22 6:54 PM, Rebecca Cran wrote:
> Add a Memory32Fixed function to generate code for the corresponding
> Memory32Fixed macro in AML.
>
> Signed-off-by: Rebecca Cran <quic_rcran@quicinc.com>
> ---
>  DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h                        | 33 ++++++++++++
>  DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c | 57 ++++++++++++++++++++
>  2 files changed, 90 insertions(+)
>
> diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> index af18bf8e4871..2491ade397c6 100644
> --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h
> @@ -592,6 +592,39 @@ AmlCodeGenRdDWordMemory (
>    OUT       AML_DATA_NODE_HANDLE    *NewRdNode  OPTIONAL
>    );
>  
> +/** Code generation for the "Memory32Fixed ()" ASL macro.
> +
> +  The Resource Data effectively created is a 32-bit Memory Resource
> +  Data. Cf ACPI 6.4:
> +   - s19.6.83 "Memory Resource Descriptor Macro".
> +   - s19.2.8 "Memory32FixedTerm".
> +
> +  See ACPI 6.4 spec, s19.2.8 for more.
> +
> +  @param [in]  IsReadWrite          ReadAndWrite parameter.
> +  @param [in]  Address              AddressBase parameter.
> +  @param [in]  RangeLength          Range length.
> +  @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] NewMemNode           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
> +AmlCodeGenMemory32Fixed (
> +  BOOLEAN                 IsReadWrite,
> +  UINT32                  Address,
> +  UINT32                  RangeLength,
> +  AML_OBJECT_NODE_HANDLE  NameOpNode,
> +  AML_DATA_NODE_HANDLE    *NewMemNode
> +  );
> +
>  /** Code generation for the "WordBusNumber ()" ASL function.
>  
>    The Resource Data effectively created is a Word Address Space Resource
> diff --git a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> index 40d8c2b07ae3..19fb76dc5b45 100644
> --- a/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> +++ b/DynamicTablesPkg/Library/Common/AmlLib/CodeGen/AmlResourceDataCodeGen.c
> @@ -609,6 +609,63 @@ AmlCodeGenRdDWordMemory (
>             );
>  }
>  
> +/** Code generation for the "Memory32Fixed ()" ASL macro.
> +
> +  The Resource Data effectively created is a 32-bit Memory Resource
> +  Data. Cf ACPI 6.4:
> +   - s19.6.83 "Memory Resource Descriptor Macro".
> +   - s19.2.8 "Memory32FixedTerm".
> +
> +  See ACPI 6.4 spec, s19.2.8 for more.
> +
> +  @param [in]  IsReadWrite          ReadAndWrite parameter.
> +  @param [in]  Addres               AddressBase parameter.
> +  @param [in]  RangeLength          Range length.
> +  @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] NewMemNode           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
> +AmlCodeGenMemory32Fixed (

I missed it in the v1, but other similar functions have an 'Rd' for Resource Data
in their name, so I think it should be AmlCodeGenRdMemory32Fixed() instead.


> +  BOOLEAN                 IsReadWrite,
> +  UINT32                  Address,
> +  UINT32                  RangeLength,
> +  AML_OBJECT_NODE_HANDLE  NameOpNode,
> +  AML_DATA_NODE_HANDLE    *NewMemNode
> +  )
> +{
> +  EFI_STATUS                                     Status;
> +  AML_DATA_NODE                                  *MemNode;
> +  EFI_ACPI_32_BIT_FIXED_MEMORY_RANGE_DESCRIPTOR  RangeDesc;
> +
> +  RangeDesc.Header.Header.Byte = 0x86;
> +  RangeDesc.Header.Length      = 0x09;

Is it possible to use the macros available instead of hard-coded values ?
Cf the AmlCodeRdGenRegister() function and the macro:
ACPI_LARGE_32_BIT_MEMORY_RANGE_DESCRIPTOR_NAME

> +  RangeDesc.Information        = IsReadWrite ? BIT0 : 0;
> +  RangeDesc.BaseAddress        = Address;
> +  RangeDesc.Length             = RangeLength;
> +
> +  Status = AmlCreateDataNode (
> +             EAmlNodeDataTypeResourceData,
> +             (UINT8 *)&RangeDesc,
> +             sizeof (RangeDesc),
> +             &MemNode
> +             );
> +  if (EFI_ERROR (Status)) {
> +    ASSERT (0);
> +    return Status;
> +  }
> +
> +  return LinkRdNode (MemNode, NameOpNode, NewMemNode);
> +}
> +
>  /** Code generation for the "WordSpace ()" ASL function.
>  
>    The Resource Data effectively created is a Word Address Space Resource