Hi Ard, Please find my response inline marked [SAMI].
Regards, Sami Mujawar -----Original Message----- From: Ard Biesheuvel <ard.biesheu...@arm.com> Sent: 07 July 2020 02:17 PM To: Sami Mujawar <sami.muja...@arm.com>; devel@edk2.groups.io Cc: l...@nuviainc.com; ray...@intel.com; Alexandru Elisei <alexandru.eli...@arm.com>; Andre Przywara <andre.przyw...@arm.com>; Matteo Carlini <matteo.carl...@arm.com>; Laura Moretta <laura.more...@arm.com>; nd <n...@arm.com> Subject: Re: [PATCH v4 01/15] PcAtChipsetPkg: Add MMIO Support to RTC driver On 7/7/20 3:47 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> This code looks good to me now, but please drop the unnecessary whitespace and header changes to PcRtc.h, and the whitespace changes to PcRtcEntry.c [SAMI] I will fix this and repost the patch. With that, Reviewed-by: Ard Biesheuvel <ard.biesheu...@arm.com> > --- > > Notes: > 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/PcRtc.h > | 2 + > PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > | 58 +++++++++- > PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf > | 8 ++ > 5 files changed, 190 insertions(+), 14 deletions(-) > > diff --git a/PcAtChipsetPkg/PcAtChipsetPkg.dec > b/PcAtChipsetPkg/PcAtChipsetPkg.dec > index > 88de5cceea593176c3a2425a5963b66b789f2b9e..ed2d95550b8d153995b30cdc290c > f3bb905e211b 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|0x000000 > 0E > > + ## Specifies RTC Index Register address in MMIO space. > + # @Prompt RTC Index Register address > + > + gPcAtChipsetPkgTokenSpaceGuid.PcdRtcIndexRegister64|0x0|UINT64|0x000 > + 00022 > + > + ## Specifies RTC Target Register address in MMIO space. > + # @Prompt RTC Target Register address > + > + gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister64|0x0|UINT64|0x00 > + 000023 > + > [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..64f36f6fbbd1b03967bd1a1290d1 > 08d5b0f294fa 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/PcRtc.h > b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h > index > 47293ce44c5a1f4792892892f7da40d7f0a5a001..c1cb9466b74587233de4e78c0c0d > 537ca6b66cf3 100644 > --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h > +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.h > @@ -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) 2019 - 2020, ARM Limited. All rights reserved.<BR> > > SPDX-License-Identifier: BSD-2-Clause-Patent > > @@ -371,4 +372,5 @@ PcRtcAcpiTableChangeCallback ( > IN EFI_EVENT Event, > IN VOID *Context > ); > + > #endif > diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > index > ccda6331373bfe4069b0a59495b5e5cc731c8fc8..9c5f1ff169c80d1a9580d90336f1 > 62968b63bfa2 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 @@ -125,12 +156,17 @@ InitializePcRtc ( > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > - EFI_STATUS Status; > - EFI_EVENT Event; > + EFI_STATUS Status; > + EFI_EVENT Event; > > 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/PcatRealTimeClockRuntimeD > xe.inf > b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeD > xe.inf index > c73ee98105e510f9e4e23c1a6c1e5c505325d2c9..0d8eca28b65954b073a72fc4fe5a > d6247320e79d 100644 > --- > a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeD > xe.inf > +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRunt > +++ imeDxe.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 (#62302): https://edk2.groups.io/g/devel/message/62302 Mute This Topic: https://groups.io/mt/75354083/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
