From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marek Vasut Date: Thu, 11 Oct 2018 12:03:37 +0200 Subject: [U-Boot] [PATCH v2 3/4] arm: socfpga: stratix10: Add Stratix10 FPGA into FPGA device table In-Reply-To: <1539238908.40335.46.camel@intel.com> 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> Message-ID: <75db3a59-3ca8-708b-b6cf-3934a2e2e457@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 >>>>>>>>> --- >>>>>>>>>  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. >>>>>> 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