From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@semihalf-com.20150623.gappssmtp.com header.s=20150623 header.b=uq51QumU; spf=none, err=SPF record not found (domain: semihalf.com, ip: 209.85.160.194, mailfrom: mw@semihalf.com) Received: from mail-qt1-f194.google.com (mail-qt1-f194.google.com [209.85.160.194]) by groups.io with SMTP; Mon, 15 Apr 2019 22:49:37 -0700 Received: by mail-qt1-f194.google.com with SMTP id w5so21945710qtb.11 for ; Mon, 15 Apr 2019 22:49:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=semihalf-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=9UbsUkqwKFbp7P454i8vHC1A7L8MYJTKR87vkLcDPkA=; b=uq51QumUmZvC6dQ99NkgSEx3cRsU5C3nv5JiTHNvCeYA3lpxPWoBgYXcCnMzDRen86 StSDfSItITk82mzJva9mktHWNOxDbxMSXbHy91IYSFrM6joqZsYBSycIjGP5JMBrVXGK cwXzK53t+NNd864GkJ6UdU5siDl6KCKSavnBtF7zD+5NPQ6SgOzFktMEcsEoDUe/FrH/ /5bGUtjvlHo6D9h2RxCpEx2Rmz+lXlu5RtEPy1eiFtClbVdvFlWEvyJaklhI2Y8rTevV bIbhQG7RbXlTB0/dXLafQpMkP84LIe8V5hGAmGdp5Nu9Qghqw/nEcBAYebj0GKfV+5th UAlA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=9UbsUkqwKFbp7P454i8vHC1A7L8MYJTKR87vkLcDPkA=; b=HEfOid1payGnmp1FmO8bXug4giaVGbJ4ryvwsMsL2D8/YM9bH+wSaet817wuJI402b xR5VMmT8r33IKzLleaPG1l7hGgwODBErWQ5X2Zwhqfxtm4bBeO9EM54Qn1yZ31cWNCsJ GumTeI7IRwFZg+VrxGRojxcXsOEz/qehjq2qL62vjdofTukYzVyCGc4tOP8p7k7IF4+A uj0aobSGQcAb+jMKJ4NGcXc1CwbKqHOHlCuPDraRfuKAx1B9CZbPU1LZFJvzB0z1iKGw MNJslaUl9Js0d2dYA4gtnN6NI1VeoP5UGJV5l8ON8f5hQjKOG72SyFGCKfBVAtUsh29v Qs2g== X-Gm-Message-State: APjAAAUES4yo/YLNiVTfnGWTHY3bX2xk24UNW8p+fzdz2r+du3HejGP4 wpvFDfxD4f3IRHskqQYWEcrOOwQw22kAH4ux8vijew== X-Google-Smtp-Source: APXvYqwinATdXjse3/JbDp3M0uviulvDUSVST8pQ/0hO/SlJ1A+VKnrAgImKdAnzUtsS3agltgQZsdmadeYwDvhr2bo= X-Received: by 2002:ac8:21bc:: with SMTP id 57mr60608484qty.51.1555393775766; Mon, 15 Apr 2019 22:49:35 -0700 (PDT) MIME-Version: 1.0 References: <1555064376-22302-1-git-send-email-mw@semihalf.com> <1555064376-22302-4-git-send-email-mw@semihalf.com> In-Reply-To: From: "Marcin Wojtas" Date: Tue, 16 Apr 2019 07:49:24 +0200 Message-ID: Subject: Re: [edk2-platforms: PATCH 3/6] Marvell/Armada7k8k: Implement custom DtLoaderLib To: Ard Biesheuvel Cc: edk2-devel-groups-io , Leif Lindholm , Nadav Haklai , =?UTF-8?B?SmFuIETEhWJyb8Wb?= , Grzegorz Jaszczyk , Kostya Porotchkin , Jeremy Linton , Jici Gao Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable wt., 16 kwi 2019 o 07:01 Ard Biesheuvel napisa= =C5=82(a): > > On Mon, 15 Apr 2019 at 21:54, Marcin Wojtas wrote: > > > > Hi Ard, > > > > pon., 15 kwi 2019 o 21:01 Ard Biesheuvel na= pisa=C5=82(a): > > > > > > On Mon, 15 Apr 2019 at 02:52, Marcin Wojtas wrote: > > > > > > > > Hi Ard, > > > > > > > > > > > > pt., 12 kwi 2019 o 23:15 Ard Biesheuvel = napisa=C5=82(a): > > > > > > > > > > On Fri, 12 Apr 2019 at 03:20, Marcin Wojtas wro= te: > > > > > > > > > > > > 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 > > > > > > --- > > > > > > Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc = | 2 +- > > > > > > Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/D= xeDtPlatformDtbLoaderLib.inf | 43 +++++++ > > > > > > Silicon/Marvell/Include/IndustryStandard/MvSmc.h = | 2 + > > > > > > Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoaderLib/D= xeDtPlatformDtbLoaderLib.c | 130 ++++++++++++++++++++ > > > > > > 4 files changed, 176 insertions(+), 1 deletion(-) > > > > > > create mode 100644 Silicon/Marvell/Armada7k8k/Library/DxeDtPla= tformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.inf > > > > > > create mode 100644 Silicon/Marvell/Armada7k8k/Library/DxeDtPla= tformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.c > > > > > > > > > > > > diff --git a/Silicon/Marvell/Armada7k8k/Armada7k8k.dsc.inc b/Si= licon/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/DxePer= formanceLib.inf > > > > > > MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/U= efiMemoryAllocationLib.inf > > > > > > NonDiscoverableDeviceRegistrationLib|MdeModulePkg/Library/No= nDiscoverableDeviceRegistrationLib/NonDiscoverableDeviceRegistrationLib.inf > > > > > > - DtPlatformDtbLoaderLib|EmbeddedPkg/Library/DxeDtPlatformDtbL= oaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.inf > > > > > > + DtPlatformDtbLoaderLib|Silicon/Marvell/Armada7k8k/Library/Dx= eDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.inf > > > > > > > > > > > > [LibraryClasses.common.UEFI_APPLICATION] > > > > > > PerformanceLib|MdeModulePkg/Library/DxePerformanceLib/DxePer= formanceLib.inf > > > > > > diff --git a/Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDt= bLoaderLib/DxeDtPlatformDtbLoaderLib.inf b/Silicon/Marvell/Armada7k8k/Libra= ry/DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.inf > > > > > > new file mode 100644 > > > > > > index 0000000..ec3f3a5 > > > > > > --- /dev/null > > > > > > +++ b/Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoader= Lib/DxeDtPlatformDtbLoaderLib.inf > > > > > > @@ -0,0 +1,43 @@ > > > > > > +/** @file > > > > > > +* > > > > > > +* Copyright (c) 2017, Linaro, Ltd. All rights reserved. > > > > > > +* Copyright (c) 2018, Marvell International Ltd. and its affi= liates > > > > > > +* > > > > > > > > > > Please fix the date here as well as below > > > > > > > > > > > +* This program and the accompanying materials > > > > > > +* are licensed and made available under the terms and conditi= ons 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 E= XPRESS OR IMPLIED. > > > > > > +* > > > > > > +**/ > > > > > > + > > > > > > +[Defines] > > > > > > + INF_VERSION =3D 0x0001001A > > > > > > + BASE_NAME =3D DxeDtPlatformDtbLoaderLib= Default > > > > > > + FILE_GUID =3D dde55569-ad3f-421d-b94b-3= be66bb916ce > > > > > > + MODULE_TYPE =3D DXE_DRIVER > > > > > > + VERSION_STRING =3D 1.0 > > > > > > + LIBRARY_CLASS =3D DtPlatformDtbLoaderLib|DX= E_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/DxeDtPlatformDt= bLoaderLib/DxeDtPlatformDtbLoaderLib.c b/Silicon/Marvell/Armada7k8k/Library= /DxeDtPlatformDtbLoaderLib/DxeDtPlatformDtbLoaderLib.c > > > > > > new file mode 100644 > > > > > > index 0000000..b189f47 > > > > > > --- /dev/null > > > > > > +++ b/Silicon/Marvell/Armada7k8k/Library/DxeDtPlatformDtbLoader= Lib/DxeDtPlatformDtbLoaderLib.c > > > > > > @@ -0,0 +1,130 @@ > > > > > > +/** @file > > > > > > +* > > > > > > +* Copyright (c) 2017, Linaro, Ltd. All rights reserved. > > > > > > +* Copyright (c) 2018, Marvell International Ltd. and its affi= liates > > > > > > +* > > > > > > +* This program and the accompanying materials > > > > > > +* are licensed and made available under the terms and conditi= ons 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 E= XPRESS OR IMPLIED. > > > > > > +* > > > > > > +**/ > > > > > > + > > > > > > +#include > > > > > > + > > > > > > +#include > > > > > > + > > > > > > +#include > > > > > > +#include > > > > > > +#include > > > > > > +#include > > > > > > +#include > > > > > > +#include > > > > > > +#include > > > > > > + > > > > > > > > > > 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 worl= d. > > > > > > + > > > > > > + @param[in] Event Event structure > > > > > > + @param[in] Context Notification context > > > > > > + > > > > > > +**/ > > > > > > +STATIC > > > > > > +VOID > > > > > > +Armada7k8kExitBootEvent ( > > > > > > + IN EFI_EVENT Event, > > > > > > + IN VOID *Context > > > > > > + ) > > > > > > +{ > > > > > > + ARM_SMC_ARGS SmcRegs =3D {0}; > > > > > > + > > > > > > + SmcRegs.Arg0 =3D MV_SMC_ID_PMU_IRQ_DISABLE; > > > > > > + ArmCallSmc (&SmcRegs); > > > > > > + > > > > > > + return; > > > > > > +} > > > > > > + > > > > > > +/** > > > > > > + Return a pool allocated copy of the DTB image that is approp= riate 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 succ= essfully > > > > > > + @retval EFI_NOT_FOUND No suitable DTB image co= uld 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 =3D {0}; > > > > > > + EFI_STATUS Status; > > > > > > + VOID *OrigDtb; > > > > > > + VOID *CopyDtb; > > > > > > + UINTN OrigDtbSize; > > > > > > + UINTN BufferSize; > > > > > > + > > > > > > + Status =3D GetSectionFromAnyFv (&gDtPlatformDefaultDtbFileGu= id, > > > > > > + EFI_SECTION_RAW, > > > > > > + 0, > > > > > > + &OrigDtb, > > > > > > + &OrigDtbSize); > > > > > > + if (EFI_ERROR (Status)) { > > > > > > + return EFI_NOT_FOUND; > > > > > > + } > > > > > > + > > > > > > + CopyDtb =3D AllocateCopyPool (OrigDtbSize, OrigDtb); > > > > > > + if (CopyDtb =3D=3D NULL) { > > > > > > + return EFI_OUT_OF_RESOURCES; > > > > > > + } > > > > > > + > > > > > > + *Dtb =3D CopyDtb; > > > > > > + *DtbSize =3D OrigDtbSize; > > > > > > + > > > > > > + /* Enable EL3 handler for regardless HW description */ > > > > > > + SmcRegs.Arg0 =3D MV_SMC_ID_PMU_IRQ_ENABLE; > > > > > > + ArmCallSmc (&SmcRegs); > > > > > > + > > > > > > + /* > > > > > > + * Get the current DT/ACPI preference from the DtAcpiPref va= riable. > > > > > > + * Register ExitBootServices event only in case the DT will = be used. > > > > > > + */ > > > > > > + BufferSize =3D sizeof (DtAcpiPref); > > > > > > + Status =3D gRT->GetVariable (L"DtAcpiPref", > > > > > > + &gDtPlatformFormSetGuid, > > > > > > + NULL, > > > > > > + &BufferSize, > > > > > > + &DtAcpiPref); > > > > > > + if (EFI_ERROR (Status) || DtAcpiPref.Pref =3D=3D DT_ACPI_SEL= ECT_DT) { > > > > > > + Status =3D 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 vari= able > > > > > 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. A lot of combination just to avoid using custom DtLoaderLib (simple, only smc in EBS) or checking the status of gEdkiiPlatformHasAcpiGuid in the EBS :) I thought about forcing PlatInitDxe to load after DtPlatformDxe, but that ends with the chicken and egg problem (variables vs setting pin control). I could also add extra drive (PlatDeinitDxe) only for that purpose, but I do not like this idea either. So, I'll implement chained events. Thanks, Marcin