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

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

On Mon, Jun 04, 2018 at 04:10:16AM +0000, Meenakshi Aggarwal wrote:
> 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,

Excellent. thanks.

> 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.

Mainly what I'm after is reducing duplication of code and definitions.
If that can be resolved by board-specific include files including
common headers or a "common bits" library pulled in by board-specific
libraries, that sounds good to me.

Thanks,

Leif