Re: [platforms: PATCH v4 3/7] SolidRun/Armada80x0McBin: Introduce board description library

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

On Sat, 10 Nov 2018 at 00:02, Marcin Wojtas <mw@semihalf.com> wrote:
>
> From: Tomasz Michalec <tm@semihalf.com>
>
> This patch implements ArmadaBoarDescLib library for
> Armada80x0McBin comunity board and add to it ArmadaBoardDescSdMmcGet
> function with description of connected Xenon host controllers.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc                                         |  3 +
>  Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf | 34 ++++++++++
>  Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c   | 66 ++++++++++++++++++++
>  3 files changed, 103 insertions(+)
>  create mode 100644 Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
>  create mode 100644 Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c
>
> diff --git a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
> index 52e2b9b..077224d 100644
> --- a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
> +++ b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBin.dsc
> @@ -55,6 +55,9 @@
>  [Components.AARCH64]
>    Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin.inf
>
> +[LibraryClasses.common]
> +  ArmadaBoardDescLib|Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
> +
>  ################################################################################
>  #
>  # Pcd Section - list of all EDK II PCD Entries defined by this Platform
> diff --git a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
> new file mode 100644
> index 0000000..63a4f66
> --- /dev/null
> +++ b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.inf
> @@ -0,0 +1,34 @@
> +## @file
> +#
> +#  Copyright (C) 2018, Marvell International Ltd. and its affiliates<BR>
> +#
> +#  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                    = 0x0001001A
> +  BASE_NAME                      = ArmadaMcBinBoardDescLib
> +  FILE_GUID                      = 8208558f-5f33-46e2-b5c5-43354384389e
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ArmadaBoardDescLib
> +
> +[Sources]
> +  Armada80x0McBinBoardDescLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  IoLib
> diff --git a/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c
> new file mode 100644
> index 0000000..9e38ce0
> --- /dev/null
> +++ b/Platform/SolidRun/Armada80x0McBin/Armada80x0McBinBoardDescLib/Armada80x0McBinBoardDescLib.c
> @@ -0,0 +1,66 @@
> +/**
> +*
> +*  Copyright (C) 2018, Marvell International Ltd. and its affiliates.
> +*
> +*  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 <Uefi.h>
> +
> +#include <Library/ArmadaBoardDescLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +//
> +// Order of devices in SdMmcDescTemplate has to be in par with ArmadaSoCDescLib
> +//
> +STATIC
> +MV_BOARD_SDMMC_DESC mMcBinSdMmcDescTemplate[] = {
> +  { /* eMMC 0xF06E0000 */
> +    0,     /* SOC will be filled by MvBoardDescDxe */
> +    0,     /* SdMmcDevCount will be filled by MvBoardDescDxe */
> +    FALSE, /* Xenon1v8Enabled */
> +    TRUE,  /* Xenon8BitBusEnabled */
> +    TRUE,  /* XenonSlowModeEnabled */
> +    0x40,  /* XenonTuningStepDivisor */
> +    EmbeddedSlot /* SlotType */
> +  },
> +  { /* SD/MMC 0xF2780000 */
> +    0,     /* SOC will be filled by MvBoardDescDxe */
> +    0,     /* SdMmcDevCount will be filled by MvBoardDescDxe */
> +    FALSE, /* Xenon1v8Enabled */
> +    FALSE, /* Xenon8BitBusEnabled */
> +    FALSE, /* XenonSlowModeEnabled */
> +    0x19,  /* XenonTuningStepDivisor */
> +    EmbeddedSlot /* SlotType */
> +  }
> +};
> +
> +EFI_STATUS
> +EFIAPI
> +ArmadaBoardDescSdMmcGet (
> +  IN OUT UINTN               *SdMmcDevCount,
> +  IN OUT MV_BOARD_SDMMC_DESC **SdMmcDesc

There are OUT not IN OUT

> +  )
> +{
> +  *SdMmcDevCount = ARRAY_SIZE (mMcBinSdMmcDescTemplate);
> +
> +  *SdMmcDesc = AllocateCopyPool (sizeof (mMcBinSdMmcDescTemplate),
> +                 &mMcBinSdMmcDescTemplate);
> +  if (*SdMmcDesc == NULL) {
> +    DEBUG ((DEBUG_ERROR, "%a: Cannot allocate memory\n", __FUNCTION__));
> +    return EFI_OUT_OF_RESOURCES;

Please do the checks first, and only assign the outputs when it is
guaranteed that you will return succes.

With those changes:

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


> +  }
> +
> +  return EFI_SUCCESS;
> +}
> --
> 2.7.4
>