From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.5882.1634117580959247644 for ; Wed, 13 Oct 2021 02:33:01 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 68E591063; Wed, 13 Oct 2021 02:33:00 -0700 (PDT) Received: from [192.168.1.16] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 643A03F70D; Wed, 13 Oct 2021 02:32:59 -0700 (PDT) Message-ID: <10acb554-d94e-ecc3-ce53-786b0131990e@arm.com> Date: Wed, 13 Oct 2021 10:32:49 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 From: "PierreGondois" Subject: Re: [edk2-devel] [PATCH v2 3/7] Platform/ARM/N1Sdp: Introduce platform DXE driver To: devel@edk2.groups.io, khasim.mohammed@arm.com Cc: Deepak Pandey , Sami Mujawar References: <20211010182956.13526-1-khasim.mohammed@arm.com> <20211010182956.13526-4-khasim.mohammed@arm.com> In-Reply-To: <20211010182956.13526-4-khasim.mohammed@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Hi Khasim, I have some remarks about the patch: On 10/10/21 19:29, Khasim Mohammed via groups.io wrote: > Add an initial platform DXE driver and support for ramdisk devices. > > Signed-off-by: Deepak Pandey > Signed-off-by: Khasim Syed Mohammed > --- > .../N1Sdp/Drivers/PlatformDxe/PlatformDxe.c | 51 +++++++++++++++++++ > .../N1Sdp/Drivers/PlatformDxe/PlatformDxe.inf | 44 ++++++++++++++++ > 2 files changed, 95 insertions(+) > create mode 100644 Platform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.= c > create mode 100644 Platform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.= inf > > diff --git a/Platform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.c b/Pla= tform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.c > new file mode 100644 > index 0000000000..3abe2228ad > --- /dev/null > +++ b/Platform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.c > @@ -0,0 +1,51 @@ > +/** @file > + > + Copyright (c) 2021, ARM Limited. All rights reserved.
> + > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > + > +/** > + Entrypoint of Platform Dxe Driver > + > + @param ImageHandle[in] The firmware allocated handle for the = EFI image. > + @param SystemTable[in] A pointer to the EFI System Table. > + > + @retval EFI_SUCCESS The entry point is executed successful= ly. It seems the function can also return other error codes. > +**/ > +EFI_STATUS > +EFIAPI > +ArmN1SdpEntryPoint ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + EFI_STATUS Status; > + EFI_RAM_DISK_PROTOCOL *RamDisk; > + EFI_DEVICE_PATH_PROTOCOL *DevicePath; > + > + Status =3D EFI_UNSUPPORTED; > + if (FeaturePcdGet (PcdRamDiskSupported)) { > + Status =3D gBS->LocateProtocol (&gEfiRamDiskProtocolGuid, NULL, (V= OID**) &RamDisk); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: Couldn't find the RAM Disk protocol - = %r\n", __FUNCTION__, Status)); These 2 lines seem quite long, is it possible to split them on multiple lines ? (Same for the debug message below) > + return Status; > + } > + > + Status =3D RamDisk->Register ( > + (UINTN)PcdGet32 (PcdRamDiskBase), > + (UINTN)PcdGet32 (PcdRamDiskSize), > + &gEfiVirtualCdGuid, > + NULL, > + &DevicePath); I think the gEfiVirtualCdGuid should be listed in a [Guid] section of the .inf file with the following comment: ## SOMETIMES_CONSUMES=C2=A0=C2=A0 ## GUID The same should be done for Platform/ARM/Morello/Drivers/PlatformDxe/PlatformDxeFvp.c NIT: is it possible to have the closing parenthesis on a new line as: &DevicePath ); > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_ERROR, "%a: Failed to register RAM Disk - %r\n", _= _FUNCTION__, Status)); > + } > + } > + return Status; > +} > diff --git a/Platform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.inf b/P= latform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.inf > new file mode 100644 > index 0000000000..925bde4063 > --- /dev/null > +++ b/Platform/ARM/N1Sdp/Drivers/PlatformDxe/PlatformDxe.inf > @@ -0,0 +1,44 @@ > +## @file > +# Platform DXE driver for N1Sdp > +# > +# Copyright (c) 2021, ARM Limited. All rights reserved.
> +# > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +## > + > +[Defines] > + INF_VERSION =3D 0x0001001B > + BASE_NAME =3D PlatformDxe > + FILE_GUID =3D 11fc8b5a-377d-47a8-aee9-0093d3d34= 07f > + MODULE_TYPE =3D DXE_DRIVER > + VERSION_STRING =3D 1.0 > + ENTRY_POINT =3D ArmN1SdpEntryPoint > + > +[Sources.common] > + PlatformDxe.c > + > +[Packages] > + ArmPkg/ArmPkg.dec > + ArmPlatformPkg/ArmPlatformPkg.dec > + EmbeddedPkg/EmbeddedPkg.dec > + MdeModulePkg/MdeModulePkg.dec > + MdePkg/MdePkg.dec > + Platform/ARM/N1Sdp/N1SdpPlatform.dec > + > +[LibraryClasses] > + HobLib > + UefiDriverEntryPoint > + > +[Protocols] > + gEfiRamDiskProtocolGuid > + > +[FeaturePcd] > + gArmN1SdpTokenSpaceGuid.PcdRamDiskSupported > + > +[FixedPcd] > + gArmN1SdpTokenSpaceGuid.PcdRamDiskBase > + gArmN1SdpTokenSpaceGuid.PcdRamDiskSize > + > +[Depex] > + gEfiRamDiskProtocolGuid