From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ang, Chee Hong Date: Tue, 9 Oct 2018 03:03:55 +0000 Subject: [U-Boot] [PATCH v2 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table In-Reply-To: <784044b2-297b-2f5e-30bd-e6c606d6f28d@denx.de> References: <1538992107-37855-1-git-send-email-chee.hong.ang@intel.com> <1538992107-37855-4-git-send-email-chee.hong.ang@intel.com> <1539011430.38820.9.camel@intel.com> <784044b2-297b-2f5e-30bd-e6c606d6f28d@denx.de> Message-ID: <1539054234.40335.11.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-10-08 at 22:32 +0200, Marek Vasut wrote: > On 10/08/2018 05:10 PM, Ang, Chee Hong wrote: > > > > On Mon, 2018-10-08 at 11:57 +0200, Marek Vasut wrote: > > > > > > On 10/08/2018 11:48 AM, chee.hong.ang at intel.com 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/misc.c     | 29 > > > > +++++++++++++++++++++++++++++ > > > >  arch/arm/mach-socfpga/misc_s10.c |  2 ++ > > > >  drivers/fpga/altera.c            |  6 ++++++ > > > >  include/altera.h                 |  4 ++++ > > > >  4 files changed, 41 insertions(+) > > > > > > > > diff --git a/arch/arm/mach-socfpga/misc.c b/arch/arm/mach- > > > > socfpga/misc.c > > > > index a4f6d5c..7986b58 100644 > > > > --- a/arch/arm/mach-socfpga/misc.c > > > > +++ b/arch/arm/mach-socfpga/misc.c > > > > @@ -88,6 +88,27 @@ int overwrite_console(void) > > > >  #endif > > > >   > > > >  #ifdef CONFIG_FPGA > > > > +#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 > > > >   */ > > > > @@ -107,6 +128,7 @@ static Altera_desc altera_fpga[] = { > > > >   0 > > > >   }, > > > >  }; > > > > +#endif > > > >   > > > >  /* add device descriptor to FPGA device table */ > > > >  void socfpga_fpga_add(void) > > > > @@ -116,6 +138,13 @@ void socfpga_fpga_add(void) > > > >   for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) > > > >   fpga_add(fpga_altera, &altera_fpga[i]); > > > >  } > > > > + > > > > +#else > > > > + > > > > +__weak void socfpga_fpga_add(void) > > > > +{ > > > > +} > > > Why is a __weak function defined only in else-statement ? > > > > > > It should be defined always, with a sane default implementation. > > I will remove the empty function in #else-statement and define the > > default function like this : > > > > /* add device descriptor to FPGA device table */ > > void socfpga_fpga_add(void) > > { > > #ifdef CONFIG_FPGA > > int i; > > fpga_init(); > > for (i = 0; i < ARRAY_SIZE(altera_fpga); i++) > > fpga_add(fpga_altera, &altera_fpga[i]); > > #endif > > } > > > > Is that OK? > Can't you have __weak empty implementation of socfpga_fpga_add() and > implement a version per platform ? Would that work and make sense ? socfpga_fpga_add() as shown above is a generic function for adding FPGA devices to FPGA driver which applies to all our platforms. This is the reason why it is defined in misc.c instead of misc_.c. It turned out we already have this defined in misc.h: #ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif So I don't think I need to make any changes to socfpga_fpga_add() in misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c because it was unnecessary. I will submit v3 for this patch and you can comment further. The v3 patch will be simpler. Thanks. > > btw. the best solution would be to fix this proper and implement a > DM/DT > based probing of the FPGA, including a proper driver(s) in > drivers/fpga/ > instead of putting all the crud into arch/arm/mach-socfpga ... >