Re: [PATCH edk2-platforms 13/41] Silicon/NXP : Add support of IfcLib

[Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>]

> -----Original Message-----
> From: Leif Lindholm <leif.lindholm@linaro.org>
> Sent: Wednesday, December 19, 2018 6:56 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: ard.biesheuvel@linaro.org; michael.d.kinney@intel.com; edk2-
> devel@lists.01.org; Udit Kumar <udit.kumar@nxp.com>; Varun Sethi
> <V.Sethi@nxp.com>
> Subject: Re: [PATCH edk2-platforms 13/41] Silicon/NXP : Add support of IfcLib
> 
> On Wed, Nov 28, 2018 at 08:31:27PM +0530, Meenakshi Aggarwal wrote:
> > Add support of IfcLib, it will be used to perform any operation on IFC
> > controller.
> 
> Expand acronym.
> 
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> > ---
> >  Silicon/NXP/Include/Library/IfcLib.h  |  26 +++++
> >  Silicon/NXP/Library/IfcLib/IfcLib.c   | 150 +++++++++++++++++++++++++++
> >  Silicon/NXP/Library/IfcLib/IfcLib.h   | 190
> ++++++++++++++++++++++++++++++++++
> >  Silicon/NXP/Library/IfcLib/IfcLib.inf |  38 +++++++
> 
> Names Ifc -> NxpIfc please.
> 
Is this renaming really needed for IfcLib, I can change the guard and header file name.

Please suggest?

