Re: [PATCH edk2-platforms 25/41] Silicon/NXP:SocLib support for initialization of peripherals

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

On Wed, Nov 28, 2018 at 08:31:39PM +0530, Meenakshi Aggarwal wrote:
> From: Wasim Khan <wasim.khan@nxp.com>
> 
> Added SocInit function that initializes peripherals
> and print board and soc information for LS2088ARDB Board.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Wasim Khan <wasim.khan@nxp.com>
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> ---
>  Silicon/NXP/Include/Chassis3/SerDes.h        |  91 ++++++++++++++
>  Silicon/NXP/Include/Chassis3/Soc.h           | 144 +++++++++++++++++++++
>  Silicon/NXP/LS2088A/Include/SocSerDes.h      |  67 ++++++++++
>  Silicon/NXP/Library/SocLib/Chassis.c         |  38 ++++++
>  Silicon/NXP/Library/SocLib/Chassis.h         |  17 +++
>  Silicon/NXP/Library/SocLib/Chassis3/Soc.c    | 180 +++++++++++++++++++++++++++
>  Silicon/NXP/Library/SocLib/LS2088aSocLib.inf |  50 ++++++++
>  Silicon/NXP/Library/SocLib/SerDes.c          |   3 +
>  8 files changed, 590 insertions(+)
>  create mode 100644 Silicon/NXP/Include/Chassis3/SerDes.h
>  create mode 100644 Silicon/NXP/Include/Chassis3/Soc.h
>  create mode 100644 Silicon/NXP/LS2088A/Include/SocSerDes.h
>  create mode 100644 Silicon/NXP/Library/SocLib/Chassis3/Soc.c
>  create mode 100644 Silicon/NXP/Library/SocLib/LS2088aSocLib.inf
> 
> diff --git a/Silicon/NXP/Include/Chassis3/SerDes.h b/Silicon/NXP/Include/Chassis3/SerDes.h
> new file mode 100644
> index 0000000..a77ddd5
> --- /dev/null
> +++ b/Silicon/NXP/Include/Chassis3/SerDes.h
> @@ -0,0 +1,91 @@
> +/** SerDes.h
> + The Header file of SerDes Module for Chassis 3
> +
> + Copyright 2017 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
> + http://opensource.org/licenses/bsd-license.php
> +
> + 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 __SERDES_H__
> +#define __SERDES_H__

NXP_ prefix?

> +
> +#include <Uefi/UefiBaseType.h>
> +
> +#define SRDS_MAX_LANES    8
> +
> +//
> +// SerDes lane protocols/devices
> +//
> +typedef enum {
> +  NONE = 0,
> +  PCIE1,
> +  PCIE2,
> +  PCIE3,
> +  PCIE4,
> +  SATA1,
> +  SATA2,
> +  XAUI1,
> +  XAUI2,
> +  XFI1,
> +  XFI2,
> +  XFI3,
> +  XFI4,
> +  XFI5,
> +  XFI6,
> +  XFI7,
> +  XFI8,
> +  SGMII1,
> +  SGMII2,
> +  SGMII3,
> +  SGMII4,
> +  SGMII5,
> +  SGMII6,
> +  SGMII7,
> +  SGMII8,
> +  SGMII9,
> +  SGMII10,
> +  SGMII11,
> +  SGMII12,
> +  SGMII13,
> +  SGMII14,
> +  SGMII15,
> +  SGMII16,
> +  QSGMII_A,
> +  QSGMII_B,
> +  QSGMII_C,
> +  QSGMII_D,
> +  // Number of entries in this enum
> +  SERDES_PRTCL_COUNT

CamelCase for enum member names - throughout.

> +} SERDES_PROTOCOL;
> +
> +typedef enum {
> +  SRDS_1  = 0,
> +  SRDS_2,
> +  SRDS_MAX_NUM
> +} SERDES_NUMBER;
> +
> +typedef struct {
> +  UINT16 Protocol;
> +  UINT8  SrdsLane[SRDS_MAX_LANES];
> +} SERDES_CONFIG;
> +
> +typedef VOID
> +(*SERDES_PROBE_LANES_CALLBACK) (
> +  IN SERDES_PROTOCOL LaneProtocol,
> +  IN VOID *Arg
> +  );
> +
> +VOID
> +SerDesProbeLanes(
> +  IN SERDES_PROBE_LANES_CALLBACK SerDesLaneProbeCallback,
> +  IN VOID *Arg
> +  );
> +
> +#endif /* __SERDES_H */
> diff --git a/Silicon/NXP/Include/Chassis3/Soc.h b/Silicon/NXP/Include/Chassis3/Soc.h
> new file mode 100644
> index 0000000..8d967e7
> --- /dev/null
> +++ b/Silicon/NXP/Include/Chassis3/Soc.h
> @@ -0,0 +1,144 @@
> +/** Soc.h
> +*  Header defining the Base addresses, sizes, flags etc for chassis 1
> +*
> +*  Copyright 2017 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
> +*  http://opensource.org/licenses/bsd-license.php
> +*
> +*  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 __SOC_H__
> +#define __SOC_H__

NXP_ prefix?

> +
> +#define FSL_CLK_GRPA_ADDR          0x01300000
> +#define FSL_CLK_GRPB_ADDR          0x01310000
> +
> +#define FSL_CLUSTER_CLOCKS         { 1, 1, 4, 4 } /* LS208x */
> +#define TP_CLUSTER_EOC_MASK        0x80000000      /* Mask for End of clusters */
> +#define NUM_CC_PLLS                6
> +#define CLK_FREQ                   100000000
> +#define MAX_CPUS                   16
> +#define CHECK_CLUSTER(Cluster)     ((Cluster & TP_CLUSTER_EOC_MASK) != TP_CLUSTER_EOC_MASK)
> +
> +/* RCW SERDES MACRO */
> +#define RCWSR_INDEX                28
> +#define RCWSR_SRDS1_PRTCL_MASK     0x00ff0000
> +#define RCWSR_SRDS1_PRTCL_SHIFT    16
> +#define RCWSR_SRDS2_PRTCL_MASK     0xff000000
> +#define RCWSR_SRDS2_PRTCL_SHIFT    24
> +
> +/* SMMU Defintions */
> +#define SMMU_BASE_ADDR             0x05000000
> +#define SMMU_REG_SCR0              (SMMU_BASE_ADDR + 0x0)
> +#define SMMU_REG_SACR              (SMMU_BASE_ADDR + 0x10)
> +#define SMMU_REG_IDR1              (SMMU_BASE_ADDR + 0x24)
> +#define SMMU_REG_NSCR0             (SMMU_BASE_ADDR + 0x400)
> +#define SMMU_REG_NSACR             (SMMU_BASE_ADDR + 0x410)
> +
> +#define SCR0_USFCFG_MASK           0x00000400
> +#define SCR0_CLIENTPD_MASK         0x00000001
> +#define SACR_PAGESIZE_MASK         0x00010000
> +
> +typedef struct {
> +  UINTN FreqProcessor[MAX_CPUS];
> +  UINTN FreqSystemBus;
> +  UINTN FreqDdrBus;
> +  UINTN FreqDdrBus2;
> +  UINTN FreqLocalBus;
> +  UINTN FreqSdhc;
> +  UINTN FreqFman[1];

Fman?
Why [1]=?

> +  UINTN FreqQman;

Qman?

> +  UINTN FreqPme;

Pme?

> +} SYS_INFO;
> +
> +/* Device Configuration and Pin Control */
> +typedef struct {
> +  UINT32   PorSr1;         /* POR status 1 */
> +  UINT32   PorSr2;         /* POR status 2 */
> +  UINT8    Res008[0x18];

Why is the first reserved field of the struct called #8?
Oh, 8 bytes in?
I would much rather see the offset written in a comment next to it.
Please use decimal size for array.
Also, please write out Reserved.
Both comments apply throughout.

> +  UINT32   GppOrCr1;       /* General-purpose POR configuration */
> +  UINT32   GppOrCr2;       /* General-purpose POR configuration 2 */
> +  UINT32   DcfgFuseSr;    /* Fuse status register */

Comment indentation.

> +  UINT32   GppOrCr3;
> +  UINT32   GppOrCr4;
> +  UINT8    Res034[0x3C];
> +  UINT32   DevDisr;        /* Device disable control */
> +  UINT32   DevDisr2;       /* Device disable control 2 */
> +  UINT32   DevDisr3;       /* Device disable control 3 */
> +  UINT32   DevDisr4;       /* Device disable control 4 */
> +  UINT32   DevDisr5;       /* Device disable control 5 */
> +  UINT32   DevDisr6;       /* Device disable control 6 */
> +  UINT32   DevDisr7;       /* Device disable control 7 */
> +  UINT8    Res08c[0x4];
> +  UINT32   CoreDisrUpper;  /* uppper portion for support of 64 cores */
> +  UINT32   CoreDisrLower;  /* lower portion for support of 64 cores */
> +  UINT8    Res098[0x8];
> +  UINT32   Pvr;            /* Processor version */
> +  UINT32   Svr;            /* System version */
> +  UINT32   Mvr;            /* Manufacturing version */
> +  UINT8    Res0ac[0x54];
> +  UINT32   RcwSr[32];      /* Reset control word status */
> +#define CHASSIS3_RCWSR_0_SYS_PLL_RAT_SHIFT    2
> +#define CHASSIS3_RCWSR_0_SYS_PLL_RAT_MASK     0x1f
> +#define CHASSIS3_RCWSR_0_MEM_PLL_RAT_SHIFT    10
> +#define CHASSIS3_RCWSR_0_MEM_PLL_RAT_MASK     0x3f
> +#define CHASSIS3_RCWSR_0_MEM2_PLL_RAT_SHIFT   18
> +#define CHASSIS3_RCWSR_0_MEM2_PLL_RAT_MASK    0x3f
> +  UINT8    Res180[0x80];
> +  UINT32   ScratchRw[32];  /* Scratch Read/Write */
> +  UINT8    Res280[0x80];
> +  UINT32   ScratchW1R[4];  /* Scratch Read (Write once) */
> +  UINT8    Res310[0xF0];
> +  UINT32   BootLocPtrL;      /* Low addr : Boot location pointer */
> +  UINT32   BootLocPtrH;      /* High addr : Boot location pointer */
> +  UINT8    Res408[0xF8];
> +  UINT8    Res500[0x240];
> +  UINT32   TpItyp[64];
> +  struct {
> +    UINT32 Upper;
> +    UINT32 Lower;
> +  } TpCluster[3];
> +  UINT8    Res858[0x7A8];
> +} CCSR_GUR;
> +
> +/* Clocking */
> +typedef struct {
> +  struct {
> +    UINT32 Csr;        /* core cluster n clock control status */
> +    UINT8  Res04[0x1C];
> +  } ClkCnCsr[8];
> +} CCSR_CLT_CTRL;
> +
> +/* Clock Cluster */
> +typedef struct {
> +  struct {
> +    UINT8      Res00[0x10];
> +    UINT32     Csr;             /* core cluster n clock control status */
> +    UINT8      Res14[0xC];
> +  } HwnCsr[3];
> +  UINT8      Res60[0x20];
> +  struct {
> +    UINT32     Gsr;             /* core cluster n clock general status */
> +    UINT8      Res84[0x1C];
> +  } PllnGsr[3];
> +  UINT8      Rese0[0x20];
> +} CCSR_CLK_CLUSTER;
> +
> +VOID
> +GetSysInfo (
> +  OUT SYS_INFO *
> +  );
> +
> +UINT32
> +EFIAPI
> +GurRead (
> +  IN  UINTN     Address
> +  );
> +
> +#endif /* __SOC_H__ */
> diff --git a/Silicon/NXP/LS2088A/Include/SocSerDes.h b/Silicon/NXP/LS2088A/Include/SocSerDes.h
> new file mode 100644
> index 0000000..9135423
> --- /dev/null
> +++ b/Silicon/NXP/LS2088A/Include/SocSerDes.h
> @@ -0,0 +1,67 @@
> +/** @file
> + The Header file of SerDes Module for LS2088A
> +
> + Copyright 2017 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
> + http://opensource.org/licenses/bsd-license.php
> +
> + 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 __SOC_SERDES_H__
> +#define __SOC_SERDES_H__
> +
> +#include <Chassis3/SerDes.h>
> +
> +SERDES_CONFIG SerDes1ConfigTbl[] = {

Needs to be in a .c file.
Also, STATIC and m-prefix.

> +  // SerDes 1
> +  { 0x03, { PCIE2, PCIE2, PCIE2, PCIE2, PCIE1, PCIE1, PCIE1, PCIE1 } },
> +  { 0x05, { PCIE2, PCIE2, PCIE2, PCIE2, SGMII4, SGMII3, SGMII2, SGMII1 } },
> +  { 0x07, { SGMII8, SGMII7, SGMII6, SGMII5, SGMII4, SGMII3, SGMII2, SGMII1 } },
> +  { 0x09, { SGMII8, SGMII7, SGMII6, SGMII5, SGMII4, SGMII3, SGMII2, SGMII1 } },
> +  { 0x0A, { SGMII8, SGMII7, SGMII6, SGMII5, SGMII4, SGMII3, SGMII2, SGMII1 } },
> +  { 0x0C, { SGMII8, SGMII7, SGMII6, SGMII5, SGMII4, SGMII3, SGMII2, SGMII1 } },
> +  { 0x0E, { SGMII8, SGMII7, SGMII6, SGMII5, SGMII4, SGMII3, SGMII2, SGMII1 } },
> +  { 0x26, { SGMII8, SGMII7, SGMII6, SGMII5, SGMII4, SGMII3, XFI2, XFI1 } },
> +  { 0x28, { SGMII8, SGMII7, SGMII6, SGMII5, XFI4, XFI3, XFI2, XFI1 } },
> +  { 0x2A, { XFI8, XFI7, XFI6, XFI5, XFI4, XFI3, XFI2, XFI1 } },
> +  { 0x2B, { SGMII8, SGMII7, SGMII6, SGMII5, XAUI1, XAUI1, XAUI1, XAUI1 } },
> +  { 0x32, { XAUI2, XAUI2, XAUI2, XAUI2, XAUI1, XAUI1, XAUI1, XAUI1 } },
> +  { 0x33, { PCIE2, PCIE2, PCIE2, PCIE2, QSGMII_D, QSGMII_C, QSGMII_B, QSGMII_A } },
> +  { 0x35, { QSGMII_D, QSGMII_C, QSGMII_B, PCIE2, XFI4, XFI3, XFI2, XFI1 } },
> +  {}
> +};
> +
> +SERDES_CONFIG SerDes2ConfigTbl[] = {

Same as above.

> +  // SerDes 2
> +  { 0x07, { SGMII9, SGMII10, SGMII11, SGMII12, SGMII13, SGMII14, SGMII15, SGMII16 } },
> +  { 0x09, { SGMII9, SGMII10, SGMII11, SGMII12, SGMII13, SGMII14, SGMII15, SGMII16 } },
> +  { 0x0A, { SGMII9, SGMII10, SGMII11, SGMII12, SGMII13, SGMII14, SGMII15, SGMII16 } },
> +  { 0x0C, { SGMII9, SGMII10, SGMII11, SGMII12, SGMII13, SGMII14, SGMII15, SGMII16 } },
> +  { 0x0E, { SGMII9, SGMII10, SGMII11, SGMII12, SGMII13, SGMII14, SGMII15, SGMII16 } },
> +  { 0x3D, { PCIE3, PCIE3, PCIE3, PCIE3, PCIE3, PCIE3, PCIE3, PCIE3 } },
> +  { 0x3E, { PCIE3, PCIE3, PCIE3, PCIE3, PCIE3, PCIE3, PCIE3, PCIE3 } },
> +  { 0x3F, { PCIE3, PCIE3, PCIE3, PCIE3, PCIE4, PCIE4, PCIE4, PCIE4 } },
> +  { 0x40, { PCIE3, PCIE3, PCIE3, PCIE3, PCIE4, PCIE4, PCIE4, PCIE4 } },
> +  { 0x41, { PCIE3, PCIE3, PCIE3, PCIE3, PCIE4, PCIE4, SATA1, SATA2 } },
> +  { 0x42, { PCIE3, PCIE3, PCIE3, PCIE3, PCIE4, PCIE4, SATA1, SATA2 } },
> +  { 0x43, { PCIE3, PCIE3, PCIE3, PCIE3, NONE, NONE, SATA1, SATA2 } },
> +  { 0x44, { PCIE3, PCIE3, PCIE3, PCIE3, NONE, NONE, SATA1, SATA2 } },
> +  { 0x45, { SGMII9, SGMII10, SGMII11, SGMII12, PCIE4, PCIE4, PCIE4, PCIE4 } },
> +  { 0x47, { PCIE3, SGMII10, SGMII11, SGMII12, PCIE4, SGMII14, SGMII15, SGMII16 } },
> +  { 0x49, { SGMII9, SGMII10, SGMII11, SGMII12, PCIE4, PCIE4, SATA1, SATA2 } },
> +  { 0x4A, { SGMII9, SGMII10, SGMII11, SGMII12, PCIE4, PCIE4, SATA1, SATA2 } },
> +  {}
> +};
> +
> +SERDES_CONFIG *SerDesConfigTbl[] = {

Same as above, but g-prefix.
Probably want Nxp/Qoriq prefix too.

> +  SerDes1ConfigTbl,
> +  SerDes2ConfigTbl
> +};
> +
> +#endif /* __SOC_SERDES_H */
> diff --git a/Silicon/NXP/Library/SocLib/Chassis.c b/Silicon/NXP/Library/SocLib/Chassis.c
> index e8e69a6..58f1ba7 100644
> --- a/Silicon/NXP/Library/SocLib/Chassis.c
> +++ b/Silicon/NXP/Library/SocLib/Chassis.c
> @@ -16,6 +16,8 @@
>  #include <Base.h>
>  #ifdef CHASSIS2
>  #include <Chassis2/Soc.h>
> +#elif CHASSIS3
> +#include <Chassis3/Soc.h>
>  #endif
>  #include <Library/BaseLib.h>
>  #include <Library/IoAccessLib.h>
> @@ -46,6 +48,7 @@ GurRead (
>  STATIC CPU_TYPE CpuTypeList[] = {
>    CPU_TYPE_ENTRY (LS1043A, LS1043A, 4),
>    CPU_TYPE_ENTRY (LS1046A, LS1046A, 4),
> +  CPU_TYPE_ENTRY (LS2088A, LS2088A, 8),
>  };
>  
>  /*
> @@ -133,6 +136,41 @@ CpuNumCores (
>  }
>  
>  /*
> + *  Return core's cluster

Need a more detailed description - what format is "Core"?

> + */
> +INT32
> +QoriqCoreToCluster (
> +  IN UINTN Core
> +  )
> +{
> +  CCSR_GUR  *GurBase;
> +  UINTN     ClusterIndex;
> +  UINTN     Count;
> +  UINT32    Cluster;
> +  UINT32    Type;
> +  UINTN     InitiatorIndex;
> +
> +  GurBase = (VOID *)PcdGet64 (PcdGutsBaseAddr);
> +  ClusterIndex = 0;
> +  Count = 0;
> +  do {
> +    Cluster = GurRead ((UINTN)&GurBase->TpCluster[ClusterIndex].Lower);
> +    for (InitiatorIndex = 0; InitiatorIndex < TP_INIT_PER_CLUSTER; InitiatorIndex++) {
> +      Type = InitiatorType (Cluster, InitiatorIndex);
> +      if (Type) {
> +        if (Count == Core) {
> +          return ClusterIndex;
> +        }
> +        Count++;
> +      }
> +    }
> +    ClusterIndex++;
> +  } while (CHECK_CLUSTER (Cluster));
> +
> +  return -1;      // cannot identify the cluster

Please use a #define.

> +}
> +
> +/*
>   *  Return the type of core i.e. A53, A57 etc of inputted
>   *  core number.
>   */
> diff --git a/Silicon/NXP/Library/SocLib/Chassis.h b/Silicon/NXP/Library/SocLib/Chassis.h
> index 5b7e5c4..3ac18bf 100644
> --- a/Silicon/NXP/Library/SocLib/Chassis.h
> +++ b/Silicon/NXP/Library/SocLib/Chassis.h
> @@ -57,6 +57,7 @@ CpuMaskNext (
>  #define SVR_WO_E                    0xFFFFFE
>  #define SVR_LS1043A                 0x879200
>  #define SVR_LS1046A                 0x870700
> +#define SVR_LS2088A                 0x870901
>  
>  #define SVR_MAJOR(svr)              (((svr) >> 4) & 0xf)
>  #define SVR_MINOR(svr)              (((svr) >> 0) & 0xf)
> @@ -142,4 +143,20 @@ CpuNumCores (
>    VOID
>    );
>  
> +/*
> + * Return the type of initiator for core/hardware accelerator for given core index.
> + */
> +UINTN
> +QoriqCoreToType (
> +  IN UINTN Core
> +  );

A function of this name was added in 3/41 - please move the
declaration to the same patch.

> +
> +/*
> + *  Return the cluster of initiator for core/hardware accelerator for given core index.
> + */
> +INT32
> +QoriqCoreToCluster (
> +  IN UINTN Core
> +  );
> +
>  #endif /* __CHASSIS_H__ */
> diff --git a/Silicon/NXP/Library/SocLib/Chassis3/Soc.c b/Silicon/NXP/Library/SocLib/Chassis3/Soc.c
> new file mode 100644
> index 0000000..0fc92f4
> --- /dev/null
> +++ b/Silicon/NXP/Library/SocLib/Chassis3/Soc.c
> @@ -0,0 +1,180 @@
> +/** @Soc.c
> +  SoC specific Library containg functions to initialize various SoC components
> +
> +  Copyright 2017 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
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  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 <Base.h>
> +#include <Chassis.h>
> +#include <Chassis3/Soc.h>
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/SerialPortLib.h>
> +
> +VOID
> +GetSysInfo (
> +  OUT SYS_INFO *PtrSysInfo
> +  )
> +{
> +  UINT32 Index;
> +  CCSR_GUR *GurBase;
> +  CCSR_CLT_CTRL *ClkBase;
> +  CCSR_CLK_CLUSTER  *ClkGrp[2] = {

That 2 looks superfluous.

> +    (VOID *) (FSL_CLK_GRPA_ADDR),
> +    (VOID *) (FSL_CLK_GRPB_ADDR)
> +  };
> +
> +  CONST UINT8 CoreCplxPll[16] = {
> +    [0] = 0,        // CC1 PPL / 1
> +    [1] = 0,        // CC1 PPL / 2
> +    [2] = 0,        // CC1 PPL / 4
> +    [4] = 1,        // CC2 PPL / 1
> +    [5] = 1,        // CC2 PPL / 2
> +    [6] = 1,        // CC2 PPL / 4
> +    [8] = 2,        // CC3 PPL / 1
> +    [9] = 2,        // CC3 PPL / 2
> +    [10] = 2,       // CC3 PPL / 4
> +    [12] = 3,       // CC4 PPL / 1
> +    [13] = 3,       // CC4 PPL / 2
> +    [14] = 3,       // CC4 PPL / 4
> +  };
> +
> +  CONST UINT8 CoreCplxPllDivisor[16] = {
> +    [0] = 1,        // CC1 PPL / 1
> +    [1] = 2,        // CC1 PPL / 2
> +    [2] = 4,        // CC1 PPL / 4
> +    [4] = 1,        // CC2 PPL / 1
> +    [5] = 2,        // CC2 PPL / 2
> +    [6] = 4,        // CC2 PPL / 4
> +    [8] = 1,        // CC3 PPL / 1
> +    [9] = 2,        // CC3 PPL / 2
> +    [10] = 4,       // CC3 PPL / 4
> +    [12] = 1,       // CC4 PPL / 1
> +    [13] = 2,       // CC4 PPL / 2
> +    [14] = 4,       // CC4 PPL / 4
> +  };
> +
> +  INT32 CcGroup[12] = FSL_CLUSTER_CLOCKS;

Why is this one initialised via a macro, but the others inline?
Also, why is this one not CONST, given the others, and the constant
initialiser?

Please move all of these preinitialised structures out of this
function and give them the STATIC attribute and an m-prefix.

And regardless of initialiser, that 12 looks like an accident waiting
to happen. Why is it 12? The initialiser has 4 elements. Can the 12
be dropped?

> +  UINTN PllCount;
> +  UINTN Cluster;
> +  UINTN FreqCPll[NUM_CC_PLLS];
> +  UINTN PllRatio[NUM_CC_PLLS];
> +  UINTN SysClk;
> +  UINT32 Cpu;
> +  UINT32 CPllSel;
> +  UINT32 CplxPll;
> +  VOID  *Offset;
> +
> +  SetMem (PtrSysInfo, sizeof (SYS_INFO), 0);
> +
> +  GurBase = (VOID *)PcdGet64 (PcdGutsBaseAddr);
> +  ClkBase = (VOID *)PcdGet64 (PcdClkBaseAddr);
> +  SysClk = CLK_FREQ;
> +
> +  PtrSysInfo->FreqSystemBus = SysClk;
> +  PtrSysInfo->FreqDdrBus = PcdGet64 (PcdDdrClk);
> +  PtrSysInfo->FreqDdrBus2 = PcdGet64 (PcdDdrClk);
> +
> +  //
> +  // selects the platform clock:SYSCLK ratio and calculate
> +  // system frequency
> +  //
> +  PtrSysInfo->FreqSystemBus *=
> +    (GurRead ((UINTN)&GurBase->RcwSr[0]) >> CHASSIS3_RCWSR_0_SYS_PLL_RAT_SHIFT) &
> +    CHASSIS3_RCWSR_0_SYS_PLL_RAT_MASK;

This pattern, repeated thrice, seems ripe for a macro of some sort:
#define xxx(a, pll) (((a) >> CHASSIS3_RCWSR_0_ ## pll ## _PLL_RAT_SHIFT) & \
                     CHASSIS3_RCWSR_0_ ## pll ## _PLL_RAT_MASK)
?

Called as
  xxx (GurRead ((UINTN)&GurBase->RcwSr[0]), SYS);

> +
> +  //
> +  // Platform clock is half of platform PLL
> +  //
> +  PtrSysInfo->FreqSystemBus /= PcdGet32 (PcdPlatformFreqDiv);
> +
> +  //
> +  // selects the DDR PLL:SYSCLK Ratio and calculate DDR frequency
> +  //
> +  PtrSysInfo->FreqDdrBus *=
> +    (GurRead ((UINTN)&GurBase->RcwSr[0]) >> CHASSIS3_RCWSR_0_MEM_PLL_RAT_SHIFT) &
> +    CHASSIS3_RCWSR_0_MEM_PLL_RAT_MASK;
> +
> +  PtrSysInfo->FreqDdrBus2 *=
> +    (GurRead ((UINTN)&GurBase->RcwSr[0]) >> CHASSIS3_RCWSR_0_MEM2_PLL_RAT_SHIFT) &
> +    CHASSIS3_RCWSR_0_MEM2_PLL_RAT_MASK;
> +
> +  for (PllCount = 0; PllCount < NUM_CC_PLLS; PllCount++) {
> +    Offset = (VOID *)((UINTN)ClkGrp[PllCount/3] +

Why /3? Can it be replaced by a #define?

> +        __builtin_offsetof (CCSR_CLK_CLUSTER, PllnGsr[PllCount%3].Gsr));

Indent to be aligned with statement it is a continuation of.

> +    PllRatio[PllCount] = (GurRead ((UINTN)Offset) >> 1) & 0x3f;

#define for that 0x3f please. And preferably for the 1 as well.

> +    FreqCPll[PllCount] = SysClk * PllRatio[PllCount];
> +  }
> +
> +  //
> +  // Calculate Core frequency
> +  //
> +  ForEachCpu (Index, Cpu, CpuNumCores (), CpuMask ()) {
> +    Cluster = QoriqCoreToCluster (Cpu);
> +    ASSERT_EFI_ERROR (Cluster);
> +    CPllSel = (GurRead ((UINTN)&ClkBase->ClkCnCsr[Cluster].Csr) >> 27) & 0xf;

#defines for 27 and 0xf?

> +    CplxPll = CoreCplxPll[CPllSel];
> +    CplxPll += CcGroup[Cluster] - 1;

Why -1? #define?

> +    PtrSysInfo->FreqProcessor[Cpu] = FreqCPll[CplxPll] / CoreCplxPllDivisor[CPllSel];
> +  }
> +  PtrSysInfo->FreqSdhc = PtrSysInfo->FreqSystemBus/PcdGet32 (PcdPlatformFreqDiv);
> +}
> +
> +/**
> +  Perform the early initialization.
> +  This function is called by the ArmPlatformPkg/Pei or ArmPlatformPkg/Pei/PlatformPeim
> +
> +**/
> +VOID
> +SocInit (
> +  VOID
> +  )
> +{
> +  CHAR8 Buffer[100];
> +  UINTN CharCount;
> +
> +  //
> +  // Initialize SMMU
> +  //
> +  SmmuInit ();
> +
> +  //
> +  //  Initialize the Serial Port.
> +  //  Early serial port initialization is required to print RCW,
> +  //  Soc and CPU infomation at the begining of UEFI boot.
> +  //
> +  SerialPortInitialize ();
> +
> +  CharCount = AsciiSPrint (Buffer, sizeof (Buffer),
> +    "\nUEFI firmware (version %s built at %a on %a)\n\r",
> +    (CHAR16*)PcdGetPtr (PcdFirmwareVersionString), __TIME__, __DATE__);
> +  SerialPortWrite ((UINT8 *) Buffer, CharCount);
> +
> +  //
> +  // Print CPU information
> +  //

Function name sufficient, comment not needed.

> +  PrintCpuInfo ();
> +
> +  //
> +  // Print Reset Controll Word
> +  //
> +  PrintRCW ();

PrintResetControlWord ();
Then the comment can be deleted.

> +
> +  //
> +  // Print Soc Personality information
> +  //
> +  PrintSoc ();

PrintSocPersonalityInfo ();
Then the comment can be deleted.

/
    Leif

> +}
> diff --git a/Silicon/NXP/Library/SocLib/LS2088aSocLib.inf b/Silicon/NXP/Library/SocLib/LS2088aSocLib.inf
> new file mode 100644
> index 0000000..3d9237d
> --- /dev/null
> +++ b/Silicon/NXP/Library/SocLib/LS2088aSocLib.inf
> @@ -0,0 +1,50 @@
> +#  @file
> +#
> +#  Copyright 2017 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
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  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                    = 0x0001001A
> +  BASE_NAME                      = SocLib
> +  FILE_GUID                      = 3b233a6a-0ee1-42a3-a7f7-c285b5ba80dc
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = SocLib
> +
> +[Packages]
> +  MdeModulePkg/MdeModulePkg.dec
> +  MdePkg/MdePkg.dec
> +  Silicon/NXP/NxpQoriqLs.dec
> +  Silicon/NXP/LS2088A/LS2088A.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  IoAccessLib
> +  SerialPortLib
> +
> +[Sources.common]
> +  Chassis.c
> +  Chassis3/Soc.c
> +  SerDes.c
> +
> +[BuildOptions]
> +  GCC:*_*_*_CC_FLAGS = -DCHASSIS3
> +
> +[FixedPcd]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> +  gNxpQoriqLsTokenSpaceGuid.PcdGutsBaseAddr
> +  gNxpQoriqLsTokenSpaceGuid.PcdPlatformFreqDiv
> +  gNxpQoriqLsTokenSpaceGuid.PcdSerdes2Enabled
> +  gNxpQoriqLsTokenSpaceGuid.PcdGurBigEndian
> +  gNxpQoriqLsTokenSpaceGuid.PcdClkBaseAddr
> +  gNxpQoriqLsTokenSpaceGuid.PcdDdrClk
> diff --git a/Silicon/NXP/Library/SocLib/SerDes.c b/Silicon/NXP/Library/SocLib/SerDes.c
> index e31e4f3..9eba8ae 100644
> --- a/Silicon/NXP/Library/SocLib/SerDes.c
> +++ b/Silicon/NXP/Library/SocLib/SerDes.c
> @@ -16,6 +16,9 @@
>  #ifdef CHASSIS2
>  #include <Chassis2/SerDes.h>
>  #include <Chassis2/Soc.h>
> +#elif CHASSIS3
> +#include <Chassis3/SerDes.h>
> +#include <Chassis3/Soc.h>
>  #endif
>  #include <Library/DebugLib.h>
>  #include <SocSerDes.h>
> -- 
> 1.9.1
>