Re: [platforms: PATCH 1/2] Marvell/Armada7k8k: Add basic sample at reset library

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Feb 16, 2018 at 05:35:26PM +0100, Marcin Wojtas wrote:
> From: Igal Liberman <igall@marvell.com>
> 
> The sample at reset library adds the following functionalities:
> - MvSARGetCpuFreq - Get the CPU frequency
> - MvSARGetDramFreq - Get the DRAM frequency
> - MvSARGetPcieClkDirection - Determine the PCIe clock direction
>   for two types specified in CP110 HW block. It will be needed
>   for proper configuration during the PCIE SerDes training.

Please add an explanation of what a SAR is.
Since it appears to be a specific thing referred to under that name,
it is correct to use the abbreviation throughout, but it needs to be
introduced.

Similarly, from coding standards, section 4.1.3.2
---
Any abbreviation used, which is not documented in this specification,
or in the UEFI Specification shall be placed into a Glossary section
of the File header as specified in See "File Heading".
---

(search for "File Heading" in
https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/5_source_files/52_spacing.html)

I would appreciate if you could include this in .c and .h files of
this patch.

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Igal Liberman <igall@marvell.com>
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.c   | 166 ++++++++++++++++++++
>  Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.inf |  54 +++++++
>  Silicon/Marvell/Include/Library/MvSARLib.h                               |  57 +++++++
>  Silicon/Marvell/Marvell.dec                                              |   3 +
>  4 files changed, 280 insertions(+)
>  create mode 100644 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.c
>  create mode 100644 Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.inf
>  create mode 100644 Silicon/Marvell/Include/Library/MvSARLib.h
> 
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.c b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.c
> new file mode 100644
> index 0000000..27ada6f
> --- /dev/null
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.c
> @@ -0,0 +1,166 @@
> +/********************************************************************************
> +Copyright (C) 2018 Marvell International Ltd.
> +
> +Marvell BSD License Option
> +
> +If you received this File from Marvell, you may opt to use, redistribute and/or
> +modify this File under the following licensing terms.
> +Redistribution and use in source and binary forms, with or without modification,
> +are permitted provided that the following conditions are met:
> +
> +* Redistributions of source code must Retain the above copyright notice,
> +  this list of conditions and the following disclaimer.
> +
> +* Redistributions in binary form must reproduce the above copyright
> +  notice, this list of conditions and the following disclaimer in the
> +  documentation and/or other materials provided with the distribution.
> +
> +* Neither the name of Marvell nor the names of its contributors may be
> +  used to endorse or promote products derived from this software without
> +  specific prior written permission.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +*******************************************************************************/
> +
> +#include <Uefi.h>
> +
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/MvSARLib.h>
> +
> +#define SAR_MAX_OPTIONS           16
> +
> +#define CPU_CLOCK_ID              0
> +#define DDR_CLOCK_ID              1
> +#define RING_CLOCK_ID             2
> +
> +#define MV_AP_SAR_BASE            0xf06f8200

AP?

> +#define SAR_CLOCK_FREQ_MODE_MASK  0x1f
> +
> +#define MV_CP_SAR_BASE(_CpIndex)  (0xf2000000 + (0x2000000 * _CpIndex) + 0x400200)

CP?
Missing parantheses in macro.

If CP and AP are also common terms used in documentation for this
component, please add it to the glossary (but not commit message).

> +
> +#define MAX_CP_COUNT              2
> +#define MAX_PCIE_CLK_TYPE_COUNT   2
> +
> +#define CP0_PCIE0_CLK_MASK        0x4
> +#define CP0_PCIE1_CLK_MASK        0x8
> +#define CP1_PCIE0_CLK_MASK        0x1
> +#define CP1_PCIE1_CLK_MASK        0x2
> +#define CP0_PCIE0_CLK_OFFSET        2
> +#define CP0_PCIE1_CLK_OFFSET        3
> +#define CP1_PCIE0_CLK_OFFSET        0
> +#define CP1_PCIE1_CLK_OFFSET        1

Are these not just derivatives of each other?
I.e. CP0_PCIE0_CLK_MASK is the same as (1 << CP0_PCIE0_CLK_OFFSET)?
If so, can they be defined as such?

> +
> +typedef enum {
> +  CPU_2000_DDR_1200_RCLK_1200 = 0x0,
> +  CPU_2000_DDR_1050_RCLK_1050 = 0x1,
> +  CPU_1600_DDR_800_RCLK_800   = 0x4,
> +  CPU_1800_DDR_1200_RCLK_1200 = 0x6,
> +  CPU_1800_DDR_1050_RCLK_1050 = 0x7,
> +  CPU_1600_DDR_1050_RCLK_1050 = 0x0d,
> +  CPU_1000_DDR_650_RCLK_650   = 0x13,
> +  CPU_1300_DDR_800_RCLK_800   = 0x14,
> +  CPU_1300_DDR_650_RCLK_650   = 0x17,
> +  CPU_1200_DDR_800_RCLK_800   = 0x19,
> +  CPU_1400_DDR_800_RCLK_800   = 0x1a,
> +  CPU_600_DDR_800_RCLK_800    = 0x1b,
> +  CPU_800_DDR_800_RCLK_800    = 0x1c,
> +  CPU_1000_DDR_800_RCLK_800   = 0x1d,
> +} ClockingOptions;

