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

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

On Mon, 15 Apr 2019 at 21:54, Marcin Wojtas <mw@semihalf.com> wrote:
>
> Hi Ard,
>
> pon., 15 kwi 2019 o 21:01 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
> >
> > On Mon, 15 Apr 2019 at 02:52, Marcin Wojtas <mw@semihalf.com> wrote:
> > >
> > > Hi Ard,
> > >
> > >
> > > pt., 12 kwi 2019 o 23:15 Ard Biesheuvel <ard.biesheuvel@linaro.org> napisał(a):
> > > >
> > > > 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.
> > >
> > > Ok, I'll move it to DXE. If possible, I'd prefer to use Armada's
> > > PlatformInitDxe - would it be accepatble for you to check
> > > gEdkiiPlatformHasAcpiGuid in ExitBootEvent?
> > >
> >
> > Yes, but using OnReadyToBoot is more idiomatic for things like this.
>
> I just checked it - unfortunately it is triggered to early. I need the
> PMU irq handling as long as I'm in UEFI (or boot with ACPI). With the
> gEfiEventReadyToBootGuid the event hits before I enter the shell, so
> I'm moving to DXE, but stick with ExitBoot event solution.
>

Perhaps you could check for the protocol at ready to boot, and only
install the exit boot handler when booting in ACPI mode?

It seems a bit convoluted perhaps, but it is better to do as little as
possible in exit boot services.