Re: [PATCH 11/19] Silicon/NXP: Add Chassis Lib for Chassis2

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

On Fri, Feb 07, 2020 at 18:13:20 +0530, Pankaj Bansal wrote:
> Add ChassisLib for Chassis2.

What is a Chassis2?
This adds a new package, a new library class - it needs more description.

Can we expect to see Silicon/NXP/Library/SocLib/Chassis.c broken out
into its own package in future?

> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
> ---
>  Silicon/NXP/Chassis2/Chassis2.dec             |  23 +++
>  Silicon/NXP/Chassis2/Chassis2.dsc.inc         |  10 +
>  Silicon/NXP/Chassis2/Include/Chassis.h        |  42 ++++
>  .../Chassis2/Library/ChassisLib/ChassisLib.c  | 186 ++++++++++++++++++
>  .../Library/ChassisLib/ChassisLib.inf         |  41 ++++
>  Silicon/NXP/Include/Library/ChassisLib.h      |  41 ++++
>  Silicon/NXP/NxpQoriqLs.dec                    |   4 +
>  7 files changed, 347 insertions(+)
>  create mode 100644 Silicon/NXP/Chassis2/Chassis2.dec
>  create mode 100644 Silicon/NXP/Chassis2/Chassis2.dsc.inc
>  create mode 100644 Silicon/NXP/Chassis2/Include/Chassis.h
>  create mode 100644 Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
>  create mode 100644 Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
>  create mode 100644 Silicon/NXP/Include/Library/ChassisLib.h
> 
> diff --git a/Silicon/NXP/Chassis2/Chassis2.dec b/Silicon/NXP/Chassis2/Chassis2.dec
> new file mode 100644
> index 0000000000..106b118188
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Chassis2.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
> +
> diff --git a/Silicon/NXP/Chassis2/Chassis2.dsc.inc b/Silicon/NXP/Chassis2/Chassis2.dsc.inc
> new file mode 100644
> index 0000000000..db8e5a92ea
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Chassis2.dsc.inc
> @@ -0,0 +1,10 @@
> +#  @file
> +#
> +#  Copyright 2020 NXP
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +#
> +
> +[LibraryClasses.common]
> +  ChassisLib|Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> diff --git a/Silicon/NXP/Chassis2/Include/Chassis.h b/Silicon/NXP/Chassis2/Include/Chassis.h
> new file mode 100644
> index 0000000000..48ba2e7bfb
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Include/Chassis.h
> @@ -0,0 +1,42 @@
> +/** @file
> +
> +  Copyright 2020 NXP
> +
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +#ifndef __CHASSIS_H__
> +#define __CHASSIS_H__

Please drop leading __ from incude guards.

> +
> +#define  NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS  0x1EE0000
> +
> +#define  TP_CLUSTER_ITYPE_IDX  0x3f
> +#define  TP_CLUSTER_EOC        BIT31
> +#define  TP_ITYPE_AVAILABLE    BIT0
> +#define  TP_ITYPE_TYPE(x)      (((x) & 0x06) >> 1)
> +#define  TP_ITYPE_ARM          0x0
> +#define  TP_ITYPE_VERSION(x)   (((x) & 0xe0) >> 5)
> +
> +#define  TP_ITYPE_VERSION_A53   0x2
> +#define  TP_ITYPE_VERSION_A72   0x4
> +
> +/**
> +  The Device Configuration Unit provides general purpose configuration and status for the
> +  device. These registers only support 32-bit accesses.
> +**/
> +#pragma pack(1)
> +typedef struct {
> +  UINT8   Reserved0[0x100 - 0x0];
> +  UINT32  RcwSr[16]; // Reset Control Word Status Register
> +  UINT8   Reserved140[0x200 - 0x140];
> +  UINT32  ScratchRw[16]; /// Scratch Read / Write Register
> +  UINT8   Reserved240[0x740-0x240];
> +  UINT32  TpItyp[65]; /// Topology Initiator Type Register
> +  struct {
> +    UINT32  Lower;
> +    UINT32  Upper;
> +  }TpCluster[8];
> +} NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG;
> +#pragma pack()
> +
> +#endif
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> new file mode 100644
> index 0000000000..fa6a36e96f
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> @@ -0,0 +1,186 @@
> +/** @file
> +  Chassis specific functions common to all SOCs based on a specific Chessis
> +
> +  Copyright 2020 NXP
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Chassis.h>
> +#include <Uefi.h>
> +#include <Library/IoLib.h>
> +#include <Library/IoAccessLib.h>

Plese sort includes alphabetically.

> +#include <Library/PcdLib.h>
> +#include <Library/SerialPortLib.h>
> +#include <Ppi/ArmMpCoreInfo.h>
> +
> +UINT32
> +EFIAPI
> +DcfgRead32 (

OK, so after 3 years of review, I failed to spot the set I merged
doesn't actually make use of the GetMmioOperations* functions
introduced in IoAccessLib to get away from this needless code
duplication.

Ideally, I would like to see some patches that remedy this situation
in the alredy merged code.
But it certainly needs to be used for any new additions.

> +  IN  UINTN     Address
> +  )
> +{
> +  if (FeaturePcdGet (PcdDcfgBigEndian)) {
> +    return SwapMmioRead32 (Address);
> +  } else {
> +    return MmioRead32 (Address);
> +  }
> +}
> +
> +UINT32
> +EFIAPI
> +DcfgWrite32 (
> +  IN      UINTN                     Address,
> +  IN      UINT32                    Value
> +  )
> +{
> +  if (FeaturePcdGet (PcdDcfgBigEndian)) {
> +    return SwapMmioWrite32 (Address, Value);
> +  } else {
> +    return MmioWrite32 (Address, Value);
> +  }
> +}
> +
> +/**
> +  Get the type of core in cluster
> +
> +  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

Please wrap long lines.

> +
> +  @param[in]    TpItypeIdx  Index of Core to be searched in TpItyp in Device Config Registers.

TpItype or TpItyp?
Neither Tp nor Itype are known abbreviations, so need to be explained
in file comment header, unless they can be given more generically
descriptive names.

> +  @param[out]   CoreType    If the core is ARM core then the type of core i.e. A53/A72 etc.
> +                            These cores are identified based on their codes like TP_ITYPE_VERSION_A72
> +
> +  @return  EFI_NOT_FOUND   No enabled ARM core found
> +  @return  EFI_SUCCESS     An enabled ARM core found
> +**/
> +STATIC
> +EFI_STATUS
> +SocGetCoreType (
> +  IN  UINT8  TpItypeIdx,
> +  OUT UINT8  *CoreType
> +  )
> +{
> +  NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG *Dcfg;
> +  UINT32                                TpItype;
> +
> +  Dcfg = (NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG *)NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS;
> +  TpItype = MmioRead32 ((UINTN)&Dcfg->TpItyp[TpItypeIdx]);
> +  if (TpItype & TP_ITYPE_AVAILABLE) {
> +    if (TP_ITYPE_TYPE (TpItype) == TP_ITYPE_ARM) {
> +      *CoreType = TP_ITYPE_VERSION (TpItype);
> +    } else {
> +      return EFI_NOT_FOUND;
> +    }
> +  } else {
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> +  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

Please wrap long lines.

> +  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
> +__attribute__((weak))

There is nothing *wrong* about using weak symbols, but it usually done
either for some specific workarouns, or as part of a defined usage
model to override things at different levels of software.

It is not clear to me what that usage model is here - could you
document it please?

> +SocGetMpCoreInfo (
> +  OUT UINT8           *NxpCoreTable,
> +  OUT ARM_CORE_INFO   *ArmCoreTable,
> +  IN  UINTN           CoreTableSize
> +  )
> +{
> +  NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG  *Dcfg;
> +  UINT32                                 TpClusterLower;
> +  UINT8                                  TpClusterParser;

TpCusterLower/TpClusterParser are not generic terms - could they be
documented or renamed more generically.

> +  UINT8                                  ClusterIndex;
> +  UINT8                                  CoreIndex;
> +  UINTN                                  CoreCount;
> +  UINT8                                  CoreType;
> +  EFI_STATUS                             Status;
> +
> +  Dcfg = (NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG *)NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS;
> +  ClusterIndex = 0;
> +  CoreCount = 0;
> +  while (TRUE) {
> +    TpClusterLower = MmioRead32 ((UINTN)&Dcfg->TpCluster[ClusterIndex].Lower);
> +    for (CoreIndex = 0; CoreIndex < (sizeof (TpClusterLower) / sizeof (TpClusterParser)); CoreIndex++) {

I don't see a value in calculating sizeof(UINT32)/sizeof(UINT8).
A strategically placed #define would be more clear (and shorten the
line length).

> +      TpClusterParser = (TpClusterLower >> (8 * CoreIndex));

And I would prefer to see that 8 replaced by another #define.

> +      Status = SocGetCoreType (TpClusterParser & TP_CLUSTER_ITYPE_IDX, &CoreType);
> +      if (Status != EFI_NOT_FOUND) {
> +        ArmCoreTable[CoreCount].ClusterId = ClusterIndex;
> +        ArmCoreTable[CoreCount].CoreId = CoreIndex;
> +        ArmCoreTable[CoreCount].MailboxSetAddress = 0;
> +        ArmCoreTable[CoreCount].MailboxGetAddress = 0;
> +        ArmCoreTable[CoreCount].MailboxClearAddress = 0;
> +        ArmCoreTable[CoreCount].MailboxClearValue = ~0;
> +
> +        NxpCoreTable[CoreCount] = CoreType;
> +        CoreCount++;
> +        if (CoreCount == CoreTableSize) {
> +          break;
> +        }
> +      }
> +    }
> +    if (TpClusterLower & TP_CLUSTER_EOC) {
> +      break;
> +    }
> +    if (CoreCount == CoreTableSize) {
> +      break;
> +    }
> +    ClusterIndex++;
> +  }
> +
> +  return CoreCount;
> +}
> +
> +/**
> +  Function to initialize Chassis Specific functions
> + **/
> +VOID
> +ChassisInit (
> +  VOID
> +  )
> +{
> +  UINT64              BaudRate;
> +  UINT32              ReceiveFifoDepth;
> +  EFI_PARITY_TYPE     Parity;
> +  UINT8               DataBits;
> +  EFI_STOP_BITS_TYPE  StopBits;
> +  UINT32              Timeout;
> +
> +  BaudRate = FixedPcdGet64 (PcdUartDefaultBaudRate);
> +  ReceiveFifoDepth = FixedPcdGet16 (PcdUartDefaultReceiveFifoDepth);  // Use default FIFO depth
> +  Timeout = 0;
> +  Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
> +  DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
> +  StopBits = (EFI_STOP_BITS_TYPE) FixedPcdGet8 (PcdUartDefaultStopBits);
> +
> +  //
> +  // Early init serial Port to get board information.
> +  //
> +  SerialPortSetAttributes (
> +    &BaudRate,
> +    &ReceiveFifoDepth,
> +    &Timeout,
> +    &Parity,
> +    &DataBits,
> +    &StopBits
> +  );
> +}
> diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> new file mode 100644
> index 0000000000..4964bb4e82
> --- /dev/null
> +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> @@ -0,0 +1,41 @@
> +#/**  @file
> +#
> +#  Copyright 2020 NXP
> +#
> +#  SPDX-License-Identifier: BSD-2-Clause
> +#
> +#**/
> +
> +[Defines]
> +  INF_VERSION                    = 0x0001001A
> +  BASE_NAME                      = Chassis2Lib
> +  FILE_GUID                      = fae0d077-5fc2-494f-b8e1-c51a3023ee3e
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = ChassisLib
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  ArmPkg/ArmPkg.dec
> +  Silicon/NXP/NxpQoriqLs.dec
> +  Silicon/NXP/Chassis2/Chassis2.dec

Please sort packages alphabetically.

> +
> +[LibraryClasses]
> +  IoLib
> +  IoAccessLib

Please sort library classes alphabetically.

> +  PcdLib
> +  SerialPortLib
> +
> +[Sources.common]
> +  ChassisLib.c
> +
> +[FeaturePcd]
> +  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
> +
> +[FixedPcd]
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
> +  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultReceiveFifoDepth

Please sort Pcds alphabetically (i.e. swap final two lines).

> +
> diff --git a/Silicon/NXP/Include/Library/ChassisLib.h b/Silicon/NXP/Include/Library/ChassisLib.h
> new file mode 100644
> index 0000000000..b51b024374
> --- /dev/null
> +++ b/Silicon/NXP/Include/Library/ChassisLib.h
> @@ -0,0 +1,41 @@
> +/** @file
> +  I2c Lib to control I2c controller.
> +
> +  Copyright 2020 NXP
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#ifndef __CHASSIS_LIB_H__
> +#define __CHASSIS_LIB_H__

Please drop leading __ from include guards.

> +
> +#include <Chassis.h>

Include files should only include those files needed for their own
definitions.

> +
> +/**
> +  Read Dcfg register
> +**/
> +UINT32
> +EFIAPI
> +DcfgRead32 (
> +  IN  UINTN     Address
> +  );
> +
> +/**
> +  Write Dcfg register
> +**/
> +UINT32
> +EFIAPI
> +DcfgWrite32 (
> +  IN      UINTN                     Address,
> +  IN      UINT32                    Value
> +  );
> +

The above two prototypes can be deleted.

> +/**
> +  Function to initialize Chassis Specific functions
> + **/
> +VOID
> +ChassisInit (
> +  VOID
> +  );
> +
> +#endif
> diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> index b478560450..d8989657e6 100644
> --- a/Silicon/NXP/NxpQoriqLs.dec
> +++ b/Silicon/NXP/NxpQoriqLs.dec
> @@ -14,6 +14,9 @@
>    Include
>  
>  [LibraryClasses]
> +  ##  @libraryclass  Provides Chassis specific functions to other modules
> +  ChassisLib|Include/Library/ChassisLib.h
> +
>    ##  @libraryclass  Provides services to read/write to I2c devices
>    I2cLib|Include/Library/I2cLib.h
>  
> @@ -34,4 +37,5 @@
>  
>  [PcdsFeatureFlag]
>    gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x00000315
> +  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|FALSE|BOOLEAN|0x00000316

I would prefer for this flag to be added with the other *BigEndian
flags in the same file.

Looping back to a previous patch - looking at the Pcd tokens used, it
would be more convenient if PcdI2cErratumA009203 was kept as part of a
separate group and token 0x315 was given to PcdDcfgBigEndian.

/
    Leif

>  
> -- 
> 2.17.1
>