Hi Jeff,

Please find my feedback inline marked [SAMI].

Regards,

Sami Mujawar

On 15/09/2022 02:10 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        |  80 ++++++++++

 2 files changed, 211 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;

[SAMI] I think the number of parameters to AmlCreateCpcNode () can be reduced if we pass the CPC information using a structure. For this I think we can do the following

1. Create a new file called DynamicTablesPkg/Include/Library/AmlCpcInfo.h

2. AmlCpcInfo.h shall define a structure as below

   /** 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)
*/

   typedef struct AmlCpcInfo {

   // Copy all the contents of the CM_ARM_CPC_INFO structure above.

  } AML_CPC_INFO;

3. In DynamicTablesPkg\Include\ArmNameSpaceObjects.h

      #include <Library/AmlCpcInfo.h>

and replace the definition for CM_ARM_CPC_INFO as

   /** 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 AML_CPC_INFO CM_ARM_CPC_INFO;
[/SAMI]

+

 #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..c7e0d51397 100644

--- a/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c

+++ b/DynamicTablesPkg/Library/Common/TableHelperLib/ConfigurationManagerObjectParser.c

@@ -423,6 +423,84 @@ STATIC CONST CM_OBJ_PARSER  CmPciInterruptMapInfoParser[] = {

     ARRAY_SIZE (CmArmGenericInterruptParser) },

 };

 

+/** A parser for EArmObjCpcInfo.

+*/

+STATIC CONST CM_OBJ_PARSER  CmArmCpcInfoParser[] = {

+  { "Revision",                              4,                                               "0x%llx", NULL },
[SAMI] I believe the format specifiers need to be updated to "0x%x" as the field width is 32 bits.

+  { "HighestPerformanceBuffer",              sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "HighestPerformanceInteger",             4,                                               "0x%llx", NULL },

+  { "NominalPerformanceBuffer",              sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "NominalPerformanceInteger",             4,                                               "0x%llx", NULL },

+  { "LowestNonlinearPerformanceBuffer",      sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "LowestNonlinearPerformanceInteger",     4,                                               "0x%llx", NULL },

+  { "LowestPerformanceBuffer",               sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "LowestPerformanceInteger",              4,                                               "0x%llx", NULL },

+  { "GuaranteedPerformanceRegister",         sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "DesiredPerformanceRegister",            sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "MinimumPerformanceRegister",            sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "MaximumPerformanceRegister",            sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "PerformanceReductionToleranceRegister", sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "TimeWindowRegister",                    sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "CounterWraparoundTimeBuffer",           sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "CounterWraparoundTimeInteger",          4,                                               "0x%llx", NULL },

+  { "ReferencePerformanceCounterRegister",   sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "DeliveredPerformanceCounterRegister",   sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "PerformanceLimitedRegister",            sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "CPPCEnableRegister",                    sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "AutonomousSelectionEnableBuffer",       sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "AutonomousSelectionEnableInteger",      4,                                               "0x%llx", NULL },

+  { "AutonomousActivityWindowRegister",      sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "EnergyPerformancePreferenceRegister",   sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "ReferencePerformanceBuffer",            sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "ReferencePerformanceInteger",           4,                                               "0x%llx", NULL },

+  { "LowestFrequencyBuffer",                 sizeof (EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE),

+    NULL, NULL, AcpiGenericAddressParser,

+    ARRAY_SIZE (AcpiGenericAddressParser) },

+  { "LowestFrequencyInteger",                4,                                               "0x%llx", NULL },

+  { "NominalFrequencyBuffer",                sizeof (EFI_ACPI_6_4_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 +579,8 @@ STATIC CONST CM_OBJ_PARSER_ARRAY  ArmNamespaceObjectParser[] = {

     ARRAY_SIZE (CmArmPciAddressMapInfoParser) },

   { "EArmObjPciInterruptMapInfo",          CmPciInterruptMapInfoParser,

     ARRAY_SIZE (CmPciInterruptMapInfoParser) },

+  { "EArmObjCpcInfo",                      CmArmCpcInfoParser,

+    ARRAY_SIZE (CmArmCpcInfoParser) },

   { "EArmObjMax",                          NULL,                                  0                                },

 };