Hi Pierre, Thank you for this patch. Please find my feedback inline marked [SAMI].
Regards, Sami Mujawar On 12/12/2023, 09:31, "[email protected] <mailto:[email protected]>" <[email protected] <mailto:[email protected]>> wrote: From: Pierre Gondois <[email protected] <mailto:[email protected]>> The SsdtCpuTopologyGenerator can generate _CPC objects. This is done by querying the SCP for the relevant performance state information through SCMI. CM_ARM_CPC_INFO are then populated and used to generate _CPC objects in the Ssdt Cpu topology. Use the DynamicTablesScmiInfoLib and add the handling to generate _CPC information. Note that using _CPC is only possible if SCP is correctly tuned to advertise performance levels on an abstract and unified scale. A basic check is done to prevent the _CPC generation otherwise. Signed-off-by: Pierre Gondois <[email protected] <mailto:[email protected]>> --- .../ConfigurationManager.c | 233 +++++++++++++++++- .../ConfigurationManager.h | 3 + .../ConfigurationManagerDxe.inf | 1 + 3 files changed, 234 insertions(+), 3 deletions(-) diff --git a/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c b/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c index 15292f810a28..ba5727f65619 100644 --- a/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c +++ b/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.c @@ -14,6 +14,7 @@ #include <IndustryStandard/MemoryMappedConfigurationSpaceAccessTable.h> #include <IndustryStandard/SerialPortConsoleRedirectionTable.h> #include <Library/ArmLib.h> +#include <Library/DynamicTablesScmiInfoLib.h> #include <Library/DebugLib.h> #include <Library/IoLib.h> #include <Library/PcdLib.h> @@ -756,6 +757,8 @@ EDKII_PLATFORM_REPOSITORY_INFO ArmJunoPlatformRepositoryInfo = { 4, }, }, + { // CPC info, dynamically populated. + }, [SAMI] I think we should remove the above 2 lines. Please see my feedback for patch 2/3. Also, if new fields are added then they should be before the CPC info objects. That way we can skip the initialisation of the CPC info in here. [/SAMI] }; /** A helper function for returning the Configuration Manager Objects. @@ -1212,6 +1215,55 @@ GetPsdInfo ( return EFI_SUCCESS; } +/** Return Cpc Info. + + @param [in] This Pointer to the Configuration Manager Protocol. + @param [in] CmObjectId The Object ID of the CM object requested + @param [in] SearchToken A unique token for identifying the requested + CM_ARM_PCI_INTERRUPT_MAP_INFO object. + @param [in, out] CmObject Pointer to the Configuration Manager Object + descriptor describing the requested Object. + + @retval EFI_SUCCESS Success. + @retval EFI_INVALID_PARAMETER A parameter is invalid. + @retval EFI_NOT_FOUND The required object information is not found. +**/ +EFI_STATUS +EFIAPI +GetCpcInfo ( + IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL * CONST This, + IN CONST CM_OBJECT_ID CmObjectId, + IN CONST CM_OBJECT_TOKEN SearchToken, + IN OUT CM_OBJ_DESCRIPTOR * CONST CmObject + ) +{ + EDKII_PLATFORM_REPOSITORY_INFO * PlatformRepo; + UINT32 TotalObjCount; + UINT32 ObjIndex; + + if ((This == NULL) || (CmObject == NULL)) { + ASSERT (This != NULL); + ASSERT (CmObject != NULL); + return EFI_INVALID_PARAMETER; + } + + PlatformRepo = This->PlatRepoInfo; + + TotalObjCount = ARRAY_SIZE (PlatformRepo->CpcInfo); + + for (ObjIndex = 0; ObjIndex < TotalObjCount; ObjIndex++) { + if (SearchToken == (CM_OBJECT_TOKEN)&PlatformRepo->CpcInfo[ObjIndex]) { + CmObject->ObjectId = CmObjectId; + CmObject->Size = sizeof (PlatformRepo->CpcInfo[ObjIndex]); + CmObject->Data = (VOID*)&PlatformRepo->CpcInfo[ObjIndex]; + CmObject->Count = 1; + return EFI_SUCCESS; + } + } + + return EFI_SUCCESS; +} + /** Return a list of Configuration Manager object references pointed to by the given input token. @@ -1633,6 +1685,19 @@ GetArmNameSpaceObject ( ); break; + case EArmObjCpcInfo: + Status = HandleCmObjectRefByToken ( + This, + CmObjectId, + PlatformRepo->CpcInfo, + sizeof (PlatformRepo->CpcInfo), + ARRAY_SIZE (PlatformRepo->CpcInfo), + Token, + GetCpcInfo, + CmObject + ); + break; + default: { Status = EFI_NOT_FOUND; DEBUG (( @@ -1788,6 +1853,151 @@ EDKII_CONFIGURATION_MANAGER_PROTOCOL ArmJunoPlatformConfigManagerProtocol = { &ArmJunoPlatformRepositoryInfo }; +/** Clear Cpc information. + + If populating _CPC information fails, remove GicC tokens pointing + to Cpc CmObj to avoid creating corrupted _CPC objects. + + @param [in] PlatformRepo Platfom Info repository. + + @retval EFI_SUCCESS Success. +**/ +STATIC +EFI_STATUS +EFIAPI +ClearCpcInfo ( + EDKII_PLATFORM_REPOSITORY_INFO *PlatformRepo + ) +{ + CM_ARM_GICC_INFO *GicCInfo; + + GicCInfo = (CM_ARM_GICC_INFO*)&PlatformRepo->GicCInfo; + + GicCInfo[0].CpcToken = CM_NULL_TOKEN; + GicCInfo[1].CpcToken = CM_NULL_TOKEN; + GicCInfo[2].CpcToken = CM_NULL_TOKEN; + GicCInfo[3].CpcToken = CM_NULL_TOKEN; + GicCInfo[4].CpcToken = CM_NULL_TOKEN; + GicCInfo[5].CpcToken = CM_NULL_TOKEN; + + return EFI_SUCCESS; +} + +/** Use the SCMI protocol to populate CPC objects dynamically. + + @param [in] PlatformRepo Platfom Info repository. + @param [in] DomainId Id of the DVFS domain to probe. + + @retval EFI_SUCCESS Success. + @retval EFI_INVALID_PARAMETER A parameter is invalid. + @retval EFI_UNSUPPORTED Not supported. + @retval !(EFI_SUCCESS) An error occured. +**/ +STATIC +EFI_STATUS +EFIAPI +PopulateCpcInfo ( + EDKII_PLATFORM_REPOSITORY_INFO *PlatformRepo, + IN UINT32 DomainId + ) +{ + EFI_STATUS Status; + CM_ARM_GICC_INFO *GicCInfo; + AML_CPC_INFO *CpcInfo; + + if ((PlatformRepo == NULL) || + ((DomainId != PSD_BIG_DOMAIN_ID) && + (DomainId != PSD_LITTLE_DOMAIN_ID))) { + Status = EFI_INVALID_PARAMETER; + ASSERT_EFI_ERROR (Status); + return Status; + } + + if (PlatformRepo->JunoRevision != JUNO_REVISION_R2) { + /* Available frequencies are different on Juno R[0|1|2]. _CPC was + * only tested on Juno R2, so only enable support for this version. + */ + Status = EFI_UNSUPPORTED; + return Status; + } [SAMI] I think this check should be moved outside this function into InitializePlatformRepository() and if the Juno revision is not R2, the code in InitializePlatformRepository() should perform default initialisation by calling ClearCpcInfo(). [/SAMI] + + CpcInfo = &PlatformRepo->CpcInfo[DomainId]; + GicCInfo = (CM_ARM_GICC_INFO*)&PlatformRepo->GicCInfo; + + Status = DynamicTablesScmiInfoGetFastChannel ( + PlatformRepo->PsdInfo[DomainId].Domain, + CpcInfo + ); + if (EFI_ERROR (Status)) { + return Status; + } + + /* CPPC must advertise performances on a 'continuous, abstract, unit-less + performance scale', i.e. CPU performances on an asymmetric platform + nust be represented on a unified scale. + CPU performance values are obtained from SCP through SCMI and advertised + to the OS via the _CPC objects. SCP currently maps performance requests + to frequency requests. + Thus, SCP must be modified to advertise (and correctly handle) + performance values on a unified scale. + + Check that SCP is using a unified scale by checking that the advertised + lowest/nominal frequencies are not the default ones. + */ + if (((DomainId == PSD_BIG_DOMAIN_ID) && + (CpcInfo->LowestPerformanceInteger == 600000000) && + (CpcInfo->NominalPerformanceInteger == 1000000000)) || + ((DomainId == PSD_LITTLE_DOMAIN_ID) && + (CpcInfo->LowestPerformanceInteger == 450000000) && + (CpcInfo->NominalPerformanceInteger == 800000000))) { + return EFI_UNSUPPORTED; + } + + // Juno R2's lowest/nominal frequencies. + // Nominal frequency != Highest frequency. + if (DomainId == PSD_BIG_DOMAIN_ID) { + CpcInfo->LowestFrequencyInteger = 600; + CpcInfo->NominalFrequencyInteger = 1000; + } else { + CpcInfo->LowestFrequencyInteger = 450; + CpcInfo->NominalFrequencyInteger = 800; + } + + // The mapping Psd -> CPUs is available here. + if (DomainId == PSD_BIG_DOMAIN_ID) { + GicCInfo[0].CpcToken = (CM_OBJECT_TOKEN)CpcInfo; + GicCInfo[1].CpcToken = (CM_OBJECT_TOKEN)CpcInfo; + } else { + GicCInfo[2].CpcToken = (CM_OBJECT_TOKEN)CpcInfo; + GicCInfo[3].CpcToken = (CM_OBJECT_TOKEN)CpcInfo; + GicCInfo[4].CpcToken = (CM_OBJECT_TOKEN)CpcInfo; + GicCInfo[5].CpcToken = (CM_OBJECT_TOKEN)CpcInfo; + } + + /* + Arm advises to use FFH to the following registers which uses AMU counters: + - ReferencePerformanceCounterRegister + - DeliveredPerformanceCounterRegister + Cf. Arm Functional Fixed Hardware Specification + s3.2 Performance management and Collaborative Processor Performance Control + + AMU is not supported by the Juno, so clear these registers. + */ + CpcInfo->ReferencePerformanceCounterRegister.AddressSpaceId = EFI_ACPI_6_5_SYSTEM_MEMORY; + CpcInfo->ReferencePerformanceCounterRegister.RegisterBitWidth = 0; + CpcInfo->ReferencePerformanceCounterRegister.RegisterBitOffset = 0; + CpcInfo->ReferencePerformanceCounterRegister.AccessSize = 0; + CpcInfo->ReferencePerformanceCounterRegister.Address = 0; + + CpcInfo->DeliveredPerformanceCounterRegister.AddressSpaceId = EFI_ACPI_6_5_SYSTEM_MEMORY; + CpcInfo->DeliveredPerformanceCounterRegister.RegisterBitWidth = 0; + CpcInfo->DeliveredPerformanceCounterRegister.RegisterBitOffset = 0; + CpcInfo->DeliveredPerformanceCounterRegister.AccessSize = 0; + CpcInfo->DeliveredPerformanceCounterRegister.Address = 0; + + return Status; +} + /** Entrypoint of Configuration Manager Dxe. @@ -1807,6 +2017,8 @@ ConfigurationManagerDxeInitialize ( ) { EFI_STATUS Status; + UINT32 Index; + BOOLEAN CpcFailed; Status = gBS->InstallProtocolInterface ( &ImageHandle, @@ -1821,7 +2033,7 @@ ConfigurationManagerDxeInitialize ( " Status = %r\n", Status )); - goto error_handler; + return Status; } Status = InitializePlatformRepository ( @@ -1836,6 +2048,21 @@ ConfigurationManagerDxeInitialize ( )); } -error_handler: - return Status; + CpcFailed = FALSE; + for (Index = 0; Index < PSD_DOMAIN_COUNT; Index++) { + Status = PopulateCpcInfo (&ArmJunoPlatformRepositoryInfo, Index); + if (EFI_ERROR (Status)) { + DEBUG ((DEBUG_WARN, "WARN: Could not populate _CPC.\n")); + CpcFailed = TRUE; + break; + } + } + + if (CpcFailed) { + // _CPC information is not mandatory and SCP might not support some + // SCMI requests. Failing should not prevent from booting. + ClearCpcInfo (&ArmJunoPlatformRepositoryInfo); + } [SAMI] The above code should be moved in InitializePlatformRepository(). + + return EFI_SUCCESS; } diff --git a/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h b/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h index e58e9cbecb23..fe3edd548faf 100644 --- a/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h +++ b/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManager.h @@ -303,6 +303,9 @@ typedef struct PlatformRepositoryInfo { // Power domains CM_ARM_PSD_INFO PsdInfo[PSD_DOMAIN_COUNT]; + // Cpc info (1 for each PSD domain) + CM_ARM_CPC_INFO CpcInfo[PSD_DOMAIN_COUNT]; + /// Juno Board Revision UINT32 JunoRevision; } EDKII_PLATFORM_REPOSITORY_INFO; diff --git a/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf b/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf index 91bffe8d5d82..53060bf56531 100644 --- a/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf +++ b/Platform/ARM/JunoPkg/ConfigurationManager/ConfigurationManagerDxe/ConfigurationManagerDxe.inf @@ -35,6 +35,7 @@ [Packages] [LibraryClasses] ArmPlatformLib + DynamicTablesScmiInfoLib PrintLib UefiBootServicesTableLib UefiDriverEntryPoint -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#114390): https://edk2.groups.io/g/devel/message/114390 Mute This Topic: https://groups.io/mt/103127068/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
