Thanks for review Ard. > -----Original Message----- > From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] > Sent: Monday, June 11, 2018 3:35 PM > To: Udit Kumar <udit.ku...@nxp.com> > Cc: Leif Lindholm <leif.lindh...@linaro.org>; edk2-devel@lists.01.org > Subject: Re: [PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq > Support > > 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.
I hope, now you agree to have this lib 😊 > On 5 June 2018 at 19:59, Udit Kumar <udit.ku...@nxp.com> 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 <udit.ku...@nxp.com> > > --- > > 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. Ok > > 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 > > +* > > > +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fo > pe > > +nsource.org%2Flicenses%2Fbsd- > license.php&data=02%7C01%7Cudit.kumar%40 > > > +nxp.com%7C2ac27eb60055478ac37d08d5cf82d550%7C686ea1d3bc2b4c > 6fa92cd99c > > > +5c301635%7C0%7C0%7C636643083195138179&sdata=C%2F2MhK2ZA6 > XPmC5c8byWlK3 > > +xAC6rFmLYofPKlj6M7%2FI%3D&reserved=0 > > +* > > +* 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. My miss, thanks will do in v3 > > + > > + > > +/** > > + Return frequency of PL011. > > + > > Mention the baud clock? Sure > > + By default this function returns FixedPcdGet32 (PL011UartClkInHz) > > + > > Drop this line please, it is not part of the prototype Ok > > + @return Return frequency of PL011 > > + > > +**/ > > +UINT32 > > +ArmPlatformGetPL011ClockFreq ( > > The ArmPlatform prefix is unnecessary here: please use > PL011UartClockGetFreq() instead. Ok > > + 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 > > +* > > > +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fo > pe > > +nsource.org%2Flicenses%2Fbsd- > license.php&data=02%7C01%7Cudit.kumar%40 > > > +nxp.com%7C2ac27eb60055478ac37d08d5cf82d550%7C686ea1d3bc2b4c > 6fa92cd99c > > > +5c301635%7C0%7C0%7C636643083195138179&sdata=C%2F2MhK2ZA6 > XPmC5c8byWlK3 > > +xAC6rFmLYofPKlj6M7%2FI%3D&reserved=0 > > +* > > +* 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 <Library/PL011UartClockLib.h> > > + > > +/** > > + Return clock in for PL011 Uart IP. > > +**/ > > +UINT32 > > Please add EFIAPI even if it is defined to an empty string when using > GCC/ARM. Ok > > +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. Ok, I will move to lib class declaration > > + 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 # > > > +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fo > pe > > +nsource.org%2Flicenses%2Fbsd- > license.php&data=02%7C01%7Cudit.kumar%40 > > > +nxp.com%7C2ac27eb60055478ac37d08d5cf82d550%7C686ea1d3bc2b4c > 6fa92cd99c > > > +5c301635%7C0%7C0%7C636643083195138179&sdata=C%2F2MhK2ZA6 > XPmC5c8byWlK3 > > +xAC6rFmLYofPKlj6M7%2FI%3D&reserved=0 > > +# > > +# 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 > Ok > > + 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? May be not , copy-paste mistake > > > + ArmPkg/ArmPkg.dec > > + ArmPlatformPkg/ArmPlatformPkg.dec > > + > > Sort alphabetically please. Ok > > +[LibraryClasses] > > + ArmLib > > + DebugLib > > + > > +[Sources.common] > > + PL011UartClockLib.c > > + > > +[FixedPcd] > > + gArmPlatformTokenSpaceGuid.PL011UartClkInHz > > + > > -- > > 1.9.1 > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
