public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Udit Kumar <udit.kumar@nxp.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
	Leif Lindholm <leif.lindholm@linaro.org>
Subject: Re: [PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq Support
Date: Tue, 5 Jun 2018 17:15:01 +0000	[thread overview]
Message-ID: <AM6PR0402MB33340389F7803043B34A9CEF91660@AM6PR0402MB3334.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <CAKv+Gu8NYdyNK2EPPQb3enBExzY4zqKFK6KiVi67JjYQ_qeUwQ@mail.gmail.com>



> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Tuesday, June 5, 2018 5:56 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 14:23, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > 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?
> >
> >> ---
> >>  .../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
> >>

  reply	other threads:[~2018-06-05 17:15 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-04 23:35 [PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq Support Udit Kumar
2018-06-04 23:35 ` [PATCH 2/2] ArmPlatformPkg: Include ArmPlatformClock Lib Udit Kumar
2018-06-05 12:24   ` Ard Biesheuvel
2018-06-05 17:13     ` Udit Kumar
2018-06-05 12:23 ` [PATCH 1/2] ArmPlatformPkg: PL011 Dynamic clock freq Support Ard Biesheuvel
2018-06-05 12:25   ` Ard Biesheuvel
2018-06-05 17:15     ` Udit Kumar [this message]
2018-06-05 17:11   ` Udit Kumar
2018-06-05 12:30 ` Alexei Fedorov
2018-06-05 17:16   ` Udit Kumar
  -- strict thread matches above, loose matches on Subject: below --
2018-06-05 17:59 [PATCH v2 0/2] " Udit Kumar
2018-06-05 17:59 ` [PATCH 1/2] " Udit Kumar
2018-06-11 10:05   ` Ard Biesheuvel
2018-06-11 10:32     ` Udit Kumar

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=AM6PR0402MB33340389F7803043B34A9CEF91660@AM6PR0402MB3334.eurprd04.prod.outlook.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox