Re: [PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq Support

[Udit Kumar <udit.kumar@nxp.com>]

> -----Original Message-----
> From: Alexei Fedorov [mailto:Alexei.Fedorov@arm.com]
> Sent: Tuesday, June 5, 2018 6:01 PM
> To: Udit Kumar <udit.kumar@nxp.com>; edk2-devel@lists.01.org;
> ard.biesheuvel@linaro.org; leif.lindholm@linaro.org
> Subject: RE: [edk2] [PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq
> Support
> 
> Please see my comment in-lined
> 
> > -----Original Message-----
> > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Udit
> > Kumar
> > Sent: 05 June 2018 00:36
> > To: edk2-devel@lists.01.org; ard.biesheuvel@linaro.org;
> > leif.lindholm@linaro.org
> > Subject: [edk2] [PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq
> > Support
> >
> > 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.kumar@nxp.com>
> > ---
> >  .../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
> >
> > 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
> > @@ -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%2Fope
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%40
> >
> +nxp.com%7Cdd365496e05b46d7be4508d5cae0286e%7C686ea1d3bc2b4c6fa9
> 2cd99c
> >
> +5c301635%7C0%7C1%7C636637986464241388&sdata=rmypRtJplfoFQHAftihQ
> tfqHQ
> > +IcLO0Xele3IKdab6fM%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
> 
> Why cannot this function just return FixedPcdGet32 (PL011UartClkInHz) if
> dynamic clocking is not supported?

Ok, agreed with Ard's comment too 
Thanks
 
> > +
> > +  @return Return frequency of PL011
> > +
> > +**/
> > +UINT32
> > +ArmPlatformGetPL011ClockFreq (
> > +  VOID
> > +  );
> > +
> > +#endif
> > diff --git
> > a/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLib.i
> > nf
> > b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLib.i
> > nf
> > new file mode 100644
> > index 0000000..b708ad3
> > --- /dev/null
> > +++ b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockL
> > +++ ib
> > +++ .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%2Fope
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%40
> >
> +nxp.com%7Cdd365496e05b46d7be4508d5cae0286e%7C686ea1d3bc2b4c6fa9
> 2cd99c
> >
> +5c301635%7C0%7C1%7C636637986464241388&sdata=rmypRtJplfoFQHAftihQ
> tfqHQ
> > +IcLO0Xele3IKdab6fM%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
> > +  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/ArmPlatformClockLibNu
> > ll.c
> > b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLibNu
> > ll.c
> > new file mode 100644
> > index 0000000..28eaa63
> > --- /dev/null
> > +++ b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockL
> > +++ ib
> > +++ Null.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%2Fope
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%40
> >
> +nxp.com%7Cdd365496e05b46d7be4508d5cae0286e%7C686ea1d3bc2b4c6fa9
> 2cd99c
> >
> +5c301635%7C0%7C1%7C636637986464241388&sdata=rmypRtJplfoFQHAftihQ
> tfqHQ
> > +IcLO0Xele3IKdab6fM%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;
> > +}
> > --
> > 1.9.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flis
> > ts.01.org%2Fmailman%2Flistinfo%2Fedk2-
> devel&data=02%7C01%7Cudit.kumar%
> >
> 40nxp.com%7Cdd365496e05b46d7be4508d5cae0286e%7C686ea1d3bc2b4c6fa
> 92cd99
> >
> c5c301635%7C0%7C1%7C636637986464241388&sdata=u3j1ouO0GgtN2uKGAQ
> BdJ%2Bh
> > PdQkIjfyd3t7N%2FYyTCTA%3D&reserved=0
> IMPORTANT NOTICE: The contents of this email and any attachments are
> confidential and may also be privileged. If you are not the intended recipient,
> please notify the sender immediately and do not disclose the contents to any
> other person, use it for any purpose, or store or copy the information in any
> medium. Thank you.