Re: [edk2-platforms] [PATCH v2 03/11] SocLib : Add support for initialization of peripherals

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

I am a bit confused - none of the feedback from last review seems to
have been addressed. Comments below.

On Thu, Nov 21, 2019 at 21:55:06 +0530, Meenakshi Aggarwal wrote:
> Add SocInit function that initializes peripherals
> and print board and soc information.
> 
> Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> ---
>  Silicon/NXP/Library/SocLib/LS1043aSocLib.inf |  45 ++
>  Silicon/NXP/Include/Chassis2/LsSerDes.h      |  62 +++
>  Silicon/NXP/Include/Chassis2/NxpSoc.h        | 361 ++++++++++++++
>  Silicon/NXP/Include/DramInfo.h               |  38 ++
>  Silicon/NXP/LS1043A/Include/SocSerDes.h      |  51 ++
>  Silicon/NXP/Library/SocLib/NxpChassis.h      | 136 ++++++
>  Silicon/NXP/Library/SocLib/Chassis.c         | 498 ++++++++++++++++++++
>  Silicon/NXP/Library/SocLib/Chassis2/Soc.c    | 162 +++++++
>  Silicon/NXP/Library/SocLib/SerDes.c          | 268 +++++++++++
>  9 files changed, 1621 insertions(+)
 
