Re: [edk2-devel] [edk2-platforms][PATCH V4 2/4] Platform/ARM/N1Sdp: Add N1SdpNtFwConfigPei PEI module

["Sami Mujawar" <sami.mujawar@arm.com>]

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" <sahil@arm.com <mailto:sahil@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 <sahil@arm.com <mailto:sahil@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-