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 6D3EA2119A472 for ; Mon, 11 Jun 2018 03:05:16 -0700 (PDT) Received: by mail-io0-x242.google.com with SMTP id g7-v6so23177327ioh.11 for ; Mon, 11 Jun 2018 03:05:16 -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=cMzTfFHcP2yTj01bBAy1nFqjjPfb9GXFFPggjnPCGrE=; b=C3vXhSbWO0OLS3mxXF/DCUS/LXOpy/n7LGyvrQoLAJNLNUg68MQieap6Ylj+97BxpV RKQDCKXFpVuOyuOrdXn/SXVM5ovG9SiJ1RH150EhKBtrsapdBr14mnW4uOT6anZfOzva BZpfLdhc1+2dyWW90Tq/VOItyqqYyASIHJYhU= 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=cMzTfFHcP2yTj01bBAy1nFqjjPfb9GXFFPggjnPCGrE=; b=kikHejE1p6qZOvTXgPmx4DdbeMGAUY0hrjO8qeUr43R6YvDPhadJqVubhaGHCbKdoa ihVL99IE5AIQBhyPiszh0xtKXjQYPI7bpWV2nNwOXm268bS3SVMOX8IxYAaB4lH0NjeA FcGUjZnuyKULLvuYqSKIIdK+cmUxx7RamxV8pflYNmX7mVN8zQWPt/oPY6vWZKUem+q7 aUVZQ6zHKWX5+cXQLINqXzmvFBEdsr9olmRx8kJ6v5BBq6hLl1MyDVSvyXMhslQd3Vuk Pn0foH1IiNUsC91oq2ujiU+93D5fMY047WX3zRKZ2QkWGqvrD0OagvS9H44JadV5THfX Jh3w== X-Gm-Message-State: APt69E3QSoY1NDYTt7IvuDtjgeT8CjQ8PzuzZ7QzKubRIoeCq0XVIkpf PtWRTDzXbUiXjRbMpPXoj2dNeKInKSz+rVscZ46qaA== X-Google-Smtp-Source: ADUXVKLLi1XkDp9qyxBa+2HPhQSWr3qkIGjN4hfup91ygY88+t+gvCob/9OwCIG+SmlJOE8ZNVoTy6kkwnapEs1+h84= X-Received: by 2002:a6b:dd0b:: with SMTP id f11-v6mr13369424ioc.173.1528711516238; Mon, 11 Jun 2018 03:05:16 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Mon, 11 Jun 2018 03:05:15 -0700 (PDT) In-Reply-To: <1528221595-22145-2-git-send-email-udit.kumar@nxp.com> References: <1528221595-22145-1-git-send-email-udit.kumar@nxp.com> <1528221595-22145-2-git-send-email-udit.kumar@nxp.com> From: Ard Biesheuvel Date: Mon, 11 Jun 2018 12:05:15 +0200 Message-ID: To: Udit Kumar Cc: Leif Lindholm , "edk2-devel@lists.01.org" 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: Mon, 11 Jun 2018 10:05:17 -0000 Content-Type: text/plain; charset="UTF-8" Hello Udit, Apologies for not bringing this up the first time, but I have some additional comments. The first time around, I only had a cursory look because at that point I was still skeptical whether we needed this library in the first place. On 5 June 2018 at 19:59, 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 implements default lib, which is using Pcd. > Platform which needs dynamic clocking needs implement > PL011UartClockLib > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Udit Kumar > --- > ArmPlatformPkg/ArmPlatformPkg.dec | 1 + > ArmPlatformPkg/Include/Library/PL011UartClockLib.h | 32 +++++++++++++++++++ > .../Library/PL011UartClockLib/PL011UartClockLib.c | 29 +++++++++++++++++ > .../PL011UartClockLib/PL011UartClockLib.inf | 37 ++++++++++++++++++++++ Please add a reference to the new library in the [Components] section of ArmPlatformPkg.dsc as well, so we can build it standalone. > 4 files changed, 99 insertions(+) > create mode 100644 ArmPlatformPkg/Include/Library/PL011UartClockLib.h > create mode 100644 ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.c > create mode 100644 ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf > > diff --git a/ArmPlatformPkg/ArmPlatformPkg.dec b/ArmPlatformPkg/ArmPlatformPkg.dec > index dff4598..5f67e74 100644 > --- a/ArmPlatformPkg/ArmPlatformPkg.dec > +++ b/ArmPlatformPkg/ArmPlatformPkg.dec > @@ -36,6 +36,7 @@ > LcdHwLib|Include/Library/LcdHwLib.h > LcdPlatformLib|Include/Library/LcdPlatformLib.h > NorFlashPlatformLib|Include/Library/NorFlashPlatformLib.h > + PL011UartClockLib|Include/Library/PL011UartClockLib.h > PL011UartLib|Include/Library/PL011UartLib.h > > [Guids.common] > diff --git a/ArmPlatformPkg/Include/Library/PL011UartClockLib.h b/ArmPlatformPkg/Include/Library/PL011UartClockLib.h > new file mode 100644 > index 0000000..93813a0 > --- /dev/null > +++ b/ArmPlatformPkg/Include/Library/PL011UartClockLib.h > @@ -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 __PL011CLOCKLIB_H__ > +#define __PL011CLOCKLIB_H__ Nit: use __PL011UARTCLOCKLIB_H__ to match the filename. > + > + > +/** > + Return frequency of PL011. > + Mention the baud clock? > + By default this function returns FixedPcdGet32 (PL011UartClkInHz) > + Drop this line please, it is not part of the prototype > + @return Return frequency of PL011 > + > +**/ > +UINT32 > +ArmPlatformGetPL011ClockFreq ( The ArmPlatform prefix is unnecessary here: please use PL011UartClockGetFreq() instead. > + VOID > + ); > + > +#endif > diff --git a/ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.c b/ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.c > new file mode 100644 > index 0000000..b56af14 > --- /dev/null > +++ b/ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.c > @@ -0,0 +1,29 @@ > +/** @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 Please add EFIAPI even if it is defined to an empty string when using GCC/ARM. > +ArmPlatformGetPL011ClockFreq ( > + VOID > + ) > +{ > + // This function needs to be implemented on platforms which supports > + // dynamic clocking to avoid re-building of UEFI firmware for PL011 > + // clock change Please drop this comment, it does not belong here. You can add something along these lines in the declaration of the library class if you want, but you can drop it altogether IMO. > + return FixedPcdGet32 (PL011UartClkInHz); > +} > diff --git a/ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf b/ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf > new file mode 100644 > index 0000000..5f6f699 > --- /dev/null > +++ b/ArmPlatformPkg/Library/PL011UartClockLib/PL011UartClockLib.inf > @@ -0,0 +1,37 @@ > +#/* @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 Please use 0x0001001A > + BASE_NAME = PL011UartClockLib EDK2 usually has a BaseXxxLib implementation and does not reuse the library class name for the base implementation, so I am fine with this. Leif? > + FILE_GUID = af8fef24-afbb-472a-b8b7-13101a79703c > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = PL011UartClockLib > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec Is this line needed? > + ArmPkg/ArmPkg.dec > + ArmPlatformPkg/ArmPlatformPkg.dec > + Sort alphabetically please. > +[LibraryClasses] > + ArmLib > + DebugLib > + > +[Sources.common] > + PL011UartClockLib.c > + > +[FixedPcd] > + gArmPlatformTokenSpaceGuid.PL011UartClkInHz > + > -- > 1.9.1 >