Re: [edk2-devel] [PATCH 1/3] DynamicTablesPkg: Add CM_ARM_CPC_INFO object

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

Hello Jeff,
Please find some remarks inline:

On 9/7/22 17:15, Jeff Brasen via groups.io wrote:
> Introduce the CM_ARM_CPC_INFO CmObj in the ArmNameSpaceObjects.
> 
> This allows to describe CPC information, as described in ACPI 6.4,
> 
> s8.4.7.1 "_CPC (Continuous Performance Control)".
> 
> 
> 
> Signed-off-by: Jeff Brasen <jbrasen@nvidia.com>
> 
> ---
> 
>   .../Include/ArmNameSpaceObjects.h             | 146 ++++++++++++++++--
> 
>   .../ConfigurationManagerObjectParser.c        |  79 ++++++++++
> 
>   2 files changed, 208 insertions(+), 17 deletions(-)
> 
> 
> 
> diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> 
> index 102e0f96be..4d3f9ae534 100644
> 
> --- a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> 
> +++ b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
> 
> @@ -63,6 +63,7 @@ typedef enum ArmObjectID {
> 
>     EArmObjPciInterruptMapInfo,          ///< 39 - Pci Interrupt Map Info
> 
>     EArmObjRmr,                          ///< 40 - Reserved Memory Range Node
> 
>     EArmObjMemoryRangeDescriptor,        ///< 41 - Memory Range Descriptor
> 
> +  EArmObjCpcInfo,                      ///< 42 - Continuous Performance Control Info
> 
>     EArmObjMax
> 
>   } EARM_OBJECT_ID;
> 
>   

[snip]

> 
> +/** A structure that describes the Cpc information.
> 
> +
> 
> +  Continuous Performance Control is described in DSDT/SSDT and associated
> 
> +  to cpus/clusters in the cpu topology.
> 
> +
> 
> +  Unsupported Optional registers should be encoded with NULL resource
> 
> +  Register {(SystemMemory, 0, 0, 0, 0)}
> 
> +
> 
> +  For values that support Integer or Buffer, integer will be used
> 
> +  if buffer is NULL resource.
> 
> +  If resource is not NULL then Integer must be 0
> 
> +
> 
> +  Cf. ACPI 6.4, s8.4.7.1 _CPC (Continuous Performance Control)
> 
> +
> 
> +  ID: EArmObjCpcInfo
> 
> +*/
> 
> +typedef struct CmArmCpcInfo {

I think it would be good to have the CPC Revision entry here aswell.
The NumEntries can be inferred from the Revision.

> 
> +  /// Indicates the highest level of performance the processor
> 
> +  /// is theoretically capable of achieving.
> 
> +  EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE    HighestPerformanceBuffer;

I think we can use ACPI 6.4 structs (it should be the same).
This comment can be applied to all the *6_3* objects.

> 
> +  UINT32                                    HighestPerformanceInteger;
> 
> +
> 

[snip]

> 
> @@ -501,6 +578,8 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  ArmNamespaceObjectParser[] = {
> 
>       ARRAY_SIZE (CmArmPciAddressMapInfoParser) },
> 
>     { "EArmObjPciInterruptMapInfo",          CmPciInterruptMapInfoParser,
> 
>       ARRAY_SIZE (CmPciInterruptMapInfoParser) },
> 

(for Sami)
Not related to this patchset, but the following parsers are missing:
-EArmObjRmr
-EArmObjMemoryRangeDescriptor

> +  { "EArmObjCpcInfo",                      CmArmCpcInfoParser,
> 
> +    ARRAY_SIZE (CmArmCpcInfoParser) },
> 
>     { "EArmObjMax",                          NULL,                                  0                                },
> 
>   };
> 
>   
>