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


Sami Mujawar
 

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@...>

---

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

};

Join devel@edk2.groups.io to automatically receive all group messages.