Re: [PATCH 12/19] Silicon/NXP/LS1043A: Add SocLib

["Leif Lindholm" <leif@nuviainc.com>]

On Fri, Feb 07, 2020 at 18:13:21 +0530, Pankaj Bansal wrote:
> Add SocLib for LS1043A as per new directory structure

OK, now I'm confused again.

Separating the Chassis library from the SoC library seemed sensible.
But now we're merging them together again, only the other way around?
Can you explain why?

> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  Silicon/NXP/Chassis2/LS1043A/Include/Soc.h    | 56 +++++++++++++
>  Silicon/NXP/Chassis2/LS1043A/LS1043A.dec      | 23 ++++++
>  Silicon/NXP/Chassis2/LS1043A/LS1043A.dsc.inc  | 30 +++++++
>  .../Chassis2/LS1043A/Library/SocLib/SocLib.c  | 73 +++++++++++++++++
>  .../LS1043A/Library/SocLib/SocLib.inf         | 32 ++++++++
>  Silicon/NXP/Include/Library/SocLib.h          | 81 +++++++++++++++++++
>  Silicon/NXP/Include/Ppi/NxpPlatformGetClock.h | 47 +++++++++++
>  Silicon/NXP/NxpQoriqLs.dec                    |  3 +
>  8 files changed, 345 insertions(+)
>  create mode 100644 Silicon/NXP/Chassis2/LS1043A/Include/Soc.h
>  create mode 100644 Silicon/NXP/Chassis2/LS1043A/LS1043A.dec
>  create mode 100644 Silicon/NXP/Chassis2/LS1043A/LS1043A.dsc.inc
>  create mode 100644 Silicon/NXP/Chassis2/LS1043A/Library/SocLib/SocLib.c
>  create mode 100644 Silicon/NXP/Chassis2/LS1043A/Library/SocLib/SocLib.inf
>  create mode 100644 Silicon/NXP/Include/Library/SocLib.h
>  create mode 100644 Silicon/NXP/Include/Ppi/NxpPlatformGetClock.h
> 
> diff --git a/Silicon/NXP/Chassis2/LS1043A/Include/Soc.h b/Silicon/NXP/Chassis2/LS1043A/Include/Soc.h
> new file mode 100644
> index 0000000000..3b11b2b126
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/LS1043A/Include/Soc.h
> @@ -0,0 +1,56 @@
> +/** @file
> +
> +  Copyright 2020 NXP
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef __SOC_H__
> +#define __SOC_H__

Please drop leading __ from include guards.

> +
> +#include <Chassis.h>
> +
> +/**
> +  Soc Memory Map
> +**/
> +#define LS1043A_DRAM0_PHYS_ADDRESS   0x80000000
> +#define LS1043A_DRAM0_SIZE           SIZE_2GB
> +#define LS1043A_DRAM1_PHYS_ADDRESS   0x880000000
> +#define LS1043A_DRAM1_SIZE           0x780000000 // 30 GB
> +
> +#define LS1043A_CCSR_PHYS_ADDRESS    0x1000000
> +#define LS1043A_CCSR_SIZE            0xF000000
> +
> +#define LS1043A_IFC0_PHYS_ADDRESS    0x60000000
> +#define LS1043A_IFC0_SIZE            SIZE_512MB
> +#define LS1043A_IFC1_PHYS_ADDRESS    0x620000000
> +#define LS1043A_IFC1_SIZE            0xE0000000 // 3.5 GB
> +
> +#define LS1043A_QSPI_PHYS_ADDRESS    0x40000000
> +#define LS1043A_QSPI_SIZE            SIZE_512MB
> +
> +#define LS1043A_QMAN_SW_PORTAL_PHYS_ADDRESS    0x500000000
> +#define LS1043A_QMAN_SW_PORTAL_SIZE            SIZE_128MB
> +#define LS1043A_BMAN_SW_PORTAL_PHYS_ADDRESS    0x508000000
> +#define LS1043A_BMAN_SW_PORTAL_SIZE            SIZE_128MB
> +
> +#define LS1043A_PCI0_PHYS_ADDRESS    0x4000000000
> +#define LS1043A_PCI1_PHYS_ADDRESS    0x4800000000
> +#define LS1043A_PCI2_PHYS_ADDRESS    0x5000000000
> +#define LS1043A_PCI_SIZE             SIZE_32GB
> +
> +#define LS1043A_I2C0_PHYS_ADDRESS    0x2180000
> +#define LS1043A_I2C_SIZE             0x10000
> +#define LS1043A_I2C_NUM_CONTROLLERS  4
> +
> +#define LS1043A_DCFG_ADDRESS         NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS
> +
> +/**
> +  Reset Control Word (RCW) Bits
> +**/
> +#define SYS_PLL_RAT(x)  (((x) & 0x7c) >> 2) // Bits 2-6

