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

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

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.

>  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
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  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
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  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
> +  http://opensource.org/licenses/bsd-license.php
> +
> +  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
> +#  http://opensource.org/licenses/bsd-license.php
> +#
> +#  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
>