On 11/15/2018 08:13 AM, Ang, Chee Hong wrote:
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@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@in >>>>>>> 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_<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.
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]); }
Better submit the whole patch, it's hard to review pieces.