Re: [edk2-platforms: PATCH 3/6] Marvell/Armada7k8k: Implement custom DtLoaderLib

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

On Fri, 12 Apr 2019 at 03:20, Marcin Wojtas <mw@semihalf.com> wrote:
>
> In order to provide special handling for DT environment,
> implement custom version of DtLoaderLib, which registers
> ExitBootServices event upon generic 'DtAcpiPref' variable.
> The routine is responsible for disabling the PMU workaround,
> that is valid only for UEFI and OS + APCI.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc                                              |   2 +-
>  Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.inf |  43 +++++++
>  Silicon/Marvell/Include/IndustryStandard/MvSmc.h                                           |   2 +
>  Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.c   | 130 ++++++++++++++++++++
>  4 files changed, 176 insertions(+), 1 deletion(-)
>  create mode 100644 Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.inf
>  create mode 100644 Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.c
>
> diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> index 8291582..5ed742f 100644
> --- a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> +++ b/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc
> @@ -200,7 +200,7 @@
>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
>    MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>    NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/NonDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf
> -  DtPlatformDtbLoaderLib|EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.inf
> +  DtPlatformDtbLoaderLib|Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.inf
>
>  [LibraryClasses.common.UEFI_APPLICATION]
>    PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePerformanceLib.inf
> diff --git a/Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.inf b/Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.inf
> new file mode 100644
> index 0000000..ec3f3a5
> --- /dev/null
> +++ b/Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.inf
> @@ -0,0 +1,43 @@
> +/** @file
> +*
> +*  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
> +*  Copyright (c) 2018, Marvell International Ltd. and its affiliates
> +*

Please fix the date here as well as below

> +*  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                      = DxeDtPlatformDtbLoaderLibDefault
> +  FILE_GUID                      = dde55569-ad3f-421d-b94b-3be66bb916ce
> +  MODULE_TYPE                    = DXE_DRIVER
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = DtPlatformDtbLoaderLib|DXE_DRIVER
> +
> +[Sources]
> +  DxeDtPlatformDtbLoaderLib.c
> +
> +[Packages]
> +  ArmPkg/ArmPkg.dec
> +  EmbeddedPkg/EmbeddedPkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  ArmSmcLib
> +  BaseLib
> +  DebugLib
> +  DxeServicesLib
> +  MemoryAllocationLib
> +  UefiRuntimeServicesTableLib
> +

Are you really using all of these?

> +[Guids]
> +  gDtPlatformDefaultDtbFileGuid
> +  gDtPlatformFormSetGuid

and these?

> diff --git a/Silicon/Marvell/Include/IndustryStandard/MvSmc.h b/Silicon/Marvell/Include/IndustryStandard/MvSmc.h
> index 0c90f11..e5c89d9 100644
> --- a/Silicon/Marvell/Include/IndustryStandard/MvSmc.h
> +++ b/Silicon/Marvell/Include/IndustryStandard/MvSmc.h
> @@ -20,5 +20,7 @@
>  #define MV_SMC_ID_COMPHY_POWER_OFF        0x82000002
>  #define MV_SMC_ID_COMPHY_PLL_LOCK         0x82000003
>  #define MV_SMC_ID_DRAM_SIZE               0x82000010
> +#define MV_SMC_ID_PMU_IRQ_ENABLE          0x82000012
> +#define MV_SMC_ID_PMU_IRQ_DISABLE         0x82000013
>
>  #endif //__MV_SMC_H__
> diff --git a/Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.c b/Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.c
> new file mode 100644
> index 0000000..b189f47
> --- /dev/null
> +++ b/Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.c
> @@ -0,0 +1,130 @@
> +/** @file
> +*
> +*  Copyright (c) 2017, Linaro, Ltd. All rights reserved.
> +*  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 <PiDxe.h>
> +
> +#include <IndustryStandard/MvSmc.h>
> +
> +#include <Library/ArmSmcLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DxeServicesLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiRuntimeServicesTableLib.h>
> +

Please make sure that all libraries you include are listed in the .inf

> +STATIC EFI_EVENT mArmada7k8kExitBootServicesEvent;
> +
> +#define DT_ACPI_SELECT_DT       0x0
> +#define DT_ACPI_SELECT_ACPI     0x1
> +
> +typedef struct {
> +  UINT8         Pref;
> +  UINT8         Reserved[3];
> +} DT_ACPI_VARSTORE_DATA;
> +
> +/**
> +  Disable extra EL3 handling of the PMU interrupts for DT world.
> +
> +  @param[in]   Event                  Event structure
> +  @param[in]   Context                Notification context
> +
> +**/
> +STATIC
> +VOID
> +Armada7k8kExitBootEvent (
> +  IN EFI_EVENT  Event,
> +  IN VOID      *Context
> +  )
> +{
> +  ARM_SMC_ARGS SmcRegs = {0};
> +
> +  SmcRegs.Arg0 = MV_SMC_ID_PMU_IRQ_DISABLE;
> +  ArmCallSmc (&SmcRegs);
> +
> +  return;
> +}
> +
> +/**
> +  Return a pool allocated copy of the DTB image that is appropriate for
> +  booting the current platform via DT.
> +
> +  @param[out]   Dtb                   Pointer to the DTB copy
> +  @param[out]   DtbSize               Size of the DTB copy
> +
> +  @retval       EFI_SUCCESS           Operation completed successfully
> +  @retval       EFI_NOT_FOUND         No suitable DTB image could be located
> +  @retval       EFI_OUT_OF_RESOURCES  No pool memory available
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +DtPlatformLoadDtb (
> +  OUT   VOID        **Dtb,
> +  OUT   UINTN       *DtbSize
> +  )
> +{
> +  DT_ACPI_VARSTORE_DATA  DtAcpiPref;
> +  ARM_SMC_ARGS           SmcRegs = {0};
> +  EFI_STATUS             Status;
> +  VOID                   *OrigDtb;
> +  VOID                   *CopyDtb;
> +  UINTN                  OrigDtbSize;
> +  UINTN                  BufferSize;
> +
> +  Status = GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGuid,
> +             EFI_SECTION_RAW,
> +             0,
> +             &OrigDtb,
> +             &OrigDtbSize);
> +  if (EFI_ERROR (Status)) {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  CopyDtb = AllocateCopyPool (OrigDtbSize, OrigDtb);
> +  if (CopyDtb == NULL) {
> +    return EFI_OUT_OF_RESOURCES;
> +  }
> +
> +  *Dtb = CopyDtb;
> +  *DtbSize = OrigDtbSize;
> +
> +  /* Enable EL3 handler for regardless HW description */
> +  SmcRegs.Arg0 = MV_SMC_ID_PMU_IRQ_ENABLE;
> +  ArmCallSmc (&SmcRegs);
> +
> +  /*
> +   * Get the current DT/ACPI preference from the DtAcpiPref variable.
> +   * Register ExitBootServices event only in case the DT will be used.
> +   */
> +  BufferSize = sizeof (DtAcpiPref);
> +  Status = gRT->GetVariable (L"DtAcpiPref",
> +                  &gDtPlatformFormSetGuid,
> +                  NULL,
> +                  &BufferSize,
> +                  &DtAcpiPref);
> +  if (EFI_ERROR (Status) || DtAcpiPref.Pref == DT_ACPI_SELECT_DT) {
> +    Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES,
> +                    TPL_NOTIFY,
> +                    Armada7k8kExitBootEvent,
> +                    NULL,
> +                    &mArmada7k8kExitBootServicesEvent);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Fail to register EBS event\n", __FUNCTION__));
> +    }
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> --
> 2.7.4
>

Actually, looking at what you are trying to do, can you use the
gEdkiiPlatformHasAcpiGuid protocol instead of looking at the variable
directly? That will be installed by the DT platform driver when
choosing ACPI mode.

It would need to be another driver, but it is more appropriate in any
case to wire this up in some platform DXE rather than the generic DTB
loader.