On 5 June 2018 at 14:23, Ard Biesheuvel <[email protected]> wrote: > On 5 June 2018 at 01:35, Udit Kumar <[email protected]> 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 <[email protected]> > > 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 <Library/ArmPlatformClockLib.h> >> + >> + >> + >> +/** >> + 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 >> _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
