Re: [edk2-platforms: PATCH v2 3/6] Marvell/Armada7k8k: Implement PMU interrupt handling

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

On Tue, 16 Apr 2019 at 02:23, Marcin Wojtas <mw@semihalf.com> wrote:
>
> Generic handling of the PMU interrupts in UEFI and Linux with
> ACPI require enabling a dedicated handler in the EL3.
> Extend the PlatInitDxe with enabler code.
>
> Because for DT world the EL3 service must remain disabled,
> switch it off in the ExitBootServicesEvent. Its execution
> depends on the gEdkiiPlatformHasAcpiGuid status check in the new
> gEfiEventReadyToBootGuid routine.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf |  6 ++
>  Silicon/Marvell/Include/IndustryStandard/MvSmc.h               |  2 +
>  Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c   | 82 ++++++++++++++++++++
>  3 files changed, 90 insertions(+)
>
> diff --git a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
> index e707fe9..df10526 100644
> --- a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
> +++ b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.inf
> @@ -25,6 +25,7 @@
>    PlatInitDxe.c
>
>  [Packages]
> +  ArmPkg/ArmPkg.dec
>    EmbeddedPkg/EmbeddedPkg.dec
>    MdeModulePkg/MdeModulePkg.dec
>    MdePkg/MdePkg.dec
> @@ -32,6 +33,7 @@
>
>  [LibraryClasses]
>    ArmadaIcuLib
> +  ArmSmcLib
>    ComPhyLib
>    DebugLib
>    MppLib
> @@ -40,6 +42,10 @@
>    UefiDriverEntryPoint
>    UtmiPhyLib
>
> +[Guids]
> +  gEdkiiPlatformHasAcpiGuid
> +  gEfiEventReadyToBootGuid
> +
>  [Protocols]
>    gMarvellPlatformInitCompleteProtocolGuid    ## PRODUCES
>
> 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/Drivers/PlatInitDxe/PlatInitDxe.c b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c
> index 18b6783..4012fd7 100644
> --- a/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c
> +++ b/Silicon/Marvell/Armada7k8k/Drivers/PlatInitDxe/PlatInitDxe.c
> @@ -12,7 +12,12 @@
>
>  **/
>
> +#include <Guid/EventGroup.h>
> +
> +#include <IndustryStandard/MvSmc.h>
> +
>  #include <Library/ArmadaIcuLib.h>
> +#include <Library/ArmSmcLib.h>
>  #include <Library/DebugLib.h>
>  #include <Library/MppLib.h>
>  #include <Library/MvComPhyLib.h>
> @@ -21,6 +26,62 @@
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Library/UtmiPhyLib.h>
>
> +STATIC EFI_EVENT mArmada7k8kExitBootServicesEvent;
> +
> +/**
> +  Disable extra EL3 handling of the PMU interrupts for DT world.
> +
> +  @param[in]   Event                  Event structure
> +  @param[in]   Context                Notification context
> +
> +**/
> +STATIC
> +VOID

Please add EFIAPI here. I know it evaluates to nothing at the moment,
but since this is called via a function pointer by the DXE core, it
should have the correct prototype.

> +Armada7k8kExitBootServicesHandler (
> +  IN EFI_EVENT  Event,
> +  IN VOID      *Context
> +  )
> +{
> +  ARM_SMC_ARGS SmcRegs = {0};
> +
> +  SmcRegs.Arg0 = MV_SMC_ID_PMU_IRQ_DISABLE;
> +  ArmCallSmc (&SmcRegs);
> +
> +  return;
> +}
> +
> +/**
> +  Check if we boot with DT and trigger EBS event in such case.
> +
> +  @param[in]   Event                  Event structure
> +  @param[in]   Context                Notification context
> +
> +**/
> +STATIC
> +VOID

Same here

> +Armada7k8kOnReadyToBootHandler (
> +  IN EFI_EVENT  Event,
> +  IN VOID      *Context
> +  )
> +{
> +  EFI_STATUS    Status;
> +  VOID         *Interface;
> +
> +  Status = gBS->LocateProtocol (&gEdkiiPlatformHasAcpiGuid,
> +                  NULL,
> +                  (VOID **)&Interface);
> +  if (EFI_ERROR (Status)) {

Can we invert the logic (i.e. return early on success) with a comment
explaining why?

> +    Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES,
> +                    TPL_NOTIFY,
> +                    Armada7k8kExitBootServicesHandler,
> +                    NULL,
> +                    &mArmada7k8kExitBootServicesEvent);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: Fail to register EBS event\n", __FUNCTION__));
> +    }

Just use ASSERT_EFI_ERROR() here - this never fails in practice.

> +  }

Please close the event - ReadyToBoot could fire multiple times and you
only want the notification once.

> +}
> +
>  EFI_STATUS
>  EFIAPI
>  ArmadaPlatInitDxeEntryPoint (
> @@ -28,7 +89,9 @@ ArmadaPlatInitDxeEntryPoint (
>    IN EFI_SYSTEM_TABLE   *SystemTable
>    )
>  {
> +  ARM_SMC_ARGS  SmcRegs = {0};
>    EFI_STATUS    Status;
> +  EFI_EVENT     Event;
>
>    DEBUG ((DEBUG_ERROR, "\nArmada Platform Init\n\n"));
>
> @@ -43,5 +106,24 @@ ArmadaPlatInitDxeEntryPoint (
>    MppInitialize ();
>    ArmadaIcuInitialize ();
>
> +  /*
> +   * Enable EL3 PMU interrupt handler and
> +   * register the ReadyToBoot event.
> +   */
> +  SmcRegs.Arg0 = MV_SMC_ID_PMU_IRQ_ENABLE;
> +  ArmCallSmc (&SmcRegs);
> +
> +  Status = gBS->CreateEventEx (EVT_NOTIFY_SIGNAL,
> +                  TPL_CALLBACK,
> +                  Armada7k8kOnReadyToBootHandler,
> +                  NULL,
> +                  &gEfiEventReadyToBootGuid,
> +                  &Event);
> +  if (EFI_ERROR (Status)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Fail to register OnReadyToBoot event\n",
> +      __FUNCTION__));
> +  }
> +

Just use ASSERT_EFI_ERROR here (as above)

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

I'll pick up the other patches, so no need to resend the whole series.