Hi Sahil, There are multiple items that need fixing in this patch. Also, the changes in Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec from Patch 3/4 must be part of this patch. To save my time from reviewing the whole thing again, I am going to fix that before merging.
However, do find my response inline marked [SAMI] and please do go through my feedback so that you know what to look for next time. Regards, Sami Mujawar On 04/01/2024, 13:16, "sahil" <sa...@arm.com <mailto:sa...@arm.com>> wrote: This patch adds a PEI to parse NT_FW_CONFIG and pass it to other PEI modules(as PPI) and DXE modules(as HOB). Signed-off-by: sahil <sa...@arm.com <mailto:sa...@arm.com>> --- Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf | 41 ++++++ Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c | 132 ++++++++++++++++++++ [SAMI] I think these files should be moved to Silicon/ARM/NeoverseN1Soc/Library/N1SdpNtFwConfigPei. I will do that before merging. 2 files changed, 173 insertions(+) diff --git a/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf new file mode 100644 index 000000000000..363351b5a1df --- /dev/null +++ b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf @@ -0,0 +1,41 @@ +## @file +# This PEI module parse the NtFwConfig for N1Sdp platform and produce +# the PPI and HOB. +# +# Copyright (c) 2024, ARM Limited. All rights reserved.<BR> +# +# SPDX-License-Identifier: BSD-2-Clause-Patent +# +## + +[Defines] + INF_VERSION = 0x0001001B + BASE_NAME = N1SdpNtFwConfigPei + FILE_GUID = CE76D56C-D3A5-4763-9138-DF09E1D1B614 + MODULE_TYPE = PEIM + VERSION_STRING = 1.0 + ENTRY_POINT = NtFwConfigPeEntryPoint + +[Sources] + NtFwConfigPei.c + +[Packages] + EmbeddedPkg/EmbeddedPkg.dec + MdePkg/MdePkg.dec + Silicon/ARM/NeoverseN1Soc/NeoverseN1Soc.dec + +[LibraryClasses] + DebugLib + FdtLib + HobLib + PeimEntryPoint + +[Ppis] + gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid + gArmNeoverseN1SocParameterPpiGuid + +[Guids] + gArmNeoverseN1SocPlatformInfoDescriptorGuid + +[Depex] + gArmNeoverseN1SocParameterPpiGuid diff --git a/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c new file mode 100644 index 000000000000..330377d21a79 --- /dev/null +++ b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c @@ -0,0 +1,132 @@ +/** @file + + Copyright (c) 2024, ARM Limited. All rights reserved.<BR> + SPDX-License-Identifier: BSD-2-Clause-Patent + +**/ + +#include <Library/DebugLib.h> +#include <Library/HobLib.h> +#include <Library/PeiServicesLib.h> + +#include <NeoverseN1Soc.h> +#include <libfdt.h> + +STATIC EFI_PEI_PPI_DESCRIPTOR mPpi; + +/** + The entrypoint of the module, parse NtFwConfig and produce the PPI and HOB. + + @param[in] FileHandle Handle of the file being invoked. + @param[in] PeiServices Describes the list of possible PEI Services. + + @retval EFI_SUCCESS Either no NT_FW_CONFIG was given by EL3 firmware + OR the N1Sdp FDT HOB was successfully created. [SAMI] Would it not be enough to say Success? The documentation for the function already mentions what is expected. + @retval EFI_NOT_FOUND Error processing the DTB + @retval EFI_OUT_OF_RESOURCES Could not allocate memory for the HOB + @retval * Other errors are possible. +**/ +EFI_STATUS +EFIAPI +NtFwConfigPeEntryPoint ( + IN EFI_PEI_FILE_HANDLE FileHandle, + IN CONST EFI_PEI_SERVICES **PeiServices + ) +{ + CONST NEOVERSEN1SOC_EL3_FW_HANDOFF_PARAM_PPI *ParamPpi; + CONST UINT32 *Property; + INT32 Offset; + NEOVERSEN1SOC_PLAT_INFO *PlatInfo; + INT32 Status; [SAMI] This should be EFI_STATUS. + + PlatInfo = BuildGuidHob ( + &gArmNeoverseN1SocPlatformInfoDescriptorGuid, + sizeof (*PlatInfo) + ); + + if (PlatInfo == NULL) { + DEBUG (( + DEBUG_ERROR, + "[%a]: failed to allocate platform info HOB\n", + gEfiCallerBaseName + )); + return EFI_OUT_OF_RESOURCES; + } + + Status = PeiServicesLocatePpi ( + &gArmNeoverseN1SocParameterPpiGuid, + 0, + NULL, + (VOID **)&ParamPpi + ); [SAMI] I think Locate PPI should be done fist, that way if it fails we do not allocate the HOB. + + if (EFI_ERROR (Status)) { + DEBUG (( + DEBUG_ERROR, + "[%a]: failed to locate gArmNeoverseN1SocParameterPpiGuid - %r\n", + gEfiCallerBaseName, + Status + )); + return Status; + } + + if (fdt_check_header (ParamPpi->NtFwConfig) != 0) { [SAMI] There should be a check to see if ParamPpi is NULL. Also ParamPpi is dereferenced too many times in this function. It is advisable to have a local variable instead. + DEBUG ((DEBUG_ERROR, "Invalid DTB file %p passed\n", ParamPpi->NtFwConfig)); + return EFI_NOT_FOUND; + } + + Offset = fdt_subnode_offset (ParamPpi->NtFwConfig, 0, "platform-info"); + if (Offset == -FDT_ERR_NOTFOUND) { + DEBUG ((DEBUG_ERROR, "Invalid DTB : platform-info node not found\n")); + return EFI_NOT_FOUND; + } + + Property = fdt_getprop (ParamPpi->NtFwConfig, Offset, "local-ddr-size", NULL); + if (Property == NULL) { + DEBUG ((DEBUG_ERROR, "local-ddr-size property not found\n")); + return EFI_NOT_FOUND; + } + + PlatInfo->LocalDdrSize = fdt32_to_cpu (*Property); + + Property = fdt_getprop (ParamPpi->NtFwConfig, Offset, "remote-ddr-size", NULL); + if (Property == NULL) { + DEBUG ((DEBUG_ERROR, "remote-ddr-size property not found\n")); + return EFI_NOT_FOUND; + } + + PlatInfo->RemoteDdrSize = fdt32_to_cpu (*Property); + + Property = fdt_getprop (ParamPpi->NtFwConfig, Offset, "secondary-chip-count", NULL); + if (Property == NULL) { + DEBUG ((DEBUG_ERROR, "secondary-chip-count property not found\n")); + return EFI_NOT_FOUND; + } + + PlatInfo->SecondaryChipCount = fdt32_to_cpu (*Property); + + Property = fdt_getprop (ParamPpi->NtFwConfig, Offset, "multichip-mode", NULL); + if (Property == NULL) { + DEBUG ((DEBUG_ERROR, "multichip-mode property not found\n")); + return EFI_NOT_FOUND; + } + + PlatInfo->MultichipMode = fdt32_to_cpu (*Property); + + mPpi.Flags = EFI_PEI_PPI_DESCRIPTOR_PPI + | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST; + mPpi.Guid = &gArmNeoverseN1SocPlatformInfoDescriptorPpiGuid; + mPpi.Ppi = PlatInfo; + + Status = PeiServicesInstallPpi (&mPpi); + if (EFI_ERROR (Status)) { + DEBUG (( + DEBUG_ERROR, + "[%a]: failed to install PEI service - %r\n", + gEfiCallerBaseName, + Status + )); + } + + return Status; +} -- 2.25.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#116257): https://edk2.groups.io/g/devel/message/116257 Mute This Topic: https://groups.io/mt/103521644/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
