Re: [edk2-devel] [edk2-platforms][PATCH V4 2/4] Platform/ARM/N1Sdp: Add N1SdpNtFwConfigPei PEI module
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" 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 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 ..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.
+#
+# 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 ..330377d21a79
--- /dev/null
+++ b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c
@@ -0,0 +1,132 @@
+/** @file
+
+ Copyright (c) 2024, ARM Limited. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include
+#include
+#include
+
+#include
+#include
+
+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"));
Re: [edk2-devel] [edk2-platforms][PATCH V4 2/4] Platform/ARM/N1Sdp: Add N1SdpNtFwConfigPei PEI module
Hi All,
Please find the links to previous discussions below :
V1 -
https://edk2.groups.io/g/devel/topic/96088980#100022
V2 -
https://edk2.groups.io/g/devel/topic/96671861#103652
V3 -
https://edk2.groups.io/g/devel/topic/100912169#112452
Thanks,
Sahil
On Thu, 4 Jan 2024 at 18:46, sahil 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
> ---
> Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf | 41 ++
> Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c | 132
>
> 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 ..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.
> +#
> +# 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 ..330377d21a79
> --- /dev/null
> +++ b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c
> @@ -0,0 +1,132 @@
> +/** @file
> +
> + Copyright (c) 2024, ARM Limited. All rights reserved.
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +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.
> + @retval EFI_NOT_FOUNDError 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;
> +
> + 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
> + );
> +
> + 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) {
> +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) {
> +
[edk2-devel] [edk2-platforms][PATCH V4 2/4] Platform/ARM/N1Sdp: Add N1SdpNtFwConfigPei PEI module
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
---
Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.inf | 41 ++
Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c | 132
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 ..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.
+#
+# 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 ..330377d21a79
--- /dev/null
+++ b/Platform/ARM/N1Sdp/Drivers/N1SdpNtFwConfigPei/NtFwConfigPei.c
@@ -0,0 +1,132 @@
+/** @file
+
+ Copyright (c) 2024, ARM Limited. All rights reserved.
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include
+#include
+#include
+
+#include
+#include
+
+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.
+ @retval EFI_NOT_FOUNDError 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;
+
+ 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
+ );
+
+ 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) {
+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"