RAT is an unknown term, what is it short for?

> +
> +typedef NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG LS1043A_DEVICE_CONFIG;
> +
> +#endif
> +
> diff --git a/Silicon/NXP/Chassis2/LS1043A/LS1043A.dec b/Silicon/NXP/Chassis2/LS1043A/LS1043A.dec
> new file mode 100644
> index 0000000000..106b118188
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/LS1043A/LS1043A.dec
> @@ -0,0 +1,23 @@
> +#/** @file
> +# NXP Layerscape processor package.
> +#
> +# Copyright 2020 NXP
> +#
> +# SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#**/
> +
> +[Defines]
> +  DEC_SPECIFICATION              = 1.27
> +  PACKAGE_VERSION                = 0.1
> +
> +################################################################################
> +#
> +# Include Section - list of Include Paths that are provided by this package.
> +#                   Comments are used for Keywords and Module Types.
> +#
> +#
> +################################################################################
> +[Includes.common]
> +  Include                        # Root include for the package

*If* we are merging Soc and Chassis again - why do we need separate
include paths?

> +
> diff --git a/Silicon/NXP/Chassis2/LS1043A/LS1043A.dsc.inc b/Silicon/NXP/Chassis2/LS1043A/LS1043A.dsc.inc
> new file mode 100644
> index 0000000000..4511203443
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/LS1043A/LS1043A.dsc.inc
> @@ -0,0 +1,30 @@
> +#  @file
> +#
> +#  Copyright 2020 NXP
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +
> +[LibraryClasses.common]
> +  SocLib|Silicon/NXP/Chassis2/LS1043A/Library/SocLib/SocLib.inf
> +
> +################################################################################
> +#
> +# Pcd Section - list of all EDK II PCD Entries defined by this Platform
> +#
> +################################################################################
> +[PcdsFeatureFlag.common]
> +  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|TRUE
> +
> +[PcdsFixedAtBuild.common]
> +## ns16550 Serial Terminal
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSerialRegisterBase|0x21c0500
> +  gArmPlatformTokenSpaceGuid.PcdCoreCount|4
> +
> +[PcdsDynamicDefault.common]
> +  #
> +  # ARM General Interrupt Controller
> +  gArmTokenSpaceGuid.PcdGicDistributorBase|0x1401000
> +  gArmTokenSpaceGuid.PcdGicInterruptInterfaceBase|0x1402000
> +
> diff --git a/Silicon/NXP/Chassis2/LS1043A/Library/SocLib/SocLib.c b/Silicon/NXP/Chassis2/LS1043A/Library/SocLib/SocLib.c
> new file mode 100644
> index 0000000000..2a08ad87db
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/LS1043A/Library/SocLib/SocLib.c
> @@ -0,0 +1,73 @@
> +/** @file
> +
> + Copyright 2017-2020 NXP
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> + **/
> +#include <Library/ChassisLib.h>
> +#include <Library/SocLib.h>
> +
> +/**
> +  Return the input clock frequency to an IP Module.
> +  This function reads the RCW bits and calculates the  PLL multipler/divider values to be applied
> +  to various IP modules.
> +  If a module is disabled or doesn't exist on platform, then return zero.
> +
> +  @param[in]  BaseClock  Base clock to which PLL multipler/divider values is to be applied.
> +  @param[in]  ClockType  IP modules whose clock value is to be retrieved
> +  @param[in]  Args       Variable Args lists that is parsed based on the ClockType
> +                         e.g. if there are multiple modules of same type then this value tells the
> +                         instance of module for which clock is to be retrieved.
> +                         (e.g. if there are four i2c controllers in SOC, then this value can be 1, 2, 3, 4)
> +                         for IP modules which have only single instance in SOC (e.g. one QSPI controller)
> +                         this value can be null (i.e. no arg)

Please wrap long lines (throughout).

> +
> +  @return > 0  Return the input clock frequency to an IP Module
> +            0  either IP module doesn't exist in SOC
> +               or IP module instance doesn't exist in SOC
> +               or IP module instance is disabled. i.e. no input clock is provided to IP module instance.
> +**/
> +UINT64
> +SocGetClock (
> +  IN  UINT64  BaseClock,
> +  IN  UINT32  ClockType,
> +  IN  VA_LIST Args
> +  )
> +{
> +  LS1043A_DEVICE_CONFIG         *Dcfg;
> +  UINT32                        RcwSr;
> +  UINT64                        ReturnValue;
> +
> +  ReturnValue = 0;
> +  Dcfg = (LS1043A_DEVICE_CONFIG  *)LS1043A_DCFG_ADDRESS;

Extra space in cast.

> +
> +  switch (ClockType) {
> +  case NXP_UART_CLOCK:
> +  case NXP_I2C_CLOCK:
> +    RcwSr = DcfgRead32 ((UINTN)&Dcfg->RcwSr[0]);
> +    ReturnValue = BaseClock * SYS_PLL_RAT (RcwSr);
> +    break;
> +  default:
> +    break;
> +  }
> +
> +  return ReturnValue;
> +}
> +
> +/**
> +  Function to initialize SoC specific constructs
> +  CPU Info
> +  SoC Personality
> +  Board Personality
> +  RCW prints
> + **/
> +VOID
> +SocInit (
> +  VOID
> +  )
> +{
> +  ChassisInit ();
> +
> +  return;
> +}
> +
> diff --git a/Silicon/NXP/Chassis2/LS1043A/Library/SocLib/SocLib.inf b/Silicon/NXP/Chassis2/LS1043A/Library/SocLib/SocLib.inf
> new file mode 100644
> index 0000000000..c9a4fbc01f
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/LS1043A/Library/SocLib/SocLib.inf
> @@ -0,0 +1,32 @@
> +#@file
> +#
> +#  Component description file for SocLib module
> +#
> +#  Copyright 2017-2020 NXP
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +
> +
> +[Defines]
> +  INF_VERSION                     = 0x0001000A
> +  BASE_NAME                       = SocLib
> +  FILE_GUID                       = 9b046753-2b4f-42d8-bfb3-468892fe17d4
> +  MODULE_TYPE                     = BASE
> +  VERSION_STRING                  = 1.0
> +  LIBRARY_CLASS                   = SocLib
> +
> +[Sources.common]
> +  SocLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  ArmPkg/ArmPkg.dec
> +  ArmPlatformPkg/ArmPlatformPkg.dec
> +  Silicon/NXP/NxpQoriqLs.dec
> +  Silicon/NXP/Chassis2/Chassis2.dec
> +  Silicon/NXP/Chassis2/LS1043A/LS1043A.dec

