[U-Boot] [PATCH v4 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table

[Ang, Chee Hong <chee.hong.ang@intel.com>]

On Mon, 2018-11-19 at 14:12 +0100, Marek Vasut wrote:
> On 11/19/2018 10:57 AM, Simon Goldschmidt wrote:
> > 
> > On Mon, Nov 19, 2018 at 10:46 AM <chee.hong.ang@intel.com> wrote:
> > > 
> > > 
> > > From: "Ang, Chee Hong" <chee.hong.ang@intel.com>
> > > 
> > > Enable 'fpga' command in u-boot. User will be able to use the
> > > FPGA
> > > command to program the FPGA on Stratix10 SoC.
> > > 
> > > Signed-off-by: Ang, Chee Hong <chee.hong.ang@intel.com>
> > > ---
> > >  arch/arm/mach-socfpga/Makefile            |  4 +++
> > >  arch/arm/mach-socfpga/fpga_device.c       | 59
> > > +++++++++++++++++++++++++++++++
> > >  arch/arm/mach-socfpga/include/mach/misc.h |  4 ---
> > >  arch/arm/mach-socfpga/misc.c              | 31 +---------------
> > >  arch/arm/mach-socfpga/misc_s10.c          |  2 ++
> > >  drivers/fpga/altera.c                     |  6 ++++
> > >  include/altera.h                          |  4 +++
> > >  7 files changed, 76 insertions(+), 34 deletions(-)
> > >  create mode 100644 arch/arm/mach-socfpga/fpga_device.c
> > > 
> > > diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-
> > > socfpga/Makefile
> > > index e667204..2ff1b3f 100644
> > > --- a/arch/arm/mach-socfpga/Makefile
> > > +++ b/arch/arm/mach-socfpga/Makefile
> > > @@ -10,6 +10,10 @@ obj-y        += clock_manager.o
> > >  obj-y  += misc.o
> > >  obj-y  += reset_manager.o
> > > 
> > > +ifdef CONFIG_FPGA
> > > +obj-y  += fpga_device.o
> > > +endif
> > > +
> > >  ifdef CONFIG_TARGET_SOCFPGA_GEN5
> > >  obj-y  += clock_manager_gen5.o
> > >  obj-y  += misc_gen5.o
> > > diff --git a/arch/arm/mach-socfpga/fpga_device.c b/arch/arm/mach-
> > > socfpga/fpga_device.c
> > > new file mode 100644
> > > index 0000000..97b27eb
> > > --- /dev/null
> > > +++ b/arch/arm/mach-socfpga/fpga_device.c
> > > @@ -0,0 +1,59 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright (C) 2018 Intel Corporation <www.intel.com>
> > > + */
> > > +
> > > +#include <common.h>
> > > +#include <altera.h>
> > > +
> > > +#ifdef CONFIG_FPGA_STRATIX10
> > > +/*
> > > + * FPGA programming support for SoC FPGA Stratix 10
> > > + */
> > > +static Altera_desc altera_fpga[] = {
> > > +       {
> > > +               /* Family */
> > > +               Intel_FPGA_Stratix10,
> > > +               /* Interface type */
> > > +               secure_device_manager_mailbox,
> > > +               /* No limitation as additional data will be
> > > ignored */
> > > +               -1,
> > > +               /* No device function table */
> > > +               NULL,
> > > +               /* Base interface address specified in driver */
> > > +               NULL,
> > > +               /* No cookie implementation */
> > > +               0
> > > +       },
> > > +};
> > > +#else
> > > +/*
> > > + * FPGA programming support for SoC FPGA Cyclone V
> > > + */
> > > +static Altera_desc altera_fpga[] = {
> > > +       {
> > > +               /* Family */
> > > +               Altera_SoCFPGA,
> > > +               /* Interface type */
> > > +               fast_passive_parallel,
> > > +               /* No limitation as additional data will be
> > > ignored */
> > > +               -1,
> > > +               /* No device function table */
> > > +               NULL,
> > > +               /* Base interface address specified in driver */
> > > +               NULL,
> > > +               /* No cookie implementation */
> > > +               0
> > > +       },
> > > +};
> > > +#endif
> > > +
> > > +/* add device descriptor to FPGA device table */
> > > +void socfpga_fpga_add(void)
> > > +{
> > > +       int i;
> > > +
> > > +       fpga_init();
> > > +       for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > > +               fpga_add(fpga_altera, &altera_fpga[i]);
> > > +}
> > > diff --git a/arch/arm/mach-socfpga/include/mach/misc.h
> > > b/arch/arm/mach-socfpga/include/mach/misc.h
> > > index 4fc9570..6fa9495 100644
> > > --- a/arch/arm/mach-socfpga/include/mach/misc.h
> > > +++ b/arch/arm/mach-socfpga/include/mach/misc.h
> > > @@ -15,11 +15,7 @@ struct bsel {
> > > 
> > >  extern struct bsel bsel_str[];
> > > 
> > > -#ifdef CONFIG_FPGA
> > >  void socfpga_fpga_add(void);
> > > -#else
> > > -static inline void socfpga_fpga_add(void) {}
> > > -#endif
> > > 
> > >  #ifdef CONFIG_TARGET_SOCFPGA_GEN5
> > >  void socfpga_sdram_remap_zero(void);
> > > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach-
> > > socfpga/misc.c
> > > index a4f6d5c..55f846a 100644
> > > --- a/arch/arm/mach-socfpga/misc.c
> > > +++ b/arch/arm/mach-socfpga/misc.c
> > > @@ -87,36 +87,7 @@ int overwrite_console(void)
> > >  }
> > >  #endif
> > > 
> > > -#ifdef CONFIG_FPGA
> > > -/*
> > > - * FPGA programming support for SoC FPGA Cyclone V
> > > - */
> > > -static Altera_desc altera_fpga[] = {
> > > -       {
> > > -               /* Family */
> > > -               Altera_SoCFPGA,
> > > -               /* Interface type */
> > > -               fast_passive_parallel,
> > > -               /* No limitation as additional data will be
> > > ignored */
> > > -               -1,
> > > -               /* No device function table */
> > > -               NULL,
> > > -               /* Base interface address specified in driver */
> > > -               NULL,
> > > -               /* No cookie implementation */
> > > -               0
> > > -       },
> > > -};
> > > -
> > > -/* add device descriptor to FPGA device table */
> > > -void socfpga_fpga_add(void)
> > > -{
> > > -       int i;
> > > -       fpga_init();
> > > -       for (i = 0; i < ARRAY_SIZE(altera_fpga); i++)
> > > -               fpga_add(fpga_altera, &altera_fpga[i]);
> > > -}
> > > -#endif
> > > +__weak void socfpga_fpga_add(void) {}
> > I'm not sure I completely followed the discussion of previous
> > versions
> > of this series, but is this really the coding style U-Boot wants?
> > I would have thought weak functions are used for unknown extension
> > points (multiple architectures or boards), but this is a config
> > option
> > in a defined set of files. In my opinion, using weak here is not
> > the
> > right thing to do.
> > 
> > I'd rather add a header file fpga_device.h that declares this
> > function
> > depending on CONFIG_FPGA.
> My understanding is that the goal was to deduplicate the function for
> Gen5 somehow, but you're right, this looks odd.
> 
> This function should always be declared for SoCFPGA, except with
> different tables for different FPGAs.
This functions is general to all platforms except it is adding
different table for different platform. But this function is depending
on whether CONFIG_FPGA is defined or not. So if I declare this function
as empty function if CONFIG_FPGA is not defined then it would be
something similar to what I already addressed in previous v3 patchsets.
In v3 patchsets the tables and socfpga_fpga_add() function were already
declared in arch/arm/mach-socfpga/misc.c.
Can you guys take a look at v3 patchsets ?
https://lists.denx.de/pipermail/u-boot/2018-October/343561.html

Thanks.
>