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

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

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, June 5, 2018 5:54 PM
> To: Udit Kumar <udit.kumar@nxp.com>
> Cc: edk2-devel@lists.01.org; Leif Lindholm <leif.lindholm@linaro.org>
> Subject: Re: [edk2][PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq
> Support
> 
> On 5 June 2018 at 01:35, Udit Kumar <udit.kumar@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.kumar@nxp.com>
> 
> I'm not crazy about this. How exactly are you reading the frequency in your
> case?

On our SOC, we have sysclk, which is going to system PLL. System PLL generate the
clocks for different IPs. 
Sysclk can be 66MHz to 100Mhz, the value of sysclk is read from FPGA register.
To get IP clock, we need to read system PLL multiplier, Which is derived from 
a programmable hardware configuration called RCW (Reset configuration Word)   

 
> > ---
> >  .../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?

Ok, I will rename it 
 
> 
> > 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

Ok 
 
> > @@ -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
> e
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%40
> >
> +nxp.com%7Cad5930949e7843640e4e08d5cadf3192%7C686ea1d3bc2b4
> c6fa92cd99c
> >
> +5c301635%7C0%7C0%7C636637982321509959&sdata=EjBUXzY%2F0DcM
> OZ22tlOlSK0
> > +tZqANTqrgS%2BtgzLXLxrY%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.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%2Fop
> e
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%40
> >
> +nxp.com%7Cad5930949e7843640e4e08d5cadf3192%7C686ea1d3bc2b4
> c6fa92cd99c
> >
> +5c301635%7C0%7C0%7C636637982321509959&sdata=EjBUXzY%2F0DcM
> OZ22tlOlSK0
> > +tZqANTqrgS%2BtgzLXLxrY%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/ArmPlatformClockLibN
> u
> > ll.c
> >
> b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockLibN
> u
> > ll.c
> > new file mode 100644
> > index 0000000..28eaa63
> > --- /dev/null
> > +++
> b/ArmPlatformPkg/Library/ArmPlatformClockLibNull/ArmPlatformClockL
> > +++ ibNull.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
> e
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cudit.kumar%40
> >
> +nxp.com%7Cad5930949e7843640e4e08d5cadf3192%7C686ea1d3bc2b4
> c6fa92cd99c
> >
> +5c301635%7C0%7C0%7C636637982321509959&sdata=EjBUXzY%2F0DcM
> OZ22tlOlSK0
> > +tZqANTqrgS%2BtgzLXLxrY%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.

Thanks, will do in v2. 
 
> > +}
> > --
> > 1.9.1
> >