> >  Silicon/NXP/NxpQoriqLs.dec            |   1 +
> >  5 files changed, 405 insertions(+)
> >  create mode 100644 Silicon/NXP/Include/Library/IfcLib.h
> >  create mode 100644 Silicon/NXP/Library/IfcLib/IfcLib.c
> >  create mode 100644 Silicon/NXP/Library/IfcLib/IfcLib.h
> >  create mode 100644 Silicon/NXP/Library/IfcLib/IfcLib.inf
> >
> > diff --git a/Silicon/NXP/Include/Library/IfcLib.h
> > b/Silicon/NXP/Include/Library/IfcLib.h
> > new file mode 100644
> > index 0000000..8d2c151
> > --- /dev/null
> > +++ b/Silicon/NXP/Include/Library/IfcLib.h
> > @@ -0,0 +1,26 @@
> > +/** @IfcLib.h
> > +
> > +  The integrated flash controller (IFC) is used to interface with
> > + external asynchronous  NAND flash, asynchronous NOR flash, SRAM, generic
> ASIC memories and EPROM.
> > +
> > +  Copyright 2018 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
> > +
> > + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
> > + ensource.org%2Flicenses%2Fbsd-
> license.php&amp;data=02%7C01%7Cmeenaks
> > +
> hi.aggarwal%40nxp.com%7C62c0401c65ac481e650508d665b57ea9%7C686ea1
> d3b
> > +
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C636808227519182943&amp;sdata=4
> UMgN
> > + 7laz86jDwTlvJrHWkqdum8qmZPGWrhESvjHBMQ%3D&amp;reserved=0
> > +
> > +  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 __IFC_LIB_H__
> > +#define __IFC_LIB_H__
> 
> Header guard NXP_ (and/or QORIQ_) prefix.
> 
> > +
> > +VOID
> > +IfcInit (
> > +  VOID
> > +  );
> > +
> > +#endif //__IFC_LIB_H__
> > diff --git a/Silicon/NXP/Library/IfcLib/IfcLib.c
> > b/Silicon/NXP/Library/IfcLib/IfcLib.c
> > new file mode 100644
> > index 0000000..8cf02ae
> > --- /dev/null
> > +++ b/Silicon/NXP/Library/IfcLib/IfcLib.c
> > @@ -0,0 +1,150 @@
> > +/** @IfcLib.c
> > +
> > +  The integrated flash controller (IFC) is used to interface with
> > + external asynchronous/  synchronous NAND flash, asynchronous NOR
> > + flash, SRAM, generic ASIC memory and  EPROM.
> > +  It has eight chip-selects, to which a maximum of eight flash
> > + devices can be attached,  although only one of these can be accessed at any
> given time.
> > +
> > +  Copyright 2018 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
> > +
> > + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
> > + ensource.org%2Flicenses%2Fbsd-
> license.php&amp;data=02%7C01%7Cmeenaks
> > +
> hi.aggarwal%40nxp.com%7C62c0401c65ac481e650508d665b57ea9%7C686ea1
> d3b
> > +
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C636808227519182943&amp;sdata=4
> UMgN
> > + 7laz86jDwTlvJrHWkqdum8qmZPGWrhESvjHBMQ%3D&amp;reserved=0
> > +
> > +  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 <Library/IoAccessLib.h>
> > +#include "IfcLib.h"
> > +
> > +STATIC MMIO_OPERATIONS_32 *mMmioOps;
> > +
> > +STATIC UINT8 mNandCS;
> > +STATIC UINT8 mNorCS;
> > +STATIC UINT8 mFpgaCS;
> > +
> > +VOID
> 
> Local only?
> If so, STATIC please.
> 
> > +SetTimings (
> > +  IN  UINT8        CS,
> > +  IN  IFC_TIMINGS  IfcTimings
> > +  )
> > +{
> > +  IFC_REGS*        IfcRegs;
> > +
> > +  IfcRegs = (IFC_REGS*)PcdGet64 (PcdIfcBaseAddr);
> > +
> > +  // Configure Extended chip select property registers
> > + mMmioOps->Write ((UINTN)&IfcRegs->CsprCs[CS].CsprExt,
> > + IfcTimings.CsprExt);
> > +
> > +  // Configure Fpga timing registers
> > +  mMmioOps->Write ((UINTN)&IfcRegs->FtimCs[CS].Ftim[IFC_FTIM0],
> > + IfcTimings.Ftim[0]);  mMmioOps->Write
> > + ((UINTN)&IfcRegs->FtimCs[CS].Ftim[IFC_FTIM1], IfcTimings.Ftim[1]);
> > + mMmioOps->Write ((UINTN)&IfcRegs->FtimCs[CS].Ftim[IFC_FTIM2],
> > + IfcTimings.Ftim[2]);  mMmioOps->Write
> > + ((UINTN)&IfcRegs->FtimCs[CS].Ftim[IFC_FTIM3], IfcTimings.Ftim[3]);
> > +
> > +  // Configure chip select option registers  mMmioOps->Write
> > + ((UINTN)&IfcRegs->CsprCs[CS].Cspr, IfcTimings.Cspr);
> > +
> > +  // Configure address mask registers  mMmioOps->Write
> > + ((UINTN)&IfcRegs->AmaskCs[CS].Amask, IfcTimings.Amask);
> > +
> > +  // Configure chip select property registers  mMmioOps->Write
> > + ((UINTN)&IfcRegs->CsorCs[CS].Csor, IfcTimings.Csor);
> > +
> > +  return;
> > +}
> > +
> > +VOID
> 
> Local only?
> If so, STATIC please.
> 
> > +NandInit(
> > +  VOID
> > +  )
> > +{
> > +  IFC_REGS*       IfcRegs;
> > +  IFC_TIMINGS     NandIfcTimings;
> > +
> > +  IfcRegs = (IFC_REGS*)PcdGet64 (PcdIfcBaseAddr);
> > +
> > +  // Get Nand Flash Timings
> > +  GetIfcNandFlashTimings (&NandIfcTimings);
> > +
> > +  // Validate chip select
> > +  if (NandIfcTimings.CS < IFC_CS_MAX) {
> > +    mNandCS = NandIfcTimings.CS;
> > +
> > +    // clear event registers
> > +    mMmioOps->Write ((UINTN)&IfcRegs->IfcNand.PgrdcmplEvtStat, ~0U);
> > +
> > +    mMmioOps->Write ((UINTN)&IfcRegs->IfcNand.NandEvterStat, ~0U);
> > +
> > +    // Enable error and event for any detected errors
> > +    mMmioOps->Write ((UINTN)&IfcRegs->IfcNand.NandEvterEn,
> > +      IFC_NAND_EVTER_EN_OPC_EN |
> 
> Indentation should be to function name, not struct name.
> (Please address throughout.)
> 
> > +      IFC_NAND_EVTER_EN_PGRDCMPL_EN |
> > +      IFC_NAND_EVTER_EN_FTOER_EN |
> > +      IFC_NAND_EVTER_EN_WPER_EN);
> > +    mMmioOps->Write ((UINTN)&IfcRegs->IfcNand.Ncfgr, 0x0);
> > +
> > +    SetTimings (mNandCS, NandIfcTimings);  }
> > +
> > +  return;
> > +}
> > +
> > +VOID
> 
> Local only?
> If so, STATIC please.
> 
> > +FpgaInit (
> > +  VOID
> > +  )
> > +{
> > +  IFC_TIMINGS     FpgaIfcTimings;
> > +
> > +  // Get Fpga Flash Timings
> > +  GetIfcFpgaTimings (&FpgaIfcTimings);
> > +
> > +  // Validate chip select
> > +  if (FpgaIfcTimings.CS < IFC_CS_MAX) {
> > +    mFpgaCS = FpgaIfcTimings.CS;
> > +    SetTimings (mFpgaCS, FpgaIfcTimings);  }
> > +
> > +  return;
> > +}
> > +
> > +VOID
> > +NorInit (
> > +  VOID
> > +  )
> > +{
> > +  IFC_TIMINGS     NorIfcTimings;
> > +
> > +  // Get NOR Flash Timings
> > +  GetIfcNorFlashTimings (&NorIfcTimings);
> > +
> > +  // Validate chip select
> > +  if (NorIfcTimings.CS < IFC_CS_MAX) {
> > +    mNorCS = NorIfcTimings.CS;
> > +    SetTimings (mNorCS, NorIfcTimings);  }
> > +
> > +  return;
> > +}
> > +
> > +//
> > +// IFC has NOR , NAND and FPGA
> > +//
> > +VOID
> > +IfcInit (
> > +  VOID
> > +  )
> > +{
> > +  mMmioOps = GetMmioOperations32 (FixedPcdGetBool (PcdIfcBigEndian));
> > +
> > +  NorInit();
> > +  NandInit();
> > +  FpgaInit();
> > +
> > +  return;
> > +}
> > diff --git a/Silicon/NXP/Library/IfcLib/IfcLib.h
> > b/Silicon/NXP/Library/IfcLib/IfcLib.h
> > new file mode 100644
> > index 0000000..38ce247
> > --- /dev/null
> > +++ b/Silicon/NXP/Library/IfcLib/IfcLib.h
> > @@ -0,0 +1,190 @@
> > +/** @IfcLib.h
> > +
> > +  The integrated flash controller (IFC) is used to interface with
> > + external asynchronous/  synchronous NAND flash, asynchronous NOR
> > + flash, SRAM, generic ASIC memory and  EPROM.
> > +  It has eight chip-selects, to which a maximum of eight flash
> > + devices can be attached,  although only one of these can be accessed at any
> given time.
> > +
> > +  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
> > +
> > + https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fop
> > + ensource.org%2Flicenses%2Fbsd-
> license.php&amp;data=02%7C01%7Cmeenaks
> > +
> hi.aggarwal%40nxp.com%7C62c0401c65ac481e650508d665b57ea9%7C686ea1
> d3b
> > +
> c2b4c6fa92cd99c5c301635%7C0%7C0%7C636808227519182943&amp;sdata=4
> UMgN
> > + 7laz86jDwTlvJrHWkqdum8qmZPGWrhESvjHBMQ%3D&amp;reserved=0
> > +
> > +  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 __IFC_LIB_H__
> > +#define __IFC_LIB_H__
> 
> NXP_ and/or QORIQ_ prefix?
> 
> > +
> > +#include <Ifc.h>
> > +#include <Uefi.h>
> > +
> > +#define IFC_NAND_RESERVED_SIZE      FixedPcdGet32
> (PcdIfcNandReservedSize)
> > +
> > +typedef enum {
> > +  IFC_FTIM0 = 0,
> > +  IFC_FTIM1,
> > +  IFC_FTIM2,
> > +  IFC_FTIM3,
> 
> CamelCase member names please, throughout.
> 
> > +} IFC_FTIMS;
> > +
> > +typedef struct {
> > +  UINT32 CsprExt;
> > +  UINT32 Cspr;
> > +  UINT32 Res;
> > +} IFC_CSPR;
> > +
> > +typedef struct {
> > +  UINT32 Amask;
> 
> AddressMask?
> 
> > +  UINT32 Res[0x2];
> 
> Is this "Reserved"?
> If so, please write out in full.
> Also please drop the hex prefix.
> Apples throughout.
> 
> > +} IFC_AMASK;
> > +
> > +typedef struct {
> > +  UINT32 Csor;
> > +  UINT32 CsorExt;
> > +  UINT32 Res;
> > +} IFC_CSOR;
> > +
> > +typedef struct {
> > +  UINT32 Ftim[4];
> > +  UINT32 Res[0x8];
> > +}IFC_FTIM ;
> > +
> > +typedef struct {
> > +  UINT32 Ncfgr;
> > +  UINT32 Res1[0x4];
> > +  UINT32 NandFcr0;
> > +  UINT32 NandFcr1;
> > +  UINT32 Res2[0x8];
> > +  UINT32 Row0;
> > +  UINT32 Res3;
> > +  UINT32 Col0;
> > +  UINT32 Res4;
> > +  UINT32 Row1;
> > +  UINT32 Res5;
> > +  UINT32 Col1;
> > +  UINT32 Res6;
> > +  UINT32 Row2;
> > +  UINT32 Res7;
> > +  UINT32 Col2;
> > +  UINT32 Res8;
> > +  UINT32 Row3;
> > +  UINT32 Res9;
> > +  UINT32 Col3;
> > +  UINT32 Res10[0x24];
> > +  UINT32 NandFbcr;
> > +  UINT32 Res11;
> > +  UINT32 NandFir0;
> > +  UINT32 NandFir1;
> > +  UINT32 nandFir2;
> > +  UINT32 Res12[0x10];
> > +  UINT32 NandCsel;
> > +  UINT32 Res13;
> > +  UINT32 NandSeqStrt;
> > +  UINT32 Res14;
> > +  UINT32 NandEvterStat;
> > +  UINT32 Res15;
> > +  UINT32 PgrdcmplEvtStat;
> > +  UINT32 Res16[0x2];
> > +  UINT32 NandEvterEn;
> > +  UINT32 Res17[0x2];
> > +  UINT32 NandEvterIntrEn;
> > +  UINT32 Res18[0x2];
> > +  UINT32 NandErattr0;
> > +  UINT32 NandErattr1;
> > +  UINT32 Res19[0x10];
> > +  UINT32 NandFsr;
> > +  UINT32 Res20;
> > +  UINT32 NandEccstat[4];
> > +  UINT32 Res21[0x20];
> > +  UINT32 NanNdcr;
> > +  UINT32 Res22[0x2];
> > +  UINT32 NandAutobootTrgr;
> > +  UINT32 Res23;
> > +  UINT32 NandMdr;
> > +  UINT32 Res24[0x5C];
> > +} IFC_NAND;
> > +
> > +/*
> > + * IFC controller NOR Machine registers  */ typedef struct {
> > +  UINT32 NorEvterStat;
> > +  UINT32 Res1[0x2];
> > +  UINT32 NorEvterEn;
> > +  UINT32 Res2[0x2];
> > +  UINT32 NorEvterIntrEn;
> > +  UINT32 Res3[0x2];
> > +  UINT32 NorErattr0;
> > +  UINT32 NorErattr1;
> > +  UINT32 NorErattr2;
> > +  UINT32 Res4[0x4];
> > +  UINT32 NorCr;
> > +  UINT32 Res5[0xEF];
> > +} IFC_NOR;
> > +
> > +/*
> > + * IFC controller GPCM Machine registers
> > + */
> > +typedef struct  {
> 
> extra space
> 
> > +  UINT32 GpcmEvterStat;
> > +  UINT32 Res1[0x2];
> > +  UINT32 GpcmEvterEn;
> > +  UINT32 Res2[0x2];
> > +  UINT32 gpcmEvterIntrEn;
> > +  UINT32 Res3[0x2];
> > +  UINT32 GpcmErattr0;
> > +  UINT32 GpcmErattr1;
> > +  UINT32 GcmErattr2;
> > +  UINT32 GpcmStat;
> > +} IFC_GPCM;
> > +
> > +/*
> > + * IFC Controller Registers
> > + */
> > +typedef struct {
> > +  UINT32      IfcRev;
> > +  UINT32      Res1[0x2];
> > +  IFC_CSPR    CsprCs[IFC_BANK_COUNT];
> > +  UINT8       Res2[IFC_CSPR_REG_LEN - IFC_CSPR_USED_LEN];
> > +  IFC_AMASK   AmaskCs[IFC_BANK_COUNT];
> > +  UINT8       Res3[IFC_AMASK_REG_LEN - IFC_AMASK_USED_LEN];
> > +  IFC_CSOR    CsorCs[IFC_BANK_COUNT];
> > +  UINT8       Res4[IFC_CSOR_REG_LEN - IFC_CSOR_USED_LEN];
> > +  IFC_FTIM    FtimCs[IFC_BANK_COUNT];
> > +  UINT8       Res5[IFC_FTIM_REG_LEN - IFC_FTIM_USED_LEN];
> > +  UINT32      RbStat;
> > +  UINT32      RbMap;
> > +  UINT32      WpMap;
> > +  UINT32      IfcGcr;
> > +  UINT32      Res7[0x2];
> > +  UINT32      CmEvter_stat;
> > +  UINT32      Res8[0x2];
> > +  UINT32      CmEvterEn;
> > +  UINT32      Res9[0x2];
> > +  UINT32      CmEvterIntrEn;
> > +  UINT32      Res10[0x2];
> > +  UINT32      CmErattr0;
> > +  UINT32      CmErattr1;
> > +  UINT32      Res11[0x2];
> > +  UINT32      IfcCcr;
> > +  UINT32      IfcCsr;
> > +  UINT32      DdrCcrLow;
> > +  UINT32      Res12[IFC_NAND_RESERVED_SIZE];
> > +  IFC_NAND    IfcNand;
> > +  IFC_NOR     IfcNor;
> > +  IFC_GPCM    IfcGpcm;
> > +} IFC_REGS;
> > +
> > +extern VOID GetIfcNorFlashTimings (IFC_TIMINGS * NorIfcTimings);
> > +
> > +extern VOID GetIfcFpgaTimings (IFC_TIMINGS  *FpgaIfcTimings);
> > +
> > +extern VOID GetIfcNandFlashTimings (IFC_TIMINGS * NandIfcTimings);
> 
> Please move these function declarations to the (first) patch that adds
> implementations of these functions.
> 
> > +
> > +#endif //__IFC_LIB_H__
> > diff --git a/Silicon/NXP/Library/IfcLib/IfcLib.inf
> b/Silicon/NXP/Library/IfcLib/IfcLib.inf
> > new file mode 100644
> > index 0000000..989eb44
> > --- /dev/null
> > +++ b/Silicon/NXP/Library/IfcLib/IfcLib.inf
> > @@ -0,0 +1,38 @@
> > +#  IfcLib.inf
> > +#
> > +#  Component description file for IFC Library
> > +#
> > +#  Copyright 2018 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
> > +#
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopenso
> urce.org%2Flicenses%2Fbsd-
> license.php&amp;data=02%7C01%7Cmeenakshi.aggarwal%40nxp.com%7C62c0
> 401c65ac481e650508d665b57ea9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7
> C0%7C0%7C636808227519182943&amp;sdata=4UMgN7laz86jDwTlvJrHWkqdu
> m8qmZPGWrhESvjHBMQ%3D&amp;reserved=0
> > +#
> > +#  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                      = IfcLib
> 
> Nxp
> 
> > +  FILE_GUID                      = a465d76c-0785-4ee7-bd72-767983d575a2
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = IfcLib
> 
> Nxp
> 
> /
>     Leif
> 
> > +
> > +[Sources.common]
> > +  IfcLib.c
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  Silicon/NXP/NxpQoriqLs.dec
> > +
> > +[LibraryClasses]
> > +  BoardLib
> > +  IoAccessLib
> > +
> > +[FixedPcd]
> > +  gNxpQoriqLsTokenSpaceGuid.PcdIfcBaseAddr
> > +  gNxpQoriqLsTokenSpaceGuid.PcdIfcBigEndian
> > +  gNxpQoriqLsTokenSpaceGuid.PcdIfcNandReservedSize
> > diff --git a/Silicon/NXP/NxpQoriqLs.dec b/Silicon/NXP/NxpQoriqLs.dec
> > index df64ad6..bd89da4 100644
> > --- a/Silicon/NXP/NxpQoriqLs.dec
> > +++ b/Silicon/NXP/NxpQoriqLs.dec
> > @@ -77,6 +77,7 @@
> >    gNxpQoriqLsTokenSpaceGuid.PcdCcsrBaseAddr|0x0|UINT64|0x00000128
> >    gNxpQoriqLsTokenSpaceGuid.PcdCcsrSize|0x0|UINT64|0x00000129
> >    gNxpQoriqLsTokenSpaceGuid.PcdDramMemSize|0x0|UINT64|0x0000012A
> > +  gNxpQoriqLsTokenSpaceGuid.PcdIfcBaseAddr|0x0|UINT64|0x0000012B
> >
> >    #
> >    # IFC PCDs
> > --
> > 1.9.1
> >