Re: [PATCH v2 18/28] Silicon/NXP: Add Chassis2 Package

["Pankaj Bansal" <pankaj.bansal@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm <leif@nuviainc.com>
> Sent: Wednesday, April 1, 2020 7:48 PM
> To: Pankaj Bansal (OSS) <pankaj.bansal@oss.nxp.com>
> Cc: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>; Michael D Kinney
> <michael.d.kinney@intel.com>; devel@edk2.groups.io; Varun Sethi
> <V.Sethi@nxp.com>; Samer El-Haj-Mahmoud <Samer.El-Haj-
> Mahmoud@arm.com>; Jon Nettleton <jon@solid-run.com>
> Subject: Re: [PATCH v2 18/28] Silicon/NXP: Add Chassis2 Package
> 
> On Fri, Mar 20, 2020 at 20:05:33 +0530, Pankaj Bansal wrote:
> > From: Pankaj Bansal <pankaj.bansal@nxp.com>
> >
> > A Chassis is a base framework used for building SoCs.
> > We can think of Chassis/Soc/Platform(a.k.a Borad) in Oops terms.
> 
>                                              Board?
> "Oops" is not a term I am familiar with other than as an exclamation -
> can you elaborate please?

Oops: Object-Oriented Programming System
Basically concept of base classes and derived classes.

> 
> > Chassis is base. Soc is based on some Chassis.
> > Platform is based on some Soc.
> >
> > SOCs that are designed around same chassis, reuse most of the components.
> >
> > Therefore, add the package for Chassis2. LS1043A and LS1046A SOCs belong
> > to Chassis2.
> >
> > 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        | 34 +++++++
> >  .../Chassis2/Library/ChassisLib/ChassisLib.c  | 97 +++++++++++++++++++
> >  .../Library/ChassisLib/ChassisLib.inf         | 34 +++++++
> >  Silicon/NXP/Include/Library/ChassisLib.h      | 51 ++++++++++
> >  Silicon/NXP/NxpQoriqLs.dec                    |  4 +
> >  7 files changed, 253 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 000000000000..a0048bd784ea
> > --- /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 000000000000..db8e5a92eacb
> > --- /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 000000000000..72bd97efd004
> > --- /dev/null
> > +++ b/Silicon/NXP/Chassis2/Include/Chassis.h
> > @@ -0,0 +1,34 @@
> > +/** @file
> > +
> > +  Copyright 2020 NXP
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +
> > +**/
> > +#ifndef CHASSIS_H__
> > +#define CHASSIS_H__
> > +
> > +#define  NXP_LAYERSCAPE_CHASSIS2_DCFG_ADDRESS  0x1EE0000
> > +
> > +/* SMMU Defintions */
> > +#define SMMU_BASE_ADDR             0x09000000
> > +#define SMMU_REG_SCR0              (SMMU_BASE_ADDR + 0x0)
> > +#define SMMU_REG_SACR              (SMMU_BASE_ADDR + 0x10)
> > +#define SMMU_REG_NSCR0             (SMMU_BASE_ADDR + 0x400)
> > +
> > +#define SCR0_USFCFG_MASK           0x00000400
> > +#define SCR0_CLIENTPD_MASK         0x00000001
> > +#define SACR_PAGESIZE_MASK         0x00010000
> > +
> > +/**
> > +  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
> 
> If this is the formal word as used in official documentation, it still
> needs listing in a glossary in the file comment header.

This is register in SOC.
I thought the explanation in structure definition is fine for registers.
In https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/6_documenting_software/64_what_you_must_comment.html
6.4.4 Comment data structure declarations and #define statements.
The structure data members won't be used standalone in code without including the structure definition.
So any abbreviation can be referred from structure definition right ?
Also I referred https://edk2-docs.gitbooks.io/edk-ii-c-coding-standards-specification/content/v/release/2.20/appendix_a_common_examples.html
Type declarations.

/// Structure without forward reference.
typedef struct {
  UINT32                   Signature;  ///< Signature description.
  EFI_HANDLE               Handle;     ///< Handle description.
  EFI_PROD_PROT1_PROTOCOL  ProdProt1;  ///< ProdProt1 description.
  EFI_PROD_PROT2_PROTOCOL  ProdProt2;  ///< ProdProt2 description.
} DRIVER_NAME_INSTANCE;

> 
> > +} NXP_LAYERSCAPE_CHASSIS2_DEVICE_CONFIG;
> > +#pragma pack()
> > +
> > +#endif // CHASSIS_H__
> > diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> > new file mode 100644
> > index 000000000000..816e0fa29c4a
> > --- /dev/null
> > +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.c
> > @@ -0,0 +1,97 @@
> > +/** @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>
> 
> Can you flip the two above lines around to get them in order?

OK

> 
> > +#include <Library/PcdLib.h>
> > +#include <Library/SerialPortLib.h>
> > +
> > +/**
> > +  Read Dcfg register
> > +
> > +  @param  Address The MMIO register to read.
> > +
> > +  @return The value read.
> > +**/
> > +UINT32
> > +EFIAPI
> > +DcfgRead32 (
> > +  IN  UINTN     Address
> > +  )
> > +{
> 
> Please replace these with the GetMmioOperations call, as designed.
> 
> In fact, please provide an additional patch deleting all of the direct
> SwapMmio* functions from IoAccessLib.h and changing all of the
> SwapMmio* function definitions in IoAccessLib.c to STATIC to prevent
> future accidental direct use.

ok

> 
> /
>     Leif
> 
> > +  if (FeaturePcdGet (PcdDcfgBigEndian)) {
> > +    return SwapMmioRead32 (Address);
> > +  } else {
> > +    return MmioRead32 (Address);
> > +  }
> > +}
> > +
> > +/**
> > +  Write Dcfg register
> > +
> > +  @param  Address The MMIO register to write.
> > +  @param  Value   The value to write to the MMIO register.
> > +
> > +  @return Value.
> > +**/
> > +UINT32
> > +EFIAPI
> > +DcfgWrite32 (
> > +  IN      UINTN                     Address,
> > +  IN      UINT32                    Value
> > +  )
> > +{
> > +  if (FeaturePcdGet (PcdDcfgBigEndian)) {
> > +    return SwapMmioWrite32 (Address, Value);
> > +  } else {
> > +    return MmioWrite32 (Address, Value);
> > +  }
> > +}
> > +
> > +/*
> > + * Setup SMMU in bypass mode
> > + * and also set its pagesize
> > + */
> > +STATIC
> > +VOID
> > +SmmuInit (
> > +  VOID
> > +  )
> > +{
> > +  UINT32 Value;
> > +
> > +  /* set pagesize as 64K and ssmu-500 in bypass mode */
> > +  Value = (MmioRead32 ((UINTN)SMMU_REG_SACR) |
> SACR_PAGESIZE_MASK);
> > +  MmioWrite32 ((UINTN)SMMU_REG_SACR, Value);
> > +
> > +  Value = (MmioRead32 ((UINTN)SMMU_REG_SCR0) |
> SCR0_CLIENTPD_MASK);
> > +  Value &= ~SCR0_USFCFG_MASK;
> > +  MmioWrite32 ((UINTN)SMMU_REG_SCR0, Value);
> > +
> > +  Value = (MmioRead32 ((UINTN)SMMU_REG_NSCR0) |
> SCR0_CLIENTPD_MASK);
> > +  Value &= ~SCR0_USFCFG_MASK;
> > +  MmioWrite32 ((UINTN)SMMU_REG_NSCR0, Value);
> > +}
> > +
> > +/**
> > +  Function to initialize Chassis Specific functions
> > + **/
> > +VOID
> > +ChassisInit (
> > +  VOID
> > +  )
> > +{
> > +  //
> > +  // Early init serial Port to get board information.
> > +  //
> > +  SerialPortInitialize ();
> > +
> > +  SmmuInit ();
> > +}
> > diff --git a/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> > new file mode 100644
> > index 000000000000..2bb16af53134
> > --- /dev/null
> > +++ b/Silicon/NXP/Chassis2/Library/ChassisLib/ChassisLib.inf
> > @@ -0,0 +1,34 @@
> > +#  @file
> > +#
> > +#  Copyright 2020 NXP
> > +#
> > +#  SPDX-License-Identifier: BSD-2-Clause
> > +#
> > +#
> > +
> > +[Defines]
> > +  INF_VERSION                    = 1.27
> > +  BASE_NAME                      = Chassis2Lib
> > +  FILE_GUID                      = fae0d077-5fc2-494f-b8e1-c51a3023ee3e
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = ChassisLib
> > +
> > +[Packages]
> > +  ArmPkg/ArmPkg.dec
> > +  MdePkg/MdePkg.dec
> > +  Silicon/NXP/Chassis2/Chassis2.dec
> > +  Silicon/NXP/NxpQoriqLs.dec
> > +
> > +[LibraryClasses]
> > +  IoAccessLib
> > +  IoLib
> > +  PcdLib
> > +  SerialPortLib
> > +
> > +[Sources.common]
> > +  ChassisLib.c
> > +
> > +[FeaturePcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian
> > +
> > diff --git a/Silicon/NXP/Include/Library/ChassisLib.h
> b/Silicon/NXP/Include/Library/ChassisLib.h
> > new file mode 100644
> > index 000000000000..89992a4b6fd5
> > --- /dev/null
> > +++ b/Silicon/NXP/Include/Library/ChassisLib.h
> > @@ -0,0 +1,51 @@
> > +/** @file
> > +  Chassis Lib to provide Chessis specific functionality to all SOCs in
> > +  a Chassis.
> > +
> > +  Copyright 2020 NXP
> > +
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +**/
> > +
> > +#ifndef CHASSIS_LIB_H__
> > +#define CHASSIS_LIB_H__
> > +
> > +#include <Chassis.h>
> > +
> > +/**
> > +  Read Dcfg register
> > +
> > +  @param  Address The MMIO register to read.
> > +
> > +  @return The value read.
> > +**/
> > +UINT32
> > +EFIAPI
> > +DcfgRead32 (
> > +  IN  UINTN     Address
> > +  );
> > +
> > +/**
> > +  Write Dcfg register
> > +
> > +  @param  Address The MMIO register to write.
> > +  @param  Value   The value to write to the MMIO register.
> > +
> > +  @return Value.
> > +**/
> > +UINT32
> > +EFIAPI
> > +DcfgWrite32 (
> > +  IN      UINTN                     Address,
> > +  IN      UINT32                    Value
> > +  );
> > +
> > +/**
> > +  Function to initialize Chassis Specific functions
> > + **/
> > +VOID
> > +ChassisInit (
> > +  VOID
> > +  );
> > +
> > +#endif // CHASSIS_LIB_H__
> > diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> > index 2ac047a89274..3e79f502c127 100644
> > --- a/Silicon/NXP/NxpQoriqLs.dec
> > +++ b/Silicon/NXP/NxpQoriqLs.dec
> > @@ -14,6 +14,9 @@ [Includes]
> >    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
> >
> > @@ -29,4 +32,5 @@ [PcdsFixedAtBuild.common]
> >
> >  [PcdsFeatureFlag]
> >
> gNxpQoriqLsTokenSpaceGuid.PcdI2cErratumA009203|FALSE|BOOLEAN|0x0000
> 0315
> > +
> gNxpQoriqLsTokenSpaceGuid.PcdDcfgBigEndian|FALSE|BOOLEAN|0x00000316
> >
> > --
> > 2.17.1
> >