From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::442; helo=mail-wr1-x442.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x442.google.com (mail-wr1-x442.google.com [IPv6:2a00:1450:4864:20::442]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1701521959CB2 for ; Sat, 13 Oct 2018 03:51:23 -0700 (PDT) Received: by mail-wr1-x442.google.com with SMTP id e4-v6so16005944wrs.0 for ; Sat, 13 Oct 2018 03:51:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=pW9sin0F8DE5JXUepbGY/PUH6/M4VW4CMFY10AJIfu8=; b=OE4RPDJXKocC2gxUZZAG9UWum748BRmFNeMaTWmOmfHxXZ/dKHDUoryT5QjGKJDNMw +VgOLwsrmtsgiPewJB4HZ0al2Uq0EVw7xko3YUtBLzubw+i3F35bJVr/iXpLvba8NvlG KqVoQMhyXy08zHJkj+n7IdDYz2UGiDl7n4jd4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=pW9sin0F8DE5JXUepbGY/PUH6/M4VW4CMFY10AJIfu8=; b=ud5HvXvI7SvcioZ9nY0jZQoL44LUD6Z5HF0bLvAE2AAHvsMiS3M0XGyZopOnYViNgG qE71Zx7MiGXU/Tu2sNGeODjWi0+12nHpxMCZkKDzvYk3D1/udq2siICBAeZMubm2sa9x 8v0T3SypOd0JunMDN+EddoGUWKlyVhRSNhfrSM6ME2pe933S/8GpGfC83PgX+4ukD7OU 6/ZZyw70I8F+znSZpCjm8o7ARb4byIVqrI+6yF2okvbqdBImuUqnvuE/v9E8SLvQkdlx r69yz07L4c4k3FszQKmQ/AeC4yHL6Q4dzGbBIduzpPXGUWd0bVqW7pucqocYmH+HZrso GbNw== X-Gm-Message-State: ABuFfojOiHqBBYmVvKbR5vW5W2KG2WZRLMkc20FhOa+nNjBPpoAi38sB tQewqcpG8DYnkiwrZpQ1QNR+7w== X-Google-Smtp-Source: ACcGV63nt2qmETcWEThsh27C2E/zIpX93Nz6KBDaoojoyrllsLDNDmgQVN+FeS76SYALBGTC7IUi0w== X-Received: by 2002:adf:e18e:: with SMTP id k14-v6mr8530881wri.36.1539427881517; Sat, 13 Oct 2018 03:51:21 -0700 (PDT) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id x66-v6sm3722591wme.35.2018.10.13.03.51.19 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 13 Oct 2018 03:51:20 -0700 (PDT) Date: Sat, 13 Oct 2018 11:51:18 +0100 From: Leif Lindholm To: Sami Mujawar Cc: edk2-devel@lists.01.org, ard.biesheuvel@linaro.org, ruiyu.ni@intel.com, evan.lloyd@arm.com, Matteo.Carlini@arm.com, Stephanie.Hughes-Fitt@arm.com, nd@arm.com Message-ID: <20181013105118.tiszwakyihfssim3@bivouac.eciton.net> References: <20181012144009.48732-1-sami.mujawar@arm.com> <20181012144009.48732-3-sami.mujawar@arm.com> MIME-Version: 1.0 In-Reply-To: <20181012144009.48732-3-sami.mujawar@arm.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH v1 2/6] PcAtChipsetPkg: Add MMIO Support to RTC driver X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 13 Oct 2018 10:51:24 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Fri, Oct 12, 2018 at 03:40:05PM +0100, 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. > > When MMIO support is selected (PcdRtcUseMmio == TRUE) the driver > maps the MMIO region used by the RTC as runtime memory so that the > RTC registers are accessible post ExitBootServices. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Sami Mujawar > --- > The changes can be seen at https://github.com/samimujawar/edk2/commit/d9fa7df2204c12883a795c7df429578c310725bf > > Notes: > v1: > - Add support to read/write from RTC registers using MMIO access [SAMI] > > PcAtChipsetPkg/PcAtChipsetPkg.dec | 8 ++ > PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c | 38 ++++++- > PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c | 112 +++++++++++++++++++- > PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf | 8 ++ > 4 files changed, 162 insertions(+), 4 deletions(-) > > diff --git a/PcAtChipsetPkg/PcAtChipsetPkg.dec b/PcAtChipsetPkg/PcAtChipsetPkg.dec > index 0e66ff0ba3770cf765cfe36d592151d72eaa238a..5aaf5e73811c4dba3b83768f1535e028938ead1f 100644 > --- a/PcAtChipsetPkg/PcAtChipsetPkg.dec > +++ b/PcAtChipsetPkg/PcAtChipsetPkg.dec > @@ -6,6 +6,7 @@ > # > # Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
> # Copyright (c) 2017, AMD Inc. All rights reserved.
> +# Copyright (c) 2018, ARM Limited. All rights reserved.
> # > # This program and the accompanying materials > # are licensed and made available under the terms and conditions of the BSD License > @@ -202,5 +203,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule] > # @Prompt RTC Target Register address > gPcAtChipsetPkgTokenSpaceGuid.PcdRtcTargetRegister|0x71|UINT8|0x0000001F > > + ## 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|0x00000020 > + > [UserExtensions.TianoCore."ExtraFiles"] > PcAtChipsetPkgExtra.uni > diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtc.c > index 7965eb8aa55b92caef7a63834695506123935e2d..ee746add57b9778bf09a4d9eb6881d06caa14aba 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, ARM Limited. All rights reserved.
> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > @@ -16,6 +17,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > #include "PcRtc.h" > > +extern EFI_PHYSICAL_ADDRESS mRtcRegisterBase; > + > // > // Days of month. > // > @@ -72,7 +75,21 @@ RtcRead ( > IN UINT8 Address > ) > { > - IoWrite8 (PcdGet8 (PcdRtcIndexRegister), (UINT8) (Address | (UINT8) (IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80))); > + if (FixedPcdGetBool (PcdRtcUseMmio)) { > + MmioWrite8 ( > + mRtcRegisterBase, > + (UINT8)(Address | (UINT8)(MmioRead8 (mRtcRegisterBase) & 0x80)) > + ); > + return MmioRead8 ( > + mRtcRegisterBase + (PcdGet8 (PcdRtcTargetRegister) - > + PcdGet8 (PcdRtcIndexRegister)) > + ); > + } > + > + IoWrite8 ( > + PcdGet8 (PcdRtcIndexRegister), > + (UINT8)(Address | (UINT8)(IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80)) > + ); > return IoRead8 (PcdGet8 (PcdRtcTargetRegister)); So, this isn't my package, but I would much rather see this solved with local (STATIC) wrapper functions (i.e. RtcRead*/RtcWrite*) for the accessors instead of conditionalising which one to use at every call site. / Leif > } > > @@ -90,8 +107,23 @@ RtcWrite ( > IN UINT8 Data > ) > { > - IoWrite8 (PcdGet8 (PcdRtcIndexRegister), (UINT8) (Address | (UINT8) (IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80))); > - IoWrite8 (PcdGet8 (PcdRtcTargetRegister), Data); > + if (FixedPcdGetBool (PcdRtcUseMmio)) { > + MmioWrite8 ( > + mRtcRegisterBase, > + (UINT8)(Address | (UINT8)(MmioRead8 (mRtcRegisterBase) & 0x80)) > + ); > + MmioWrite8 ( > + mRtcRegisterBase + (PcdGet8 (PcdRtcTargetRegister) - > + PcdGet8 (PcdRtcIndexRegister)), > + Data > + ); > + } else { > + IoWrite8 ( > + PcdGet8 (PcdRtcIndexRegister), > + (UINT8)(Address | (UINT8)(IoRead8 (PcdGet8 (PcdRtcIndexRegister)) & 0x80)) > + ); > + IoWrite8 (PcdGet8 (PcdRtcTargetRegister), Data); > + } > } > > /** > diff --git a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > index 56ddc3ed5b1accc2b1a99ab4515f851f89fb6e43..d6d7251309001a203282eb7022cc71ac4f0ef62e 100644 > --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcRtcEntry.c > @@ -2,6 +2,7 @@ > Provides Set/Get time operations. > > Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> +Copyright (c) 2018, ARM Limited. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD License > which accompanies this distribution. The full text of the license may be found at > @@ -12,12 +13,17 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > > **/ > > +#include > #include "PcRtc.h" > > PC_RTC_MODULE_GLOBALS mModuleGlobal; > > EFI_HANDLE mHandle = NULL; > > +STATIC EFI_EVENT mVirtualAddrChangeEvent; > + > +EFI_PHYSICAL_ADDRESS mRtcRegisterBase; > + > /** > Returns the current time and date information, and the time-keeping capabilities > of the hardware platform. > @@ -112,6 +118,29 @@ 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**)&mRtcRegisterBase); > +} > + > +/** > The user Entry Point for PcRTC module. > > This is the entrhy point for PcRTC module. It installs the UEFI runtime service > @@ -133,10 +162,75 @@ InitializePcRtc ( > { > EFI_STATUS Status; > EFI_EVENT Event; > + EFI_PHYSICAL_ADDRESS RtcPageBase; > > EfiInitializeLock (&mModuleGlobal.RtcLock, TPL_CALLBACK); > mModuleGlobal.CenturyRtcAddress = GetCenturyRtcAddress (); > > + if (FixedPcdGetBool (PcdRtcUseMmio)) { > + mRtcRegisterBase = PcdGet8 (PcdRtcIndexRegister); > + RtcPageBase = mRtcRegisterBase & ~(EFI_PAGE_SIZE - 1); > + > + // Declare the controller as EFI_MEMORY_RUNTIME > + Status = gDS->AddMemorySpace ( > + EfiGcdMemoryTypeMemoryMappedIo, > + RtcPageBase, > + EFI_PAGE_SIZE, > + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, "Failed to add memory space. Status = %r\n", > + Status > + )); > + return Status; > + } > + > + Status = gDS->AllocateMemorySpace ( > + EfiGcdAllocateAddress, > + EfiGcdMemoryTypeMemoryMappedIo, > + 0, > + EFI_PAGE_SIZE, > + &RtcPageBase, > + ImageHandle, > + NULL > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "Failed to allocate memory space. Status = %r\n", > + Status > + )); > + gDS->RemoveMemorySpace ( > + RtcPageBase, > + EFI_PAGE_SIZE > + ); > + return Status; > + } > + > + Status = gDS->SetMemorySpaceAttributes ( > + RtcPageBase, > + EFI_PAGE_SIZE, > + EFI_MEMORY_UC | EFI_MEMORY_RUNTIME > + ); > + if (EFI_ERROR (Status)) { > + DEBUG (( > + DEBUG_ERROR, > + "Failed to set memory attributes. Status = %r\n", > + Status > + )); > + gDS->FreeMemorySpace ( > + RtcPageBase, > + EFI_PAGE_SIZE > + ); > + gDS->RemoveMemorySpace ( > + RtcPageBase, > + EFI_PAGE_SIZE > + ); > + return Status; > + } > + } > + > Status = PcRtcInit (&mModuleGlobal); > ASSERT_EFI_ERROR (Status); > > @@ -171,7 +265,23 @@ InitializePcRtc ( > NULL, > NULL > ); > - ASSERT_EFI_ERROR (Status); > + if (EFI_ERROR (Status)) { > + ASSERT_EFI_ERROR (Status); > + return Status; > + } > + > + if (FixedPcdGetBool (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 876298e1e0f3745cd3213509ffed4e091fbedef8..2dee5a77cfc23878a64a809bcb64e3745230ea5a 100644 > --- a/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf > +++ b/PcAtChipsetPkg/PcatRealTimeClockRuntimeDxe/PcatRealTimeClockRuntimeDxe.inf > @@ -6,6 +6,7 @@ > # > # Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.
> # Copyright (c) 2017, AMD Inc. All rights reserved.
> +# Copyright (c) 2018, ARM Limited. All rights reserved.
> # > # This program and the accompanying materials > # are licensed and made available under the terms and conditions of the BSD License > @@ -55,6 +56,7 @@ [LibraryClasses] > BaseLib > PcdLib > ReportStatusCodeLib > + DxeServicesTableLib > > [Protocols] > gEfiRealTimeClockArchProtocolGuid ## PRODUCES > @@ -68,10 +70,13 @@ [Guids] > ## SOMETIMES_CONSUMES ## SystemTable > gEfiAcpiTableGuid > > + gEfiEventVirtualAddressChangeGuid > + > [FixedPcd] > gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterA ## CONSUMES > gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterB ## CONSUMES > gPcAtChipsetPkgTokenSpaceGuid.PcdInitialValueRtcRegisterD ## CONSUMES > + gPcAtChipsetPkgTokenSpaceGuid.PcdRtcUseMmio ## CONSUMES > > [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdRealTimeClockUpdateTimeout ## CONSUMES > @@ -83,5 +88,8 @@ [Pcd] > [Depex] > gEfiVariableArchProtocolGuid AND gEfiVariableWriteArchProtocolGuid > > +[Depex.common.DXE_RUNTIME_DRIVER] > + gEfiCpuArchProtocolGuid > + > [UserExtensions.TianoCore."ExtraFiles"] > PcRtcExtra.uni > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > >