Thanks will fix those in v5. Will see if Sami has any other comments before pushing these.
-Jeff > -----Original Message----- > From: Pierre Gondois <pierre.gond...@arm.com> > Sent: Tuesday, September 20, 2022 8:54 AM > To: Jeff Brasen <jbra...@nvidia.com>; devel@edk2.groups.io > Cc: ardb+tianoc...@kernel.org; alexei.fedo...@arm.com; n...@arm.com; > sami.muja...@arm.com > Subject: Re: [PATCH v4 2/3] DynamicTablesPkg: AML Code generation to add > _CPC entries > > External email: Use caution opening links or attachments > > > Hello Jeff, > Just 2 minors comments. Maybe Sami will have more. > Otherwise, for the 3 patches: > Reviewed-by: Pierre Gondois <pierre.gond...@arm.com> > > Regards, > Pierre > > On 9/20/22 00:01, Jeff Brasen wrote: > > _CPC entries can describe CPU performance information. > > The object is described in ACPI 6.4 s8.4.7.1. > > "_CPC (Continuous Performance Control)". > > > > Add AmlCreateCpcNode() helper function to add _CPC entries to an > > existing CPU object. > > > > Signed-off-by: Jeff Brasen <jbra...@nvidia.com> > > --- > > .../Include/Library/AmlLib/AmlLib.h | 54 ++ > > .../Common/AmlLib/CodeGen/AmlCodeGen.c | 476 > ++++++++++++++++++ > > 2 files changed, 530 insertions(+) > > > > diff --git a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > index 39968660f2..ebaccba811 100644 > > --- a/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > +++ b/DynamicTablesPkg/Include/Library/AmlLib/AmlLib.h > > [snip] > > > + > > +/** Adds an integer or register to the package > > + > > + @ingroup CodeGenApis > > + > > + @param [in] Register If provided, register that will be added to > package > > + @param [in] Integer If Register is NULL, integer that will be > > added to > the package > > + @param [in] PackageNode Package to add value to > > + > > + @retval EFI_SUCCESS The function completed successfully. > > + @retval EFI_INVALID_PARAMETER Invalid parameter. > > + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > > +**/ > > +STATIC > > +EFI_STATUS > > +EFIAPI > > +AmlAddRegisterOrIntegerToPackage ( > > + IN EFI_ACPI_6_4_GENERIC_ADDRESS_STRUCTURE *Register OPTIONAL, > > + IN UINT32 Integer, > > + IN AML_OBJECT_NODE_HANDLE PackageNode > > + ) > > +{ > > + EFI_STATUS Status; > > + AML_OBJECT_NODE_HANDLE IntegerNode; > > + > > + IntegerNode = NULL; > > + > > I think that with IsNullGenericAddress(), the first > (Register != NULL) > is not necessary. > > > + if ((Register != NULL) && !IsNullGenericAddress (Register)) { > > + Status = AmlAddRegisterToPackage (Register, PackageNode); } else > > + { > > + Status = AmlCodeGenInteger (Integer, &IntegerNode); > > + if (EFI_ERROR (Status)) { > > + ASSERT_EFI_ERROR (Status); > > + return Status; > > + } > > + > > + Status = AmlVarListAddTail ( > > + (AML_NODE_HANDLE)PackageNode, > > + (AML_NODE_HANDLE)IntegerNode > > + ); > > + } > > + > > + if (EFI_ERROR (Status)) { > > + ASSERT_EFI_ERROR (Status); > > + if (IntegerNode != NULL) { > > + AmlDeleteTree ((AML_NODE_HANDLE)IntegerNode); > > + } > > + } > > + > > + return Status; > > +} > > + > > +/** Create a _CPC node. > > + > > + Creates and optionally adds the following node > > + Name(_CPC, Package() > > + { > > + NumEntries, // Integer > > + Revision, // Integer > > + HighestPerformance, // Integer or Buffer (Resource > Descriptor) > > + NominalPerformance, // Integer or Buffer (Resource > Descriptor) > > + LowestNonlinearPerformance, // Integer or Buffer (Resource > Descriptor) > > + LowestPerformance, // Integer or Buffer (Resource > Descriptor) > > + GuaranteedPerformanceRegister, // Buffer (Resource > > Descriptor) > > + DesiredPerformanceRegister , // Buffer (Resource > > Descriptor) > > + MinimumPerformanceRegister , // Buffer (Resource > > Descriptor) > > + MaximumPerformanceRegister , // Buffer (Resource > > Descriptor) > > + PerformanceReductionToleranceRegister, // Buffer (Resource > Descriptor) > > + TimeWindowRegister, // Buffer (Resource > > Descriptor) > > + CounterWraparoundTime, // Integer or Buffer (Resource > Descriptor) > > + ReferencePerformanceCounterRegister, // Buffer (Resource > Descriptor) > > + DeliveredPerformanceCounterRegister, // Buffer (Resource > Descriptor) > > + PerformanceLimitedRegister, // Buffer (Resource > > Descriptor) > > + CPPCEnableRegister // Buffer (Resource > > Descriptor) > > + AutonomousSelectionEnable, // Integer or Buffer (Resource > Descriptor) > > + AutonomousActivityWindowRegister, // Buffer (Resource > Descriptor) > > + EnergyPerformancePreferenceRegister, // Buffer (Resource > Descriptor) > > + ReferencePerformance // Integer or Buffer (Resource > Descriptor) > > + LowestFrequency, // Integer or Buffer (Resource > Descriptor) > > + NominalFrequency // Integer or Buffer (Resource > Descriptor) > > + }) > > + > > + If resource buffer is NULL then integer will be used. > > + > > + Cf. ACPI 6.4, s8.4.7.1 _CPC (Continuous Performance Control) > > + > > + @ingroup CodeGenApis > > + > > + @param [in] CpcInfo CpcInfo object > > + @param [in] ParentNode If provided, set ParentNode as the > parent > > + of the node created. > > + @param [out] NewCpcNode If success and provided, contains the > created node. > > + > > + @retval EFI_SUCCESS The function completed successfully. > > + @retval EFI_INVALID_PARAMETER Invalid parameter. > > + @retval EFI_OUT_OF_RESOURCES Failed to allocate memory. > > +**/ > > +EFI_STATUS > > +EFIAPI > > +AmlCreateCpcNode ( > > + IN AML_CPC_INFO *CpcInfo, > > + IN AML_NODE_HANDLE ParentNode OPTIONAL, > > + OUT AML_OBJECT_NODE_HANDLE *NewCpcNode OPTIONAL > > + ) > > +{ > > + EFI_STATUS Status; > > + AML_OBJECT_NODE_HANDLE CpcNode; > > + AML_OBJECT_NODE_HANDLE CpcPackage; > > + UINT32 NumberOfEntries; > > + > > + if ((CpcInfo == NULL) || > > + ((ParentNode == NULL) && (NewCpcNode == NULL))) { > > + ASSERT (0); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + // Revision 3 per ACPI 6.4 specification if (CpcInfo->Revision == > > + 3) { > > + // NumEntries 23 per ACPI 6.4 specification > > + NumberOfEntries = 23; > > + } else { > > + ASSERT (0); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > It seems CpcInfo is already checked earlier. It seems the checks below could > be part of the other checks above (where ParentNode is checked). > > > + if ((CpcInfo == NULL) || > > + (IsNullGenericAddress (&CpcInfo->HighestPerformanceBuffer) && > > + (CpcInfo->HighestPerformanceInteger == 0)) || > > + (IsNullGenericAddress (&CpcInfo->NominalPerformanceBuffer) && > > + (CpcInfo->NominalPerformanceInteger == 0)) || > > + (IsNullGenericAddress (&CpcInfo- > >LowestNonlinearPerformanceBuffer) && > > + (CpcInfo->LowestNonlinearPerformanceInteger == 0)) || > > + (IsNullGenericAddress (&CpcInfo->LowestPerformanceBuffer) && > > + (CpcInfo->LowestPerformanceInteger == 0)) || > > + IsNullGenericAddress (&CpcInfo->DesiredPerformanceRegister) || > > + IsNullGenericAddress (&CpcInfo- > >ReferencePerformanceCounterRegister) || > > + IsNullGenericAddress (&CpcInfo- > >DeliveredPerformanceCounterRegister) || > > + IsNullGenericAddress (&CpcInfo->PerformanceLimitedRegister)) > > + { > > + ASSERT (0); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + CpcPackage = NULL; > > + > > + Status = AmlCodeGenNamePackage ("_CPC", NULL, &CpcNode); if > > + (EFI_ERROR (Status)) { > > + ASSERT_EFI_ERROR (Status); > > + return Status; > > + } > > + > > [snip] -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#94013): https://edk2.groups.io/g/devel/message/94013 Mute This Topic: https://groups.io/mt/93792018/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-