> diff --git a/Silicon/NXP/Library/SocLib/Chassis.c b/Silicon/NXP/Library/SocLib/Chassis.c
> new file mode 100644
> index 000000000000..5dda6f8c2662
> --- /dev/null
> +++ b/Silicon/NXP/Library/SocLib/Chassis.c
> @@ -0,0 +1,498 @@
> +/** @file
> +  SoC specific Library containg functions to initialize various SoC components
> +
> +  Copyright 2017-2019 NXP
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#ifdef CHASSIS2
> +#include <Chassis2/NxpSoc.h>
> +#elif CHASSIS3
> +#include <Chassis3/NxpSoc.h>
> +#endif
> +#include <Library/ArmSmcLib.h>
> +#include <Library/BaseLib.h>
> +#include <Library/IoAccessLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/IoLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/SerialPortLib.h>
> +
> +#include <DramInfo.h>
> +#include "NxpChassis.h"
> +
> +/*
> + *  Structure to list available SOCs.
> + *  Name, Soc Version, Number of Cores
> + */
> +STATIC CPU_TYPE mCpuTypeList[] = {
> +  CPU_TYPE_ENTRY (LS1043A, LS1043A, 4),
> +  CPU_TYPE_ENTRY (LS1046A, LS1046A, 4),
> +  CPU_TYPE_ENTRY (LS2088A, LS2088A, 8),
> +};
> +
> +UINT32
> +EFIAPI
> +GurRead (
> +  IN  UINTN     Address
> +  )
> +{
> +  if (FixedPcdGetBool (PcdGurBigEndian)) {
> +    return SwapMmioRead32 (Address);
> +  } else {
> +    return MmioRead32 (Address);
> +  }
> +}
> +
> +/*
> + * Return the type of initiator (core or hardware accelerator)
> + */
> +UINT32
> +InitiatorType (
> +  IN UINT32 Cluster,
> +  IN UINTN  InitId
> +  )
> +{
> +  CCSR_GUR *GurBase;
> +  UINT32   Idx;
> +  UINT32   Type;
> +
> +  GurBase = (VOID *)PcdGet64 (PcdGutsBaseAddr);
> +  Idx = (Cluster >> (InitId * 8)) & TP_CLUSTER_INIT_MASK;
> +  Type = GurRead ((UINTN)&GurBase->TpItyp[Idx]);
> +
> +  if (Type & TP_ITYP_AV_MASK) {
> +    return Type;
> +  }
> +
> +  return 0;
> +}
> +
> +/*
> + *  Return the mask for number of cores on this SOC.
> + */
> +UINT32
> +CpuMask (
> +  VOID
> +  )
> +{
> +  CCSR_GUR  *GurBase;
> +  UINTN     ClusterIndex;
> +  UINTN     Count;
> +  UINT32    Cluster;
> +  UINT32    Type;
> +  UINT32    Mask;
> +  UINTN     InitiatorIndex;
> +
> +  GurBase = (VOID *)PcdGet64 (PcdGutsBaseAddr);
> +  ClusterIndex = 0;
> +  Count = 0;
> +  Mask = 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 (TP_ITYP_TYPE_MASK (Type) == TP_ITYP_TYPE_ARM) {
> +          Mask |= 1 << Count;
> +        }
> +        Count++;
> +      }
> +    }
> +    ClusterIndex++;
> +  } while (CHECK_CLUSTER (Cluster));
> +
> +  return Mask;
> +}
> +
> +/*
> + *  Return the number of cores on this SOC.
> + */
> +UINTN
> +CpuNumCores (
> +  VOID
> +  )
> +{
> +  UINTN Count;
> +  UINTN Num;
> +
> +  Count = 0;
> +  Num = CpuMask ();
> +
> +  while (Num) {
> +    Count += Num & 1;
> +    Num >>= 1;
> +  }
> +
> +  return Count;
> +}
> +
> +/*
> + *  Return core's cluster
> + */
> +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
> +}
> +
> +/*
> + *  Return the type of core i.e. A53, A57 etc of inputted
> + *  core number.
> + */
> +UINTN
> +QoriqCoreToType (
> +  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 Type;
> +        }
> +        Count++;
> +      }
> +    }
> +    ClusterIndex++;
> +  } while (CHECK_CLUSTER (Cluster));
> +
> +  return EFI_NOT_FOUND;      /* cannot identify the cluster */
> +}
> +
> +STATIC
> +UINTN
> +CpuMaskNext (
> +  IN  UINTN  Cpu,
> +  IN  UINTN  Mask
> +  )
> +{
> +  for (Cpu++; !((1 << Cpu) & Mask); Cpu++);
> +
> +  return Cpu;
> +}
> +
> +/*
> + * Print CPU information
> + */
> +VOID
> +PrintCpuInfo (
> +  VOID
> +  )
> +{
> +  SYS_INFO SysInfo;
> +  UINTN    CoreIndex;
> +  UINTN    Core;
> +  UINT32   Type;
> +  UINT32   NumCpus;
> +  UINT32   Mask;
> +  CHAR8    *CoreName;
> +
> +  GetSysInfo (&SysInfo);
> +  DEBUG ((DEBUG_INIT, "Clock Configuration:"));
> +
> +  NumCpus = CpuNumCores ();
> +  Mask = CpuMask ();
> +
> +  for (CoreIndex = 0, Core = CpuMaskNext(-1, Mask);
> +       CoreIndex < NumCpus;
> +       CoreIndex++, Core = CpuMaskNext(Core, Mask))
> +  {
> +    if (!(CoreIndex % 3)) {
> +      DEBUG ((DEBUG_INIT, "\n      "));
> +    }
> +
> +    Type = TP_ITYP_VERSION (QoriqCoreToType (Core));
> +    switch (Type) {
> +      case TY_ITYP_VERSION_A7:
> +        CoreName = "A7";
> +        break;
> +      case TY_ITYP_VERSION_A53:
> +        CoreName = "A53";
> +        break;
> +      case TY_ITYP_VERSION_A57:
> +        CoreName = "A57";
> +        break;
> +      case TY_ITYP_VERSION_A72:
> +        CoreName = "A72";
> +        break;
> +      default:
> +        CoreName = " Unknown Core ";
> +    }
> +    DEBUG ((DEBUG_INIT, "CPU%d(%a):%-4d MHz  ",
> +      Core, CoreName, SysInfo.FreqProcessor[Core] / MHZ));
> +  }
> +
> +  DEBUG ((DEBUG_INIT, "\n      Bus:      %-4d MHz  ", SysInfo.FreqSystemBus / MHZ));
> +  DEBUG ((DEBUG_INIT, "DDR:      %-4d MT/s", SysInfo.FreqDdrBus / MHZ));
> +
> +  if (SysInfo.FreqFman[0] != 0) {
> +    DEBUG ((DEBUG_INIT, "\n      FMAN:     %-4d MHz  ",  SysInfo.FreqFman[0] / MHZ));
> +  }
> +
> +  DEBUG ((DEBUG_INIT, "\n"));
> +}
> +
> +/*
> + * Return system bus frequency
> + */
> +UINT64
> +GetBusFrequency (
> +   VOID
> +  )
> +{
> +  SYS_INFO SocSysInfo;
> +
> +  GetSysInfo (&SocSysInfo);
> +
> +  return SocSysInfo.FreqSystemBus;
> +}
> +
> +/*
> + * Return SDXC bus frequency
> + */
> +UINT64
> +GetSdxcFrequency (
> +   VOID
> +  )
> +{
> +  SYS_INFO SocSysInfo;
> +
> +  GetSysInfo (&SocSysInfo);
> +
> +  return SocSysInfo.FreqSdhc;
> +}
> +
> +/*
> + * Print Soc information
> + */
> +VOID
> +PrintSoc (
> +  VOID
> +  )
> +{
> +  CHAR8    Buf[20];
> +  CCSR_GUR *GurBase;
> +  UINTN    Count;
> +  //
> +  // Svr : System Version Register
> +  //
> +  UINTN    Svr;
> +  UINTN    Ver;
> +
> +  GurBase = (VOID *)PcdGet64 (PcdGutsBaseAddr);
> +
> +  Svr = GurRead ((UINTN)&GurBase->Svr);
> +  Ver = SVR_SOC_VER (Svr);
> +
> +  for (Count = 0; Count < ARRAY_SIZE (mCpuTypeList); Count++) {
> +    if ((mCpuTypeList[Count].SocVer & SVR_WO_E) == Ver) {
> +      AsciiStrCpyS (Buf, AsciiStrnLenS (mCpuTypeList[Count].Name, 7) + 1,

As I said in previous review - the second parameter should be sizeof (Buf).

> +        (CONST CHAR8 *)mCpuTypeList[Count].Name);
> +
> +      if (IS_E_PROCESSOR (Svr)) {
> +        AsciiStrCatS (Buf,
> +          (AsciiStrLen (Buf) + AsciiStrLen ((CONST CHAR8 *)"E") + 1),

And here too.

> +          (CONST CHAR8 *)"E");

And please drop all casts to CHAR8 * in this function (as requested in
previous review) - they are not needed.

> +      }
> +      break;
> +    }
> +  }
> +
> +  DEBUG ((DEBUG_INFO, "SoC: %a (0x%x); Rev %d.%d\n",
> +          Buf, Svr, SVR_MAJOR (Svr), SVR_MINOR (Svr)));
> +
> +  return;
> +}