Re: [PATCH edk2-platforms v2 3/6] Platform/ARM/Sgi: add initial platform dxe driver implementation

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 12 April 2018 at 20:47, Thomas Abraham <thomas.abraham@arm.com> wrote:
> From: Daniil Egranov <daniil.egranov@arm.com>
>
> Add a initial platform dxe driver which starts of being almost
> an empty implemenation.

implemenTation

>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Daniil Egranov <daniil.egranov@arm.com>
> Signed-off-by: Thomas Abraham <thomas.abraham@arm.com>
> ---
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c   | 25 ++++++++
>  .../ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf | 74 ++++++++++++++++++++++
>  2 files changed, 99 insertions(+)
>  create mode 100644 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
>  create mode 100644 Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
>
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> new file mode 100644
> index 0000000..2da768a
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.c
> @@ -0,0 +1,25 @@
> +/** @file
> +*
> +*  Copyright (c) 2018, ARM Limited. All rights reserved.
> +*
> +*  This program and the accompanying materials are licensed and made available
> +*  under the terms and conditions of the BSD License which accompanies this
> +*  distribution.  The full text of the license may be found at
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +*  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +*
> +**/
> +
> +#include <Library/DebugLib.h>
> +
> +EFI_STATUS
> +EFIAPI
> +ArmSgiPkgEntryPoint (
> +  IN EFI_HANDLE         ImageHandle,
> +  IN EFI_SYSTEM_TABLE   *SystemTable
> +  )
> +{
> +  return EFI_SUCCESS;
> +}
> diff --git a/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> new file mode 100644
> index 0000000..379d7f4
> --- /dev/null
> +++ b/Platform/ARM/SgiPkg/Drivers/PlatformDxe/PlatformDxe.inf
> @@ -0,0 +1,74 @@
> +#
> +#  Copyright (c) 2018, ARM Limited. All rights reserved.
> +#
> +#  This program and the accompanying materials are licensed and made available
> +#  under the terms and conditions of the BSD License which accompanies this
> +#  distribution.  The full text of the license may be found at
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +#
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005

0x0001001A

> +  BASE_NAME                      = PlatformDxe
> +  FILE_GUID                      = 54cee352-c4cd-4d80-8524-54325c3a528e
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  ENTRY_POINT                    = ArmSgiPkgEntryPoint
> +
> +[Sources.common]
> +  PlatformDxe.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  edk2-platforms/Platform/ARM/SgiPkg/SgiPlatform.dec

Drop the edk2-platforms prefix here

> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  OvmfPkg/OvmfPkg.dec
> +
> +[LibraryClasses]
> +  AcpiLib
> +  BaseMemoryLib
> +  DebugLib
> +  DxeServicesTableLib
> +  FdtLib
> +  HobLib
> +  IoLib
> +  PcdLib
> +  PrintLib
> +  SerialPortLib
> +  UefiBootServicesTableLib
> +  UefiRuntimeServicesTableLib
> +  UefiLib
> +  UefiDriverEntryPoint
> +  VirtioMmioDeviceLib
> +

Do you really need all these packages and library classes for an empty
implementation?

> +[Guids]
> +  #gArmGlobalVariableGuid
> +  gEfiEndOfDxeEventGroupGuid
> +  gEfiFileInfoGuid
> +  gEfiHobListGuid
> +  gFdtTableGuid
> +  gEfiAcpi10TableGuid
> +  gEfiAcpiTableGuid
> +

Same here

> +[Protocols]
> +  gEfiBlockIoProtocolGuid
> +  gEfiDevicePathFromTextProtocolGuid
> +  gEfiSimpleFileSystemProtocolGuid
> +

and here

> +[FeaturePcd]
> +  gArmSgiTokenSpaceGuid.PcdVirtioSupported
> +
> +[FixedPcd]
> +  gArmTokenSpaceGuid.PcdSystemMemoryBase
> +  gArmTokenSpaceGuid.PcdSystemMemorySize
> +
> +  gArmTokenSpaceGuid.PcdHypFvBaseAddress
> +  gArmTokenSpaceGuid.PcdHypFvSize
> +
> +[Depex]
> +  TRUE

I think it is fine to start out with an empty file and add things as
you go, but this should also cover the dependencies declared in this
file. I.e., add package, library class, guid and protocol dependencies
as needed when you add the functionality to the driver.