Hi Ard, On 10/05/20 12:58, Ard Biesheuvel wrote: > On 10/2/20 11:13 PM, Sami Mujawar wrote: >> Some virtual machine managers like Kvmtool emulate the MC146818 >> RTC controller in the MMIO space so that architectures that do >> not support I/O Mapped I/O can use the RTC. This patch adds MMIO >> support to the RTC controller driver. >> >> The PCD PcdRtcUseMmio has been added to select I/O or MMIO support. >> If PcdRtcUseMmio is: >> TRUE - Indicates the RTC port registers are in MMIO space. >> FALSE - Indicates the RTC port registers are in I/O space. >> Default is I/O space. >> >> Additionally two new PCDs PcdRtcIndexRegister64 and >> PcdRtcTargetRegister64 have been introduced to provide the base >> address for the RTC registers in the MMIO space. >> >> When MMIO support is selected (PcdRtcUseMmio == TRUE) the driver >> converts the pointers to the RTC MMIO registers so that the >> RTC registers are accessible post ExitBootServices. >> >> Signed-off-by: Sami Mujawar <sami.muja...@arm.com> >> Reviewed-by: Ard Biesheuvel <ard.biesheu...@arm.com> > > Thanks for the resend. > > Ray, do you have any concerns regarding this patch? The series looks > otherwise ok, and I would like to get this merged this week.
Thank you very much for handling this. I've skimmed the series quickly and it mostly adds new files. I think the risk of regressions is really low. Please feel free to merge this from my side. For wherever it applies in the series: Acked-by: Laszlo Ersek <ler...@redhat.com> Thanks Laszlo > > Guomin, you replied before that you were intending to review this patch > after August. Do you still intend to do so? > > If I don't hear from you by the end of this week, I am going to assume > there are no objections from your side. > > Thanks, > Ard. > > >> --- >> >> Notes: >> v5: >> - Updated based on review comments. >> [Sami] >> - Drop the unnecessary whitespace and header changes to >> [Ard] >> PcRtc.h, and the whitespace changes to PcRtcEntry.c >> Ref: https://edk2.groups.io/g/devel/topic/75354083 >> v4: >> - Updated based on review comments. >> [Sami] >> - Use static helper functions instead of function pointers. >> [Ard] >> Ref: https://edk2.groups.io/g/devel/topic/75081468 >> v3: >> - Make PcdRtcUseMmio a feature PCD. >> [Sami] >> - Read the RTC MMIO base address from the DT. >> [Andre] >> - Introduce PCDs for RTC Index and Target register base >> [Sami] >> address in the MMIO space. >> - Move RTC MMIO region mapping code to a separate platform >> [Sami] >> specific library. This library also reads the base addresses >> for the RTC from DT and configures the RTC Index and Target >> register PCDs. >> Ref: https://edk2.groups.io/g/devel/topic/74200905#60307 >> v2: >> - Code review comments incorporated. >> [Sami] >> v1: >> - Add support to read/write from RTC registers using >> [Sami] >> MMIO access >> - Use wrapper functions for RtcRead/Write accessors >> [Leif] >> Ref: https://edk2.groups.io/g/devel/topic/30915281#30695 >> >> >> PcAtChipsetPkg/PcAtChipsetPkg.dec >> | 16 +++ >> >> PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c >> | 120 ++++++++++++++++++-- >> >> PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> | 54 ++++++++- >> >> PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf >> | 8 ++ >> 4 files changed, 186 insertions(+), 12 deletions(-) >> >> diff --git a/PcAtChipsetPkg/PcAtChipsetPkg.dec >> b/PcAtChipsetPkg/PcAtChipsetPkg.dec >> index >> 88de5cceea593176c3a2425a5963b66b789f2b9e..ed2d95550b8d153995b30cdc290cf3bb905e211b >> 100644 >> --- a/PcAtChipsetPkg/PcAtChipsetPkg.dec >> +++ b/PcAtChipsetPkg/PcAtChipsetPkg.dec >> @@ -6,6 +6,7 @@ >> # >> # Copyright (c) 2009 - 2019, Intel Corporation. All rights >> reserved.<BR> >> # Copyright (c) 2017, AMD Inc. All rights reserved.<BR> >> +# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.<BR> >> # >> # SPDX-License-Identifier: BSD-2-Clause-Patent >> # >> @@ -41,6 +42,13 @@ [PcdsFeatureFlag] >> # @Prompt Configure HPET to use MSI. >> >> gPcAtChipsetPkgTokenSpaceGuid.PcdHpetMsiEnable|TRUE|BOOLEAN|0x00001000 >> + ## Indicates the RTC port registers are in MMIO space, or in I/O >> space. >> + # Default is I/O space.<BR><BR> >> + # TRUE - RTC port registers are in MMIO space.<BR> >> + # FALSE - RTC port registers are in I/O space.<BR> >> + # @Prompt RTC port registers use MMIO. >> + gPcAtChipsetPkgTokenSpaceGuid.PcdRtcUseMmio|FALSE|BOOLEAN|0x00000021 >> + >> [PcdsFixedAtBuild, PcdsDynamic, PcdsDynamicEx, PcdsPatchableInModule] >> ## This PCD specifies the base address of the HPET timer. >> # @Prompt HPET base address. >> @@ -68,6 +76,14 @@ [PcdsFixedAtBuild, PcdsDynamic, PcdsDynamicEx, >> PcdsPatchableInModule] >> # @Expression 0x80000001 | >> gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear < >> gPcAtChipsetPkgTokenSpaceGuid.PcdMinimalValidYear + 100 >> >> gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear|2097|UINT16|0x0000000E >> + ## Specifies RTC Index Register address in MMIO space. >> + # @Prompt RTC Index Register address >> + >> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64|0x0|UINT64|0x00000022 >> + >> + ## Specifies RTC Target Register address in MMIO space. >> + # @Prompt RTC Target Register address >> + >> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64|0x0|UINT64|0x00000023 >> >> + >> [PcdsFixedAtBuild, PcdsPatchableInModule] >> ## Defines the ACPI register set base address. >> # The invalid 0xFFFF is as its default value. It must be >> configured to the real value. >> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c >> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c >> index >> 52af17941786ef81c3911512ee64551724e67209..64f36f6fbbd1b03967bd1a1290d108d5b0f294fa >> 100644 >> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c >> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c >> @@ -3,6 +3,7 @@ >> Copyright (c) 2006 - 2018, Intel Corporation. All rights >> reserved.<BR> >> Copyright (c) 2017, AMD Inc. All rights reserved.<BR> >> +Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.<BR> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> @@ -10,6 +11,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent >> #include "PcRtc.h" >> +extern UINTN mRtcIndexRegister; >> +extern UINTN mRtcTargetRegister; >> + >> // >> // Days of month. >> // >> @@ -54,38 +58,132 @@ IsWithinOneDay ( >> ); >> /** >> + Read RTC content through its registers using IO access. >> + >> + @param Address Address offset of RTC. It is recommended to use >> + macros such as RTC_ADDRESS_SECONDS. >> + >> + @return The data of UINT8 type read from RTC. >> +**/ >> +STATIC >> +UINT8 >> +IoRtcRead ( >> + IN UINTN Address >> + ) >> +{ >> + IoWrite8 ( >> + PcdGet8 (PcdRtcIndexRegister), >> + (UINT8)(Address | (UINT8)(IoRead8 (PcdGet8 (PcdRtcIndexRegister)) >> & 0x80)) >> + ); >> + return IoRead8 (PcdGet8 (PcdRtcTargetRegister)); >> +} >> + >> +/** >> + Write RTC through its registers using IO access. >> + >> + @param Address Address offset of RTC. It is recommended to use >> + macros such as RTC_ADDRESS_SECONDS. >> + @param Data The content you want to write into RTC. >> + >> +**/ >> +STATIC >> +VOID >> +IoRtcWrite ( >> + IN UINTN Address, >> + IN UINT8 Data >> + ) >> +{ >> + IoWrite8 ( >> + PcdGet8 (PcdRtcIndexRegister), >> + (UINT8)(Address | (UINT8)(IoRead8 (PcdGet8 (PcdRtcIndexRegister)) >> & 0x80)) >> + ); >> + IoWrite8 (PcdGet8 (PcdRtcTargetRegister), Data); >> +} >> + >> +/** >> + Read RTC content through its registers using MMIO access. >> + >> + @param Address Address offset of RTC. It is recommended to use >> + macros such as RTC_ADDRESS_SECONDS. >> + >> + @return The data of UINT8 type read from RTC. >> +**/ >> +STATIC >> +UINT8 >> +MmioRtcRead ( >> + IN UINTN Address >> + ) >> +{ >> + MmioWrite8 ( >> + mRtcIndexRegister, >> + (UINT8)(Address | (UINT8)(MmioRead8 (mRtcIndexRegister) & 0x80)) >> + ); >> + return MmioRead8 (mRtcTargetRegister); >> +} >> + >> +/** >> + Write RTC through its registers using MMIO access. >> + >> + @param Address Address offset of RTC. It is recommended to use >> + macros such as RTC_ADDRESS_SECONDS. >> + @param Data The content you want to write into RTC. >> + >> +**/ >> +STATIC >> +VOID >> +MmioRtcWrite ( >> + IN UINTN Address, >> + IN UINT8 Data >> + ) >> +{ >> + MmioWrite8 ( >> + mRtcIndexRegister, >> + (UINT8)(Address | (UINT8)(MmioRead8 (mRtcIndexRegister) & 0x80)) >> + ); >> + MmioWrite8 (mRtcTargetRegister, Data); >> +} >> + >> +/** >> Read RTC content through its registers. >> - @param Address Address offset of RTC. It is recommended to use >> macros such as >> - RTC_ADDRESS_SECONDS. >> + @param Address Address offset of RTC. It is recommended to use >> + macros such as RTC_ADDRESS_SECONDS. >> @return The data of UINT8 type read from RTC. >> **/ >> +STATIC >> UINT8 >> RtcRead ( >> - IN UINT8 Address >> + IN UINTN Address >> ) >> { >> - IoWrite8 (PcdGet8 (PcdRtcIndexRegister), (UINT8) (Address | (UINT8) >> (IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80))); >> - return IoRead8 (PcdGet8 (PcdRtcTargetRegister)); >> + if (FeaturePcdGet (PcdRtcUseMmio)) { >> + return MmioRtcRead (Address); >> + } >> + >> + return IoRtcRead (Address); >> } >> /** >> Write RTC through its registers. >> - @param Address Address offset of RTC. It is recommended to use >> macros such as >> - RTC_ADDRESS_SECONDS. >> - @param Data The content you want to write into RTC. >> + @param Address Address offset of RTC. It is recommended to use >> + macros such as RTC_ADDRESS_SECONDS. >> + @param Data The content you want to write into RTC. >> **/ >> +STATIC >> VOID >> RtcWrite ( >> - IN UINT8 Address, >> + IN UINTN Address, >> IN UINT8 Data >> ) >> { >> - IoWrite8 (PcdGet8 (PcdRtcIndexRegister), (UINT8) (Address | (UINT8) >> (IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80))); >> - IoWrite8 (PcdGet8 (PcdRtcTargetRegister), Data); >> + if (FeaturePcdGet (PcdRtcUseMmio)) { >> + MmioRtcWrite (Address, Data); >> + } else { >> + IoRtcWrite (Address, Data); >> + } >> } >> /** >> diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> index >> ccda6331373bfe4069b0a59495b5e5cc731c8fc8..606b88adafb7ef5d81e32e212794a5ccae9d0443 >> 100644 >> --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c >> @@ -2,16 +2,23 @@ >> Provides Set/Get time operations. >> Copyright (c) 2006 - 2018, Intel Corporation. All rights >> reserved.<BR> >> +Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.<BR> >> SPDX-License-Identifier: BSD-2-Clause-Patent >> **/ >> +#include <Library/DxeServicesTableLib.h> >> #include "PcRtc.h" >> PC_RTC_MODULE_GLOBALS mModuleGlobal; >> EFI_HANDLE mHandle = NULL; >> +STATIC EFI_EVENT mVirtualAddrChangeEvent; >> + >> +UINTN mRtcIndexRegister; >> +UINTN mRtcTargetRegister; >> + >> /** >> Returns the current time and date information, and the >> time-keeping capabilities >> of the hardware platform. >> @@ -106,6 +113,30 @@ PcRtcEfiSetWakeupTime ( >> } >> /** >> + Fixup internal data so that EFI can be called in virtual mode. >> + Call the passed in Child Notify event and convert any pointers in >> + lib to virtual mode. >> + >> + @param[in] Event The Event that is being processed >> + @param[in] Context Event Context >> +**/ >> +VOID >> +EFIAPI >> +LibRtcVirtualNotifyEvent ( >> + IN EFI_EVENT Event, >> + IN VOID *Context >> + ) >> +{ >> + // Only needed if you are going to support the OS calling RTC >> functions in >> + // virtual mode. You will need to call EfiConvertPointer (). To >> convert any >> + // stored physical addresses to virtual address. After the OS >> transitions to >> + // calling in virtual mode, all future runtime calls will be made >> in virtual >> + // mode. >> + EfiConvertPointer (0x0, (VOID**)&mRtcIndexRegister); >> + EfiConvertPointer (0x0, (VOID**)&mRtcTargetRegister); >> +} >> + >> +/** >> The user Entry Point for PcRTC module. >> This is the entry point for PcRTC module. It installs the UEFI >> runtime service >> @@ -131,6 +162,11 @@ InitializePcRtc ( >> EfiInitializeLock (&mModuleGlobal.RtcLock, TPL_CALLBACK); >> mModuleGlobal.CenturyRtcAddress = GetCenturyRtcAddress (); >> + if (FeaturePcdGet (PcdRtcUseMmio)) { >> + mRtcIndexRegister = (UINTN)PcdGet64 (PcdRtcIndexRegister64); >> + mRtcTargetRegister = (UINTN)PcdGet64 (PcdRtcTargetRegister64); >> + } >> + >> Status = PcRtcInit (&mModuleGlobal); >> ASSERT_EFI_ERROR (Status); >> @@ -165,7 +201,23 @@ InitializePcRtc ( >> NULL, >> NULL >> ); >> - ASSERT_EFI_ERROR (Status); >> + if (EFI_ERROR (Status)) { >> + ASSERT_EFI_ERROR (Status); >> + return Status; >> + } >> + >> + if (FeaturePcdGet (PcdRtcUseMmio)) { >> + // Register for the virtual address change event >> + Status = gBS->CreateEventEx ( >> + EVT_NOTIFY_SIGNAL, >> + TPL_NOTIFY, >> + LibRtcVirtualNotifyEvent, >> + NULL, >> + &gEfiEventVirtualAddressChangeGuid, >> + &mVirtualAddrChangeEvent >> + ); >> + ASSERT_EFI_ERROR (Status); >> + } >> return Status; >> } >> diff --git >> a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf >> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf >> >> index >> c73ee98105e510f9e4e23c1a6c1e5c505325d2c9..0d8eca28b65954b073a72fc4fe5ad6247320e79d >> 100644 >> --- >> a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf >> >> +++ >> b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf >> >> @@ -6,6 +6,7 @@ >> # >> # Copyright (c) 2006 - 2019, Intel Corporation. All rights >> reserved.<BR> >> # Copyright (c) 2017, AMD Inc. All rights reserved.<BR> >> +# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.<BR> >> # >> # SPDX-License-Identifier: BSD-2-Clause-Patent >> # >> @@ -61,6 +62,11 @@ [Guids] >> ## SOMETIMES_CONSUMES ## SystemTable >> gEfiAcpiTableGuid >> + gEfiEventVirtualAddressChangeGuid >> + >> +[FeaturePcd] >> + gPcAtChipsetPkgTokenSpaceGuid.PcdRtcUseMmio ## >> CONSUMES >> + >> [FixedPcd] >> gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterA ## >> CONSUMES >> gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterB ## >> CONSUMES >> @@ -72,6 +78,8 @@ [Pcd] >> gPcAtChipsetPkgTokenSpaceGuid.PcdMaximalValidYear ## >> CONSUMES >> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister ## >> CONSUMES >> gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister ## >> CONSUMES >> + gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64 ## >> CONSUMES >> + gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64 ## >> CONSUMES >> [Depex] >> gEfiVariableArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#65917): https://edk2.groups.io/g/devel/message/65917 Mute This Topic: https://groups.io/mt/77270941/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
