Re: [PATCH v2 1/3] DynamicTablesPkg: Add CM_ARM_CPC_INFO object

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

Hi Jeff,

Apologies for the delay in providing feedback. I was half way through 
your patch series when you sent the v3.

For this patch please fine my feedbakc inline marked [SAMI].  I believe 
some of these still apply for v3.

I will provide review feedback for the v3 series shortly.

Regards,

Sami Mujawar

On 14/09/2022 10:34 pm, Jeff Brasen 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             | 148 ++++++++++++++++--
>
>   .../ConfigurationManagerObjectParser.c        |  79 ++++++++++
>
>   2 files changed, 210 insertions(+), 17 deletions(-)
>
>
>
> diff --git a/DynamicTablesPkg/Include/ArmNameSpaceObjects.h b/DynamicTablesPkg/Include/ArmNameSpaceObjects.h
>
> index 102e0f96be..d76cc08e14 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;
>
>   
>
> @@ -97,99 +98,105 @@ typedef struct CmArmPowerManagementProfileInfo {
>
>   */
>
>   typedef struct CmArmGicCInfo {
>
>     /// The GIC CPU Interface number.
>
> -  UINT32    CPUInterfaceNumber;
>
> +  UINT32             CPUInterfaceNumber;
>
>   
>
>     /** The ACPI Processor UID. This must match the
>
>         _UID of the CPU Device object information described
>
>         in the DSDT/SSDT for the CPU.
>
>     */
>
> -  UINT32    AcpiProcessorUid;
>
> +  UINT32             AcpiProcessorUid;
>
>   
>
>     /** The flags field as described by the GICC structure
>
>         in the ACPI Specification.
>
>     */
>
> -  UINT32    Flags;
>
> +  UINT32             Flags;
>
>   
>
>     /** The parking protocol version field as described by
>
>       the GICC structure in the ACPI Specification.
>
>     */
>
> -  UINT32    ParkingProtocolVersion;
>
> +  UINT32             ParkingProtocolVersion;
>
>   
>
>     /** The Performance Interrupt field as described by
>
>         the GICC structure in the ACPI Specification.
>
>     */
>
> -  UINT32    PerformanceInterruptGsiv;
>
> +  UINT32             PerformanceInterruptGsiv;
>
>   
>
>     /** The CPU Parked address field as described by
>
>         the GICC structure in the ACPI Specification.
>
>     */
>
> -  UINT64    ParkedAddress;
>
> +  UINT64             ParkedAddress;
>
>   
>
>     /** The base address for the GIC CPU Interface
>
>         as described by the GICC structure in the
>
>         ACPI Specification.
>
>     */
>
> -  UINT64    PhysicalBaseAddress;
>
> +  UINT64             PhysicalBaseAddress;
>
>   
>
>     /** The base address for GICV interface
>
>         as described by the GICC structure in the
>
>         ACPI Specification.
>
>     */
>
> -  UINT64    GICV;
>
> +  UINT64             GICV;
>
>   
>
>     /** The base address for GICH interface
>
>         as described by the GICC structure in the
>
>         ACPI Specification.
>
>     */
>
> -  UINT64    GICH;
>
> +  UINT64             GICH;
>
>   
>
>     /** The GICV maintenance interrupt
>
>         as described by the GICC structure in the
>
>         ACPI Specification.
>
>     */
>
> -  UINT32    VGICMaintenanceInterrupt;
>
> +  UINT32             VGICMaintenanceInterrupt;
>
>   
>
>     /** The base address for GICR interface
>
>         as described by the GICC structure in the
>
>         ACPI Specification.
>
>     */
>
> -  UINT64    GICRBaseAddress;
>
> +  UINT64             GICRBaseAddress;
>
>   
>
>     /** The MPIDR for the CPU
>
>         as described by the GICC structure in the
>
>         ACPI Specification.
>
>     */
>
> -  UINT64    MPIDR;
>
> +  UINT64             MPIDR;
>
>   
>
>     /** The Processor Power Efficiency class
>
>         as described by the GICC structure in the
>
>         ACPI Specification.
>
>     */
>
> -  UINT8     ProcessorPowerEfficiencyClass;
>
> +  UINT8              ProcessorPowerEfficiencyClass;
>
>   
>
>     /** Statistical Profiling Extension buffer overflow GSIV. Zero if
>
>         unsupported by this processor. This field was introduced in
>
>         ACPI 6.3 (MADT revision 5) and is therefore ignored when
>
>         generating MADT revision 4 or lower.
>
>     */
>
> -  UINT16    SpeOverflowInterrupt;
>
> +  UINT16             SpeOverflowInterrupt;
>
>   
>
>     /** The proximity domain to which the logical processor belongs.
>
>         This field is used to populate the GICC affinity structure
>
>         in the SRAT table.
>
>     */
>
> -  UINT32    ProximityDomain;
>
> +  UINT32             ProximityDomain;
>
>   
>
>     /** The clock domain to which the logical processor belongs.
>
>         This field is used to populate the GICC affinity structure
>
>         in the SRAT table.
>
>     */
>
> -  UINT32    ClockDomain;
>
> +  UINT32             ClockDomain;
>
>   
>
>     /** The GICC Affinity flags field as described by the GICC Affinity structure
>
>         in the SRAT table.
>
>     */
>
> -  UINT32    AffinityFlags;
>
> +  UINT32             AffinityFlags;
>
> +
>
> +  /** Optional field: Reference Token for the Cpc info of this processor.
>
> +      Token identifying a CM_ARM_OBJ_REF structure, itself referencing
>
> +      CM_ARM_CPC_INFO objects.
>
> +  */
>
> +  CM_OBJECT_TOKEN    CpcToken;
>
>   } CM_ARM_GICC_INFO;
>
>   
>
>   /** A structure that describes the
>
> @@ -1070,6 +1077,113 @@ typedef struct CmArmRmrDescriptor {
>
>     UINT64    Length;
>
>   } CM_ARM_MEMORY_RANGE_DESCRIPTOR;
>
>   
>
> +/** 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 {
>
> +  /// The revision number of the _CPC package format.
>
> +  UINT32                                    Revision;
>
> +
>
> +  /// Indicates the highest level of performance the processor
>
> +  /// is theoretically capable of achieving.
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    HighestPerformanceBuffer;
>
> +  UINT32                                    HighestPerformanceInteger;
>
> +
>
> +  /// Indicates the highest sustained performance level of the processor.
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    NominalPerformanceBuffer;
>
> +  UINT32                                    NominalPerformanceInteger;
>
> +
>
> +  /// Indicates the lowest performance level of the processor with non-linear power savings.
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    LowestNonlinearPerformanceBuffer;
>
> +  UINT32                                    LowestNonlinearPerformanceInteger;
>
> +
>
> +  /// Indicates the lowest performance level of the processor..
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    LowestPerformanceBuffer;
>
> +  UINT32                                    LowestPerformanceInteger;
>
> +
>
> +  /// Guaranteed Performance Register Buffer.
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    GuaranteedPerformanceRegister;
>
> +
>
> +  /// Desired Performance Register Buffer.
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    DesiredPerformanceRegister;
>
> +
>
> +  /// Minimum Performance Register Buffer.
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    MinimumPerformanceRegister;
>
> +
>
> +  /// Maximum Performance Register Buffer.
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    MaximumPerformanceRegister;
>
> +
>
> +  /// Performance Reduction Tolerance Register.
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    PerformanceReductionToleranceRegister;
>
> +
>
> +  /// Time Window Register.
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    TimeWindowRegister;
>
> +
>
> +  /// Counter Wraparound Time
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    CounterWraparoundTimeBuffer;
>
> +  UINT32                                    CounterWraparoundTimeInteger;
>
> +
>
> +  /// Reference Performance Counter Register
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    ReferencePerformanceCounterRegister;
>
> +
>
> +  /// Delivered Performance Counter Register
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    DeliveredPerformanceCounterRegister;
>
> +
>
> +  /// Performance Limited Register
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    PerformanceLimitedRegister;
>
> +
>
> +  /// CPPC EnableRegister
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    CPPCEnableRegister;
>
> +
>
> +  /// Autonomous Selection Enable
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    AutonomousSelectionEnableBuffer;
>
> +  UINT32                                    AutonomousSelectionEnableInteger;
>
> +
>
> +  /// AutonomousActivity-WindowRegister
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    AutonomousActivityWindowRegister;
>
> +
>
> +  /// EnergyPerformance-PreferenceRegister
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    EnergyPerformancePreferenceRegister;
>
> +
>
> +  /// Reference Performance
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    ReferencePerformanceBuffer;
>
> +  UINT32                                    ReferencePerformanceInteger;
>
> +
>
> +  /// Lowest Frequency
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    LowestFrequencyBuffer;
>
> +  UINT32                                    LowestFrequencyInteger;
>
> +
>
> +  /// Nominal Frequency
>
> +  /// Optional
>
> +  EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE    NominalFrequencyBuffer;
>
> +  UINT32                                    NominalFrequencyInteger;
>
> +} CM_ARM_CPC_INFO;
>
> +
>
>   #pragma pack()
>
>   
>
>   #endif // ARM_NAMESPACE_OBJECTS_H_
>
> diff --git a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
>
> index c1b21d24a4..e2c608443b 100644
>
> --- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
>
> +++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c
>
> @@ -423,6 +423,83 @@ STATIC CONST CM_OBJ_PARSER  CmPciInterruptMapInfoParser[] = {
>
>       ARRAY_SIZE (CmArmGenericInterruptParser) },
>
>   };
>
>   
>
> +/** A parser for EArmObjCpcInfo.
>
> +*/
>
> +STATIC CONST CM_OBJ_PARSER  CmArmCpcInfoParser[] = {
[SAMI] I think the Revision field is missing here.
>
> +  { "HighestPerformanceBuffer",              sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "HighestPerformanceInteger",             4,                                              "0x%llx",  NULL },
[SAMI] Since this field is 32bit, I think the format specifier should be 
"0x%x". Same applies to the other format specifiers below.
>
> +  { "NominalPerformanceBuffer",              sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "NominalPerformanceInteger",             4,                                              "0x%llx",  NULL },
>
> +  { "LowestNonlinearPerformanceBuffer",      sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "LowestNonlinearPerformanceInteger",     4,                                              "0x%llx",  NULL },
>
> +  { "LowestPerformanceBuffer",               sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "LowestPerformanceInteger",              4,                                              "0x%llx",  NULL },
>
> +  { "GuaranteedPerformanceRegister",         sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "DesiredPerformanceRegister",            sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "MinimumPerformanceRegister",            sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "MaximumPerformanceRegister",            sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "PerformanceReductionToleranceRegister", sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "TimeWindowRegister",                    sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "CounterWraparoundTimeBuffer",           sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "CounterWraparoundTimeInteger",          4,                                              "0x%llx",  NULL },
>
> +  { "ReferencePerformanceCounterRegister",   sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "DeliveredPerformanceCounterRegister",   sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "PerformanceLimitedRegister",            sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "CPPCEnableRegister",                    sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "AutonomousSelectionEnableBuffer",       sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "AutonomousSelectionEnableInteger",      4,                                              "0x%llx",  NULL },
>
> +  { "AutonomousActivityWindowRegister",      sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "EnergyPerformancePreferenceRegister",   sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "ReferencePerformanceBuffer",            sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "ReferencePerformanceInteger",           4,                                              "0x%llx",  NULL },
>
> +  { "LowestFrequencyBuffer",                 sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "LowestFrequencyInteger",                4,                                              "0x%llx",  NULL },
>
> +  { "NominalFrequencyBuffer",                sizeof (EFI_ACPI_6_3_GENERIC_ADDRESS_STRUCTURE),
>
> +    NULL, NULL, AcpiGenericAddressParser,
>
> +    ARRAY_SIZE (AcpiGenericAddressParser) },
>
> +  { "NominalFrequencyInteger",               4,                                              "0x%llx",  NULL },
>
> +};
>
> +
>
>   /** A parser for Arm namespace objects.
>
>   */
>
>   STATIC CONST CM_OBJ_PARSER_ARRAY  ArmNamespaceObjectParser[] = {
>
> @@ -501,6 +578,8 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  ArmNamespaceObjectParser[] = {
>
>       ARRAY_SIZE (CmArmPciAddressMapInfoParser) },
>
>     { "EArmObjPciInterruptMapInfo",          CmPciInterruptMapInfoParser,
>
>       ARRAY_SIZE (CmPciInterruptMapInfoParser) },
>
> +  { "EArmObjCpcInfo",                      CmArmCpcInfoParser,
>
> +    ARRAY_SIZE (CmArmCpcInfoParser) },
>
>     { "EArmObjMax",                          NULL,                                  0                                },
>
>   };
>
>   
>