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
> >>
next prev parent 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