From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ang, Chee Hong Date: Mon, 19 Nov 2018 16:46:22 +0000 Subject: [U-Boot] [PATCH v4 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table In-Reply-To: <8a46b37d-4c97-9c0e-2cc3-882b35caa5a4@denx.de> References: <1542620622-130784-1-git-send-email-chee.hong.ang@intel.com> <1542620622-130784-4-git-send-email-chee.hong.ang@intel.com> <8a46b37d-4c97-9c0e-2cc3-882b35caa5a4@denx.de> Message-ID: <1542645980.62436.32.camel@intel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 wrote: > > > > > > > > > From: "Ang, Chee Hong" > > > > > > 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 > > > --- > > >  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 > > > + */ > > > + > > > +#include > > > +#include > > > + > > > +#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. >