Please sort packages alphabetically.

> +
> +[LibraryClasses]
> +  ChassisLib
> +
> diff --git a/Silicon/NXP/Include/Library/SocLib.h b/Silicon/NXP/Include/Library/SocLib.h
> new file mode 100644
> index 0000000000..3def396171
> --- /dev/null
> +++ b/Silicon/NXP/Include/Library/SocLib.h
> @@ -0,0 +1,81 @@
> +/** @file
> +
> +  Copyright 2020 NXP
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __SOC_LIB_H__
> +#define __SOC_LIB_H__

Please drop leading __ from header guards.

> +
> +#include <Soc.h>
> +#include <Uefi.h>
> +#include <Ppi/ArmMpCoreInfo.h>
> +#include <Ppi/NxpPlatformGetClock.h>
> +
> +/**
> +  Return the number of cores present in SOC
> +
> +  This function returns the number of cores present in SOC.
> +  and also their position (cluster number and core number) in the form of ARM_CORE_INFO array
> +  and NxpCoreTable array.
> +  NxpCoreTable array can be used to find out the type of core. it's values are of type
> +  TP_ITYPE_VERSION_*.
> +  The number of cores present in SOC can vary depending on which flavour of SOC is being used.
> +  This function doesn't allocte any memory and must be provided memory for array of ARM_CORE_INFO
> +  and NxpCoreTable for maximum number of cores the SOC can have.
> +
> +  @param[out]  NxpCoreTable        array of UINT8 for maximum number of cores the SOC can have.
> +  @param[out]  ArmCoreTable        array of ARM_CORE_INFO for maximum number of cores the SOC can have.
> +  @param[in]   ArmCoreTableSize    Size of ArmCoreTable
> +
> +  @return         Actual number of cores present in SOC. After calling this function only the returned value number of
> +                      entries in ArmCoreTable are valid entries.
> +**/
> +UINTN
> +SocGetMpCoreInfo (
> +  OUT UINT8           *NxpCoreTable,
> +  OUT ARM_CORE_INFO   *ArmCoreTable,
> +  IN  UINTN           ArmCoreTableSize
> +  );
> +
> +/**
> +  Return the input clock frequency to an IP Module.
> +  This function reads the RCW bits and calculates the  PLL multipler/divider values to be applied
> +  to various IP modules.
> +  If a module is disabled or doesn't exist on platform, then return zero.
> +
> +  @param[in]  BaseClock  Base clock to which PLL multipler/divider values is to be applied.
> +  @param[in]  ClockType  IP modules whose clock value is to be retrieved
> +  @param[in]  Args       Variable Args lists that is parsed based on the ClockType
> +                         e.g. if there are multiple modules of same type then this value tells the
> +                         instance of module for which clock is to be retrieved.
> +                         (e.g. if there are four i2c controllers in SOC, then this value can be 1, 2, 3, 4)
> +                         for IP modules which have only single instance in SOC (e.g. one QSPI controller)
> +                         this value can be null (i.e. no arg)
> +
> +  @return > 0  Return the input clock frequency to an IP Module
> +            0  either IP module doesn't exist in SOC
> +               or IP module instance doesn't exist in SOC
> +               or IP module instance is disabled. i.e. no input clock is provided to IP module instance.
> +**/
> +UINT64
> +SocGetClock (
> +  IN  UINT64  BaseClock,
> +  IN  UINT32  ClockType,
> +  IN  VA_LIST Args
> +  );
> +
> +/**
> +  Function to initialize SoC specific constructs
> +  CPU Info
> +  SoC Personality
> +  Board Personality
> +  RCW prints
> + **/
> +VOID
> +SocInit (
> +  VOID
> +  );
> +
> +#endif // __SOC_LIB_H__
> diff --git a/Silicon/NXP/Include/Ppi/NxpPlatformGetClock.h b/Silicon/NXP/Include/Ppi/NxpPlatformGetClock.h
> new file mode 100644
> index 0000000000..2c8c97987d
> --- /dev/null
> +++ b/Silicon/NXP/Include/Ppi/NxpPlatformGetClock.h
> @@ -0,0 +1,47 @@
> +/** @file
> +*
> +*  Copyright 2020 NXP
> +*
> +*  SPDX-License-Identifier: BSD-2-Clause-Patent
> +*
> +**/
> +
> +#ifndef __NXP_PLATFORM_PPI_H__
> +#define __NXP_PLATFORM_PPI_H__