TianoCore coding style suggests name should be CLOCKING_OPTIONS.
And that 

Also, I would prefer to see all of the above definitions broken out
into a local include.

> +
> +static const UINT32 PllFreqTbl[SAR_MAX_OPTIONS][4] = {

STATIC CONST (throughout).

Ideally, this table would be named PllFrequencyTable.

But really, this needs be an array of struct objects, rather than
using live-coded offsets in the code.

> +  /* CPU   DDR   Ring  [MHz] */
> +  {2000,  1200,  1200, CPU_2000_DDR_1200_RCLK_1200},
> +  {2000,  1050,  1050, CPU_2000_DDR_1050_RCLK_1050},
> +  {1800,  1200,  1200, CPU_1800_DDR_1200_RCLK_1200},
> +  {1800,  1050,  1050, CPU_1800_DDR_1050_RCLK_1050},
> +  {1600,  1050,  1050, CPU_1600_DDR_1050_RCLK_1050},
> +  {1300,  800 ,  800 , CPU_1300_DDR_800_RCLK_800},
> +  {1300,  650 ,  650 , CPU_1300_DDR_650_RCLK_650},
> +  {1600,  800 ,  800 , CPU_1600_DDR_800_RCLK_800},
> +  {1000,  650 ,  650 , CPU_1000_DDR_650_RCLK_650},
> +  {1200,  800 ,  800 , CPU_1200_DDR_800_RCLK_800},
> +  {1400,  800 ,  800 , CPU_1400_DDR_800_RCLK_800},
> +  {600 ,  800 ,  800 , CPU_600_DDR_800_RCLK_800},
> +  {800 ,  800 ,  800 , CPU_800_DDR_800_RCLK_800},
> +  {1000,  800 ,  800 , CPU_1000_DDR_800_RCLK_800}
> +};
> +
> +static const UINT32 PcieClkMask[MAX_CP_COUNT][MAX_PCIE_CLK_TYPE_COUNT] = {

PcieClockMask.

> +  {CP0_PCIE0_CLK_MASK, CP0_PCIE1_CLK_MASK},
> +  {CP1_PCIE0_CLK_MASK, CP1_PCIE1_CLK_MASK}
> +};
> +
> +static const UINT32 PcieClkOffset[MAX_CP_COUNT][MAX_PCIE_CLK_TYPE_COUNT] = {

PcieClockOffset

> +  {CP0_PCIE0_CLK_OFFSET, CP0_PCIE1_CLK_OFFSET},
> +  {CP1_PCIE0_CLK_OFFSET, CP1_PCIE1_CLK_OFFSET}
> +};
> +
> +UINT32
> +EFIAPI
> +MvSARGetCpuFreq (

Frequency

> +  VOID
> +  )
> +{
> +  UINT32 ClkVal;

ClockValue

> +  UINT32 Index;
> +
> +  ClkVal = MmioAnd32 (MV_AP_SAR_BASE, SAR_CLOCK_FREQ_MODE_MASK);
> +
> +  for (Index = 0; Index < SAR_MAX_OPTIONS; Index++) {
> +    if (PllFreqTbl[Index][3] == ClkVal) {
> +      break;
> +    }
> +  }
> +
> +  return PllFreqTbl[Index][CPU_CLOCK_ID];
> +}
> +
> +UINT32
> +EFIAPI
> +MvSARGetDramFreq (

Frequency

> +  VOID
> +  )
> +{
> +  UINT32 ClkVal;
> +  UINT32 Index;
> +
> +  ClkVal = MmioAnd32 (MV_AP_SAR_BASE, SAR_CLOCK_FREQ_MODE_MASK);
> +
> +  for (Index = 0; Index < SAR_MAX_OPTIONS; Index++) {
> +    if (PllFreqTbl[Index][3] == ClkVal) {
> +      break;
> +    }
> +  }
> +
> +  return PllFreqTbl[Index][DDR_CLOCK_ID];
> +}
> +
> +UINT32
> +EFIAPI
> +MvSARGetPcieClkDirection (

Clock

> +  IN UINT32 CpIndex,
> +  IN UINT32 PcieIndex
> +  )
> +{
> +  UINT32 ClkDir;

ClockDirection.

/
    Leif

> +
> +  ASSERT (CpIndex < MAX_CP_COUNT);
> +  ASSERT (PcieIndex < MAX_PCIE_CLK_TYPE_COUNT);
> +
> +  ClkDir = MmioAnd32 (MV_CP_SAR_BASE (CpIndex),
> +             PcieClkMask[CpIndex][PcieIndex] >>
> +             PcieClkOffset[CpIndex][PcieIndex]);
> +
> +  return ClkDir;
> +}
> diff --git a/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.inf b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.inf
> new file mode 100644
> index 0000000..32b3fec
> --- /dev/null
> +++ b/Silicon/Marvell/Armada7k8k/Library/Armada7k8kSARLib/Armada7k8kSARLib.inf
> @@ -0,0 +1,54 @@
> +# Copyright (C) 2018 Marvell International Ltd.
> +#
> +# Marvell BSD License Option
> +#
> +# If you received this File from Marvell, you may opt to use, redistribute and/or
> +# modify this File under the following licensing terms.
> +# Redistribution and use in source and binary forms, with or without modification,
> +# are permitted provided that the following conditions are met:
> +#
> +# * Redistributions of source code must retain the above copyright notice,
> +#   this list of conditions and the following disclaimer.
> +#
> +# * Redistributions in binary form must reproduce the above copyright
> +#   notice, this list of conditions and the following disclaimer in the
> +#   documentation and/or other materials provided with the distribution.
> +#
> +# * Neither the name of Marvell nor the names of its contributors may be
> +#   used to endorse or promote products derived from this software without
> +#   specific prior written permission.
> +#
> +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> +# WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> +# ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> +# (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +# LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> +# ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +# (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> +# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +#
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010019
> +  BASE_NAME                      = Armada7k8kSARLib
> +  FILE_GUID                      = 03e022c7-9bd7-4608-aa21-379deaac2430
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = MvSARLib
> +
> +[Sources]
> +  Armada7k8kSARLib.c
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/Marvell/Marvell.dec
> +
> +[LibraryClasses]
> +  DebugLib
> +  IoLib
> +
> +[Depex]
> +  TRUE
> diff --git a/Silicon/Marvell/Include/Library/MvSARLib.h b/Silicon/Marvell/Include/Library/MvSARLib.h
> new file mode 100644
> index 0000000..1985a84
> --- /dev/null
> +++ b/Silicon/Marvell/Include/Library/MvSARLib.h
> @@ -0,0 +1,57 @@
> +/********************************************************************************
> +Copyright (C) 2018 Marvell International Ltd.
> +
> +Marvell BSD License Option
> +
> +If you received this File from Marvell, you may opt to use, redistribute and/or
> +modify this File under the following licensing terms.
> +Redistribution and use in source and binary forms, with or without modification,
> +are permitted provided that the following conditions are met:
> +
> +* Redistributions of source code must Retain the above copyright notice,
> +  this list of conditions and the following disclaimer.
> +
> +* Redistributions in binary form must reproduce the above copyright
> +  notice, this list of conditions and the following disclaimer in the
> +  documentation and/or other materials provided with the distribution.
> +
> +* Neither the name of Marvell nor the names of its contributors may be
> +  used to endorse or promote products derived from this software without
> +  specific prior written permission.
> +
> +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND
> +ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> +WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> +DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE LIABLE FOR
> +ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> +(INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> +LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON
> +ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> +SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> +
> +*******************************************************************************/
> +
> +#ifndef __MV_SAR_LIB_H__
> +#define __MV_SAR_LIB_H__
> +
> +UINT32
> +EFIAPI
> +MvSARGetCpuFreq (
> +  VOID
> +  );
> +
> +UINT32
> +EFIAPI
> +MvSARGetDramFreq (
> +  VOID
> +  );
> +
> +UINT32
> +EFIAPI
> +MvSARGetPcieClkDirection (
> +  IN UINT32        CpIndex,
> +  IN UINT32        PcieIndex
> +  );
> +
> +#endif /* __MV_SAR_LIB_H__ */
> diff --git a/Silicon/Marvell/Marvell.dec b/Silicon/Marvell/Marvell.dec
> index 2eb6238..b188882 100644
> --- a/Silicon/Marvell/Marvell.dec
> +++ b/Silicon/Marvell/Marvell.dec
> @@ -59,6 +59,9 @@
>    gMarvellFvbDxeGuid = { 0x42903750, 0x7e61, 0x4aaf, { 0x83, 0x29, 0xbf, 0x42, 0x36, 0x4e, 0x24, 0x85 } }
>    gMarvellSpiFlashDxeGuid = { 0x49d7fb74, 0x306d, 0x42bd, { 0x94, 0xc8, 0xc0, 0xc5, 0x4b, 0x18, 0x1d, 0xd7 } }
>  
> +[LibraryClasses]
> +  MvSARLib|Include/Library/MvSARLib.h
> +
>  [Protocols]
>    # installed as a protocol by PlatInitDxe to force ordering between DXE drivers
>    # that depend on the lowlevel platform initialization having been completed
> -- 
> 2.7.4
>