From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ang, Chee Hong Date: Wed, 14 Nov 2018 07:09:16 +0000 Subject: [U-Boot] [PATCH v2 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table In-Reply-To: <75db3a59-3ca8-708b-b6cf-3934a2e2e457@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> <1539054234.40335.11.camel@intel.com> <1539149457.40335.42.camel@intel.com> <1539238908.40335.46.camel@intel.com> <75db3a59-3ca8-708b-b6cf-3934a2e2e457@denx.de> Message-ID: <1542179355.62436.13.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 Thu, 2018-10-11 at 10:03 +0000, Marek Vasut wrote: > On 10/11/2018 08:21 AM, Ang, Chee Hong wrote: > > > > 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 > > > > > > > > > com> > > > > > > > > > > --- > > > > > > > > > >  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. > I was hoping to turn this into __weak function. Below are the new changes for new patch: Empty weak function in arch/arm/mach-socfpga/misc.c: /* add device descriptor to FPGA device table */ __weak void socfpga_fpga_add(void) { } In arch/arm/mach-socfpga/misc_aria10.c and arch/arm/mach- socfpga/misc_gen5.c: #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 In arch/arm/mach-socfpga/misc_s10.c: #ifdef CONFIG_FPGA /*  * 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 }, }; /* 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 With this new implementation, each platform overrides the 'socfpga_fpga_add' to add its own fpga device. The problem here is since our aria10 and gen5 are adding same fpga device, there will be duplication of code for these 2 platforms. What do you think ? If you are OK with this implementation, I can submit a new patch for review again. 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 > > > > > > > ... > > > > > 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. > Thanks > > --  > Best regards, > Marek Vasut