Please drop leading __ from header guards.

> +
> +#include <Uefi.h>
> +
> +#define NXP_PLATFORM_GET_CLOCK_PPI_GUID  \
> +  { 0x6847cc74, 0xe9ec, 0x4f8f, {0xa2, 0x9d, 0xab, 0x44, 0xe7, 0x54, 0xa8, 0xfc} }
> +
> +typedef enum _NXP_IP_CLOCK {
> +  NXP_SYSTEM_CLOCK,
> +  NXP_UART_CLOCK,
> +  NXP_I2C_CLOCK
> +} NXP_IP_CLOCK;
> +
> +/**
> +  Get the clocks supplied by Platform(Board) to NXP Layerscape SOC
> +
> +  The core can be of type ARM or PowerPC or Hardware Accelerator.
> +  If the core is enabled and of type ARM EFI_SUCCESS is returned and a code for type of ARM core is returned
> +
> +  @param[in]    ClockType  Type of clock
> +  @param[in]    ...              Variable argument list which is parsed based on ClockType
> +
> +  @return         Actual Clock Frequency. return value 0 should be interpreted as clock not provided by Board.

Funky alignment of argument/return descriptions.

/
    Leif

> +**/
> +typedef
> +UINT64
> +(EFIAPI * NXP_PLATFORM_GET_CLOCK)(
> +  IN  UINT32  ClockType,
> +  ...
> +  );
> +
> +typedef struct {
> +  NXP_PLATFORM_GET_CLOCK   PlatformGetClock;
> +} NXP_PLATFORM_GET_CLOCK_PPI;
> +
> +extern EFI_GUID gNxpPlatformGetClockPpiGuid;
> +
> +#endif
> diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> index d8989657e6..4f14cc9848 100644
> --- a/Silicon/NXP/NxpQoriqLs.dec
> +++ b/Silicon/NXP/NxpQoriqLs.dec
> @@ -14,6 +14,9 @@
>    Include
>  
>  [LibraryClasses]
> +  ##  @libraryclass  Provides Soc specific functions to other modules
> +  SocLib|Include/Library/SocLib.h
> +
>    ##  @libraryclass  Provides Chassis specific functions to other modules
>    ChassisLib|Include/Library/ChassisLib.h
>  
> -- 
> 2.17.1
>