From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ang, Chee Hong Date: Thu, 15 Nov 2018 07:13:00 +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> <1539238908.40335.46.camel@intel.com> <75db3a59-3ca8-708b-b6cf-3934a2e2e457@denx.de> <1542179355.62436.13.camel@intel.com> Message-ID: <1542265979.62436.20.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-11-14 at 12:52 +0100, Marek Vasut wrote: > On 11/14/2018 08:09 AM, Ang, Chee Hong wrote: > > > > 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 > > > > > > > > > > > tel. > > > > > > > > > > > > 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 ? > I think you can create a common code for Gen5 somehow, right ? I think I will add a new file arch/arm/mach-socfpga/fpga_devices.c and put the common code in it so that different platforms can share the common implementation which override the weak 'socfpga_fpga_add' function. The new file will have the following code: #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]); } > > > > > If you are OK with this implementation, I can submit a new patch > > for > > review again. Thanks. > Submit the patches, yes. >