From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=BkF7PqQw; spf=pass (domain: linaro.org, ip: 209.85.166.196, mailfrom: ard.biesheuvel@linaro.org) Received: from mail-it1-f196.google.com (mail-it1-f196.google.com [209.85.166.196]) by groups.io with SMTP; Tue, 16 Apr 2019 11:51:11 -0700 Received: by mail-it1-f196.google.com with SMTP id a190so436039ite.4 for ; Tue, 16 Apr 2019 11:51:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=KWoUCAkglPP0wZxuKIX8ef/lanomEW4QvbLCfxkMXME=; b=BkF7PqQwk5bcAlRPYlRNZiFW0hKAkNzQA7x9Sb6ItqaaTX+3R0pXqPHPzbykgGB5f3 2VJweBcqRkjKvR8Ueuu6pCXTnPrsoI7sCFBBXpg0jlYVyY5kBlSlXr7A5g7z2EHoV1Pw 30UMn1bg0YCFQBj27juOAjEGlumjX6DZB1Ruel09XxCFQ3dB9k6w2rZmD7zw1FhOcddi KEF0kc1KHGkuT7gn1ae5IMjYlRhdk4x9NbKrzM1rPnfMSbG+YRwoG10VZsmxFvI4kYY6 uddJzk77Qqqhpe5qXVwMBXx9cKtpSd0x5uWNhBElBJM4JYyyvdP/UmyqjX3nNiSZgLDf dcDw== 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; bh=KWoUCAkglPP0wZxuKIX8ef/lanomEW4QvbLCfxkMXME=; b=YxI4I/2zYrWmIgFlAUrBMwQ3sZadEOpVVk5PcXNRMKAHlfV2CosEPKCWmkRFUQK7fE TzKbVcZgFzDj4yEn4Xgk8DkEtz75GDYkht2F6xDQIoFruIiZKD+7qGG6boPbf+x5v5xd 4KAJ1WGFBuux70imfUvrUt6njaN73U8Bf2frL71ow/lF5Wnw5zppNxqWeiBvHXJp7gJf MvK9UJvzlKMrpOdpmqYCdtWlJsDukKrcK4HA/IGrSHGZ5bvkSO8R7qZnHs/mIGxI1j1I jYhZF5M6W8Fe5nSoZj1aYmzAxjRnkloQUPw9WS02VBQu0HZRO5ZWrMmWXIOj6V4kbHD7 XtCA== X-Gm-Message-State: APjAAAVoTvfQ3lGm0vf+alBp6drW7Q2xO5M5ioUkU8MtJZ1CAZtoBCld htpk7R/6ph6WsEM25QJ4wazmYc1w/TWl+qYZE1ZcQw== X-Google-Smtp-Source: APXvYqz287+ajMKwFUdQjmrvOtzur5XXJ7hJAKLXDMVzf1WOY0uZTIRoYwGE+LGjMJo3HEIwTCNLbqanAolx4ilRMbM= X-Received: by 2002:a02:bb06:: with SMTP id y6mr57125636jan.98.1555440670653; Tue, 16 Apr 2019 11:51:10 -0700 (PDT) MIME-Version: 1.0 References: <1555406546-5261-1-git-send-email-mw@semihalf.com> <1555406546-5261-4-git-send-email-mw@semihalf.com> In-Reply-To: <1555406546-5261-4-git-send-email-mw@semihalf.com> From: "Ard Biesheuvel" Date: Tue, 16 Apr 2019 11:51:01 -0700 Message-ID: Subject: Re: [edk2-platforms: PATCH v2 3/6] Marvell/Armada7k8k: Implement PMU interrupt handling To: Marcin Wojtas 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" On Tue, 16 Apr 2019 at 02:23, Marcin Wojtas 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 > --- > 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 > + > +#include > + > #include > +#include > #include > #include > #include > @@ -21,6 +26,62 @@ > #include > #include > > +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.