Re: [PATCH v4 01/15] PcAtChipsetPkg: Add MMIO Support to RTC driver

["Sami Mujawar" <sami.mujawar@arm.com>]

Hi Ard,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

-----Original Message-----
From: Ard Biesheuvel <ard.biesheuvel@arm.com> 
Sent: 07 July 2020 02:17 PM
To: Sami Mujawar <Sami.Mujawar@arm.com>; devel@edk2.groups.io
Cc: leif@nuviainc.com; ray.ni@intel.com; Alexandru Elisei <Alexandru.Elisei@arm.com>; Andre Przywara <Andre.Przywara@arm.com>; Matteo Carlini <Matteo.Carlini@arm.com>; Laura Moretta <Laura.Moretta@arm.com>; nd <nd@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.mujawar@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.biesheuvel@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
>