Re: [PATCH edk2-platforms 22/39] Platform/NXP: LS1046 RDB Board FPGA library

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

Hi Leif,

> -----Original Message-----
> From: Leif Lindholm [mailto:leif.lindholm@linaro.org]
> Sent: Thursday, April 19, 2018 8:14 PM
> To: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> Cc: ard.biesheuvel@linaro.org; edk2-devel@lists.01.org; Udit Kumar
> <udit.kumar@nxp.com>; Varun Sethi <V.Sethi@nxp.com>; Vabhav Sharma
> <vabhav.sharma@nxp.com>
> Subject: Re: [PATCH edk2-platforms 22/39] Platform/NXP: LS1046 RDB Board
> FPGA library
> 
> On Fri, Feb 16, 2018 at 02:20:18PM +0530, Meenakshi wrote:
> > From: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> >
> > Library to provide functions for accessing FPGA on LS1046ARDB board.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Vabhav <vabhav.sharma@nxp.com>
> > Signed-off-by: Meenakshi Aggarwal <meenakshi.aggarwal@nxp.com>
> 
> I compare this one to LS1043aRdbPkg/Library/FpgaLib, and the differences in
> the .c file are
> --- Platform/NXP/LS1043aRdbPkg/Library/FpgaLib/FpgaLib.c 2018-04-18
> 15:13:08.507949763 +0100
> +++ Platform/NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.c 2018-04-18
> +++ 15:13:08.531949605 +0100
> @@ -1,5 +1,5 @@
>  /** @FpgaLib.c
> -  Fpga Library for LS1043A-RDB board, containing functions to
> +  Fpga Library for LS1046A-RDB board, containing functions to
>    program and read the Fpga registers.
> 
>    FPGA is connected to IFC Controller and so MMIO APIs are used @@ -137,6
> +137,8 @@
>    Sd1RefClkSel = FPGA_READ(Sd1RefClkSel);
>    DEBUG((DEBUG_INFO, "SD1_CLK1 = %a, SD1_CLK2 = %a\n",
>                Sd1RefClkSel ? SERDES_FREQ2 : SERDES_FREQ1, SERDES_FREQ1));
> +  DEBUG((DEBUG_INFO, "SD2_CLK1 = %a, SD2_CLK2 = %a\n",
> +              SERDES_FREQ1, SERDES_FREQ1));
> 
>    return;
>  }
> 
> Could these two libraries be merged into a single LS104xx variant?
> 
> The LS2088a one seems to have substantial differences, so that makes sense to
> keep separate.
> 
We were planning to keep this library common for both LS1046 and LS1043,
But as this is board specific library, and in case, same soc be used on any other board,
then there will be changes in common library which might not be significant for other.

Also, FPGA_REG_SET structure is different for both, luckily here the new struct members
are added in the end of structure, but we cannot be sure about same in future boards.

So we decided to keep board specific stuff separate.

> No other comments on this patch.
> 
> /
>     Leif
> 
> > ---
> >  .../NXP/LS1046aRdbPkg/Include/Library/FpgaLib.h    |  97 ++++++++++++++
> >  .../NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.c    | 144
> +++++++++++++++++++++
> >  .../NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.inf  |  32 +++++
> >  3 files changed, 273 insertions(+)
> >  create mode 100644
> > Platform/NXP/LS1046aRdbPkg/Include/Library/FpgaLib.h
> >  create mode 100644
> > Platform/NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.c
> >  create mode 100644
> > Platform/NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.inf
> >
> > diff --git a/Platform/NXP/LS1046aRdbPkg/Include/Library/FpgaLib.h
> > b/Platform/NXP/LS1046aRdbPkg/Include/Library/FpgaLib.h
> > new file mode 100644
> > index 0000000..c8f7411
> > --- /dev/null
> > +++ b/Platform/NXP/LS1046aRdbPkg/Include/Library/FpgaLib.h
> > @@ -0,0 +1,97 @@
> > +/** FpgaLib.h
> > +*  Header defining the LS1046a Fpga specific constants (Base
> > +addresses, sizes, flags)
> > +*
> > +*  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%2Fope
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.agg
> >
> +arwal%40nxp.com%7C5d08c304527946a5bf8908d5a6040dc0%7C686ea1d3bc
> 2b4c6f
> >
> +a92cd99c5c301635%7C0%7C0%7C636597458708526882&sdata=%2FFRG6Itp%
> 2B23zi
> > +FZzr8QqfiGX7IImR8L6QjjTL2R%2Fgpw%3D&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 __LS1046A_FPGA_H__
> > +#define __LS1046A_FPGA_H__
> > +
> > +/**
> > +   FPGA register set of LS1046ARDB board-specific.
> > + **/
> > +typedef struct {
> > +  UINT8  FpgaVersionMajor; // 0x0 - FPGA Major Revision Register
> > +  UINT8  FpgaVersionMinor; // 0x1 - FPGA Minor Revision Register
> > +  UINT8  PcbaVersion;      // 0x2 - PCBA Revision Register
> > +  UINT8  SystemReset;      // 0x3 - system reset register
> > +  UINT8  SoftMuxOn;        // 0x4 - Switch Control Enable Register
> > +  UINT8  RcwSource1;       // 0x5 - Reset config word 1
> > +  UINT8  RcwSource2;       // 0x6 - Reset config word 1
> > +  UINT8  Vbank;            // 0x7 - Flash bank selection Control
> > +  UINT8  SysclkSelect;     // 0x8 - System clock selection Control
> > +  UINT8  UartSel;          // 0x9 - Uart selection Control
> > +  UINT8  Sd1RefClkSel;     // 0xA - Serdes1 reference clock selection Control
> > +  UINT8  TdmClkMuxSel;     // 0xB - TDM Clock Mux selection Control
> > +  UINT8  SdhcSpiCsSel;     // 0xC - SDHC/SPI Chip select selection Control
> > +  UINT8  StatusLed;        // 0xD - Status Led
> > +  UINT8  GlobalReset;      // 0xE - Global reset
> > +  UINT8  SdEmmc;           // 0xF - SD or EMMC Interface Control Regsiter
> > +  UINT8  VddEn;            // 0x10 - VDD Voltage Control Enable Register
> > +  UINT8  VddSel;           // 0x11 - VDD Voltage Control Register
> > +} FPGA_REG_SET;
> > +
> > +/**
> > +   Function to read FPGA register.
> > +**/
> > +UINT8
> > +FpgaRead (
> > +  UINTN  Reg
> > +  );
> > +
> > +/**
> > +   Function to write FPGA register.
> > +**/
> > +VOID
> > +FpgaWrite (
> > +  UINTN  Reg,
> > +  UINT8  Value
> > +  );
> > +
> > +/**
> > +   Function to read FPGA revision.
> > +**/
> > +VOID
> > +FpgaRevBit (
> > +  UINT8  *Value
> > +  );
> > +
> > +/**
> > +   Function to initialize FPGA timings.
> > +**/
> > +VOID
> > +FpgaInit (
> > +  VOID
> > +  );
> > +
> > +/**
> > +   Function to print board personality.
> > +**/
> > +VOID
> > +PrintBoardPersonality (
> > +  VOID
> > +  );
> > +
> > +#define FPGA_BASE_PHYS          0x7fb00000
> > +
> > +#define SRC_VBANK               0x25
> > +#define SRC_NAND                0x106
> > +#define SRC_QSPI                0x44
> > +#define SRC_SD                  0x40
> > +
> > +#define SERDES_FREQ1            "100.00 MHz"
> > +#define SERDES_FREQ2            "156.25 MHz"
> > +
> > +#define FPGA_READ(Reg)          FpgaRead (OFFSET_OF (FPGA_REG_SET, Reg))
> > +#define FPGA_WRITE(Reg, Value)  FpgaWrite (OFFSET_OF (FPGA_REG_SET,
> > +Reg), Value)
> > +
> > +#endif
> > diff --git a/Platform/NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.c
> > b/Platform/NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.c
> > new file mode 100644
> > index 0000000..90cc1ea
> > --- /dev/null
> > +++ b/Platform/NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.c
> > @@ -0,0 +1,144 @@
> > +/** @FpgaLib.c
> > +  Fpga Library for LS1046A-RDB board, containing functions to
> > +  program and read the Fpga registers.
> > +
> > +  FPGA is connected to IFC Controller and so MMIO APIs are used  to
> > + read/write FPGA registers
> > +
> > +  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&data=02%7C01%7Cmeenakshi.a
> > +
> ggarwal%40nxp.com%7C5d08c304527946a5bf8908d5a6040dc0%7C686ea1d3b
> c2b4
> > +
> c6fa92cd99c5c301635%7C0%7C0%7C636597458708526882&sdata=%2FFRG6It
> p%2B
> > + 23ziFZzr8QqfiGX7IImR8L6QjjTL2R%2Fgpw%3D&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 <Base.h>
> > +#include <Library/BaseLib.h>
> > +#include <Library/DebugLib.h>
> > +#include <Library/FpgaLib.h>
> > +#include <Library/IoLib.h>
> > +
> > +/**
> > +   Function to read FPGA register.
> > +
> > +   @param  Reg  Register offset of FPGA to read.
> > +
> > +**/
> > +UINT8
> > +FpgaRead (
> > +  IN  UINTN  Reg
> > +  )
> > +{
> > +  VOID       *Base;
> > +
> > +  Base = (VOID *)FPGA_BASE_PHYS;
> > +
> > +  return MmioRead8 ((UINTN)(Base + Reg)); }
> > +
> > +/**
> > +   Function to write FPGA register.
> > +
> > +   @param  Reg   Register offset of FPGA to write.
> > +   @param  Value Value to be written.
> > +
> > +**/
> > +VOID
> > +FpgaWrite (
> > +  IN  UINTN  Reg,
> > +  IN  UINT8  Value
> > +  )
> > +{
> > +  VOID       *Base;
> > +
> > +  Base = (VOID *)FPGA_BASE_PHYS;
> > +
> > +  MmioWrite8 ((UINTN)(Base + Reg), Value); }
> > +
> > +/**
> > +   Function to reverse the number.
> > +
> > +   @param  *Value  pointer to number to reverse.
> > +
> > +   @retval *Value  reversed value.
> > +
> > +**/
> > +VOID
> > +FpgaRevBit (
> > +  OUT UINT8  *Value
> > +  )
> > +{
> > +  UINT8      Rev;
> > +  UINT8      Val;
> > +  UINTN      Index;
> > +
> > +  Val = *Value;
> > +  Rev = Val & 1;
> > +  for (Index = 1; Index <= 7; Index++) {
> > +    Val >>= 1;
> > +    Rev <<= 1;
> > +    Rev |= Val & 1;
> > +  }
> > +
> > +  *Value = Rev;
> > +}
> > +
> > +/**
> > +   Function to print board personality.
> > +
> > +**/
> > +VOID
> > +PrintBoardPersonality (
> > +  VOID
> > +  )
> > +{
> > +  UINT8  RcwSrc1;
> > +  UINT8  RcwSrc2;
> > +  UINT32 RcwSrc;
> > +  UINT32 Sd1RefClkSel;
> > +
> > +  RcwSrc1 = FPGA_READ(RcwSource1);
> > +  RcwSrc2 = FPGA_READ(RcwSource2);
> > +  FpgaRevBit (&RcwSrc1);
> > +  RcwSrc = RcwSrc1;
> > +  RcwSrc = (RcwSrc << 1) | RcwSrc2;
> > +
> > +  switch (RcwSrc) {
> > +    case SRC_VBANK:
> > +      DEBUG ((DEBUG_INFO, "vBank: %d\n", FPGA_READ(Vbank)));
> > +      break;
> > +    case SRC_NAND:
> > +      DEBUG ((DEBUG_INFO, "NAND\n"));
> > +      break;
> > +    case SRC_QSPI:
> > +      DEBUG ((DEBUG_INFO, "QSPI vBank %d\n", FPGA_READ(Vbank)));
> > +      break;
> > +    case SRC_SD:
> > +      DEBUG ((DEBUG_INFO, "SD\n"));
> > +      break;
> > +    default:
> > +      DEBUG ((DEBUG_INFO, "Invalid setting of SW5\n"));
> > +      break;
> > +  }
> > +
> > +  DEBUG ((DEBUG_INFO, "FPGA:  V%x.%x\nPCBA:  V%x.0\n",
> > +              FPGA_READ(FpgaVersionMajor),
> > +              FPGA_READ(FpgaVersionMinor),
> > +              FPGA_READ(PcbaVersion)));
> > +
> > +  DEBUG ((DEBUG_INFO, "SERDES Reference Clocks:\n"));
> > +
> > +  Sd1RefClkSel = FPGA_READ(Sd1RefClkSel);  DEBUG((DEBUG_INFO,
> > + "SD1_CLK1 = %a, SD1_CLK2 = %a\n",
> > +              Sd1RefClkSel ? SERDES_FREQ2 : SERDES_FREQ1,
> > + SERDES_FREQ1));  DEBUG((DEBUG_INFO, "SD2_CLK1 = %a, SD2_CLK2 =
> %a\n",
> > +              SERDES_FREQ1, SERDES_FREQ1));
> > +
> > +  return;
> > +}
> > diff --git a/Platform/NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.inf
> > b/Platform/NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.inf
> > new file mode 100644
> > index 0000000..afc41e3
> > --- /dev/null
> > +++ b/Platform/NXP/LS1046aRdbPkg/Library/FpgaLib/FpgaLib.inf
> > @@ -0,0 +1,32 @@
> > +#  @FpgaLib.inf
> > +#
> > +#  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%2Fope
> > +nsource.org%2Flicenses%2Fbsd-
> license.php&data=02%7C01%7Cmeenakshi.agg
> >
> +arwal%40nxp.com%7C5d08c304527946a5bf8908d5a6040dc0%7C686ea1d3bc
> 2b4c6f
> >
> +a92cd99c5c301635%7C0%7C0%7C636597458708526882&sdata=%2FFRG6Itp%
> 2B23zi
> > +FZzr8QqfiGX7IImR8L6QjjTL2R%2Fgpw%3D&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                    = 0x0001000A
> > +  BASE_NAME                      = FpgaLib
> > +  FILE_GUID                      = 6e06ebbf-3a1d-47be-b4f6-1d82f2a9ac73
> > +  MODULE_TYPE                    = BASE
> > +  VERSION_STRING                 = 1.0
> > +  LIBRARY_CLASS                  = FpgaLib
> > +
> > +[Sources.common]
> > +  FpgaLib.c
> > +
> > +[Packages]
> > +  MdePkg/MdePkg.dec
> > +  Platform/NXP/LS1046aRdbPkg/LS1046aRdbPkg.dec
> > +  Silicon/NXP/NxpQoriqLs.dec
> > +
> > +[LibraryClasses]
> > +  BaseLib
> > +  IoLib
> > --
> > 1.9.1
> >