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....@intel.com wrote: >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> From: "Ang, Chee Hong" <chee.hong....@intel.com> >>>>>>>>> >>>>>>>>> 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 <chee.hong....@intel.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_<platform_name>.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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot