From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::242; helo=mail-io0-x242.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x242.google.com (mail-io0-x242.google.com [IPv6:2607:f8b0:4001:c06::242]) (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 ABF21209884BC for ; Tue, 5 Jun 2018 05:25:42 -0700 (PDT) Received: by mail-io0-x242.google.com with SMTP id l19-v6so3091143ioj.5 for ; Tue, 05 Jun 2018 05:25:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=5LkWLdC4M7eVf1fmpql29espFOWdojuG7U6lYdiq6HA=; b=HrqoZH6tj48liIsFpubiD3Dlcpw1vWI0LV1gM1OLg2BRWBZzN+xKrYgTTI7SOhtdTF IoNGrDQe+LBajGkrL277G7g36m2ddAsSziMaWQWYU6trQCDxTFH3Z2JDRloIoJ2ULFaQ KaYBJZ3eirwOfnSqDjhMdJnUdoN13dMrIWAcQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=5LkWLdC4M7eVf1fmpql29espFOWdojuG7U6lYdiq6HA=; b=pNroEy5FfZxCiNSKMGBETzCxFykdADZq1Mp6WtsfYIwkqVsXMagqBRgMhduIZFYfTY 68CQAgQqXb4DIp9daCwleYmAIKH9zJHe7STT1/jUjGTYqm0Px9vovf71XPfcp98tPqRc z7C0idV67EVB0r/MMrssM8hQAU+euSLSRpkG23bavC7TatjEVNiipIUljWErEMol4uko XhLygOta1o1UjrsbPJY7ufVTVEbGnegj+ZcctO7RDLVgvql0c1Xipb/4eZji3jQxbUjI LTzWFtXcSDOkxiMcH+puhTmrdu9kZXSKo2xo9M4JXz4H1CoYBu0L1InTgZX69VXcxDjy ZI+A== X-Gm-Message-State: APt69E0yuJAIYZIrO05aDWeqjqhmT129rRwelVFEIt9aZH15BKWmBUzW Mu1l5rduiaa2kGxcAuxksiCt0RPUuEwcktkP9wGzIWC2VGc= X-Google-Smtp-Source: ADUXVKLlP15q507VHgoPWmFRWIUQvGJHvvCPKdQ431Dp0ZSNXwqAjBQU82FyGp6SD+uC19iUYmPEYZJXVY4KXz0SBMk= X-Received: by 2002:a6b:6709:: with SMTP id b9-v6mr24602180ioc.170.1528201542039; Tue, 05 Jun 2018 05:25:42 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bb86:0:0:0:0:0 with HTTP; Tue, 5 Jun 2018 05:25:41 -0700 (PDT) In-Reply-To: References: <1528155339-5050-1-git-send-email-udit.kumar@nxp.com> From: Ard Biesheuvel Date: Tue, 5 Jun 2018 14:25:41 +0200 Message-ID: To: Udit Kumar Cc: "edk2-devel@lists.01.org" , Leif Lindholm Subject: Re: [PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq Support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Jun 2018 12:25:42 -0000 Content-Type: text/plain; charset="UTF-8" On 5 June 2018 at 14:23, Ard Biesheuvel wrote: > On 5 June 2018 at 01:35, Udit Kumar wrote: >> Some platform support dynamic clocking, Which is controlled >> by some jumper setting or hardware registers. >> Result of that PCD PL011UartClkInHz needs to be updated for >> frequency change. >> This patch implements support for dynamic frequency for >> PL011 uart. >> This patch implement NULL lib for such platform where >> Pcd clock frequency to PL011 can change >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Udit Kumar > > I'm not crazy about this. How exactly are you reading the frequency in > your case? > >> --- >> .../Include/Library/ArmPlatformClockLib.h | 32 ++++++++++++++++++++ >> .../ArmPlatformClockLib.inf | 33 ++++++++++++++++++++ >> .../ArmPlatformClockLibNull.c | 35 ++++++++++++++++++++++ >> 3 files changed, 100 insertions(+) >> create mode 100644 ArmPlatformPkg/Include/Library/ArmPlatformClockLib.h >> create mode 100644 ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLib.inf >> create mode 100644 ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLibNull.c >> > > ArmPlatformClockLib doesn't make sense, since this is specific to PL011, no? > > >> diff --git a/ArmPlatformPkg/Include/Library/ArmPlatformClockLib.h b/ArmPlatformPkg/Include/Library/ArmPlatformClockLib.h >> new file mode 100644 >> index 0000000..f9d1425 >> --- /dev/null >> +++ b/ArmPlatformPkg/Include/Library/ArmPlatformClockLib.h > > Please add new library classes to the package .dec file > >> @@ -0,0 +1,32 @@ >> +/** @file >> +* >> +* Copyright 2018 NXP >> +* >> +* 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 >> +* http://opensource.org/licenses/bsd-license.php >> +* >> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> +* >> +**/ >> + >> +#ifndef _ARMPLATFORMCLOCKLIB_H_ >> +#define _ARMPLATFORMCLOCKLIB_H_ >> + >> + >> +/** >> + Return frequency of PL011. >> + >> + If this function return 0 then fixed value in Pcd will be used >> + >> + @return Return frequency of PL011 >> + >> +**/ >> +UINT32 >> +ArmPlatformGetPL011ClockFreq ( >> + VOID >> + ); >> + >> +#endif >> diff --git a/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLib.inf b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLib.inf >> new file mode 100644 >> index 0000000..b708ad3 >> --- /dev/null >> +++ b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLib.inf >> @@ -0,0 +1,33 @@ >> +#/* @file >> +# Copyright 2018 NXP >> +# >> +# 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 >> +# http://opensource.org/licenses/bsd-license.php >> +# >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> +# >> +#*/ >> + >> +[Defines] >> + INF_VERSION = 0x0001000A >> + BASE_NAME = ArmPlatformClockLibNull I forgot: the Null suffix is inappropriate here. Please invent a name that reflects the fixed, constant nature of the clock frequency. >> + FILE_GUID = af8fef24-afbb-472a-b8b7-13101a79703c >> + MODULE_TYPE = BASE >> + VERSION_STRING = 1.0 >> + LIBRARY_CLASS = ArmPlatformClockLib >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + MdeModulePkg/MdeModulePkg.dec >> + ArmPkg/ArmPkg.dec >> + ArmPlatformPkg/ArmPlatformPkg.dec >> + >> +[LibraryClasses] >> + ArmLib >> + DebugLib >> + >> +[Sources.common] >> + ArmPlatformClockLibNull.c >> diff --git a/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLibNull.c b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLibNull.c >> new file mode 100644 >> index 0000000..28eaa63 >> --- /dev/null >> +++ b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLibNull.c >> @@ -0,0 +1,35 @@ >> +/** @file >> +* >> +* Copyright 2018 NXP >> +* >> +* 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 >> +* http://opensource.org/licenses/bsd-license.php >> +* >> +* THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +* WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. >> +* >> +**/ >> + >> +#include >> + >> + >> + >> +/** >> + Return clock in for PL011 Uart IP. >> +**/ >> +UINT32 >> +ArmPlatformGetPL011ClockFreq ( >> + VOID >> + ) >> +{ >> + // Implement platform specific code >> + // and return PL011 freq >> + >> + // Some platform supports dynamic clocking, >> + // This function needs to be implemented on platforms which supports >> + // dynamic clocking to avoid re-building of UEFI firmware for PL011 >> + // clock change >> + return 0; > > Why not return the PCD value here? That way, you can also get rid of > the horrid conditional expression in the next patch. > >> +} >> -- >> 1.9.1 >>