From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web12.11083.1601979418407899020 for ; Tue, 06 Oct 2020 03:16:58 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=JgDBrMvS; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1601979417; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=E/WiV+dPJTViIMTzxcq4cqqm6dGOGuoKsZUb18ZSqZU=; b=JgDBrMvSbXLfgD3URDNlYVzrw/931gVQiXhlE6r9tVXlZuzgQZ3/LKwdqz0Bqk6CzH9b1c y1YlTx2fKtfbUGnlZauGPoQXmH5Jza8Aw1x6l2iekXTF7kNgO5jMP8SKyojgOdbBPfMC9m SHV6b70kcEyBjFDHtPmmhwal1ojlVb4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-487-DFaThZdAOFOsf22QGXfhNA-1; Tue, 06 Oct 2020 06:16:54 -0400 X-MC-Unique: DFaThZdAOFOsf22QGXfhNA-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id B718D2FD09; Tue, 6 Oct 2020 10:16:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-235.ams2.redhat.com [10.36.112.235]) by smtp.corp.redhat.com (Postfix) with ESMTP id 63CC81A800; Tue, 6 Oct 2020 10:16:49 +0000 (UTC) Subject: Re: [PATCH v5 01/15] PcAtChipsetPkg: Add MMIO Support to RTC driver To: Ard Biesheuvel , 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, Andrew Fish , "Kinney, Michael D" References: <20201002211409.43888-1-sami.mujawar@arm.com> <20201002211409.43888-2-sami.mujawar@arm.com> <6186f413-8ec9-ded5-6290-82397e19904b@arm.com> From: "Laszlo Ersek" Message-ID: <03c60daf-1a51-e66f-a7cf-6e8667cf4760@redhat.com> Date: Tue, 6 Oct 2020 12:16:48 +0200 MIME-Version: 1.0 In-Reply-To: <6186f413-8ec9-ded5-6290-82397e19904b@arm.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 >> 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. 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 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.
>>   # 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 >> >