From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ang, Chee Hong Date: Thu, 11 Oct 2018 06:21:50 +0000 Subject: [U-Boot] [PATCH v2 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table In-Reply-To: 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> <1539054234.40335.11.camel@intel.com> <1539149457.40335.42.camel@intel.com> Message-ID: <1539238908.40335.46.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 Wed, 2018-10-10 at 12:27 +0200, Marek Vasut wrote: > On 10/10/2018 07:30 AM, Ang, Chee Hong wrote: > > > > On Tue, 2018-10-09 at 14:48 +0200, Marek Vasut wrote: > > > > > > On 10/09/2018 05:03 AM, Ang, Chee Hong wrote: > > > > > > > > > > > > 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 > > > Right, if you had one socfpga_fpga_add() per platform + generic > > > empty > > > one, you could drop that whole thing ^. > > Yes. It's being addressed in v3 patch: > > https://lists.denx.de/pipermail/u-boot/2018-October/343561.html > So where did the function go in there ? I don't see any __weak > anything. I don't have to add anything in my v3 patchsets to make this work. It's already taken care by arch/arm/mach-socfpga/include/mach/misc.h as shown below: #ifdef CONFIG_FPGA void socfpga_fpga_add(void); #else static inline void socfpga_fpga_add(void) {} #endif An empty default socfpga_fpga_add() will be defined if CONFIG_FPGA is not defined. > > > > > > > > > > > > > > > > > > > > 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 > > > > > ... > > > What do you think about this ^ > > > > > I do agree DM/DT is the proper way to implement driver. > > But right now those FPGA drivers in drivers/fpga need to be at > > least > > call fpga_add() to add themselves into FPGA device table so that > > their > > callback functions can be invoked correctly when user issue 'fpga > > load', 'fpga info' at the command prompt. > > So in other words, all drivers in drivers/fpga rely on > > drivers/fpga/fpga.c (FPGA core driver) to work. > Well, that should be fixed so that they probe from DT, just like any > other driver. I'm not fond of adding stuff to arch/arm/ ... > > > > > We already have all our fpga drivers in drivers/fpga : > > drivers/fpga/stratix10.c (NEW. In this patchset) > > drivers/fpga/stratixII.c (upstreamed) > > drivers/fpga/strixv.c (upstreamed) > > drivers/fpga/cyclon2.c (upstreamed) > > and others... > > > > We only define the FPGA device structure in arch/arm/mach- > > socfpga/misc.c and call fpga_add() to add our FPGA device driver > > into > > the global FPGA device table then FPGA core driver will handle the > > FPGA > > operations by invoking the FPGA driver's callback functions. > Right, which should be moved to drivers too and which should use DT. > > > > > So for proper DM/DT implementation, drivers/fpga/fpga.c need to be > > changed as well because this is the core of the FPGA driver.I think > > changing the core of the FPGA driver to support DM/DT would make > > more > > sense than I only change my FPGA driver to extract info from DTB > > file > > into a device structure then specifically call fpga_add() again to > > add > > the device structure to the FPGA core driver. > Yes, can you add it to your list once we flesh out this patchset ? > OK.