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@intel.com wrote:
From: "Ang, Chee Hong" chee.hong.ang@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.ang@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 beignored */
-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 ^.
So I don't think I need to make any changes to socfpga_fpga_add() in misc.c. I just have to remove ifdef CONFIG_FPGA in misc_s10.c because it was unnecessary. I will submit v3 for this patch and you can comment further. The v3 patch will be simpler. Thanks.
Please don't submit stuff before the discussion concluded, it's pointless.
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 ^