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

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

Thanks for review Ard. 

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, June 11, 2018 3:35 PM
> To: Udit Kumar <udit.kumar@nxp.com>
> Cc: Leif Lindholm <leif.lindholm@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.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 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.kumar@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
> >