> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org]
> Sent: Tuesday, June 5, 2018 5:56 PM
> To: Udit Kumar <udit.ku...@nxp.com>
> Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindh...@linaro.org>
> Subject: Re: [edk2][PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq
> Support
>
> On 5 June 2018 at 14:23, Ard Biesheuvel <ard.biesheu...@linaro.org> wrote:
> > On 5 June 2018 at 01:35, 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 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 <udit.ku...@nxp.com>
> >
> > 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.in
> >> f create mode 100644
> >> ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLibNul
> >> l.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
> >> +*
> >> +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
> >> +ensource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%
> >>
> +40nxp.com%7C1293bf5f16dd456e1a4708d5cadf750d%7C686ea1d3bc2b4c6fa
> 92cd
> >>
> +99c5c301635%7C0%7C0%7C636637983450007594&sdata=1qT2ZTJAYabHBvcQ
> kNXnP
> >> +PPQgM038f7hSYUa7r2Hhvo%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 _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/ArmPlatformClock
> >> +++ Lib.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 #
> >> +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
> >> +ensource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%
> >>
> +40nxp.com%7C1293bf5f16dd456e1a4708d5cadf750d%7C686ea1d3bc2b4c6fa
> 92cd
> >>
> +99c5c301635%7C0%7C0%7C636637983450007594&sdata=1qT2ZTJAYabHBvcQ
> kNXnP
> >> +PPQgM038f7hSYUa7r2Hhvo%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
> >> + 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.
Agree, with return of Pcd value, NULL make no sense
Will update in v2
Thanks
> >> + 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/ArmPlatformClockLibN
> >> ull.c
> >> b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLibN
> >> ull.c
> >> new file mode 100644
> >> index 0000000..28eaa63
> >> --- /dev/null
> >> +++ b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClock
> >> +++ LibNull.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
> >> +*
> >> +https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
> >> +ensource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%
> >>
> +40nxp.com%7C1293bf5f16dd456e1a4708d5cadf750d%7C686ea1d3bc2b4c6fa
> 92cd
> >>
> +99c5c301635%7C0%7C0%7C636637983450007594&sdata=1qT2ZTJAYabHBvcQ
> kNXnP
> >> +PPQgM038f7hSYUa7r2Hhvo%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/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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
