From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.31994.1601895521288206427 for ; Mon, 05 Oct 2020 03:58:41 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: ard.biesheuvel@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 6A3A0106F; Mon, 5 Oct 2020 03:58:40 -0700 (PDT) Received: from [192.168.1.81] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id AF6363F66B; Mon, 5 Oct 2020 03:58:37 -0700 (PDT) Subject: Re: [PATCH v5 01/15] PcAtChipsetPkg: Add MMIO Support to RTC driver To: Sami Mujawar , devel@edk2.groups.io, ray.ni@intel.com, Guomin Jiang Cc: leif@nuviainc.com, Alexandru.Elisei@arm.com, Andre.Przywara@arm.com, Matteo.Carlini@arm.com, Ben.Adderson@arm.com, nd@arm.com, Laszlo Ersek , Andrew Fish , "Kinney, Michael D" References: <20201002211409.43888-1-sami.mujawar@arm.com> <20201002211409.43888-2-sami.mujawar@arm.com> From: "Ard Biesheuvel" Message-ID: <6186f413-8ec9-ded5-6290-82397e19904b@arm.com> Date: Mon, 5 Oct 2020 12:58:25 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <20201002211409.43888-2-sami.mujawar@arm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Reviewed-by: Ard Biesheuvel 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. 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.
> # Copyright (c) 2017, AMD Inc. All rights reserved.
> +# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
> # > # 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.

> + # TRUE - RTC port registers are in MMIO space.
> + # FALSE - RTC port registers are in I/O space.
> + # @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.
> Copyright (c) 2017, AMD Inc. All rights reserved.
> +Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
> > 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.
> +Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > +#include > #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.
> # Copyright (c) 2017, AMD Inc. All rights reserved.
> +# Copyright (c) 2018 - 2020, ARM Limited. All rights reserved.
> # > # 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 >