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 > > --- > > .../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 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 */ typedefAML_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 }, > > }; > > >